[OSy] Fwd: Re: UPDATE: Immo Hellwig Ex.Ass.4

[Petr Tůma]

Forwarding in case it helps other people ... PT


-------- Forwarded Message --------
Subject: Re: UPDATE: Immo Hellwig Ex.Ass.4
Date: Wed, 12 Dec 2018 10:02:16 +0100
From: Petr Tůma <petr.tuma at d3s.mff.cuni.cz>
To: Immo <immo.hellwig at stud.uni-goettingen.de>

Hi Immo,

> My last last mail nearly got obsolete, so here an update:

OK :) By the way, would you mind if I forwarded our conversation to the course mailing list ? I'm sure some of 
the points would help some other students too ...

> 1. What function can I use to "make sure the rest of the function is executing on a single cpu"? Also my 
> solution for stdio,kernel-syscall and kbd is really laggy. Is it reasonable to see this as the reason?

Usually, the comment you refer to is a hint that you should disable interrupts at that point in your code. For 
example, in the getc function in the kernel keyboard driver, you access the shared variables of the keyboard 
ring buffer (kbd_buffer). If multiple threads were to call this function at the same time, you might see a race 
condition, and disabling interrupts is a fairly simple way to solve the issue on a single CPU machine.

About the lag. Looking at your keyboard driver code, I see some things I would perhaps do differently. Some of 
them might contribute to the overall behavior:

- The kbd_handle function is an interrupt handler, called by the interrupt handling function in 
kernel/exc/int.c whenever a key is pressed. You should not need to call the function from other places, in 
particular it is strange that you would call it from getc.

- Still about the kbd_handle function. When it is used correctly (that is, only called when a key is pressed), 
you do not need to have a while loop in there waiting for kbd_getchar to return a non zero value. In fact, any 
kind of potentially unbounded loop in an interrupt handler is wrong, you need to be able to guarantee that the 
handler will return fairly quickly.

- Taken together, the two points above might explain some of the lag. Imagine your userspace program calls 
getc, which eventually lands in the keyboard driver getc. If no key is pressed, the calling thread goes to 
sleep. After key press, the kbd_handler function is called in response to the interrupt generated by the 
keyboard hardware. It will read the key, put it into the buffer, and wake the thread waiting for the key. In 
your implementation, this thread immediately calls the kbd_handler function again, but the keyboard hardware 
has already informed your code about that one key press, so it will report no key pressed, and the kbd_handler 
function will keep waiting for another key in the while loop. After that key is pressed, the function will 
return to getc, which will report the first key of the pair, creating an impression of a lag.

> 2. In the sys_exit function we are demanded to terminate the main, which can be found in the process-structure 
> as "process->main_uthread", id use thread_destroy, but that only works for thread_t not for uthread. On top of 
> that uthread is never really defined. In thread.h it is defined using forward declaration. Can I assume based 
> on this, that I can just cast it?

I think the assignment text is poorly worded on this point, sorry about this. For exit to make sense, it should 
terminate all threads of the calling process. If you look into the sys_thread_create function, you will see 
that it maintains a list of userspace threads in the process structure (uthread_list). Exit should therefore 
traverse this list and destroy the listed threads.

To destroy a userspace thread when you have an uthread reference, you should access its thread field to get the 
thread reference for thread_destroy, and then remove the uthread from the list and free the memory. See 
kernel/proc/sys_thread.h for the definition of the uthread structure, and, for example, the end of the 
process_create function in kernel/proc/process.c for code that releases allocated uthread (in the branch that 
reacts to main thread creation failure).

> 3. At the moment i'm just vma_map in sys_vma_map, same for vma_map and sys_vma_map. Do I have to check and 
> return if there is any memory left?

I think your code already does the check - vma_map will return an error code (ENOMEM) if it cannot allocate the 
requested block,  and your sys_vma_map returns that to the userspace application, which is I think fine (there 
is little else you could do in that situation anyway, it is the userspace application that has to handle the 
error).

Strictly speaking, you should test the pointer passed to vma_map to make sure it is valid, otherwise your 
userspace application can overwrite kernel memory by calling vma_map and passing it a pointer to that memory as 
argument. Check out the vma_check_user function in kernel/mm/vmm.c.

> 4. At the moment I copied quite a lot from kernel-side malloc.c into user-malloc.c (attachment). i don't feel 
> really confident with that solution an it also doesnt seem right, because for some reason i cant call 
> malloc_heap(size), within the malloc function. Also I'm worried, that within the malloc_heap I manipulate 
> physical memory, what doesnt seem right in userspace. I assumed that in userspace the equivalent of the 
> frame_allocator are the vma_syscalls. Is that correct?

Let me start from the end of your question. Yes, your userspace implementation will need to replace the 
frame_allocator calls by calls to vma_map and vma_unmap. Since these already return virtual addresses, you do 
not have to care about that ADDR_IN_KSEG0 (phys) stuff, you can simply use the addresses returned by the vma 
syscalls directly.

Do not worry about copying the kernel malloc code into userspace, in fact this is an acceptable solution. 
Briefly browsing through the kernel malloc, I think these are the things that might need handling:

- Frame size, just define it as 4096 (that is, FRAME_WIDTH as 12, and so on) to avoid pulling the frame 
allocator header file into userspace.

- Same goes for some other macros, such as ALIGN_UP from kernel/include/c.h, just copy those as needed.

- Calls to frame_alloc and frame_free should be replaced by vma_map and vma_unmap, this removes the need for 
using ADDR_FROM_KSEG0 and ADDR_IN_KSEG0.

- The lists are in a header that can probably be copied without modifications.

And I do not see much else.

> 5. I implemented printf on my own, even tho i could have used the predefined solution by passing the arguments 
> to the kernel, because I didnt see that exists. Do have to change that?

Doing printf on your own is actually better I think (passing the pointers to the kernel needs a bit of extra 
work anyway, see the note on pointer checking above, so even in kernel you would need to have a separate printf 
implementation for calls from userspace).

Regards, Petr