On Mon, 3 Mar 2025 at 07:55, Mateusz Guzik <mjguzik@xxxxxxxxx> wrote:
>
> Can you guys try out the patch below?
>
> It changes things up so that there is no need to read 2 different vars.
No, please don't do it this way.
I think the memory ordering is interesting, and we ignored it -
incorrectly - because all the "normal" cases are done either under the
pipe lock (safe), or are done with "wait_event()" that will retry on
wakeups.
And then we got the subtle issues with "was woken, but raced with
order of operations" case got missed. This has probably been around
forever (possibly since we got rid of the BKL).
But I don't like the "add separate full/empty fields that duplicate
things", just to have those written always under the lock, and then
loaded as one op.
I think there are better models.
So I think I'd prefer the "add the barrier" model.
We could also possibly just make head/tail be 16-bit fields, and then
read things atomically by reading them as a single 32-bit word. That
would expose the (existing) alpha issues more, since alpha doesn't
have atomic 16-bit writes, but I can't find it in myself to care. I
guess we could make it be two aligned 32-bit fields on alpha, and just
use 64-bit reads.
We already treat those fields specially with the whole READ_ONCE()
dance, so treating them even more specially would not be a noticeably
different situation.
Hmm?
I just generally dislike redundant information in data structures.
Then you get into nasty cases where some path forgets to update the
redundant fields correctly. So I'd really just prefer the existing
model, just with being careful about this case.
Linus
