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

[Eric Dumazet]

On Tue, Apr 13, 2021 at 4:29 PM Mathieu Desnoyers
 wrote:
>
> - On Apr 13, 2021, at 3:36 AM, Eric Dumazet eric.duma...@gmail.com wrote:
>
> > From: Eric Dumazet 
> >
> > 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 
> > Cc: Mathieu Desnoyers 
> > Cc: Peter Zijlstra 
> > Cc: "Paul E. McKenney" 
> > Cc: Boqun Feng 
> > Cc: Arjun Roy 
> > Cc: Ingo Molnar 
> > ---
> > 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.

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

[Mathieu Desnoyers]

- On Apr 13, 2021, at 3:36 AM, Eric Dumazet eric.duma...@gmail.com wrote:

> From: Eric Dumazet 
> 
> 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 
> Cc: Mathieu Desnoyers 
> Cc: Peter Zijlstra 
> Cc: "Paul E. McKenney" 
> Cc: Boqun Feng 
> Cc: Arjun Roy 
> Cc: Ingo Molnar 
> ---
> 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, >rseq->cpu_id_start))
> - return -EFAULT;
> - if (put_user(cpu_id, >rseq->cpu_id))
> - return -EFAULT;
> + if (!user_write_access_begin(r, sizeof(*r)))
> + goto efault;
> + unsafe_put_user(cpu_id, >cpu_id_start, efault_end);
> + unsafe_put_user(cpu_id, >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

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

[Eric Dumazet]

From: Eric Dumazet 

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 
Cc: Mathieu Desnoyers 
Cc: Peter Zijlstra 
Cc: "Paul E. McKenney" 
Cc: Boqun Feng 
Cc: Arjun Roy 
Cc: Ingo Molnar 
---
 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, >rseq->cpu_id_start))
-   return -EFAULT;
-   if (put_user(cpu_id, >rseq->cpu_id))
-   return -EFAULT;
+   if (!user_write_access_begin(r, sizeof(*r)))
+   goto efault;
+   unsafe_put_user(cpu_id, >cpu_id_start, efault_end);
+   unsafe_put_user(cpu_id, >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