| Message ID | 20240511001214.173711-5-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: support sqe group and provide group kbuf | expand |
On 5/11/24 01:12, Ming Lei wrote: > Prepare for supporting sqe group, which requires to post group leader's > CQE after all members' CQEs are posted. For group leader request, we can't > do that in io_req_complete_post, and REQ_F_CQE_SKIP can't be set in > io_free_req(). Can you elaborate what exactly we can't do and why? > So move marking REQ_F_CQE_SKIP out of io_free_req(). That makes io_free_req() a very confusing function, it tells that it just frees the request but in reality can post a CQE. If you really need it, just add a new function. > No functional change. > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > io_uring/io_uring.c | 5 +++-- > io_uring/timeout.c | 3 +++ > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c > index e4be930e0f1e..c184c9a312df 100644 > --- a/io_uring/io_uring.c > +++ b/io_uring/io_uring.c > @@ -1027,8 +1027,6 @@ __cold void io_free_req(struct io_kiocb *req) > { > /* refs were already put, restore them for io_req_task_complete() */ > req->flags &= ~REQ_F_REFCOUNT; > - /* we only want to free it, don't post CQEs */ > - req->flags |= REQ_F_CQE_SKIP; > req->io_task_work.func = io_req_task_complete; > io_req_task_work_add(req); > } > @@ -1797,6 +1795,9 @@ struct io_wq_work *io_wq_free_work(struct io_wq_work *work) > if (req_ref_put_and_test(req)) { > if (req->flags & IO_REQ_LINK_FLAGS) > nxt = io_req_find_next(req); > + > + /* we have posted CQEs in io_req_complete_post() */ > + req->flags |= REQ_F_CQE_SKIP; > io_free_req(req); > } > return nxt ? &nxt->work : NULL; > diff --git a/io_uring/timeout.c b/io_uring/timeout.c > index 1c9bf07499b1..202f540aa314 100644 > --- a/io_uring/timeout.c > +++ b/io_uring/timeout.c > @@ -47,6 +47,9 @@ static inline void io_put_req(struct io_kiocb *req) > { > if (req_ref_put_and_test(req)) { > io_queue_next(req); > + > + /* we only want to free it, don't post CQEs */ > + req->flags |= REQ_F_CQE_SKIP; > io_free_req(req); > } > }
On Mon, Jun 10, 2024 at 02:23:50AM +0100, Pavel Begunkov wrote: > On 5/11/24 01:12, Ming Lei wrote: > > Prepare for supporting sqe group, which requires to post group leader's > > CQE after all members' CQEs are posted. For group leader request, we can't > > do that in io_req_complete_post, and REQ_F_CQE_SKIP can't be set in > > io_free_req(). > > Can you elaborate what exactly we can't do and why? group leader's CQE is always posted after other members are posted. > > > So move marking REQ_F_CQE_SKIP out of io_free_req(). > > That makes io_free_req() a very confusing function, it tells > that it just frees the request but in reality can post a > CQE. If you really need it, just add a new function. io_free_req() never posts CQE. This patch can help to move setting REQ_F_CQE_SKIP around real post code, and it can make current code more readable. Thanks, Ming
On 6/11/24 14:28, Ming Lei wrote: > On Mon, Jun 10, 2024 at 02:23:50AM +0100, Pavel Begunkov wrote: >> On 5/11/24 01:12, Ming Lei wrote: >>> Prepare for supporting sqe group, which requires to post group leader's >>> CQE after all members' CQEs are posted. For group leader request, we can't >>> do that in io_req_complete_post, and REQ_F_CQE_SKIP can't be set in >>> io_free_req(). >> >> Can you elaborate what exactly we can't do and why? > > group leader's CQE is always posted after other members are posted. > >> >>> So move marking REQ_F_CQE_SKIP out of io_free_req(). >> >> That makes io_free_req() a very confusing function, it tells >> that it just frees the request but in reality can post a >> CQE. If you really need it, just add a new function. > > io_free_req() never posts CQE. Right, that's the intention and that's why it sets REQ_F_CQE_SKIP. Without it, even if you patch all call sites that they set it themselves, it turns into a misleading function. > This patch can help to move setting REQ_F_CQE_SKIP around > real post code, and it can make current code more readable.
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index e4be930e0f1e..c184c9a312df 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -1027,8 +1027,6 @@ __cold void io_free_req(struct io_kiocb *req) { /* refs were already put, restore them for io_req_task_complete() */ req->flags &= ~REQ_F_REFCOUNT; - /* we only want to free it, don't post CQEs */ - req->flags |= REQ_F_CQE_SKIP; req->io_task_work.func = io_req_task_complete; io_req_task_work_add(req); } @@ -1797,6 +1795,9 @@ struct io_wq_work *io_wq_free_work(struct io_wq_work *work) if (req_ref_put_and_test(req)) { if (req->flags & IO_REQ_LINK_FLAGS) nxt = io_req_find_next(req); + + /* we have posted CQEs in io_req_complete_post() */ + req->flags |= REQ_F_CQE_SKIP; io_free_req(req); } return nxt ? &nxt->work : NULL; diff --git a/io_uring/timeout.c b/io_uring/timeout.c index 1c9bf07499b1..202f540aa314 100644 --- a/io_uring/timeout.c +++ b/io_uring/timeout.c @@ -47,6 +47,9 @@ static inline void io_put_req(struct io_kiocb *req) { if (req_ref_put_and_test(req)) { io_queue_next(req); + + /* we only want to free it, don't post CQEs */ + req->flags |= REQ_F_CQE_SKIP; io_free_req(req); } }
Prepare for supporting sqe group, which requires to post group leader's CQE after all members' CQEs are posted. For group leader request, we can't do that in io_req_complete_post, and REQ_F_CQE_SKIP can't be set in io_free_req(). So move marking REQ_F_CQE_SKIP out of io_free_req(). No functional change. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- io_uring/io_uring.c | 5 +++-- io_uring/timeout.c | 3 +++ 2 files changed, 6 insertions(+), 2 deletions(-)