Re: [PATCH v4 22/30] context_tracking: Exit CT_STATE_IDLE upon irq/nmi entry

Frederic Weisbecker
2025-02-07T18:38:00Z

Le Fri, Feb 07, 2025 at 06:06:45PM +0100, Valentin Schneider a écrit :
> On 27/01/25 12:17, Valentin Schneider wrote:
> > On 22/01/25 01:22, Frederic Weisbecker wrote:
> >> And NMIs interrupting userspace don't call
> >> enter_from_user_mode(). In fact they don't call irqentry_enter_from_user_mode()
> >> like regular IRQs but irqentry_nmi_enter() instead. Well that's for archs
> >> implementing common entry code, I can't speak for the others.
> >>
> >
> > That I didn't realize, so thank you for pointing it out. Having another
> > look now, I mistook DEFINE_IDTENTRY_RAW(exc_int3) for the general case
> > when it really isn't :(
> >
> >> Unifying the behaviour between user and idle such that the IRQs/NMIs exit the
> >> CT_STATE can be interesting but I fear this may not come for free. You would
> >> need to save the old state on IRQ/NMI entry and restore it on exit.
> >>
> >
> > That's what I tried to avoid, but it sounds like there's no nice way around it.
> >
> >> Do we really need it?
> >>
> >
> > Well, my problem with not doing IDLE->KERNEL transitions on IRQ/NMI is that
> > this leads the IPI deferral logic to observe a technically-out-of-sync sate
> > for remote CPUs. Consider:
> >
> > CPUx CPUy
> > state := CT_STATE_IDLE
> > ...
> > ~>IRQ
> > ...
> > ct_nmi_enter()
> > [in the kernel proper by now]
> >
> > text_poke_bp_batch()
> > ct_set_cpu_work(CPUy, CT_WORK_SYNC)
> > READ CPUy ct->state
> > `-> CT_IDLE_STATE
> > `-> defer IPI
> >
> >
> > I thought this meant I would need to throw out the "defer IPIs if CPU is
> > idle" part, but AIUI this also affects CT_STATE_USER and CT_STATE_GUEST,
> > which is a bummer :(
>
> Soooo I've been thinking...
>
> Isn't
>
> (context_tracking.state & CT_RCU_WATCHING)
>
> pretty much a proxy for knowing whether a CPU is executing in kernelspace,
> including NMIs?

You got it!

>
> NMI interrupts userspace/VM/idle -> ct_nmi_enter() -> it becomes true
> IRQ interrupts idle -> ct_irq_enter() -> it becomes true
> IRQ interrupts userspace -> __ct_user_exit() -> it becomes true
> IRQ interrupts VM -> __ct_user_exit() -> it becomes true
>
> IOW, if I gate setting deferred work by checking for this instead of
> explicitely CT_STATE_KERNEL, "it should work" and prevent the
> aforementioned issue? Or should I be out drinking instead? :-)

Exactly it should work! Now that doesn't mean you can't go out
for a drink :-)

Thanks.