[PATCH v2 0/2] optimise blk_try_enter_queue()

[PATCH v2 0/2] optimise blk_try_enter_queue()

[Pavel Begunkov]

Kill extra rcu_read_lock/unlock() pair in blk_try_enter_queue().
Testing with io_uring (high batching) with nullblk:

Before:
3.20%  io_uring  [kernel.vmlinux]  [k] __rcu_read_unlock
3.05%  io_uring  [kernel.vmlinux]  [k] __rcu_read_lock

After:
2.52%  io_uring  [kernel.vmlinux]  [k] __rcu_read_unlock
2.28%  io_uring  [kernel.vmlinux]  [k] __rcu_read_lock

Doesn't necessarily translates into 1.4% perfofrmance improvement
but nice to have.

v2: rcu_read_lock_held() warning (Tejun)

Pavel Begunkov (2):
  percpu_ref: percpu_ref_tryget_live() version holding RCU
  block: kill extra rcu lock/unlock in queue enter

 block/blk-core.c                |  2 +-
 include/linux/percpu-refcount.h | 33 +++++++++++++++++++++++----------
 2 files changed, 24 insertions(+), 11 deletions(-)

-- 
2.33.1

[PATCH v2 1/2] percpu_ref: percpu_ref_tryget_live() version holding RCU

[Pavel Begunkov]

Add percpu_ref_tryget_live_rcu(), which is a version of
percpu_ref_tryget_live() but the user is responsible for enclosing it in
a RCU read lock section.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 include/linux/percpu-refcount.h | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/include/linux/percpu-refcount.h b/include/linux/percpu-refcount.h
index ae16a9856305..b31d3f3312ce 100644
--- a/include/linux/percpu-refcount.h
+++ b/include/linux/percpu-refcount.h
@@ -266,6 +266,28 @@ static inline bool percpu_ref_tryget(struct percpu_ref *ref)
 	return percpu_ref_tryget_many(ref, 1);
 }
 
+/**
+ * percpu_ref_tryget_live_rcu - same as percpu_ref_tryget_live() but the
+ * caller is responsible for taking RCU.
+ *
+ * This function is safe to call as long as @ref is between init and exit.
+ */
+static inline bool percpu_ref_tryget_live_rcu(struct percpu_ref *ref)
+{
+	unsigned long __percpu *percpu_count;
+	bool ret = false;
+
+	WARN_ON_ONCE(!rcu_read_lock_held());
+
+	if (likely(__ref_is_percpu(ref, &percpu_count))) {
+		this_cpu_inc(*percpu_count);
+		ret = true;
+	} else if (!(ref->percpu_count_ptr & __PERCPU_REF_DEAD)) {
+		ret = atomic_long_inc_not_zero(&ref->data->count);
+	}
+	return ret;
+}
+
 /**
  * percpu_ref_tryget_live - try to increment a live percpu refcount
  * @ref: percpu_ref to try-get
@@ -283,20 +305,11 @@ static inline bool percpu_ref_tryget(struct percpu_ref *ref)
  */
 static inline bool percpu_ref_tryget_live(struct percpu_ref *ref)
 {
-	unsigned long __percpu *percpu_count;
 	bool ret = false;
 
 	rcu_read_lock();
-
-	if (__ref_is_percpu(ref, &percpu_count)) {
-		this_cpu_inc(*percpu_count);
-		ret = true;
-	} else if (!(ref->percpu_count_ptr & __PERCPU_REF_DEAD)) {
-		ret = atomic_long_inc_not_zero(&ref->data->count);
-	}
-
+	ret = percpu_ref_tryget_live_rcu(ref);
 	rcu_read_unlock();
-
 	return ret;
 }
 
-- 
2.33.1

Re: [PATCH v2 1/2] percpu_ref: percpu_ref_tryget_live() version holding RCU

[Dennis Zhou]

Hello,

