[PATCH v2] io_uring/futex: Factor out common free logic into io_free_ifd()

[PATCH v2] io_uring/futex: Factor out common free logic into io_free_ifd()

[Sidong Yang]

This patch introduces io_free_ifd() that try to cache or free
io_futex_data. It could be used for completion. It also could be used
for error path in io_futex_wait(). Old code just release the ifd but it
could be cached.

Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
---
v2: use io_free_ifd() for completion
---
 io_uring/futex.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/io_uring/futex.c b/io_uring/futex.c
index e29662f039e1..94a7159f9cff 100644
--- a/io_uring/futex.c
+++ b/io_uring/futex.c
@@ -44,6 +44,13 @@ void io_futex_cache_free(struct io_ring_ctx *ctx)
 	io_alloc_cache_free(&ctx->futex_cache, kfree);
 }
 
+static void io_free_ifd(struct io_ring_ctx *ctx, struct io_futex_data *ifd)
+{
+	if (!io_alloc_cache_put(&ctx->futex_cache, ifd)) {
+		kfree(ifd);
+	}
+}
+
 static void __io_futex_complete(struct io_kiocb *req, struct io_tw_state *ts)
 {
 	req->async_data = NULL;
@@ -57,8 +64,7 @@ static void io_futex_complete(struct io_kiocb *req, struct io_tw_state *ts)
 	struct io_ring_ctx *ctx = req->ctx;
 
 	io_tw_lock(ctx, ts);
-	if (!io_alloc_cache_put(&ctx->futex_cache, ifd))
-		kfree(ifd);
+	io_free_ifd(ctx, ifd);
 	__io_futex_complete(req, ts);
 }
 
@@ -353,13 +359,13 @@ int io_futex_wait(struct io_kiocb *req, unsigned int issue_flags)
 		return IOU_ISSUE_SKIP_COMPLETE;
 	}
 
+	io_free_ifd(ctx, ifd);
 done_unlock:
 	io_ring_submit_unlock(ctx, issue_flags);
 done:
 	if (ret < 0)
 		req_set_fail(req);
 	io_req_set_res(req, ret, 0);
-	kfree(ifd);
 	return IOU_OK;
 }
 
-- 
2.43.0

RE: [PATCH v2] io_uring/futex: Factor out common free logic into io_free_ifd()

[lizetao]

Hi,

> -----Original Message-----
> From: Sidong Yang <sidong.yang@furiosa.ai>
> Sent: Friday, January 24, 2025 11:44 PM
> To: io-uring <io-uring@vger.kernel.org>; Jens Axboe <axboe@kernel.dk>
> Cc: Sidong Yang <sidong.yang@furiosa.ai>
> Subject: [PATCH v2] io_uring/futex: Factor out common free logic into
> io_free_ifd()
> 
> This patch introduces io_free_ifd() that try to cache or free io_futex_data. It
> could be used for completion. It also could be used for error path in
> io_futex_wait(). Old code just release the ifd but it could be cached.
> 
> Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
> ---
> v2: use io_free_ifd() for completion
> ---
>  io_uring/futex.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/io_uring/futex.c b/io_uring/futex.c index
> e29662f039e1..94a7159f9cff 100644
> --- a/io_uring/futex.c
> +++ b/io_uring/futex.c
> @@ -44,6 +44,13 @@ void io_futex_cache_free(struct io_ring_ctx *ctx)
>  	io_alloc_cache_free(&ctx->futex_cache, kfree);  }
> 
> +static void io_free_ifd(struct io_ring_ctx *ctx, struct io_futex_data
> +*ifd) {
> +	if (!io_alloc_cache_put(&ctx->futex_cache, ifd)) {
> +		kfree(ifd);
> +	}
> +}

inline static void io_free_ifd(struct io_ring_ctx *ctx, struct io_futex_data *ifd)
{
	if (!io_alloc_cache_put(&ctx->futex_cache, ifd))
		kfree(ifd);
}

Maybe inline function would be better here, and the code format needs to be fine-tuned.

> +
>  static void __io_futex_complete(struct io_kiocb *req, struct io_tw_state *ts)  {
>  	req->async_data = NULL;
> @@ -57,8 +64,7 @@ static void io_futex_complete(struct io_kiocb *req, struct
> io_tw_state *ts)
>  	struct io_ring_ctx *ctx = req->ctx;
> 
>  	io_tw_lock(ctx, ts);
> -	if (!io_alloc_cache_put(&ctx->futex_cache, ifd))
> -		kfree(ifd);
> +	io_free_ifd(ctx, ifd);
>  	__io_futex_complete(req, ts);
>  }
> 
> @@ -353,13 +359,13 @@ int io_futex_wait(struct io_kiocb *req, unsigned int
> issue_flags)
>  		return IOU_ISSUE_SKIP_COMPLETE;
>  	}
> 
> +	io_free_ifd(ctx, ifd);
>  done_unlock:
>  	io_ring_submit_unlock(ctx, issue_flags);
>  done:
>  	if (ret < 0)
>  		req_set_fail(req);
>  	io_req_set_res(req, ret, 0);
> -	kfree(ifd);

Since kfree() is deleted, it is redundant to initialize ifd to NULL. You can consider modifying it like this:
-       struct io_futex_data *ifd = NULL;
        struct futex_hash_bucket *hb;
+       struct io_futex_data *ifd;

>  	return IOU_OK;
>  }
> 
> --
> 2.43.0
> 

--
Li Zetao

Re: [PATCH v2] io_uring/futex: Factor out common free logic into io_free_ifd()

[Sidong Yang]

On Sat, Jan 25, 2025 at 07:17:06AM +0000, lizetao wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Sidong Yang <sidong.yang@furiosa.ai>
> > Sent: Friday, January 24, 2025 11:44 PM
> > To: io-uring <io-uring@vger.kernel.org>; Jens Axboe <axboe@kernel.dk>
> > Cc: Sidong Yang <sidong.yang@furiosa.ai>
> > Subject: [PATCH v2] io_uring/futex: Factor out common free logic into
> > io_free_ifd()
> > 
> > This patch introduces io_free_ifd() that try to cache or free io_futex_data. It
> > could be used for completion. It also could be used for error path in
> > io_futex_wait(). Old code just release the ifd but it could be cached.
> > 
> > Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai>
> > ---
> > v2: use io_free_ifd() for completion
> > ---
> >  io_uring/futex.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/io_uring/futex.c b/io_uring/futex.c index
> > e29662f039e1..94a7159f9cff 100644
> > --- a/io_uring/futex.c
> > +++ b/io_uring/futex.c
> > @@ -44,6 +44,13 @@ void io_futex_cache_free(struct io_ring_ctx *ctx)
> >  	io_alloc_cache_free(&ctx->futex_cache, kfree);  }
> > 
> > +static void io_free_ifd(struct io_ring_ctx *ctx, struct io_futex_data
> > +*ifd) {
> > +	if (!io_alloc_cache_put(&ctx->futex_cache, ifd)) {
> > +		kfree(ifd);
> > +	}
> > +}
> 
> inline static void io_free_ifd(struct io_ring_ctx *ctx, struct io_futex_data *ifd)
> {
> 	if (!io_alloc_cache_put(&ctx->futex_cache, ifd))
> 		kfree(ifd);
> }
> 
> Maybe inline function would be better here, and the code format needs to be fine-tuned.

Thanks, I missed formatting. inline also good.
> 
> > +
> >  static void __io_futex_complete(struct io_kiocb *req, struct io_tw_state *ts)  {
> >  	req->async_data = NULL;
> > @@ -57,8 +64,7 @@ static void io_futex_complete(struct io_kiocb *req, struct
> > io_tw_state *ts)
> >  	struct io_ring_ctx *ctx = req->ctx;
> > 
> >  	io_tw_lock(ctx, ts);
> > -	if (!io_alloc_cache_put(&ctx->futex_cache, ifd))
> > -		kfree(ifd);
> > +	io_free_ifd(ctx, ifd);
> >  	__io_futex_complete(req, ts);
> >  }
> > 
> > @@ -353,13 +359,13 @@ int io_futex_wait(struct io_kiocb *req, unsigned int
> > issue_flags)
> >  		return IOU_ISSUE_SKIP_COMPLETE;
> >  	}
> > 
> > +	io_free_ifd(ctx, ifd);
> >  done_unlock:
> >  	io_ring_submit_unlock(ctx, issue_flags);
> >  done:
> >  	if (ret < 0)
> >  		req_set_fail(req);
> >  	io_req_set_res(req, ret, 0);
> > -	kfree(ifd);
> 
> Since kfree() is deleted, it is redundant to initialize ifd to NULL. You can consider modifying it like this:
> -       struct io_futex_data *ifd = NULL;
>         struct futex_hash_bucket *hb;
> +       struct io_futex_data *ifd;

This is also good. but is it actually orthogonal to this patch? ifd could be initialized with io_alloc_ifd().

Thanks,
Sidong

> 
> >  	return IOU_OK;
> >  }
> > 
> > --
> > 2.43.0
> > 
> 
> --
> Li Zetao
>