[SOLVED]Multitasking Bugs

[lolxdfly]

I am trying to implement multitasking. However I have some bugs and strange behavior. After one week of bug-search and testing I decided to ask here.

I switch the cpu state at IR0 with pointer access.
You can find my code here: https://gitlab.com/lolxdfly/kernel

As you can see I have 4 tasks, which should print "A", "BB", "CCC", "DDDD" on the screen. However if I start the kernel I can only see Bs, Cs and Ds. There is no A, but my current_task switches like 0 1 2 3 0 1 2 ... . I found out that the first task which is registered does not get executed.

Another problem is the out of memory errors I get if the tasks are running for some minutes. But I think this is normal because I have not implemented Paging yet. I think this does need a fix yet... I just wanted to mention it.

The last thing is the strage behavior of the IDT in connection with multitasking. In idt.cpp you will find this line:

Code: Select all

idt_entries[num].type_attr = type_attr | 0x60;

The "| 0x60" should be useless since I have the "| IDT_PRIVILEGE_RING3" at the funtion call in IDT::init().
But without it my multitasking results in ISR01 if a task is registered. The type_attr has the same value(0xEE) with or without the 0x60. I checked this by printing it. This behavior is only connected to multitasking! The PIT and Keyboard Interrups work fine wihout the "| 0x60".

Especially the last thing confuses me. I hope that someone can help me

[simeonz]

I think that your first schedule_task is picking up the context of your idle thread and thus overwrites the starting context of task 0. You must provide a task structure for the idle task and increment num_tasks in TaskManager::init, or perform a vacuous register_task there. I haven't investigated the memory issue for the time being.

[lolxdfly]

Code: Select all

int TaskManager::current_task = -1;
int TaskManager::num_tasks = 0;

void TaskManager::schedule_task(registers_t &regs)
{
	if(num_tasks == 0)
		return;
	
	if (current_task >= 0) //current_task = -1 at first executiion
		tasks[current_task]->state = &regs; //this should be skipped at first execution
	
	current_task++;
	current_task %= num_tasks;

	screen << "curtask: " << current_task << endl;
	
	regs = *tasks[current_task]->state;
}

current_task is initialized with -1. So it should skip the line where it saves the register to tasks[current_task]->state.
Then current_task becomes 0 and the registers are set to tasks[current_task]->state. At the next Interrup regs should have the value of the processed task and it will be saved to the array again.

Am I wrong here? That would explain that the first task does not get executed.

[simeonz]

current_task is initialized with -1. So it should skip the line where it saves the register to tasks[current_task]->state.
Then current_task becomes 0 and the registers are set to tasks[current_task]->state. At the next Interrup regs should have the value of the processed task and it will be saved to the array again.

Am I wrong here? That would explain that the first task does not get executed.

No, you are not wrong. I didn't analyze the code enough. I noticed that your idle task was unaccounted for and wanted to fit this somehow with the issue.

Anyway, how about this then. We know that you are saving your registers on the stack, then passing them by reference. You take the address of the registers from that reference argument and save it in the state field of the task structure. So far so good. However, in order to switch to the next task, you overwrite the registers of the current task. This accomplishes the switch in your PIT handler, but now the register context of the old task is overwritten with the context of the task you just switched into. It would be best if you switch the stack first, and then perform popal (as you seem to have been meaning to, judging by the commented line in the handler.)

The "| 0x60" should be useless since I have the "| IDT_PRIVILEGE_RING3" at the funtion call in IDT::init().
But without it my multitasking results in ISR01 if a task is registered. The type_attr has the same value(0xEE) with or without the 0x60. I checked this by printing it. This behavior is only connected to multitasking! The PIT and Keyboard Interrups work fine wihout the "| 0x60".

That just makes no sense to me. It could have had something to do with integral promotions and such, but couldn't figure out anything wrong with the code. Can't you disassemble and see if you can figure out what setgate arguments are pushed on the stack?

By the way. I don't see anything stopping the PIT performing the first tick somewhere between the task registerations. Which given the fact that idle task seems to never be given back control would provoke sporadic loss of tasks B, C, or D early on. Just mentioning if you get that as well.

