[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHk-=whs5MdtNjzFkTyaUy=vHi=qwWgPi0JgTe6OYUYMNSRZfg@mail.gmail.com>
Date: Wed, 6 Mar 2024 12:06:00 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: "Paul E. McKenney" <paulmck@...nel.org>, linke li <lilinke99@...com>, joel@...lfernandes.org,
boqun.feng@...il.com, dave@...olabs.net, frederic@...nel.org,
jiangshanlai@...il.com, josh@...htriplett.org, linux-kernel@...r.kernel.org,
mathieu.desnoyers@...icios.com, qiang.zhang1211@...il.com,
quic_neeraju@...cinc.com, rcu@...r.kernel.org
Subject: Re: [PATCH] rcutorture: Fix rcu_torture_pipe_update_one()/rcu_torture_writer()
data race and concurrency bug
On Wed, 6 Mar 2024 at 11:45, Steven Rostedt <rostedt@...dmis.org> wrote:
>
> Here's the back story. I received the following patch:
>
> https://lore.kernel.org/all/tencent_BA1473492BC618B473864561EA3AB1418908@qq.com/
>
> I didn't like it. My reply was:
>
> > - rbwork->wait_index++;
> > + WRITE_ONCE(rbwork->wait_index, READ_ONCE(rbwork->wait_index) + 1);
>
> I mean the above is really ugly. If this is the new thing to do, we need
> better macros.
>
> If anything, just convert it to an atomic_t.
The right thing is definitely to convert it to an atomic_t.
The memory barriers can probably also be turned into atomic ordering,
although we don't always have all the variates.
But for example, that
/* Make sure to see the new wait index */
smp_rmb();
if (wait_index != work->wait_index)
break;
looks odd, and should probably do an "atomic_read_acquire()" instead
of a rmb and a (non-atomic and non-READ_ONCE thing).
The first READ_ONCE() should probably also be that atomic_read_acquire() op.
On the writing side, my gut feel is that the
rbwork->wait_index++;
/* make sure the waiters see the new index */
smp_wmb();
should be an "atomic_inc_release(&rbwork->wait_index);" but we don't
actually have that operation. We only have the "release" versions for
things that return a value.
So it would probably need to be either
atomic_inc(&rbwork->wait_index);
/* make sure the waiters see the new index */
smp_wmb();
or
atomic_inc_return_release(&rbwork->wait_index);
or we'd need to add the "basic atomics with ordering semantics" (which
we aren't going to do unless we end up with a lot more people who want
them).
I dunno. I didn't look all *that* closely at the code. The above might
be garbage too. Somebody who actually knows the code should think
about what ordering they actually were looking for.
(And I note that 'wait_index' is of type 'long' in 'struct
rb_irq_work', so I guess it should be "atomic_long_t" instead - just
shows how little attention I paid on the first read-through, which
should make everybody go "I need to double-check Linus here")
Linus
Powered by blists - more mailing lists