linux-kernel - Re: [PATCH v3] sched/clock: Avoid false sharing for sched_clock

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKfTPtA-UoLj-b6guqFk9MrHFJXaNwvYJw2tZ0Sey9hCRTkh3w@mail.gmail.com>
Date: Fri, 16 Jan 2026 17:41:39 +0100
From: Vincent Guittot <vincent.guittot@...aro.org>
To: K Prateek Nayak <kprateek.nayak@....com>
Cc: Wangyang Guo <wangyang.guo@...el.com>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>, 
	Dietmar Eggemann <dietmar.eggemann@....com>, Steven Rostedt <rostedt@...dmis.org>, 
	Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>, 
	Valentin Schneider <vschneid@...hat.com>, linux-kernel@...r.kernel.org, 
	Shrikanth Hegde <sshegde@...ux.ibm.com>, Benjamin Lei <benjamin.lei@...el.com>, 
	Tim Chen <tim.c.chen@...ux.intel.com>, Tianyou Li <tianyou.li@...el.com>
Subject: Re: [PATCH v3] sched/clock: Avoid false sharing for sched_clock_irqtime

On Fri, 16 Jan 2026 at 15:22, K Prateek Nayak <kprateek.nayak@....com> wrote:
>
> Hello Vincent,
>
> On 1/16/2026 3:52 PM, Vincent Guittot wrote:
> > On Fri, 16 Jan 2026 at 10:43, K Prateek Nayak <kprateek.nayak@....com> wrote:
> >>
> >> Hello Wangyang,
> >>
> >> On 1/16/2026 8:09 AM, Wangyang Guo wrote:
> >>>   */
> >>>  DEFINE_PER_CPU(struct irqtime, cpu_irqtime);
> >>>
> >>> -int sched_clock_irqtime;
> >>> -
> >>>  void enable_sched_clock_irqtime(void)
> >>>  {
> >>> -     sched_clock_irqtime = 1;
> >>> +     static_branch_enable(&sched_clock_irqtime);
> >>>  }
> >>>
> >>> +static void __disable_sched_clock_irqtime(struct work_struct *work)
> >>> +{
> >>> +     static_branch_disable(&sched_clock_irqtime);
> >>> +}
> >>> +
> >>> +static DECLARE_WORK(sched_clock_irqtime_work, __disable_sched_clock_irqtime);
> >>> +
> >>>  void disable_sched_clock_irqtime(void)
> >>>  {
> >>> -     sched_clock_irqtime = 0;
> >>> +     /* disable_sched_clock_irqtime can be called in atomic
> >>> +      * context with mark_tsc_unstable(), use wq to avoid
> >>> +      * "sleeping in atomic context" warning.
> >>> +      */
> >>> +     if (irqtime_enabled())
> >>> +             schedule_work(&sched_clock_irqtime_work);
> >>>  }
> >>
> >> Your approach looks good to avoid the scheduling while atomic issue.
> >> Just a small observation: The only user of disable_sched_clock_irqtime()
> >> is tsc_.*mark_unstable() which calls clear_sched_clock_stable() just
> >> before doing disable_sched_clock_irqtime().
> >>
> >> It makes me wonder if we can just reuse "sched_clock_work" to also\
> >> disable sched_clock_irqtime()?
> >>
> >> Peter, Vincent, do we need to do enable_sched_clock_irqtime() that early
> >> when we detect TSC freq / sched_clock_register() or can we wait until we
> >> do __set_sched_clock_stable()?
> >
> > By default we don't need a workqueue to disable sched clock irq time
> > but only tsc clock needs it just like when it disables
> > sched_lock_stable
> >
> > So the enablement during init should remain the same. Why would all
> > sched clocks delay their irq time accounting just for tsc.
>
> Yeah it was a stupid idea to consolidate the sched_clock() enable,
> disable with __sched_clock_stable enable / disable. Clearly I missed the
> whole CONFIG_HAVE_UNSTABLE_SCHED_CLOCK dependency.
>
> >
> > Furthermore, __set_sched_clock_stable() is under
> > CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
> >
> > I think that disabling irq time accounting if it was enabled in
> > __sched_clock_work() should be good
>
> I though about this and you can have this particular case as a result
> of when TSC vs sched_clock is marked unstable:
>
>   tsc_init()
>     enable_sched_clock_irqtime() # irqtime accounting is enabled here
>     ...
>     if (unsynchronized_tsc()) # true
>       mark_tsc_unstable()
>         clear_sched_clock_stable()
>           __sched_clock_stable_early = 0;
>           ...
>           if (static_key_count(&sched_clock_running.key) == 2) # Only happens at sched_clock_init_late()
>             __clear_sched_clock_stable(); # Never executed
>   ...
>
>   # late_initcall() phase
>   sched_clock_init_late()
>     if (__sched_clock_stable_early) # Already false
>       __set_sched_clock_stable(); # sched_clock is never marked stable
>
>   # TSC unstable; irqtime_enabled() is true
>
>
> The current approach from Wangyang covers this case so the v3 should be
> good as is.

The need of a workqueue is only for CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
and I'd like to keep it there instead of making the use of a workqueue
the default behavior for disabling irq time accounting whereas it's
not needed

So either we can use a single workqueue in unstable clock to handle
the disabling of one or both __sched_clock_stable and
sched_clock_irqtime

or  unstable clock need another workqueue for disabling sched_clock_irqtime

>
> --
> Thanks and Regards,
> Prateek
>