Application problem

[Combuster]

hint: numerically lower privilege actually means higher privilege.

[Nessphoro]

Okay guys, you're all talking obvious stuff

[Gigasoft]

Actually, hold on.

So I would load the current task's ESP into ESP0? Shouldn't the kernel's stack go there?

Yes, the bottom of the current task's kernel mode stack. If all tasks share the same kernel mode stack, which I highly disrecommend, then it only needs to be loaded once. This makes things unnecessarily complicated though, so don't.

[Nessphoro]

Okay that solved part of the problem,
now reg dump at least shows the right values - but I get triple fault soon after that though, and ASM's CALL still doesn't work

And another slight problem, on task switch all register data will get corrupted since it was pushed on stack - and on a switch I will load another stack in, in which case it yields unpredictable results

[Gigasoft]

Well, let's see your task switching function.

[Nessphoro]

Code: Select all

void Switch()
{
    if (!task)
    {
        return;
    }
    /*Pushed by ISR handler*/
    registers* regs= get_curent_registers();
    if(!task->FirstRun)
    {
        mem_copy((void*)&task->regs,regs,sizeof(registers));
    }
    else
    {
        task->FirstRun=false;
    }
    task=task->Next;
    if(task->Next->Exit)
    {
        Task* temp=task->Next;
        task->Next=task->Next->Next;
        task=task->Next;
        CleanUpTask(temp);
    }
    while(task->Sleeping)
    {
        task=task->Next;
    }
    mem_copy(regs,(void*)&task->regs,sizeof(registers));
    if(task->FirstRun)
    {
        task->FirstRun=false;
    }
    set_kernel_stack(task->regs.esp); //Your advise 
    switch_page_directory(task->Directory);
}

[Gigasoft]

You are loading the new task's user mode registers into the old task's stack. Either you have a single stack or you have one stack per task, you have to think about which you are doing before you implement it! If you switch between the two, you have to rewrite the entire kernel. Some microkernels use a single stack (such as Pistachio and the PlayStation BIOS), others typically don't. The disadvantage is that the kernel must perform all operations asynchronously.

[Nessphoro]

Hmm,I don't even touch the stack of old task.

But nevertheless, any suggestions on implementing it the "right" way?

[Gigasoft]

Hmm,I don't even touch the stack of old task.

Sure you do.

Code: Select all

registers* regs= get_curent_registers(); // Trap frame in old task's stack
...
mem_copy(regs,(void*)&task->regs,sizeof(registers)); // Copy new task's registers into old trap frame

The way you were doing it before, with only one kernel stack, can also be called a "right way", there are a few designs that are like this, but it does make things tricky since every piece of operating system code must perform all lengthy operations asynchronously.

The way preferred by most is to include operating system code in the multitasking mechanism, not just user mode code. Then, you do not modify user mode trap frames at all. All you basically do is save the stack pointer, get the saved stack pointer from the new task, update the TSS so that ESP0 points to the current thread's kernel stack end, and update CR3 if needed. Needless to say, the function that performs the switch can't be written in C. Remember to save and restore EBX, ESI, EDI and EBP as well. System services can then block as much as they need, and perform lengthy operations without having to constantly worry about finishing using "the" stack so that another thread can run.

[Nessphoro]

But hold on, get_current_registers just gives a pointer to where ISR pushed the registers, I modify them and they get popped at IRET (of course with POPA and some extra things), no?

I might be mistaken by how the whole idea works.

[Combuster]

If you use a single stack in kernel land, modifying the stack's register image is the right way. However, the entire kernel stack frame must be identical at each call to Switch(), i.e. you can only call it from one location, which can only be called from one location etc until you hit a stack switch: an interrupt. In other words, such a scheduler can only be called from one and the same timer interrupt and nowhere else (like from within Sleep() or BlockThread()) If you do try, everything the kernel put on the stack will get overwritten because the other processes are unable to see that that part of memory was used by another.

Multiple kernel stacks work differently: it writes whatever needs to be stored to either the stack or some other per-thread storage, then it backs up the current value of ESP. After that ESP can be rewritten with the ESP of another thread, and other task-related registers should be set as well: TSS->ESP0, task pointers, CR3 and CR0.TS (and other special registers depending on design) which unlike the kernel ESP does not change. That has the advantage that what each system call does on the stack gets preserved over task switches, which also means that you can switch tasks from practically everywhere.

Mixing both without knowing what they are meant to do is bound to give issues. Right now, changing ESP0 does not practically change the single kernel-stack model as one task's stack still gets overwritten with the data of another.

Also, your implementation has severe algorithmic flaws that might end up starting unrunnable tasks. (EDIT: I even spotted a possible denial-of-service by starving certain tasks)

[Gigasoft]

If you use a single stack in kernel land, modifying the stack's register image is the right way. However, the entire kernel stack frame must be identical at each call to Switch(), i.e. you can only call it from one location, which can only be called from one location etc until you hit a stack switch: an interrupt. In other words, such a scheduler can only be called from one and the same timer interrupt and nowhere else (like from within Sleep() or BlockThread()) If you do try, everything the kernel put on the stack will get overwritten because the other processes are unable to see that that part of memory was used by another.

No, he would be modifying the ISR's stack frame, not the stack frame of Switch (or so I hope). It does not matter how Switch was called, because all functions will return anyway, ending up in the ISR returning to the new user mode task. In this case, the kernel itself is entirely singlethreaded. (I assume that the ISR's stack frame is at a fixed location, or that the location is saved by the ISR.)

[Combuster]

The point is that you delete all temporary kernel state associated with that thread. Consider:

Thread A calls read
a syscall is made
read needs to wait for disk I/O
Thread A is marked as blocked
A task switch is made.
The kernel stack is unwound because there's nothing read() can do (and if it could, the memory of process A was inaccessible)
The kernel returns to process B
(...)
Disk interrupt occurs and A is unblocked.
(...)
A switch to thread A occurs.
Control is returned to userspace. There is no kernel stackframe regarding to thread A (it has been overwritten), and therefore only the userland state can be resumed.
A continues executing after the syscall without read ever having been completed.
Profit.

Of course, there are ways around this, but they are functionally equivalent to storing a kernel-level continuation per thread, i.e. the same functionality as having a dedicated kernel stack per thread has.

[Nessphoro]

Also, your implementation has severe algorithmic flaws that might end up starting unrunnable tasks. (EDIT: I even spotted a possible denial-of-service by starving certain tasks)

It uses round robin scheduling, it switches every 20 ms - so I do not see how one can starve a process.

But I think I got the point, and I must re-do most of my tasking

[Combuster]

It uses round robin scheduling, it switches every 20 ms - so I do not see how one can starve a process.

I mentioned your implementation for a reason: check your code again.

Assume task A is running, and task B, C, D etc. are queued for execution in that order.
Bug 1: Task A kills task B. Expected result: B gets cleaned up, C gets executed. Actual result: B gets scheduled anyway, the kill is incorrectly postponed.
Bug 2: Task A kills task C. Expected result: C gets cleaned up, B is executed. Actual result: B is denied processor time, C gets cleaned up, D gets executed next.
Bug 3..8: Hopefully you get the point by now.

Even if you do plan on rewriting your code, it is a good exercise to locate the source of the bug in your current version so that you won't make it again in your next implementation.

But I think I got the point, and I must re-do most of my tasking

Actually, the point was that you should know what you are doing, and not blindly believe what we are saying (or overestimating your own skills for that matter)