2021-04-13 13:49:26

by Eric Dumazet

Subject: [PATCH 0/3] rseq: minor optimizations

From: Eric Dumazet <[email protected]>

rseq is a heavy user of copy to/from user data in fast paths.
This series tries to reduce the cost.

Eric Dumazet (3):
rseq: optimize rseq_update_cpu_id()
rseq: remove redundant access_ok()
rseq: optimise for 64bit arches

kernel/rseq.c | 26 ++++++++++++++++++++------
1 file changed, 20 insertions(+), 6 deletions(-)

--
2.31.1.295.g9ea45b61b8-goog

2021-04-13 14:03:34

by Eric Dumazet

Subject: [PATCH 1/3] rseq: optimize rseq_update_cpu_id()

From: Eric Dumazet <[email protected]>

Two put_user() in rseq_update_cpu_id() are replaced
by a pair of unsafe_put_user() with appropriate surroundings.

This removes one stac/clac pair on x86 in fast path.

Signed-off-by: Eric Dumazet <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Boqun Feng <[email protected]>
Cc: Arjun Roy <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
kernel/rseq.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/kernel/rseq.c b/kernel/rseq.c
index a4f86a9d6937cdfa2f13d1dcc9be863c1943d06f..d2689ccbb132c0fc8ec0924008771e5ee1ca855e 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -84,13 +84,20 @@
static int rseq_update_cpu_id(struct task_struct *t)
{
u32 cpu_id = raw_smp_processor_id();
+ struct rseq *r = t->rseq;

- if (put_user(cpu_id, &t->rseq->cpu_id_start))
- return -EFAULT;
- if (put_user(cpu_id, &t->rseq->cpu_id))
- return -EFAULT;
+ if (!user_write_access_begin(r, sizeof(*r)))
+ goto efault;
+ unsafe_put_user(cpu_id, &r->cpu_id_start, efault_end);
+ unsafe_put_user(cpu_id, &r->cpu_id, efault_end);
+ user_write_access_end();
trace_rseq_update(t);
return 0;
+
+efault_end:
+ user_write_access_end();
+efault:
+ return -EFAULT;
}

static int rseq_reset_rseq_cpu_id(struct task_struct *t)
--
2.31.1.295.g9ea45b61b8-goog

2021-04-13 20:36:30

by Mathieu Desnoyers

Subject: Re: [PATCH 1/3] rseq: optimize rseq_update_cpu_id()

----- On Apr 13, 2021, at 3:36 AM, Eric Dumazet [email protected] wrote:

> From: Eric Dumazet <[email protected]>
>
> Two put_user() in rseq_update_cpu_id() are replaced
> by a pair of unsafe_put_user() with appropriate surroundings.
>
> This removes one stac/clac pair on x86 in fast path.
>
> Signed-off-by: Eric Dumazet <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Cc: Arjun Roy <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> ---
> kernel/rseq.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index
> a4f86a9d6937cdfa2f13d1dcc9be863c1943d06f..d2689ccbb132c0fc8ec0924008771e5ee1ca855e
> 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -84,13 +84,20 @@
> static int rseq_update_cpu_id(struct task_struct *t)
> {
> u32 cpu_id = raw_smp_processor_id();
> + struct rseq *r = t->rseq;

AFAIU the variable above should be a struct rseq __user *.

Elsewhere in the file we use "rseq" rather than "r" for struct rseq __user *
variable name, it would be better to keep the naming consistent across the file
if possible.

Thanks,

Mathieu

>
> - if (put_user(cpu_id, &t->rseq->cpu_id_start))
> - return -EFAULT;
> - if (put_user(cpu_id, &t->rseq->cpu_id))
> - return -EFAULT;
> + if (!user_write_access_begin(r, sizeof(*r)))
> + goto efault;
> + unsafe_put_user(cpu_id, &r->cpu_id_start, efault_end);
> + unsafe_put_user(cpu_id, &r->cpu_id, efault_end);
> + user_write_access_end();
> trace_rseq_update(t);
> return 0;
> +
> +efault_end:
> + user_write_access_end();
> +efault:
> + return -EFAULT;
> }
>
> static int rseq_reset_rseq_cpu_id(struct task_struct *t)
> --
> 2.31.1.295.g9ea45b61b8-goog

--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

2021-04-13 21:04:47

by Eric Dumazet

Subject: Re: [PATCH 1/3] rseq: optimize rseq_update_cpu_id()

On Tue, Apr 13, 2021 at 4:29 PM Mathieu Desnoyers
<[email protected]> wrote:
>
> ----- On Apr 13, 2021, at 3:36 AM, Eric Dumazet [email protected] wrote:
>
> > From: Eric Dumazet <[email protected]>
> >
> > Two put_user() in rseq_update_cpu_id() are replaced
> > by a pair of unsafe_put_user() with appropriate surroundings.
> >
> > This removes one stac/clac pair on x86 in fast path.
> >
> > Signed-off-by: Eric Dumazet <[email protected]>
> > Cc: Mathieu Desnoyers <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: "Paul E. McKenney" <[email protected]>
> > Cc: Boqun Feng <[email protected]>
> > Cc: Arjun Roy <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > ---
> > kernel/rseq.c | 15 +++++++++++----
> > 1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/rseq.c b/kernel/rseq.c
> > index
> > a4f86a9d6937cdfa2f13d1dcc9be863c1943d06f..d2689ccbb132c0fc8ec0924008771e5ee1ca855e
> > 100644
> > --- a/kernel/rseq.c
> > +++ b/kernel/rseq.c
> > @@ -84,13 +84,20 @@
> > static int rseq_update_cpu_id(struct task_struct *t)
> > {
> > u32 cpu_id = raw_smp_processor_id();
> > + struct rseq *r = t->rseq;
>
> AFAIU the variable above should be a struct rseq __user *.
>
> Elsewhere in the file we use "rseq" rather than "r" for struct rseq __user *
> variable name, it would be better to keep the naming consistent across the file
> if possible.

Absolutely, thanks for the feedback.