[PATCH] timers/nohz: Annotate lockless accesses to got_idle_tick

[PATCH] timers/nohz: Annotate lockless accesses to got_idle_tick

[Kunwu Chan]

tick_sched_do_timer(), called from the tick interrupt handler, sets
ts->got_idle_tick when a tick fires while the CPU is idle.  The idle
path reads and clears it via tick_nohz_idle_got_tick() to detect
whether the tick handler has run.

The flag is accessed locklessly from hardirq and task context.  A
concurrent set can be overwritten by the clear.

Use READ_ONCE() and WRITE_ONCE() to annotate the lockless accesses.

Signed-off-by: Kunwu Chan <kunwu.chan@gmail.com>
---
 kernel/time/tick-sched.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 6c3fea386713..c0ab32e543b3 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -269,7 +269,7 @@ static void tick_sched_do_timer(struct tick_sched *ts, ktime_t now)
 	}
 
 	if (tick_sched_flag_test(ts, TS_FLAG_INIDLE))
-		ts->got_idle_tick = 1;
+		WRITE_ONCE(ts->got_idle_tick, 1);
 }
 
 static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs)
@@ -1248,8 +1248,8 @@ bool tick_nohz_idle_got_tick(void)
 {
 	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
 
-	if (ts->got_idle_tick) {
-		ts->got_idle_tick = 0;
+	if (READ_ONCE(ts->got_idle_tick)) {
+		WRITE_ONCE(ts->got_idle_tick, 0);
 		return true;
 	}
 	return false;
-- 
2.43.0

Re: [PATCH] timers/nohz: Annotate lockless accesses to got_idle_tick

[Thomas Gleixner]

On Sun, Sep 20 2026 at 16:54, Kunwu Chan wrote:
> tick_sched_do_timer(), called from the tick interrupt handler, sets
> ts->got_idle_tick when a tick fires while the CPU is idle.  The idle
> path reads and clears it via tick_nohz_idle_got_tick() to detect
> whether the tick handler has run.
>
> The flag is accessed locklessly from hardirq and task context.  A
> concurrent set can be overwritten by the clear.
>
> Use READ_ONCE() and WRITE_ONCE() to annotate the lockless accesses.

The following scenarios still happen:

A)

              if (READ_ONCE(got_tick))      // reads true
-> interrupt       
       WRITE_ONCE(got_tick, true);   

                   WRITE_ONCE(got_tick, false);

B)

              if (READ_ONCE(got_tick))      // reads false
-> interrupt       
       WRITE_ONCE(got_tick, true);   

#A is harmless because got_tick is already true and #B is harmless as
 well because the got_tick read result is only a hint for the next idle
 invocation.

As this is stricly per CPU the READ/WRITE_ONCE() does not change
anything in terms of ordering. So what exactly is solved by the
READ/WRITE_ONCE()?

Thanks,

        tglx

Re: [PATCH] timers/nohz: Annotate lockless accesses to got_idle_tick

[Paul E. McKenney]

On Tue, Sep 29, 2026 at 02:21:32PM +0200, Thomas Gleixner wrote:
> On Sun, Sep 20 2026 at 16:54, Kunwu Chan wrote:
> > tick_sched_do_timer(), called from the tick interrupt handler, sets
> > ts->got_idle_tick when a tick fires while the CPU is idle.  The idle
> > path reads and clears it via tick_nohz_idle_got_tick() to detect
> > whether the tick handler has run.
> >
> > The flag is accessed locklessly from hardirq and task context.  A
> > concurrent set can be overwritten by the clear.
> >
> > Use READ_ONCE() and WRITE_ONCE() to annotate the lockless accesses.
> 
> The following scenarios still happen:
> 
> A)
> 
>               if (READ_ONCE(got_tick))      // reads true
> -> interrupt       
>        WRITE_ONCE(got_tick, true);   
> 
>                    WRITE_ONCE(got_tick, false);
> 
> B)
> 
>               if (READ_ONCE(got_tick))      // reads false
> -> interrupt       
>        WRITE_ONCE(got_tick, true);   
> 
> #A is harmless because got_tick is already true and #B is harmless as
>  well because the got_tick read result is only a hint for the next idle
>  invocation.
> 
> As this is stricly per CPU the READ/WRITE_ONCE() does not change
> anything in terms of ordering. So what exactly is solved by the
> READ/WRITE_ONCE()?

This is a step towards allowing more-strict KCSAN checks to be enabled,
namely data races between base code and interrupt handlers.  KCSAN has
found a few bugs of this type in RCU over the past year or so, which
suggests that similar bugs might be lurking elsewhere.

Yes, in this particular case, current compilers don't have a huge amount
of freedom to mess things up, but the standard really does permit the
compiler to use a to-be-stored-to location as a temporary just prior to
that store.

This is like the situation with lockdep, where we tell it about
deadlock-free situations that it does not see as being deadlock-free.

Seem reasonable?

							Thanx, Paul

Re: [PATCH] timers/nohz: Annotate lockless accesses to got_idle_tick

[Thomas Gleixner]

