On Thu, Jun 16, 2022 at 10:22:12AM +0100, Pavel Begunkov wrote:
> Currently we do two extra spin lock/unlock pairs to add a poll/apoll
> request to the cancellation hash table and remove it from there.
>
> On the submission side we often already hold ->uring_lock and tw
> completion is likely to hold it as well. Add a second cancellation hash
> table protected by ->uring_lock. In concerns for latency because of a
> need to have the mutex locked on the completion side, use the new table
> only in following cases:
>
> 1) IORING_SETUP_SINGLE_ISSUER: only one task grabs uring_lock, so there
> is little to no contention and so the main tw hander will almost
> always end up grabbing it before calling callbacks.
>
> 2) IORING_SETUP_SQPOLL: same as with single issuer, only one task is
> a major user of ->uring_lock.
>
> 3) apoll: we normally grab the lock on the completion side anyway to
> execute the request, so it's free.
>
> Signed-off-by: Pavel Begunkov <asml.silence@xxxxxxxxx>
<snip>
> -/*
> - * Returns true if we found and killed one or more poll requests
> - */
> -__cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk,
> - bool cancel_all)
> +static __cold bool io_poll_remove_all_table(struct task_struct *tsk,
> + struct io_hash_table *table,
> + bool cancel_all)
> {
> - struct io_hash_table *table = &ctx->cancel_table;
> unsigned nr_buckets = 1U << table->hash_bits;
> struct hlist_node *tmp;
> struct io_kiocb *req;
> @@ -557,6 +592,17 @@ __cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk,
> return found;
> }
>
> +/*
> + * Returns true if we found and killed one or more poll requests
> + */
> +__cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk,
> + bool cancel_all)
> + __must_hold(&ctx->uring_lock)
> +{
> + return io_poll_remove_all_table(tsk, &ctx->cancel_table, cancel_all) |
> + io_poll_remove_all_table(tsk, &ctx->cancel_table_locked, cancel_all);
> +}
Clang warns:
io_uring/poll.c:602:9: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
return io_poll_remove_all_table(tsk, &ctx->cancel_table, cancel_all) |
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
||
io_uring/poll.c:602:9: note: cast one or both operands to int to silence this warning
1 error generated.
I assume this is intentional so io_poll_remove_all_table() gets called
twice every time? If so, would you be opposed to unrolling this a bit to
make it clear to the compiler? Alternatively, we could change the return
type of io_poll_remove_all_table to be an int or add a cast to int like
the note mentioned but that is rather ugly to me. I can send a formal
patch depending on your preference.
Cheers,
Nathan
diff --git a/io_uring/poll.c b/io_uring/poll.c
index 2e068e05732a..6a70bc220971 100644
--- a/io_uring/poll.c
+++ b/io_uring/poll.c
@@ -599,8 +599,12 @@ __cold bool io_poll_remove_all(struct io_ring_ctx *ctx, struct task_struct *tsk,
bool cancel_all)
__must_hold(&ctx->uring_lock)
{
- return io_poll_remove_all_table(tsk, &ctx->cancel_table, cancel_all) |
- io_poll_remove_all_table(tsk, &ctx->cancel_table_locked, cancel_all);
+ bool ret;
+
+ ret = io_poll_remove_all_table(tsk, &ctx->cancel_table, cancel_all);
+ ret |= io_poll_remove_all_table(tsk, &ctx->cancel_table_locked, cancel_all);
+
+ return ret;
}
static struct io_kiocb *io_poll_find(struct io_ring_ctx *ctx, bool poll_only,
