Moved uspace field from thread_t to proc_t

[rafalcieslak]

Moving td_uspace to p_uspace turned out to be more complicated than I expected it to be. This is because we've been using the td_uspace field in two ways:

1. To store the userspace map of a particular user-mode thread
2. To keep a temporary userspace map that is different from 1. when we need to do some operations on it and need to be sure it will be re-activated when the current thread continues after preemption.

If we want the uspace to be a property of a process and shared between multiple threads, than we can no longer use it for 2, it wouldn't make any sense. Therefore I had to significantly modify the semantics of vm_map_activate. Until now, it would activate a vm_map and assign it to the current thread. As threads no longer have each their own vm_map reference, the former is no longer possible. So now all vm_map_activate does is activating the vm_map for use. This, in turn, means that we need to make sure to disable preemption while we're using an active vm_map that is not native to the current thread/process. Therefore I've been forced to add extra critical_sections to vm_map_clone and do_exec, as both these temporarily activate a different userspace vm_map in order to prepare it.

I've also had to completely disable the sched test - as threads no longer carry a uspace, the test is pointless. It was already marked as KTEST_FLAG_BROKEN, so it's not like we've been using it a lot.

[cahirwpz]

There's better place to keep it – please move it to pcpu structure.

[rafalcieslak]

I'm not sure that works at all? Instead of reading which vm_map this thread process uses, you just activate the one that is currently active. That doesn't change the current map at all, right? Or do I miss something?

[cahirwpz]

Oops... you're right. It's a shame I introduced a bug with this change -_-

[cahirwpz]

As you know critical_* mechanism is not a generic way to synchronize threads. It should be limited to the implementation of basic synchronization primitives and some parts of scheduler. Every time we use critical_* outside of this scope shows a design flaw in our kernel, and wrapping significant parts of do_exec and vm_map_clone makes the flaw apparent to full extent.

We have to find an alternative way to synchronize the parts of kernel that needlessly use critical_* primitive. Firstly we should identify minimal guarantees that are needed to perform the task and then choose right solution. Perhaps it's time to revisit #242. I'll reread spl(9) and kpreempt(9). Your suggestions are welcome!

[rafalcieslak]

Yes, I've only used critical_* due to the lack of any better alternative. But I suppose we don't necessarily need better synchronization features for this particular PR.

First thing is that I wouldn't worry much about vm_map_clone - the entire function is a temporary hack which we hope to remove soon once we have better memory management features available.

do_exec is different - we care about its implementation. The nature of the exec system call is such that on successful exec, all other threads of the calling process are terminated (and it's probably rare for an exec to get called by a multi-threaded context). We could hypothetically terminate all other threads before preparing the new vm_map. That would be convenient, once other threads are terminated, we can safely assign the vmap to p_uspace, and only then prepare the vmap - this way we'll be safe if we get preempted, as no other threads would share the vmap yet. However, if exec happens to fail, all other threads must stay alive (or do they? I can't find specific info on that anywhere, but it sounds logical not to terminate them if exec would not succeed.) - so before we terminate them we need to be sure that no further errors may happen. So for this to work, we would need to validate the entire ELF file first, then kill other threads, then assign a new vmap, then setup the vmap according to the ELF file. If that sounds okay to you, I can try re-implement exec to use this two-stage setup, and we'll never need a critical section. OTOH, if we want the other threads alive while we prepare new process memory map, then the minimal guarantee we need for this to work is that no other threads of the same process can continue while we're setting up vmap.

Side note: I haven't thought about it in greater detail, but what if a thread that activates a particular uspace would lock a recursive mutex on that vm_map structure? The lock would be released/acquired in the vm_map_activate function. When editing a vm_map, lock the mutex recursively once more. This way no other threads will be able to activate a vm_map while it's being edited by another thread, and no thread will be able to edit a vm_map while it's in use by another thread (e.g. on another processor). It sounds almost to simple to be true...?

[cahirwpz]

Multithreading doesn't play well with unix process model. Handling properly fork in multithreaded application is problematic. POSIX.1 specification for exec* doesn't say exactly what to do when exec call fails:

A call to any exec function from a process with more than one thread shall result in all threads being terminated and the new executable image being loaded and executed.

A programmer that calls fork or exec in multithreaded application does not understand consequences of running multiple threads very well. IMO in most cases introduction of threads is driven by bad design decisions.

Let's ignore weird corner cases and support the most common scenario well , which is fork + exec. When exec fails a programmer is supposed to terminate the process.

[rafalcieslak]

I've managed to confirm that Linux destroys all threads on a call to execve, regardless whether the call was successful or not - and that is also a good indication that it's okay if we do the same. I'll update this branch soon.

[cahirwpz]

Is really critical_enter() required here? Can we make weaker assumptions?

Perhaps thread_self()->proc->p_nthreads == 1 assertion is just enough to perform this step correctly?

[rafalcieslak]

We'd need to check if there is only one thread in this process AND set proc->p_uspace = newmap for the duration of the copy process.

EDIT: Also, this would only work if clone is called by a user thread. Kernel threads have their userspace map reset to NULL on each context switch.

[cahirwpz]

Ok. So the minimum assumption we must take is that we enter vm_map_clone with:

• user-space thread (i.e. withp_uspace in place),
• the only thread in a process – thus we know that no other thread (within the same process) will be started in the meantime.

If that is correct, please drop critical section and enforce above assumptions with assert.