[PATCH 0/2] optimise blk_try_enter_queue()

[PATCH 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.

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 | 31 +++++++++++++++++++++----------
 2 files changed, 22 insertions(+), 11 deletions(-)

-- 
2.33.1

[PATCH 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 | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/include/linux/percpu-refcount.h b/include/linux/percpu-refcount.h
index ae16a9856305..13e26add7d5c 100644
--- a/include/linux/percpu-refcount.h
+++ b/include/linux/percpu-refcount.h
@@ -266,6 +266,26 @@ 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;
+
+	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 +303,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 1/2] percpu_ref: percpu_ref_tryget_live() version holding RCU

[Tejun Heo]

On Wed, Oct 20, 2021 at 09:03:18PM +0100, Pavel Begunkov wrote:
> +/**
> + * 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;
> +
> +	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;
> +}

Can we please add rcu_read_lock_held() assertion? Other than that, looks
fine to me.

Thanks.

-- 
tejun

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

[Pavel Begunkov]

On 10/20/21 21:10, Tejun Heo wrote:
> On Wed, Oct 20, 2021 at 09:03:18PM +0100, Pavel Begunkov wrote:
>> +/**
>> + * 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;
>> +
>> +	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;
>> +}
> 
> Can we please add rcu_read_lock_held() assertion? Other than that, looks
> fine to me.

Will add, thanks

-- 
Pavel Begunkov

[PATCH 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