On Thu, Mar 27, 2025, Yan Zhao wrote:
> On Fri, Mar 21, 2025 at 12:49:42PM +0100, Paolo Bonzini wrote:
> > On Wed, Mar 19, 2025 at 5:17 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > Yan posted a patch to fudge around the issue[*], I strongly objected (and still
> > > object) to making a functional and confusing code change to fudge around a lockdep
> > > false positive.
> >
> > In that thread I had made another suggestion, which Yan also tried,
> > which was to use subclasses:
> >
> > - in the sched_out path, which cannot race with the others:
> > raw_spin_lock_nested(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu), 1);
> >
> > - in the irq and sched_in paths, which can race with each other:
> > raw_spin_lock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
> Hi Paolo, Sean, Maxim,
>
> The sched_out path still may race with sched_in path. e.g.
> CPU 0 CPU 1
> ----------------- ---------------
> vCPU 0 sched_out
> vCPU 1 sched_in
> vCPU 1 sched_out vCPU 0 sched_in
>
> vCPU 0 sched_in may race with vCPU 1 sched_out on CPU 0's wakeup list.
>
>
> So, the situation is
> sched_in, sched_out: race
> sched_in, irq: race
> sched_out, irq: mutual exclusive, do not race
>
>
> Hence, do you think below subclasses assignments reasonable?
> irq: subclass 0
> sched_out: subclass 1
> sched_in: subclasses 0 and 1
>
> As inspired by Sean's solution, I made below patch to inform lockdep that the
> sched_in path involves both subclasses 0 and 1 by adding a line
> "spin_acquire(&spinlock->dep_map, 1, 0, _RET_IP_)".
>
> I like it because it accurately conveys the situation to lockdep :)
Me too :-)
Can you give your SoB? I wrote comments and a changelog to explain to myself
(yet again), what the problem is, and why it's a false positive. I also want
to change the local_irq_{save,restore}() into a lockdep assertion in a prep patch,
because this and the self-IPI trick rely on IRQs being disabled until the task
is fully scheduled out and the scheduler locks are dopped.
