public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH] io_uring: Use slab for struct io_buffer objects
@ 2023-08-30  0:36 Gabriel Krisman Bertazi
  2023-09-07 18:39 ` Gabriel Krisman Bertazi
  2023-09-07 18:55 ` Jeff Moyer
  0 siblings, 2 replies; 4+ messages in thread
From: Gabriel Krisman Bertazi @ 2023-08-30  0:36 UTC (permalink / raw)
  To: axboe; +Cc: io-uring, Gabriel Krisman Bertazi

The allocation of struct io_buffer for metadata of provided buffers is
done through a custom allocator that directly gets pages and
fragments them.  But, slab would do just fine, as this is not a hot path
(in fact, it is a deprecated feature) and, by keeping a custom allocator
implementation we lose benefits like tracking, poisoning,
sanitizers. Finally, the custom code is more complex and requires
keeping the list of pages in struct ctx for no good reason.  This patch
cleans this path up and just uses slab.

I microbenchmarked it by forcing the allocation of a large number of
objects with the least number of io_uring commands possible (keeping
nbufs=USHRT_MAX), with and without the patch.  There is a slight
increase in time spent in the allocation with slab, of course, but even
when allocating to system resources exhaustion, which is not very
realistic and happened around 1/2 billion provided buffers for me, it
wasn't a significant hit in system time.  Specially if we think of a
real-world scenario, an application doing register/unregister of
provided buffers will hit ctx->io_buffers_cache more often than actually
going to slab.

Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
---
 include/linux/io_uring_types.h |  2 --
 io_uring/io_uring.c            |  5 +++-
 io_uring/io_uring.h            |  1 +
 io_uring/kbuf.c                | 47 +++++++++++++++++++---------------
 4 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index f04ce513fadb..45da4895d832 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -346,8 +346,6 @@ struct io_ring_ctx {
 	struct wait_queue_head		rsrc_quiesce_wq;
 	unsigned			rsrc_quiesce;
 
-	struct list_head		io_buffers_pages;
-
 	#if defined(CONFIG_UNIX)
 		struct socket		*ring_sock;
 	#endif
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index e1a23f4993d3..556b42cddf75 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -314,7 +314,6 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	spin_lock_init(&ctx->completion_lock);
 	spin_lock_init(&ctx->timeout_lock);
 	INIT_WQ_LIST(&ctx->iopoll_list);
-	INIT_LIST_HEAD(&ctx->io_buffers_pages);
 	INIT_LIST_HEAD(&ctx->io_buffers_comp);
 	INIT_LIST_HEAD(&ctx->defer_list);
 	INIT_LIST_HEAD(&ctx->timeout_list);
@@ -4622,6 +4621,10 @@ static int __init io_uring_init(void)
 				offsetof(struct io_kiocb, cmd.data),
 				sizeof_field(struct io_kiocb, cmd.data), NULL);
 
+	io_buf_cachep = kmem_cache_create("io_buffer", sizeof(struct io_buffer), 0,
+					  SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT,
+					  NULL);
+
 	return 0;
 };
 __initcall(io_uring_init);
diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h
index 3e6ff3cd9a24..5e4e9b1362e1 100644
--- a/io_uring/io_uring.h
+++ b/io_uring/io_uring.h
@@ -338,6 +338,7 @@ static inline bool io_req_cache_empty(struct io_ring_ctx *ctx)
 }
 
 extern struct kmem_cache *req_cachep;
+extern struct kmem_cache *io_buf_cachep;
 
 static inline struct io_kiocb *io_extract_req(struct io_ring_ctx *ctx)
 {
diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
index 2f0181521c98..38141735dc46 100644
--- a/io_uring/kbuf.c
+++ b/io_uring/kbuf.c
@@ -19,6 +19,8 @@
 
 #define BGID_ARRAY	64
 
+struct kmem_cache *io_buf_cachep;
+
 struct io_provide_buf {
 	struct file			*file;
 	__u64				addr;
@@ -259,6 +261,8 @@ static int __io_remove_buffers(struct io_ring_ctx *ctx,
 void io_destroy_buffers(struct io_ring_ctx *ctx)
 {
 	struct io_buffer_list *bl;
+	struct list_head *item, *tmp;
+	struct io_buffer *buf;
 	unsigned long index;
 	int i;
 
@@ -274,12 +278,9 @@ void io_destroy_buffers(struct io_ring_ctx *ctx)
 		kfree(bl);
 	}
 
-	while (!list_empty(&ctx->io_buffers_pages)) {
-		struct page *page;
-
-		page = list_first_entry(&ctx->io_buffers_pages, struct page, lru);
-		list_del_init(&page->lru);
-		__free_page(page);
+	list_for_each_safe(item, tmp, &ctx->io_buffers_cache) {
+		buf = list_entry(item, struct io_buffer, list);
+		kmem_cache_free(io_buf_cachep, buf);
 	}
 }
 
@@ -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];
+	int allocated;
 
 	/*
 	 * Completions that don't happen inline (eg not under uring_lock) will
@@ -386,22 +388,25 @@ static int io_refill_buffer_cache(struct io_ring_ctx *ctx)
 
 	/*
 	 * No free buffers and no completion entries either. Allocate a new
-	 * page worth of buffer entries and add those to our freelist.
+	 * batch of buffer entries and add those to our freelist.
 	 */
-	page = alloc_page(GFP_KERNEL_ACCOUNT);
-	if (!page)
-		return -ENOMEM;
 
-	list_add(&page->lru, &ctx->io_buffers_pages);
-
-	buf = page_address(page);
-	bufs_in_page = PAGE_SIZE / sizeof(*buf);
-	while (bufs_in_page) {
-		list_add_tail(&buf->list, &ctx->io_buffers_cache);
-		buf++;
-		bufs_in_page--;
+	allocated = kmem_cache_alloc_bulk(io_buf_cachep, GFP_KERNEL_ACCOUNT,
+					  ARRAY_SIZE(bufs), (void **) bufs);
+	if (unlikely(allocated <= 0)) {
+		/*
+		 * Bulk alloc is all-or-nothing. If we fail to get a batch,
+		 * retry single alloc to be on the safe side.
+		 */
+		bufs[0] = kmem_cache_alloc(io_buf_cachep, GFP_KERNEL);
+		if (!bufs[0])
+			return -ENOMEM;
+		allocated = 1;
 	}
 
+	while (allocated)
+		list_add_tail(&bufs[--allocated]->list, &ctx->io_buffers_cache);
+
 	return 0;
 }
 
-- 
2.41.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] io_uring: Use slab for struct io_buffer objects
  2023-08-30  0:36 [PATCH] io_uring: Use slab for struct io_buffer objects Gabriel Krisman Bertazi
@ 2023-09-07 18:39 ` Gabriel Krisman Bertazi
  2023-09-07 18:55 ` Jeff Moyer
  1 sibling, 0 replies; 4+ messages in thread
From: Gabriel Krisman Bertazi @ 2023-09-07 18:39 UTC (permalink / raw)
  To: axboe; +Cc: io-uring

Gabriel Krisman Bertazi <[email protected]> writes:

> The allocation of struct io_buffer for metadata of provided buffers is
> done through a custom allocator that directly gets pages and
> fragments them.  But, slab would do just fine, as this is not a hot path
> (in fact, it is a deprecated feature) and, by keeping a custom allocator
> implementation we lose benefits like tracking, poisoning,
> sanitizers. Finally, the custom code is more complex and requires
> keeping the list of pages in struct ctx for no good reason.  This patch
> cleans this path up and just uses slab.
>
> I microbenchmarked it by forcing the allocation of a large number of
> objects with the least number of io_uring commands possible (keeping
> nbufs=USHRT_MAX), with and without the patch.  There is a slight
> increase in time spent in the allocation with slab, of course, but even
> when allocating to system resources exhaustion, which is not very
> realistic and happened around 1/2 billion provided buffers for me, it
> wasn't a significant hit in system time.  Specially if we think of a
> real-world scenario, an application doing register/unregister of
> provided buffers will hit ctx->io_buffers_cache more often than actually
> going to slab.
>
> Signed-off-by: Gabriel Krisman Bertazi <[email protected]>

Hi Jens,

Any feedback on this?

-- 
Gabriel Krisman Bertazi

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] io_uring: Use slab for struct io_buffer objects
  2023-08-30  0:36 [PATCH] io_uring: Use slab for struct io_buffer objects Gabriel Krisman Bertazi
  2023-09-07 18:39 ` Gabriel Krisman Bertazi
@ 2023-09-07 18:55 ` Jeff Moyer
  2023-09-08  1:30   ` Gabriel Krisman Bertazi
  1 sibling, 1 reply; 4+ messages in thread
From: Jeff Moyer @ 2023-09-07 18:55 UTC (permalink / raw)
  To: Gabriel Krisman Bertazi; +Cc: axboe, io-uring

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.

> +	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.


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] io_uring: Use slab for struct io_buffer objects
  2023-09-07 18:55 ` Jeff Moyer
@ 2023-09-08  1:30   ` Gabriel Krisman Bertazi
  0 siblings, 0 replies; 4+ messages in thread
From: Gabriel Krisman Bertazi @ 2023-09-08  1:30 UTC (permalink / raw)
  To: Jeff Moyer; +Cc: axboe, io-uring

Jeff Moyer <[email protected]> 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-09-08  1:30 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-30  0:36 [PATCH] io_uring: Use slab for struct io_buffer objects Gabriel Krisman Bertazi
2023-09-07 18:39 ` Gabriel Krisman Bertazi
2023-09-07 18:55 ` Jeff Moyer
2023-09-08  1:30   ` Gabriel Krisman Bertazi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox