optimise blk_try_enter_queue()

PATCH Superseded Pavel Begunkov 2 participants 5 messages Oct 20, 2021 – Oct 21, 2021

Subsystems (3)

Memory management Block layer Per-CPU memory allocator

lore

Share & tools

lore

Pavel Begunkov

author

2021-10-20 20:03 UTC #0 lore raw

cover optimise blk_try_enter_queue()

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(-)

Signature · 1 line

2.33.1

Pavel Begunkov

author

2021-10-20 20:03 UTC #1 lore raw

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

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

---
 include/linux/percpu-refcount.h | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

include/linux/percpu-refcount.h +21 ~~-10~~

index ae16a9856305..13e26add7d5c 100644
@@ -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;
 }

Signature · 1 line

2.33.1

Pavel Begunkov

author

2021-10-20 20:03 UTC #4 lore raw

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

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

---
 block/blk-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

block/blk-core.c +1 -1

index 88752e51d2b6..20e76aeb50f5 100644
@@ -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;
 
 	/*

Signature · 1 line

2.33.1

Tejun Heo

maintainer

2021-10-20 20:10 UTC #2 lore raw

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

In reply to Pavel Begunkov

Quoted 9 lines · Pavel Begunkov

include/linux/percpu-refcount.h +9 -0

+/**
+ * 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;

include/linux/percpu-refcount.h static inline bool percpu_ref_tryget(struct percpu_ref *ref) · line 287

Quoted 10 lines

include/linux/percpu-refcount.h +10 -0

+	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.

Signature · 1 line

tejun

Pavel Begunkov

author

2021-10-21 08:42 UTC #3 lore raw

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

Quoted 15 lines · Tejun Heo

Quoted 14 lines · Pavel Begunkov

include/linux/percpu-refcount.h +14 -0

+/**
+ * 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;

Quoted 8 lines

include/linux/percpu-refcount.h static inline bool percpu_ref_tryget(struct percpu_ref *ref) · line 287
Quoted 5 lines
include/linux/percpu-refcount.h +5 -0
+	} 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

Signature · 1 line

Pavel Begunkov