[GIT PULL] pipe: nonblocking rw for io_uring

[GIT PULL] pipe: nonblocking rw for io_uring

[Christian Brauner]

Hey Linus,

/* Summary */
This contains Jens' work to support FMODE_NOWAIT and thus IOCB_NOWAIT
for pipes ensuring that all places can deal with non-blocking requests.

To this end, pass down the information that this is a nonblocking
request so that pipe locking, allocation, and buffer checking correctly
deal with those.

The series is small but it felt standalone enough that I didn't want to
lump it together with other, generic vfs work.

/* Testing */
clang: Ubuntu clang version 15.0.6
gcc: (Ubuntu 12.2.0-3ubuntu1) 12.2.0

All patches are based on 6.3-rc2 and have been sitting in linux-next.
No build failures or warnings were observed. All old and new tests in
fstests, selftests, and LTP pass without regressions.

/* Conflicts */
At the time of creating this PR no merge conflicts were reported from
linux-next and no merge conflicts showed up doing a test-merge with
current mainline.

The following changes since commit eeac8ede17557680855031c6f305ece2378af326:

  Linux 6.3-rc2 (2023-03-12 16:36:44 -0700)

are available in the Git repository at:

  git@gitolite.kernel.org:pub/scm/linux/kernel/git/vfs/vfs tags/v6.4/vfs.pipe

for you to fetch changes up to ec30adeb289d9054efae4e285b269438ce63fe03:

  pipe: set FMODE_NOWAIT on pipes (2023-03-15 11:37:29 -0600)

Please consider pulling these changes from the signed v6.4/vfs.pipe tag.

Thanks!
Christian

----------------------------------------------------------------
v6.4/vfs.pipe

----------------------------------------------------------------
Jens Axboe (3):
      fs: add 'nonblock' parameter to pipe_buf_confirm() and fops method
      pipe: enable handling of IOCB_NOWAIT
      pipe: set FMODE_NOWAIT on pipes

 fs/fuse/dev.c             |  4 ++--
 fs/pipe.c                 | 42 ++++++++++++++++++++++++++++++++++--------
 fs/splice.c               | 11 +++++++----
 include/linux/pipe_fs_i.h |  8 +++++---
 4 files changed, 48 insertions(+), 17 deletions(-)

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Linus Torvalds]

On Fri, Apr 21, 2023 at 7:02 AM Christian Brauner <brauner@kernel.org> wrote:
>
> This contains Jens' work to support FMODE_NOWAIT and thus IOCB_NOWAIT
> for pipes ensuring that all places can deal with non-blocking requests.
>
> To this end, pass down the information that this is a nonblocking
> request so that pipe locking, allocation, and buffer checking correctly
> deal with those.

Ok, I pulled this, but then I unpulled it again.

Doing conditional locking for O_NONBLOCK and friends is not ok. Yes,
it's been done, and I may even have let some slip through, but it's
just WRONG.

There is absolutely no situation where a "ok, so the lock on this data
structure was taken, we'll go to some async model" is worth it.

Every single time I've seen this, it's been some developer who thinks
that O_NONBLOCk is somehow some absolute "this cannot schedule" thing.
And every single time it's been broken and horrid crap that just made
everything more complicated and slowed things down.

If some lock wait is a real problem, then the lock needs to be just
fixed. Not "ok, let's make a non-blocking version and fall back if
it's held".

Note that FMODE_NOWAIT does not mean (and *CANNOT* mean) that you'd
somehow be able to do the IO in some atomic context anyway. Many of
our kernel locks don't even support that (eg mutexes).

So thinking that FMODE_NOWAIT is that kind of absolute is the wrong
kind of thinking entirely.

FMODE_NOWAIT should mean that no *IO* gets done. And yes, that might
mean that allocations fail too. But not this kind of "let's turn
locking into 'trylock' stuff".

The whoe flag is misnamed. It should have been FMODE_NOIO, the same
way we have IOCB_NOIO.

If you want FMODE_ATOMIC, then that is something entirely and
completely different, and is probably crazy.

