| Message ID | 0c95251624397ea6def568ff040cad2d7926fd51.1668963050.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | poll_refs armoring | expand |
On 11/20/22 16:57, Pavel Begunkov wrote: > Replace atomically substracting the ownership reference at the end of > arming a poll with a cmpxchg. We try to release ownership by setting 0 > assuming that poll_refs didn't change while we were arming. If it did > change, we keep the ownership and use it to queue a tw, which is fully > capable to process all events and (even tolerates spurious wake ups). > > It's a bit more elegant as we reduce races b/w setting the cancellation > flag and getting refs with this release, and with that we don't have to > worry about any kinds of underflows. It's not the fastest path for > polling. The performance difference b/w cmpxchg and atomic dec is > usually negligible and it's not the fastest path. Jens, can you add a couple of tags? Not a fix but the second patch depends on it but applies cleanly even without 1/2, which may mess things up. Cc: stable@vger.kernel.org Fixes: aa43477b04025 ("io_uring: poll rework") > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > io_uring/poll.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/io_uring/poll.c b/io_uring/poll.c > index 055632e9092a..1b78b527075d 100644 > --- a/io_uring/poll.c > +++ b/io_uring/poll.c > @@ -518,7 +518,6 @@ static int __io_arm_poll_handler(struct io_kiocb *req, > unsigned issue_flags) > { > struct io_ring_ctx *ctx = req->ctx; > - int v; > > INIT_HLIST_NODE(&req->hash_node); > req->work.cancel_seq = atomic_read(&ctx->cancel_seq); > @@ -586,11 +585,10 @@ static int __io_arm_poll_handler(struct io_kiocb *req, > > if (ipt->owning) { > /* > - * Release ownership. If someone tried to queue a tw while it was > - * locked, kick it off for them. > + * Try to release ownership. If we see a change of state, e.g. > + * poll was waken up, queue up a tw, it'll deal with it. > */ > - v = atomic_dec_return(&req->poll_refs); > - if (unlikely(v & IO_POLL_REF_MASK)) > + if (atomic_cmpxchg(&req->poll_refs, 1, 0) != 1) > __io_poll_execute(req, 0); > } > return 0;
On 11/25/22 6:51 AM, Pavel Begunkov wrote: > On 11/20/22 16:57, Pavel Begunkov wrote: >> Replace atomically substracting the ownership reference at the end of >> arming a poll with a cmpxchg. We try to release ownership by setting 0 >> assuming that poll_refs didn't change while we were arming. If it did >> change, we keep the ownership and use it to queue a tw, which is fully >> capable to process all events and (even tolerates spurious wake ups). >> >> It's a bit more elegant as we reduce races b/w setting the cancellation >> flag and getting refs with this release, and with that we don't have to >> worry about any kinds of underflows. It's not the fastest path for >> polling. The performance difference b/w cmpxchg and atomic dec is >> usually negligible and it's not the fastest path. > > Jens, can you add a couple of tags? Not a fix but the second patch > depends on it but applies cleanly even without 1/2, which may mess > things up. > > Cc: stable@vger.kernel.org > Fixes: aa43477b04025 ("io_uring: poll rework") Sure, done.
diff --git a/io_uring/poll.c b/io_uring/poll.c index 055632e9092a..1b78b527075d 100644 --- a/io_uring/poll.c +++ b/io_uring/poll.c @@ -518,7 +518,6 @@ static int __io_arm_poll_handler(struct io_kiocb *req, unsigned issue_flags) { struct io_ring_ctx *ctx = req->ctx; - int v; INIT_HLIST_NODE(&req->hash_node); req->work.cancel_seq = atomic_read(&ctx->cancel_seq); @@ -586,11 +585,10 @@ static int __io_arm_poll_handler(struct io_kiocb *req, if (ipt->owning) { /* - * Release ownership. If someone tried to queue a tw while it was - * locked, kick it off for them. + * Try to release ownership. If we see a change of state, e.g. + * poll was waken up, queue up a tw, it'll deal with it. */ - v = atomic_dec_return(&req->poll_refs); - if (unlikely(v & IO_POLL_REF_MASK)) + if (atomic_cmpxchg(&req->poll_refs, 1, 0) != 1) __io_poll_execute(req, 0); } return 0;
Replace atomically substracting the ownership reference at the end of arming a poll with a cmpxchg. We try to release ownership by setting 0 assuming that poll_refs didn't change while we were arming. If it did change, we keep the ownership and use it to queue a tw, which is fully capable to process all events and (even tolerates spurious wake ups). It's a bit more elegant as we reduce races b/w setting the cancellation flag and getting refs with this release, and with that we don't have to worry about any kinds of underflows. It's not the fastest path for polling. The performance difference b/w cmpxchg and atomic dec is usually negligible and it's not the fastest path. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- io_uring/poll.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)