2024-02-25 03:13:05

by linke li

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

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 <[email protected]>
---
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)

2024-03-07 21:15:30

by Thomas Gleixner

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

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

2024-03-08 08:59:10

by linke li

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

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.