* [BUG] KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
@ 2023-08-17 19:32 Mirsad Todorovac
2023-08-18 14:15 ` Mirsad Todorovac
0 siblings, 1 reply; 11+ messages in thread
From: Mirsad Todorovac @ 2023-08-17 19:32 UTC (permalink / raw
To: linux-kernel; +Cc: Frederic Weisbecker, Thomas Gleixner, Ingo Molnar
[-- Attachment #1: Type: text/plain, Size: 1827 bytes --]
Hi,
This is your friendly bug reporter. :-)
The environment is the vanilla torvalds kernel on Ubuntu 22.04 LTS and a Ryzen 7950X assembled box.
[ 7145.500079] ==================================================================
[ 7145.500109] BUG: KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
[ 7145.500139] write to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 26:
[ 7145.500155] tick_nohz_idle_stop_tick+0x3b1/0x4a0
[ 7145.500173] do_idle+0x1e3/0x250
[ 7145.500186] cpu_startup_entry+0x20/0x30
[ 7145.500199] start_secondary+0x129/0x160
[ 7145.500216] secondary_startup_64_no_verify+0x17e/0x18b
[ 7145.500245] read to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 16:
[ 7145.500261] tick_nohz_next_event+0xe7/0x1e0
[ 7145.500277] tick_nohz_get_sleep_length+0xa7/0xe0
[ 7145.500293] menu_select+0x82/0xb90
[ 7145.500309] cpuidle_select+0x44/0x60
[ 7145.500324] do_idle+0x1c2/0x250
[ 7145.500336] cpu_startup_entry+0x20/0x30
[ 7145.500350] start_secondary+0x129/0x160
[ 7145.500367] secondary_startup_64_no_verify+0x17e/0x18b
[ 7145.500392] value changed: 0x0000001a -> 0xffffffff
[ 7145.500411] Reported by Kernel Concurrency Sanitizer on:
[ 7145.500421] CPU: 16 PID: 0 Comm: swapper/16 Tainted: G L 6.5.0-rc6-net-cfg-kcsan-00038-g16931859a650 #35
[ 7145.500439] Hardware name: ASRock X670E PG Lightning/X670E PG Lightning, BIOS 1.21 04/26/2023
[ 7145.500449] ==================================================================
Please find dmesg log (what was available in the ring buffer) and the lshw output attached.
NOTE: There are no proprietary drivers loaded and Canonical livepatch isn't working for this kernel,
probably previous ACPI KCSAN BUGs turn this taint on.
Thank you very much for your time evaluating this report.
Best regards,
Mirsad Todorovac
[-- Attachment #2: dmesg-3.log.xz --]
[-- Type: application/x-xz, Size: 67080 bytes --]
[-- Attachment #3: lshw.txt.xz --]
[-- Type: application/x-xz, Size: 6516 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
2023-08-17 19:32 [BUG] KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event Mirsad Todorovac
@ 2023-08-18 14:15 ` Mirsad Todorovac
2024-03-07 19:43 ` Sean Anderson
0 siblings, 1 reply; 11+ messages in thread
From: Mirsad Todorovac @ 2023-08-18 14:15 UTC (permalink / raw
To: linux-kernel; +Cc: Frederic Weisbecker, Thomas Gleixner, Ingo Molnar
On 8/17/23 21:32, Mirsad Todorovac wrote:
> Hi,
>
> This is your friendly bug reporter. :-)
>
> The environment is the vanilla torvalds kernel on Ubuntu 22.04 LTS and a Ryzen 7950X assembled box.
>
> [ 7145.500079] ==================================================================
> [ 7145.500109] BUG: KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
>
> [ 7145.500139] write to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 26:
> [ 7145.500155] tick_nohz_idle_stop_tick+0x3b1/0x4a0
> [ 7145.500173] do_idle+0x1e3/0x250
> [ 7145.500186] cpu_startup_entry+0x20/0x30
> [ 7145.500199] start_secondary+0x129/0x160
> [ 7145.500216] secondary_startup_64_no_verify+0x17e/0x18b
>
> [ 7145.500245] read to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 16:
> [ 7145.500261] tick_nohz_next_event+0xe7/0x1e0
> [ 7145.500277] tick_nohz_get_sleep_length+0xa7/0xe0
> [ 7145.500293] menu_select+0x82/0xb90
> [ 7145.500309] cpuidle_select+0x44/0x60
> [ 7145.500324] do_idle+0x1c2/0x250
> [ 7145.500336] cpu_startup_entry+0x20/0x30
> [ 7145.500350] start_secondary+0x129/0x160
> [ 7145.500367] secondary_startup_64_no_verify+0x17e/0x18b
>
> [ 7145.500392] value changed: 0x0000001a -> 0xffffffff
>
> [ 7145.500411] Reported by Kernel Concurrency Sanitizer on:
> [ 7145.500421] CPU: 16 PID: 0 Comm: swapper/16 Tainted: G L 6.5.0-rc6-net-cfg-kcsan-00038-g16931859a650 #35
> [ 7145.500439] Hardware name: ASRock X670E PG Lightning/X670E PG Lightning, BIOS 1.21 04/26/2023
> [ 7145.500449] ==================================================================
>
> Please find dmesg log (what was available in the ring buffer) and the lshw output attached.
>
> NOTE: There are no proprietary drivers loaded and Canonical livepatch isn't working for this kernel,
> probably previous ACPI KCSAN BUGs turn this taint on.
>
> Thank you very much for your time evaluating this report.
P.S.
According to Mr. Heo, I will add the decoded stacktrace to this bug report, which ought to have been done
in the first place :-/
[ 7145.500079] ==================================================================
[ 7145.500109] BUG: KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
[ 7145.500139] write to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 26:
[ 7145.500155] tick_nohz_idle_stop_tick (kernel/time/tick-sched.c:904 kernel/time/tick-sched.c:1130)
[ 7145.500173] do_idle (kernel/sched/idle.c:215 kernel/sched/idle.c:282)
[ 7145.500186] cpu_startup_entry (kernel/sched/idle.c:378 (discriminator 1))
[ 7145.500199] start_secondary (arch/x86/kernel/smpboot.c:210 arch/x86/kernel/smpboot.c:294)
[ 7145.500216] secondary_startup_64_no_verify (arch/x86/kernel/head_64.S:441)
[ 7145.500245] read to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 16:
[ 7145.500261] tick_nohz_next_event (kernel/time/tick-sched.c:868)
[ 7145.500277] tick_nohz_get_sleep_length (kernel/time/tick-sched.c:1250)
[ 7145.500293] menu_select (drivers/cpuidle/governors/menu.c:283)
[ 7145.500309] cpuidle_select (drivers/cpuidle/cpuidle.c:359)
[ 7145.500324] do_idle (kernel/sched/idle.c:208 kernel/sched/idle.c:282)
[ 7145.500336] cpu_startup_entry (kernel/sched/idle.c:378 (discriminator 1))
[ 7145.500350] start_secondary (arch/x86/kernel/smpboot.c:210 arch/x86/kernel/smpboot.c:294)
[ 7145.500367] secondary_startup_64_no_verify (arch/x86/kernel/head_64.S:441)
[ 7145.500392] value changed: 0x0000001a -> 0xffffffff
[ 7145.500411] Reported by Kernel Concurrency Sanitizer on:
[ 7145.500421] CPU: 16 PID: 0 Comm: swapper/16 Tainted: G L 6.5.0-rc6-net-cfg-kcsan-00038-g16931859a650 #35
[ 7145.500439] Hardware name: ASRock X670E PG Lightning/X670E PG Lightning, BIOS 1.21 04/26/2023
[ 7145.500449] ==================================================================
The code seems to be:
/*
* If this CPU is the one which updates jiffies, then give up
* the assignment and let it be taken by the CPU which runs
* the tick timer next, which might be this CPU as well. If we
* don't drop this here the jiffies might be stale and
* do_timer() never invoked. Keep track of the fact that it
* was the one which had the do_timer() duty last.
*/
if (cpu == tick_do_timer_cpu) {
tick_do_timer_cpu = TICK_DO_TIMER_NONE;
→ ts->do_timer_last = 1;
} else if (tick_do_timer_cpu != TICK_DO_TIMER_NONE) {
ts->do_timer_last = 0;
}
but I am not sure why this happens.
Maybe a WRITE_ONCE(ts->do_timer_last, 1) is required?
Or is it another KCSAN false positive ...
Thank you very much for investigating.
Best regards,
Mirsad Todorovac
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
2023-08-18 14:15 ` Mirsad Todorovac
@ 2024-03-07 19:43 ` Sean Anderson
2024-03-09 16:13 ` Mirsad Todorovac
2024-03-11 14:48 ` Thomas Gleixner
0 siblings, 2 replies; 11+ messages in thread
From: Sean Anderson @ 2024-03-07 19:43 UTC (permalink / raw
To: Mirsad Todorovac, linux-kernel
Cc: Frederic Weisbecker, Thomas Gleixner, Ingo Molnar
On 8/18/23 10:15, Mirsad Todorovac wrote:
>
>
> On 8/17/23 21:32, Mirsad Todorovac wrote:
>> Hi,
>>
>> This is your friendly bug reporter. :-)
>>
>> The environment is the vanilla torvalds kernel on Ubuntu 22.04 LTS and a Ryzen 7950X assembled box.
>>
>> [ 7145.500079] ==================================================================
>> [ 7145.500109] BUG: KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
>>
>> [ 7145.500139] write to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 26:
>> [ 7145.500155] tick_nohz_idle_stop_tick+0x3b1/0x4a0
>> [ 7145.500173] do_idle+0x1e3/0x250
>> [ 7145.500186] cpu_startup_entry+0x20/0x30
>> [ 7145.500199] start_secondary+0x129/0x160
>> [ 7145.500216] secondary_startup_64_no_verify+0x17e/0x18b
>>
>> [ 7145.500245] read to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 16:
>> [ 7145.500261] tick_nohz_next_event+0xe7/0x1e0
>> [ 7145.500277] tick_nohz_get_sleep_length+0xa7/0xe0
>> [ 7145.500293] menu_select+0x82/0xb90
>> [ 7145.500309] cpuidle_select+0x44/0x60
>> [ 7145.500324] do_idle+0x1c2/0x250
>> [ 7145.500336] cpu_startup_entry+0x20/0x30
>> [ 7145.500350] start_secondary+0x129/0x160
>> [ 7145.500367] secondary_startup_64_no_verify+0x17e/0x18b
>>
>> [ 7145.500392] value changed: 0x0000001a -> 0xffffffff
>>
>> [ 7145.500411] Reported by Kernel Concurrency Sanitizer on:
>> [ 7145.500421] CPU: 16 PID: 0 Comm: swapper/16 Tainted: G L 6.5.0-rc6-net-cfg-kcsan-00038-g16931859a650 #35
>> [ 7145.500439] Hardware name: ASRock X670E PG Lightning/X670E PG Lightning, BIOS 1.21 04/26/2023
>> [ 7145.500449] ==================================================================
>>
>> Please find dmesg log (what was available in the ring buffer) and the lshw output attached.
>>
>> NOTE: There are no proprietary drivers loaded and Canonical livepatch isn't working for this kernel,
>> probably previous ACPI KCSAN BUGs turn this taint on.
>>
>> Thank you very much for your time evaluating this report.
>
> P.S.
>
> According to Mr. Heo, I will add the decoded stacktrace to this bug report, which ought to have been done
> in the first place :-/
>
> [ 7145.500079] ==================================================================
> [ 7145.500109] BUG: KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
>
> [ 7145.500139] write to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 26:
> [ 7145.500155] tick_nohz_idle_stop_tick (kernel/time/tick-sched.c:904 kernel/time/tick-sched.c:1130)
> [ 7145.500173] do_idle (kernel/sched/idle.c:215 kernel/sched/idle.c:282)
> [ 7145.500186] cpu_startup_entry (kernel/sched/idle.c:378 (discriminator 1))
> [ 7145.500199] start_secondary (arch/x86/kernel/smpboot.c:210 arch/x86/kernel/smpboot.c:294)
> [ 7145.500216] secondary_startup_64_no_verify (arch/x86/kernel/head_64.S:441)
>
> [ 7145.500245] read to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 16:
> [ 7145.500261] tick_nohz_next_event (kernel/time/tick-sched.c:868)
> [ 7145.500277] tick_nohz_get_sleep_length (kernel/time/tick-sched.c:1250)
> [ 7145.500293] menu_select (drivers/cpuidle/governors/menu.c:283)
> [ 7145.500309] cpuidle_select (drivers/cpuidle/cpuidle.c:359)
> [ 7145.500324] do_idle (kernel/sched/idle.c:208 kernel/sched/idle.c:282)
> [ 7145.500336] cpu_startup_entry (kernel/sched/idle.c:378 (discriminator 1))
> [ 7145.500350] start_secondary (arch/x86/kernel/smpboot.c:210 arch/x86/kernel/smpboot.c:294)
> [ 7145.500367] secondary_startup_64_no_verify (arch/x86/kernel/head_64.S:441)
>
> [ 7145.500392] value changed: 0x0000001a -> 0xffffffff
>
> [ 7145.500411] Reported by Kernel Concurrency Sanitizer on:
> [ 7145.500421] CPU: 16 PID: 0 Comm: swapper/16 Tainted: G L 6.5.0-rc6-net-cfg-kcsan-00038-g16931859a650 #35
> [ 7145.500439] Hardware name: ASRock X670E PG Lightning/X670E PG Lightning, BIOS 1.21 04/26/2023
> [ 7145.500449] ==================================================================
>
> The code seems to be:
>
>
> /*
> * If this CPU is the one which updates jiffies, then give up
> * the assignment and let it be taken by the CPU which runs
> * the tick timer next, which might be this CPU as well. If we
> * don't drop this here the jiffies might be stale and
> * do_timer() never invoked. Keep track of the fact that it
> * was the one which had the do_timer() duty last.
> */
> if (cpu == tick_do_timer_cpu) {
> tick_do_timer_cpu = TICK_DO_TIMER_NONE;
> → ts->do_timer_last = 1;
> } else if (tick_do_timer_cpu != TICK_DO_TIMER_NONE) {
> ts->do_timer_last = 0;
>
> }
>
> but I am not sure why this happens.
>
> Maybe a WRITE_ONCE(ts->do_timer_last, 1) is required?
>
> Or is it another KCSAN false positive ...
I have seen a few more KCSAN reports regarding this variable:
[ 153.285199] ==================================================================
[ 153.292529] BUG: KCSAN: data-race in tick_nohz_next_event+0x15c/0x29c
[ 153.299118]
[ 153.300685] race at unknown origin, with read to 0xffffffc082aa1048 of 4 bytes by task 0 on cpu 2:
[ 153.309760] tick_nohz_next_event+0x15c/0x29c
[ 153.314256] tick_nohz_get_sleep_length+0xd8/0x118
[ 153.319186] menu_select+0x1e0/0xc08
[ 153.322875] cpuidle_select+0x4c/0x60
[ 153.326686] do_idle+0x1e0/0x2b4
[ 153.330062] cpu_startup_entry+0x34/0x3c
[ 153.334133] secondary_start_kernel+0x1e8/0x21c
[ 153.338821] __secondary_switched+0xb8/0xbc
[ 153.343143]
[ 153.344706] no locks held by swapper/2/0.
[ 153.348802] irq event stamp: 129586
[ 153.352370] hardirqs last enabled at (129585): [<ffffffc081acaa2c>] el1_interrupt+0x40/0x50
[ 153.360964] hardirqs last disabled at (129586): [<ffffffc0800d8fec>] do_idle+0xc8/0x2b4
[ 153.369132] softirqs last enabled at (129560): [<ffffffc08001071c>] __do_softirq+0x47c/0x500
[ 153.377786] softirqs last disabled at (129549): [<ffffffc080017134>] ____do_softirq+0x10/0x1c
[ 153.386457]
[ 153.388019] value changed: 0xffffffff -> 0x00000001
[ 153.392993]
[ 153.394555] Reported by Kernel Concurrency Sanitizer on:
[ 153.399947] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 6.6.20+ #102
[ 153.406595] Hardware name: xlnx,zynqmp (DT)
[ 153.410866] ==================================================================
and:
[ 75.362055] ==================================================================
[ 75.369461] BUG: KCSAN: data-race in tick_sched_do_timer / tick_sched_do_timer
[ 75.376854]
[ 75.378426] write to 0xffffffc082aa1048 of 4 bytes by interrupt on cpu 1:
[ 75.385335] tick_sched_do_timer+0x124/0x168
[ 75.389744] tick_sched_timer+0x48/0xc8
[ 75.393720] __hrtimer_run_queues+0x200/0x5f0
[ 75.398198] hrtimer_interrupt+0x1b0/0x3f4
[ 75.402417] arch_timer_handler_phys+0x40/0x54
[ 75.406983] handle_percpu_devid_irq+0xf8/0x20c
[ 75.411679] generic_handle_domain_irq+0x48/0x64
[ 75.416435] gic_handle_irq+0xc4/0x100
[ 75.420298] call_on_irq_stack+0x24/0x4c
[ 75.424360] do_interrupt_handler+0xc0/0xc4
[ 75.428692] el1_interrupt+0x34/0x50
[ 75.432415] el1h_64_irq_handler+0x18/0x24
[ 75.436668] el1h_64_irq+0x64/0x68
[ 75.440184] percpu_counter_add_batch+0xd8/0x1b8
[ 75.444967] set_pte_range+0x14c/0x2c8
[ 75.448838] filemap_map_pages+0x2dc/0xa38
[ 75.453091] __handle_mm_fault+0x1304/0x17dc
[ 75.457492] handle_mm_fault+0xa0/0x374
[ 75.461450] do_page_fault+0x1f8/0x620
[ 75.465365] do_translation_fault+0x118/0x138
[ 75.469887] do_mem_abort+0x5c/0xe4
[ 75.473516] el0_da+0x5c/0xac
[ 75.476623] el0t_64_sync_handler+0xe4/0x158
[ 75.481050] el0t_64_sync+0x190/0x194
[ 75.484834]
[ 75.486405] 5 locks held by (d-logind)/293:
[ 75.490693] #0: ffffff88076753c0 (&vma->vm_lock->lock){++++}-{3:3}, at: lock_vma_under_rcu+0x15c/0x5c0
[ 75.500441] #1: ffffffc082b042f0 (rcu_read_lock){....}-{1:2}, at: __handle_mm_fault+0x1260/0x17dc
[ 75.509755] #2: ffffffc082b042f0 (rcu_read_lock){....}-{1:2}, at: filemap_map_pages+0x0/0xa38
[ 75.518748] #3: ffffffc082b042f0 (rcu_read_lock){....}-{1:2}, at: __pte_offset_map+0x0/0x2a0
[ 75.527662] #4: ffffff880762b018 (ptlock_ptr(ptdesc)){+.+.}-{2:2}, at: __pte_offset_map_lock+0xa4/0x264
[ 75.537488] irq event stamp: 1761
[ 75.540890] hardirqs last enabled at (1760): [<ffffffc080bc1f78>] percpu_counter_add_batch+0x160/0x1b8
[ 75.550465] hardirqs last disabled at (1761): [<ffffffc081aca990>] el1_interrupt+0x24/0x50
[ 75.558893] softirqs last enabled at (1534): [<ffffffc08001071c>] __do_softirq+0x47c/0x500
[ 75.567382] softirqs last disabled at (1529): [<ffffffc080017134>] ____do_softirq+0x10/0x1c
[ 75.575889]
[ 75.577460] read to 0xffffffc082aa1048 of 4 bytes by interrupt on cpu 2:
[ 75.584273] tick_sched_do_timer+0x48/0x168
[ 75.588596] tick_sched_timer+0x48/0xc8
[ 75.592572] __hrtimer_run_queues+0x200/0x5f0
[ 75.597050] hrtimer_interrupt+0x1b0/0x3f4
[ 75.601269] arch_timer_handler_phys+0x40/0x54
[ 75.605835] handle_percpu_devid_irq+0xf8/0x20c
[ 75.610531] generic_handle_domain_irq+0x48/0x64
[ 75.615287] gic_handle_irq+0xc4/0x100
[ 75.619150] call_on_irq_stack+0x24/0x4c
[ 75.623204] do_interrupt_handler+0xc0/0xc4
[ 75.627535] el1_interrupt+0x34/0x50
[ 75.631258] el1h_64_irq_handler+0x18/0x24
[ 75.635512] el1h_64_irq+0x64/0x68
[ 75.639027] lock_acquire+0x13c/0x2f0
[ 75.642820] count_memcg_event_mm.part.0+0x70/0x2a8
[ 75.647829] handle_mm_fault+0x1d8/0x374
[ 75.651874] do_page_fault+0x1f8/0x620
[ 75.655789] do_mem_abort+0x5c/0xe4
[ 75.659417] el0_da+0x5c/0xac
[ 75.662524] el0t_64_sync_handler+0xe4/0x158
[ 75.666951] el0t_64_sync+0x190/0x194
[ 75.670736]
[ 75.672307] 2 locks held by systemd/1:
[ 75.676152] #0: ffffff88042627b0 (&vma->vm_lock->lock){++++}-{3:3}, at: lock_vma_under_rcu+0x15c/0x5c0
[ 75.685900] #1: ffffffc082b042f0 (rcu_read_lock){....}-{1:2}, at: count_memcg_event_mm.part.0+0x0/0x2a8
[ 75.695726] irq event stamp: 576347
[ 75.699302] hardirqs last enabled at (576346): [<ffffffc08001af70>] local_daif_restore+0x38/0x54
[ 75.708338] hardirqs last disabled at (576347): [<ffffffc081aca990>] el1_interrupt+0x24/0x50
[ 75.716940] softirqs last enabled at (576161): [<ffffffc08001071c>] __do_softirq+0x47c/0x500
[ 75.725602] softirqs last disabled at (576154): [<ffffffc080017134>] ____do_softirq+0x10/0x1c
[ 75.734283]
[ 75.735854] value changed: 0xffffffff -> 0x00000000
[ 75.740836]
[ 75.742407] Reported by Kernel Concurrency Sanitizer on:
[ 75.747808] CPU: 2 PID: 1 Comm: systemd Not tainted 6.6.20+ #101
[ 75.754290] Hardware name: xlnx,zynqmp (DT)
[ 75.758563] ==================================================================
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).
--Sean
[Embedded World 2024, SECO SpA]<https://www.messe-ticket.de/Nuernberg/embeddedworld2024/Register/ew24517689>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
2024-03-07 19:43 ` Sean Anderson
@ 2024-03-09 16:13 ` Mirsad Todorovac
2024-03-11 14:48 ` Thomas Gleixner
1 sibling, 0 replies; 11+ messages in thread
From: Mirsad Todorovac @ 2024-03-09 16:13 UTC (permalink / raw
To: Sean Anderson; +Cc: Linux Kernel Mailing List
Hi, Sean,
I am reducing Cc: so if you could please attach the decode_stacktrace.sh output of your build.
Thanks,
Mirsad
On 3/7/24 20:43, Sean Anderson wrote:
> On 8/18/23 10:15, Mirsad Todorovac wrote:
>>
>>
>> On 8/17/23 21:32, Mirsad Todorovac wrote:
>>> Hi,
>>>
>>> This is your friendly bug reporter. :-)
>>>
>>> The environment is the vanilla torvalds kernel on Ubuntu 22.04 LTS and a Ryzen 7950X assembled box.
>>>
>>> [ 7145.500079] ==================================================================
>>> [ 7145.500109] BUG: KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
>>>
>>> [ 7145.500139] write to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 26:
>>> [ 7145.500155] tick_nohz_idle_stop_tick+0x3b1/0x4a0
>>> [ 7145.500173] do_idle+0x1e3/0x250
>>> [ 7145.500186] cpu_startup_entry+0x20/0x30
>>> [ 7145.500199] start_secondary+0x129/0x160
>>> [ 7145.500216] secondary_startup_64_no_verify+0x17e/0x18b
>>>
>>> [ 7145.500245] read to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 16:
>>> [ 7145.500261] tick_nohz_next_event+0xe7/0x1e0
>>> [ 7145.500277] tick_nohz_get_sleep_length+0xa7/0xe0
>>> [ 7145.500293] menu_select+0x82/0xb90
>>> [ 7145.500309] cpuidle_select+0x44/0x60
>>> [ 7145.500324] do_idle+0x1c2/0x250
>>> [ 7145.500336] cpu_startup_entry+0x20/0x30
>>> [ 7145.500350] start_secondary+0x129/0x160
>>> [ 7145.500367] secondary_startup_64_no_verify+0x17e/0x18b
>>>
>>> [ 7145.500392] value changed: 0x0000001a -> 0xffffffff
>>>
>>> [ 7145.500411] Reported by Kernel Concurrency Sanitizer on:
>>> [ 7145.500421] CPU: 16 PID: 0 Comm: swapper/16 Tainted: G L 6.5.0-rc6-net-cfg-kcsan-00038-g16931859a650 #35
>>> [ 7145.500439] Hardware name: ASRock X670E PG Lightning/X670E PG Lightning, BIOS 1.21 04/26/2023
>>> [ 7145.500449] ==================================================================
>>>
>>> Please find dmesg log (what was available in the ring buffer) and the lshw output attached.
>>>
>>> NOTE: There are no proprietary drivers loaded and Canonical livepatch isn't working for this kernel,
>>> probably previous ACPI KCSAN BUGs turn this taint on.
>>>
>>> Thank you very much for your time evaluating this report.
>>
>> P.S.
>>
>> According to Mr. Heo, I will add the decoded stacktrace to this bug report, which ought to have been done
>> in the first place :-/
>>
>> [ 7145.500079] ==================================================================
>> [ 7145.500109] BUG: KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
>>
>> [ 7145.500139] write to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 26:
>> [ 7145.500155] tick_nohz_idle_stop_tick (kernel/time/tick-sched.c:904 kernel/time/tick-sched.c:1130)
>> [ 7145.500173] do_idle (kernel/sched/idle.c:215 kernel/sched/idle.c:282)
>> [ 7145.500186] cpu_startup_entry (kernel/sched/idle.c:378 (discriminator 1))
>> [ 7145.500199] start_secondary (arch/x86/kernel/smpboot.c:210 arch/x86/kernel/smpboot.c:294)
>> [ 7145.500216] secondary_startup_64_no_verify (arch/x86/kernel/head_64.S:441)
>>
>> [ 7145.500245] read to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 16:
>> [ 7145.500261] tick_nohz_next_event (kernel/time/tick-sched.c:868)
>> [ 7145.500277] tick_nohz_get_sleep_length (kernel/time/tick-sched.c:1250)
>> [ 7145.500293] menu_select (drivers/cpuidle/governors/menu.c:283)
>> [ 7145.500309] cpuidle_select (drivers/cpuidle/cpuidle.c:359)
>> [ 7145.500324] do_idle (kernel/sched/idle.c:208 kernel/sched/idle.c:282)
>> [ 7145.500336] cpu_startup_entry (kernel/sched/idle.c:378 (discriminator 1))
>> [ 7145.500350] start_secondary (arch/x86/kernel/smpboot.c:210 arch/x86/kernel/smpboot.c:294)
>> [ 7145.500367] secondary_startup_64_no_verify (arch/x86/kernel/head_64.S:441)
>>
>> [ 7145.500392] value changed: 0x0000001a -> 0xffffffff
>>
>> [ 7145.500411] Reported by Kernel Concurrency Sanitizer on:
>> [ 7145.500421] CPU: 16 PID: 0 Comm: swapper/16 Tainted: G L 6.5.0-rc6-net-cfg-kcsan-00038-g16931859a650 #35
>> [ 7145.500439] Hardware name: ASRock X670E PG Lightning/X670E PG Lightning, BIOS 1.21 04/26/2023
>> [ 7145.500449] ==================================================================
>>
>> The code seems to be:
>>
>>
>> /*
>> * If this CPU is the one which updates jiffies, then give up
>> * the assignment and let it be taken by the CPU which runs
>> * the tick timer next, which might be this CPU as well. If we
>> * don't drop this here the jiffies might be stale and
>> * do_timer() never invoked. Keep track of the fact that it
>> * was the one which had the do_timer() duty last.
>> */
>> if (cpu == tick_do_timer_cpu) {
>> tick_do_timer_cpu = TICK_DO_TIMER_NONE;
>> → ts->do_timer_last = 1;
>> } else if (tick_do_timer_cpu != TICK_DO_TIMER_NONE) {
>> ts->do_timer_last = 0;
>>
>> }
>>
>> but I am not sure why this happens.
>>
>> Maybe a WRITE_ONCE(ts->do_timer_last, 1) is required?
>>
>> Or is it another KCSAN false positive ...
>
> I have seen a few more KCSAN reports regarding this variable:
>
> [ 153.285199] ==================================================================
> [ 153.292529] BUG: KCSAN: data-race in tick_nohz_next_event+0x15c/0x29c
> [ 153.299118]
> [ 153.300685] race at unknown origin, with read to 0xffffffc082aa1048 of 4 bytes by task 0 on cpu 2:
> [ 153.309760] tick_nohz_next_event+0x15c/0x29c
> [ 153.314256] tick_nohz_get_sleep_length+0xd8/0x118
> [ 153.319186] menu_select+0x1e0/0xc08
> [ 153.322875] cpuidle_select+0x4c/0x60
> [ 153.326686] do_idle+0x1e0/0x2b4
> [ 153.330062] cpu_startup_entry+0x34/0x3c
> [ 153.334133] secondary_start_kernel+0x1e8/0x21c
> [ 153.338821] __secondary_switched+0xb8/0xbc
> [ 153.343143]
> [ 153.344706] no locks held by swapper/2/0.
> [ 153.348802] irq event stamp: 129586
> [ 153.352370] hardirqs last enabled at (129585): [<ffffffc081acaa2c>] el1_interrupt+0x40/0x50
> [ 153.360964] hardirqs last disabled at (129586): [<ffffffc0800d8fec>] do_idle+0xc8/0x2b4
> [ 153.369132] softirqs last enabled at (129560): [<ffffffc08001071c>] __do_softirq+0x47c/0x500
> [ 153.377786] softirqs last disabled at (129549): [<ffffffc080017134>] ____do_softirq+0x10/0x1c
> [ 153.386457]
> [ 153.388019] value changed: 0xffffffff -> 0x00000001
> [ 153.392993]
> [ 153.394555] Reported by Kernel Concurrency Sanitizer on:
> [ 153.399947] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 6.6.20+ #102
> [ 153.406595] Hardware name: xlnx,zynqmp (DT)
> [ 153.410866] ==================================================================
>
> and:
>
> [ 75.362055] ==================================================================
> [ 75.369461] BUG: KCSAN: data-race in tick_sched_do_timer / tick_sched_do_timer
> [ 75.376854]
> [ 75.378426] write to 0xffffffc082aa1048 of 4 bytes by interrupt on cpu 1:
> [ 75.385335] tick_sched_do_timer+0x124/0x168
> [ 75.389744] tick_sched_timer+0x48/0xc8
> [ 75.393720] __hrtimer_run_queues+0x200/0x5f0
> [ 75.398198] hrtimer_interrupt+0x1b0/0x3f4
> [ 75.402417] arch_timer_handler_phys+0x40/0x54
> [ 75.406983] handle_percpu_devid_irq+0xf8/0x20c
> [ 75.411679] generic_handle_domain_irq+0x48/0x64
> [ 75.416435] gic_handle_irq+0xc4/0x100
> [ 75.420298] call_on_irq_stack+0x24/0x4c
> [ 75.424360] do_interrupt_handler+0xc0/0xc4
> [ 75.428692] el1_interrupt+0x34/0x50
> [ 75.432415] el1h_64_irq_handler+0x18/0x24
> [ 75.436668] el1h_64_irq+0x64/0x68
> [ 75.440184] percpu_counter_add_batch+0xd8/0x1b8
> [ 75.444967] set_pte_range+0x14c/0x2c8
> [ 75.448838] filemap_map_pages+0x2dc/0xa38
> [ 75.453091] __handle_mm_fault+0x1304/0x17dc
> [ 75.457492] handle_mm_fault+0xa0/0x374
> [ 75.461450] do_page_fault+0x1f8/0x620
> [ 75.465365] do_translation_fault+0x118/0x138
> [ 75.469887] do_mem_abort+0x5c/0xe4
> [ 75.473516] el0_da+0x5c/0xac
> [ 75.476623] el0t_64_sync_handler+0xe4/0x158
> [ 75.481050] el0t_64_sync+0x190/0x194
> [ 75.484834]
> [ 75.486405] 5 locks held by (d-logind)/293:
> [ 75.490693] #0: ffffff88076753c0 (&vma->vm_lock->lock){++++}-{3:3}, at: lock_vma_under_rcu+0x15c/0x5c0
> [ 75.500441] #1: ffffffc082b042f0 (rcu_read_lock){....}-{1:2}, at: __handle_mm_fault+0x1260/0x17dc
> [ 75.509755] #2: ffffffc082b042f0 (rcu_read_lock){....}-{1:2}, at: filemap_map_pages+0x0/0xa38
> [ 75.518748] #3: ffffffc082b042f0 (rcu_read_lock){....}-{1:2}, at: __pte_offset_map+0x0/0x2a0
> [ 75.527662] #4: ffffff880762b018 (ptlock_ptr(ptdesc)){+.+.}-{2:2}, at: __pte_offset_map_lock+0xa4/0x264
> [ 75.537488] irq event stamp: 1761
> [ 75.540890] hardirqs last enabled at (1760): [<ffffffc080bc1f78>] percpu_counter_add_batch+0x160/0x1b8
> [ 75.550465] hardirqs last disabled at (1761): [<ffffffc081aca990>] el1_interrupt+0x24/0x50
> [ 75.558893] softirqs last enabled at (1534): [<ffffffc08001071c>] __do_softirq+0x47c/0x500
> [ 75.567382] softirqs last disabled at (1529): [<ffffffc080017134>] ____do_softirq+0x10/0x1c
> [ 75.575889]
> [ 75.577460] read to 0xffffffc082aa1048 of 4 bytes by interrupt on cpu 2:
> [ 75.584273] tick_sched_do_timer+0x48/0x168
> [ 75.588596] tick_sched_timer+0x48/0xc8
> [ 75.592572] __hrtimer_run_queues+0x200/0x5f0
> [ 75.597050] hrtimer_interrupt+0x1b0/0x3f4
> [ 75.601269] arch_timer_handler_phys+0x40/0x54
> [ 75.605835] handle_percpu_devid_irq+0xf8/0x20c
> [ 75.610531] generic_handle_domain_irq+0x48/0x64
> [ 75.615287] gic_handle_irq+0xc4/0x100
> [ 75.619150] call_on_irq_stack+0x24/0x4c
> [ 75.623204] do_interrupt_handler+0xc0/0xc4
> [ 75.627535] el1_interrupt+0x34/0x50
> [ 75.631258] el1h_64_irq_handler+0x18/0x24
> [ 75.635512] el1h_64_irq+0x64/0x68
> [ 75.639027] lock_acquire+0x13c/0x2f0
> [ 75.642820] count_memcg_event_mm.part.0+0x70/0x2a8
> [ 75.647829] handle_mm_fault+0x1d8/0x374
> [ 75.651874] do_page_fault+0x1f8/0x620
> [ 75.655789] do_mem_abort+0x5c/0xe4
> [ 75.659417] el0_da+0x5c/0xac
> [ 75.662524] el0t_64_sync_handler+0xe4/0x158
> [ 75.666951] el0t_64_sync+0x190/0x194
> [ 75.670736]
> [ 75.672307] 2 locks held by systemd/1:
> [ 75.676152] #0: ffffff88042627b0 (&vma->vm_lock->lock){++++}-{3:3}, at: lock_vma_under_rcu+0x15c/0x5c0
> [ 75.685900] #1: ffffffc082b042f0 (rcu_read_lock){....}-{1:2}, at: count_memcg_event_mm.part.0+0x0/0x2a8
> [ 75.695726] irq event stamp: 576347
> [ 75.699302] hardirqs last enabled at (576346): [<ffffffc08001af70>] local_daif_restore+0x38/0x54
> [ 75.708338] hardirqs last disabled at (576347): [<ffffffc081aca990>] el1_interrupt+0x24/0x50
> [ 75.716940] softirqs last enabled at (576161): [<ffffffc08001071c>] __do_softirq+0x47c/0x500
> [ 75.725602] softirqs last disabled at (576154): [<ffffffc080017134>] ____do_softirq+0x10/0x1c
> [ 75.734283]
> [ 75.735854] value changed: 0xffffffff -> 0x00000000
> [ 75.740836]
> [ 75.742407] Reported by Kernel Concurrency Sanitizer on:
> [ 75.747808] CPU: 2 PID: 1 Comm: systemd Not tainted 6.6.20+ #101
> [ 75.754290] Hardware name: xlnx,zynqmp (DT)
> [ 75.758563] ==================================================================
>
> 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).
>
> --Sean
>
> [Embedded World 2024, SECO SpA]<https://www.messe-ticket.de/Nuernberg/embeddedworld2024/Register/ew24517689>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
2024-03-07 19:43 ` Sean Anderson
2024-03-09 16:13 ` Mirsad Todorovac
@ 2024-03-11 14:48 ` Thomas Gleixner
2024-03-11 16:30 ` Sean Anderson
1 sibling, 1 reply; 11+ messages in thread
From: Thomas Gleixner @ 2024-03-11 14:48 UTC (permalink / raw
To: Sean Anderson, Mirsad Todorovac, linux-kernel
Cc: Frederic Weisbecker, Ingo Molnar
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 == cpu) {
+ if (READ_ONCE(tick_do_timer_cpu) == 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 == TICK_DO_TIMER_BOOT) {
- tick_do_timer_cpu = cpu;
+ if (READ_ONCE(tick_do_timer_cpu) == TICK_DO_TIMER_BOOT) {
+ WRITE_ONCE(tick_do_timer_cpu, cpu);
tick_next_period = 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 = -1;
- WARN_ON(tick_do_timer_cpu != cpu);
+ WARN_ON(READ_ON_ONCE(tick_do_timer_cpu) != 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 == dying_cpu)
tick_do_timer_cpu = 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 = smp_processor_id();
+ int tick_cpu, cpu = 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 == TICK_DO_TIMER_NONE)) {
+ tick_cpu = READ_ONCE(tick_do_timer_cpu);
+
+ if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && unlikely(tick_cpu == TICK_DO_TIMER_NONE)) {
#ifdef CONFIG_NO_HZ_FULL
WARN_ON_ONCE(tick_nohz_full_running);
#endif
- tick_do_timer_cpu = cpu;
+ WRITE_ONCE(tick_do_timer_cpu, cpu);
+ tick_cpu = cpu;
}
/* Check if jiffies need an update */
- if (tick_do_timer_cpu == cpu)
+ if (tick_cpu == 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 == cpu)
+ if (tick_nohz_full_running && READ_ONCE(tick_do_timer_cpu) == cpu)
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 = get_jiffies_update(&basejiff);
ts->last_jiffies = basejiff;
@@ -946,9 +950,9 @@ static ktime_t tick_nohz_next_event(stru
* Otherwise we can sleep as long as we want.
*/
delta = timekeeping_max_deferment();
- if (cpu != tick_do_timer_cpu &&
- (tick_do_timer_cpu != TICK_DO_TIMER_NONE ||
- !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST)))
+ tick_cpu = READ_ONCE(tick_do_timer_cpu);
+ if (tick_cpu != cpu &&
+ (tick_cpu != TICK_DO_TIMER_NONE || !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST)))
delta = KTIME_MAX;
/* Calculate the next expiry time */
@@ -969,6 +973,7 @@ static void tick_nohz_stop_tick(struct t
unsigned long basejiff = ts->last_jiffies;
u64 basemono = ts->timer_expires_base;
bool timer_idle = 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 == tick_do_timer_cpu) {
- tick_do_timer_cpu = TICK_DO_TIMER_NONE;
+ tick_cpu = READ_ONCE(tick_do_timer_cpu);
+ if (tick_cpu == 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 != TICK_DO_TIMER_NONE) {
+ } else if (tick_cpu != 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 = 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 == cpu)
+ if (tick_cpu == cpu)
return false;
/* Should not happen for nohz-full */
- if (WARN_ON_ONCE(tick_do_timer_cpu == TICK_DO_TIMER_NONE))
+ if (WARN_ON_ONCE(tick_cpu == TICK_DO_TIMER_NONE))
return false;
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [BUG] KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
2024-03-11 14:48 ` Thomas Gleixner
@ 2024-03-11 16:30 ` Sean Anderson
2024-04-09 10:29 ` timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu Thomas Gleixner
0 siblings, 1 reply; 11+ messages in thread
From: Sean Anderson @ 2024-03-11 16:30 UTC (permalink / raw
To: Thomas Gleixner, Mirsad Todorovac, linux-kernel
Cc: Frederic Weisbecker, Ingo Molnar
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 == cpu) {
> + if (READ_ONCE(tick_do_timer_cpu) == 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 == TICK_DO_TIMER_BOOT) {
> - tick_do_timer_cpu = cpu;
> + if (READ_ONCE(tick_do_timer_cpu) == TICK_DO_TIMER_BOOT) {
> + WRITE_ONCE(tick_do_timer_cpu, cpu);
> tick_next_period = 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 = -1;
> - WARN_ON(tick_do_timer_cpu != cpu);
> + WARN_ON(READ_ON_ONCE(tick_do_timer_cpu) != 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 == dying_cpu)
> tick_do_timer_cpu = 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 = smp_processor_id();
> + int tick_cpu, cpu = 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 == TICK_DO_TIMER_NONE)) {
> + tick_cpu = READ_ONCE(tick_do_timer_cpu);
> +
> + if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && unlikely(tick_cpu == TICK_DO_TIMER_NONE)) {
> #ifdef CONFIG_NO_HZ_FULL
> WARN_ON_ONCE(tick_nohz_full_running);
> #endif
> - tick_do_timer_cpu = cpu;
> + WRITE_ONCE(tick_do_timer_cpu, cpu);
> + tick_cpu = cpu;
> }
>
> /* Check if jiffies need an update */
> - if (tick_do_timer_cpu == cpu)
> + if (tick_cpu == 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 == cpu)
> + if (tick_nohz_full_running && READ_ONCE(tick_do_timer_cpu) == cpu)
> 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 = get_jiffies_update(&basejiff);
> ts->last_jiffies = basejiff;
> @@ -946,9 +950,9 @@ static ktime_t tick_nohz_next_event(stru
> * Otherwise we can sleep as long as we want.
> */
> delta = timekeeping_max_deferment();
> - if (cpu != tick_do_timer_cpu &&
> - (tick_do_timer_cpu != TICK_DO_TIMER_NONE ||
> - !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST)))
> + tick_cpu = READ_ONCE(tick_do_timer_cpu);
> + if (tick_cpu != cpu &&
> + (tick_cpu != TICK_DO_TIMER_NONE || !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST)))
> delta = KTIME_MAX;
>
> /* Calculate the next expiry time */
> @@ -969,6 +973,7 @@ static void tick_nohz_stop_tick(struct t
> unsigned long basejiff = ts->last_jiffies;
> u64 basemono = ts->timer_expires_base;
> bool timer_idle = 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 == tick_do_timer_cpu) {
> - tick_do_timer_cpu = TICK_DO_TIMER_NONE;
> + tick_cpu = READ_ONCE(tick_do_timer_cpu);
> + if (tick_cpu == 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 != TICK_DO_TIMER_NONE) {
> + } else if (tick_cpu != 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 = 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 == cpu)
> + if (tick_cpu == cpu)
> return false;
>
> /* Should not happen for nohz-full */
> - if (WARN_ON_ONCE(tick_do_timer_cpu == TICK_DO_TIMER_NONE))
> + if (WARN_ON_ONCE(tick_cpu == 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/embeddedworld2024/Register/ew24517689>
^ permalink raw reply [flat|nested] 11+ messages in thread
* timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu
2024-03-11 16:30 ` Sean Anderson
@ 2024-04-09 10:29 ` Thomas Gleixner
2024-04-09 21:18 ` [tip: timers/urgent] " tip-bot2 for Thomas Gleixner
2024-04-10 8:22 ` tip-bot2 for Thomas Gleixner
0 siblings, 2 replies; 11+ messages in thread
From: Thomas Gleixner @ 2024-04-09 10:29 UTC (permalink / raw
To: Sean Anderson, Mirsad Todorovac, linux-kernel
Cc: Frederic Weisbecker, Ingo Molnar
tick_do_timer_cpu is used lockless to check which CPU needs to take care
of the per tick timekeeping duty. This is done to avoid a thundering
herd problem on jiffies_lock.
The read and writes are not annotated so KCSAN complains about data races:
BUG: KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
write to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 26:
tick_nohz_idle_stop_tick+0x3b1/0x4a0
do_idle+0x1e3/0x250
read to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 16:
tick_nohz_next_event+0xe7/0x1e0
tick_nohz_get_sleep_length+0xa7/0xe0
menu_select+0x82/0xb90
cpuidle_select+0x44/0x60
do_idle+0x1c2/0x250
value changed: 0x0000001a -> 0xffffffff
Annotate them with READ/WRITE_ONCE() to document the intentional data race.
Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Sean Anderson <sean.anderson@seco.com>
---
kernel/time/tick-common.c | 17 +++++++++--------
kernel/time/tick-sched.c | 36 ++++++++++++++++++++++--------------
2 files changed, 31 insertions(+), 22 deletions(-)
--- 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 == cpu) {
+ if (READ_ONCE(tick_do_timer_cpu) == 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 == TICK_DO_TIMER_BOOT) {
- tick_do_timer_cpu = cpu;
+ if (READ_ONCE(tick_do_timer_cpu) == TICK_DO_TIMER_BOOT) {
+ WRITE_ONCE(tick_do_timer_cpu, cpu);
tick_next_period = 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 = -1;
- WARN_ON(tick_do_timer_cpu != cpu);
+ WARN_ON(READ_ON_ONCE(tick_do_timer_cpu) != 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 == dying_cpu)
tick_do_timer_cpu = 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 = smp_processor_id();
+ int tick_cpu, cpu = 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 == TICK_DO_TIMER_NONE)) {
+ tick_cpu = READ_ONCE(tick_do_timer_cpu);
+
+ if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && unlikely(tick_cpu == TICK_DO_TIMER_NONE)) {
#ifdef CONFIG_NO_HZ_FULL
WARN_ON_ONCE(tick_nohz_full_running);
#endif
- tick_do_timer_cpu = cpu;
+ WRITE_ONCE(tick_do_timer_cpu, cpu);
+ tick_cpu = cpu;
}
/* Check if jiffies need an update */
- if (tick_do_timer_cpu == cpu)
+ if (tick_cpu == 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 == cpu)
+ if (tick_nohz_full_running && READ_ONCE(tick_do_timer_cpu) == cpu)
return false;
return true;
}
@@ -891,6 +894,7 @@ static ktime_t tick_nohz_next_event(stru
{
u64 basemono, next_tick, delta, expires;
unsigned long basejiff;
+ int tick_cpu;
basemono = get_jiffies_update(&basejiff);
ts->last_jiffies = basejiff;
@@ -947,9 +951,9 @@ static ktime_t tick_nohz_next_event(stru
* Otherwise we can sleep as long as we want.
*/
delta = timekeeping_max_deferment();
- if (cpu != tick_do_timer_cpu &&
- (tick_do_timer_cpu != TICK_DO_TIMER_NONE ||
- !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST)))
+ tick_cpu = READ_ONCE(tick_do_timer_cpu);
+ if (tick_cpu != cpu &&
+ (tick_cpu != TICK_DO_TIMER_NONE || !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST)))
delta = KTIME_MAX;
/* Calculate the next expiry time */
@@ -970,6 +974,7 @@ static void tick_nohz_stop_tick(struct t
unsigned long basejiff = ts->last_jiffies;
u64 basemono = ts->timer_expires_base;
bool timer_idle = 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. */
@@ -1007,10 +1012,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 == tick_do_timer_cpu) {
- tick_do_timer_cpu = TICK_DO_TIMER_NONE;
+ tick_cpu = READ_ONCE(tick_do_timer_cpu);
+ if (tick_cpu == 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 != TICK_DO_TIMER_NONE) {
+ } else if (tick_cpu != TICK_DO_TIMER_NONE) {
tick_sched_flag_clear(ts, TS_FLAG_DO_TIMER_LAST);
}
@@ -1173,15 +1179,17 @@ static bool can_stop_idle_tick(int cpu,
return false;
if (tick_nohz_full_enabled()) {
+ int tick_cpu = 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 == cpu)
+ if (tick_cpu == cpu)
return false;
/* Should not happen for nohz-full */
- if (WARN_ON_ONCE(tick_do_timer_cpu == TICK_DO_TIMER_NONE))
+ if (WARN_ON_ONCE(tick_cpu == TICK_DO_TIMER_NONE))
return false;
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tip: timers/urgent] timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu
2024-04-09 10:29 ` timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu Thomas Gleixner
@ 2024-04-09 21:18 ` tip-bot2 for Thomas Gleixner
2024-04-10 8:15 ` Ingo Molnar
2024-04-10 8:22 ` tip-bot2 for Thomas Gleixner
1 sibling, 1 reply; 11+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2024-04-09 21:18 UTC (permalink / raw
To: linux-tip-commits
Cc: Mirsad Todorovac, Thomas Gleixner, Sean Anderson, x86,
linux-kernel
The following commit has been merged into the timers/urgent branch of tip:
Commit-ID: 0200ddd723d6056d0b1c1ebde4b11e75a0f2ed7e
Gitweb: https://git.kernel.org/tip/0200ddd723d6056d0b1c1ebde4b11e75a0f2ed7e
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 09 Apr 2024 12:29:12 +02:00
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 09 Apr 2024 23:09:44 +02:00
timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu
tick_do_timer_cpu is used lockless to check which CPU needs to take care
of the per tick timekeeping duty. This is done to avoid a thundering
herd problem on jiffies_lock.
The read and writes are not annotated so KCSAN complains about data races:
BUG: KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
write to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 26:
tick_nohz_idle_stop_tick+0x3b1/0x4a0
do_idle+0x1e3/0x250
read to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 16:
tick_nohz_next_event+0xe7/0x1e0
tick_nohz_get_sleep_length+0xa7/0xe0
menu_select+0x82/0xb90
cpuidle_select+0x44/0x60
do_idle+0x1c2/0x250
value changed: 0x0000001a -> 0xffffffff
Annotate them with READ/WRITE_ONCE() to document the intentional data race.
Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Sean Anderson <sean.anderson@seco.com>
Link: https://lore.kernel.org/r/87cyqy7rt3.ffs@tglx
---
kernel/time/tick-common.c | 17 +++++++++--------
kernel/time/tick-sched.c | 36 ++++++++++++++++++++++--------------
2 files changed, 31 insertions(+), 22 deletions(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index fb0fdec..a285f4d 100644
--- 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 == cpu) {
+ if (READ_ONCE(tick_do_timer_cpu) == cpu) {
raw_spin_lock(&jiffies_lock);
write_seqcount_begin(&jiffies_seq);
@@ -215,8 +216,8 @@ static void tick_setup_device(struct tick_device *td,
* If no cpu took the do_timer update, assign it to
* this cpu:
*/
- if (tick_do_timer_cpu == TICK_DO_TIMER_BOOT) {
- tick_do_timer_cpu = cpu;
+ if (READ_ONCE(tick_do_timer_cpu) == TICK_DO_TIMER_BOOT) {
+ WRITE_ONCE(tick_do_timer_cpu, cpu);
tick_next_period = ktime_get();
#ifdef CONFIG_NO_HZ_FULL
/*
@@ -232,7 +233,7 @@ static void tick_setup_device(struct tick_device *td,
!tick_nohz_full_cpu(cpu)) {
tick_take_do_timer_from_boot();
tick_do_timer_boot_cpu = -1;
- WARN_ON(tick_do_timer_cpu != cpu);
+ WARN_ON(READ_ON_ONCE(tick_do_timer_cpu) != cpu);
#endif
}
@@ -406,10 +407,10 @@ void tick_assert_timekeeping_handover(void)
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 == dying_cpu)
tick_do_timer_cpu = cpumask_first(cpu_online_mask);
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 1331216..71a792c 100644
--- 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(struct tick_sched *ts,
static void tick_sched_do_timer(struct tick_sched *ts, ktime_t now)
{
- int cpu = smp_processor_id();
+ int tick_cpu, cpu = 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 tick_sched *ts, ktime_t now)
* 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 == TICK_DO_TIMER_NONE)) {
+ tick_cpu = READ_ONCE(tick_do_timer_cpu);
+
+ if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && unlikely(tick_cpu == TICK_DO_TIMER_NONE)) {
#ifdef CONFIG_NO_HZ_FULL
WARN_ON_ONCE(tick_nohz_full_running);
#endif
- tick_do_timer_cpu = cpu;
+ WRITE_ONCE(tick_do_timer_cpu, cpu);
+ tick_cpu = cpu;
}
/* Check if jiffies need an update */
- if (tick_do_timer_cpu == cpu)
+ if (tick_cpu == cpu)
tick_do_update_jiffies64(now);
/*
@@ -610,7 +613,7 @@ bool tick_nohz_cpu_hotpluggable(unsigned int cpu)
* 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 == cpu)
+ if (tick_nohz_full_running && READ_ONCE(tick_do_timer_cpu) == cpu)
return false;
return true;
}
@@ -891,6 +894,7 @@ static ktime_t tick_nohz_next_event(struct tick_sched *ts, int cpu)
{
u64 basemono, next_tick, delta, expires;
unsigned long basejiff;
+ int tick_cpu;
basemono = get_jiffies_update(&basejiff);
ts->last_jiffies = basejiff;
@@ -947,9 +951,9 @@ static ktime_t tick_nohz_next_event(struct tick_sched *ts, int cpu)
* Otherwise we can sleep as long as we want.
*/
delta = timekeeping_max_deferment();
- if (cpu != tick_do_timer_cpu &&
- (tick_do_timer_cpu != TICK_DO_TIMER_NONE ||
- !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST)))
+ tick_cpu = READ_ONCE(tick_do_timer_cpu);
+ if (tick_cpu != cpu &&
+ (tick_cpu != TICK_DO_TIMER_NONE || !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST)))
delta = KTIME_MAX;
/* Calculate the next expiry time */
@@ -970,6 +974,7 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
unsigned long basejiff = ts->last_jiffies;
u64 basemono = ts->timer_expires_base;
bool timer_idle = 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. */
@@ -1007,10 +1012,11 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
* do_timer() never gets invoked. Keep track of the fact that it
* was the one which had the do_timer() duty last.
*/
- if (cpu == tick_do_timer_cpu) {
- tick_do_timer_cpu = TICK_DO_TIMER_NONE;
+ tick_cpu = READ_ONCE(tick_do_timer_cpu);
+ if (tick_cpu == 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 != TICK_DO_TIMER_NONE) {
+ } else if (tick_cpu != TICK_DO_TIMER_NONE) {
tick_sched_flag_clear(ts, TS_FLAG_DO_TIMER_LAST);
}
@@ -1173,15 +1179,17 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts)
return false;
if (tick_nohz_full_enabled()) {
+ int tick_cpu = 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 == cpu)
+ if (tick_cpu == cpu)
return false;
/* Should not happen for nohz-full */
- if (WARN_ON_ONCE(tick_do_timer_cpu == TICK_DO_TIMER_NONE))
+ if (WARN_ON_ONCE(tick_cpu == TICK_DO_TIMER_NONE))
return false;
}
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [tip: timers/urgent] timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu
2024-04-09 21:18 ` [tip: timers/urgent] " tip-bot2 for Thomas Gleixner
@ 2024-04-10 8:15 ` Ingo Molnar
2024-04-10 14:54 ` Thomas Gleixner
0 siblings, 1 reply; 11+ messages in thread
From: Ingo Molnar @ 2024-04-10 8:15 UTC (permalink / raw
To: linux-kernel
Cc: linux-tip-commits, Mirsad Todorovac, Thomas Gleixner,
Sean Anderson, x86
* tip-bot2 for Thomas Gleixner <tip-bot2@linutronix.de> wrote:
> The following commit has been merged into the timers/urgent branch of tip:
>
> Commit-ID: 0200ddd723d6056d0b1c1ebde4b11e75a0f2ed7e
> Gitweb: https://git.kernel.org/tip/0200ddd723d6056d0b1c1ebde4b11e75a0f2ed7e
> Author: Thomas Gleixner <tglx@linutronix.de>
> AuthorDate: Tue, 09 Apr 2024 12:29:12 +02:00
> Committer: Thomas Gleixner <tglx@linutronix.de>
> CommitterDate: Tue, 09 Apr 2024 23:09:44 +02:00
>
> timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu
> @@ -232,7 +233,7 @@ static void tick_setup_device(struct tick_device *td,
> !tick_nohz_full_cpu(cpu)) {
> tick_take_do_timer_from_boot();
> tick_do_timer_boot_cpu = -1;
> - WARN_ON(tick_do_timer_cpu != cpu);
> + WARN_ON(READ_ON_ONCE(tick_do_timer_cpu) != cpu);
So there's no such thing as READ_ON_ONCE() - I've amended the commit with
the fix below.
Thanks,
Ingo
kernel/time/tick-common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index a285f4deb96d..d88b13076b79 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -233,7 +233,7 @@ static void tick_setup_device(struct tick_device *td,
!tick_nohz_full_cpu(cpu)) {
tick_take_do_timer_from_boot();
tick_do_timer_boot_cpu = -1;
- WARN_ON(READ_ON_ONCE(tick_do_timer_cpu) != cpu);
+ WARN_ON(READ_ONCE(tick_do_timer_cpu) != cpu);
#endif
}
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [tip: timers/urgent] timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu
2024-04-09 10:29 ` timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu Thomas Gleixner
2024-04-09 21:18 ` [tip: timers/urgent] " tip-bot2 for Thomas Gleixner
@ 2024-04-10 8:22 ` tip-bot2 for Thomas Gleixner
1 sibling, 0 replies; 11+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2024-04-10 8:22 UTC (permalink / raw
To: linux-tip-commits
Cc: Mirsad Todorovac, Thomas Gleixner, Ingo Molnar, Sean Anderson,
x86, linux-kernel
The following commit has been merged into the timers/urgent branch of tip:
Commit-ID: f87cbcb345d059f0377b4fa0ba1b766a17fc3710
Gitweb: https://git.kernel.org/tip/f87cbcb345d059f0377b4fa0ba1b766a17fc3710
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 09 Apr 2024 12:29:12 +02:00
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 10 Apr 2024 10:13:42 +02:00
timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu
tick_do_timer_cpu is used lockless to check which CPU needs to take care
of the per tick timekeeping duty. This is done to avoid a thundering
herd problem on jiffies_lock.
The read and writes are not annotated so KCSAN complains about data races:
BUG: KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event
write to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 26:
tick_nohz_idle_stop_tick+0x3b1/0x4a0
do_idle+0x1e3/0x250
read to 0xffffffff8a2bda30 of 4 bytes by task 0 on cpu 16:
tick_nohz_next_event+0xe7/0x1e0
tick_nohz_get_sleep_length+0xa7/0xe0
menu_select+0x82/0xb90
cpuidle_select+0x44/0x60
do_idle+0x1c2/0x250
value changed: 0x0000001a -> 0xffffffff
Annotate them with READ/WRITE_ONCE() to document the intentional data race.
Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Tested-by: Sean Anderson <sean.anderson@seco.com>
Link: https://lore.kernel.org/r/87cyqy7rt3.ffs@tglx
---
kernel/time/tick-common.c | 17 +++++++++--------
kernel/time/tick-sched.c | 36 ++++++++++++++++++++++--------------
2 files changed, 31 insertions(+), 22 deletions(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index fb0fdec..d88b130 100644
--- 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 == cpu) {
+ if (READ_ONCE(tick_do_timer_cpu) == cpu) {
raw_spin_lock(&jiffies_lock);
write_seqcount_begin(&jiffies_seq);
@@ -215,8 +216,8 @@ static void tick_setup_device(struct tick_device *td,
* If no cpu took the do_timer update, assign it to
* this cpu:
*/
- if (tick_do_timer_cpu == TICK_DO_TIMER_BOOT) {
- tick_do_timer_cpu = cpu;
+ if (READ_ONCE(tick_do_timer_cpu) == TICK_DO_TIMER_BOOT) {
+ WRITE_ONCE(tick_do_timer_cpu, cpu);
tick_next_period = ktime_get();
#ifdef CONFIG_NO_HZ_FULL
/*
@@ -232,7 +233,7 @@ static void tick_setup_device(struct tick_device *td,
!tick_nohz_full_cpu(cpu)) {
tick_take_do_timer_from_boot();
tick_do_timer_boot_cpu = -1;
- WARN_ON(tick_do_timer_cpu != cpu);
+ WARN_ON(READ_ONCE(tick_do_timer_cpu) != cpu);
#endif
}
@@ -406,10 +407,10 @@ void tick_assert_timekeeping_handover(void)
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 == dying_cpu)
tick_do_timer_cpu = cpumask_first(cpu_online_mask);
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 1331216..71a792c 100644
--- 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(struct tick_sched *ts,
static void tick_sched_do_timer(struct tick_sched *ts, ktime_t now)
{
- int cpu = smp_processor_id();
+ int tick_cpu, cpu = 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 tick_sched *ts, ktime_t now)
* 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 == TICK_DO_TIMER_NONE)) {
+ tick_cpu = READ_ONCE(tick_do_timer_cpu);
+
+ if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && unlikely(tick_cpu == TICK_DO_TIMER_NONE)) {
#ifdef CONFIG_NO_HZ_FULL
WARN_ON_ONCE(tick_nohz_full_running);
#endif
- tick_do_timer_cpu = cpu;
+ WRITE_ONCE(tick_do_timer_cpu, cpu);
+ tick_cpu = cpu;
}
/* Check if jiffies need an update */
- if (tick_do_timer_cpu == cpu)
+ if (tick_cpu == cpu)
tick_do_update_jiffies64(now);
/*
@@ -610,7 +613,7 @@ bool tick_nohz_cpu_hotpluggable(unsigned int cpu)
* 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 == cpu)
+ if (tick_nohz_full_running && READ_ONCE(tick_do_timer_cpu) == cpu)
return false;
return true;
}
@@ -891,6 +894,7 @@ static ktime_t tick_nohz_next_event(struct tick_sched *ts, int cpu)
{
u64 basemono, next_tick, delta, expires;
unsigned long basejiff;
+ int tick_cpu;
basemono = get_jiffies_update(&basejiff);
ts->last_jiffies = basejiff;
@@ -947,9 +951,9 @@ static ktime_t tick_nohz_next_event(struct tick_sched *ts, int cpu)
* Otherwise we can sleep as long as we want.
*/
delta = timekeeping_max_deferment();
- if (cpu != tick_do_timer_cpu &&
- (tick_do_timer_cpu != TICK_DO_TIMER_NONE ||
- !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST)))
+ tick_cpu = READ_ONCE(tick_do_timer_cpu);
+ if (tick_cpu != cpu &&
+ (tick_cpu != TICK_DO_TIMER_NONE || !tick_sched_flag_test(ts, TS_FLAG_DO_TIMER_LAST)))
delta = KTIME_MAX;
/* Calculate the next expiry time */
@@ -970,6 +974,7 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
unsigned long basejiff = ts->last_jiffies;
u64 basemono = ts->timer_expires_base;
bool timer_idle = 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. */
@@ -1007,10 +1012,11 @@ static void tick_nohz_stop_tick(struct tick_sched *ts, int cpu)
* do_timer() never gets invoked. Keep track of the fact that it
* was the one which had the do_timer() duty last.
*/
- if (cpu == tick_do_timer_cpu) {
- tick_do_timer_cpu = TICK_DO_TIMER_NONE;
+ tick_cpu = READ_ONCE(tick_do_timer_cpu);
+ if (tick_cpu == 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 != TICK_DO_TIMER_NONE) {
+ } else if (tick_cpu != TICK_DO_TIMER_NONE) {
tick_sched_flag_clear(ts, TS_FLAG_DO_TIMER_LAST);
}
@@ -1173,15 +1179,17 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts)
return false;
if (tick_nohz_full_enabled()) {
+ int tick_cpu = 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 == cpu)
+ if (tick_cpu == cpu)
return false;
/* Should not happen for nohz-full */
- if (WARN_ON_ONCE(tick_do_timer_cpu == TICK_DO_TIMER_NONE))
+ if (WARN_ON_ONCE(tick_cpu == TICK_DO_TIMER_NONE))
return false;
}
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [tip: timers/urgent] timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu
2024-04-10 8:15 ` Ingo Molnar
@ 2024-04-10 14:54 ` Thomas Gleixner
0 siblings, 0 replies; 11+ messages in thread
From: Thomas Gleixner @ 2024-04-10 14:54 UTC (permalink / raw
To: Ingo Molnar, linux-kernel
Cc: linux-tip-commits, Mirsad Todorovac, Sean Anderson, x86
On Wed, Apr 10 2024 at 10:15, Ingo Molnar wrote:
>> @@ -232,7 +233,7 @@ static void tick_setup_device(struct tick_device *td,
>> !tick_nohz_full_cpu(cpu)) {
>> tick_take_do_timer_from_boot();
>> tick_do_timer_boot_cpu = -1;
>> - WARN_ON(tick_do_timer_cpu != cpu);
>> + WARN_ON(READ_ON_ONCE(tick_do_timer_cpu) != cpu);
>
> So there's no such thing as READ_ON_ONCE() - I've amended the commit with
> the fix below.
Can you hand me a brown paper bag please?
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-04-10 14:54 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-17 19:32 [BUG] KCSAN: data-race in tick_nohz_idle_stop_tick / tick_nohz_next_event Mirsad Todorovac
2023-08-18 14:15 ` Mirsad Todorovac
2024-03-07 19:43 ` Sean Anderson
2024-03-09 16:13 ` Mirsad Todorovac
2024-03-11 14:48 ` Thomas Gleixner
2024-03-11 16:30 ` Sean Anderson
2024-04-09 10:29 ` timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu Thomas Gleixner
2024-04-09 21:18 ` [tip: timers/urgent] " tip-bot2 for Thomas Gleixner
2024-04-10 8:15 ` Ingo Molnar
2024-04-10 14:54 ` Thomas Gleixner
2024-04-10 8:22 ` tip-bot2 for Thomas Gleixner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).