[lolxdfly]

Thank you for your answer.

Attempt 1:
I uncommend the "mov %eax, %esp" and commended "add $4, %esp" out. I also uncommended the return types at irq_handler(registers_t &regs) of course. So it will change the stack. This results in ISR1 if a task is registered.

Attempt 2:
I also tried to fix it the other way around and changed my schedule_task to:

Code: Select all

	
	if (current_task >= 0)
		*tasks[current_task]->state = regs;

I do not copy the reference of regs. I copy the value of regs instead. This causes ISR6 somehow.

Attempt 3:
Then I changed my irq_handler back to non-return type. With this code it works for some milliseconds. But then the screen turns in this: http://i.epvpimg.com/Contfab.png. Sometimes I can see an ISR6 error. QEMU freezes and shows this screen:

It says something about invalid tss type, but I do not use TSS at the moment.

I will take a look at the disassembled code tomorrow

I already thought about the problem of never coming back to the idle task. It's something I still have to solve but it should not affect the other bugs. By the way do I have to return to the idle task? I would just pass something like a new "main" task to register_task inside TaskManager::init() and put an infinite loop at the end of TaskManager::init().

[simeonz]

I honestly expect attempt 2 and 3 not to work, because the "state" field has to always point to the top of the stack. This is its conceptual designation. It is not storage set aside at the bottom of the stack as some OSes have/had, that you can assign into for transient task related information. What I expect to happen, if I understand correctly, is that your task functions are allocating stack frames over the starting context that was created in register_task, which is ok, since it is no longer relevant, but since the state field pointer is not updated, you will overwrite those frames eventually in schedule_task. Why is it working for a while, I cannot explain. I mean, I don't know how it can survive more then one turnaround of the tasks. I wouldn't worry about the "invalid TSS type" though. Probably qemu is trying too hard to emit TSS information, where there is currently none.

Anyway, what is more important is why attempt 1 is not working. Did you remove the last regs assignment in schedule_task? Could you post the schedule_task and irq handler code after the changes?

I also am not sure how you get ISR1 either. ISR6 is understandable, since it indicates something like ret in random memory. ISR1 on the other hand requires that the access breakpoints are enabled in dr7. So, may be they are and dr0 to dr3 are set to trap on null accesses?

P.S.
You are not really supposed to return control to the idle task, but since you had a loop there, I figured you probably had the intention to run it. Currently, all your tasks are indefinitely busy, so it is irrelevant. The idle task is only conceptual, in the sense that it is only connected to the idle priority class that marks some system tasks and allows them to run in the background when nothing else is running. Other than that, idle means that the cpu services interrupts, deferred context free work, and cross-CPU requests, or enters a low-power state.

Here are a couple of good threads from this forum:
How to implement System Idle process
Question about HLT

[lolxdfly]

I think I misunderstood you if schedule_task should also be changed. This is my code now:

Code: Select all

.extern irq_handler
irq_common_stub:
    pushal                  #Pushes eax, ecx, edx, ebx, esp, ebp, esi, edi

    cld                     #Make sure direction flag is forward. We don't know what state it is in when interrupt occurs
    push %esp               #Pass a reference of registers
    call irq_handler		#call c++ handler
	mov %eax, %esp			#change stack to restore regs (may useless since reg is pointer type)
	#add $4, %esp			#clean up reference

    popal                   #Pops edi, esi, ebp, esp, ebx, edx, ecx, eax
    add $8, %esp  			#Cleans up the pushed error code and pushed IRQ number
    iret           			#returns and enable ints

Code: Select all

extern "C" registers_t& irq_handler(registers_t &regs)
{
    //Send an EOI (end of interrupt) signal to the PICs.
    //If this interrupt involved the slave.
    if (regs.intr >= IRQ00 + 8)
    {
        //Send reset signal to slave.
        outb(PIC2_COMMAND, PIC_EOI);
    }
    //Send reset signal to master. (As well as slave, if necessary).
    outb(PIC1_COMMAND, PIC_EOI);

	//multitasking
	if(regs.intr == IRQ00)
		TaskManager::schedule_task(regs);
	
	if (IDT::interrupt_handlers[regs.intr] != nullptr)
    {
        isr_t handler = IDT::interrupt_handlers[regs.intr];
        handler(regs);
    }
	
	return regs;
}

