Re: [PATCH 0/2] fix hangs with shared sqpoll

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

On 16/04/2021 14:04, Jens Axboe wrote:
> On 4/15/21 6:26 PM, Pavel Begunkov wrote:
>> On 16/04/2021 01:22, Pavel Begunkov wrote:
>>> Late catched 5.12 bug with nasty hangs. Thanks Jens for a reproducer.
>>
>> 1/2 is basically a rip off of one of old Jens' patches, but can't
>> find it anywhere. If you still have it, especially if it was
>> reviewed/etc., may make sense to go with it instead
> 
> I wonder if we can do something like the below instead - we don't
> care about a particularly stable count in terms of wakeup
> reliance, and it'd save a nasty sync atomic switch.

But we care about it being monotonous. There are nuances with it.
I think, non sync'ed summing may put it to eternal sleep.

Are you looking to save on switching? It's almost always is already
dying with prior ref_kill

> 
> Totally untested...
> 
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 6c182a3a221b..9edbcf01ea49 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -8928,7 +8928,7 @@ static void io_uring_cancel_sqpoll(struct io_ring_ctx *ctx)
>  	atomic_inc(&tctx->in_idle);
>  	do {
>  		/* read completions before cancelations */
> -		inflight = tctx_inflight(tctx, false);
> +		inflight = percpu_ref_sum(&ctx->refs);
>  		if (!inflight)
>  			break;
>  		io_uring_try_cancel_requests(ctx, current, NULL);
> @@ -8939,7 +8939,7 @@ static void io_uring_cancel_sqpoll(struct io_ring_ctx *ctx)
>  		 * avoids a race where a completion comes in before we did
>  		 * prepare_to_wait().
>  		 */
> -		if (inflight == tctx_inflight(tctx, false))
> +		if (inflight == percpu_ref_sum(&ctx->refs))
>  			schedule();
>  		finish_wait(&tctx->wait, &wait);
>  	} while (1);
> diff --git a/include/linux/percpu-refcount.h b/include/linux/percpu-refcount.h
> index 16c35a728b4c..2f29f34bc993 100644
> --- a/include/linux/percpu-refcount.h
> +++ b/include/linux/percpu-refcount.h
> @@ -131,6 +131,7 @@ void percpu_ref_kill_and_confirm(struct percpu_ref *ref,
>  void percpu_ref_resurrect(struct percpu_ref *ref);
>  void percpu_ref_reinit(struct percpu_ref *ref);
>  bool percpu_ref_is_zero(struct percpu_ref *ref);
> +long percpu_ref_sum(struct percpu_ref *ref);
>  
>  /**
>   * percpu_ref_kill - drop the initial ref
> diff --git a/lib/percpu-refcount.c b/lib/percpu-refcount.c
> index a1071cdefb5a..b09ed9fdd32d 100644
> --- a/lib/percpu-refcount.c
> +++ b/lib/percpu-refcount.c
> @@ -475,3 +475,31 @@ void percpu_ref_resurrect(struct percpu_ref *ref)
>  	spin_unlock_irqrestore(&percpu_ref_switch_lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(percpu_ref_resurrect);
> +
> +/**
> + * percpu_ref_sum - return approximate ref counts
> + * @ref: perpcu_ref to sum
> + *
> + * Note that this should only really be used to compare refs, as by the
> + * very nature of percpu references, the value may be stale even before it
> + * has been returned.
> + */
> +long percpu_ref_sum(struct percpu_ref *ref)
> +{
> +	unsigned long __percpu *percpu_count;
> +	long ret;
> +
> +	rcu_read_lock();
> +	if (__ref_is_percpu(ref, &percpu_count)) {
> +		ret = atomic_long_read(&ref->data->count);
> +	} else {
> +		int cpu;
> +
> +		ret = 0;
> +		for_each_possible_cpu(cpu)
> +			ret += *per_cpu_ptr(percpu_count, cpu);
> +	}
> +	rcu_read_unlock();
> +
> +	return ret;
> +}
> 

-- 
Pavel Begunkov