On Wed, 6 Mar 2024 at 11:45, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote: > > Here's the back story. I received the following patch: > > https://lore.kernel.org/all/tencent_BA1473492BC618B473864561EA3AB1418908@xxxxxx/ > > 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
