* [PATCH io_uring-5.14 v2] io_uring: remove double poll wait entry for pure poll @ 2021-07-23 9:22 Hao Xu 2021-07-23 14:31 ` Pavel Begunkov 0 siblings, 1 reply; 8+ messages in thread From: Hao Xu @ 2021-07-23 9:22 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi For pure poll requests, we should remove the double poll wait entry. And io_poll_remove_double() is good enough for it compared with io_poll_remove_waitqs(). Signed-off-by: Hao Xu <[email protected]> --- v1-->v2 delete redundant io_poll_remove_double() 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 f2fe4eca150b..c5fe8b9e26b4 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -4903,7 +4903,6 @@ static bool io_poll_complete(struct io_kiocb *req, __poll_t mask) if (req->poll.events & EPOLLONESHOT) flags = 0; if (!io_cqring_fill_event(ctx, req->user_data, error, flags)) { - io_poll_remove_waitqs(req); req->poll.done = true; flags = 0; } @@ -4926,6 +4925,7 @@ static void io_poll_task_func(struct io_kiocb *req) done = io_poll_complete(req, req->result); if (done) { + io_poll_remove_double(req); hash_del(&req->hash_node); } else { req->result = 0; @@ -5113,7 +5113,7 @@ static __poll_t __io_arm_poll_handler(struct io_kiocb *req, ipt->error = -EINVAL; spin_lock_irq(&ctx->completion_lock); - if (ipt->error) + if (ipt->error || (mask && (poll->events & EPOLLONESHOT))) io_poll_remove_double(req); if (likely(poll->head)) { spin_lock(&poll->head->lock); @@ -5185,7 +5185,6 @@ static int io_arm_poll_handler(struct io_kiocb *req) ret = __io_arm_poll_handler(req, &apoll->poll, &ipt, mask, io_async_wake); if (ret || ipt.error) { - io_poll_remove_double(req); spin_unlock_irq(&ctx->completion_lock); if (ret) return IO_APOLL_READY; -- 2.24.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH io_uring-5.14 v2] io_uring: remove double poll wait entry for pure poll 2021-07-23 9:22 [PATCH io_uring-5.14 v2] io_uring: remove double poll wait entry for pure poll Hao Xu @ 2021-07-23 14:31 ` Pavel Begunkov 2021-07-23 16:22 ` Jens Axboe 2021-07-24 4:48 ` Hao Xu 0 siblings, 2 replies; 8+ messages in thread From: Pavel Begunkov @ 2021-07-23 14:31 UTC (permalink / raw) To: Hao Xu, Jens Axboe; +Cc: io-uring, Joseph Qi On 7/23/21 10:22 AM, Hao Xu wrote: > For pure poll requests, we should remove the double poll wait entry. > And io_poll_remove_double() is good enough for it compared with > io_poll_remove_waitqs(). 5.14 in the subject hints me that it's a fix. Is it? Can you add what it fixes or expand on why it's better? > Signed-off-by: Hao Xu <[email protected]> > --- > > v1-->v2 > delete redundant io_poll_remove_double() > > 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 f2fe4eca150b..c5fe8b9e26b4 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -4903,7 +4903,6 @@ static bool io_poll_complete(struct io_kiocb *req, __poll_t mask) > if (req->poll.events & EPOLLONESHOT) > flags = 0; > if (!io_cqring_fill_event(ctx, req->user_data, error, flags)) { > - io_poll_remove_waitqs(req); > req->poll.done = true; > flags = 0; > } > @@ -4926,6 +4925,7 @@ static void io_poll_task_func(struct io_kiocb *req) > > done = io_poll_complete(req, req->result); > if (done) { > + io_poll_remove_double(req); > hash_del(&req->hash_node); > } else { > req->result = 0; > @@ -5113,7 +5113,7 @@ static __poll_t __io_arm_poll_handler(struct io_kiocb *req, > ipt->error = -EINVAL; > > spin_lock_irq(&ctx->completion_lock); > - if (ipt->error) > + if (ipt->error || (mask && (poll->events & EPOLLONESHOT))) > io_poll_remove_double(req); > if (likely(poll->head)) { > spin_lock(&poll->head->lock); > @@ -5185,7 +5185,6 @@ static int io_arm_poll_handler(struct io_kiocb *req) > ret = __io_arm_poll_handler(req, &apoll->poll, &ipt, mask, > io_async_wake); > if (ret || ipt.error) { > - io_poll_remove_double(req); > spin_unlock_irq(&ctx->completion_lock); > if (ret) > return IO_APOLL_READY; > -- Pavel Begunkov ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH io_uring-5.14 v2] io_uring: remove double poll wait entry for pure poll 2021-07-23 14:31 ` Pavel Begunkov @ 2021-07-23 16:22 ` Jens Axboe 2021-07-24 4:48 ` Hao Xu 1 sibling, 0 replies; 8+ messages in thread From: Jens Axboe @ 2021-07-23 16:22 UTC (permalink / raw) To: Pavel Begunkov, Hao Xu; +Cc: io-uring, Joseph Qi On 7/23/21 8:31 AM, Pavel Begunkov wrote: > On 7/23/21 10:22 AM, Hao Xu wrote: >> For pure poll requests, we should remove the double poll wait entry. >> And io_poll_remove_double() is good enough for it compared with >> io_poll_remove_waitqs(). > > 5.14 in the subject hints me that it's a fix. Is it? > Can you add what it fixes or expand on why it's better? Ditto that, the commit message explains what is being done, it should explain _why_ it's being done. For the 'what' part you can read the code. So while the patch doesn't look wrong, I also can't quite tell why the change is necessary. -- Jens Axboe ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH io_uring-5.14 v2] io_uring: remove double poll wait entry for pure poll 2021-07-23 14:31 ` Pavel Begunkov 2021-07-23 16:22 ` Jens Axboe @ 2021-07-24 4:48 ` Hao Xu 2021-07-26 12:40 ` Pavel Begunkov 1 sibling, 1 reply; 8+ messages in thread From: Hao Xu @ 2021-07-24 4:48 UTC (permalink / raw) To: Pavel Begunkov, Jens Axboe; +Cc: io-uring, Joseph Qi 在 2021/7/23 下午10:31, Pavel Begunkov 写道: > On 7/23/21 10:22 AM, Hao Xu wrote: >> For pure poll requests, we should remove the double poll wait entry. >> And io_poll_remove_double() is good enough for it compared with >> io_poll_remove_waitqs(). > > 5.14 in the subject hints me that it's a fix. Is it? > Can you add what it fixes or expand on why it's better? Hi Pavel, I found that for poll_add() requests, it doesn't remove the double poll wait entry when it's done, neither after vfs_poll() or in the poll completion handler. The patch is mainly to fix it. > > >> Signed-off-by: Hao Xu <[email protected]> >> --- >> >> v1-->v2 >> delete redundant io_poll_remove_double() >> >> 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 f2fe4eca150b..c5fe8b9e26b4 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -4903,7 +4903,6 @@ static bool io_poll_complete(struct io_kiocb *req, __poll_t mask) >> if (req->poll.events & EPOLLONESHOT) >> flags = 0; >> if (!io_cqring_fill_event(ctx, req->user_data, error, flags)) { >> - io_poll_remove_waitqs(req); Currently I only see it does that with io_poll_remove_waitqs() when cqring overflow and then ocqe allocation failed. Using io_poll_remove_waitqs() here is not very suitable since (1) it calls __io_poll_remove_one() which set poll->cancelled = true, why do we set poll->cancelled and poll->done to true at the same time though I think that doesn't cause any problem. (2) it does list_del_init(&poll->wait.entry) and hash_del(&req->hash_node) which has been already done. Correct me if I'm wrong since I may misunderstand the code. Regards, Hao >> req->poll.done = true; >> flags = 0; >> } >> @@ -4926,6 +4925,7 @@ static void io_poll_task_func(struct io_kiocb *req) >> >> done = io_poll_complete(req, req->result); >> if (done) { >> + io_poll_remove_double(req); >> hash_del(&req->hash_node); >> } else { >> req->result = 0; >> @@ -5113,7 +5113,7 @@ static __poll_t __io_arm_poll_handler(struct io_kiocb *req, >> ipt->error = -EINVAL; >> >> spin_lock_irq(&ctx->completion_lock); >> - if (ipt->error) >> + if (ipt->error || (mask && (poll->events & EPOLLONESHOT))) >> io_poll_remove_double(req); >> if (likely(poll->head)) { >> spin_lock(&poll->head->lock); >> @@ -5185,7 +5185,6 @@ static int io_arm_poll_handler(struct io_kiocb *req) >> ret = __io_arm_poll_handler(req, &apoll->poll, &ipt, mask, >> io_async_wake); >> if (ret || ipt.error) { >> - io_poll_remove_double(req); >> spin_unlock_irq(&ctx->completion_lock); >> if (ret) >> return IO_APOLL_READY; >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH io_uring-5.14 v2] io_uring: remove double poll wait entry for pure poll 2021-07-24 4:48 ` Hao Xu @ 2021-07-26 12:40 ` Pavel Begunkov 2021-07-26 14:39 ` Hao Xu 0 siblings, 1 reply; 8+ messages in thread From: Pavel Begunkov @ 2021-07-26 12:40 UTC (permalink / raw) To: Hao Xu, Jens Axboe; +Cc: io-uring, Joseph Qi On 7/24/21 5:48 AM, Hao Xu wrote: > 在 2021/7/23 下午10:31, Pavel Begunkov 写道: >> On 7/23/21 10:22 AM, Hao Xu wrote: >>> For pure poll requests, we should remove the double poll wait entry. >>> And io_poll_remove_double() is good enough for it compared with >>> io_poll_remove_waitqs(). >> >> 5.14 in the subject hints me that it's a fix. Is it? >> Can you add what it fixes or expand on why it's better? > Hi Pavel, I found that for poll_add() requests, it doesn't remove the > double poll wait entry when it's done, neither after vfs_poll() or in > the poll completion handler. The patch is mainly to fix it. Ok, sounds good. Please resend with updated description, and let's add some tags. Fixes: 88e41cf928a6 ("io_uring: add multishot mode for IORING_OP_POLL_ADD") Cc: [email protected] # 5.13+ Also, I'd prefer the commit title to make more clear that it's a fix. E.g. "io_uring: fix poll requests leaking second poll entries". Btw, seems it should fix hangs in ./poll-mshot-update >> >>> Signed-off-by: Hao Xu <[email protected]> >>> --- >>> >>> v1-->v2 >>> delete redundant io_poll_remove_double() >>> >>> 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 f2fe4eca150b..c5fe8b9e26b4 100644 >>> --- a/fs/io_uring.c >>> +++ b/fs/io_uring.c >>> @@ -4903,7 +4903,6 @@ static bool io_poll_complete(struct io_kiocb *req, __poll_t mask) >>> if (req->poll.events & EPOLLONESHOT) >>> flags = 0; >>> if (!io_cqring_fill_event(ctx, req->user_data, error, flags)) { >>> - io_poll_remove_waitqs(req); > Currently I only see it does that with io_poll_remove_waitqs() when > cqring overflow and then ocqe allocation failed. Using > io_poll_remove_waitqs() here is not very suitable since (1) it calls > __io_poll_remove_one() which set poll->cancelled = true, why do we set > poll->cancelled and poll->done to true at the same time though I think > that doesn't cause any problem. (2) it does > list_del_init(&poll->wait.entry) and hash_del(&req->hash_node) which > has been already done. > Correct me if I'm wrong since I may misunderstand the code. > > Regards, > Hao >>> req->poll.done = true; >>> flags = 0; >>> } >>> @@ -4926,6 +4925,7 @@ static void io_poll_task_func(struct io_kiocb *req) >>> done = io_poll_complete(req, req->result); >>> if (done) { >>> + io_poll_remove_double(req); >>> hash_del(&req->hash_node); >>> } else { >>> req->result = 0; >>> @@ -5113,7 +5113,7 @@ static __poll_t __io_arm_poll_handler(struct io_kiocb *req, >>> ipt->error = -EINVAL; >>> spin_lock_irq(&ctx->completion_lock); >>> - if (ipt->error) >>> + if (ipt->error || (mask && (poll->events & EPOLLONESHOT))) >>> io_poll_remove_double(req); >>> if (likely(poll->head)) { >>> spin_lock(&poll->head->lock); >>> @@ -5185,7 +5185,6 @@ static int io_arm_poll_handler(struct io_kiocb *req) >>> ret = __io_arm_poll_handler(req, &apoll->poll, &ipt, mask, >>> io_async_wake); >>> if (ret || ipt.error) { >>> - io_poll_remove_double(req); >>> spin_unlock_irq(&ctx->completion_lock); >>> if (ret) >>> return IO_APOLL_READY; >>> >> > -- Pavel Begunkov ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH io_uring-5.14 v2] io_uring: remove double poll wait entry for pure poll 2021-07-26 12:40 ` Pavel Begunkov @ 2021-07-26 14:39 ` Hao Xu 2021-07-27 22:46 ` Jens Axboe 0 siblings, 1 reply; 8+ messages in thread From: Hao Xu @ 2021-07-26 14:39 UTC (permalink / raw) To: Pavel Begunkov, Jens Axboe; +Cc: io-uring, Joseph Qi 在 2021/7/26 下午8:40, Pavel Begunkov 写道: > On 7/24/21 5:48 AM, Hao Xu wrote: >> 在 2021/7/23 下午10:31, Pavel Begunkov 写道: >>> On 7/23/21 10:22 AM, Hao Xu wrote: >>>> For pure poll requests, we should remove the double poll wait entry. >>>> And io_poll_remove_double() is good enough for it compared with >>>> io_poll_remove_waitqs(). >>> >>> 5.14 in the subject hints me that it's a fix. Is it? >>> Can you add what it fixes or expand on why it's better? >> Hi Pavel, I found that for poll_add() requests, it doesn't remove the >> double poll wait entry when it's done, neither after vfs_poll() or in >> the poll completion handler. The patch is mainly to fix it. > > Ok, sounds good. Please resend with updated description, and > let's add some tags. > > Fixes: 88e41cf928a6 ("io_uring: add multishot mode for IORING_OP_POLL_ADD") > Cc: [email protected] # 5.13+ > > Also, I'd prefer the commit title to make more clear that it's a > fix. E.g. "io_uring: fix poll requests leaking second poll entries". > > Btw, seems it should fix hangs in ./poll-mshot-update Sure,I'll send v3 soon, sorry for my unprofessionalism.. > > >>> >>>> Signed-off-by: Hao Xu <[email protected]> >>>> --- >>>> >>>> v1-->v2 >>>> delete redundant io_poll_remove_double() >>>> >>>> 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 f2fe4eca150b..c5fe8b9e26b4 100644 >>>> --- a/fs/io_uring.c >>>> +++ b/fs/io_uring.c >>>> @@ -4903,7 +4903,6 @@ static bool io_poll_complete(struct io_kiocb *req, __poll_t mask) >>>> if (req->poll.events & EPOLLONESHOT) >>>> flags = 0; >>>> if (!io_cqring_fill_event(ctx, req->user_data, error, flags)) { >>>> - io_poll_remove_waitqs(req); >> Currently I only see it does that with io_poll_remove_waitqs() when >> cqring overflow and then ocqe allocation failed. Using >> io_poll_remove_waitqs() here is not very suitable since (1) it calls >> __io_poll_remove_one() which set poll->cancelled = true, why do we set >> poll->cancelled and poll->done to true at the same time though I think >> that doesn't cause any problem. (2) it does >> list_del_init(&poll->wait.entry) and hash_del(&req->hash_node) which >> has been already done. >> Correct me if I'm wrong since I may misunderstand the code. >> >> Regards, >> Hao >>>> req->poll.done = true; >>>> flags = 0; >>>> } >>>> @@ -4926,6 +4925,7 @@ static void io_poll_task_func(struct io_kiocb *req) >>>> done = io_poll_complete(req, req->result); >>>> if (done) { >>>> + io_poll_remove_double(req); >>>> hash_del(&req->hash_node); >>>> } else { >>>> req->result = 0; >>>> @@ -5113,7 +5113,7 @@ static __poll_t __io_arm_poll_handler(struct io_kiocb *req, >>>> ipt->error = -EINVAL; >>>> spin_lock_irq(&ctx->completion_lock); >>>> - if (ipt->error) >>>> + if (ipt->error || (mask && (poll->events & EPOLLONESHOT))) >>>> io_poll_remove_double(req); >>>> if (likely(poll->head)) { >>>> spin_lock(&poll->head->lock); >>>> @@ -5185,7 +5185,6 @@ static int io_arm_poll_handler(struct io_kiocb *req) >>>> ret = __io_arm_poll_handler(req, &apoll->poll, &ipt, mask, >>>> io_async_wake); >>>> if (ret || ipt.error) { >>>> - io_poll_remove_double(req); >>>> spin_unlock_irq(&ctx->completion_lock); >>>> if (ret) >>>> return IO_APOLL_READY; >>>> >>> >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH io_uring-5.14 v2] io_uring: remove double poll wait entry for pure poll 2021-07-26 14:39 ` Hao Xu @ 2021-07-27 22:46 ` Jens Axboe 2021-07-28 6:06 ` Hao Xu 0 siblings, 1 reply; 8+ messages in thread From: Jens Axboe @ 2021-07-27 22:46 UTC (permalink / raw) To: Hao Xu, Pavel Begunkov; +Cc: io-uring, Joseph Qi On 7/26/21 8:39 AM, Hao Xu wrote: > 在 2021/7/26 下午8:40, Pavel Begunkov 写道: >> On 7/24/21 5:48 AM, Hao Xu wrote: >>> 在 2021/7/23 下午10:31, Pavel Begunkov 写道: >>>> On 7/23/21 10:22 AM, Hao Xu wrote: >>>>> For pure poll requests, we should remove the double poll wait entry. >>>>> And io_poll_remove_double() is good enough for it compared with >>>>> io_poll_remove_waitqs(). >>>> >>>> 5.14 in the subject hints me that it's a fix. Is it? >>>> Can you add what it fixes or expand on why it's better? >>> Hi Pavel, I found that for poll_add() requests, it doesn't remove the >>> double poll wait entry when it's done, neither after vfs_poll() or in >>> the poll completion handler. The patch is mainly to fix it. >> >> Ok, sounds good. Please resend with updated description, and >> let's add some tags. >> >> Fixes: 88e41cf928a6 ("io_uring: add multishot mode for IORING_OP_POLL_ADD") >> Cc: [email protected] # 5.13+ >> >> Also, I'd prefer the commit title to make more clear that it's a >> fix. E.g. "io_uring: fix poll requests leaking second poll entries". >> >> Btw, seems it should fix hangs in ./poll-mshot-update > Sure,I'll send v3 soon, sorry for my unprofessionalism.. Are you going to send out v3? -- Jens Axboe ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH io_uring-5.14 v2] io_uring: remove double poll wait entry for pure poll 2021-07-27 22:46 ` Jens Axboe @ 2021-07-28 6:06 ` Hao Xu 0 siblings, 0 replies; 8+ messages in thread From: Hao Xu @ 2021-07-28 6:06 UTC (permalink / raw) To: Jens Axboe, Pavel Begunkov; +Cc: io-uring, Joseph Qi 在 2021/7/28 上午6:46, Jens Axboe 写道: > On 7/26/21 8:39 AM, Hao Xu wrote: >> 在 2021/7/26 下午8:40, Pavel Begunkov 写道: >>> On 7/24/21 5:48 AM, Hao Xu wrote: >>>> 在 2021/7/23 下午10:31, Pavel Begunkov 写道: >>>>> On 7/23/21 10:22 AM, Hao Xu wrote: >>>>>> For pure poll requests, we should remove the double poll wait entry. >>>>>> And io_poll_remove_double() is good enough for it compared with >>>>>> io_poll_remove_waitqs(). >>>>> >>>>> 5.14 in the subject hints me that it's a fix. Is it? >>>>> Can you add what it fixes or expand on why it's better? >>>> Hi Pavel, I found that for poll_add() requests, it doesn't remove the >>>> double poll wait entry when it's done, neither after vfs_poll() or in >>>> the poll completion handler. The patch is mainly to fix it. >>> >>> Ok, sounds good. Please resend with updated description, and >>> let's add some tags. >>> >>> Fixes: 88e41cf928a6 ("io_uring: add multishot mode for IORING_OP_POLL_ADD") >>> Cc: [email protected] # 5.13+ >>> >>> Also, I'd prefer the commit title to make more clear that it's a >>> fix. E.g. "io_uring: fix poll requests leaking second poll entries". >>> >>> Btw, seems it should fix hangs in ./poll-mshot-update >> Sure,I'll send v3 soon, sorry for my unprofessionalism.. > > Are you going to send out v3? > v3 sent. Btw I'm working on letting fast poll support multishot, I believe that will benefit non-persistent programming, let's see if it helps accept(). ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-07-28 6:06 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-07-23 9:22 [PATCH io_uring-5.14 v2] io_uring: remove double poll wait entry for pure poll Hao Xu 2021-07-23 14:31 ` Pavel Begunkov 2021-07-23 16:22 ` Jens Axboe 2021-07-24 4:48 ` Hao Xu 2021-07-26 12:40 ` Pavel Begunkov 2021-07-26 14:39 ` Hao Xu 2021-07-27 22:46 ` Jens Axboe 2021-07-28 6:06 ` Hao Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox