linux-kernel - Re: [PATCH 3/5] cpuidle: Handle TIF_NR_POLLING on behalf of CPUIDLE_FLAG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z2LWX5jOTOD89yPl@localhost.localdomain>
Date: Wed, 18 Dec 2024 15:04:15 +0100
From: Frederic Weisbecker <frederic@...nel.org>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	linux-pm@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH 3/5] cpuidle: Handle TIF_NR_POLLING on behalf of
 CPUIDLE_FLAG_MWAIT states

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@...nel.org> wrote:
> >
> > From: Peter Zijlstra <peterz@...radead.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@...radead.org>
> > Signed-off-by: Frederic Weisbecker <frederic@...nel.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.