Timer (PIT) code failure

[Candamir]

Hi! I'm basically following the steps from bkerndev on BonaFide, but while running my own kernel, I found a runtime error and I don't know how to fix it. This is my timer file, as I said, it is from bkerndev, but I _do_ understand everything.

Code: Select all

#include "system.h"

/* This will keep track of how many ticks that the system
*  has been running for */
int timer_ticks = 0;

/** The timer frequency.
*/

int freq = 18; // default: 18.222 Hz

/** Increment our timer variable
*/

void timer_handler_standard(struct regs *r)
{
   timer_ticks = timer_ticks + 1;
}

/** Call timer_handler_standard
*/

void timer_handler(struct regs *r)
{
    timer_handler_standard(r);
}

/** Call timer_handler_standard and prints out a message if a 
*    second has passed.
*/

void timer_handler_message(struct regs *r)
{
   timer_handler_standard(r);
   /* Every freq clocks (approximately 1 second), we will
   *  display a message on the screen */
   if (timer_ticks % freq == 0)
   {
      puts("One second has passed.\n");
   }
}

/** Sets the frequency for the timer in Hz.
 */

void timer_phase(int hz)
{
   int divisor = 1193180 / hz;       /* Calculate our divisor */
   outportb(0x43, 0x36);             /* Set our command byte 0x36 */
   outportb(0x40, divisor & 0xFF);   /* Set low byte of divisor */
   outportb(0x40, divisor >> 8);     /* Set high byte of divisor */
   freq = hz;
}

/* This will continuously loop until the given time has
*  been reached.
*/
void timer_wait(int ticks)
{
    unsigned long eticks;

    eticks = timer_ticks + ticks;
    while(timer_ticks < eticks)
    {
        ;
    }
}

/* Sets up the system clock by setting the frequency to 1kHz */
void timer_install()
{
   timer_phase(1000);
   irq_install_handler(0, timer_handler);
}

void timer_install_message()
{
   timer_install();
   irq_install_handler(0, timer_handler_message);
}

I used timer_install_message() to prove that my timer did work in that aspect, it printed out the message every second, just as I expected. But when using the timer_wait function, the method doesn't ever return.

Does anyone know what's wrong here?

Cheers

[Brendan]

Hi,


I might be wrong, but...

Code: Select all

/* This will keep track of how many ticks that the system
*  has been running for */
int timer_ticks = 0;

Shouldn't this be:

Code: Select all

volatile int timer_ticks = 0;