On Wed, Sep 30 2026 at 13:00, Paul E. McKenney wrote:
> On Tue, Sep 29, 2026 at 02:21:32PM +0200, Thomas Gleixner wrote:
>> On Sun, Sep 20 2026 at 16:54, Kunwu Chan wrote:
>> > tick_sched_do_timer(), called from the tick interrupt handler, sets
>> > ts->got_idle_tick when a tick fires while the CPU is idle.  The idle
>> > path reads and clears it via tick_nohz_idle_got_tick() to detect
>> > whether the tick handler has run.
>> >
>> > The flag is accessed locklessly from hardirq and task context.  A
>> > concurrent set can be overwritten by the clear.
>> >
>> > Use READ_ONCE() and WRITE_ONCE() to annotate the lockless accesses.
>> 
>> The following scenarios still happen:
>> 
>> A)
>> 
>>               if (READ_ONCE(got_tick))      // reads true
>> -> interrupt       
>>        WRITE_ONCE(got_tick, true);   
>> 
>>                    WRITE_ONCE(got_tick, false);
>> 
>> B)
>> 
>>               if (READ_ONCE(got_tick))      // reads false
>> -> interrupt       
>>        WRITE_ONCE(got_tick, true);   
>> 
>> #A is harmless because got_tick is already true and #B is harmless as
>>  well because the got_tick read result is only a hint for the next idle
>>  invocation.
>> 
>> As this is stricly per CPU the READ/WRITE_ONCE() does not change
>> anything in terms of ordering. So what exactly is solved by the
>> READ/WRITE_ONCE()?
>
> This is a step towards allowing more-strict KCSAN checks to be enabled,
> namely data races between base code and interrupt handlers.  KCSAN has
> found a few bugs of this type in RCU over the past year or so, which
> suggests that similar bugs might be lurking elsewhere.
>
> Yes, in this particular case, current compilers don't have a huge amount
> of freedom to mess things up, but the standard really does permit the
> compiler to use a to-be-stored-to location as a temporary just prior to
> that store.
>
> This is like the situation with lockdep, where we tell it about
> deadlock-free situations that it does not see as being deadlock-free.
>
> Seem reasonable?

Yes, but then the change log should clearly state exactly that along
with a corresponding comment in the code. These boilerplate copy&pasta
changes are annoying as hell because they fail to provide any reasonable
rationale for the proposed change. See Documentation/process/...

I'm refusing to accept this slop whether it's generated by humans or any
form of tools.

That said, I further have to say that just reusing READ/WRITE_ONCE() for
this is completely wrong.

A strict and harmless per CPU modification/race is very much different
from cross CPU races. So avoiding any potential confusion and thereby
allowing tools like KCSAN to treat them differently is useful.

Using the same annotation for something which is strictly a per CPU
situation prevents to detect the accidential cross CPU access which
might not be safe at all.

Even if the momentary implementation falls back to the same mechanism
for the tools the value of implied code documentation is valuable. It
avoids extensive comments and it also allows tools to differentiate the
meaning in the future.

READ/WRITE_ONCE() are just non-distinguishable "paper over all race
problems which tools might complain about" hammers.

But we all should know by now that "Birmingham screwdrivers" are more
than suboptimal.

Thanks,

        tglx

Re: [PATCH] timers/nohz: Annotate lockless accesses to got_idle_tick

[Paul E. McKenney]

[ Adding Marco Elver on CC. ]