We have done it in one area (RCU pathname lookup), and it was worth it
there, and it was a *huge* undertaking. It was worth it, but it was
worth it because it was a serious thing with some serious design and a
critical area.

Not this kind of "conditional trylock" garbage which just means that
people will require 'poll()' to now add the lock to the waitqueue, or
make all callers go into some "let's use a different thread instead"
logic.

                             Linus

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Jens Axboe]

On 4/24/23 3:05?PM, Linus Torvalds wrote:
> On Fri, Apr 21, 2023 at 7:02?AM Christian Brauner <brauner@kernel.org> wrote:
>>
>> This contains Jens' work to support FMODE_NOWAIT and thus IOCB_NOWAIT
>> for pipes ensuring that all places can deal with non-blocking requests.
>>
>> To this end, pass down the information that this is a nonblocking
>> request so that pipe locking, allocation, and buffer checking correctly
>> deal with those.
> 
> Ok, I pulled this, but then I unpulled it again.
> 
> Doing conditional locking for O_NONBLOCK and friends is not ok. Yes,
> it's been done, and I may even have let some slip through, but it's
> just WRONG.
> 
> There is absolutely no situation where a "ok, so the lock on this data
> structure was taken, we'll go to some async model" is worth it.
> 
> Every single time I've seen this, it's been some developer who thinks
> that O_NONBLOCk is somehow some absolute "this cannot schedule" thing.
> And every single time it's been broken and horrid crap that just made
> everything more complicated and slowed things down.
> 
> If some lock wait is a real problem, then the lock needs to be just
> fixed. Not "ok, let's make a non-blocking version and fall back if
> it's held".
> 
> Note that FMODE_NOWAIT does not mean (and *CANNOT* mean) that you'd
> somehow be able to do the IO in some atomic context anyway. Many of
> our kernel locks don't even support that (eg mutexes).
> 
> So thinking that FMODE_NOWAIT is that kind of absolute is the wrong
> kind of thinking entirely.
> 
> FMODE_NOWAIT should mean that no *IO* gets done. And yes, that might
> mean that allocations fail too. But not this kind of "let's turn
> locking into 'trylock' stuff".
> 
> The whoe flag is misnamed. It should have been FMODE_NOIO, the same
> way we have IOCB_NOIO.
> 
> If you want FMODE_ATOMIC, then that is something entirely and
> completely different, and is probably crazy.
> 
> We have done it in one area (RCU pathname lookup), and it was worth it
> there, and it was a *huge* undertaking. It was worth it, but it was
> worth it because it was a serious thing with some serious design and a
> critical area.
> 
> Not this kind of "conditional trylock" garbage which just means that
> people will require 'poll()' to now add the lock to the waitqueue, or
> make all callers go into some "let's use a different thread instead"
> logic.

I'm fully agreeing with you on the semantics for
FMODE_NOWAIT/IOCB_NOWAIT, and also agree that they are misnamed and
really should be _NOIO. There is no conceptual misunderstanding there,
nor do I care about atomic semantics. Obviously the above is for
io_uring to be able to improve the performance on pipes, because right
now everything is punted to io-wq and that severly hampers performance
with how pipes are generally used. io_uring doesn't care about atomic
context, but it very much cares about NOT sleeping for IO during issue
as that has adverse performance implications.

If we don't ever wait for IO with the pipe lock held, then we can skip
the conditional locking. But with splice, that's not at all the case! We
most certainly wait for IO there with the pipe lock held.

And yes I totally agree that conditional locking is not pretty, but for
some cases it really is a necessary evil. People have complained about
io_uring pipe performance, and I ran some testing myself. Being able to
sanely read/write from/to pipes without punting to io-wq is an easy 10x
improvement for that use case. How can I enable FMODE_NOWAIT on pipes if
we have splice holding the pipe lock over IO? It's not feasible.

So please reconsider, doing IO to/from pipes efficiently is actually
kind of useful for io_uring...

-- 
Jens Axboe

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Linus Torvalds]

On Mon, Apr 24, 2023 at 2:22 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> If we don't ever wait for IO with the pipe lock held, then we can skip
> the conditional locking. But with splice, that's not at all the case! We
> most certainly wait for IO there with the pipe lock held.

