Re: [PATCH 3/5] cpuidle: Handle TIF_NR_POLLING on behalf of CPUIDLE_FLAG_MWAIT states

[Frederic Weisbecker <frederic@kernel.org>]

Le Wed, Dec 18, 2024 at 02:24:36PM +0100, Rafael J. Wysocki a écrit :
> On Fri, Dec 6, 2024 at 2:04 PM Frederic Weisbecker <frederic@kernel.org> wrote:
> >
> > From: Peter Zijlstra <peterz@infradead.org>
> >
> > The current handling of TIF_NR_POLLING is a bit of a maze:
> >
> > 1) TIF_NR_POLLING is set on idle entry (one atomic set)
> >
> > 2) Once cpuidle has selected an appropriate state and the tick is
> >    evaluated and then possibly stopped, TIF_NR_POLLING is cleared
> >    (one RmW operation)
> >
> > 2) The cpuidle state is then called with TIF_NR_POLLING cleared but if
> >    the state polls on (or monitors) need_resched() it sets again
> >    TIF_NR_POLLING before sleeping and clears it on wake-up. Summary:
> >    another pair of set/clear
> >
> > 3) Set back TIF_NR_POLLING (one atomic set)
> >
> > 4) goto 2) if need_resched() is not set
> >
> > All those costly atomic operations, fully ordered RmW for some of
> > them, could be avoided if the cpuidle core knew in advance if the target
> > state polls on (or monitors) need_resched(). If so, TIF_NR_POLLING could
> > simply be set once upon entering the idle loop and cleared once after
> > idle loop exit.
> >
> > Start dealing with that with handling TIF_NR_POLLING on behalf of
> > mwait based states.
> >
> > [fweisbec: _ Handle broadcast properly
> >            _ Ignore mwait_idle() as it can be used by default_idle_call()]
> >
> > Not-yet-signed-off-by: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> > ---
> >  arch/x86/include/asm/mwait.h |  3 +--
> >  drivers/cpuidle/cpuidle.c    | 22 +++++++++++++++++++-
> >  include/linux/sched/idle.h   |  7 ++++++-
> >  kernel/sched/idle.c          | 40 +++++++++++++-----------------------
> >  4 files changed, 42 insertions(+), 30 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
> > index 920426d691ce..3634d00e5c37 100644
> > --- a/arch/x86/include/asm/mwait.h
> > +++ b/arch/x86/include/asm/mwait.h
> > @@ -116,7 +116,7 @@ static __always_inline void __sti_mwait(unsigned long eax, unsigned long ecx)
> >   */
> >  static __always_inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
> >  {
> > -       if (static_cpu_has_bug(X86_BUG_MONITOR) || !current_set_polling_and_test()) {
> > +       if (static_cpu_has_bug(X86_BUG_MONITOR) || !need_resched()) {
> 
> I'm not sure how X86_BUG_MONITOR is going to work after the change.
> 
> As is, X86_BUG_MONITOR prevents current_set_polling_and_test() from
> being called, so __current_set_polling() will not be called and
> TIF_POLLING_NRFLAG will not be set, so an IPI is going to be used for
> CPU wakeup, which is what X86_BUG_MONITOR wants.
> 
> Preventing need_resched() from being called doesn't have this effect
> and the rest of the patch doesn't do anything about X86_BUG_MONITOR.
> 
> Is anything missing?

I fear you're right, I overlooked that. Probably I should set CPUIDLE_FLAG_MWAIT
only if !boot_cpu_has_bug(X86_BUG_MONITOR). Lemme see that.

Thanks.