* [PATCH 0/3] io_uring: submission path cleanup
@ 2019-12-16 23:22 Pavel Begunkov
2019-12-16 23:22 ` [PATCH 1/3] io_uring: rename prev to head Pavel Begunkov
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Pavel Begunkov @ 2019-12-16 23:22 UTC (permalink / raw)
To: Jens Axboe, io-uring, linux-kernel
Pretty straighforward cleanups. The last patch saves the exact behaviour,
but do link enqueuing from a more suitable place.
Pavel Begunkov (3):
io_uring: rename prev to head
io_uring: move trace_submit_sqe into submit_sqe
io_uring: move *queue_link_head() from common path
fs/io_uring.c | 47 +++++++++++++++++++++--------------------------
1 file changed, 21 insertions(+), 26 deletions(-)
--
2.24.0
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 1/3] io_uring: rename prev to head 2019-12-16 23:22 [PATCH 0/3] io_uring: submission path cleanup Pavel Begunkov @ 2019-12-16 23:22 ` Pavel Begunkov 2019-12-16 23:22 ` [PATCH 2/3] io_uring: move trace_submit_sqe into submit_sqe Pavel Begunkov ` (3 subsequent siblings) 4 siblings, 0 replies; 21+ messages in thread From: Pavel Begunkov @ 2019-12-16 23:22 UTC (permalink / raw) To: Jens Axboe, io-uring, linux-kernel Calling "prev" a head of a link is a bit misleading. Rename it Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/io_uring.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 339b57aac5ca..96ddfc52cb0f 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -3399,10 +3399,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, * conditions are true (normal request), then just queue it. */ if (*link) { - struct io_kiocb *prev = *link; + struct io_kiocb *head = *link; if (req->sqe->flags & IOSQE_IO_DRAIN) - (*link)->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; + head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; if (req->sqe->flags & IOSQE_IO_HARDLINK) req->flags |= REQ_F_HARDLINK; @@ -3415,11 +3415,11 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, ret = io_req_defer_prep(req); if (ret) { /* fail even hard links since we don't submit */ - prev->flags |= REQ_F_FAIL_LINK; + head->flags |= REQ_F_FAIL_LINK; goto err_req; } - trace_io_uring_link(ctx, req, prev); - list_add_tail(&req->link_list, &prev->link_list); + trace_io_uring_link(ctx, req, head); + list_add_tail(&req->link_list, &head->link_list); } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { req->flags |= REQ_F_LINK; if (req->sqe->flags & IOSQE_IO_HARDLINK) -- 2.24.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/3] io_uring: move trace_submit_sqe into submit_sqe 2019-12-16 23:22 [PATCH 0/3] io_uring: submission path cleanup Pavel Begunkov 2019-12-16 23:22 ` [PATCH 1/3] io_uring: rename prev to head Pavel Begunkov @ 2019-12-16 23:22 ` Pavel Begunkov 2019-12-16 23:22 ` [PATCH 3/3] io_uring: move *queue_link_head() from common path Pavel Begunkov ` (2 subsequent siblings) 4 siblings, 0 replies; 21+ messages in thread From: Pavel Begunkov @ 2019-12-16 23:22 UTC (permalink / raw) To: Jens Axboe, io-uring, linux-kernel For better locality, call trace_io_uring_submit_sqe() from submit_sqe() rather than io_submit_sqes(). No functional change. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/io_uring.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 96ddfc52cb0f..bac9e711e38d 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -3375,7 +3375,8 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, struct io_ring_ctx *ctx = req->ctx; int ret; - req->user_data = req->sqe->user_data; + req->user_data = READ_ONCE(req->sqe->user_data); + trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async); /* enforce forwards compatibility on users */ if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) { @@ -3569,8 +3570,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, req->has_user = *mm != NULL; req->in_async = async; req->needs_fixed_file = async; - trace_io_uring_submit_sqe(ctx, req->sqe->user_data, - true, async); if (!io_submit_sqe(req, statep, &link)) break; /* -- 2.24.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/3] io_uring: move *queue_link_head() from common path 2019-12-16 23:22 [PATCH 0/3] io_uring: submission path cleanup Pavel Begunkov 2019-12-16 23:22 ` [PATCH 1/3] io_uring: rename prev to head Pavel Begunkov 2019-12-16 23:22 ` [PATCH 2/3] io_uring: move trace_submit_sqe into submit_sqe Pavel Begunkov @ 2019-12-16 23:22 ` Pavel Begunkov 2019-12-16 23:38 ` Pavel Begunkov 2019-12-17 14:00 ` Dmitry Dolgov 2019-12-17 18:15 ` [PATCH 0/3] io_uring: submission path cleanup Jens Axboe 2019-12-17 19:26 ` [PATCH v2 " Pavel Begunkov 4 siblings, 2 replies; 21+ messages in thread From: Pavel Begunkov @ 2019-12-16 23:22 UTC (permalink / raw) To: Jens Axboe, io-uring, linux-kernel Move io_queue_link_head() to links handling code in io_submit_sqe(), so it wouldn't need extra checks and would have better data locality. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/io_uring.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index bac9e711e38d..a880ed1409cb 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, struct io_kiocb **link) { struct io_ring_ctx *ctx = req->ctx; + unsigned int sqe_flags; int ret; + sqe_flags = READ_ONCE(req->sqe->flags); req->user_data = READ_ONCE(req->sqe->user_data); trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async); /* enforce forwards compatibility on users */ - if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) { + if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) { ret = -EINVAL; goto err_req; } @@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, if (*link) { struct io_kiocb *head = *link; - if (req->sqe->flags & IOSQE_IO_DRAIN) + if (sqe_flags & IOSQE_IO_DRAIN) head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; - if (req->sqe->flags & IOSQE_IO_HARDLINK) + if (sqe_flags & IOSQE_IO_HARDLINK) req->flags |= REQ_F_HARDLINK; if (io_alloc_async_ctx(req)) { @@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, } trace_io_uring_link(ctx, req, head); list_add_tail(&req->link_list, &head->link_list); - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { + + /* last request of a link, enqueue the link */ + if (!(sqe_flags & IOSQE_IO_LINK)) { + io_queue_link_head(head); + *link = NULL; + } + } else if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { req->flags |= REQ_F_LINK; - if (req->sqe->flags & IOSQE_IO_HARDLINK) + if (sqe_flags & IOSQE_IO_HARDLINK) req->flags |= REQ_F_HARDLINK; INIT_LIST_HEAD(&req->link_list); @@ -3540,10 +3548,8 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, } for (i = 0; i < nr; i++) { - struct io_kiocb *req; - unsigned int sqe_flags; + struct io_kiocb *req = io_get_req(ctx, statep); - req = io_get_req(ctx, statep); if (unlikely(!req)) { if (!submitted) submitted = -EAGAIN; @@ -3563,8 +3569,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, } submitted++; - sqe_flags = req->sqe->flags; - req->ring_file = ring_file; req->ring_fd = ring_fd; req->has_user = *mm != NULL; @@ -3572,14 +3576,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, req->needs_fixed_file = async; if (!io_submit_sqe(req, statep, &link)) break; - /* - * If previous wasn't linked and we have a linked command, - * that's the end of the chain. Submit the previous link. - */ - if (!(sqe_flags & IOSQE_IO_LINK) && link) { - io_queue_link_head(link); - link = NULL; - } } if (link) -- 2.24.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path 2019-12-16 23:22 ` [PATCH 3/3] io_uring: move *queue_link_head() from common path Pavel Begunkov @ 2019-12-16 23:38 ` Pavel Begunkov 2019-12-17 16:45 ` Jens Axboe 2019-12-17 14:00 ` Dmitry Dolgov 1 sibling, 1 reply; 21+ messages in thread From: Pavel Begunkov @ 2019-12-16 23:38 UTC (permalink / raw) To: Jens Axboe, io-uring, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 3749 bytes --] On 17/12/2019 02:22, Pavel Begunkov wrote: > Move io_queue_link_head() to links handling code in io_submit_sqe(), > so it wouldn't need extra checks and would have better data locality. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > fs/io_uring.c | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index bac9e711e38d..a880ed1409cb 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, > struct io_kiocb **link) > { > struct io_ring_ctx *ctx = req->ctx; > + unsigned int sqe_flags; > int ret; > > + sqe_flags = READ_ONCE(req->sqe->flags); > req->user_data = READ_ONCE(req->sqe->user_data); > trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async); > > /* enforce forwards compatibility on users */ > - if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) { > + if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) { > ret = -EINVAL; > goto err_req; > } > @@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, > if (*link) { > struct io_kiocb *head = *link; > > - if (req->sqe->flags & IOSQE_IO_DRAIN) > + if (sqe_flags & IOSQE_IO_DRAIN) > head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; > > - if (req->sqe->flags & IOSQE_IO_HARDLINK) > + if (sqe_flags & IOSQE_IO_HARDLINK) > req->flags |= REQ_F_HARDLINK; > > if (io_alloc_async_ctx(req)) { > @@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, > } > trace_io_uring_link(ctx, req, head); > list_add_tail(&req->link_list, &head->link_list); > - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { > + > + /* last request of a link, enqueue the link */ > + if (!(sqe_flags & IOSQE_IO_LINK)) { This looks suspicious (as well as in the current revision). Returning back to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not IOSQE_IO_LINK? I don't find any check. In other words, should it be as follows? !(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) > + io_queue_link_head(head); > + *link = NULL; > + } > + } else if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { > req->flags |= REQ_F_LINK; > - if (req->sqe->flags & IOSQE_IO_HARDLINK) > + if (sqe_flags & IOSQE_IO_HARDLINK) > req->flags |= REQ_F_HARDLINK; > > INIT_LIST_HEAD(&req->link_list); > @@ -3540,10 +3548,8 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, > } > > for (i = 0; i < nr; i++) { > - struct io_kiocb *req; > - unsigned int sqe_flags; > + struct io_kiocb *req = io_get_req(ctx, statep); > > - req = io_get_req(ctx, statep); > if (unlikely(!req)) { > if (!submitted) > submitted = -EAGAIN; > @@ -3563,8 +3569,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, > } > > submitted++; > - sqe_flags = req->sqe->flags; > - > req->ring_file = ring_file; > req->ring_fd = ring_fd; > req->has_user = *mm != NULL; > @@ -3572,14 +3576,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, > req->needs_fixed_file = async; > if (!io_submit_sqe(req, statep, &link)) > break; > - /* > - * If previous wasn't linked and we have a linked command, > - * that's the end of the chain. Submit the previous link. > - */ > - if (!(sqe_flags & IOSQE_IO_LINK) && link) { > - io_queue_link_head(link); > - link = NULL; > - } > } > > if (link) > -- Pavel Begunkov [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path 2019-12-16 23:38 ` Pavel Begunkov @ 2019-12-17 16:45 ` Jens Axboe 2019-12-17 17:37 ` Jens Axboe 0 siblings, 1 reply; 21+ messages in thread From: Jens Axboe @ 2019-12-17 16:45 UTC (permalink / raw) To: Pavel Begunkov, io-uring, linux-kernel On 12/16/19 4:38 PM, Pavel Begunkov wrote: > On 17/12/2019 02:22, Pavel Begunkov wrote: >> Move io_queue_link_head() to links handling code in io_submit_sqe(), >> so it wouldn't need extra checks and would have better data locality. >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> fs/io_uring.c | 32 ++++++++++++++------------------ >> 1 file changed, 14 insertions(+), 18 deletions(-) >> >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index bac9e711e38d..a880ed1409cb 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >> struct io_kiocb **link) >> { >> struct io_ring_ctx *ctx = req->ctx; >> + unsigned int sqe_flags; >> int ret; >> >> + sqe_flags = READ_ONCE(req->sqe->flags); >> req->user_data = READ_ONCE(req->sqe->user_data); >> trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async); >> >> /* enforce forwards compatibility on users */ >> - if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) { >> + if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) { >> ret = -EINVAL; >> goto err_req; >> } >> @@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >> if (*link) { >> struct io_kiocb *head = *link; >> >> - if (req->sqe->flags & IOSQE_IO_DRAIN) >> + if (sqe_flags & IOSQE_IO_DRAIN) >> head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; >> >> - if (req->sqe->flags & IOSQE_IO_HARDLINK) >> + if (sqe_flags & IOSQE_IO_HARDLINK) >> req->flags |= REQ_F_HARDLINK; >> >> if (io_alloc_async_ctx(req)) { >> @@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >> } >> trace_io_uring_link(ctx, req, head); >> list_add_tail(&req->link_list, &head->link_list); >> - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { >> + >> + /* last request of a link, enqueue the link */ >> + if (!(sqe_flags & IOSQE_IO_LINK)) { > > This looks suspicious (as well as in the current revision). Returning back > to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not > IOSQE_IO_LINK? I don't find any check. > > In other words, should it be as follows? > !(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) Yeah, I think that should check for both. I'm fine with either approach in general: - IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set or - IOSQE_IO_HARDLINK implies IOSQE_IO_LINK Seems like the former is easier to verify in terms of functionality, since we can rest easy if we check this early and -EINVAL if that isn't the case. What do you think? -- Jens Axboe ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path 2019-12-17 16:45 ` Jens Axboe @ 2019-12-17 17:37 ` Jens Axboe 2019-12-17 17:52 ` Pavel Begunkov 0 siblings, 1 reply; 21+ messages in thread From: Jens Axboe @ 2019-12-17 17:37 UTC (permalink / raw) To: Pavel Begunkov, io-uring, linux-kernel On 12/17/19 9:45 AM, Jens Axboe wrote: > On 12/16/19 4:38 PM, Pavel Begunkov wrote: >> On 17/12/2019 02:22, Pavel Begunkov wrote: >>> Move io_queue_link_head() to links handling code in io_submit_sqe(), >>> so it wouldn't need extra checks and would have better data locality. >>> >>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>> --- >>> fs/io_uring.c | 32 ++++++++++++++------------------ >>> 1 file changed, 14 insertions(+), 18 deletions(-) >>> >>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>> index bac9e711e38d..a880ed1409cb 100644 >>> --- a/fs/io_uring.c >>> +++ b/fs/io_uring.c >>> @@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>> struct io_kiocb **link) >>> { >>> struct io_ring_ctx *ctx = req->ctx; >>> + unsigned int sqe_flags; >>> int ret; >>> >>> + sqe_flags = READ_ONCE(req->sqe->flags); >>> req->user_data = READ_ONCE(req->sqe->user_data); >>> trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async); >>> >>> /* enforce forwards compatibility on users */ >>> - if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) { >>> + if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) { >>> ret = -EINVAL; >>> goto err_req; >>> } >>> @@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>> if (*link) { >>> struct io_kiocb *head = *link; >>> >>> - if (req->sqe->flags & IOSQE_IO_DRAIN) >>> + if (sqe_flags & IOSQE_IO_DRAIN) >>> head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; >>> >>> - if (req->sqe->flags & IOSQE_IO_HARDLINK) >>> + if (sqe_flags & IOSQE_IO_HARDLINK) >>> req->flags |= REQ_F_HARDLINK; >>> >>> if (io_alloc_async_ctx(req)) { >>> @@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>> } >>> trace_io_uring_link(ctx, req, head); >>> list_add_tail(&req->link_list, &head->link_list); >>> - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { >>> + >>> + /* last request of a link, enqueue the link */ >>> + if (!(sqe_flags & IOSQE_IO_LINK)) { >> >> This looks suspicious (as well as in the current revision). Returning back >> to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not >> IOSQE_IO_LINK? I don't find any check. >> >> In other words, should it be as follows? >> !(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) > > Yeah, I think that should check for both. I'm fine with either approach > in general: > > - IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set > > or > > - IOSQE_IO_HARDLINK implies IOSQE_IO_LINK > > Seems like the former is easier to verify in terms of functionality, > since we can rest easy if we check this early and -EINVAL if that isn't > the case. > > What do you think? If you agree, want to send in a patch for that for 5.5? Then I can respin for-5.6/io_uring on top of that, and we can apply your cleanups there. -- Jens Axboe ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path 2019-12-17 17:37 ` Jens Axboe @ 2019-12-17 17:52 ` Pavel Begunkov 2019-12-17 18:01 ` Jens Axboe 0 siblings, 1 reply; 21+ messages in thread From: Pavel Begunkov @ 2019-12-17 17:52 UTC (permalink / raw) To: Jens Axboe, io-uring, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 3409 bytes --] On 17/12/2019 20:37, Jens Axboe wrote: > On 12/17/19 9:45 AM, Jens Axboe wrote: >> On 12/16/19 4:38 PM, Pavel Begunkov wrote: >>> On 17/12/2019 02:22, Pavel Begunkov wrote: >>>> Move io_queue_link_head() to links handling code in io_submit_sqe(), >>>> so it wouldn't need extra checks and would have better data locality. >>>> >>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>> --- >>>> fs/io_uring.c | 32 ++++++++++++++------------------ >>>> 1 file changed, 14 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>>> index bac9e711e38d..a880ed1409cb 100644 >>>> --- a/fs/io_uring.c >>>> +++ b/fs/io_uring.c >>>> @@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>>> struct io_kiocb **link) >>>> { >>>> struct io_ring_ctx *ctx = req->ctx; >>>> + unsigned int sqe_flags; >>>> int ret; >>>> >>>> + sqe_flags = READ_ONCE(req->sqe->flags); >>>> req->user_data = READ_ONCE(req->sqe->user_data); >>>> trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async); >>>> >>>> /* enforce forwards compatibility on users */ >>>> - if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) { >>>> + if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) { >>>> ret = -EINVAL; >>>> goto err_req; >>>> } >>>> @@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>>> if (*link) { >>>> struct io_kiocb *head = *link; >>>> >>>> - if (req->sqe->flags & IOSQE_IO_DRAIN) >>>> + if (sqe_flags & IOSQE_IO_DRAIN) >>>> head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; >>>> >>>> - if (req->sqe->flags & IOSQE_IO_HARDLINK) >>>> + if (sqe_flags & IOSQE_IO_HARDLINK) >>>> req->flags |= REQ_F_HARDLINK; >>>> >>>> if (io_alloc_async_ctx(req)) { >>>> @@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>>> } >>>> trace_io_uring_link(ctx, req, head); >>>> list_add_tail(&req->link_list, &head->link_list); >>>> - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { >>>> + >>>> + /* last request of a link, enqueue the link */ >>>> + if (!(sqe_flags & IOSQE_IO_LINK)) { >>> >>> This looks suspicious (as well as in the current revision). Returning back >>> to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not >>> IOSQE_IO_LINK? I don't find any check. >>> >>> In other words, should it be as follows? >>> !(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) >> >> Yeah, I think that should check for both. I'm fine with either approach >> in general: >> >> - IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set >> >> or >> >> - IOSQE_IO_HARDLINK implies IOSQE_IO_LINK >> >> Seems like the former is easier to verify in terms of functionality, >> since we can rest easy if we check this early and -EINVAL if that isn't >> the case. >> >> What do you think? > > If you agree, want to send in a patch for that for 5.5? Then I can respin > for-5.6/io_uring on top of that, and we can apply your cleanups there. > Yes, that's the idea. Already got a patch, if you haven't done it yet. Just was thinking, whether to add a check for not setting both flags at the same moment in the "imply" case. Would give us 1 state in 2 bits for future use. -- Pavel Begunkov [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path 2019-12-17 17:52 ` Pavel Begunkov @ 2019-12-17 18:01 ` Jens Axboe 2019-12-17 18:05 ` Pavel Begunkov 0 siblings, 1 reply; 21+ messages in thread From: Jens Axboe @ 2019-12-17 18:01 UTC (permalink / raw) To: Pavel Begunkov, io-uring, linux-kernel On 12/17/19 10:52 AM, Pavel Begunkov wrote: > On 17/12/2019 20:37, Jens Axboe wrote: >> On 12/17/19 9:45 AM, Jens Axboe wrote: >>> On 12/16/19 4:38 PM, Pavel Begunkov wrote: >>>> On 17/12/2019 02:22, Pavel Begunkov wrote: >>>>> Move io_queue_link_head() to links handling code in io_submit_sqe(), >>>>> so it wouldn't need extra checks and would have better data locality. >>>>> >>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>>> --- >>>>> fs/io_uring.c | 32 ++++++++++++++------------------ >>>>> 1 file changed, 14 insertions(+), 18 deletions(-) >>>>> >>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>>>> index bac9e711e38d..a880ed1409cb 100644 >>>>> --- a/fs/io_uring.c >>>>> +++ b/fs/io_uring.c >>>>> @@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>>>> struct io_kiocb **link) >>>>> { >>>>> struct io_ring_ctx *ctx = req->ctx; >>>>> + unsigned int sqe_flags; >>>>> int ret; >>>>> >>>>> + sqe_flags = READ_ONCE(req->sqe->flags); >>>>> req->user_data = READ_ONCE(req->sqe->user_data); >>>>> trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async); >>>>> >>>>> /* enforce forwards compatibility on users */ >>>>> - if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) { >>>>> + if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) { >>>>> ret = -EINVAL; >>>>> goto err_req; >>>>> } >>>>> @@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>>>> if (*link) { >>>>> struct io_kiocb *head = *link; >>>>> >>>>> - if (req->sqe->flags & IOSQE_IO_DRAIN) >>>>> + if (sqe_flags & IOSQE_IO_DRAIN) >>>>> head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; >>>>> >>>>> - if (req->sqe->flags & IOSQE_IO_HARDLINK) >>>>> + if (sqe_flags & IOSQE_IO_HARDLINK) >>>>> req->flags |= REQ_F_HARDLINK; >>>>> >>>>> if (io_alloc_async_ctx(req)) { >>>>> @@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>>>> } >>>>> trace_io_uring_link(ctx, req, head); >>>>> list_add_tail(&req->link_list, &head->link_list); >>>>> - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { >>>>> + >>>>> + /* last request of a link, enqueue the link */ >>>>> + if (!(sqe_flags & IOSQE_IO_LINK)) { >>>> >>>> This looks suspicious (as well as in the current revision). Returning back >>>> to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not >>>> IOSQE_IO_LINK? I don't find any check. >>>> >>>> In other words, should it be as follows? >>>> !(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) >>> >>> Yeah, I think that should check for both. I'm fine with either approach >>> in general: >>> >>> - IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set >>> >>> or >>> >>> - IOSQE_IO_HARDLINK implies IOSQE_IO_LINK >>> >>> Seems like the former is easier to verify in terms of functionality, >>> since we can rest easy if we check this early and -EINVAL if that isn't >>> the case. >>> >>> What do you think? >> >> If you agree, want to send in a patch for that for 5.5? Then I can respin >> for-5.6/io_uring on top of that, and we can apply your cleanups there. >> > Yes, that's the idea. Already got a patch, if you haven't done it yet. I haven't. > Just was thinking, whether to add a check for not setting both flags > at the same moment in the "imply" case. Would give us 1 state in 2 bits > for future use. Not sure I follow what you're saying here, can you elaborate? -- Jens Axboe ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path 2019-12-17 18:01 ` Jens Axboe @ 2019-12-17 18:05 ` Pavel Begunkov 2019-12-17 18:07 ` Jens Axboe 0 siblings, 1 reply; 21+ messages in thread From: Pavel Begunkov @ 2019-12-17 18:05 UTC (permalink / raw) To: Jens Axboe, io-uring, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 4147 bytes --] On 17/12/2019 21:01, Jens Axboe wrote: > On 12/17/19 10:52 AM, Pavel Begunkov wrote: >> On 17/12/2019 20:37, Jens Axboe wrote: >>> On 12/17/19 9:45 AM, Jens Axboe wrote: >>>> On 12/16/19 4:38 PM, Pavel Begunkov wrote: >>>>> On 17/12/2019 02:22, Pavel Begunkov wrote: >>>>>> Move io_queue_link_head() to links handling code in io_submit_sqe(), >>>>>> so it wouldn't need extra checks and would have better data locality. >>>>>> >>>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>>>> --- >>>>>> fs/io_uring.c | 32 ++++++++++++++------------------ >>>>>> 1 file changed, 14 insertions(+), 18 deletions(-) >>>>>> >>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>>>>> index bac9e711e38d..a880ed1409cb 100644 >>>>>> --- a/fs/io_uring.c >>>>>> +++ b/fs/io_uring.c >>>>>> @@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>>>>> struct io_kiocb **link) >>>>>> { >>>>>> struct io_ring_ctx *ctx = req->ctx; >>>>>> + unsigned int sqe_flags; >>>>>> int ret; >>>>>> >>>>>> + sqe_flags = READ_ONCE(req->sqe->flags); >>>>>> req->user_data = READ_ONCE(req->sqe->user_data); >>>>>> trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async); >>>>>> >>>>>> /* enforce forwards compatibility on users */ >>>>>> - if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) { >>>>>> + if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) { >>>>>> ret = -EINVAL; >>>>>> goto err_req; >>>>>> } >>>>>> @@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>>>>> if (*link) { >>>>>> struct io_kiocb *head = *link; >>>>>> >>>>>> - if (req->sqe->flags & IOSQE_IO_DRAIN) >>>>>> + if (sqe_flags & IOSQE_IO_DRAIN) >>>>>> head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; >>>>>> >>>>>> - if (req->sqe->flags & IOSQE_IO_HARDLINK) >>>>>> + if (sqe_flags & IOSQE_IO_HARDLINK) >>>>>> req->flags |= REQ_F_HARDLINK; >>>>>> >>>>>> if (io_alloc_async_ctx(req)) { >>>>>> @@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>>>>> } >>>>>> trace_io_uring_link(ctx, req, head); >>>>>> list_add_tail(&req->link_list, &head->link_list); >>>>>> - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { >>>>>> + >>>>>> + /* last request of a link, enqueue the link */ >>>>>> + if (!(sqe_flags & IOSQE_IO_LINK)) { >>>>> >>>>> This looks suspicious (as well as in the current revision). Returning back >>>>> to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not >>>>> IOSQE_IO_LINK? I don't find any check. >>>>> >>>>> In other words, should it be as follows? >>>>> !(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) >>>> >>>> Yeah, I think that should check for both. I'm fine with either approach >>>> in general: >>>> >>>> - IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set >>>> >>>> or >>>> >>>> - IOSQE_IO_HARDLINK implies IOSQE_IO_LINK >>>> >>>> Seems like the former is easier to verify in terms of functionality, >>>> since we can rest easy if we check this early and -EINVAL if that isn't >>>> the case. >>>> >>>> What do you think? >>> >>> If you agree, want to send in a patch for that for 5.5? Then I can respin >>> for-5.6/io_uring on top of that, and we can apply your cleanups there. >>> >> Yes, that's the idea. Already got a patch, if you haven't done it yet. > > I haven't. > >> Just was thinking, whether to add a check for not setting both flags >> at the same moment in the "imply" case. Would give us 1 state in 2 bits >> for future use. > > Not sure I follow what you're saying here, can you elaborate? > Sure #define IOSQE_IO_LINK (1U << 2) /* links next sqe */ #define IOSQE_IO_HARDLINK (1U << 3) /* like LINK, but stronger */ That's 2 consequent bits, so 4 states: 0,0 -> not a link 1,0 -> common link 0,1 -> hard link 1,1 -> reserved, space for another link-quirk type But that would require additional check, i.e. if (flags&(LINK|HARDLINK) == (LINK|HARDLINK)) ... -- Pavel Begunkov [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path 2019-12-17 18:05 ` Pavel Begunkov @ 2019-12-17 18:07 ` Jens Axboe 2019-12-17 18:12 ` Pavel Begunkov 0 siblings, 1 reply; 21+ messages in thread From: Jens Axboe @ 2019-12-17 18:07 UTC (permalink / raw) To: Pavel Begunkov, io-uring, linux-kernel On 12/17/19 11:05 AM, Pavel Begunkov wrote: > On 17/12/2019 21:01, Jens Axboe wrote: >> On 12/17/19 10:52 AM, Pavel Begunkov wrote: >>> On 17/12/2019 20:37, Jens Axboe wrote: >>>> On 12/17/19 9:45 AM, Jens Axboe wrote: >>>>> On 12/16/19 4:38 PM, Pavel Begunkov wrote: >>>>>> On 17/12/2019 02:22, Pavel Begunkov wrote: >>>>>>> Move io_queue_link_head() to links handling code in io_submit_sqe(), >>>>>>> so it wouldn't need extra checks and would have better data locality. >>>>>>> >>>>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>>>>> --- >>>>>>> fs/io_uring.c | 32 ++++++++++++++------------------ >>>>>>> 1 file changed, 14 insertions(+), 18 deletions(-) >>>>>>> >>>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>>>>>> index bac9e711e38d..a880ed1409cb 100644 >>>>>>> --- a/fs/io_uring.c >>>>>>> +++ b/fs/io_uring.c >>>>>>> @@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>>>>>> struct io_kiocb **link) >>>>>>> { >>>>>>> struct io_ring_ctx *ctx = req->ctx; >>>>>>> + unsigned int sqe_flags; >>>>>>> int ret; >>>>>>> >>>>>>> + sqe_flags = READ_ONCE(req->sqe->flags); >>>>>>> req->user_data = READ_ONCE(req->sqe->user_data); >>>>>>> trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async); >>>>>>> >>>>>>> /* enforce forwards compatibility on users */ >>>>>>> - if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) { >>>>>>> + if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) { >>>>>>> ret = -EINVAL; >>>>>>> goto err_req; >>>>>>> } >>>>>>> @@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>>>>>> if (*link) { >>>>>>> struct io_kiocb *head = *link; >>>>>>> >>>>>>> - if (req->sqe->flags & IOSQE_IO_DRAIN) >>>>>>> + if (sqe_flags & IOSQE_IO_DRAIN) >>>>>>> head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; >>>>>>> >>>>>>> - if (req->sqe->flags & IOSQE_IO_HARDLINK) >>>>>>> + if (sqe_flags & IOSQE_IO_HARDLINK) >>>>>>> req->flags |= REQ_F_HARDLINK; >>>>>>> >>>>>>> if (io_alloc_async_ctx(req)) { >>>>>>> @@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >>>>>>> } >>>>>>> trace_io_uring_link(ctx, req, head); >>>>>>> list_add_tail(&req->link_list, &head->link_list); >>>>>>> - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { >>>>>>> + >>>>>>> + /* last request of a link, enqueue the link */ >>>>>>> + if (!(sqe_flags & IOSQE_IO_LINK)) { >>>>>> >>>>>> This looks suspicious (as well as in the current revision). Returning back >>>>>> to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not >>>>>> IOSQE_IO_LINK? I don't find any check. >>>>>> >>>>>> In other words, should it be as follows? >>>>>> !(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) >>>>> >>>>> Yeah, I think that should check for both. I'm fine with either approach >>>>> in general: >>>>> >>>>> - IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set >>>>> >>>>> or >>>>> >>>>> - IOSQE_IO_HARDLINK implies IOSQE_IO_LINK >>>>> >>>>> Seems like the former is easier to verify in terms of functionality, >>>>> since we can rest easy if we check this early and -EINVAL if that isn't >>>>> the case. >>>>> >>>>> What do you think? >>>> >>>> If you agree, want to send in a patch for that for 5.5? Then I can respin >>>> for-5.6/io_uring on top of that, and we can apply your cleanups there. >>>> >>> Yes, that's the idea. Already got a patch, if you haven't done it yet. >> >> I haven't. >> >>> Just was thinking, whether to add a check for not setting both flags >>> at the same moment in the "imply" case. Would give us 1 state in 2 bits >>> for future use. >> >> Not sure I follow what you're saying here, can you elaborate? >> > > Sure > > #define IOSQE_IO_LINK (1U << 2) /* links next sqe */ > #define IOSQE_IO_HARDLINK (1U << 3) /* like LINK, but stronger */ > > That's 2 consequent bits, so 4 states: > 0,0 -> not a link > 1,0 -> common link > 0,1 -> hard link > 1,1 -> reserved, space for another link-quirk type > > But that would require additional check, i.e. > > if (flags&(LINK|HARDLINK) == (LINK|HARDLINK)) ... Ah, I see. In terms of usability, I think it makes more sense to have IOSQE_LINK | IOSQE_HARDLINK be the same as just IOSQE_LINK. It would be nice to retain that for something else, but I think it'll be more confusing to users. -- Jens Axboe ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path 2019-12-17 18:07 ` Jens Axboe @ 2019-12-17 18:12 ` Pavel Begunkov 2019-12-17 18:15 ` Jens Axboe 0 siblings, 1 reply; 21+ messages in thread From: Pavel Begunkov @ 2019-12-17 18:12 UTC (permalink / raw) To: Jens Axboe, io-uring, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 2828 bytes --] On 17/12/2019 21:07, Jens Axboe wrote: > On 12/17/19 11:05 AM, Pavel Begunkov wrote: >> On 17/12/2019 21:01, Jens Axboe wrote: >>> On 12/17/19 10:52 AM, Pavel Begunkov wrote: >>>> On 17/12/2019 20:37, Jens Axboe wrote: >>>>> On 12/17/19 9:45 AM, Jens Axboe wrote: >>>>>> On 12/16/19 4:38 PM, Pavel Begunkov wrote: >>>>>>> On 17/12/2019 02:22, Pavel Begunkov wrote: >>>>>>>> - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { >>>>>>>> + >>>>>>>> + /* last request of a link, enqueue the link */ >>>>>>>> + if (!(sqe_flags & IOSQE_IO_LINK)) { >>>>>>> >>>>>>> This looks suspicious (as well as in the current revision). Returning back >>>>>>> to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not >>>>>>> IOSQE_IO_LINK? I don't find any check. >>>>>>> >>>>>>> In other words, should it be as follows? >>>>>>> !(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) >>>>>> >>>>>> Yeah, I think that should check for both. I'm fine with either approach >>>>>> in general: >>>>>> >>>>>> - IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set >>>>>> >>>>>> or >>>>>> >>>>>> - IOSQE_IO_HARDLINK implies IOSQE_IO_LINK >>>>>> >>>>>> Seems like the former is easier to verify in terms of functionality, >>>>>> since we can rest easy if we check this early and -EINVAL if that isn't >>>>>> the case. >>>>>> >>>>>> What do you think? >>>>> >>>>> If you agree, want to send in a patch for that for 5.5? Then I can respin >>>>> for-5.6/io_uring on top of that, and we can apply your cleanups there. >>>>> >>>> Yes, that's the idea. Already got a patch, if you haven't done it yet. >>> >>> I haven't. >>> >>>> Just was thinking, whether to add a check for not setting both flags >>>> at the same moment in the "imply" case. Would give us 1 state in 2 bits >>>> for future use. >>> >>> Not sure I follow what you're saying here, can you elaborate? >>> >> >> Sure >> >> #define IOSQE_IO_LINK (1U << 2) /* links next sqe */ >> #define IOSQE_IO_HARDLINK (1U << 3) /* like LINK, but stronger */ >> >> That's 2 consequent bits, so 4 states: >> 0,0 -> not a link >> 1,0 -> common link >> 0,1 -> hard link >> 1,1 -> reserved, space for another link-quirk type >> >> But that would require additional check, i.e. >> >> if (flags&(LINK|HARDLINK) == (LINK|HARDLINK)) ... > > Ah, I see. In terms of usability, I think it makes more sense to have > > IOSQE_LINK | IOSQE_HARDLINK > > be the same as just IOSQE_LINK. It would be nice to retain that for Probably, you meant it to be the same as __IOSQE_HARDLINK__ > something else, but I think it'll be more confusing to users. > Yeah, and it's easier for something like: sqe->flags |= IOSQE_LINK; [some code] if (timer_or_whatever()) sqe->flags |= IOSQE_HARDLINK; -- Pavel Begunkov [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path 2019-12-17 18:12 ` Pavel Begunkov @ 2019-12-17 18:15 ` Jens Axboe 0 siblings, 0 replies; 21+ messages in thread From: Jens Axboe @ 2019-12-17 18:15 UTC (permalink / raw) To: Pavel Begunkov, io-uring, linux-kernel On 12/17/19 11:12 AM, Pavel Begunkov wrote: > On 17/12/2019 21:07, Jens Axboe wrote: >> On 12/17/19 11:05 AM, Pavel Begunkov wrote: >>> On 17/12/2019 21:01, Jens Axboe wrote: >>>> On 12/17/19 10:52 AM, Pavel Begunkov wrote: >>>>> On 17/12/2019 20:37, Jens Axboe wrote: >>>>>> On 12/17/19 9:45 AM, Jens Axboe wrote: >>>>>>> On 12/16/19 4:38 PM, Pavel Begunkov wrote: >>>>>>>> On 17/12/2019 02:22, Pavel Begunkov wrote: >>>>>>>>> - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { >>>>>>>>> + >>>>>>>>> + /* last request of a link, enqueue the link */ >>>>>>>>> + if (!(sqe_flags & IOSQE_IO_LINK)) { >>>>>>>> >>>>>>>> This looks suspicious (as well as in the current revision). Returning back >>>>>>>> to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not >>>>>>>> IOSQE_IO_LINK? I don't find any check. >>>>>>>> >>>>>>>> In other words, should it be as follows? >>>>>>>> !(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) >>>>>>> >>>>>>> Yeah, I think that should check for both. I'm fine with either approach >>>>>>> in general: >>>>>>> >>>>>>> - IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set >>>>>>> >>>>>>> or >>>>>>> >>>>>>> - IOSQE_IO_HARDLINK implies IOSQE_IO_LINK >>>>>>> >>>>>>> Seems like the former is easier to verify in terms of functionality, >>>>>>> since we can rest easy if we check this early and -EINVAL if that isn't >>>>>>> the case. >>>>>>> >>>>>>> What do you think? >>>>>> >>>>>> If you agree, want to send in a patch for that for 5.5? Then I can respin >>>>>> for-5.6/io_uring on top of that, and we can apply your cleanups there. >>>>>> >>>>> Yes, that's the idea. Already got a patch, if you haven't done it yet. >>>> >>>> I haven't. >>>> >>>>> Just was thinking, whether to add a check for not setting both flags >>>>> at the same moment in the "imply" case. Would give us 1 state in 2 bits >>>>> for future use. >>>> >>>> Not sure I follow what you're saying here, can you elaborate? >>>> >>> >>> Sure >>> >>> #define IOSQE_IO_LINK (1U << 2) /* links next sqe */ >>> #define IOSQE_IO_HARDLINK (1U << 3) /* like LINK, but stronger */ >>> >>> That's 2 consequent bits, so 4 states: >>> 0,0 -> not a link >>> 1,0 -> common link >>> 0,1 -> hard link >>> 1,1 -> reserved, space for another link-quirk type >>> >>> But that would require additional check, i.e. >>> >>> if (flags&(LINK|HARDLINK) == (LINK|HARDLINK)) ... >> >> Ah, I see. In terms of usability, I think it makes more sense to have >> >> IOSQE_LINK | IOSQE_HARDLINK >> >> be the same as just IOSQE_LINK. It would be nice to retain that for > > Probably, you meant it to be the same as __IOSQE_HARDLINK__ > >> something else, but I think it'll be more confusing to users. >> > > Yeah, and it's easier for something like: > > sqe->flags |= IOSQE_LINK; > [some code] > if (timer_or_whatever()) > sqe->flags |= IOSQE_HARDLINK; Precisely. So let's keep it as-is. -- Jens Axboe ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path 2019-12-16 23:22 ` [PATCH 3/3] io_uring: move *queue_link_head() from common path Pavel Begunkov 2019-12-16 23:38 ` Pavel Begunkov @ 2019-12-17 14:00 ` Dmitry Dolgov 2019-12-17 14:16 ` Pavel Begunkov 1 sibling, 1 reply; 21+ messages in thread From: Dmitry Dolgov @ 2019-12-17 14:00 UTC (permalink / raw) To: Pavel Begunkov; +Cc: Jens Axboe, io-uring, linux-kernel > On Tue, Dec 17, 2019 at 02:22:09AM +0300, Pavel Begunkov wrote: > > Move io_queue_link_head() to links handling code in io_submit_sqe(), > so it wouldn't need extra checks and would have better data locality. > > --- > fs/io_uring.c | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index bac9e711e38d..a880ed1409cb 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, > struct io_kiocb **link) > { > struct io_ring_ctx *ctx = req->ctx; > + unsigned int sqe_flags; > int ret; > > + sqe_flags = READ_ONCE(req->sqe->flags); Just out of curiosity, why READ_ONCE it necessary here? I though, that since io_submit_sqes happens within a uring_lock, it's already protected. Do I miss something? > @@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, > } > trace_io_uring_link(ctx, req, head); > list_add_tail(&req->link_list, &head->link_list); > - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { > + > + /* last request of a link, enqueue the link */ > + if (!(sqe_flags & IOSQE_IO_LINK)) { Yes, as you mentioned in the previous email, it seems correct that if IOSQE_IO_HARDLINK imply IOSQE_IO_LINK, then here we need to check both. > + io_queue_link_head(head); > + *link = NULL; > + } > + } else if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path 2019-12-17 14:00 ` Dmitry Dolgov @ 2019-12-17 14:16 ` Pavel Begunkov 0 siblings, 0 replies; 21+ messages in thread From: Pavel Begunkov @ 2019-12-17 14:16 UTC (permalink / raw) To: Dmitry Dolgov; +Cc: Jens Axboe, io-uring, linux-kernel On 12/17/2019 5:00 PM, Dmitry Dolgov wrote: >> On Tue, Dec 17, 2019 at 02:22:09AM +0300, Pavel Begunkov wrote: >> >> Move io_queue_link_head() to links handling code in io_submit_sqe(), >> so it wouldn't need extra checks and would have better data locality. >> >> --- >> fs/io_uring.c | 32 ++++++++++++++------------------ >> 1 file changed, 14 insertions(+), 18 deletions(-) >> >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index bac9e711e38d..a880ed1409cb 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >> struct io_kiocb **link) >> { >> struct io_ring_ctx *ctx = req->ctx; >> + unsigned int sqe_flags; >> int ret; >> >> + sqe_flags = READ_ONCE(req->sqe->flags); > > Just out of curiosity, why READ_ONCE it necessary here? I though, that > since io_submit_sqes happens within a uring_lock, it's already > protected. Do I miss something? > SQEs are rw-shared with the userspace, that's it. Probably, there are more places where proper READ_ONCE() annotations have been lost. >> @@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, >> } >> trace_io_uring_link(ctx, req, head); >> list_add_tail(&req->link_list, &head->link_list); >> - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { >> + >> + /* last request of a link, enqueue the link */ >> + if (!(sqe_flags & IOSQE_IO_LINK)) { > > Yes, as you mentioned in the previous email, it seems correct that if > IOSQE_IO_HARDLINK imply IOSQE_IO_LINK, then here we need to check both. > >> + io_queue_link_head(head); >> + *link = NULL; >> + } >> + } else if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { -- Pavel Begunkov ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/3] io_uring: submission path cleanup 2019-12-16 23:22 [PATCH 0/3] io_uring: submission path cleanup Pavel Begunkov ` (2 preceding siblings ...) 2019-12-16 23:22 ` [PATCH 3/3] io_uring: move *queue_link_head() from common path Pavel Begunkov @ 2019-12-17 18:15 ` Jens Axboe 2019-12-17 19:26 ` [PATCH v2 " Pavel Begunkov 4 siblings, 0 replies; 21+ messages in thread From: Jens Axboe @ 2019-12-17 18:15 UTC (permalink / raw) To: Pavel Begunkov, io-uring, linux-kernel On 12/16/19 4:22 PM, Pavel Begunkov wrote: > Pretty straighforward cleanups. The last patch saves the exact behaviour, > but do link enqueuing from a more suitable place. > > Pavel Begunkov (3): > io_uring: rename prev to head > io_uring: move trace_submit_sqe into submit_sqe > io_uring: move *queue_link_head() from common path > > fs/io_uring.c | 47 +++++++++++++++++++++-------------------------- > 1 file changed, 21 insertions(+), 26 deletions(-) Can you respin this on top of the hardlink patch? -- Jens Axboe ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 0/3] io_uring: submission path cleanup 2019-12-16 23:22 [PATCH 0/3] io_uring: submission path cleanup Pavel Begunkov ` (3 preceding siblings ...) 2019-12-17 18:15 ` [PATCH 0/3] io_uring: submission path cleanup Jens Axboe @ 2019-12-17 19:26 ` Pavel Begunkov 2019-12-17 19:26 ` [PATCH 1/3] io_uring: rename prev to head Pavel Begunkov ` (3 more replies) 4 siblings, 4 replies; 21+ messages in thread From: Pavel Begunkov @ 2019-12-17 19:26 UTC (permalink / raw) To: Jens Axboe, io-uring, linux-kernel Pretty straighforward cleanups. The last patch saves the exact behaviour, but do link enqueuing from a more suitable place. v2: rebase Pavel Begunkov (3): io_uring: rename prev to head io_uring: move trace_submit_sqe into submit_sqe io_uring: move *queue_link_head() from common path fs/io_uring.c | 47 +++++++++++++++++++++-------------------------- 1 file changed, 21 insertions(+), 26 deletions(-) -- 2.24.0 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/3] io_uring: rename prev to head 2019-12-17 19:26 ` [PATCH v2 " Pavel Begunkov @ 2019-12-17 19:26 ` Pavel Begunkov 2019-12-17 19:26 ` [PATCH 2/3] io_uring: move trace_submit_sqe into submit_sqe Pavel Begunkov ` (2 subsequent siblings) 3 siblings, 0 replies; 21+ messages in thread From: Pavel Begunkov @ 2019-12-17 19:26 UTC (permalink / raw) To: Jens Axboe, io-uring, linux-kernel Calling "prev" a head of a link is a bit misleading. Rename it Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/io_uring.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index eb6d897ea087..e8ce224dc82c 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -3399,10 +3399,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, * conditions are true (normal request), then just queue it. */ if (*link) { - struct io_kiocb *prev = *link; + struct io_kiocb *head = *link; if (req->sqe->flags & IOSQE_IO_DRAIN) - (*link)->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; + head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; if (req->sqe->flags & IOSQE_IO_HARDLINK) req->flags |= REQ_F_HARDLINK; @@ -3415,11 +3415,11 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, ret = io_req_defer_prep(req); if (ret) { /* fail even hard links since we don't submit */ - prev->flags |= REQ_F_FAIL_LINK; + head->flags |= REQ_F_FAIL_LINK; goto err_req; } - trace_io_uring_link(ctx, req, prev); - list_add_tail(&req->link_list, &prev->link_list); + trace_io_uring_link(ctx, req, head); + list_add_tail(&req->link_list, &head->link_list); } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { req->flags |= REQ_F_LINK; if (req->sqe->flags & IOSQE_IO_HARDLINK) -- 2.24.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/3] io_uring: move trace_submit_sqe into submit_sqe 2019-12-17 19:26 ` [PATCH v2 " Pavel Begunkov 2019-12-17 19:26 ` [PATCH 1/3] io_uring: rename prev to head Pavel Begunkov @ 2019-12-17 19:26 ` Pavel Begunkov 2019-12-17 19:26 ` [PATCH v2 3/3] io_uring: move *queue_link_head() from common path Pavel Begunkov 2019-12-17 21:15 ` [PATCH v2 0/3] io_uring: submission path cleanup Jens Axboe 3 siblings, 0 replies; 21+ messages in thread From: Pavel Begunkov @ 2019-12-17 19:26 UTC (permalink / raw) To: Jens Axboe, io-uring, linux-kernel For better locality, call trace_io_uring_submit_sqe() from submit_sqe() rather than io_submit_sqes(). No functional change. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/io_uring.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index e8ce224dc82c..ee461bcd3121 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -3375,7 +3375,8 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, struct io_ring_ctx *ctx = req->ctx; int ret; - req->user_data = req->sqe->user_data; + req->user_data = READ_ONCE(req->sqe->user_data); + trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async); /* enforce forwards compatibility on users */ if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) { @@ -3569,8 +3570,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, req->has_user = *mm != NULL; req->in_async = async; req->needs_fixed_file = async; - trace_io_uring_submit_sqe(ctx, req->sqe->user_data, - true, async); if (!io_submit_sqe(req, statep, &link)) break; /* -- 2.24.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 3/3] io_uring: move *queue_link_head() from common path 2019-12-17 19:26 ` [PATCH v2 " Pavel Begunkov 2019-12-17 19:26 ` [PATCH 1/3] io_uring: rename prev to head Pavel Begunkov 2019-12-17 19:26 ` [PATCH 2/3] io_uring: move trace_submit_sqe into submit_sqe Pavel Begunkov @ 2019-12-17 19:26 ` Pavel Begunkov 2019-12-17 21:15 ` [PATCH v2 0/3] io_uring: submission path cleanup Jens Axboe 3 siblings, 0 replies; 21+ messages in thread From: Pavel Begunkov @ 2019-12-17 19:26 UTC (permalink / raw) To: Jens Axboe, io-uring, linux-kernel Move io_queue_link_head() to links handling code in io_submit_sqe(), so it wouldn't need extra checks and would have better data locality. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/io_uring.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index ee461bcd3121..8ab96ca0ad28 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, struct io_kiocb **link) { struct io_ring_ctx *ctx = req->ctx; + unsigned int sqe_flags; int ret; + sqe_flags = READ_ONCE(req->sqe->flags); req->user_data = READ_ONCE(req->sqe->user_data); trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async); /* enforce forwards compatibility on users */ - if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) { + if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) { ret = -EINVAL; goto err_req; } @@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, if (*link) { struct io_kiocb *head = *link; - if (req->sqe->flags & IOSQE_IO_DRAIN) + if (sqe_flags & IOSQE_IO_DRAIN) head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN; - if (req->sqe->flags & IOSQE_IO_HARDLINK) + if (sqe_flags & IOSQE_IO_HARDLINK) req->flags |= REQ_F_HARDLINK; if (io_alloc_async_ctx(req)) { @@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, } trace_io_uring_link(ctx, req, head); list_add_tail(&req->link_list, &head->link_list); - } else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { + + /* last request of a link, enqueue the link */ + if (!(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK))) { + io_queue_link_head(head); + *link = NULL; + } + } else if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) { req->flags |= REQ_F_LINK; - if (req->sqe->flags & IOSQE_IO_HARDLINK) + if (sqe_flags & IOSQE_IO_HARDLINK) req->flags |= REQ_F_HARDLINK; INIT_LIST_HEAD(&req->link_list); @@ -3540,10 +3548,8 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, } for (i = 0; i < nr; i++) { - struct io_kiocb *req; - unsigned int sqe_flags; + struct io_kiocb *req = io_get_req(ctx, statep); - req = io_get_req(ctx, statep); if (unlikely(!req)) { if (!submitted) submitted = -EAGAIN; @@ -3563,8 +3569,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, } submitted++; - sqe_flags = req->sqe->flags; - req->ring_file = ring_file; req->ring_fd = ring_fd; req->has_user = *mm != NULL; @@ -3572,14 +3576,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, req->needs_fixed_file = async; if (!io_submit_sqe(req, statep, &link)) break; - /* - * If previous wasn't linked and we have a linked command, - * that's the end of the chain. Submit the previous link. - */ - if (!(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) && link) { - io_queue_link_head(link); - link = NULL; - } } if (link) -- 2.24.0 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/3] io_uring: submission path cleanup 2019-12-17 19:26 ` [PATCH v2 " Pavel Begunkov ` (2 preceding siblings ...) 2019-12-17 19:26 ` [PATCH v2 3/3] io_uring: move *queue_link_head() from common path Pavel Begunkov @ 2019-12-17 21:15 ` Jens Axboe 3 siblings, 0 replies; 21+ messages in thread From: Jens Axboe @ 2019-12-17 21:15 UTC (permalink / raw) To: Pavel Begunkov, io-uring, linux-kernel On 12/17/19 12:26 PM, Pavel Begunkov wrote: > Pretty straighforward cleanups. The last patch saves the exact behaviour, > but do link enqueuing from a more suitable place. > > v2: rebase > > Pavel Begunkov (3): > io_uring: rename prev to head > io_uring: move trace_submit_sqe into submit_sqe > io_uring: move *queue_link_head() from common path > > fs/io_uring.c | 47 +++++++++++++++++++++-------------------------- > 1 file changed, 21 insertions(+), 26 deletions(-) Applied, thanks. -- Jens Axboe ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-12-17 21:16 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-16 23:22 [PATCH 0/3] io_uring: submission path cleanup Pavel Begunkov 2019-12-16 23:22 ` [PATCH 1/3] io_uring: rename prev to head Pavel Begunkov 2019-12-16 23:22 ` [PATCH 2/3] io_uring: move trace_submit_sqe into submit_sqe Pavel Begunkov 2019-12-16 23:22 ` [PATCH 3/3] io_uring: move *queue_link_head() from common path Pavel Begunkov 2019-12-16 23:38 ` Pavel Begunkov 2019-12-17 16:45 ` Jens Axboe 2019-12-17 17:37 ` Jens Axboe 2019-12-17 17:52 ` Pavel Begunkov 2019-12-17 18:01 ` Jens Axboe 2019-12-17 18:05 ` Pavel Begunkov 2019-12-17 18:07 ` Jens Axboe 2019-12-17 18:12 ` Pavel Begunkov 2019-12-17 18:15 ` Jens Axboe 2019-12-17 14:00 ` Dmitry Dolgov 2019-12-17 14:16 ` Pavel Begunkov 2019-12-17 18:15 ` [PATCH 0/3] io_uring: submission path cleanup Jens Axboe 2019-12-17 19:26 ` [PATCH v2 " Pavel Begunkov 2019-12-17 19:26 ` [PATCH 1/3] io_uring: rename prev to head Pavel Begunkov 2019-12-17 19:26 ` [PATCH 2/3] io_uring: move trace_submit_sqe into submit_sqe Pavel Begunkov 2019-12-17 19:26 ` [PATCH v2 3/3] io_uring: move *queue_link_head() from common path Pavel Begunkov 2019-12-17 21:15 ` [PATCH v2 0/3] io_uring: submission path cleanup Jens Axboe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.