On Thu, Oct 01, 2026 at 12:16:45AM +0200, Thomas Gleixner wrote:
> On Wed, Sep 30 2026 at 13:00, Paul E. McKenney wrote:
> > On Tue, Sep 29, 2026 at 02:21:32PM +0200, Thomas Gleixner wrote:
> >> On Sun, Sep 20 2026 at 16:54, Kunwu Chan wrote:
> >> > tick_sched_do_timer(), called from the tick interrupt handler, sets
> >> > ts->got_idle_tick when a tick fires while the CPU is idle.  The idle
> >> > path reads and clears it via tick_nohz_idle_got_tick() to detect
> >> > whether the tick handler has run.
> >> >
> >> > The flag is accessed locklessly from hardirq and task context.  A
> >> > concurrent set can be overwritten by the clear.
> >> >
> >> > Use READ_ONCE() and WRITE_ONCE() to annotate the lockless accesses.
> >> 
> >> The following scenarios still happen:
> >> 
> >> A)
> >> 
> >>               if (READ_ONCE(got_tick))      // reads true
> >> -> interrupt       
> >>        WRITE_ONCE(got_tick, true);   
> >> 
> >>                    WRITE_ONCE(got_tick, false);
> >> 
> >> B)
> >> 
> >>               if (READ_ONCE(got_tick))      // reads false
> >> -> interrupt       
> >>        WRITE_ONCE(got_tick, true);   
> >> 
> >> #A is harmless because got_tick is already true and #B is harmless as
> >>  well because the got_tick read result is only a hint for the next idle
> >>  invocation.
> >> 
> >> As this is stricly per CPU the READ/WRITE_ONCE() does not change
> >> anything in terms of ordering. So what exactly is solved by the
> >> READ/WRITE_ONCE()?
> >
> > This is a step towards allowing more-strict KCSAN checks to be enabled,
> > namely data races between base code and interrupt handlers.  KCSAN has
> > found a few bugs of this type in RCU over the past year or so, which
> > suggests that similar bugs might be lurking elsewhere.
> >
> > Yes, in this particular case, current compilers don't have a huge amount
> > of freedom to mess things up, but the standard really does permit the
> > compiler to use a to-be-stored-to location as a temporary just prior to
> > that store.
> >
> > This is like the situation with lockdep, where we tell it about
> > deadlock-free situations that it does not see as being deadlock-free.
> >
> > Seem reasonable?
> 
> Yes, but then the change log should clearly state exactly that along
> with a corresponding comment in the code. These boilerplate copy&pasta
> changes are annoying as hell because they fail to provide any reasonable
> rationale for the proposed change. See Documentation/process/...
> 
> I'm refusing to accept this slop whether it's generated by humans or any
> form of tools.
> 
> That said, I further have to say that just reusing READ/WRITE_ONCE() for
> this is completely wrong.
> 
> A strict and harmless per CPU modification/race is very much different
> from cross CPU races. So avoiding any potential confusion and thereby
> allowing tools like KCSAN to treat them differently is useful.
> 
> Using the same annotation for something which is strictly a per CPU
> situation prevents to detect the accidential cross CPU access which
> might not be safe at all.
> 
> Even if the momentary implementation falls back to the same mechanism
> for the tools the value of implied code documentation is valuable. It
> avoids extensive comments and it also allows tools to differentiate the
> meaning in the future.
> 
> READ/WRITE_ONCE() are just non-distinguishable "paper over all race
> problems which tools might complain about" hammers.
> 
> But we all should know by now that "Birmingham screwdrivers" are more
> than suboptimal.

Yeah, if you hit something with a Birmingham screwdriver, it might not
quite realize that it has been hit.  On the other hand, if you instead use
a 16-pound (7 1/4 kg) sledgehammer, at least the fool thing will *know*
that it has been hit.  ;-)

Back to the topic at hand...

So your thought is to have a KCSAN annotation that says "should be accessed
only from the corresponding CPU".  Or maybe "from no more than one CPU".

That would certainly be useful, easy though it is for me to say.

Marco, is something like this practical for KCSAN?

There are quite a few variations, including "written from one CPU but
read from everywhere" and vice versa.  But maybe start with the things
actually proven useful.  ;-)

							Thanx, Paul

Re: [PATCH] timers/nohz: Annotate lockless accesses to got_idle_tick

[Thomas Gleixner]

On Wed, Sep 30 2026 at 15:30, Paul E. McKenney wrote:
> On Thu, Oct 01, 2026 at 12:16:45AM +0200, Thomas Gleixner wrote:
>> That said, I further have to say that just reusing READ/WRITE_ONCE() for
>> this is completely wrong.
>> 
>> A strict and harmless per CPU modification/race is very much different
>> from cross CPU races. So avoiding any potential confusion and thereby
>> allowing tools like KCSAN to treat them differently is useful.
>> 
>> Using the same annotation for something which is strictly a per CPU
>> situation prevents to detect the accidential cross CPU access which
>> might not be safe at all.
>> 
>> Even if the momentary implementation falls back to the same mechanism
>> for the tools the value of implied code documentation is valuable. It
>> avoids extensive comments and it also allows tools to differentiate the
>> meaning in the future.
>> 
>> READ/WRITE_ONCE() are just non-distinguishable "paper over all race
>> problems which tools might complain about" hammers.
>> 
>> But we all should know by now that "Birmingham screwdrivers" are more
>> than suboptimal.
>
> Yeah, if you hit something with a Birmingham screwdriver, it might not
> quite realize that it has been hit.  On the other hand, if you instead use
> a 16-pound (7 1/4 kg) sledgehammer, at least the fool thing will *know*
> that it has been hit.  ;-)
>
> Back to the topic at hand...
>
> So your thought is to have a KCSAN annotation that says "should be accessed
> only from the corresponding CPU".  Or maybe "from no more than one CPU".
>
> That would certainly be useful, easy though it is for me to say.
>
> Marco, is something like this practical for KCSAN?
>
> There are quite a few variations, including "written from one CPU but
> read from everywhere" and vice versa.  But maybe start with the things
> actually proven useful.  ;-)

Well, written from one CPU and read from everywhere is what
READ/WRITE_ONCE() is for just with the extra twist that the WRITE is
bound to a single specific CPU. But that's not the problem at hand.

What I meant is to have something like this:

        WRITE_ONCE_THIS_CPU(...)
        READ_ONCE_THIS_CPU(...)

where both operate on the same per CPU variable and the race is between
different contexts, e.g. task and interrupt.

That's first of all useful as documentation for the reader and also for
static analysis tools.

As long as KCSAN cannot make use of it the macros simply fall back to
the existing WRITE/READ_ONCE() so the problem which the patch under
discussion is trying to solve is addressed. When some future KCSAN
implementation can make this a special case with actual per CPU checks
then we get even more benefit.

Thanks,

        tglx