On Fri, Apr 12, 2024, Zide Chen wrote:
On 4/5/2024 4:01 PM, Sean Christopherson wrote:
On Fri, Mar 22, 2024, Zide Chen wrote:
Currently, the migration worker delays 1-10 us, assuming that one KVM_RUN iteration only takes a few microseconds. But if C-state exit latencies are large enough, for example, hundreds or even thousands of microseconds on server CPUs, it may happen that it's not able to bring the target CPU out of C-state before the migration worker starts to migrate it to the next CPU.
If the system workload is light, most CPUs could be at a certain level of C-state, and the vCPU thread may waste milliseconds before it can actually migrate to a new CPU.
Well fudge. That's definitely not on my bingo sheet.
Thus, the tests may be inefficient in such systems, and in some cases it may fail the migration/KVM_RUN ratio sanity check.
Since we are not able to turn off the cpuidle sub-system in run time, this patch creates an idle thread on every CPU to prevent them from entering C-states.
First off, huge thanks for debugging this! That must have been quite the task (no pun intended).
While spinning up threads on every CPU is a clever way to ensure they don't go into a deep sleep state, I'm not exactly excited about the idea of putting every reachable CPU into a busy loop. And while this doesn't add _that_ much complexity, I'm not sure the benefit (preserving the assert for all systems) is worth it. I also don't want to arbitrarily prevent idle task (as in, the kernel's idle task) interactions. E.g. it's highly (highly) unlikely, but not impossible for there to be a bug that's unique to idle tasks, or C-states, or other edge case.
Are there any metrics/stats that can be (easily) checked to grant an exception to the sanity check? That's a very hand-wavy question, as I'm not even sure what type of stat we'd want to look at. Actual runtime of a task, maybe?
If that's not easy, what if we add an off-by-default command line option to skip the sanity check? I was resistant to simply deleting the assert in the past, but that was mainly because I didn't want to delete it without understanding what was causing problems. That would allow CI environments to opt-out as needed, while still keeping the sanity check alive for enough systems to make it useful.
Sorry for not replying earlier. I overlooked your email from my inbox. :)
Alternative to the busy loop, how about using the /dev/cpu_dma_latency interface to disable c-states (I wish I had learned this before writing the initial patch)? The good thing is it can do automatic cleanup when it closes the fd.
It's probably not practical to touch /dev/cpu_dma_latency in code, e.g. on my system it's fully root-only. And forcing rseq_test to run as root, or be bookended with script commands to toggle /dev/cpu_dma_latency, is not a net positive. Lastly, fiddling with a system-wide knob in a KVM selftests is opening a can of worms I don't want to open.
However, we could have the failing TEST_ASSERT() explicitly call out /dev/cpu_dma_latency as a knob to try changing if the assert is failing. If we do that *and* add a command line option to skip the sanity check, that seems like it would give users sufficient flexibility to avoid false positives, while still maintaining good coverage.
The reason why I still think of disabling c-states is, even in the low c-states exit latency setup, it can still increase the chances of successful migration.
Otherwise, I can implement a command line option to skip the sanity check, as I was not able to find out a metrics/stats that is easy and reliable to indicate that the scheduler is not able to wake up the target CPU before the task is scheduled to another CPU.