Re: [PATCH] io_uring: pass struct io_tw_state by value

[Pavel Begunkov]

On 2/11/25 21:45, Caleb Sander Mateos wrote:
> 8e5b3b89ecaf ("io_uring: remove struct io_tw_state::locked") removed the
> only field of io_tw_state but kept it as a task work callback argument
> to "forc[e] users not to invoke them carelessly out of a wrong context".

The other unnamed reason was to limit the mess from io_uring wide
changes while it might still be used later on. It's always a pain
for backporting, so if we're doing that we might as well make it
easily convertible for future plans. Let's do

typedef struct io_tw_state iou_state_t;

Might be useful for leaking it to cmds as well.

> Passing the struct io_tw_state * argument adds a few instructions to all
> callers that can't inline the functions and see the argument is unused.
>
> So pass struct io_tw_state by value instead. Since it's a 0-sized value,
> it can be passed without any instructions needed to initialize it.
>
> Also add a comment to struct io_tw_state to explain its purpose.
>
> Signed-off-by: Caleb Sander Mateos <csander@xxxxxxxxxxxxxxx>
> ---
>   include/linux/io_uring_types.h |  3 ++-
>   io_uring/futex.c               |  6 +++---
>   io_uring/io_uring.c            | 30 +++++++++++++++---------------
>   io_uring/io_uring.h            |  8 ++++----
>   io_uring/msg_ring.c            |  2 +-
>   io_uring/notif.c               |  2 +-
>   io_uring/poll.c                |  4 ++--
>   io_uring/poll.h                |  2 +-
>   io_uring/rw.c                  |  2 +-
>   io_uring/rw.h                  |  2 +-
>   io_uring/timeout.c             |  6 +++---
>   io_uring/uring_cmd.c           |  2 +-
>   io_uring/waitid.c              |  6 +++---
>   13 files changed, 38 insertions(+), 37 deletions(-)
>
> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> index e2fef264ff8b..4abd0299fdfb 100644
> --- a/include/linux/io_uring_types.h
> +++ b/include/linux/io_uring_types.h
> @@ -434,10 +434,11 @@ struct io_ring_ctx {
>   	struct io_mapped_region		ring_region;
>   	/* used for optimised request parameter and wait argument passing  */
>   	struct io_mapped_region		param_region;
>   };
>   
> +/* Token passed to task work callbacks to indicate ctx->uring_lock is held */

It indicates the current context not locking specifically, which is
why the chunk from waitid as below is not correct:

mutex_lock(uring_lock);
struct io_tw_state dummy;
io_uring_core_helper(&dummy);
mutex_unlock(uring_lock);


Probably something like "Token passed around to indicate the current
io_uring execution context". And it doesn't need to be more specific
as long as type system works for us. It might be a good idea to add
a note in bold that nobody outside of core io_uring is allowed
creating it, though it'll be ignored anyway.

>   struct io_tw_state {
>   };


--
Pavel Begunkov