I think that then needs to just be fixed.

I really think that trylock due to "nonblocking" IO is fundamentally
wrong. Thinking that you need it is just a sign of something else
being very wrong.

That "very wrong" thing might well be splice then not honoring
non-blocking IO on a non-blocking pipe.

And I completely refuse to add that trylock hack to paper that over.
The pipe lock is *not* meant for IO.

             Linus

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Jens Axboe]

On 4/24/23 3:37?PM, Linus Torvalds wrote:
> On Mon, Apr 24, 2023 at 2:22?PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> If we don't ever wait for IO with the pipe lock held, then we can skip
>> the conditional locking. But with splice, that's not at all the case! We
>> most certainly wait for IO there with the pipe lock held.
> 
> I think that then needs to just be fixed.

I took another look at this, and the main issue is in fact splice
confirming buffers. So I do think that we can make this work by simply
having the non-block nature of it being passed down the ->confirm()
callback as that's the one that'll be waiting for IO. If we have that,
then we can disregard the pipe locking as we won't be holding it over
IO.

Which is what part of this series does, notably patch 1.

Only other oddity is pipe_to_sendpage(), which we can probably sanely
ignore.

IOW, would you be fine with a v2 of this pull request where patch 2
drops the conditional locking and just passes it to ->confirm()? That's
certainly sane, and just makes the ultimate page locking conditional to
avoid waiting on IO. I'd really hate to still be missing out on pipe
performance with io_uring.

-- 
Jens Axboe

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Linus Torvalds]

On Mon, Apr 24, 2023 at 2:55 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> I took another look at this, and the main issue is in fact splice
> confirming buffers. So I do think that we can make this work by simply
> having the non-block nature of it being passed down the ->confirm()
> callback as that's the one that'll be waiting for IO. If we have that,
> then we can disregard the pipe locking as we won't be holding it over
> IO.

Ok, that part looks fine to me.

The pipe_buf_confirm() part of the series I don't find problematic,
it's really conditional locking that I absolutely detest and has
always been a sign of problems elsewhere.

             Linus

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Jens Axboe]

On 4/24/23 4:00?PM, Linus Torvalds wrote:
> On Mon, Apr 24, 2023 at 2:55?PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> I took another look at this, and the main issue is in fact splice
>> confirming buffers. So I do think that we can make this work by simply
>> having the non-block nature of it being passed down the ->confirm()
>> callback as that's the one that'll be waiting for IO. If we have that,
>> then we can disregard the pipe locking as we won't be holding it over
>> IO.
> 
> Ok, that part looks fine to me.
> 
> The pipe_buf_confirm() part of the series I don't find problematic,
> it's really conditional locking that I absolutely detest and has
> always been a sign of problems elsewhere.

Agree, the conditional locking is the ugly bit for sure. I'll reply to
your other email as my followup to my message discovered that this isn't
enough due to mixed splice/non-splice usage.

-- 
Jens Axboe

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Jens Axboe]

On 4/24/23 3:55?PM, Jens Axboe wrote:
> On 4/24/23 3:37?PM, Linus Torvalds wrote:
>> On Mon, Apr 24, 2023 at 2:22?PM Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> If we don't ever wait for IO with the pipe lock held, then we can skip
>>> the conditional locking. But with splice, that's not at all the case! We
>>> most certainly wait for IO there with the pipe lock held.
>>
>> I think that then needs to just be fixed.
> 
> I took another look at this, and the main issue is in fact splice
> confirming buffers. So I do think that we can make this work by simply
> having the non-block nature of it being passed down the ->confirm()
> callback as that's the one that'll be waiting for IO. If we have that,
> then we can disregard the pipe locking as we won't be holding it over
> IO.
> 
> Which is what part of this series does, notably patch 1.
> 
> Only other oddity is pipe_to_sendpage(), which we can probably sanely
> ignore.
> 
> IOW, would you be fine with a v2 of this pull request where patch 2
> drops the conditional locking and just passes it to ->confirm()? That's
> certainly sane, and just makes the ultimate page locking conditional to
> avoid waiting on IO. I'd really hate to still be missing out on pipe
> performance with io_uring.