Otherwise the compiler probably optimizes your "timer_wait()" function so that the "timer_ticks" variable is cached in a register (which isn't updated).


Cheers,

Brendan

[RetainSoftware]

Guess Brendan already nailed the problem but i'd like to comment that it's not smart to compare int with unsigned long as the unsigned long can hold larger values then an int.

Code: Select all

void timer_wait(int ticks)
{
    unsigned long eticks;

    eticks = timer_ticks + ticks;
    while(timer_ticks < eticks)
    {
        ;
    }
}

The chances of looping for ever here are slim but it's better to make sure It in general good practise to treat warnings as errors because in in most cases they are.

another hint: use asm{"hlt"} in the while loop to save power and increase lifetime of laptops (only a little ;D).

just my 2 cents.

Rene

[durand]

I think in the P4's and upwards, the asembler PAUSE instruction is a hint to the processor that the code is in a tight spin. So, it starts to shut down various parts of itself and regulate voltage in order to save energy & be more efficient, etc..

Maybe: asm ("pause; hlt");

[Rob]

Keep in mind that the 'pause' instruction is supported only on the newer processor, I think the ones that have the SSE2 instruction set. I could be wrong on that...

[durand]

Yeah, you are. The 'pause' instruction is backwards compatible with all x86 processors; it's just ignored on anything prior to P4. So it's supported by all of them. However, the P4 has the implementation to regulate voltage which makes it useful. So, it's safe to include in code for older processors.

[Rob]

Thanks Durand! Guess I'll have to update my list then

[Brendan]

Hi,

I think in the P4's and upwards, the asembler PAUSE instruction is a hint to the processor that the code is in a tight spin. So, it starts to shut down various parts of itself and regulate voltage in order to save energy & be more efficient, etc..

Maybe: asm ("pause; hlt");

Not quite...

Normal for a tight loop a CPU will use any resources it can to make that loop execute as fast as possible. This is a good thing.

With hyper-threading, the CPU's resources are shared between "logical CPU", so a tight loop on one logical CPU will effect the resources that can be used by another logical CPU (or effect the other logical CPUs performance). In general this is also a good thing, as it means more instructions executed per second on average (for e.g. the tight loop might run 30% faster while the other logical CPU runs 10% slower).

For a spin-loop with hyper-threading, you don't want the spinloop to use all the resources it can to make the spinloop execute as fast as possible because the spinloop is basically just waiting anyway. What you want is the opposite - for the spinloop to use as little as possible so that the other logical CPU can use as many CPU resources as possible.

To achieve this the CPU "de-pipelines" the PAUSE instruction. This is Intel's wording, and to be honest I'm not entirely sure what they mean by "de-pipelining". Anyway the Intel manuals also mention some stuff about severe performance penalties when exiting a spin-loop caused by a possible memory order violation, so rather than trying to figure it all out I'll just quote the manual (Section 7.7.2, "PAUSE Instruction"):

"The PAUSE instruction improves the performance of IA-32 processors supporting Hyper-Threading Technology when executing a spin-wait loop and other routines where one thread is accessing a shared lock or semaphore in a tight polling loop. When executing a spin-wait loop, the processor can suffer a sever performance penalty when exiting the loop because it detects a possible memory order violation and flushes the core processor's pipeline. The PAUSE instruction provides a hint to the processor that the code sequence is a spin-wait loop. The processor uses this hint to avoid the memory order violation and prevent the pipeline flush. In addition, the PAUSE instruction de-pipelines the spin-wait loop to prevent it from consuming execution resources excessively. (See section 7.7.6.1., "Use the PAUSE Instruction in Spin-Wait Loops", for more information about using the PAUSE instruction with IA-32 processors supporting Hyper-Threading Technology.)

Basically, there is no benefit from using PAUSE before a HLT, but there are benefits for using PAUSE in polling loops, which may include something like:

Code: Select all

    in al, dx
    test al, READY_FLAG
    jne .ready

.wait:
    pause
    in al, dx
    test al, READY_FLAG
    je .wait
.ready:

Even though using it in this way (for I/O) isn't mentioned by Intel.

Also, Intel manuals do say that the PAUSE instruction is backwards compatible with all IA-32 processors, and that on older CPUs it operates the same as a NOP instruction (it's instruction encoding is actually "REP NOP").

@Durand: The PAUSE instruction does not regulate the CPUs voltage on any CPU. It saves CPU power by making it do less work (but doesn't save as much power as HLT would, as HLT causes the CPU to do nothing at all for a while).


Cheers,

Brendan

[YeXo]

I followed the same tutorial and got the same problem. I compiled it with djgpp under windows with the example .bat file. The solutiont was to delte -O from the gcc command in the .bat file. -O tells the compiler to optimize the code and in this case the compiler deletes the empty loop in timer_wait();

Hope this works for you too.

[Pype.Clicker]

-O tells the compiler to optimize the code and in this case the compiler deletes the empty loop in timer_wait();

Turning off the optimization just helps you to realize that your problem is due to optimization, but that's not a practical solution. The volatile keyword is precisely present in C to instruct the CPU that "this variable can potentially be modified at any time by some 'deus-ex-machina', so everytime I reference it, you should issue code that retrieve the value from memory, not from some cached value in registers."

(yes, all that in eight letters )

So whenever you use some variable to communicate between an interrupt handler and code waiting for the interrupt, think different ... think volatile.

[Candamir]

Thank you all, it worked!

Code: Select all

/* This will keep track of how many ticks that the system
*  has been running for */
volatile unsigned long timer_ticks = 0;

That's it.

But I didn't quite understood the thing of saving computer lifetime, do I just call, __asm__("pause"); before the loop and that's it? Mustn't I reactivate the cpu afterwards?

[Brendan]

Hi,

But I didn't quite understood the thing of saving computer lifetime, do I just call, __asm__("pause"); before the loop and that's it? Mustn't I reactivate the cpu afterwards?

You could use PAUSE, like this:

Code: Select all

void timer_wait(int ticks)
{
    unsigned long eticks;

    eticks = timer_ticks + ticks;
    while(timer_ticks < eticks) {
        __volatile__ __asm__("pause");
    }
}

This will make some Intel CPUs use less CPU resources for the loop, which will save a little bit of electricity on those CPUs.

A better idea is to use HLT, like this:

Code: Select all

void timer_wait(int ticks)
{
    unsigned long eticks;

    eticks = timer_ticks + ticks;
    while(timer_ticks < eticks) {
        __volatile__ __asm__("hlt");
    }
}

This will make the CPU do nothing until an IRQ occurs, and will save a lot of electricity (rather than just a little). It also works on all CPUs (rather than just some).

The "__volatile__" keyword here will tell GCC that the assembly code has side effects and can't be optimized out (normally, IIRC, GCC will remove assembly code unless it returns values that are used later).

Obviously, HLT is much better than PAUSE here.

For both cases, you don't need to re-active the CPU afterwards.


The next step is to remove the polling loop entirely. Typically in a multi-tasking OS you want to run other tasks instead of polling. For example, if I do "timer_wait(ONE_HOUR)" then you don't want the CPU to spend an hour doing nothing.

The normal way this is done is for the timer to have a list of threads that it needs to wake up. When a task calls "timer_wait" the task (and the time when it needs to stop waiting) is put on this list, a "sleeping" flag is set for the task (to prevent the scheduler from giving it CPU time), and the kernel does a task switch.

Sooner or later a timer IRQ occurs and checks this list and finds that a task needs to wake up. When this happens it removes the task from the list, clears the task's "sleeping" flag, and may or may not do a task switch to the task (perhaps depending on whether the task that was woken up has higher priority than the currently running task).


Cheers,

Brendan

[Candamir]

Thank you very much. Just one detail: I think you have to call __asm__ __volatile__("hlt"); instead of the other way round.
So this is my code, finally:

Code: Select all

#include "system.h"

/* This will keep track of how many ticks that the system
*  has been running for */
volatile unsigned long timer_ticks = 0;

/** The timer frequency.
*/

int freq = 18; // default: 18.222 Hz

/** Increment our timer variable
*/

void timer_handler_standard(struct regs *r)
{
   timer_ticks = timer_ticks + 1;
}

void timer_handler(struct regs *r)
{
    timer_handler_standard(r);
}

void timer_handler_message(struct regs *r)
{
   timer_handler_standard(r);
   /* Every freq clocks (approximately 1 second), we will
   *  display a message on the screen */
   if (timer_ticks % freq == 0)
   {
      puts("One second has passed.\n");
   }
}

/** Sets the frequency for the timer in Hz.
 */

void timer_phase(int hz)
{
   int divisor = 1193180 / hz;       /* Calculate our divisor */
   outportb(0x43, 0x36);             /* Set our command byte 0x36 */
   outportb(0x40, divisor & 0xFF);   /* Set low byte of divisor */
   outportb(0x40, divisor >> 8);     /* Set high byte of divisor */
   freq = hz;
}

/* This will continuously loop until the given time has
*  been reached.
* Must implement better function (Tasklist, wakeup, etc.)
*/
void timer_wait(unsigned long ticks)
{
    unsigned long eticks;

    eticks = timer_ticks + ticks;
    while(timer_ticks < eticks)
    {
        __asm__ __volatile__("hlt");
    }
}

/* Sets up the system clock by setting the frequency to 1kHz */
void timer_install()
{
   timer_phase(1000);
   irq_install_handler(0, timer_handler);
}

/** Only for debugging. Prints out a message every second has passed.
*/

void timer_install_message()
{
   timer_install();
   irq_install_handler(0, timer_handler_message);
}

That's it, I think this thread is over. I've mailed to Brandon Friesen notifying him of this tiny bug. Thanks to all who've helped.

Candamir