Code: Select all

void TaskManager::schedule_task(registers_t &regs)
{
	if(num_tasks == 0)
		return;
	
	if (current_task >= 0)
		*tasks[current_task]->state = regs;
	
	current_task++;
	current_task %= num_tasks;

	screen << "curtask: " << current_task << endl;
	
	regs = *tasks[current_task]->state;
}

If I remove the last regs assignment at schedule_task it will not call the tasks. Or should I transfer the new cpu state via return types? Without removing the last last line of schedule_task I get ISR6 now. I am not sure how I got ISR1 at my first try.

[simeonz]

I will be try to make minimal changes. However, on a side note, saving and restoring the current context every time, in every interrupt, is probably not very efficient.

Edit: On a second thought, the pusha and popa are preserving the interrupted context as well and you are just reusing them. The thing is, on IA32, the cdecl convention makes only EAX, ECX, and EDX caller-saved, so the question is whether using a single push with more memory output than necessary is a good tradeoff. But, as I said, in retrospect, this is not so important. Unfortunately, neither of the gcc attributes "interrupt", "no_caller_saved_registers", or even "naked" work for IA32 on my tests. So, you cannot benefit from saving the full context by altering the abi locally for the handler.

But, here is my version:

Code: Select all

extern "C" registers_t& irq_handler(registers_t &regs)
{
    //Send an EOI (end of interrupt) signal to the PICs.
    //If this interrupt involved the slave.
    if (regs.intr >= IRQ00 + 8)
    {
        //Send reset signal to slave.
        outb(PIC2_COMMAND, PIC_EOI);
    }
    //Send reset signal to master. (As well as slave, if necessary).
    outb(PIC1_COMMAND, PIC_EOI);
	
	if (IDT::interrupt_handlers[regs.intr] != nullptr)
    {
        isr_t handler = IDT::interrupt_handlers[regs.intr];
        handler(regs);
    }
	
	//HANDLE SCHEDULING AFTER THE MAIN HANDLERS
	//multitasking
	if(regs.intr == IRQ00)
		return TaskManager::schedule_task(regs);

	return regs;
}

Code: Select all

//RETURN TYPE
void registers_t& TaskManager::schedule_task(registers_t &regs)
{
	if(num_tasks == 0)
		return regs;
	
	if (current_task >= 0)
		tasks[current_task]->state = &regs;
	
	current_task++;
	current_task %= num_tasks;

	screen << "curtask: " << current_task << endl;
	
	//RETURN NEW STATE
	return *tasks[current_task]->state;
}

Also, using reference return type is a little too volatile. C++'s semantics for reference types are too sophisticated, yet inflexible, and may bite you in the butt later on. But that is just stylistic issue.

[lolxdfly]

I think you ment

Code: Select all

registers_t& TaskManager::schedule_task(registers_t &regs)

at

Code: Select all

void registers_t& TaskManager::schedule_task(registers_t &regs)

I tried your code. But now I get the ISR1 error again. I pushed the code to git so you can look at the exact version I am using.

Thank you for the performance comments. I will take a look at performance if the multitasking works finally.

[simeonz]

I had nothing meaningful to say at this point, so I went as far as to debug the kernel for a while. But note that debugging is the better virtue of understanding (exactly the opposite philosophy of Linus Torvalds here.)

The problem turned out to be that the inline assembly in TaskManager::register_task messes up the flags. This function is using esp based local variable references in O2, which means that the assembly gets its output operand in form relative to the stack pointer, which it also modifies. As a result, the eflags is incorrect and gets TF set, which on the first context switch results in ISR1 on the leading task instruction. This does not happen on O0. The solution I used was to define the function as:

Code: Select all

int __attribute__((optimize("-fno-omit-frame-pointer"))) TaskManager::register_task(task_func_t task)

, which guarantees that rbp will be used to refer to local variables no matter what the optimization level.

For future reference, to debug the project, add "-g3" to all build commands in make.bat except the linker. Add the following at the end of the linker script (as per the default linker script):