I guess that would still have blocking if you have someone doing splice
in a blocking fashion, and someone else trying to do RWF_NOWAIT reads or
writes to the pipe... The very thing the conditional pipe locking would
sort out.

Only fool proof alternative would seem to be having splice use a
specific pipe lock rather then pipe->mutex. And honestly pipes and
splice are so tied together than I'm not sure that doing separate
locking would be feasible.

-- 
Jens Axboe

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Linus Torvalds]

On Mon, Apr 24, 2023 at 2:37 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> And I completely refuse to add that trylock hack to paper that over.
> The pipe lock is *not* meant for IO.

If you want to paper it over, do it other ways.

I'd love to just magically fix splice, but hey, that might not be possible.

But possible fixes papering this over might be to make splice "poison
a pipe, and make io_uring falls back on io workers only on pipes that
do splice. Make any normal pipe read/write load sane.

And no, don't worry about races. If you have the same pipe used for
io_uring IO *and* somebody else then doing splice on it and racing,
just take the loss and tell people that they might hit a slow case if
they do stupid things.

Basically, the patch might look like something like

 - do_pipe() sets FMODE_NOWAIT by default when creating a pipe

 - splice then clears FMODE_NOWAIT on pipes as they are used

and now io_uring sees whether the pipe is playing nice or not.

As far as I can tell, something like that would make the
'pipe_buf_confirm()' part unnecessary too, since that's only relevant
for splice.

A fancier version might be to only do that "splice then clears
FMODE_NOWAIT" thing if the other side of the splice has not set
FMODE_NOWAIT.

Honestly, if the problem is "pipe IO is slow", then splice should not
be the thing you optimize for.

                 Linus

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Jens Axboe]

On 4/24/23 3:58?PM, Linus Torvalds wrote:
> On Mon, Apr 24, 2023 at 2:37?PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> And I completely refuse to add that trylock hack to paper that over.
>> The pipe lock is *not* meant for IO.
> 
> If you want to paper it over, do it other ways.
> 
> I'd love to just magically fix splice, but hey, that might not be possible.

Don't think it is... At least not trivially.

> But possible fixes papering this over might be to make splice "poison
> a pipe, and make io_uring falls back on io workers only on pipes that
> do splice. Make any normal pipe read/write load sane.
> 
> And no, don't worry about races. If you have the same pipe used for
> io_uring IO *and* somebody else then doing splice on it and racing,
> just take the loss and tell people that they might hit a slow case if
> they do stupid things.
> 
> Basically, the patch might look like something like
> 
>  - do_pipe() sets FMODE_NOWAIT by default when creating a pipe
> 
>  - splice then clears FMODE_NOWAIT on pipes as they are used
> 
> and now io_uring sees whether the pipe is playing nice or not.
> 
> As far as I can tell, something like that would make the
> 'pipe_buf_confirm()' part unnecessary too, since that's only relevant
> for splice.
> 
> A fancier version might be to only do that "splice then clears
> FMODE_NOWAIT" thing if the other side of the splice has not set
> FMODE_NOWAIT.
> 
> Honestly, if the problem is "pipe IO is slow", then splice should not
> be the thing you optimize for.

I think that'd be an acceptable approach, and would at least fix the
pure pipe case which I suspect is 99.9% of them, if not more. And yes,
it'd mean that we don't need to do the ->confirm() change either, as the
pipe is already tainted at that point.

I'll respin a v2, post, and send in later this merge window.

-- 
Jens Axboe

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Jens Axboe]

