[GIT PULL] io_uring updates for 6.5

[GIT PULL] io_uring updates for 6.5

[Jens Axboe]

Hi Linus,

Nothing major in this release, just a bunch of cleanups and some
optimizations around networking mostly.

Will throw a minor conflict in io_uring/net.c due to the late fixes in
mainline, you'll want to keep the kmsg->msg.msg_inq = -1U; assignment
there.

Please pull!


- Series cleaning up file request flags handling (Christoph)

- Series cleaning up request freeing and CQ locking (Pavel)

- Support for using pre-registering the io_uring fd at setup time (Josh)

- Add support for user allocated ring memory, rather than having the
  kernel allocate it. Mostly for packing rings into a huge page (me)

- Series avoiding an unnecessary double retry on receive (me)

- Maintain ordering for task_work, which also improves performance (me)

- Misc cleanups/fixes (Pavel, me)

The following changes since commit f1fcbaa18b28dec10281551dfe6ed3a3ed80e3d6:

  Linux 6.4-rc2 (2023-05-14 12:51:40 -0700)

are available in the Git repository at:

  git://git.kernel.dk/linux.git tags/for-6.5/io_uring-2023-06-23

for you to fetch changes up to c98c81a4ac37b651be7eb9d16f562fc4acc5f867:

  io_uring: merge conditional unlock flush helpers (2023-06-23 08:19:40 -0600)

----------------------------------------------------------------
for-6.5/io_uring-2023-06-23

----------------------------------------------------------------
Christoph Hellwig (8):
      io_uring: remove __io_file_supports_nowait
      io_uring: remove the mode variable in io_file_get_flags
      io_uring: remove a confusing comment above io_file_get_flags
      io_uring: remove io_req_ffs_set
      io_uring: return REQ_F_ flags from io_file_get_flags
      io_uring: use io_file_from_index in __io_sync_cancel
      io_uring: use io_file_from_index in io_msg_grab_file
      io_uring: add helpers to decode the fixed file file_ptr

Jens Axboe (16):
      net: set FMODE_NOWAIT for sockets
      block: mark bdev files as FMODE_NOWAIT if underlying device supports it
      io_uring: rely solely on FMODE_NOWAIT
      io_uring: remove sq/cq_off memset
      io_uring: return error pointer from io_mem_alloc()
      io_uring: add ring freeing helper
      io_uring: support for user allocated memory for rings/sqes
      io_uring/net: initialize struct msghdr more sanely for io_recv()
      io_uring/net: initalize msghdr->msg_inq to known value
      io_uring/net: push IORING_CQE_F_SOCK_NONEMPTY into io_recv_finish()
      io_uring/net: don't retry recvmsg() unnecessarily
      io_uring: maintain ordering for DEFER_TASKRUN tw list
      io_uring: avoid indirect function calls for the hottest task_work
      io_uring: cleanup io_aux_cqe() API
      io_uring: get rid of unnecessary 'length' variable
      io_uring: wait interruptibly for request completions on exit

Josh Triplett (1):
      io_uring: Add io_uring_setup flag to pre-register ring fd and never install it

Pavel Begunkov (14):
      io_uring: annotate offset timeout races
      io_uring/cmd: add cmd lazy tw wake helper
      nvme: optimise io_uring passthrough completion
      io_uring: open code io_put_req_find_next
      io_uring: remove io_free_req_tw
      io_uring: inline io_dismantle_req()
      io_uring: move io_clean_op()
      io_uring: don't batch task put on reqs free
      io_uring: remove IOU_F_TWQ_FORCE_NORMAL
      io_uring: kill io_cq_unlock()
      io_uring: fix acquire/release annotations
      io_uring: inline __io_cq_unlock
      io_uring: make io_cq_unlock_post static
      io_uring: merge conditional unlock flush helpers

 block/fops.c                   |   5 +-
 drivers/nvme/host/ioctl.c      |   4 +-
 include/linux/io_uring.h       |  18 +-
 include/linux/io_uring_types.h |  10 +
 include/uapi/linux/io_uring.h  |  16 +-
 io_uring/cancel.c              |   5 +-
 io_uring/filetable.c           |  11 +-
 io_uring/filetable.h           |  28 ++-
 io_uring/io_uring.c            | 497 ++++++++++++++++++++++-------------------
 io_uring/io_uring.h            |  17 +-
 io_uring/msg_ring.c            |   4 +-
 io_uring/net.c                 |  58 ++---
 io_uring/poll.c                |   6 +-
 io_uring/poll.h                |   2 +
 io_uring/rsrc.c                |   8 +-
 io_uring/rw.c                  |   6 +-
 io_uring/rw.h                  |   1 +
 io_uring/tctx.c                |  31 ++-
 io_uring/timeout.c             |   6 +-
 io_uring/uring_cmd.c           |  16 +-
 net/socket.c                   |   1 +
 21 files changed, 416 insertions(+), 334 deletions(-)

