* [PATCH] io_uring: fix duplicated increase of cached_cq_overflow @ 2019-11-15 9:37 Bob Liu 2019-11-15 9:49 ` Pavel Begunkov 0 siblings, 1 reply; 6+ messages in thread From: Bob Liu @ 2019-11-15 9:37 UTC (permalink / raw) To: axboe; +Cc: io-uring, Bob Liu cached_cq_overflow already be increased in function io_cqring_overflow_flush(). Signed-off-by: Bob Liu <[email protected]> --- fs/io_uring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 55f8b1d..eb23451 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -701,7 +701,7 @@ static void io_cqring_fill_event(struct io_kiocb *req, long res) WRITE_ONCE(cqe->flags, 0); } else if (ctx->cq_overflow_flushed) { WRITE_ONCE(ctx->rings->cq_overflow, - atomic_inc_return(&ctx->cached_cq_overflow)); + atomic_read(&ctx->cached_cq_overflow)); } else { refcount_inc(&req->refs); req->result = res; -- 2.9.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: fix duplicated increase of cached_cq_overflow 2019-11-15 9:37 [PATCH] io_uring: fix duplicated increase of cached_cq_overflow Bob Liu @ 2019-11-15 9:49 ` Pavel Begunkov 2019-11-15 12:17 ` Bob Liu 0 siblings, 1 reply; 6+ messages in thread From: Pavel Begunkov @ 2019-11-15 9:49 UTC (permalink / raw) To: Bob Liu, axboe; +Cc: io-uring On 11/15/2019 12:37 PM, Bob Liu wrote: > cached_cq_overflow already be increased in function > io_cqring_overflow_flush(). > > Signed-off-by: Bob Liu <[email protected]> > --- > fs/io_uring.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index 55f8b1d..eb23451 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -701,7 +701,7 @@ static void io_cqring_fill_event(struct io_kiocb *req, long res) > WRITE_ONCE(cqe->flags, 0); > } else if (ctx->cq_overflow_flushed) { > WRITE_ONCE(ctx->rings->cq_overflow, > - atomic_inc_return(&ctx->cached_cq_overflow)); > + atomic_read(&ctx->cached_cq_overflow)); Not really. It won't get into io_cqring_overflow_flush() if this branch is executed. See, it's enqueued for overflow in "else" right below. i.e. list_add_tail(&req->list, &ctx->cq_overflow_list); > } else { > refcount_inc(&req->refs); > req->result = res; > -- Pavel Begunkov ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: fix duplicated increase of cached_cq_overflow 2019-11-15 9:49 ` Pavel Begunkov @ 2019-11-15 12:17 ` Bob Liu 2019-11-15 12:41 ` Pavel Begunkov 0 siblings, 1 reply; 6+ messages in thread From: Bob Liu @ 2019-11-15 12:17 UTC (permalink / raw) To: Pavel Begunkov, axboe; +Cc: io-uring On 11/15/19 5:49 PM, Pavel Begunkov wrote: > On 11/15/2019 12:37 PM, Bob Liu wrote: >> cached_cq_overflow already be increased in function >> io_cqring_overflow_flush(). >> >> Signed-off-by: Bob Liu <[email protected]> >> --- >> fs/io_uring.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index 55f8b1d..eb23451 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -701,7 +701,7 @@ static void io_cqring_fill_event(struct io_kiocb *req, long res) >> WRITE_ONCE(cqe->flags, 0); >> } else if (ctx->cq_overflow_flushed) { >> WRITE_ONCE(ctx->rings->cq_overflow, >> - atomic_inc_return(&ctx->cached_cq_overflow)); >> + atomic_read(&ctx->cached_cq_overflow)); > > Not really. It won't get into io_cqring_overflow_flush() if this branch > is executed. io_cqring_overflow_flush(force=true) must have been called when this branch is executed, since io_cqring_overflow_flush() is the only place can set 'ctx->cq_overflow_flushed' to true. And 'ctx->cached_cq_overflow' may already be increased in io_cqring_overflow_flush() if force is true and cqe==NULL. static void io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force) { ... if (force) ctx->cq_overflow_flushed = true; while (!list_empty(&ctx->cq_overflow_list)) { cqe = io_get_cqring(ctx); if (!cqe && !force) break; req = list_first_entry(&ctx->cq_overflow_list, struct io_kiocb, list); list_move(&req->list, &list); if (cqe) { ... } else { WRITE_ONCE(ctx->rings->cq_overflow, atomic_inc_return(&ctx->cached_cq_overflow)); ^^^^^^^^^^^ ctx->cached_cq_overflow is increased if 'force=true' and 'ceq==NULL'. Did I miss anything? > See, it's enqueued for overflow in "else" right below. > > i.e. list_add_tail(&req->list, &ctx->cq_overflow_list); > >> } else { >> refcount_inc(&req->refs); >> req->result = res; >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: fix duplicated increase of cached_cq_overflow 2019-11-15 12:17 ` Bob Liu @ 2019-11-15 12:41 ` Pavel Begunkov 2019-11-15 13:10 ` Bob Liu 0 siblings, 1 reply; 6+ messages in thread From: Pavel Begunkov @ 2019-11-15 12:41 UTC (permalink / raw) To: Bob Liu, axboe; +Cc: io-uring On 11/15/2019 3:17 PM, Bob Liu wrote: > On 11/15/19 5:49 PM, Pavel Begunkov wrote: >> On 11/15/2019 12:37 PM, Bob Liu wrote: >>> cached_cq_overflow already be increased in function >>> io_cqring_overflow_flush(). >>> >>> Signed-off-by: Bob Liu <[email protected]> >>> --- >>> fs/io_uring.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>> index 55f8b1d..eb23451 100644 >>> --- a/fs/io_uring.c >>> +++ b/fs/io_uring.c >>> @@ -701,7 +701,7 @@ static void io_cqring_fill_event(struct io_kiocb *req, long res) >>> WRITE_ONCE(cqe->flags, 0); >>> } else if (ctx->cq_overflow_flushed) { >>> WRITE_ONCE(ctx->rings->cq_overflow, >>> - atomic_inc_return(&ctx->cached_cq_overflow)); >>> + atomic_read(&ctx->cached_cq_overflow)); >> >> Not really. It won't get into io_cqring_overflow_flush() if this branch >> is executed. > > io_cqring_overflow_flush(force=true) must have been called when this branch is executed, > since io_cqring_overflow_flush() is the only place can set 'ctx->cq_overflow_flushed' to true. > Yes, it should have been called, but it sets this flag for the future users of io_cqring_fill_event(), so any _new_ requests in io_cqring_fill_event() will overflow instead of being added to @overflow_list. Either a request completes/overflows in io_cqring_fill_event(), or it would be added to @overflow_list to be processed by io_cqring_overflow_flush(). > And 'ctx->cached_cq_overflow' may already be increased in io_cqring_overflow_flush() if force is true and cqe==NULL. > > static void io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force) > { > ... > if (force) > ctx->cq_overflow_flushed = true; > > while (!list_empty(&ctx->cq_overflow_list)) { > cqe = io_get_cqring(ctx); > if (!cqe && !force) > break; > > req = list_first_entry(&ctx->cq_overflow_list, struct io_kiocb, > list); > list_move(&req->list, &list); > if (cqe) { > ... > } else { > WRITE_ONCE(ctx->rings->cq_overflow, > atomic_inc_return(&ctx->cached_cq_overflow)); > ^^^^^^^^^^^ > ctx->cached_cq_overflow is increased if 'force=true' and 'ceq==NULL'. > > > Did I miss anything? > >> See, it's enqueued for overflow in "else" right below. >> >> i.e. list_add_tail(&req->list, &ctx->cq_overflow_list); >> >>> } else { >>> refcount_inc(&req->refs); >>> req->result = res; >>> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: fix duplicated increase of cached_cq_overflow 2019-11-15 12:41 ` Pavel Begunkov @ 2019-11-15 13:10 ` Bob Liu 2019-11-15 16:35 ` Pavel Begunkov 0 siblings, 1 reply; 6+ messages in thread From: Bob Liu @ 2019-11-15 13:10 UTC (permalink / raw) To: Pavel Begunkov, axboe; +Cc: io-uring On 11/15/19 8:41 PM, Pavel Begunkov wrote: > On 11/15/2019 3:17 PM, Bob Liu wrote: >> On 11/15/19 5:49 PM, Pavel Begunkov wrote: >>> On 11/15/2019 12:37 PM, Bob Liu wrote: >>>> cached_cq_overflow already be increased in function >>>> io_cqring_overflow_flush(). >>>> >>>> Signed-off-by: Bob Liu <[email protected]> >>>> --- >>>> fs/io_uring.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>>> index 55f8b1d..eb23451 100644 >>>> --- a/fs/io_uring.c >>>> +++ b/fs/io_uring.c >>>> @@ -701,7 +701,7 @@ static void io_cqring_fill_event(struct io_kiocb *req, long res) >>>> WRITE_ONCE(cqe->flags, 0); >>>> } else if (ctx->cq_overflow_flushed) { >>>> WRITE_ONCE(ctx->rings->cq_overflow, >>>> - atomic_inc_return(&ctx->cached_cq_overflow)); >>>> + atomic_read(&ctx->cached_cq_overflow)); >>> >>> Not really. It won't get into io_cqring_overflow_flush() if this branch >>> is executed. >> >> io_cqring_overflow_flush(force=true) must have been called when this branch is executed, >> since io_cqring_overflow_flush() is the only place can set 'ctx->cq_overflow_flushed' to true. >> > Yes, it should have been called, but it sets this flag for the future > users of io_cqring_fill_event(), so any _new_ requests in > io_cqring_fill_event() will overflow instead of being added to > @overflow_list. > Oh, I see..Thanks for the kindly explanation. > Either a request completes/overflows in io_cqring_fill_event(), > or it would be added to @overflow_list to be processed by > io_cqring_overflow_flush(). > > >> And 'ctx->cached_cq_overflow' may already be increased in io_cqring_overflow_flush() if force is true and cqe==NULL. >> >> static void io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force) >> { >> ... >> if (force) >> ctx->cq_overflow_flushed = true; >> >> while (!list_empty(&ctx->cq_overflow_list)) { >> cqe = io_get_cqring(ctx); >> if (!cqe && !force) >> break; >> >> req = list_first_entry(&ctx->cq_overflow_list, struct io_kiocb, >> list); >> list_move(&req->list, &list); >> if (cqe) { >> ... >> } else { >> WRITE_ONCE(ctx->rings->cq_overflow, >> atomic_inc_return(&ctx->cached_cq_overflow)); >> ^^^^^^^^^^^ >> ctx->cached_cq_overflow is increased if 'force=true' and 'ceq==NULL'. >> >> >> Did I miss anything? >> >>> See, it's enqueued for overflow in "else" right below. >>> >>> i.e. list_add_tail(&req->list, &ctx->cq_overflow_list); >>> >>>> } else { >>>> refcount_inc(&req->refs); >>>> req->result = res; >>>> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] io_uring: fix duplicated increase of cached_cq_overflow 2019-11-15 13:10 ` Bob Liu @ 2019-11-15 16:35 ` Pavel Begunkov 0 siblings, 0 replies; 6+ messages in thread From: Pavel Begunkov @ 2019-11-15 16:35 UTC (permalink / raw) To: Bob Liu, axboe; +Cc: io-uring [-- Attachment #1.1: Type: text/plain, Size: 563 bytes --] On 15/11/2019 16:10, Bob Liu wrote: >>> io_cqring_overflow_flush(force=true) must have been called when this branch is executed, >>> since io_cqring_overflow_flush() is the only place can set 'ctx->cq_overflow_flushed' to true. >>> >> Yes, it should have been called, but it sets this flag for the future >> users of io_cqring_fill_event(), so any _new_ requests in >> io_cqring_fill_event() will overflow instead of being added to >> @overflow_list. >> > > Oh, I see..Thanks for the kindly explanation. > Sure, no problem -- Pavel Begunkov [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-15 16:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-11-15 9:37 [PATCH] io_uring: fix duplicated increase of cached_cq_overflow Bob Liu 2019-11-15 9:49 ` Pavel Begunkov 2019-11-15 12:17 ` Bob Liu 2019-11-15 12:41 ` Pavel Begunkov 2019-11-15 13:10 ` Bob Liu 2019-11-15 16:35 ` Pavel Begunkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox