On Tue, Jan 16, 2024, Friedrich Weber wrote:
> Hi Sean,
>
> On 11/01/2024 17:00, Sean Christopherson wrote:
> > This is a known issue. It's mostly a KVM bug[...] (fix posted[...]), but I suspect
> > that a bug in the dynamic preemption model logic[...] is also contributing to the
> > behavior by causing KVM to yield on preempt models where it really shouldn't.
>
> I tried the following variants now, each applied on top of 6.7 (0dd3ee31):
>
> * [1], the initial patch series mentioned in the bugreport ("[PATCH 0/2]
> KVM: Pre-check mmu_notifier retry on x86")
> * [2], its v2 that you linked above ("[PATCH v2] KVM: x86/mmu: Retry
> fault before acquiring mmu_lock if mapping is changing")
> * [3], the scheduler patch you linked above ("[PATCH] sched/core: Drop
> spinlocks on contention iff kernel is preemptible")
> * both [2] & [3]
>
> My kernel is PREEMPT_DYNAMIC and, according to
> /sys/kernel/debug/sched/preempt, defaults to preempt=voluntary. For case
> [3], I additionally tried manually switching to preempt=full.
>
> Provided I did not mess up, I get the following results for the
> reproducer I posted:
>
> * [1] (the initial patch series): no hangs
> * [2] (its v2): hangs
> * [3] (the scheduler patch) with preempt=voluntary: no hangs
> * [3] (the scheduler patch) with preempt=full: hangs
> * [2] & [3]: no hangs
>
> So it seems like:
>
> * [1] (the initial patch series) fixes the hangs, which is consistent
> with the feedback in the bugreport [4].
> * But weirdly, its v2 [2] does not fix the hangs.
> * As long as I stay with preempt=voluntary, [3] (the scheduler patch)
> alone is already enough to fix the hangs in my case -- this I did not
> expect :)
>
> Does this make sense to you? Happy to double-check or run more tests if
> anything seems off.
Ha! It too me a few minutes to realize what went sideways with v2. KVM has an
in-flight change that switches from host virtual addresses (HVA) to guest physical
frame numbers (GFN) for the retry check, commit 8569992d64b8 ("KVM: Use gfn instead
of hva for mmu_notifier_retry").
That commit is in the KVM pull request for 6.8, and so v2 is based on top of a
branch that contains said commit. But for better or worse (probably worse), the
switch from HVA=GFN didn't change the _names_ of mmu_invalidate_range_{start,end},
only the type. So v2 applies and compiles cleanly on 6.7, but it's subtly broken
because checking for a GFN match against an HVA range is all but guaranteed to get
false negatives.
If you can try v2 on top of `git://git.kernel.org/pub/scm/virt/kvm/kvm.git next`,
that would be helpful to confirm that I didn't screw up something else.
Thanks very much for reporting back! I'm pretty sure we would have missed the
semantic conflict when backporting the fix to 6.7 and earlier, i.e. you likely
saved us from another round of bug reports for various stable trees.
