On Mon, Apr 24, 2023 at 3:45 PM Jens Axboe <axboe@xxxxxxxxx> wrote:
>
> Something like this. Not tested yet, but wanted to get your feedback
> early to avoid issues down the line.
Ok, that try_cmpxchg() loop looks odd, but I guess it's the right thing to do.
That said, you should move the
old_fmode = READ_ONCE(file->f_mode);
to outside the loop, because try_cmpxchg() will then update
'old_fmode' to the proper value and it should *not* be done again.
I also suspect that the READ_ONCE() itself is entirely superfluous,
because that is very much a value that we should let the compiler
optimize to *not* have to access more than it needs to.
So if the compiler had an earlier copy of that value, it should just
use it, rather than us forcing it to read it again.
But I suspect in this case it makes no real difference to code
generation. There's nothing else around it that uses f_mode, afaik,
and the try_cmpxchg() will reload it properly to fix any possible
races up.
The READ_ONCE() would possibly make more sense if we actually expected
that FMODE_NOWAIT bit to change more than once, but then we'd
presuably have some ordering rule and it should be a
smp_load_acquire() or whatever.
As it is, if we ever see it clear, we don't care any more, and the
real value consistency guarantee is in the try_cmpxchg itself. There
are no possible races ot mis-readings that could matter.
So I think it could/should just be something like
void pipe_clear_nowait(struct file *file)
{
fmode_t fmode = file->f_mode;
do {
if (!(fmode & FMODE_NOWAIT))
break;
} while (!try_cmpxchg(&file->f_mode, &fmode, fmode & ~FMODE_NOWAIT));
}
which sadly generates that big constant just because FMODE_NOWAIT is
up in the high bits and with the 'try_cmpxchg()', the compiler has no
choice but to get the full 32-bit value anyway.
Linus
