Re: [PATCHv4 5/5] io_uring: cache nodes and mapped buffers

[Caleb Sander Mateos <csander@purestorage.com>]

On Mon, Feb 24, 2025 at 1:01 PM Keith Busch <kbusch@kernel.org> wrote:
>
> On Tue, Feb 18, 2025 at 08:22:36PM -0800, Caleb Sander Mateos wrote:
> > > +struct io_alloc_cache {
> > > +       void                    **entries;
> > > +       unsigned int            nr_cached;
> > > +       unsigned int            max_cached;
> > > +       size_t                  elem_size;
> >
> > Is growing this field from unsigned to size_t really necessary? It
> > probably doesn't make sense to be caching allocations > 4 GB.
>
> It used to be a size_t when I initially moved the struct to here, but
> it's not anymore, so I'm out of sync. I'll fix it up.
>
> > > @@ -859,10 +924,8 @@ int io_sqe_buffers_register(struct io_ring_ctx *ctx, void __user *arg,
> > >                         }
> > >                         node->tag = tag;
> > >                 }
> > > -               data.nodes[i] = node;
> > > +               table.data.nodes[i] = node;
> > >         }
> > > -
> > > -       ctx->buf_table.data = data;
> >
> > Still don't see the need to move this assignment. Is there a reason
> > you prefer setting ctx->buf_table before initializing its nodes? I
> > find the existing code easier to follow, where the table is moved to
> > ctx->buf_table after filling it in. It's also consistent with
> > io_clone_buffers().
>
> Yes, it needs to move to earlier. The ctx buf_table needs to be set
> before any allocations from io_rsrc_node_alloc() can happen.

Got it, thank you for explaining.

Best,
Caleb