Jeff Moyer <jmoyer@xxxxxxxxxx> writes:
> Hi, Gabriel,
>
> I just have a couple of comments. I don't have an opinion on whether it
> makes sense to replace the existing allocator.
>
> -Jeff
>
>> @@ -362,11 +363,12 @@ int io_provide_buffers_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe
>> return 0;
>> }
>>
>> +#define IO_BUFFER_ALLOC_BATCH (PAGE_SIZE/sizeof(struct io_buffer))
>> +
>> static int io_refill_buffer_cache(struct io_ring_ctx *ctx)
>> {
>> - struct io_buffer *buf;
>> - struct page *page;
>> - int bufs_in_page;
>> + struct io_buffer *bufs[IO_BUFFER_ALLOC_BATCH];
>
> That's a pretty large on-stack allocation.
Indeed, that is definitely too large. Thanks for pointing it out.
Also, I just noticed the define above should actually read:
#define IO_BUFFER_ALLOC_BATCH (PAGE_SIZE/sizeof(struct io_buffer *))
I'll follow up with a v2, after I retest the impact a smaller allocation
batch would have.
>
>> + allocated = kmem_cache_alloc_bulk(io_buf_cachep, GFP_KERNEL_ACCOUNT,
>> + ARRAY_SIZE(bufs), (void **) bufs);
>> + if (unlikely(allocated <= 0)) {
>
> Can't be less than 0.
Thanks, will fix.
>
--
Gabriel Krisman Bertazi
