Conversation
github-actions
bot
added
Platform: native
carl-tud
force-pushed
the
native-gardening
branch
3 times, most recently
from
73f407d to
765cbaf
Compare
carl-tud
force-pushed
the
native-gardening
branch
2 times, most recently
from
4b402b9 to
308a30a
Compare
mguetschow
added
the
CI: ready for build
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
Thanks a lot for your spring clean-up, and for splitting the commits for easier review!
This will need some more in-depth review, especially for the second half of the commits, preferably by someone more experienced with the native code. Just some brief notes from my side while having a first look.
|
CI fails because of said U: fixed |
Okay, funny. _GNU_SOURCE somehow makes some system header define @mguetschow what do you think? I'd say I rename it to |
carl-tud
force-pushed
the
native-gardening
branch
from
308a30a to
51512a3
Compare
carl-tud
force-pushed
the
native-gardening
branch
from
51512a3 to
c4f8cbf
Compare
Sounds good to me 👍 |
benpicco
left a comment
benpicco
left a comment
There was a problem hiding this comment.
Looks good to me, thank you for cleaning this up!
I still dream of a future where we get away from makecontext() and just use pthreads and signals, so no more assembly required - but that would be a major rework…
I also don't think this will fix the dreaded #495 yet, but it's a nice cleanup overall.
|
Thanks for the review. I would also like to get away from |
carl-tud
force-pushed
the
native-gardening
branch
from
c4f8cbf to
8275208
Compare
This can actually be done. Each thread has a mutex in thread-local storage that is locked and held by the thread. One signal handler locks that mutex (again, so being deadlocked). Another signal handler unlocks that mutex. The scheduler then needs to make sure that it dead-locks all the thread except of one. A context switch happens by un-deadlocking the target thread by sending it a signal, then deadlocking itself with locking the mutex in TLS. (Technically the un-deadlocked thread may already start to execute before the currently active thread goes into deadlock, but that should be good enough). I used a similar approach to implement the infrastructure for scheduling and handed that out as tasks to students. It worked quite good. |
|
wow this even fixed the preemption issue that #19002 uncovered - it's now passing ❤️ |
6 participants
Hi!
It’s spring, so I thought it’s time for some gardening in
native/cpu.I’ve got a few PRs lined up, including one enabling the native board on arm64 (#20739) and one resurrecting macOS support (TBA).
While writing these I’ve found myself unable to really understand what’s going on in the
native/cpucode. So I thought if I were to add documentation and name a few things more descriptively, this might help anyone reading it in the future.Below, I’ll give a brief overview of the motivation behind certain changes.
As a matter of ensuring resilience against future modifications to system headers, I set_GNU_SOURCE/_XOPEN_SOURCE_GNU_SOURCE(or_XOPEN_SOURCEfor BSD/Darwin, respectively) for all code in cpu/native.Previously, these were set inconsistently, potentially leading to redefinitions, especially if you try to set them globally. This technique also proved to be quite dangerous on macOS – take
<ucontext.h>as an example:If
_XOPEN_SOURCEisn’t set, then the context struct will lack a storage buffer.[make|swap|set|get]contexthowever will assume there is indeed a storage buffer and write happily to memory far off earth. And said bug is not easy to find if some other system header importsucontext.hbefore and you forgot to set_GNU../_XOPEN…. Then your precious_GNU…/_XOPEN…definitions around<ucontext.h>become worthless. 💥Context switches
Previously, code manipulatingucontexts was duplicated; same goes for retrieving theucontext_tfrom the stack buffer. I wrapped them in zero-runtime-cost static inline’s (_context_set_fptr,_context_get_fptr). Note that these aren’t calledset_pcintentionally. There are platforms where you cannot set PC, i.e., they require another technique of transferring control back to userspace. Stay tuned for arm64 ;)makecontext madness
Pointers passed as thread arguments were previously truncated to 32 bits. This should crash, don’t really know why it didn’t already.So on 64-bit platforms, I do not use
makecontext’s mechanism of supplying thread arguments. Instead, we first go to_start_task_func64and pass the 64-bit argument in a register. I made sure that register isn’t mutated in glibc/libplatform.Then in
_start_task_func64, we invoke the real thread function and move the 64-bit argument into the proper register according to the arch’s ABI. No more castingvoid*toint.Documentation
The doxygen markup added *encloses* symbols in *named sections*. Everytime you see an@namefollowed by@{, that’s a section. To make finding sections easier, I added corresponding marks (MARK: -) which show up in editors in the minimap (e.g., VS Code, Xcode). Also fixed incorrect uses of@ingroup.Logging
While (trying) to debug native/cpu, it became apparent that understanding logs without knowing where they come from is hard. Prefixing logs with the enclosing function’s name does not help, unless you already know what said function *does*. So, for each *component*— say CPU, IRQ implementation, startup code — I prefixed logs with the rough equivalent of the filename.