On 4/24/23 4:07?PM, Jens Axboe wrote:
> On 4/24/23 3:58?PM, Linus Torvalds wrote:
>> On Mon, Apr 24, 2023 at 2:37?PM Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>>
>>> And I completely refuse to add that trylock hack to paper that over.
>>> The pipe lock is *not* meant for IO.
>>
>> If you want to paper it over, do it other ways.
>>
>> I'd love to just magically fix splice, but hey, that might not be possible.
> 
> Don't think it is... At least not trivially.
> 
>> But possible fixes papering this over might be to make splice "poison
>> a pipe, and make io_uring falls back on io workers only on pipes that
>> do splice. Make any normal pipe read/write load sane.
>>
>> And no, don't worry about races. If you have the same pipe used for
>> io_uring IO *and* somebody else then doing splice on it and racing,
>> just take the loss and tell people that they might hit a slow case if
>> they do stupid things.
>>
>> Basically, the patch might look like something like
>>
>>  - do_pipe() sets FMODE_NOWAIT by default when creating a pipe
>>
>>  - splice then clears FMODE_NOWAIT on pipes as they are used
>>
>> and now io_uring sees whether the pipe is playing nice or not.
>>
>> As far as I can tell, something like that would make the
>> 'pipe_buf_confirm()' part unnecessary too, since that's only relevant
>> for splice.
>>
>> A fancier version might be to only do that "splice then clears
>> FMODE_NOWAIT" thing if the other side of the splice has not set
>> FMODE_NOWAIT.
>>
>> Honestly, if the problem is "pipe IO is slow", then splice should not
>> be the thing you optimize for.
> 
> I think that'd be an acceptable approach, and would at least fix the
> pure pipe case which I suspect is 99.9% of them, if not more. And yes,
> it'd mean that we don't need to do the ->confirm() change either, as the
> pipe is already tainted at that point.
> 
> I'll respin a v2, post, and send in later this merge window.

Something like this. Not tested yet, but wanted to get your feedback
early to avoid issues down the line. Really just the first hunk, as
we're not really expecting f_mode to change and hence could just use
an xchg(). Seems saner to use a cmpxchg though, but maybe that's
too much of both belt and suspenders?

commit f10cbebf4dd7f77b0e87c77bb0191a5a07d5e7ac
Author: Jens Axboe <axboe@kernel.dk>
Date:   Mon Apr 24 16:32:55 2023 -0600

    splice: clear FMODE_NOWAIT on file if splice/vmsplice is used
    
    In preparation for pipes setting FMODE_NOWAIT on pipes to indicate that
    RWF_NOWAIT/IOCB_NOWAIT is fully supported, have splice and vmsplice
    clear that file flag. Splice holds the pipe lock around IO and cannot
    easily be refactored to avoid that, as splice and pipes are inherently
    tied together.
    
    By clearing FMODE_NOWAIT if splice is being used on a pipe, other users
    of the pipe will know that the pipe is no longer safe for RWF_NOWAIT
    and friends.
    
    Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/fs/splice.c b/fs/splice.c
index 2c3dec2b6dfa..3ce7c07621e2 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -37,6 +37,29 @@
 
 #include "internal.h"
 