On Thu, Oct 21, 2021 at 02:30:51PM +0100, Pavel Begunkov wrote:
> Add percpu_ref_tryget_live_rcu(), which is a version of
> percpu_ref_tryget_live() but the user is responsible for enclosing it in
> a RCU read lock section.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  include/linux/percpu-refcount.h | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/percpu-refcount.h b/include/linux/percpu-refcount.h
> index ae16a9856305..b31d3f3312ce 100644
> --- a/include/linux/percpu-refcount.h
> +++ b/include/linux/percpu-refcount.h
> @@ -266,6 +266,28 @@ static inline bool percpu_ref_tryget(struct percpu_ref *ref)
>  	return percpu_ref_tryget_many(ref, 1);
>  }
>  
> +/**
> + * percpu_ref_tryget_live_rcu - same as percpu_ref_tryget_live() but the
> + * caller is responsible for taking RCU.
> + *
> + * This function is safe to call as long as @ref is between init and exit.
> + */
> +static inline bool percpu_ref_tryget_live_rcu(struct percpu_ref *ref)
> +{
> +	unsigned long __percpu *percpu_count;
> +	bool ret = false;
> +
> +	WARN_ON_ONCE(!rcu_read_lock_held());
> +
> +	if (likely(__ref_is_percpu(ref, &percpu_count))) {
> +		this_cpu_inc(*percpu_count);
> +		ret = true;
> +	} else if (!(ref->percpu_count_ptr & __PERCPU_REF_DEAD)) {
> +		ret = atomic_long_inc_not_zero(&ref->data->count);
> +	}
> +	return ret;
> +}
> +
>  /**
>   * percpu_ref_tryget_live - try to increment a live percpu refcount
>   * @ref: percpu_ref to try-get

Nit: it's dumb convention at this point, but do you mind copying this
guy up. I like consistency.

> @@ -283,20 +305,11 @@ static inline bool percpu_ref_tryget(struct percpu_ref *ref)
>   */
>  static inline bool percpu_ref_tryget_live(struct percpu_ref *ref)
>  {
> -	unsigned long __percpu *percpu_count;
>  	bool ret = false;
>  
>  	rcu_read_lock();
> -
> -	if (__ref_is_percpu(ref, &percpu_count)) {
> -		this_cpu_inc(*percpu_count);
> -		ret = true;
> -	} else if (!(ref->percpu_count_ptr & __PERCPU_REF_DEAD)) {
> -		ret = atomic_long_inc_not_zero(&ref->data->count);
> -	}
> -
> +	ret = percpu_ref_tryget_live_rcu(ref);
>  	rcu_read_unlock();
> -
>  	return ret;
>  }
>  
> -- 
> 2.33.1
> 

Currently I'm not carrying anything and I don't expect any percpu_ref
work to come in. Jens, feel free to pick this up.

Acked-by: Dennis Zhou <dennis@kernel.org>

Thanks,
Dennis

Re: [PATCH v2 1/2] percpu_ref: percpu_ref_tryget_live() version holding RCU

[Pavel Begunkov]

