| Message ID | 20240329155054.1936666-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: return void from io_put_kbuf_comp() | expand |
On 3/29/24 9:50 AM, Ming Lei wrote: > The two callers don't handle the return value of io_put_kbuf_comp(), so > change its return type into void. We might want to consider changing the name of it too, it's a bit different in that it's just recyling/dropping this kbuf rather than posting a completion on behalf of it. Maybe io_kbuf_drop() would be better. Would distuingish it from the normal use cases of "drop this kbuf and return the cflags representation of it, as I'm posting a completionw ith it". > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > io_uring/kbuf.h | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/io_uring/kbuf.h b/io_uring/kbuf.h > index 1c7b654ee726..86931fa655ad 100644 > --- a/io_uring/kbuf.h > +++ b/io_uring/kbuf.h > @@ -119,18 +119,12 @@ static inline void __io_put_kbuf_list(struct io_kiocb *req, > } > } > > -static inline unsigned int io_put_kbuf_comp(struct io_kiocb *req) > +static inline void io_put_kbuf_comp(struct io_kiocb *req) > { > - unsigned int ret; > - > lockdep_assert_held(&req->ctx->completion_lock); > > - if (!(req->flags & (REQ_F_BUFFER_SELECTED|REQ_F_BUFFER_RING))) > - return 0; > - > - ret = IORING_CQE_F_BUFFER | (req->buf_index << IORING_CQE_BUFFER_SHIFT); > - __io_put_kbuf_list(req, &req->ctx->io_buffers_comp); > - return ret; > + if (req->flags & (REQ_F_BUFFER_SELECTED|REQ_F_BUFFER_RING)) > + __io_put_kbuf_list(req, &req->ctx->io_buffers_comp); > } If you post a v2 with the above suggestion, let's please just keep the flags checking how it is. It's consistent with what we do elsewhere.
On Fri, Mar 29, 2024 at 10:07:13AM -0600, Jens Axboe wrote: > On 3/29/24 9:50 AM, Ming Lei wrote: > > The two callers don't handle the return value of io_put_kbuf_comp(), so > > change its return type into void. > > We might want to consider changing the name of it too, it's a bit > different in that it's just recyling/dropping this kbuf rather than > posting a completion on behalf of it. > > Maybe io_kbuf_drop() would be better. Would distuingish it from the > normal use cases of "drop this kbuf and return the cflags representation > of it, as I'm posting a completionw ith it". > > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > io_uring/kbuf.h | 12 +++--------- > > 1 file changed, 3 insertions(+), 9 deletions(-) > > > > diff --git a/io_uring/kbuf.h b/io_uring/kbuf.h > > index 1c7b654ee726..86931fa655ad 100644 > > --- a/io_uring/kbuf.h > > +++ b/io_uring/kbuf.h > > @@ -119,18 +119,12 @@ static inline void __io_put_kbuf_list(struct io_kiocb *req, > > } > > } > > > > -static inline unsigned int io_put_kbuf_comp(struct io_kiocb *req) > > +static inline void io_put_kbuf_comp(struct io_kiocb *req) > > { > > - unsigned int ret; > > - > > lockdep_assert_held(&req->ctx->completion_lock); > > > > - if (!(req->flags & (REQ_F_BUFFER_SELECTED|REQ_F_BUFFER_RING))) > > - return 0; > > - > > - ret = IORING_CQE_F_BUFFER | (req->buf_index << IORING_CQE_BUFFER_SHIFT); > > - __io_put_kbuf_list(req, &req->ctx->io_buffers_comp); > > - return ret; > > + if (req->flags & (REQ_F_BUFFER_SELECTED|REQ_F_BUFFER_RING)) > > + __io_put_kbuf_list(req, &req->ctx->io_buffers_comp); > > } > > If you post a v2 with the above suggestion, let's please just keep the > flags checking how it is. It's consistent with what we do elsewhere. OK, I can rename it as io_kbuf_drop() in v2 and keep the original check style. Also the following patch removes one io_put_kbuf_comp(), I will post v2 when that patch gets feedback or merged. https://lore.kernel.org/io-uring/20240329154712.1936153-1-ming.lei@redhat.com/ thanks, Ming
diff --git a/io_uring/kbuf.h b/io_uring/kbuf.h index 1c7b654ee726..86931fa655ad 100644 --- a/io_uring/kbuf.h +++ b/io_uring/kbuf.h @@ -119,18 +119,12 @@ static inline void __io_put_kbuf_list(struct io_kiocb *req, } } -static inline unsigned int io_put_kbuf_comp(struct io_kiocb *req) +static inline void io_put_kbuf_comp(struct io_kiocb *req) { - unsigned int ret; - lockdep_assert_held(&req->ctx->completion_lock); - if (!(req->flags & (REQ_F_BUFFER_SELECTED|REQ_F_BUFFER_RING))) - return 0; - - ret = IORING_CQE_F_BUFFER | (req->buf_index << IORING_CQE_BUFFER_SHIFT); - __io_put_kbuf_list(req, &req->ctx->io_buffers_comp); - return ret; + if (req->flags & (REQ_F_BUFFER_SELECTED|REQ_F_BUFFER_RING)) + __io_put_kbuf_list(req, &req->ctx->io_buffers_comp); } static inline unsigned int io_put_kbuf(struct io_kiocb *req,
The two callers don't handle the return value of io_put_kbuf_comp(), so change its return type into void. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- io_uring/kbuf.h | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)