[PATCH 1/5] io_uring/rsrc: split out io_free_node() helper

[PATCH 1/5] io_uring/rsrc: split out io_free_node() helper

[Caleb Sander Mateos]

Split the freeing of the io_rsrc_node from io_free_rsrc_node(), for use
with nodes that haven't been fully initialized.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 io_uring/rsrc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index 45bfb37bca1e..d941256f0d8c 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -485,10 +485,16 @@ int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
 		req_set_fail(req);
 	io_req_set_res(req, ret, 0);
 	return IOU_OK;
 }
 
+static void io_free_node(struct io_ring_ctx *ctx, struct io_rsrc_node *node)
+{
+	if (!io_alloc_cache_put(&ctx->node_cache, node))
+		kvfree(node);
+}
+
 void io_free_rsrc_node(struct io_ring_ctx *ctx, struct io_rsrc_node *node)
 {
 	if (node->tag)
 		io_post_aux_cqe(ctx, node->tag, 0, 0);
 
@@ -504,12 +510,11 @@ void io_free_rsrc_node(struct io_ring_ctx *ctx, struct io_rsrc_node *node)
 	default:
 		WARN_ON_ONCE(1);
 		break;
 	}
 
-	if (!io_alloc_cache_put(&ctx->node_cache, node))
-		kvfree(node);
+	io_free_node(ctx, node);
 }
 
 int io_sqe_files_unregister(struct io_ring_ctx *ctx)
 {
 	if (!ctx->file_table.data.nr)
-- 
2.45.2

[PATCH 2/5] io_uring/rsrc: free io_rsrc_node using kfree()

[Caleb Sander Mateos]

io_rsrc_node_alloc() calls io_cache_alloc(), which uses kmalloc() to
allocate the node. So it can be freed with kfree() instead of kvfree().

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 io_uring/rsrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index d941256f0d8c..748a09cfaeaa 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -488,11 +488,11 @@ int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
 }
 
 static void io_free_node(struct io_ring_ctx *ctx, struct io_rsrc_node *node)
 {
 	if (!io_alloc_cache_put(&ctx->node_cache, node))
-		kvfree(node);
+		kfree(node);
 }
 
 void io_free_rsrc_node(struct io_ring_ctx *ctx, struct io_rsrc_node *node)
 {
 	if (node->tag)
-- 
2.45.2

[PATCH 3/5] io_uring/rsrc: call io_free_node() on io_sqe_buffer_register() failure

[Caleb Sander Mateos]

io_sqe_buffer_register() currently calls io_put_rsrc_node() if it fails
to fully set up the io_rsrc_node. io_put_rsrc_node() is more involved
than necessary, since we already know the reference count will reach 0
and no io_mapped_ubuf has been attached to the node yet.

So just call io_free_node() to release the node's memory. This also
avoids the need to temporarily set the node's buf pointer to NULL.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 io_uring/rsrc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index 748a09cfaeaa..398c6f427bcc 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -780,11 +780,10 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,
 		return NULL;
 
 	node = io_rsrc_node_alloc(ctx, IORING_RSRC_BUFFER);
 	if (!node)
 		return ERR_PTR(-ENOMEM);
-	node->buf = NULL;
 
 	ret = -ENOMEM;
 	pages = io_pin_pages((unsigned long) iov->iov_base, iov->iov_len,
 				&nr_pages);
 	if (IS_ERR(pages)) {
@@ -837,11 +836,11 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,
 done:
 	if (ret) {
 		if (imu)
 			io_free_imu(ctx, imu);
 		if (node)
-			io_put_rsrc_node(ctx, node);
+			io_free_node(ctx, node);
 		node = ERR_PTR(ret);
 	}
 	kvfree(pages);
 	return node;
 }
-- 
2.45.2

Re: [PATCH 3/5] io_uring/rsrc: call io_free_node() on io_sqe_buffer_register() failure

[Pavel Begunkov]

On 2/28/25 23:59, Caleb Sander Mateos wrote:
> io_sqe_buffer_register() currently calls io_put_rsrc_node() if it fails
> to fully set up the io_rsrc_node. io_put_rsrc_node() is more involved
> than necessary, since we already know the reference count will reach 0
> and no io_mapped_ubuf has been attached to the node yet.
> 
> So just call io_free_node() to release the node's memory. This also
> avoids the need to temporarily set the node's buf pointer to NULL.
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> ---
>   io_uring/rsrc.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
> index 748a09cfaeaa..398c6f427bcc 100644
> --- a/io_uring/rsrc.c
> +++ b/io_uring/rsrc.c
> @@ -780,11 +780,10 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,
>   		return NULL;
>   
>   	node = io_rsrc_node_alloc(ctx, IORING_RSRC_BUFFER);
>   	if (!node)
>   		return ERR_PTR(-ENOMEM);
> -	node->buf = NULL;

It's better to have it zeroed than set to a freed / invalid
value, it's a slow path.

-- 
Pavel Begunkov

Re: [PATCH 3/5] io_uring/rsrc: call io_free_node() on io_sqe_buffer_register() failure

[Jens Axboe]

On 2/28/25 6:31 PM, Pavel Begunkov wrote:
> On 2/28/25 23:59, Caleb Sander Mateos wrote:
>> io_sqe_buffer_register() currently calls io_put_rsrc_node() if it fails
>> to fully set up the io_rsrc_node. io_put_rsrc_node() is more involved
>> than necessary, since we already know the reference count will reach 0
>> and no io_mapped_ubuf has been attached to the node yet.
>>
>> So just call io_free_node() to release the node's memory. This also
>> avoids the need to temporarily set the node's buf pointer to NULL.
>>
>> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
>> ---
>>   io_uring/rsrc.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
>> index 748a09cfaeaa..398c6f427bcc 100644
>> --- a/io_uring/rsrc.c
>> +++ b/io_uring/rsrc.c
>> @@ -780,11 +780,10 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,
>>           return NULL;
>>         node = io_rsrc_node_alloc(ctx, IORING_RSRC_BUFFER);
>>       if (!node)
>>           return ERR_PTR(-ENOMEM);
>> -    node->buf = NULL;
> 
> It's better to have it zeroed than set to a freed / invalid
> value, it's a slow path.

Agree, let's leave the clear, I don't like passing uninitialized memory
around.

-- 
Jens Axboe

Re: [PATCH 3/5] io_uring/rsrc: call io_free_node() on io_sqe_buffer_register() failure

[Caleb Sander Mateos]

On Fri, Feb 28, 2025 at 6:23 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 2/28/25 6:31 PM, Pavel Begunkov wrote:
> > On 2/28/25 23:59, Caleb Sander Mateos wrote:
> >> io_sqe_buffer_register() currently calls io_put_rsrc_node() if it fails
> >> to fully set up the io_rsrc_node. io_put_rsrc_node() is more involved
> >> than necessary, since we already know the reference count will reach 0
> >> and no io_mapped_ubuf has been attached to the node yet.
> >>
> >> So just call io_free_node() to release the node's memory. This also
> >> avoids the need to temporarily set the node's buf pointer to NULL.
> >>
> >> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> >> ---
> >>   io_uring/rsrc.c | 3 +--
> >>   1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
> >> index 748a09cfaeaa..398c6f427bcc 100644
> >> --- a/io_uring/rsrc.c
> >> +++ b/io_uring/rsrc.c
> >> @@ -780,11 +780,10 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,
> >>           return NULL;
> >>         node = io_rsrc_node_alloc(ctx, IORING_RSRC_BUFFER);
> >>       if (!node)
> >>           return ERR_PTR(-ENOMEM);
> >> -    node->buf = NULL;
> >
> > It's better to have it zeroed than set to a freed / invalid
> > value, it's a slow path.
>
> Agree, let's leave the clear, I don't like passing uninitialized memory
> around.

io_rsrc_node_alloc() actually does already zero all of io_rsrc_node's
fields (file_ptr is in a union with buf):

struct io_rsrc_node *io_rsrc_node_alloc(struct io_ring_ctx *ctx, int type)
{
        struct io_rsrc_node *node;

        node = io_cache_alloc(&ctx->node_cache, GFP_KERNEL);
        if (node) {
                node->type = type;
                node->refs = 1;
                node->tag = 0;
                node->file_ptr = 0;
        }
        return node;
}

How about I remove the redundant node->buf = NULL; in a separate
patch, since it's not dependent on switching the error path to
io_free_node()?

Thanks,
Caleb

[PATCH 4/5] io_uring/rsrc: avoid NULL node check on io_sqe_buffer_register() failure

[Caleb Sander Mateos]

The done: label is only reachable if node is non-NULL. So don't bother
checking, just call io_free_node().

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 io_uring/rsrc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index 398c6f427bcc..95def9e5f3a7 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -835,12 +835,11 @@ static struct io_rsrc_node *io_sqe_buffer_register(struct io_ring_ctx *ctx,
 	}
 done:
 	if (ret) {
 		if (imu)
 			io_free_imu(ctx, imu);
-		if (node)
-			io_free_node(ctx, node);
+		io_free_node(ctx, node);
 		node = ERR_PTR(ret);
 	}
 	kvfree(pages);
 	return node;
 }
