linux-kernel - Re: [GIT PULL] pipe: nonblocking rw for io

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f9616b01-b185-a2a5-dbc9-a45735ed7e1b@kernel.dk>
Date:   Tue, 25 Apr 2023 07:46:51 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Christian Brauner <brauner@...nel.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] pipe: nonblocking rw for io_uring

On 4/24/23 9:16?PM, Linus Torvalds wrote:
> On Mon, Apr 24, 2023 at 3:45?PM Jens Axboe <axboe@...nel.dk> 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.

I'll go with this, it's definitely simpler. I suspected the important
bit was just doing the cmpxchg sanely and that the READ_ONCE() was
superflous given how it's used, and dropping the old_fmode is cleaner.

FWIW, I don't see any difference in code generation here on arm64 or
x86-64 if FMODE_NOWAIT is in the lower bits, as we could've just moved
it. We could make it the sign bit which might make the first compare
faster in general, but honestly don't think we really care that deeply
about that.

Updated the branch, it's:

https://git.kernel.dk/cgit/linux/log/?h=pipe-nonblock.2

It's just the cmpxchg patch now and the same "set FMODE_NOWAIT on pipes
unconditionally" from before.

-- 
Jens Axboe