[PATCH v4] sched/clock: Avoid false sharing for sched_clock_irqtime

Wangyang Guo posted 1 patch 1 week, 5 days ago
There is a newer version of this series
arch/x86/kernel/tsc.c  | 2 --
kernel/sched/clock.c   | 3 +++
kernel/sched/cputime.c | 8 ++++----
kernel/sched/sched.h   | 4 ++--
4 files changed, 9 insertions(+), 8 deletions(-)
[PATCH v4] sched/clock: Avoid false sharing for sched_clock_irqtime
Posted by Wangyang Guo 1 week, 5 days ago
Read-mostly sched_clock_irqtime may share the same cacheline with
frequently updated nohz struct. Make it as static_key to avoid
false sharing issue.

Details:
In kernel 6.14, we observed ~3% cycles hotspots in irqtime_account_irq
when running SPECjbb2015 in a 2-sockets system. Most of cycles spent
in reading sched_clock_irqtime, which is a read-mostly var.

perf c2c (cachelien view) shows it has false sharing with nohz struct:
     Num RmtHitm LclHitm  Offset records             Symbol
   6.25%   0.00%   0.00%   0x0       4   [k] _nohz_idle_balance.isra.0
  18.75% 100.00%   0.00%   0x8      14   [k] nohz_balance_exit_idle
   6.25%   0.00%   0.00%   0x8       8   [k] nohz_balance_enter_idle
   6.25%   0.00%   0.00%   0xc       8   [k] sched_balance_newidle
   6.25%   0.00%   0.00%  0x10      31   [k] nohz_balancer_kick
   6.25%   0.00%   0.00%  0x20      16   [k] sched_balance_newidle
  37.50%   0.00%   0.00%  0x38      50   [k] irqtime_account_irq
   6.25%   0.00%   0.00%  0x38      47   [k] account_process_tick
   6.25%   0.00%   0.00%  0x38      12   [k] account_idle_ticks

Offsets:
*  0x0 -- nohz.idle_cpu_mask (r)
*  0x8 -- nohz.nr_cpus (w)
* 0x38 -- sched_clock_irqtime (r), not in nohz, but share cacheline

The layout in /proc/kallsyms can also confirm that:
ffffffff88600d40 b nohz
ffffffff88600d68 B arch_needs_tick_broadcast
ffffffff88600d6c b __key.264
ffffffff88600d6c b __key.265
ffffffff88600d70 b dl_generation
ffffffff88600d78 b sched_clock_irqtime

With the patch applied, irqtime_account_irq hotspot disappear.

---
V4 -> V3:
- Avoid creating a new workqueue to disable static_key
- Specify kernel version for c2c result in changelog

V2 -> V3:
- Use static_key instead of a __read_mostly var.

V1 -> V2:
- Use __read_mostly instead of __cacheline_aligned to avoid wasting
  spaces.

History:
  v3: https://lore.kernel.org/all/20260116023945.1849329-1-wangyang.guo@intel.com/
  v2: https://lore.kernel.org/all/20260113074807.3404180-1-wangyang.guo@intel.com/
  v1: https://lore.kernel.org/all/20260113022958.3379650-1-wangyang.guo@intel.com/
  prev discussions: https://lore.kernel.org/all/20251211055612.4071266-1-wangyang.guo@intel.com/T/#u

Suggested-by: K Prateek Nayak <kprateek.nayak@amd.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Shrikanth Hegde <sshegde@linux.ibm.com>
Reported-by: Benjamin Lei <benjamin.lei@intel.com>
Reviewed-by: Tim Chen <tim.c.chen@linux.intel.com>
Reviewed-by: Tianyou Li <tianyou.li@intel.com>
Signed-off-by: Wangyang Guo <wangyang.guo@intel.com>
---
 arch/x86/kernel/tsc.c  | 2 --
 kernel/sched/clock.c   | 3 +++
 kernel/sched/cputime.c | 8 ++++----
 kernel/sched/sched.h   | 4 ++--
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 87e749106dda..9a62e18d1bff 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1142,7 +1142,6 @@ static void tsc_cs_mark_unstable(struct clocksource *cs)
 	tsc_unstable = 1;
 	if (using_native_sched_clock())
 		clear_sched_clock_stable();
