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: <648b2b54-7432-4578-b785-030d98f6e31c@amd.com>
Date: Fri, 16 Jan 2026 19:52:07 +0530
From: K Prateek Nayak <kprateek.nayak@....com>
To: Vincent Guittot <vincent.guittot@...aro.org>
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

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.

-- 
Thanks and Regards,
Prateek