public inbox for [email protected]
 help / color / mirror / Atom feed
From: Nathan Chancellor <[email protected]>
To: Pavel Begunkov <[email protected]>
Cc: [email protected], Jens Axboe <[email protected]>,
	[email protected]
Subject: Re: [PATCH for-next v3 16/16] io_uring: mutex locked poll hashing
Date: Fri, 17 Jun 2022 08:35:07 -0700	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <1bbad9c78c454b7b92f100bbf46730a37df7194f.1655371007.git.asml.silence@gmail.com>

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 <[email protected]>

<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,

  reply	other threads:[~2022-06-17 15:35 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-16  9:21 [PATCH for-next v3 00/16] 5.20 cleanups and poll optimisations Pavel Begunkov
2022-06-16  9:21 ` [PATCH for-next v3 01/16] io_uring: rw: delegate sync completions to core io_uring Pavel Begunkov
2022-06-16  9:21 ` [PATCH for-next v3 02/16] io_uring: kill REQ_F_COMPLETE_INLINE Pavel Begunkov
2022-06-16  9:21 ` [PATCH for-next v3 03/16] io_uring: refactor io_req_task_complete() Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 04/16] io_uring: don't inline io_put_kbuf Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 05/16] io_uring: poll: remove unnecessary req->ref set Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 06/16] io_uring: switch cancel_hash to use per entry spinlock Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 07/16] io_uring: pass poll_find lock back Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 08/16] io_uring: clean up io_try_cancel Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 09/16] io_uring: limit the number of cancellation buckets Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 10/16] io_uring: clean up io_ring_ctx_alloc Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 11/16] io_uring: use state completion infra for poll reqs Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 12/16] io_uring: add IORING_SETUP_SINGLE_ISSUER Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 13/16] io_uring: pass hash table into poll_find Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 14/16] io_uring: introduce a struct for hash table Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 15/16] io_uring: propagate locking state to poll cancel Pavel Begunkov
2022-06-16  9:22 ` [PATCH for-next v3 16/16] io_uring: mutex locked poll hashing Pavel Begunkov
2022-06-17 15:35   ` Nathan Chancellor [this message]
2022-06-18 12:07     ` Pavel Begunkov
2022-06-16 13:18 ` [PATCH for-next v3 00/16] 5.20 cleanups and poll optimisations Jens Axboe
2022-06-16 15:58 ` Hao Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox