Received-SPF: pass (google.com: domain of linux-kernel+bounces-99245-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Received-SPF: Pass (protection.outlook.com: domain of seco.com designates
 20.160.56.84 as permitted sender) receiver=protection.outlook.com;
 client-ip=20.160.56.84; helo=repost-eu.tmcas.trendmicro.com; pr=C
Authentication-Results-Original: dkim=none (message not signed)
 header.d=none;dmarc=none action=none header.from=seco.com;
Message-ID: <ab147205-41cc-4c1b-8280-000e638b6534@seco.com>
Date: Mon, 11 Mar 2024 12:30:36 -0400
User-Agent: Mozilla Thunderbird
Subject: Re: [BUG] KCSAN: data-race in tick_nohz_idle_stop_tick /
 tick_nohz_next_event
Content-Language: en-US
To: Thomas Gleixner <tglx@linutronix.de>,
 Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>,
 linux-kernel@vger.kernel.org
Cc: Frederic Weisbecker <frederic@kernel.org>, Ingo Molnar <mingo@kernel.org>
References: <40ac9903-469a-b1ea-4339-8b06f855450e@alu.unizg.hr>
 <3993c468-fdac-2ff2-c3ee-9784c098694c@alu.unizg.hr>
 <83b3c900-d4eb-457f-99e6-cb21e0eae4cb@seco.com> <87a5n4rfhu.ffs@tglx>
From: Sean Anderson <sean.anderson@seco.com>
In-Reply-To: <87a5n4rfhu.ffs@tglx>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
X-Microsoft-Antispam-Message-Info-Original:
 m5NvIk23iIX5mwtVGxJZFHZWVGF8iHN6xXyeKJF53i2M0ggTRa5g+tSprtFQrnYIi0jk8J828bug1A89Pp1+R9nEalxxyrgMD7Ocd4JKdR2jC77zHIVTVawQktQzsMeZnsYWYR7c6nIeezsKXGM3grCLoYjcr3f1k1ZiiGnYs9hBem6O1usFsyubADZEWiGwd4YTAAFemhm9vegGtSa8bWBulIrPZA+vplwRpB3oBDkZqutnlmenT8mKsBd/zFjTjikDgLWv7q/nmXVHShQHDPzMeKz7oRJiB1RnxT4iCA7jc+mJ4RZNFap75XIFjDW9NyZ1jjQWx1Q8tMvFuvvkGfkRJXu+a1iEUbso6HMRdesfyp6wClSaKryAoV8ntPnL/kH+wslSLK/HuW9QkbslrbvuZSlRU0S2fGBX14vrK756cFN9zQef6QRgt3lnOGk3xvtGPi/GSOpuUD3Gh7Eh04NkTiEFpNuYZkkIRWUaSIeM+KrQfyxaXqL1VqYGdt9UI2CCamEUbm+JskY6oYLOhkSZEVaHvRwx5i4vo/wda66egVRk9l5gihF2piWNOyY3AqRgz/42NEmxu5a4PKDh0cIVxKE2ls4C2vRIa/3qv79W2fQmz0DeiNWbICSc3FRSB71u5SQ1HcgSmUeS4igcuGTH+BQzJwy5MY269mF/6K2yMXESse3wQhmzrf/TYgnun99hZs0cWkYkuaP3ajp162ymsnX1kEDuaJ/r969i9nk=
X-Forefront-Antispam-Report-Untrusted:
 CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DB9PR03MB8847.eurprd03.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(1800799015)(52116005)(376005)(38350700005);DIR:OUT;SFP:1102;
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU0PR03MB8854
X-EOPAttributedMessage: 0
X-MS-Exchange-Transport-CrossTenantHeadersStripped:
 AM4PEPF00027A66.eurprd04.prod.outlook.com
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id-Prvs:
	50f588ab-fc62-4f2e-7af9-08dc41e896e1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info:
	cuenmYHrUME3Llm0pMBLdNLSXSD8wWhjXwHYc6ZmHHSAcCsrNlgyZG4mZV5l6Jx//QJtnEa+oOFD5jrwof5LCqZm+HcpbqVCt4Vx+o9cRW8j/gaFg61Nr0fh6NTqWvHgYW0D9uIpoioacug00DU5JRYgAykMCa9KnaSoKihc5rIva1qNlCUSgpgm+DfSVQ28VCgp9zpU0qG9czy4LHGVAXO28VJcv4HLMMTQV8iBeIZe6fpjZPEbqPDc0TD0982Upk2VxKNC4fwj0vdSHgGvnOg70epdk0MC+8wYVQBHBmThdsgieuaWCJapnJaAgqmeVBSPAbOYxt2YGLQ2gNHmNv4q/s8PDtmGJEWQDpRUaP2rGcbziLHuW1M41ylXyyUfm6mGfR2jWjQheKK7qZW7Zo94AwROwHHvtJtI5EkfaZsWH2hdMwJqP+Xz4Gp26oKBrjBwMambH34f7P525whzSGG4olZzHNN33B6JX6rgMjEhXVTCGmlEcKcmiDI4N7v3dgG/isF0He6mGtcpVV30lH83YI+JKFiT6p1Vx+iw5ToJ2v8apZdSh9IPUmqNjjQ9i2TfrEOHi4QTs2Bk0ft9YqZ4dRKMT80L5CYtwvRblhKDTYnldZ6Wmtn/+HM878wOG2o75OA7Y8VDkrQTiJCQr745NnRw2aiN1dE2ok2FMiV5BwFGPpUFX8lZIqmF/w7XBhbDy5JHWMe1pQn0g8Y4b2/do1kXxFj1RvVldpoumhjCfXBqrokHS0hsn09QGhkx
X-Forefront-Antispam-Report:
	CIP:20.160.56.84;CTRY:NL;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:repost-eu.tmcas.trendmicro.com;PTR:repost-eu.tmcas.trendmicro.com;CAT:NONE;SFS:(13230031)(36860700004)(82310400014)(1800799015)(376005);DIR:OUT;SFP:1102;
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Mar 2024 16:30:45.4782
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 54b59124-4a13-4ab9-891f-08dc41e89a36
X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=bebe97c3-6438-442e-ade3-ff17aa50e733;Ip=[20.160.56.84];Helo=[repost-eu.tmcas.trendmicro.com]
X-MS-Exchange-CrossTenant-AuthSource:
	AM4PEPF00027A66.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR03MB8394

On 3/11/24 10:48, Thomas Gleixner wrote:
> On Thu, Mar 07 2024 at 14:43, Sean Anderson wrote:
>> On 8/18/23 10:15, Mirsad Todorovac wrote:
>>
>> The justification seems to be in tick_sched_do_timer:
>>
>>         /*
>>          * Check if the do_timer duty was dropped. We don't care about
>>          * concurrency: This happens only when the CPU in charge went
>>          * into a long sleep. If two CPUs happen to assign themselves to
>>          * this duty, then the jiffies update is still serialized by
>>          * jiffies_lock.
>>          *
>>          * If nohz_full is enabled, this should not happen because the
>>          * tick_do_timer_cpu never relinquishes.
>>          */
>>
>> with the other assignment being in tick_nohz_stop_tick. I'm not familiar
>> enough with this code to say whether we should be using READ/WRITE_ONCE
>> or maybe just data_race (as would be implied by the comment above).
>
> It wants to be READ/WRITE_ONCE(). Something like the untested below,
> which applies to -next or the master branch of the tip tree.
>
> Thanks,
>
>         tglx
> ---
>
> --- a/kernel/time/tick-common.c
> +++ b/kernel/time/tick-common.c
> @@ -7,6 +7,7 @@
>   * Copyright(C) 2005-2007, Red Hat, Inc., Ingo Molnar
>   * Copyright(C) 2006-2007, Timesys Corp., Thomas Gleixner
>   */
> +#include <linux/compiler.h>
>  #include <linux/cpu.h>
>  #include <linux/err.h>
>  #include <linux/hrtimer.h>
> @@ -84,7 +85,7 @@ int tick_is_oneshot_available(void)
>   */
>  static void tick_periodic(int cpu)
>  {
> -     if (tick_do_timer_cpu =3D=3D cpu) {
> +     if (READ_ONCE(tick_do_timer_cpu) =3D=3D cpu) {
>               raw_spin_lock(&jiffies_lock);
>               write_seqcount_begin(&jiffies_seq);
>
> @@ -215,8 +216,8 @@ static void tick_setup_device(struct tic
>                * If no cpu took the do_timer update, assign it to
>                * this cpu:
>                */
> -             if (tick_do_timer_cpu =3D=3D TICK_DO_TIMER_BOOT) {
> -                     tick_do_timer_cpu =3D cpu;
> +             if (READ_ONCE(tick_do_timer_cpu) =3D=3D TICK_DO_TIMER_BOOT)=
 {
> +                     WRITE_ONCE(tick_do_timer_cpu, cpu);
>                       tick_next_period =3D ktime_get();
>  #ifdef CONFIG_NO_HZ_FULL
>                       /*
> @@ -232,7 +233,7 @@ static void tick_setup_device(struct tic
>                                               !tick_nohz_full_cpu(cpu)) {
>                       tick_take_do_timer_from_boot();
>                       tick_do_timer_boot_cpu =3D -1;
> -                     WARN_ON(tick_do_timer_cpu !=3D cpu);
> +                     WARN_ON(READ_ON_ONCE(tick_do_timer_cpu) !=3D cpu);
>  #endif
>               }
>
> @@ -406,10 +407,10 @@ void tick_assert_timekeeping_handover(vo
>  int tick_cpu_dying(unsigned int dying_cpu)
>  {
>       /*
> -      * If the current CPU is the timekeeper, it's the only one that
> -      * can safely hand over its duty. Also all online CPUs are in
> -      * stop machine, guaranteed not to be idle, therefore it's safe
> -      * to pick any online successor.
> +      * If the current CPU is the timekeeper, it's the only one that can
> +      * safely hand over its duty. Also all online CPUs are in stop
> +      * machine, guaranteed not to be idle, therefore there is no
> +      * concurrency and it's safe to pick any online successor.
>        */
>       if (tick_do_timer_cpu =3D=3D dying_cpu)
>               tick_do_timer_cpu =3D cpumask_first(cpu_online_mask);
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -8,6 +8,7 @@
>   *
>   *  Started by: Thomas Gleixner and Ingo Molnar
>   */
> +#include <linux/compiler.h>
>  #include <linux/cpu.h>
>  #include <linux/err.h>
>  #include <linux/hrtimer.h>
> @@ -204,7 +205,7 @@ static inline void tick_sched_flag_clear
>
>  static void tick_sched_do_timer(struct tick_sched *ts, ktime_t now)
>  {
> -     int cpu =3D smp_processor_id();
> +     int tick_cpu, cpu =3D smp_processor_id();
>
>       /*
>        * Check if the do_timer duty was dropped. We don't care about
> @@ -216,16 +217,18 @@ static void tick_sched_do_timer(struct t
>        * If nohz_full is enabled, this should not happen because the
>        * 'tick_do_timer_cpu' CPU never relinquishes.
>        */
> -     if (IS_ENABLED(CONFIG_NO_HZ_COMMON) &&
> -         unlikely(tick_do_timer_cpu =3D=3D TICK_DO_TIMER_NONE)) {
> +     tick_cpu =3D READ_ONCE(tick_do_timer_cpu);
> +
> +     if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && unlikely(tick_cpu =3D=3D TIC=
K_DO_TIMER_NONE)) {
>  #ifdef CONFIG_NO_HZ_FULL
>               WARN_ON_ONCE(tick_nohz_full_running);
>  #endif
> -             tick_do_timer_cpu =3D cpu;
> +             WRITE_ONCE(tick_do_timer_cpu, cpu);
> +             tick_cpu =3D cpu;
>       }
>
>       /* Check if jiffies need an update */
> -     if (tick_do_timer_cpu =3D=3D cpu)
> +     if (tick_cpu =3D=3D cpu)
>               tick_do_update_jiffies64(now);
>
>       /*
> @@ -610,7 +613,7 @@ bool tick_nohz_cpu_hotpluggable(unsigned
>        * timers, workqueues, timekeeping, ...) on behalf of full dynticks
>        * CPUs. It must remain online when nohz full is enabled.
>        */
> -     if (tick_nohz_full_running && tick_do_timer_cpu =3D=3D cpu)
> +     if (tick_nohz_full_running && READ_ONCE(tick_do_timer_cpu) =3D=3D c=
pu)
>               return false;
>       return true;
>  }
> @@ -890,6 +893,7 @@ static ktime_t tick_nohz_next_event(stru
>  {
>       u64 basemono, next_tick, delta, expires;
>       unsigned long basejiff;
> +     int tick_cpu;
>
>       basemono =3D get_jiffies_update(&basejiff);
>       ts->last_jiffies =3D basejiff;
> @@ -946,9 +950,9 @@ static ktime_t tick_nohz_next_event(stru
>        * Otherwise we can sleep as long as we want.
>        */
>       delta =3D timekeeping_max_deferment();
> -     if (cpu !=3D tick_do_timer_cpu &&
> -         (tick_do_timer_cpu !=3D TICK_DO_TIMER_NONE ||
> -          !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST)))
> +     tick_cpu =3D READ_ONCE(tick_do_timer_cpu);
> +     if (tick_cpu !=3D cpu &&
> +         (tick_cpu !=3D TICK_DO_TIMER_NONE || !tick_sched_flag_test(ts, =
TS_FLAG_DO_TIMER_LAST)))
>               delta =3D KTIME_MAX;
>
>       /* Calculate the next expiry time */
> @@ -969,6 +973,7 @@ static void tick_nohz_stop_tick(struct t
>       unsigned long basejiff =3D ts->last_jiffies;
>       u64 basemono =3D ts->timer_expires_base;
>       bool timer_idle =3D tick_sched_flag_test(ts, TS_FLAG_STOPPED);
> +     int tick_cpu;
>       u64 expires;
>
>       /* Make sure we won't be trying to stop it twice in a row. */
> @@ -1006,10 +1011,11 @@ static void tick_nohz_stop_tick(struct t
>        * do_timer() never gets invoked. Keep track of the fact that it
>        * was the one which had the do_timer() duty last.
>        */
> -     if (cpu =3D=3D tick_do_timer_cpu) {
> -             tick_do_timer_cpu =3D TICK_DO_TIMER_NONE;
> +     tick_cpu =3D READ_ONCE(tick_do_timer_cpu);
> +     if (tick_cpu =3D=3D cpu) {
> +             WRITE_ONCE(tick_do_timer_cpu, TICK_DO_TIMER_NONE);
>               tick_sched_flag_set(ts, TS_FLAG_DO_TIMER_LAST);
> -     } else if (tick_do_timer_cpu !=3D TICK_DO_TIMER_NONE) {
> +     } else if (tick_cpu !=3D TICK_DO_TIMER_NONE) {
>               tick_sched_flag_clear(ts, TS_FLAG_DO_TIMER_LAST);
>       }
>
> @@ -1172,15 +1178,17 @@ static bool can_stop_idle_tick(int cpu,
>               return false;
>
>       if (tick_nohz_full_enabled()) {
> +             int tick_cpu =3D READ_ONCE(tick_do_timer_cpu);
> +
>               /*
>                * Keep the tick alive to guarantee timekeeping progression
>                * if there are full dynticks CPUs around
>                */
> -             if (tick_do_timer_cpu =3D=3D cpu)
> +             if (tick_cpu =3D=3D cpu)
>                       return false;
>
>               /* Should not happen for nohz-full */
> -             if (WARN_ON_ONCE(tick_do_timer_cpu =3D=3D TICK_DO_TIMER_NON=
E))
> +             if (WARN_ON_ONCE(tick_cpu =3D=3D TICK_DO_TIMER_NONE))
>                       return false;
>       }

This fixes the issue for me (tested on 6.6; tweaked a bit for conflicts).

--Sean


[Embedded World 2024, SECO SpA]<https://www.messe-ticket.de/Nuernberg/embed=
dedworld2024/Register/ew24517689>