Enable SMP mode on all tests, fix bugs #18531
Conversation
zephyrbot
added
area: Test Framework
|
All checks are passing now. Review history of this comment for details about previous failed status. |
|
Addresses #14639. Note that this is never going to pass a regular CI run given how many tests it touches. It has to be validated manually. Also note that the "long line" errors are entirely lines that were too long already and are single expressions that won't split. Some tests have really long names, and all I'm doing is adding four bytes to the unit_test() macro name. |
I'm not convinced this is not just sweeping the problem under the rug again, albeit in a subtly different and more clever fashion. If we're testing features in an SMP environment, shouldn't we replicate an SMP environment? Disabling all CPUs but one, or pegging all the CPUs but one, isn't really representative of normal operation. If the problem is that the tests are written improperly (that is, assuming one CPU), then shouldn't they be rewritten? |
Do we have a volunteer? To be honest, rewriting tests is itself a kind of sweeping under the rug: our promise to users is that the code continues to do what we thought it did. Rewriting tests so that they pass is itself sort of a way to fool ourselves that we haven't broken anything. It's absolutely routine that strange old tests break new code in unexpected ways because of edge case assumptions (c.f. some of the fixes in this very PR about where reschedule points are, which weren't ever really documented). And that's a good thing. |
If I write a test that assumes that |
|
Heh, the checkpatch errors hid a commented-out pair of test cases that never got a fix. Fixed now. Note that since there's concern about risk that this involves an actual detectable difference between scheduling in SMP and UP which has been addressed by relaxing the test a bit. See comments in the third-to-last commit that was just added. It wouldn't be impossible to unify the behavior between the two, but there would be significant complexity and some performance cost. |
This touches upon what I was trying to express the other day, that the SMP vs non-SMP schedulers seem rife with lots of subtle differences of this sort, both in observed behavior by clients, and inside the scheduler code. As an RTOS, Zephyr should have rock-solid and well-defined behavior where scheduling is concerned. |
Our documentation is weak in a lot of areas, and this is obviously one of those. "Someday" we should really nail down some critical core features, like scheduling ... |
|
Ping on this. If there's resistance to merging the ztest and CI-visible configuration changes at this stage, we should at least split out and merge the fixes themselves. |
|
@andyross getting: |
andyross
force-pushed
the
smpify-tests
branch
2 times, most recently
from
8b3f0d1
to
446ef85
Compare
|
Ugh. Simple build failure on ARC (the SMP support wasn't quite complete and didn't implement the IPI trigger function), but then I made the mistake of running the x86_64 tests here with my rig in #18656 and found that, hey, SMP is showing some spurious failures that are not host load dependent. (That is, the inherent asynchrony between CPUs is causing failures and not the coarse host scheduling due to other processes interfering -- these are certainly real bugs, not artifacts). I found two. One turned out to be a simple stack overflow in the new cpuhold feature. The other seems to be a qemu race with HPET (time can go backwards between CPUs) which was easy enough to work around. I'm still seeing maybe one test run out of ~300 fail, so there's probably at least one more bug in there somewhere. |
|
Should be good to merge, given that the remaining bugs are present already and the frequency seems well under the threshold that would impact CI. |
... hmmm.. what kinds of real bugs?
I feel like every week we've got some bug to blame on QEMU. |
|
Per @nashif this is being pushed. Will merge next week. |
|
@andyross two tests at least failing: and tests/kernel/sched/schedule_api/kernel.sched |
|
Want to rebase this one? |
There were two related bugs when in SMP mode: 1. Underneath z_reschedule(), the code was inexplicably checking the swap_ok flag on the current CPU to see if it was OK to preempt the current thread, but reschedule is the DEFINITION of a schedule point and we always want to swap, even if the current thread is non-preemptible. 2. With similar symptoms: in k_yield() a previous fix correct the queue handling for SMP, but it missed the case where a thread of the SAME priority as _current was on the queue and would fail to swap. Yielding must always add the current thread to the back of the current priority. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
The loop in thread abort on SMP where we wait for the results on an IPI correctly handled the case where a thread running on another CPU gets its interrupt and self-aborts, but it missed the case where the other thread pends before receiving the interrupt. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
In uniprocessor mode, the kernel knows when a context switch "is coming" because of the cache optimization and can use that to do things like update time slice state. But on SMP the scheduler state may be updated on the other CPU at any time, so we don't know that a switch is going to happen until the last minute. Expose reset_time_slice() as a public function and call it when needed out of z_swap(). Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
Our thread struct gets initialized piecewise in a bunch of locations (this is sort of a design flaw). The is_idle field, which was introduced to identify idle threads in SMP (where there can be more than one), was correctly set for idle threads but was being left uninitialized elsewhere, and in a tiny handful of cases was turning up nonzero. The case in pipes. was particularly vexsome, as that isn't a thread at all but one of the "dummy" threads used for timeouts (another design flaw IMHO). Get this right everywhere. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
Now that we have a working IPI framework, there's no reason for the default spin loop for the SMP idle thread. Just use the default platform idle and send an IPI when a new thread is readied. Long term, this can be optimized if necessary (e.g. only send the IPI to idling CPUs, or check priorities, etc...), but for a 2-cpu system this is a very reasonable default. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
The timeout code has an optimization where it refuses to send a new timeout to the driver unless it is sooner than one already scheduled. This won't work on SMP, though, because the timeout value when timeslicing is enabled depends on the current thread, and on SMP the decision as to the next thread will not be made until later (when we swap, or exit an interrupt). Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
This test was testing for an undocumented and somewhat hyperspecific behavior: when a process reaches a reschedule point and yields to a higher priority thread, and there is another equal priority thread active, which thread gets to run when the higher priority thread finishes its work? The original scheduler (because it leaves the older thread in place in the list) implements the preemption like an interrupt and returns to the original thread, despite the fact that this then resets is time slice quantum unfairly. In SMP mode, where the current threads cannot live in the active list, the thread gets added back to the end of the queue and the other thread runs. In effect, in UP mode "yield" and "reschedule" mean very slightly different things where in SMP they act the same. We don't document either behavior, as it happens. Relax the test constraints by adding a single deliberate k_yield() to unify behavior. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
The test suite is filled with tests that make assumptions (e.g. about exactly when other threads will be scheduled) that don't work when there is another CPU available to handle the load. Add a feature to the test suite that can "hold" all but one CPU while the test executes, leveraging the very nice setup/teardown callbacks to do it. When there is only one CPU, this becomes a very fast noop of course. Note that the hold is done by disabling interrupts and spinning, so it comes with significant CPU cost and tends to drive up the load on the CI system (and cause other spurious failures on unrelated tests!), so this can't be used for long-running test cases. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
Disabling SMP mode for certain tests was a one-release thing, done to avoid having to triage every test independently (MANY are not SMP-safe), and with the knowledge that it was probably hiding bugs in the kernel. Turn it on pervasively. Tests are treated with a combination of flagging specific cases as "1cpu" where we have short-running tests that can be independently run in an otherwise SMP environment, and via setting CONFIG_MP_NUM_CPUS=1 where that's not possible (which still runs the full SMP kernel config, but with only one CPU available). Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
At least twice (to be fair: twice among thousands of test runs), I've seen this device return "backwards" times in SMP, where the counter value read from one CPU is behind the saved value already seen on the other. On hardware this should obviously never happen, HPET is a single global device. Add a simple workaround on QEMU targets so the math doesn't blow up. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
If an architecture declares support for IPI, we still want to use it only when running in SMP mode. (This also fixes a build failure on ARC, which declares CONFIG_SCHED_IPI_SUPPORTED but doesn't actually implement z_arch_sched_ipi() yet). Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
nashif
force-pushed
the
smpify-tests
branch
from
446ef85
to
f7b8b39
Compare
|
All checks passed. checkpatch (informational only, not a failure)Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
ghost
mentioned this pull request
|
Any reason we haven't merged this? |
A bunch of test work to re-enable SMP on all tests (there are two missing, which seem to be bugs in posix and cmsis_v1 -- will file bugs on those), and targetted fixes for the inevitable kernel bugs they exposed.