[PATCH] tick: use READ_ONCE() to read jiffies in concurrent environment

[PATCH] tick: use READ_ONCE() to read jiffies in concurrent environment

[linke li]

In function tick_sched_do_timer(), jiffies is read using READ_ONCE()
in line 224, while read directly in line 217

217	if (ts->last_tick_jiffies != jiffies) {
218		ts->stalled_jiffies = 0;
219		ts->last_tick_jiffies = READ_ONCE(jiffies);
220	} else {
221		if (++ts->stalled_jiffies == MAX_STALLED_JIFFIES) {
222			tick_do_update_jiffies64(now);
223			ts->stalled_jiffies = 0;
224			ts->last_tick_jiffies = READ_ONCE(jiffies);
225		}
226	}

There is patch similar to this. commit c1c0ce31b242 ("r8169: fix the KCSAN reported data-race in rtl_tx() while reading tp->cur_tx")
This patch find two read of same variable while one is protected, another
is not. And READ_ONCE() is added to protect.

Signed-off-by: linke li <lilinke99@qq.com>
---
 kernel/time/tick-sched.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 01fb50c1b17e..aa684511c25a 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -214,7 +214,7 @@ static void tick_sched_do_timer(struct tick_sched *ts, ktime_t now)
 	 * If the jiffies update stalled for too long (timekeeper in stop_machine()
 	 * or VMEXIT'ed for several msecs), force an update.
 	 */
-	if (ts->last_tick_jiffies != jiffies) {
+	if (ts->last_tick_jiffies != READ_ONCE(jiffies)) {
 		ts->stalled_jiffies = 0;
 		ts->last_tick_jiffies = READ_ONCE(jiffies);
 	} else {
-- 
2.39.3 (Apple Git-145)

Re: [PATCH] tick: use READ_ONCE() to read jiffies in concurrent environment

[Thomas Gleixner]

On Sun, Feb 25 2024 at 11:12, linke li wrote:
> In function tick_sched_do_timer(), jiffies is read using READ_ONCE()
> in line 224, while read directly in line 217
>
> 217	if (ts->last_tick_jiffies != jiffies) {
> 218		ts->stalled_jiffies = 0;
> 219		ts->last_tick_jiffies = READ_ONCE(jiffies);
> 220	} else {
> 221		if (++ts->stalled_jiffies == MAX_STALLED_JIFFIES) {
> 222			tick_do_update_jiffies64(now);
> 223			ts->stalled_jiffies = 0;
> 224			ts->last_tick_jiffies = READ_ONCE(jiffies);
> 225		}
> 226	}

Please do not paste the code which is changed by the patch into the
changelog. Describe the problem you are trying to solve.

> There is patch similar to this. commit c1c0ce31b242 ("r8169: fix the
> KCSAN reported data-race in rtl_tx() while reading tp->cur_tx")

The other patch has absolutely nothing to do with this code and . Describe
the problem and the solution.

> This patch find two read of same variable while one is protected, another
> is not. And READ_ONCE() is added to protect.

This patch finds nothing. Explain it correctly why it matters that the
first read is not marked READ_ONCE(). Is this solving a correctness
problem or are you adding it just to shut up the KCSAN warning?

> @@ -214,7 +214,7 @@ static void tick_sched_do_timer(struct tick_sched *ts, ktime_t now)
>  	 * If the jiffies update stalled for too long (timekeeper in stop_machine()
>  	 * or VMEXIT'ed for several msecs), force an update.
>  	 */
> -	if (ts->last_tick_jiffies != jiffies) {
> +	if (ts->last_tick_jiffies != READ_ONCE(jiffies)) {
>  		ts->stalled_jiffies = 0;
>  		ts->last_tick_jiffies = READ_ONCE(jiffies);
>  	} else {

Thanks,

        tglx

Re: [PATCH] tick: use READ_ONCE() to read jiffies in concurrent environment

[linke li]

I am really sorry for making a confusing commit log.

>
> This patch finds nothing. Explain it correctly why it matters that the
> first read is not marked READ_ONCE(). Is this solving a correctness
> problem or are you adding it just to shut up the KCSAN warning?
>

The first read on jiffies is a racy read, and is expected to be racy.
So Mark data races to pwd->triggered as benign using READ_ONCE. This 
patch aiming at reducing the number of benign races reported by KCSAN
in order to focus future debugging effort on harmful races.