* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).