Code: Select all

	end = .;
  
  /* Stabs debugging sections.  */
  .stab          0 : { *(.stab) }
  .stabstr       0 : { *(.stabstr) }
  .stab.excl     0 : { *(.stab.excl) }
  .stab.exclstr  0 : { *(.stab.exclstr) }
  .stab.index    0 : { *(.stab.index) }
  .stab.indexstr 0 : { *(.stab.indexstr) }
  .comment       0 : { *(.comment) }
  /* DWARF debug sections.
     Symbols in the DWARF debugging sections are relative to the beginning
     of the section so we begin them at 0.  */
  /* DWARF 1 */
  .debug          0 : { *(.debug) }
  .line           0 : { *(.line) }
  /* GNU DWARF 1 extensions */
  .debug_srcinfo  0 : { *(.debug_srcinfo) }
  .debug_sfnames  0 : { *(.debug_sfnames) }
  /* DWARF 1.1 and DWARF 2 */
  .debug_aranges  0 : { *(.debug_aranges) }
  .debug_pubnames 0 : { *(.debug_pubnames) }
  /* DWARF 2 */
  .debug_info     0 : { *(.debug_info .gnu.linkonce.wi.*) }
  .debug_abbrev   0 : { *(.debug_abbrev) }
  .debug_line     0 : { *(.debug_line .debug_line.* .debug_line_end ) }
  .debug_frame    0 : { *(.debug_frame) }
  .debug_str      0 : { *(.debug_str) }
  .debug_loc      0 : { *(.debug_loc) }
  .debug_macinfo  0 : { *(.debug_macinfo) }
  /* SGI/MIPS DWARF 2 extensions */
  .debug_weaknames 0 : { *(.debug_weaknames) }
  .debug_funcnames 0 : { *(.debug_funcnames) }
  .debug_typenames 0 : { *(.debug_typenames) }
  .debug_varnames  0 : { *(.debug_varnames) }
  /* DWARF 3 */
  .debug_pubtypes 0 : { *(.debug_pubtypes) }
  .debug_ranges   0 : { *(.debug_ranges) }
  /* DWARF Extension.  */
  .debug_macro    0 : { *(.debug_macro) }
  .debug_addr     0 : { *(.debug_addr) }
}

After the linker command, run the following:

Code: Select all

i686-elf-objcopy --only-keep-debug ../kernel.bin ../kernel.sym
i686-elf-objcopy --strip-debug ../kernel.bin

And start the debugger as such:

Code: Select all

start i686-elf-tools-windows\bin\i686-elf-gdb.exe -s kernel.sym -ex "target remote | ./qemu/qemu-system-i386 -S -gdb stdio -kernel kernel.bin"

Your gdb build has no tui mode or you would be able to watch the source in a separate pane with highlight on the current line. But you can still set breakpoints and step over, view the stack, etc. Also, I would compile with "-O0" or at least "-Og" for a while. (The bug manifested at "Og" as well.)

Use the monitor commands to view debug registers and state that gdb does not support natively. (For example, "monitor info registers".)

Edit: One last note. Use the type assembly directive to produce proper function symbols from gas files. As in ".type isr\id, @function"

[lolxdfly]

Many thanks to you. It is working now.
I would never have thought of that.
I will use the debug-features in the future.

[zesterer]

Many thanks to you. It is working now.

Don't forget to add "[SOLVED]" To the topic title so we all know

[Octocontrabass]

The problem turned out to be that the inline assembly in TaskManager::register_task messes up the flags. This function is using esp based local variable references in O2, which means that the assembly gets its output operand in form relative to the stack pointer, which it also modifies. As a result, the eflags is incorrect and gets TF set, which on the first context switch results in ISR1 on the leading task instruction. This does not happen on O0. The solution I used was to define the function as:

Code: Select all

int __attribute__((optimize("-fno-omit-frame-pointer"))) TaskManager::register_task(task_func_t task)

, which guarantees that rbp will be used to refer to local variables no matter what the optimization level.

This isn't a solution to the problem. Any future (or past) version of GCC is free to optimize the function differently, resulting in the bug resurfacing.