-- 
2.45.2

[PATCH 5/5] io_uring/rsrc: skip NULL file/buffer checks in io_free_rsrc_node()

[Caleb Sander Mateos]

io_rsrc_node's of type IORING_RSRC_FILE always have a file attached
immediately after they are allocated. IORING_RSRC_BUFFER nodes won't be
returned from io_sqe_buffer_register()/io_buffer_register_bvec() until
they have a io_mapped_ubuf attached.

So remove the checks for a NULL file/buffer in io_free_rsrc_node().

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
 io_uring/rsrc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index 95def9e5f3a7..c8b79ebcff68 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -498,16 +498,14 @@ void io_free_rsrc_node(struct io_ring_ctx *ctx, struct io_rsrc_node *node)
 	if (node->tag)
 		io_post_aux_cqe(ctx, node->tag, 0, 0);
 
 	switch (node->type) {
 	case IORING_RSRC_FILE:
-		if (io_slot_file(node))
-			fput(io_slot_file(node));
+		fput(io_slot_file(node));
 		break;
 	case IORING_RSRC_BUFFER:
-		if (node->buf)
-			io_buffer_unmap(ctx, node->buf);
+		io_buffer_unmap(ctx, node->buf);
 		break;
 	default:
 		WARN_ON_ONCE(1);
 		break;
 	}
