On Tue, Feb 18, 2025 at 12:09 PM Keith Busch <kbusch@xxxxxxxxxx> wrote:
>
> On Fri, Feb 14, 2025 at 06:22:09PM -0800, Caleb Sander Mateos wrote:
> > > +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.
>
> The cache can only have fixed size objects in them, so we have to pick
> some kind of trade off. The cache starts off empty and fills up on
> demand, so whatever we allocate needs to be of that cache's element
> size.
>
> > Consider
> > using io_alloc_cache_get() instead of io_cache_alloc(), so the
> > heap-allocated fallback uses the minimal size.
>
> We wouldn't be able to fill the cache with the new dynamic object if we
> did that.
Right, that's a good point that there's a tradeoff. I think always
allocating space for IO_CACHED_BVECS_SEGS bvecs is reasonable. Maybe
IO_CACHED_BVECS_SEGS should be slightly smaller so the allocation fits
nicely in a power of 2? Currently it looks to take up 560 bytes:
>>> 48 + 16 * 32
560
Using IO_CACHED_BVECS_SEGS = 29 instead would make it 512 bytes:
>>> 48 + 16 * 29
512
Best,
Caleb
>
> > 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.
>
> Oops. I kind of rushed this series last Friday as I needed to shut down
> very early in the day.
>
> Thanks for the comments. Will take my time for the next version.