-	disable_sched_clock_irqtime();
 	pr_info("Marking TSC unstable due to clocksource watchdog\n");
 }
 
@@ -1212,7 +1211,6 @@ void mark_tsc_unstable(char *reason)
 	tsc_unstable = 1;
 	if (using_native_sched_clock())
 		clear_sched_clock_stable();
-	disable_sched_clock_irqtime();
 	pr_info("Marking TSC unstable due to %s\n", reason);
 
 	clocksource_mark_unstable(&clocksource_tsc_early);
diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
index f5e6dd6a6b3a..2ae4fbf13431 100644
--- a/kernel/sched/clock.c
+++ b/kernel/sched/clock.c
@@ -173,6 +173,7 @@ notrace static void __sched_clock_work(struct work_struct *work)
 			scd->tick_gtod, __gtod_offset,
 			scd->tick_raw,  __sched_clock_offset);
 
+	disable_sched_clock_irqtime();
 	static_branch_disable(&__sched_clock_stable);
 }
 
@@ -238,6 +239,8 @@ static int __init sched_clock_init_late(void)
 
 	if (__sched_clock_stable_early)
 		__set_sched_clock_stable();
+	else
+		disable_sched_clock_irqtime();  /* disable if clock unstable. */
 
 	return 0;
 }
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index 7097de2c8cda..959a86206c64 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -12,6 +12,8 @@
 
 #ifdef CONFIG_IRQ_TIME_ACCOUNTING
 