On 10/21/21 15:01, Dennis Zhou wrote:
> Hello,
> 
> On Thu, Oct 21, 2021 at 02:30:51PM +0100, Pavel Begunkov wrote:
>> Add percpu_ref_tryget_live_rcu(), which is a version of
>> percpu_ref_tryget_live() but the user is responsible for enclosing it in
>> a RCU read lock section.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>   include/linux/percpu-refcount.h | 33 +++++++++++++++++++++++----------
>>   1 file changed, 23 insertions(+), 10 deletions(-)
>>
[...]
>> +
>>   /**
>>    * percpu_ref_tryget_live - try to increment a live percpu refcount
>>    * @ref: percpu_ref to try-get
> 
> Nit: it's dumb convention at this point, but do you mind copying this
> guy up. I like consistency.

Looks Jens already took it. If you still want it moved, do you mind
it in a separate patch?

And I'm not sure I follow where you want it to be, currently it's
right before percpu_ref_tryget_live, which uses it.

-- 
Pavel Begunkov

Re: [PATCH v2 1/2] percpu_ref: percpu_ref_tryget_live() version holding RCU

[Dennis Zhou]

On Fri, Oct 22, 2021 at 10:22:30AM +0100, Pavel Begunkov wrote:
> On 10/21/21 15:01, Dennis Zhou wrote:
> > Hello,
> > 
> > On Thu, Oct 21, 2021 at 02:30:51PM +0100, Pavel Begunkov wrote:
> > > Add percpu_ref_tryget_live_rcu(), which is a version of
> > > percpu_ref_tryget_live() but the user is responsible for enclosing it in
> > > a RCU read lock section.
> > > 
> > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> > > ---
> > >   include/linux/percpu-refcount.h | 33 +++++++++++++++++++++++----------
> > >   1 file changed, 23 insertions(+), 10 deletions(-)
> > > 
> [...]
> > > +
> > >   /**
> > >    * percpu_ref_tryget_live - try to increment a live percpu refcount
> > >    * @ref: percpu_ref to try-get
> > 
> > Nit: it's dumb convention at this point, but do you mind copying this
> > guy up. I like consistency.
> 
> Looks Jens already took it. If you still want it moved, do you mind
> it in a separate patch?
> 
> And I'm not sure I follow where you want it to be, currently it's
> right before percpu_ref_tryget_live, which uses it.

Don't worry about it. I meant the @ref comment line. Honestly it's not
really useful, it's just every other header block has that convention
among most of percpu related files.

If I have to make any changes in the future, I'll clean it up then.

Thanks,
Dennis

> 
> -- 
> Pavel Begunkov

[PATCH v2 2/2] block: kill extra rcu lock/unlock in queue enter

[Pavel Begunkov]

blk_try_enter_queue() already takes rcu_read_lock/unlock, so we can
avoid the second pair in percpu_ref_tryget_live(), use a newly added
percpu_ref_tryget_live_rcu().

As rcu_read_lock/unlock imply barrier()s, it's pretty noticeable,
especially for for !CONFIG_PREEMPT_RCU (default for some distributions),
where __rcu_read_lock/unlock() are not inlined.

3.20%  io_uring  [kernel.vmlinux]  [k] __rcu_read_unlock
3.05%  io_uring  [kernel.vmlinux]  [k] __rcu_read_lock

2.52%  io_uring  [kernel.vmlinux]  [k] __rcu_read_unlock
2.28%  io_uring  [kernel.vmlinux]  [k] __rcu_read_lock

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 block/blk-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 88752e51d2b6..20e76aeb50f5 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -389,7 +389,7 @@ EXPORT_SYMBOL(blk_cleanup_queue);
 static bool blk_try_enter_queue(struct request_queue *q, bool pm)
 {
 	rcu_read_lock();
-	if (!percpu_ref_tryget_live(&q->q_usage_counter))
+	if (!percpu_ref_tryget_live_rcu(&q->q_usage_counter))
 		goto fail;
 
 	/*
-- 
2.33.1

Re: [PATCH v2 0/2] optimise blk_try_enter_queue()

[Jens Axboe]

On Thu, 21 Oct 2021 14:30:50 +0100, Pavel Begunkov wrote:
> Kill extra rcu_read_lock/unlock() pair in blk_try_enter_queue().
> Testing with io_uring (high batching) with nullblk:
> 
> Before:
> 3.20%  io_uring  [kernel.vmlinux]  [k] __rcu_read_unlock
> 3.05%  io_uring  [kernel.vmlinux]  [k] __rcu_read_lock
> 
> [...]

Applied, thanks!

[1/2] percpu_ref: percpu_ref_tryget_live() version holding RCU
      commit: 3b13c168186c115501ee7d194460ba2f8c825155
[2/2] block: kill extra rcu lock/unlock in queue enter
      commit: e94f68527a35271131cdf9d3fb4eb3c2513dc3d0

Best regards,
-- 
Jens Axboe