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