-- 
Jens Axboe

Re: [GIT PULL] io_uring updates for 6.5

[Linus Torvalds]

On Sun, 25 Jun 2023 at 19:39, Jens Axboe <axboe@kernel.dk> wrote:
>
> Will throw a minor conflict in io_uring/net.c due to the late fixes in
> mainline, you'll want to keep the kmsg->msg.msg_inq = -1U; assignment
> there.

Can you please share some of those drugs you are on?

Or, better yet, admit you have a problem, and flush those things down
the toilet.

That

        kmsg->msg.msg_inq = -1U;

pattern is complete insanity.

It's truly completely insane, because:

 (a) the whole concept of "-1U" is broken garbage

 (b) msg_inq is an 'int', not "unsigned int"

I want to note that assigning "-1" to an unsigned variable is fine,
and makes perfect sense. "-1" is signed, so if the unsigned variable
is larger, then the sign extension means that assigning "-1" is the
same as setting all bits. Look, no need to worry about the size of the
end result, it always JustWorks(tm).

Ergo: -1 is fine - regardless of whether the end result is signed or unsigned.

But doing the same with "-1U" is *dangerous". Because "-1U" is an
unsigned int, if you assign it to some larger entity, you basically
get a random end result that depends on the size of 'int' and the size
of the destination.

So any time you see something like "-1U", you should go "those are
some bad bad drugs".

It doesn't just look odd - it's actively *WRONG*. It's *STUPID*. And
it's *DANGEROUS*.

Lookie here: the same completely bogus pattern exists in some testing too:

io_uring/net.c:

        if (msg->msg_inq && msg->msg_inq != -1U)

and it all happens to work, but it happens to work for all the wrong
reasons. Because  -1U is unsigned, the "msg->msg_inq != -1U"
comparison is done as "unsigned int", and msg->msg_inq (which contains
a *signed* -1) becomes 4294967295, and it all matches.

But while it happens to work, it's entirely illogical and makes no sense at all.

And if you ever end up in the situation that something is extended to
'long', it will break horribly on 64-bit architectures, since now
"-1U" will literally be 4294967295, while "msg->msg_inq" will become
-1l, and the two are *not* the same.

I don't know who came up with this crazy pattern, but it must die.
"-1U" is garbage. Yes, it means "all bits set in an 'unsigned int'",
so it does have real semantics, but dammit, those semantics are very
seldom the ones you want.

They most *definitely* aren't the ones you want if you then mix things
with a signed type.

And yes, greping for this I found some truly disgusting things elsewhere too:

mm/zsmalloc.c:
        set_freeobj(zspage, (unsigned int)(-1UL));

net/core/rtnetlink.c:
        filters->mask[i] = -1U;

both of which are impressively confused. There's sadly a number of
other cases too.

That zsmalloc.c case is impressively crazy. First you use the wrong
type, then you use the wrong operation, and then you use a cast to fix
it all up. Absolutely none of which makes any sense, and it should
just have used "-1".

The rtnetlink case is also impressive, since mask[] isn't even an
array of 'unsigned int', but a 'u32'. They may be the same thing in
the end, but it's a sign of true confusion to think that "-1u" is
somehow fine.

Again, if you want to assign "all bits set" to a random unsigned type,
just use "-1". Sign extension is _literally_ your friend, the whole
world is 2's complement, and it's the only reason "-1" works in the
first place.

And if what you want is a particular unsigned type with all bits set
(say, unsigned long), I suggest you use "~0ul" to indicate that.

