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

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

On Tue, Feb 18, 2025 at 12:12 PM Keith Busch <kbusch@kernel.org> wrote:
>
> On Sun, Feb 16, 2025 at 02:43:40PM -0800, Caleb Sander Mateos wrote:
> > > > +       io_alloc_cache_free(&table->imu_cache, kfree);
> > > > +}
> > > > +
> > > >  int io_sqe_buffers_unregister(struct io_ring_ctx *ctx)
> > > >  {
> > > >         if (!ctx->buf_table.data.nr)
> > > >                 return -ENXIO;
> > > > -       io_rsrc_data_free(ctx, &ctx->buf_table.data);
> > > > +       io_rsrc_buffer_free(ctx, &ctx->buf_table);
> > > >         return 0;
> > > >  }
> > > >
> > > > @@ -716,6 +767,15 @@ bool io_check_coalesce_buffer(struct page **page_array, int nr_pages,
> > > >         return true;
> > > >  }
> > > >
> > > > +static struct io_mapped_ubuf *io_alloc_imu(struct io_ring_ctx *ctx,
> > > > +                                          int nr_bvecs)
> > > > +{
> > > > +       if (nr_bvecs <= IO_CACHED_BVECS_SEGS)
> > > > +               return io_cache_alloc(&ctx->buf_table.imu_cache, GFP_KERNEL);
> > >
> > > If there is no entry available in the cache, this will heap-allocate
> > > one with enough space for all IO_CACHED_BVECS_SEGS bvecs. Consider
> > > using io_alloc_cache_get() instead of io_cache_alloc(), so the
> > > heap-allocated fallback uses the minimal size.
> > >
> > > Also, where are these allocations returned to the imu_cache? Looks
> > > like kvfree(imu) in io_buffer_unmap() and io_sqe_buffer_register()
> > > needs to try io_alloc_cache_put() first.
> >
> > Another issue I see is that io_alloc_cache elements are allocated with
> > kmalloc(), so they can't be freed with kvfree().
>
> You actually can kvfree(kmalloc()); Here's the kernel doc for it:
>
>   kvfree frees memory allocated by any of vmalloc(), kmalloc() or kvmalloc()

Good to know, thanks for the pointer! I guess it might be a bit more
efficient to call kfree() if we know based on nr_bvecs that the
allocation came from kmalloc(), but at least this isn't corrupting the
heap.

Best,
Caleb

>
> > When the imu is
> > freed, we could check nr_bvecs <= IO_CACHED_BVECS_SEGS to tell whether
> > to call io_alloc_cache_put() (with a fallback to kfree()) or kvfree().
>
> But you're right, it shouldn't even hit this path because it's supposed
> to try to insert the imu into the cache if that's where it was allocated
> from.