+/*
+ * Splice doesn't support FMODE_NOWAIT. Since pipes may set this flag to
+ * indicate they support non-blocking reads or writes, we must clear it
+ * here if set to avoid blocking other users of this pipe if splice is
+ * being done on it.
+ */
+static void pipe_clear_nowait(struct file *file)
+{
+	fmode_t new_fmode, old_fmode;
+
+	/*
+	 * We could get by with just doing an xchg() here as f_mode should
+	 * not change in the file's lifetime, but let's be safe and use
+	 * a cmpxchg() instead.
+	 */
+	do {
+		old_fmode = READ_ONCE(file->f_mode);
+		if (!(old_fmode & FMODE_NOWAIT))
+			break;
+		new_fmode = old_fmode & ~FMODE_NOWAIT;
+	} while (!try_cmpxchg(&file->f_mode, &old_fmode, new_fmode));
+}
+
 /*
  * Attempt to steal a page from a pipe buffer. This should perhaps go into
  * a vm helper function, it's already simplified quite a bit by the
@@ -1211,10 +1234,16 @@ static long __do_splice(struct file *in, loff_t __user *off_in,
 	ipipe = get_pipe_info(in, true);
 	opipe = get_pipe_info(out, true);
 
-	if (ipipe && off_in)
-		return -ESPIPE;
-	if (opipe && off_out)
-		return -ESPIPE;
+	if (ipipe) {
+		if (off_in)
+			return -ESPIPE;
+		pipe_clear_nowait(in);
+	}
+	if (opipe) {
+		if (off_out)
+			return -ESPIPE;
+		pipe_clear_nowait(out);
+	}
 
 	if (off_out) {
 		if (copy_from_user(&offset, off_out, sizeof(loff_t)))
@@ -1311,6 +1340,8 @@ static long vmsplice_to_user(struct file *file, struct iov_iter *iter,
 	if (!pipe)
 		return -EBADF;
 
+	pipe_clear_nowait(file);
+
 	if (sd.total_len) {
 		pipe_lock(pipe);
 		ret = __splice_from_pipe(pipe, &sd, pipe_to_user);
@@ -1339,6 +1370,8 @@ static long vmsplice_to_pipe(struct file *file, struct iov_iter *iter,
 	if (!pipe)
 		return -EBADF;
 
+	pipe_clear_nowait(file);
+
 	pipe_lock(pipe);
 	ret = wait_for_space(pipe, flags);
 	if (!ret)

-- 
Jens Axboe

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Linus Torvalds]

On Mon, Apr 24, 2023 at 3:45 PM Jens Axboe <axboe@kernel.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.

               Linus

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Jens Axboe]

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

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Linus Torvalds]

On Mon, Apr 24, 2023 at 8:16 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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.

I ended up looking around a bit, and the READ_ONCE() before the
"try_cmpxchg()" loop is definitely our common pattern.

However, I'm adding in the locking people, because I think that
pattern is wrong and historical.

I think it comes from the original cmpxchg loop model, where we did
the read inside the loop, and the READ_ONCE() was some kind of "let's
make sure it's updated every time" thing (which should be unnecessary
due to memory clobbers on the cmpxchg, but whatever...

Inside the loop, the READ_ONCE() also makes more sense in general, in
that there isn't any sane point in merging it with earlier values, so
there's no downside.

But the more I look at it, the more I'm convinced that our pattern of

        old = READ_ONCE(rq->fence.error);
        do {
                if (fatal_error(old))
                        return false;
        } while (!try_cmpxchg(&rq->fence.error, &old, error));

(to pick one random user) is simply horribly wrong.

If we have code where we had an earlier load of that same value (or an
earlier store, for that matter - anything where the compiler can
assume some starting value), then the READ_ONCE() only adds pointless
overhead.

And if we don't have it, then the READ_ONCE() doesn't add any value,
since it doesn't imply any ordering.

IOW, I think the READ_ONCE() pattern is either pointless or
detrimental. I see no upside.

So I think we should make the pattern be used with a try_cmpxchg()
loop to be either a proper *ordered* read (ie something like
"smp_load_acquire()" for reading the original value), or we should
just let the compiler read it any which way it wants. Not the
READ_ONCE() pattern we seem to have.

But I'm adding the locking maintainers to this email to make sure that
I'm not missing anything.

PeterZ in particular, who added that new (and hugely improved!)
try_cmpxchg() interface in the first place.

        Linus

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Peter Zijlstra]

On Tue, Apr 25, 2023 at 10:20:31AM -0700, Linus Torvalds wrote:

> But the more I look at it, the more I'm convinced that our pattern of
> 
>         old = READ_ONCE(rq->fence.error);
>         do {
>                 if (fatal_error(old))
>                         return false;
>         } while (!try_cmpxchg(&rq->fence.error, &old, error));
> 
> (to pick one random user) is simply horribly wrong.

The last time this came up I shared your view however Mark argued for
the READ_ONCE() thusly:

  https://lore.kernel.org/all/Y71LoCIl+IFdy9D8@FVFF77S0Q05N/T/#u

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Linus Torvalds]

On Tue, Apr 25, 2023 at 12:49 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> The last time this came up I shared your view however Mark argued for
> the READ_ONCE() thusly:
>
>   https://lore.kernel.org/all/Y71LoCIl+IFdy9D8@FVFF77S0Q05N/T/#u

Hmm.

Yes, I think Mark is right. It's not that 'old' might be wrong - that
doesn't matter because cmpxchg will work it out - it's just that 'new'
might not be consistent with the old value we then use.

Ok. I'll try to remember this, but maybe it might be worth documenting.

Jens - I don't think this actually matters for the f_mode value issue,
since the only thing that might change is that FMODE_NOWAIT bit, but I
was clearly wrong on READ_ONCE(). So that loop should have it, just to
have the right pattern after all.

My bad.

             Linus

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Jens Axboe]

On 4/25/23 1:58?PM, Linus Torvalds wrote:
> Jens - I don't think this actually matters for the f_mode value issue,
> since the only thing that might change is that FMODE_NOWAIT bit, but I
> was clearly wrong on READ_ONCE(). So that loop should have it, just to
> have the right pattern after all.

Noted, I'll update it so it's consistent with the other use cases.
Thanks!

-- 
Jens Axboe

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Linus Torvalds]

On Tue, Apr 25, 2023 at 12:58 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Ok. I'll try to remember this, but maybe it might be worth documenting.

We might document it by just making it clear that it's not that we
want to read it "once", it's that we want to have a stable value.

There might be other situations where that is all we want.

IOW, maybe we could have something like

  #define READ_STABLE(x) \
      ({ __auto_type __val = (x); OPTIMIZER_HIDE_VAR(__val); __val; })

instead - although from a quick look, the code generation is pretty
much exactly the same.

I dunno. Just throwing that idea out there as a "if reading _once_
isn't the issue, maybe we shouldn't make the code look like it is"...

                Linus

Re: [GIT PULL] pipe: nonblocking rw for io_uring

[Peter Zijlstra]

On Sun, May 07, 2023 at 04:04:23PM +0200, Jonas Oberhauser wrote:
> 
> Am 4/25/2023 um 9:58 PM schrieb Linus Torvalds:
> > Yes, I think Mark is right. It's not that 'old' might be wrong - that
> > doesn't matter because cmpxchg will work it out - it's just that 'new'
> > might not be consistent with the old value we then use.
> 
> In the general pattern, besides the potential issue raised by Mark, tearing
> may also be an issue (longer example inspired by a case we met at the end of
> the mail) where 'old' being wrong matters.

There is yet another pattern where it actually matters:

	old = READ_ONCE(*ptr);
	do {
		if (cond(old))
			return false;

		new = func(old);
	} while (!try_cmpxchg(ptr, &old, new));

	return true;

In this case we rely on old being 'coherent'. The more obvious case is
where it returns old (also not uncommon), but even if it just checks a
(multi-bit) condition on old you don't want tearing.

RE: [GIT PULL] pipe: nonblocking rw for io_uring

[David Laight]

From: Peter Zijlstra
> Sent: 08 May 2023 09:39
> 
> On Sun, May 07, 2023 at 04:04:23PM +0200, Jonas Oberhauser wrote:
> >
> > Am 4/25/2023 um 9:58 PM schrieb Linus Torvalds:
> > > Yes, I think Mark is right. It's not that 'old' might be wrong - that
> > > doesn't matter because cmpxchg will work it out - it's just that 'new'
> > > might not be consistent with the old value we then use.
> >
> > In the general pattern, besides the potential issue raised by Mark, tearing
> > may also be an issue (longer example inspired by a case we met at the end of
> > the mail) where 'old' being wrong matters.
> 
> There is yet another pattern where it actually matters:
> 
> 	old = READ_ONCE(*ptr);
> 	do {
> 		if (cond(old))
> 			return false;
> 
> 		new = func(old);
> 	} while (!try_cmpxchg(ptr, &old, new));
> 
> 	return true;
> 
> In this case we rely on old being 'coherent'. The more obvious case is
> where it returns old (also not uncommon), but even if it just checks a
> (multi-bit) condition on old you don't want tearing.

It isn't as though READ_ONCE() is expensive.

For kernel/device driver code, while CSE is useful, you pretty
much always want the compiler to always do loads into local
variables.
It is rather a shame there isn't a compiler option that
avoids these unusual any annoying operations.

Since the current 'rules' seem to require READ_ONCE() and
WRITE_ONCE() be used as pairs, why not make the data 'volatile'?
That ought to be the same as using volatile casts on all accesses.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)