[PATCH v2 0/2] optimise blk_try_enter_queue()

STALE1747d

Revision v2 of 2 in this series.

7 messages, 3 authors, 2021-10-22 · open the first message on its own page

[PATCH v2 0/2] optimise blk_try_enter_queue()

From: Pavel Begunkov <asml.silence@gmail.com>
Date: 2021-10-21 13:31:01

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

From: Pavel Begunkov <asml.silence@gmail.com>
Date: 2021-10-21 13:31:03

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

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

From: Pavel Begunkov <asml.silence@gmail.com>
Date: 2021-10-21 13:31:06

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 1/2] percpu_ref: percpu_ref_tryget_live() version holding RCU

From: Dennis Zhou <dennis@kernel.org>
Date: 2021-10-21 14:01:30

Hello,

On Thu, Oct 21, 2021 at 02:30:51PM +0100, Pavel Begunkov wrote:

quoted hunk

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.

quoted hunk

@@ -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 0/2] optimise blk_try_enter_queue()

From: Jens Axboe <axboe@kernel.dk>
Date: 2021-10-21 14:37:44

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

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

From: Pavel Begunkov <asml.silence@gmail.com>
Date: 2021-10-22 09:22:33

On 10/21/21 15:01, Dennis Zhou wrote:

Hello,

On Thu, Oct 21, 2021 at 02:30:51PM +0100, Pavel Begunkov wrote:

quoted

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

[...]

quoted

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

From: Dennis Zhou <dennis@kernel.org>
Date: 2021-10-22 19:22:54

On Fri, Oct 22, 2021 at 10:22:30AM +0100, Pavel Begunkov wrote:

On 10/21/21 15:01, Dennis Zhou wrote:

quoted

Hello,

On Thu, Oct 21, 2021 at 02:30:51PM +0100, Pavel Begunkov wrote:

quoted

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

[...]

quoted

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help