Because if you don't want to rely on sign extension, just use the
actual bit operation, for chrissake!

Ok, that was a rant about code that happens to work, but that is crap
regardless.

I'm doing this pull, but I want this idiocy fixed.

Writing illogical code, and then relying (probably without realizing
it) on some of the stranger parts of the implicit integer type
conversions in C to make that code work, that is just not good.

           Linus

Re: [GIT PULL] io_uring updates for 6.5

[Jens Axboe]

On 6/26/23 1:40?PM, Linus Torvalds wrote:
> On Sun, 25 Jun 2023 at 19:39, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> Will throw a minor conflict in io_uring/net.c due to the late fixes in
>> mainline, you'll want to keep the kmsg->msg.msg_inq = -1U; assignment
>> there.
> 
> Can you please share some of those drugs you are on?
> 
> Or, better yet, admit you have a problem, and flush those things down
> the toilet.
> 
> That
> 
>         kmsg->msg.msg_inq = -1U;
> 
> pattern is complete insanity.
> 
> It's truly completely insane, because:
> 
>  (a) the whole concept of "-1U" is broken garbage
> 
>  (b) msg_inq is an 'int', not "unsigned int"
> 
> I want to note that assigning "-1" to an unsigned variable is fine,
> and makes perfect sense. "-1" is signed, so if the unsigned variable
> is larger, then the sign extension means that assigning "-1" is the
> same as setting all bits. Look, no need to worry about the size of the
> end result, it always JustWorks(tm).
> 
> Ergo: -1 is fine - regardless of whether the end result is signed or unsigned.
> 
> But doing the same with "-1U" is *dangerous". Because "-1U" is an
> unsigned int, if you assign it to some larger entity, you basically
> get a random end result that depends on the size of 'int' and the size
> of the destination.
> 
> So any time you see something like "-1U", you should go "those are
> some bad bad drugs".
> 
> It doesn't just look odd - it's actively *WRONG*. It's *STUPID*. And
> it's *DANGEROUS*.
> 
> Lookie here: the same completely bogus pattern exists in some testing too:
> 
> io_uring/net.c:
> 
>         if (msg->msg_inq && msg->msg_inq != -1U)
> 
> and it all happens to work, but it happens to work for all the wrong
> reasons. Because  -1U is unsigned, the "msg->msg_inq != -1U"
> comparison is done as "unsigned int", and msg->msg_inq (which contains
> a *signed* -1) becomes 4294967295, and it all matches.
> 
> But while it happens to work, it's entirely illogical and makes no sense at all.
> 
> And if you ever end up in the situation that something is extended to
> 'long', it will break horribly on 64-bit architectures, since now
> "-1U" will literally be 4294967295, while "msg->msg_inq" will become
> -1l, and the two are *not* the same.

Oops yes, I can get that cleaned up. It doesn't really matter in here,
as all we need to know is "did someone assign this value or not", to
avoid relying on msg_inq _always_ returning something when we ask for
it. The actual value of it is way less interesting. Worst case scenario
here is an extra round trip if the available data just happened to
match, which seems basically impossible.

But do agree that it's confusing and bogus, will just change it to -1
in assignment and test that should be it.

> I'm doing this pull, but I want this idiocy fixed.

Thanks! I'll address it in the pre-rc1 pull.

-- 
Jens Axboe

Re: [GIT PULL] io_uring updates for 6.5

[pr-tracker-bot]

The pull request you sent on Sun, 25 Jun 2023 20:39:02 -0600:

> git://git.kernel.dk/linux.git tags/for-6.5/io_uring-2023-06-23

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/0aa69d53ac7c30f6184f88f2e310d808b32b35a5

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] io_uring updates for 6.5

[Sergey Senozhatsky]

On 6/26/23 1:40?PM, Linus Torvalds wrote:
>
> mm/zsmalloc.c:
>        set_freeobj(zspage, (unsigned int)(-1UL));

[..]

> both of which are impressively confused. There's sadly a number of
> other cases too.
>
> That zsmalloc.c case is impressively crazy. First you use the wrong
> type, then you use the wrong operation, and then you use a cast to fix
> it all up. Absolutely none of which makes any sense, and it should
> just have used "-1".

That zsmalloc code is gone, you should not see it after you pull
MM patches from Andrew.