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
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(-)
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
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(-)
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
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 <[email protected]>
> ---
> 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 <[email protected]>
Thanks,
Dennis
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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> > > ---
> > > 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