Re: [PATCH for-next v3 16/16] io_uring: mutex locked poll hashing

[Pavel Begunkov <asml.silence@gmail.com>]

On 6/17/22 16:35, Nathan Chancellor wrote:
> 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@gmail.com>
> 
> <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.

Thanks for the heads up. Yes, it's intentional, and I'm fine with either
version. Are you going to send a patch?


-- 
Pavel Begunkov