[PATCH 0/3] small rings clean up

[PATCH 0/3] small rings clean up

[Pavel Begunkov]

Some disconnected cleanup patches split out of a larger series
as they make sense by themselves.

Pavel Begunkov (3):
  io_uring: simplify io_mem_alloc return values
  io_uring: simplify io_pages_free
  io_uring/kbuf: rename is_mapped

 io_uring/io_uring.c | 20 ++++++--------------
 io_uring/kbuf.c     | 24 ++++++++++++------------
 io_uring/kbuf.h     |  2 +-
 3 files changed, 19 insertions(+), 27 deletions(-)

-- 
2.43.0

[PATCH 1/3] io_uring: simplify io_mem_alloc return values

[Pavel Begunkov]

io_mem_alloc() returns a pointer on success and a pointer-encoded error
otherwise. However, it can only fail with -ENOMEM, just return NULL on
failure. PTR_ERR is usually pretty error prone.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 14 +++++---------
 io_uring/kbuf.c     |  4 ++--
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index e7d7a456b489..1d0eac0cc8aa 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -2802,12 +2802,8 @@ static void io_rings_free(struct io_ring_ctx *ctx)
 void *io_mem_alloc(size_t size)
 {
 	gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP;
-	void *ret;
 
-	ret = (void *) __get_free_pages(gfp, get_order(size));
-	if (ret)
-		return ret;
-	return ERR_PTR(-ENOMEM);
+	return (void *) __get_free_pages(gfp, get_order(size));
 }
 
 static unsigned long rings_size(struct io_ring_ctx *ctx, unsigned int sq_entries,
@@ -3762,8 +3758,8 @@ static __cold int io_allocate_scq_urings(struct io_ring_ctx *ctx,
 	else
 		rings = io_rings_map(ctx, p->cq_off.user_addr, size);
 
-	if (IS_ERR(rings))
-		return PTR_ERR(rings);
+	if (!rings)
+		return -ENOMEM;
 
 	ctx->rings = rings;
 	if (!(ctx->flags & IORING_SETUP_NO_SQARRAY))
@@ -3787,9 +3783,9 @@ static __cold int io_allocate_scq_urings(struct io_ring_ctx *ctx,
 	else
 		ptr = io_sqes_map(ctx, p->sq_off.user_addr, size);
 
-	if (IS_ERR(ptr)) {
+	if (!ptr) {
 		io_rings_free(ctx);
-		return PTR_ERR(ptr);
+		return -ENOMEM;
 	}
 
 	ctx->sq_sqes = ptr;
diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
index 9be42bff936b..0677eae92e79 100644
--- a/io_uring/kbuf.c
+++ b/io_uring/kbuf.c
@@ -627,8 +627,8 @@ static int io_alloc_pbuf_ring(struct io_ring_ctx *ctx,
 	ibf = io_lookup_buf_free_entry(ctx, ring_size);
 	if (!ibf) {
 		ptr = io_mem_alloc(ring_size);
-		if (IS_ERR(ptr))
-			return PTR_ERR(ptr);
+		if (!ptr)
+			return -ENOMEM;
 
 		/* Allocate and store deferred free entry */
 		ibf = kmalloc(sizeof(*ibf), GFP_KERNEL_ACCOUNT);
-- 
2.43.0

Re: [PATCH 1/3] io_uring: simplify io_mem_alloc return values

[Jens Axboe]

On 3/13/24 9:52 AM, Pavel Begunkov wrote:
> io_mem_alloc() returns a pointer on success and a pointer-encoded error
> otherwise. However, it can only fail with -ENOMEM, just return NULL on
> failure. PTR_ERR is usually pretty error prone.

I take that back, this is buggy - the io_rings_map() and friends return
an error pointer. So better to keep it consistent. Dropped this one.

-- 
Jens Axboe

Re: [PATCH 1/3] io_uring: simplify io_mem_alloc return values

[Pavel Begunkov]

On 3/13/24 20:50, Jens Axboe wrote:
> On 3/13/24 9:52 AM, Pavel Begunkov wrote:
>> io_mem_alloc() returns a pointer on success and a pointer-encoded error
>> otherwise. However, it can only fail with -ENOMEM, just return NULL on
>> failure. PTR_ERR is usually pretty error prone.
> 
> I take that back, this is buggy - the io_rings_map() and friends return
> an error pointer. So better to keep it consistent. Dropped this one.
Oh crap, you're right. Did something trigger it? Because tests
are suspiciously silent, I'd assume it's not really tested,
e.g. passing misaligned uptr

-- 
Pavel Begunkov

Re: [PATCH 1/3] io_uring: simplify io_mem_alloc return values

[Jens Axboe]

On 3/13/24 2:43 PM, Pavel Begunkov wrote:
> On 3/13/24 20:50, Jens Axboe wrote:
>> On 3/13/24 9:52 AM, Pavel Begunkov wrote:
>>> io_mem_alloc() returns a pointer on success and a pointer-encoded error
>>> otherwise. However, it can only fail with -ENOMEM, just return NULL on
>>> failure. PTR_ERR is usually pretty error prone.
>>
>> I take that back, this is buggy - the io_rings_map() and friends return
>> an error pointer. So better to keep it consistent. Dropped this one.
> Oh crap, you're right. Did something trigger it? Because tests
> are suspiciously silent, I'd assume it's not really tested,
> e.g. passing misaligned uptr

I was doing a cleanup on top, and noticed it while doing that. None of
the tests triggered it.

-- 
Jens Axboe

Re: [PATCH 1/3] io_uring: simplify io_mem_alloc return values

[Gabriel Krisman Bertazi]

Pavel Begunkov <asml.silence@gmail.com> writes:

> io_mem_alloc() returns a pointer on success and a pointer-encoded error
> otherwise. However, it can only fail with -ENOMEM, just return NULL on
> failure. PTR_ERR is usually pretty error prone.
>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  io_uring/io_uring.c | 14 +++++---------
>  io_uring/kbuf.c     |  4 ++--
>  2 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index e7d7a456b489..1d0eac0cc8aa 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -2802,12 +2802,8 @@ static void io_rings_free(struct io_ring_ctx *ctx)
>  void *io_mem_alloc(size_t size)
>  {
>  	gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP;
> -	void *ret;
>  
> -	ret = (void *) __get_free_pages(gfp, get_order(size));
> -	if (ret)
> -		return ret;
> -	return ERR_PTR(-ENOMEM);
> +	return (void *) __get_free_pages(gfp, get_order(size));
>  }
>  
>  static unsigned long rings_size(struct io_ring_ctx *ctx, unsigned int sq_entries,
> @@ -3762,8 +3758,8 @@ static __cold int io_allocate_scq_urings(struct io_ring_ctx *ctx,
>  	else
>  		rings = io_rings_map(ctx, p->cq_off.user_addr, size);
>  
> -	if (IS_ERR(rings))
> -		return PTR_ERR(rings);
> +	if (!rings)
> +		return -ENOMEM;
>

Sorry, I started reviewing this, got excited about the error path quick
fix, and didn't finish the review before it got it.

I think this change is broken for the ctx->flags & IORING_SETUP_NO_MMAP
case, because io_rings_map returns ERR_PTR, and not NULL.  In addition,
io_rings_map might fail for multiple reasons, and we want to propagate
the different error codes up here.

-- 
Gabriel Krisman Bertazi

Re: [PATCH 1/3] io_uring: simplify io_mem_alloc return values

[Jens Axboe]

On 3/13/24 4:32 PM, Gabriel Krisman Bertazi wrote:
> Pavel Begunkov <asml.silence@gmail.com> writes:
> 
>> io_mem_alloc() returns a pointer on success and a pointer-encoded error
>> otherwise. However, it can only fail with -ENOMEM, just return NULL on
>> failure. PTR_ERR is usually pretty error prone.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>  io_uring/io_uring.c | 14 +++++---------
>>  io_uring/kbuf.c     |  4 ++--
>>  2 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>> index e7d7a456b489..1d0eac0cc8aa 100644
>> --- a/io_uring/io_uring.c
>> +++ b/io_uring/io_uring.c
>> @@ -2802,12 +2802,8 @@ static void io_rings_free(struct io_ring_ctx *ctx)
>>  void *io_mem_alloc(size_t size)
>>  {
>>  	gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP;
>> -	void *ret;
>>  
>> -	ret = (void *) __get_free_pages(gfp, get_order(size));
>> -	if (ret)
>> -		return ret;
>> -	return ERR_PTR(-ENOMEM);
>> +	return (void *) __get_free_pages(gfp, get_order(size));
>>  }
>>  
>>  static unsigned long rings_size(struct io_ring_ctx *ctx, unsigned int sq_entries,
>> @@ -3762,8 +3758,8 @@ static __cold int io_allocate_scq_urings(struct io_ring_ctx *ctx,
>>  	else
>>  		rings = io_rings_map(ctx, p->cq_off.user_addr, size);
>>  
>> -	if (IS_ERR(rings))
>> -		return PTR_ERR(rings);
>> +	if (!rings)
>> +		return -ENOMEM;
>>
> 
> Sorry, I started reviewing this, got excited about the error path quick
> fix, and didn't finish the review before it got it.
> 
> I think this change is broken for the ctx->flags & IORING_SETUP_NO_MMAP
> case, because io_rings_map returns ERR_PTR, and not NULL.  In addition,
> io_rings_map might fail for multiple reasons, and we want to propagate
> the different error codes up here.

Yeah, see my reply from some hours ago. I dropped it back then.

-- 
Jens Axboe

Re: [PATCH 1/3] io_uring: simplify io_mem_alloc return values

[Gabriel Krisman Bertazi]

Jens Axboe <axboe@kernel.dk> writes:

> On 3/13/24 4:32 PM, Gabriel Krisman Bertazi wrote:
>> Pavel Begunkov <asml.silence@gmail.com> writes:
>> 
>>> io_mem_alloc() returns a pointer on success and a pointer-encoded error
>>> otherwise. However, it can only fail with -ENOMEM, just return NULL on
>>> failure. PTR_ERR is usually pretty error prone.
>>>
>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>> ---
>>>  io_uring/io_uring.c | 14 +++++---------
>>>  io_uring/kbuf.c     |  4 ++--
>>>  2 files changed, 7 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>>> index e7d7a456b489..1d0eac0cc8aa 100644
>>> --- a/io_uring/io_uring.c
>>> +++ b/io_uring/io_uring.c
>>> @@ -2802,12 +2802,8 @@ static void io_rings_free(struct io_ring_ctx *ctx)
>>>  void *io_mem_alloc(size_t size)
>>>  {
>>>  	gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP;
>>> -	void *ret;
>>>  
>>> -	ret = (void *) __get_free_pages(gfp, get_order(size));
>>> -	if (ret)
>>> -		return ret;
>>> -	return ERR_PTR(-ENOMEM);
>>> +	return (void *) __get_free_pages(gfp, get_order(size));
>>>  }
>>>  
>>>  static unsigned long rings_size(struct io_ring_ctx *ctx, unsigned int sq_entries,
>>> @@ -3762,8 +3758,8 @@ static __cold int io_allocate_scq_urings(struct io_ring_ctx *ctx,
>>>  	else
>>>  		rings = io_rings_map(ctx, p->cq_off.user_addr, size);
>>>  
>>> -	if (IS_ERR(rings))
>>> -		return PTR_ERR(rings);
>>> +	if (!rings)
>>> +		return -ENOMEM;
>>>
>> 
>> Sorry, I started reviewing this, got excited about the error path quick
>> fix, and didn't finish the review before it got it.
>> 
>> I think this change is broken for the ctx->flags & IORING_SETUP_NO_MMAP
>> case, because io_rings_map returns ERR_PTR, and not NULL.  In addition,
>> io_rings_map might fail for multiple reasons, and we want to propagate
>> the different error codes up here.
>
> Yeah, see my reply from some hours ago. I dropped it back then.

ah, thanks.  I've configured lei to fetch the io_uring list every few
hours. This ended up fetching part of the thread at first, and I only saw
it dropped in the next fetch, after I sent the email. sorry for the noise.

-- 
Gabriel Krisman Bertazi

Re: [PATCH 1/3] io_uring: simplify io_mem_alloc return values

[Jens Axboe]

On 3/13/24 5:43 PM, Gabriel Krisman Bertazi wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> On 3/13/24 4:32 PM, Gabriel Krisman Bertazi wrote:
>>> Pavel Begunkov <asml.silence@gmail.com> writes:
>>>
>>>> io_mem_alloc() returns a pointer on success and a pointer-encoded error
>>>> otherwise. However, it can only fail with -ENOMEM, just return NULL on
>>>> failure. PTR_ERR is usually pretty error prone.
>>>>
>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>>> ---
>>>>  io_uring/io_uring.c | 14 +++++---------
>>>>  io_uring/kbuf.c     |  4 ++--
>>>>  2 files changed, 7 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
>>>> index e7d7a456b489..1d0eac0cc8aa 100644
>>>> --- a/io_uring/io_uring.c
>>>> +++ b/io_uring/io_uring.c
>>>> @@ -2802,12 +2802,8 @@ static void io_rings_free(struct io_ring_ctx *ctx)
>>>>  void *io_mem_alloc(size_t size)
>>>>  {
>>>>  	gfp_t gfp = GFP_KERNEL_ACCOUNT | __GFP_ZERO | __GFP_NOWARN | __GFP_COMP;
>>>> -	void *ret;
>>>>  
>>>> -	ret = (void *) __get_free_pages(gfp, get_order(size));
>>>> -	if (ret)
>>>> -		return ret;
>>>> -	return ERR_PTR(-ENOMEM);
>>>> +	return (void *) __get_free_pages(gfp, get_order(size));
>>>>  }
>>>>  
>>>>  static unsigned long rings_size(struct io_ring_ctx *ctx, unsigned int sq_entries,
>>>> @@ -3762,8 +3758,8 @@ static __cold int io_allocate_scq_urings(struct io_ring_ctx *ctx,
>>>>  	else
>>>>  		rings = io_rings_map(ctx, p->cq_off.user_addr, size);
>>>>  
>>>> -	if (IS_ERR(rings))
>>>> -		return PTR_ERR(rings);
>>>> +	if (!rings)
>>>> +		return -ENOMEM;
>>>>
>>>
>>> Sorry, I started reviewing this, got excited about the error path quick
>>> fix, and didn't finish the review before it got it.
>>>
>>> I think this change is broken for the ctx->flags & IORING_SETUP_NO_MMAP
>>> case, because io_rings_map returns ERR_PTR, and not NULL.  In addition,
>>> io_rings_map might fail for multiple reasons, and we want to propagate
>>> the different error codes up here.
>>
>> Yeah, see my reply from some hours ago. I dropped it back then.
> 
> ah, thanks.  I've configured lei to fetch the io_uring list every few
> hours. This ended up fetching part of the thread at first, and I only saw
> it dropped in the next fetch, after I sent the email. sorry for the noise.

Oh it's fine, I'd rather have one extra review than none at all :-)

-- 
Jens Axboe

[PATCH 2/3] io_uring: simplify io_pages_free

[Pavel Begunkov]

We never pass a null (top-level) pointer, remove the check.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 1d0eac0cc8aa..cfe1413511ca 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -2696,13 +2696,9 @@ void io_mem_free(void *ptr)
 
 static void io_pages_free(struct page ***pages, int npages)
 {
-	struct page **page_array;
+	struct page **page_array = *pages;
 	int i;
 
-	if (!pages)
-		return;
-
-	page_array = *pages;
 	if (!page_array)
 		return;
 
-- 
2.43.0

[PATCH 3/3] io_uring/kbuf: rename is_mapped

[Pavel Begunkov]

In buffer lists we have ->is_mapped as well as ->is_mmap, it's
pretty hard to stay sane double checking which one means what,
and in the long run there is a high chance of an eventual bug.
Rename ->is_mapped into ->is_buf_ring.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/kbuf.c | 20 ++++++++++----------
 io_uring/kbuf.h |  2 +-
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
index 0677eae92e79..4cf742749870 100644
--- a/io_uring/kbuf.c
+++ b/io_uring/kbuf.c
@@ -199,7 +199,7 @@ void __user *io_buffer_select(struct io_kiocb *req, size_t *len,
 
 	bl = io_buffer_get_list(ctx, req->buf_index);
 	if (likely(bl)) {
-		if (bl->is_mapped)
+		if (bl->is_buf_ring)
 			ret = io_ring_buffer_select(req, len, bl, issue_flags);
 		else
 			ret = io_provided_buffer_select(req, len, bl);
@@ -253,7 +253,7 @@ static int __io_remove_buffers(struct io_ring_ctx *ctx,
 	if (!nbufs)
 		return 0;
 
-	if (bl->is_mapped) {
+	if (bl->is_buf_ring) {
 		i = bl->buf_ring->tail - bl->head;
 		if (bl->is_mmap) {
 			/*
@@ -274,7 +274,7 @@ static int __io_remove_buffers(struct io_ring_ctx *ctx,
 		}
 		/* make sure it's seen as empty */
 		INIT_LIST_HEAD(&bl->buf_list);
-		bl->is_mapped = 0;
+		bl->is_buf_ring = 0;
 		return i;
 	}
 
@@ -361,7 +361,7 @@ int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags)
 	if (bl) {
 		ret = -EINVAL;
 		/* can't use provide/remove buffers command on mapped buffers */
-		if (!bl->is_mapped)
+		if (!bl->is_buf_ring)
 			ret = __io_remove_buffers(ctx, bl, p->nbufs);
 	}
 	io_ring_submit_unlock(ctx, issue_flags);
@@ -519,7 +519,7 @@ int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags)
 		}
 	}
 	/* can't add buffers via this command for a mapped buffer ring */
-	if (bl->is_mapped) {
+	if (bl->is_buf_ring) {
 		ret = -EINVAL;
 		goto err;
 	}
@@ -575,7 +575,7 @@ static int io_pin_pbuf_ring(struct io_uring_buf_reg *reg,
 	bl->buf_pages = pages;
 	bl->buf_nr_pages = nr_pages;
 	bl->buf_ring = br;
-	bl->is_mapped = 1;
+	bl->is_buf_ring = 1;
 	bl->is_mmap = 0;
 	return 0;
 error_unpin:
@@ -642,7 +642,7 @@ static int io_alloc_pbuf_ring(struct io_ring_ctx *ctx,
 	}
 	ibf->inuse = 1;
 	bl->buf_ring = ibf->mem;
-	bl->is_mapped = 1;
+	bl->is_buf_ring = 1;
 	bl->is_mmap = 1;
 	return 0;
 }
@@ -688,7 +688,7 @@ int io_register_pbuf_ring(struct io_ring_ctx *ctx, void __user *arg)
 	bl = io_buffer_get_list(ctx, reg.bgid);
 	if (bl) {
 		/* if mapped buffer ring OR classic exists, don't allow */
-		if (bl->is_mapped || !list_empty(&bl->buf_list))
+		if (bl->is_buf_ring || !list_empty(&bl->buf_list))
 			return -EEXIST;
 	} else {
 		free_bl = bl = kzalloc(sizeof(*bl), GFP_KERNEL);
@@ -730,7 +730,7 @@ int io_unregister_pbuf_ring(struct io_ring_ctx *ctx, void __user *arg)
 	bl = io_buffer_get_list(ctx, reg.bgid);
 	if (!bl)
 		return -ENOENT;
-	if (!bl->is_mapped)
+	if (!bl->is_buf_ring)
 		return -EINVAL;
 
 	__io_remove_buffers(ctx, bl, -1U);
@@ -757,7 +757,7 @@ int io_register_pbuf_status(struct io_ring_ctx *ctx, void __user *arg)
 	bl = io_buffer_get_list(ctx, buf_status.buf_group);
 	if (!bl)
 		return -ENOENT;
-	if (!bl->is_mapped)
+	if (!bl->is_buf_ring)
 		return -EINVAL;
 
 	buf_status.head = bl->head;
diff --git a/io_uring/kbuf.h b/io_uring/kbuf.h
index 5218bfd79e87..1c7b654ee726 100644
--- a/io_uring/kbuf.h
+++ b/io_uring/kbuf.h
@@ -26,7 +26,7 @@ struct io_buffer_list {
 	__u16 mask;
 
 	/* ring mapped provided buffers */
-	__u8 is_mapped;
+	__u8 is_buf_ring;
 	/* ring mapped provided buffers, but mmap'ed by application */
 	__u8 is_mmap;
 	/* bl is visible from an RCU point of view for lookup */
-- 
2.43.0

Re: [PATCH 0/3] small rings clean up

[Jens Axboe]

On Wed, 13 Mar 2024 15:52:38 +0000, Pavel Begunkov wrote:
> Some disconnected cleanup patches split out of a larger series
> as they make sense by themselves.
> 
> Pavel Begunkov (3):
>   io_uring: simplify io_mem_alloc return values
>   io_uring: simplify io_pages_free
>   io_uring/kbuf: rename is_mapped
> 
> [...]

Applied, thanks!

[1/3] io_uring: simplify io_mem_alloc return values
      commit: ccd2254dfe739e37876e984c44caa6c7f318c728
[2/3] io_uring: simplify io_pages_free
      commit: 91e0c2c9640c959f1f50f6f80a6a7e93562a9272
[3/3] io_uring/kbuf: rename is_mapped
      commit: 585abfacf0c33b11ab66a69ba884a70c9459a63c

Best regards,
-- 
Jens Axboe