On Sat, Jan 25, 2025 at 07:17:06AM +0000, lizetao wrote:
> Hi,
>
> > -----Original Message-----
> > From: Sidong Yang <sidong.yang@xxxxxxxxxx>
> > Sent: Friday, January 24, 2025 11:44 PM
> > To: io-uring <io-uring@xxxxxxxxxxxxxxx>; Jens Axboe <axboe@xxxxxxxxx>
> > Cc: Sidong Yang <sidong.yang@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > ---
> > 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
>