-- 
2.45.2

RE: [PATCH 1/5] io_uring/rsrc: split out io_free_node() helper

[lizetao]

Hi,

> -----Original Message-----
> From: Caleb Sander Mateos <csander@purestorage.com>
> Sent: Saturday, March 1, 2025 7:59 AM
> To: Jens Axboe <axboe@kernel.dk>; Pavel Begunkov <asml.silence@gmail.com>
> Cc: Caleb Sander Mateos <csander@purestorage.com>; io-
> uring@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 1/5] io_uring/rsrc: split out io_free_node() helper
> 
> Split the freeing of the io_rsrc_node from io_free_rsrc_node(), for use with
> nodes that haven't been fully initialized.
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> ---
>  io_uring/rsrc.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c index 45bfb37bca1e..d941256f0d8c
> 100644
> --- a/io_uring/rsrc.c
> +++ b/io_uring/rsrc.c
> @@ -485,10 +485,16 @@ int io_files_update(struct io_kiocb *req, unsigned int
> issue_flags)
>  		req_set_fail(req);
>  	io_req_set_res(req, ret, 0);
>  	return IOU_OK;
>  }
> 
> +static void io_free_node(struct io_ring_ctx *ctx, struct io_rsrc_node
> +*node) {

Would it be better to take a io_alloc_cache as parameter and put it into alloc_cache.h 
as a general function, so other modules can use it too, just like io_uring/futex.

> +	if (!io_alloc_cache_put(&ctx->node_cache, node))
> +		kvfree(node);
> +}
> +
>  void io_free_rsrc_node(struct io_ring_ctx *ctx, struct io_rsrc_node *node)  {
>  	if (node->tag)
>  		io_post_aux_cqe(ctx, node->tag, 0, 0);
> 
> @@ -504,12 +510,11 @@ void io_free_rsrc_node(struct io_ring_ctx *ctx,
> struct io_rsrc_node *node)
>  	default:
>  		WARN_ON_ONCE(1);
>  		break;
>  	}
> 
> -	if (!io_alloc_cache_put(&ctx->node_cache, node))
> -		kvfree(node);
> +	io_free_node(ctx, node);
>  }
> 
>  int io_sqe_files_unregister(struct io_ring_ctx *ctx)  {
>  	if (!ctx->file_table.data.nr)
> --
> 2.45.2
> 

---
Li Zetao

Re: [PATCH 1/5] io_uring/rsrc: split out io_free_node() helper

[Jens Axboe]

On Fri, 28 Feb 2025 16:59:10 -0700, Caleb Sander Mateos wrote:
> Split the freeing of the io_rsrc_node from io_free_rsrc_node(), for use
> with nodes that haven't been fully initialized.
> 
> 

Applied, thanks!

[1/5] io_uring/rsrc: split out io_free_node() helper
      commit: 6a53541829662c8f1357f522a1d6315179442bf7
[2/5] io_uring/rsrc: free io_rsrc_node using kfree()
      commit: a387b96d2a9687201318826d23c770eb794c778e
[3/5] io_uring/rsrc: call io_free_node() on io_sqe_buffer_register() failure
      commit: 13f7f9686e928dae352972a1a95b50b2d5e80d42
[4/5] io_uring/rsrc: avoid NULL node check on io_sqe_buffer_register() failure
      commit: 6e5d321a08e30f746d63fc56e7ea5c46b06fbe99
[5/5] io_uring/rsrc: skip NULL file/buffer checks in io_free_rsrc_node()
      commit: fe21a4532ef2a6852c89b352cb8ded0d37b4745c

Best regards,
-- 
Jens Axboe