+DEFINE_STATIC_KEY_FALSE(sched_clock_irqtime);
+
 /*
  * There are no locks covering percpu hardirq/softirq time.
  * They are only modified in vtime_account, on corresponding CPU
@@ -25,16 +27,14 @@
  */
 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);
 }
 
 void disable_sched_clock_irqtime(void)
 {
-	sched_clock_irqtime = 0;
+	static_branch_disable(&sched_clock_irqtime);
 }
 
 static void irqtime_account_delta(struct irqtime *irqtime, u64 delta,
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index adfb6e3409d7..ec963314287a 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -3172,11 +3172,11 @@ struct irqtime {
 };
 
 DECLARE_PER_CPU(struct irqtime, cpu_irqtime);
-extern int sched_clock_irqtime;
+DECLARE_STATIC_KEY_FALSE(sched_clock_irqtime);
 
 static inline int irqtime_enabled(void)
 {
-	return sched_clock_irqtime;
+	return static_branch_likely(&sched_clock_irqtime);
 }
 
 /*
-- 
2.47.3
Re: [PATCH v4] sched/clock: Avoid false sharing for sched_clock_irqtime
Posted by Shrikanth Hegde 1 week, 4 days ago
Hi Wangyang.

On 1/26/26 7:44 AM, Wangyang Guo wrote:
> Read-mostly sched_clock_irqtime may share the same cacheline with
> frequently updated nohz struct. Make it as static_key to avoid
> false sharing issue.
> 
> Details:
> In kernel 6.14, we observed ~3% cycles hotspots in irqtime_account_irq
> when running SPECjbb2015 in a 2-sockets system. Most of cycles spent
> in reading sched_clock_irqtime, which is a read-mostly var.
> 

IMHO, this changelog is meant for latest sched/core. having 6.14 data won't be
that relevant.

Also, changelog should be mentioning essence of calling disable in
sched_clock_init_late which was in v3 discussions.

> perf c2c (cachelien view) shows it has false sharing with nohz struct:
>       Num RmtHitm LclHitm  Offset records             Symbol
>     6.25%   0.00%   0.00%   0x0       4   [k] _nohz_idle_balance.isra.0
>    18.75% 100.00%   0.00%   0x8      14   [k] nohz_balance_exit_idle
>     6.25%   0.00%   0.00%   0x8       8   [k] nohz_balance_enter_idle
>     6.25%   0.00%   0.00%   0xc       8   [k] sched_balance_newidle
>     6.25%   0.00%   0.00%  0x10      31   [k] nohz_balancer_kick
>     6.25%   0.00%   0.00%  0x20      16   [k] sched_balance_newidle
>    37.50%   0.00%   0.00%  0x38      50   [k] irqtime_account_irq
>     6.25%   0.00%   0.00%  0x38      47   [k] account_process_tick
>     6.25%   0.00%   0.00%  0x38      12   [k] account_idle_ticks
> 
> Offsets:
> *  0x0 -- nohz.idle_cpu_mask (r)
> *  0x8 -- nohz.nr_cpus (w)
> * 0x38 -- sched_clock_irqtime (r), not in nohz, but share cacheline
> 
> The layout in /proc/kallsyms can also confirm that:
> ffffffff88600d40 b nohz
> ffffffff88600d68 B arch_needs_tick_broadcast
> ffffffff88600d6c b __key.264
> ffffffff88600d6c b __key.265
> ffffffff88600d70 b dl_generation
> ffffffff88600d78 b sched_clock_irqtime
> 
> With the patch applied, irqtime_account_irq hotspot disappear.
> 
> ---

Please dont put a --- here.

git am will strip everything after this and actual diff. So the your
signed-off-by and other details won't be captured.

If the intention was keep version history, put it after the signed-off-by
for example:
https://lore.kernel.org/lkml/20260120113246.27987-2-kprateek.nayak@amd.com/

> V4 -> V3:
> - Avoid creating a new workqueue to disable static_key
> - Specify kernel version for c2c result in changelog
> 
> V2 -> V3:
> - Use static_key instead of a __read_mostly var.
> 
> V1 -> V2:
> - Use __read_mostly instead of __cacheline_aligned to avoid wasting
>    spaces.
> 
> History:
>    v3: https://lore.kernel.org/all/20260116023945.1849329-1-wangyang.guo@intel.com/
>    v2: https://lore.kernel.org/all/20260113074807.3404180-1-wangyang.guo@intel.com/
>    v1: https://lore.kernel.org/all/20260113022958.3379650-1-wangyang.guo@intel.com/
>    prev discussions: https://lore.kernel.org/all/20251211055612.4071266-1-wangyang.guo@intel.com/T/#u
> 
> Suggested-by: K Prateek Nayak <kprateek.nayak@amd.com>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Suggested-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> Reported-by: Benjamin Lei <benjamin.lei@intel.com>
> Reviewed-by: Tim Chen <tim.c.chen@linux.intel.com>
> Reviewed-by: Tianyou Li <tianyou.li@intel.com>
> Signed-off-by: Wangyang Guo <wangyang.guo@intel.com>
> ---
>   arch/x86/kernel/tsc.c  | 2 --
>   kernel/sched/clock.c   | 3 +++
>   kernel/sched/cputime.c | 8 ++++----
>   kernel/sched/sched.h   | 4 ++--
>   4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 87e749106dda..9a62e18d1bff 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1142,7 +1142,6 @@ static void tsc_cs_mark_unstable(struct clocksource *cs)
>   	tsc_unstable = 1;
>   	if (using_native_sched_clock())
>   		clear_sched_clock_stable();
> -	disable_sched_clock_irqtime();
>   	pr_info("Marking TSC unstable due to clocksource watchdog\n");
>   }
>   
> @@ -1212,7 +1211,6 @@ void mark_tsc_unstable(char *reason)
>   	tsc_unstable = 1;
>   	if (using_native_sched_clock())
>   		clear_sched_clock_stable();
> -	disable_sched_clock_irqtime();
>   	pr_info("Marking TSC unstable due to %s\n", reason);
>   
>   	clocksource_mark_unstable(&clocksource_tsc_early);
> diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
> index f5e6dd6a6b3a..2ae4fbf13431 100644
> --- a/kernel/sched/clock.c
> +++ b/kernel/sched/clock.c
> @@ -173,6 +173,7 @@ notrace static void __sched_clock_work(struct work_struct *work)
>   			scd->tick_gtod, __gtod_offset,
>   			scd->tick_raw,  __sched_clock_offset);
>   
> +	disable_sched_clock_irqtime();
>   	static_branch_disable(&__sched_clock_stable);
>   }
>   
> @@ -238,6 +239,8 @@ static int __init sched_clock_init_late(void)
>   
>   	if (__sched_clock_stable_early)
>   		__set_sched_clock_stable();
> +	else
> +		disable_sched_clock_irqtime();  /* disable if clock unstable. */
>   
>   	return 0;
>   }
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index 7097de2c8cda..959a86206c64 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -12,6 +12,8 @@
>   
>   #ifdef CONFIG_IRQ_TIME_ACCOUNTING
>   
> +DEFINE_STATIC_KEY_FALSE(sched_clock_irqtime);
> +
>   /*
>    * There are no locks covering percpu hardirq/softirq time.
>    * They are only modified in vtime_account, on corresponding CPU
> @@ -25,16 +27,14 @@
>    */
>   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);
>   }
>   
>   void disable_sched_clock_irqtime(void)
>   {
> -	sched_clock_irqtime = 0;
> +	static_branch_disable(&sched_clock_irqtime);
>   }
>   
>   static void irqtime_account_delta(struct irqtime *irqtime, u64 delta,
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index adfb6e3409d7..ec963314287a 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3172,11 +3172,11 @@ struct irqtime {
>   };
>   
>   DECLARE_PER_CPU(struct irqtime, cpu_irqtime);
> -extern int sched_clock_irqtime;
> +DECLARE_STATIC_KEY_FALSE(sched_clock_irqtime);
>   
>   static inline int irqtime_enabled(void)
>   {
> -	return sched_clock_irqtime;
> +	return static_branch_likely(&sched_clock_irqtime);
>   }
>   
>   /*
Re: [PATCH v4] sched/clock: Avoid false sharing for sched_clock_irqtime
Posted by Guo, Wangyang 1 week, 4 days ago
On 1/27/2026 2:45 AM, Shrikanth Hegde wrote:
> Hi Wangyang.
> 
> On 1/26/26 7:44 AM, Wangyang Guo wrote:
>> Read-mostly sched_clock_irqtime may share the same cacheline with
>> frequently updated nohz struct. Make it as static_key to avoid
>> false sharing issue.
>>
>> Details:
>> In kernel 6.14, we observed ~3% cycles hotspots in irqtime_account_irq
>> when running SPECjbb2015 in a 2-sockets system. Most of cycles spent
>> in reading sched_clock_irqtime, which is a read-mostly var.
>>
> 
> IMHO, this changelog is meant for latest sched/core. having 6.14 data 
> won't be
> that relevant.
> 
> Also, changelog should be mentioning essence of calling disable in
> sched_clock_init_late which was in v3 discussions.
> 
>> perf c2c (cachelien view) shows it has false sharing with nohz struct:
>>       Num RmtHitm LclHitm  Offset records             Symbol
>>     6.25%   0.00%   0.00%   0x0       4   [k] _nohz_idle_balance.isra.0
>>    18.75% 100.00%   0.00%   0x8      14   [k] nohz_balance_exit_idle
>>     6.25%   0.00%   0.00%   0x8       8   [k] nohz_balance_enter_idle
>>     6.25%   0.00%   0.00%   0xc       8   [k] sched_balance_newidle
>>     6.25%   0.00%   0.00%  0x10      31   [k] nohz_balancer_kick
>>     6.25%   0.00%   0.00%  0x20      16   [k] sched_balance_newidle
>>    37.50%   0.00%   0.00%  0x38      50   [k] irqtime_account_irq
>>     6.25%   0.00%   0.00%  0x38      47   [k] account_process_tick
>>     6.25%   0.00%   0.00%  0x38      12   [k] account_idle_ticks
>>
>> Offsets:
>> *  0x0 -- nohz.idle_cpu_mask (r)
>> *  0x8 -- nohz.nr_cpus (w)
>> * 0x38 -- sched_clock_irqtime (r), not in nohz, but share cacheline
>>
>> The layout in /proc/kallsyms can also confirm that:
>> ffffffff88600d40 b nohz
>> ffffffff88600d68 B arch_needs_tick_broadcast
>> ffffffff88600d6c b __key.264
>> ffffffff88600d6c b __key.265
>> ffffffff88600d70 b dl_generation
>> ffffffff88600d78 b sched_clock_irqtime
>>
>> With the patch applied, irqtime_account_irq hotspot disappear.
>>
>> ---
> 
> Please dont put a --- here.
> 
> git am will strip everything after this and actual diff. So the your
> signed-off-by and other details won't be captured.
> 
> If the intention was keep version history, put it after the signed-off-by
> for example:
> https://lore.kernel.org/lkml/20260120113246.27987-2-kprateek.nayak@amd.com/
Changelog updated in v5:

https://lore.kernel.org/all/20260127031602.1907377-1-wangyang.guo@intel.com/

BR
Wangyang