optimise blk_try_enter_queue()

PATCH Applied Pavel Begunkov 3 participants 7 messages Oct 21, 2021 – Oct 22, 2021

Subsystems (3)

Memory management Block layer Per-CPU memory allocator

lore

Share & tools

lore

Pavel Begunkov

author

2021-10-21 13:30 UTC #0 lore raw

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

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

Signature · 1 line

2.33.1

Pavel Begunkov

author

2021-10-21 13:31 UTC #1 lore raw

PATCH v2 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 | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

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

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

Signature · 1 line

2.33.1

Dennis Zhou

maintainer Acked-by on v2 1/2

2021-10-21 14:01 UTC #2 lore raw

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

Hello,

Quoted 32 lines · 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 <[email protected]>

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

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

index ae16a9856305..b31d3f3312ce 100644
@@ -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);

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

Quoted 10 lines

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

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

Quoted 25 lines

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

@@ -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;
 }

Signature · 1 line

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

Thanks,
Dennis

Pavel Begunkov

author

2021-10-22 09:22 UTC #3 lore raw

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

Quoted 12 lines · Dennis Zhou

Hello,
Quoted 9 lines · 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 <[email protected]>
---
  include/linux/percpu-refcount.h | 33 +++++++++++++++++++++++----------
  1 file changed, 23 insertions(+), 10 deletions(-)

[...]

Quoted 7 lines

Quoted 4 lines
+
/**
* 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.

Signature · 1 line

Pavel Begunkov

Dennis Zhou

maintainer

2021-10-22 19:22 UTC #4 lore raw

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

Quoted 19 lines · Pavel Begunkov

Quoted 12 lines · Dennis Zhou
Hello,
Quoted 9 lines · 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 <[email protected]>
---
  include/linux/percpu-refcount.h | 33 +++++++++++++++++++++++----------
  1 file changed, 23 insertions(+), 10 deletions(-)
[...]
Quoted 5 lines
Quoted 4 lines
+
/**
* percpu_ref_tryget_live - try to increment a live percpu refcount
* @ref: percpu_ref to try-get

Quoted 8 lines

Quoted 2 lines
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

Quoted 3 lines

Signature · 2 lines
-- 
Pavel Begunkov

Pavel Begunkov

author

2021-10-21 13:31 UTC #5 lore raw

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

Jens Axboe

maintainer Applied

2021-10-21 14:37 UTC #6 lore raw

In reply to Pavel Begunkov

Quoted 8 lines · 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
[...]

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,

Signature · 1 line

Jens Axboe