| Message ID | 85a316b577e1b5204d27a96a7ce452ed6be3c2ae.1572988512.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cleanup of submission path | expand |
On 11/6/19 5:22 AM, Pavel Begunkov wrote: > After a call to io_submit_sqe(), it's already known whether it needs > to queue a link or not. Do it there, as it's simplier and doesn't keep > an extra variable across the loop. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > fs/io_uring.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index ebe2a4edd644..82c2da99cb5c 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -2687,7 +2687,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, > struct io_submit_state state, *statep = NULL; > struct io_kiocb *link = NULL; > struct io_kiocb *shadow_req = NULL; > - bool prev_was_link = false; > int i, submitted = 0; > bool mm_fault = false; > > @@ -2710,17 +2709,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, > } > } > > - /* > - * If previous wasn't linked and we have a linked command, > - * that's the end of the chain. Submit the previous link. > - */ > - if (!prev_was_link && link) { > - io_queue_link_head(ctx, link, &link->submit, shadow_req); > - link = NULL; > - shadow_req = NULL; > - } > - prev_was_link = (s.sqe->flags & IOSQE_IO_LINK) != 0; > - > if (link && (s.sqe->flags & IOSQE_IO_DRAIN)) { > if (!shadow_req) { > shadow_req = io_get_req(ctx, NULL); > @@ -2741,6 +2729,16 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, > trace_io_uring_submit_sqe(ctx, s.sqe->user_data, true, async); > io_submit_sqe(ctx, &s, statep, &link); > submitted++; > + > + /* > + * If previous wasn't linked and we have a linked command, > + * that's the end of the chain. Submit the previous link. > + */ > + if (!(s.sqe->flags & IOSQE_IO_LINK) && link) The behavior changed to 'current seq' instead of previous after dropping prev_was_link? > + io_queue_link_head(ctx, link, &link->submit, shadow_req); > + link = NULL; > + shadow_req = NULL; > + } > } > > if (link) >
On 11/6/2019 11:36 AM, Bob Liu wrote: > On 11/6/19 5:22 AM, Pavel Begunkov wrote: >> After a call to io_submit_sqe(), it's already known whether it needs >> to queue a link or not. Do it there, as it's simplier and doesn't keep >> an extra variable across the loop. >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> fs/io_uring.c | 22 ++++++++++------------ >> 1 file changed, 10 insertions(+), 12 deletions(-) >> >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index ebe2a4edd644..82c2da99cb5c 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -2687,7 +2687,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, >> struct io_submit_state state, *statep = NULL; >> struct io_kiocb *link = NULL; >> struct io_kiocb *shadow_req = NULL; >> - bool prev_was_link = false; >> int i, submitted = 0; >> bool mm_fault = false; >> >> @@ -2710,17 +2709,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, >> } >> } >> >> - /* >> - * If previous wasn't linked and we have a linked command, >> - * that's the end of the chain. Submit the previous link. >> - */ >> - if (!prev_was_link && link) { >> - io_queue_link_head(ctx, link, &link->submit, shadow_req); >> - link = NULL; >> - shadow_req = NULL; >> - } >> - prev_was_link = (s.sqe->flags & IOSQE_IO_LINK) != 0; >> - >> if (link && (s.sqe->flags & IOSQE_IO_DRAIN)) { >> if (!shadow_req) { >> shadow_req = io_get_req(ctx, NULL); >> @@ -2741,6 +2729,16 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, >> trace_io_uring_submit_sqe(ctx, s.sqe->user_data, true, async); >> io_submit_sqe(ctx, &s, statep, &link); >> submitted++; >> + >> + /* >> + * If previous wasn't linked and we have a linked command, >> + * that's the end of the chain. Submit the previous link. >> + */ >> + if (!(s.sqe->flags & IOSQE_IO_LINK) && link) > The behavior changed to 'current seq' instead of previous after dropping prev_was_link? > The old behaviour was to remember @prev_was_link for current sqe, and use at the beginning of the next iteration, where it becomes "previous/last sqe". See, prev_was_link was set after io_queue_link_head. If @i is iteration idx, then timeline was: i: sqe[i-1].is_link -> io_queue_link_head() # if (prev_was_link) i: sqe[i].is_link = prev_was_link = (sqe[i].flags & LINK) i+1: sqe[i].is_link -> io_queue_link_head() # if (prev_was_link) i+1: sqe[i+1].is_link = ... After the change, it's done at the same loop iteration by swapping order of checking @prev_was_link and io_queue_link_head(). i: sqe[i].is_link = ... i: sqe[i].is_link -> io_queue_link_head() i+1: sqe[i+1].is_link = ... i+1: sqe[i+1].is_link -> io_queue_link_head() Shouldn't change the behavior, if I'm not missing something. >> + io_queue_link_head(ctx, link, &link->submit, shadow_req); >> + link = NULL; >> + shadow_req = NULL; >> + } >> } >> >> if (link) >> >
On 11/6/2019 12:06 PM, Pavel Begunkov wrote: > On 11/6/2019 11:36 AM, Bob Liu wrote: >> On 11/6/19 5:22 AM, Pavel Begunkov wrote: >>> After a call to io_submit_sqe(), it's already known whether it needs >>> to queue a link or not. Do it there, as it's simplier and doesn't keep >>> an extra variable across the loop. >>> >>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>> --- >>> fs/io_uring.c | 22 ++++++++++------------ >>> 1 file changed, 10 insertions(+), 12 deletions(-) >>> >>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>> index ebe2a4edd644..82c2da99cb5c 100644 >>> --- a/fs/io_uring.c >>> +++ b/fs/io_uring.c >>> @@ -2687,7 +2687,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, >>> struct io_submit_state state, *statep = NULL; >>> struct io_kiocb *link = NULL; >>> struct io_kiocb *shadow_req = NULL; >>> - bool prev_was_link = false; >>> int i, submitted = 0; >>> bool mm_fault = false; >>> >>> @@ -2710,17 +2709,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, >>> } >>> } >>> >>> - /* >>> - * If previous wasn't linked and we have a linked command, >>> - * that's the end of the chain. Submit the previous link. >>> - */ >>> - if (!prev_was_link && link) { >>> - io_queue_link_head(ctx, link, &link->submit, shadow_req); >>> - link = NULL; >>> - shadow_req = NULL; >>> - } >>> - prev_was_link = (s.sqe->flags & IOSQE_IO_LINK) != 0; >>> - >>> if (link && (s.sqe->flags & IOSQE_IO_DRAIN)) { >>> if (!shadow_req) { >>> shadow_req = io_get_req(ctx, NULL); >>> @@ -2741,6 +2729,16 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, >>> trace_io_uring_submit_sqe(ctx, s.sqe->user_data, true, async); >>> io_submit_sqe(ctx, &s, statep, &link); >>> submitted++; >>> + >>> + /* >>> + * If previous wasn't linked and we have a linked command, >>> + * that's the end of the chain. Submit the previous link. >>> + */ >>> + if (!(s.sqe->flags & IOSQE_IO_LINK) && link) >> The behavior changed to 'current seq' instead of previous after dropping prev_was_link? >> > The old behaviour was to remember @prev_was_link for current sqe, and > use at the beginning of the next iteration, where it becomes > "previous/last sqe". See, prev_was_link was set after io_queue_link_head. > > If @i is iteration idx, then timeline was: > i: sqe[i-1].is_link -> io_queue_link_head() # if (prev_was_link) > i: sqe[i].is_link = prev_was_link = (sqe[i].flags & LINK) > i+1: sqe[i].is_link -> io_queue_link_head() # if (prev_was_link) > i+1: sqe[i+1].is_link = ... > > > After the change, it's done at the same loop iteration by swapping order > of checking @prev_was_link and io_queue_link_head(). > > i: sqe[i].is_link = ... > i: sqe[i].is_link -> io_queue_link_head() > i+1: sqe[i+1].is_link = ... > i+1: sqe[i+1].is_link -> io_queue_link_head() > > Shouldn't change the behavior, if I'm not missing something. > And the same goes for ordering with io_submit_sqe(), which assembles a link. i: prev_was_link = ... # for sqe[i] i: io_submit_sqe() # for sqe[i] i+1: prev_was_link -> io_queue_link_head # for sqe[i] after: i: io_submit_sqe() # for sqe[i] i: is_link = ... # for sqe[i] i: is_link -> io_queue_link_head # for sqe[i] > >>> + io_queue_link_head(ctx, link, &link->submit, shadow_req); >>> + link = NULL; >>> + shadow_req = NULL; >>> + } >>> } >>> >>> if (link) >>> >>
On 11/6/19 5:31 PM, Pavel Begunkov wrote: > On 11/6/2019 12:06 PM, Pavel Begunkov wrote: >> On 11/6/2019 11:36 AM, Bob Liu wrote: >>> On 11/6/19 5:22 AM, Pavel Begunkov wrote: >>>> After a call to io_submit_sqe(), it's already known whether it needs >>>> to queue a link or not. Do it there, as it's simplier and doesn't keep >>>> an extra variable across the loop. >>>> >>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>> --- >>>> fs/io_uring.c | 22 ++++++++++------------ >>>> 1 file changed, 10 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>>> index ebe2a4edd644..82c2da99cb5c 100644 >>>> --- a/fs/io_uring.c >>>> +++ b/fs/io_uring.c >>>> @@ -2687,7 +2687,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, >>>> struct io_submit_state state, *statep = NULL; >>>> struct io_kiocb *link = NULL; >>>> struct io_kiocb *shadow_req = NULL; >>>> - bool prev_was_link = false; >>>> int i, submitted = 0; >>>> bool mm_fault = false; >>>> >>>> @@ -2710,17 +2709,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, >>>> } >>>> } >>>> >>>> - /* >>>> - * If previous wasn't linked and we have a linked command, >>>> - * that's the end of the chain. Submit the previous link. >>>> - */ >>>> - if (!prev_was_link && link) { >>>> - io_queue_link_head(ctx, link, &link->submit, shadow_req); >>>> - link = NULL; >>>> - shadow_req = NULL; >>>> - } >>>> - prev_was_link = (s.sqe->flags & IOSQE_IO_LINK) != 0; >>>> - >>>> if (link && (s.sqe->flags & IOSQE_IO_DRAIN)) { >>>> if (!shadow_req) { >>>> shadow_req = io_get_req(ctx, NULL); >>>> @@ -2741,6 +2729,16 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, >>>> trace_io_uring_submit_sqe(ctx, s.sqe->user_data, true, async); >>>> io_submit_sqe(ctx, &s, statep, &link); >>>> submitted++; >>>> + >>>> + /* >>>> + * If previous wasn't linked and we have a linked command, >>>> + * that's the end of the chain. Submit the previous link. >>>> + */ >>>> + if (!(s.sqe->flags & IOSQE_IO_LINK) && link) >>> The behavior changed to 'current seq' instead of previous after dropping prev_was_link? >>> >> The old behaviour was to remember @prev_was_link for current sqe, and >> use at the beginning of the next iteration, where it becomes >> "previous/last sqe". See, prev_was_link was set after io_queue_link_head. >> >> If @i is iteration idx, then timeline was: >> i: sqe[i-1].is_link -> io_queue_link_head() # if (prev_was_link) >> i: sqe[i].is_link = prev_was_link = (sqe[i].flags & LINK) >> i+1: sqe[i].is_link -> io_queue_link_head() # if (prev_was_link) >> i+1: sqe[i+1].is_link = ... >> >> >> After the change, it's done at the same loop iteration by swapping order >> of checking @prev_was_link and io_queue_link_head(). >> >> i: sqe[i].is_link = ... >> i: sqe[i].is_link -> io_queue_link_head() >> i+1: sqe[i+1].is_link = ... >> i+1: sqe[i+1].is_link -> io_queue_link_head() >> >> Shouldn't change the behavior, if I'm not missing something. >> > And the same goes for ordering with io_submit_sqe(), which assembles a link. > > i: prev_was_link = ... # for sqe[i] > i: io_submit_sqe() # for sqe[i] > i+1: prev_was_link -> io_queue_link_head # for sqe[i] > > after: > i: io_submit_sqe() # for sqe[i] > i: is_link = ... # for sqe[i] > i: is_link -> io_queue_link_head # for sqe[i] > I see, sorry for the noise. Reviewed-by： Bob Liu <bob.liu@oracle.com> >> >>>> + io_queue_link_head(ctx, link, &link->submit, shadow_req); >>>> + link = NULL; >>>> + shadow_req = NULL; >>>> + } >>>> } >>>> >>>> if (link) >>>> >>>
diff --git a/fs/io_uring.c b/fs/io_uring.c index ebe2a4edd644..82c2da99cb5c 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2687,7 +2687,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, struct io_submit_state state, *statep = NULL; struct io_kiocb *link = NULL; struct io_kiocb *shadow_req = NULL; - bool prev_was_link = false; int i, submitted = 0; bool mm_fault = false; @@ -2710,17 +2709,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, } } - /* - * If previous wasn't linked and we have a linked command, - * that's the end of the chain. Submit the previous link. - */ - if (!prev_was_link && link) { - io_queue_link_head(ctx, link, &link->submit, shadow_req); - link = NULL; - shadow_req = NULL; - } - prev_was_link = (s.sqe->flags & IOSQE_IO_LINK) != 0; - if (link && (s.sqe->flags & IOSQE_IO_DRAIN)) { if (!shadow_req) { shadow_req = io_get_req(ctx, NULL); @@ -2741,6 +2729,16 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, trace_io_uring_submit_sqe(ctx, s.sqe->user_data, true, async); io_submit_sqe(ctx, &s, statep, &link); submitted++; + + /* + * If previous wasn't linked and we have a linked command, + * that's the end of the chain. Submit the previous link. + */ + if (!(s.sqe->flags & IOSQE_IO_LINK) && link) { + io_queue_link_head(ctx, link, &link->submit, shadow_req); + link = NULL; + shadow_req = NULL; + } } if (link)
After a call to io_submit_sqe(), it's already known whether it needs to queue a link or not. Do it there, as it's simplier and doesn't keep an extra variable across the loop. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/io_uring.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)