The real solution is to stop misusing inline assembly. There are no constraints that would make your inline assembly correct, so you should either move it to its own assembly file and call it separately, or rewrite it so that it doesn't violate any of GCC's assumptions.

In fact, it's not too hard to rewrite it to appease GCC:

Code: Select all

asm volatile("pushfl; popl %0;":"=rm"(reg.eflags));

A "memory" clobber* may be necessary, depending on which flags you want to retrieve.

But then, which flags do you want to retrieve? You're creating a new task, right? Why can't you set reg.eflags to some fixed value?

(*Edit: constraint -> clobber)

[simeonz]

The problem turned out to be that the inline assembly in TaskManager::register_task messes up the flags. This function is using esp based local variable references in O2, which means that the assembly gets its output operand in form relative to the stack pointer, which it also modifies. As a result, the eflags is incorrect and gets TF set, which on the first context switch results in ISR1 on the leading task instruction. This does not happen on O0. The solution I used was to define the function as:

Code: Select all

int __attribute__((optimize("-fno-omit-frame-pointer"))) TaskManager::register_task(task_func_t task)

, which guarantees that rbp will be used to refer to local variables no matter what the optimization level.

This isn't a solution to the problem. Any future (or past) version of GCC is free to optimize the function differently, resulting in the bug resurfacing.

What do you mean by "free to optimize"? If you mean that "__attribute__((optimize("-fno-omit-frame-pointer")))" is not ANSI C, then you are obviously correct. But neither is "stack pointer". You couldn't busy loop without going outside the canonical C.

The real solution is to stop misusing inline assembly. There are no constraints that would make your inline assembly correct, so you should either move it to its own assembly file and call it separately, or rewrite it so that it doesn't violate any of GCC's assumptions.

I agree that this is the more elegant solution. On the other hand, note that the entire context switch of the Linux kernel used to be written in inline assembly. And it worked for ages too.

In fact, it's not too hard to rewrite it to appease GCC:

Code: Select all

asm volatile("pushfl; popl %0;":"=rm"(reg.eflags));

A "memory" constraint may be necessary, depending on which flags you want to retrieve.

You relax the constraint and it happens to work. How could remark about behavior volatility and offer this suggestion? I agree that using the operand constraints is a better fix per se. "=r" would be a stricter constraint, and could/should work.

But then, which flags do you want to retrieve? You're creating a new task, right? Why can't you set reg.eflags to some fixed value?

I figure this is a reasonable remark. But that is again unrelated to the original issue. It is a good question though.

In the end, the OP could work out any number of alternative ways to remedy the problem, if he so chooses. In fact my fix was dirty. Even though I would use such attribute, I wouldn't use it in this situation as a long term solution. I described the issue and wanted to demonstrate the connection to the frame stability in an evident way. Also, while trying to make the code stable and future proof is always desirable, compiler agnosticism is difficult quality in low-level kernel code.

[Octocontrabass]

What do you mean by "free to optimize"? If you mean that "__attribute__((optimize("-fno-omit-frame-pointer")))" is not ANSI C, then you are obviously correct. But neither is "stack pointer". You couldn't busy loop without going outside the canonical C.

I mean that GCC is free to generate ESP-relative addresses regardless of optimization parameters, and there is no specific guarantee that -fno-omit-frame-pointer will prevent it from doing so.

I agree that this is the more elegant solution. On the other hand, note that the entire context switch of the Linux kernel used to be written in inline assembly. And it worked for ages too.

So long as all of GCC's assumptions about the inline context switch hold true, there is no problem.

You relax the constraint and it happens to work. How could remark about behavior volatility and offer this suggestion? I agree that using the operand constraints is a better fix per se. "=r" would be a stricter constraint, and could/should work.

Relaxing the constraint is an optimization. The bug fix is ensuring that ESP-relative addresses will be calculated correctly if GCC chooses to use them. Intel specifies that POP with a memory operand using ESP as a base register will increment ESP before calculating the effective address. Combined with the previous decrement from PUSH, the result is that an effective address with ESP will be calculated the way GCC expects.

Edit: I also now realize I said "constraint" when I meant "clobber" so that may have confused the meaning of my previous post.