On Mon, Feb 24, 2025 at 1:01 PM Keith Busch <kbusch@xxxxxxxxxx> 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
