Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1227025 > unrolled thread

[PATCH] kernel: fix data race in put_pid

Back to article view | Back to linux.kernel

Contents

  [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-17 15:30 +0200
    Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-17 18:20 +0200
      Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-17 18:50 +0200
        Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-17 19:50 +0200
          Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-17 20:00 +0200
            Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-17 20:10 +0200
            Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-17 20:20 +0200
              Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-17 20:40 +0200
              Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 11:00 +0200
                Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 11:10 +0200
                  Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 11:40 +0200
                    Re: [PATCH] kernel: fix data race in put_pid James Hogan <james.hogan@imgtec.com> - 2015-09-18 14:40 +0200
                      Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 14:50 +0200
                  Re: [PATCH] kernel: fix data race in put_pid "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-18 18:40 +0200
                Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-18 11:10 +0200
                  Re: [PATCH] kernel: fix data race in put_pid Will Deacon <will.deacon@arm.com> - 2015-09-18 11:30 +0200
                    Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 11:40 +0200
                    Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 13:30 +0200
                      Re: [PATCH] kernel: fix data race in put_pid Will Deacon <will.deacon@arm.com> - 2015-09-18 13:40 +0200
                      Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-18 14:00 +0200
                        Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 14:10 +0200
                          Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 14:30 +0200
                            Re: [PATCH] kernel: fix data race in put_pid Will Deacon <will.deacon@arm.com> - 2015-09-18 14:50 +0200
                              Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 15:20 +0200
                          Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-18 15:50 +0200
                            Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 16:00 +0200
                            Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-18 16:00 +0200
                              Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-18 16:50 +0200
                          Re: [PATCH] kernel: fix data race in put_pid Dmitry Vyukov <dvyukov@google.com> - 2015-09-22 10:40 +0200
                      Re: [PATCH] kernel: fix data race in put_pid Eric Dumazet <eric.dumazet@gmail.com> - 2015-09-18 18:20 +0200
                        Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 18:30 +0200
                    Re: [PATCH] kernel: fix data race in put_pid "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-18 18:00 +0200
                Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-18 15:40 +0200
                Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-18 15:40 +0200
                  Re: [PATCH] kernel: fix data race in put_pid Peter Zijlstra <peterz@infradead.org> - 2015-09-18 16:00 +0200
                    Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-18 17:10 +0200
                    Re: [PATCH] kernel: fix data race in put_pid Oleg Nesterov <oleg@redhat.com> - 2015-09-18 17:40 +0200
                    Re: [PATCH] kernel: fix data race in put_pid "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-18 18:30 +0200
        Re: [PATCH] kernel: fix data race in put_pid "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-09-17 20:00 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1227800

On Fri, Sep 18, 2015 at 01:50:01PM +0200, Dmitry Vyukov wrote:
> > +#ifndef atomic_read_ctrl
> > +static inline int atomic_read_ctrl(atomic_t *v)
> > +{
> > +       int val = atomic_read(v);
> > +       smp_read_barrier_depends(); /* Enforce control dependency. */
> > +       return val;
> > +}
> > +#endif
> > +
> >  /*
> >   * Relaxed variants of xchg, cmpxchg and some atomic operations.
> >   *
> 
> Looks good to me.
> Should we add atomic64_read_ctrl for completeness? I have not seen
> cases where it was needed, though.

Sure, and while doing another spin, let me go update the documentation
too.

---
Subject: atomic: Implement atomic_read_ctrl()
From: Peter Zijlstra <peterz@infradead.org>
Date: Fri, 18 Sep 2015 13:22:52 +0200

Provide atomic_read_ctrl() to mirror READ_ONCE_CTRL(), such that we can
more conveniently use atomics in control dependencies.

Since we can assume atomic_read() implies a READ_ONCE(), we must only
emit an extra smp_read_barrier_depends() in order to upgrade to
READ_ONCE_CTRL() semantics.

Cc: oleg@redhat.com
Cc: torvalds@linux-foundation.org
Cc: will.deacon@arm.com
Cc: paulmck@linux.vnet.ibm.com
Requested-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 Documentation/memory-barriers.txt |   17 +++++++++--------
 include/linux/atomic.h            |   18 ++++++++++++++++++
 2 files changed, 27 insertions(+), 8 deletions(-)

--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -637,7 +637,8 @@ to optimize the original example by elim
 	b = p;  /* BUG: Compiler and CPU can both reorder!!! */
 
 Finally, the READ_ONCE_CTRL() includes an smp_read_barrier_depends()
-that DEC Alpha needs in order to respect control depedencies.
+that DEC Alpha needs in order to respect control depedencies. Alternatively
+use one of atomic{,64}_read_ctrl().
 
 So don't leave out the READ_ONCE_CTRL().
 
@@ -796,9 +797,9 @@ site: https://www.cl.cam.ac.uk/~pes20/pp
 
 In summary:
 
-  (*) Control dependencies must be headed by READ_ONCE_CTRL().
-      Or, as a much less preferable alternative, interpose
-      smp_read_barrier_depends() between a READ_ONCE() and the
+  (*) Control dependencies must be headed by READ_ONCE_CTRL(),
+      atomic{,64}_read_ctrl(). Or, as a much less preferable alternative,
+      interpose smp_read_barrier_depends() between a READ_ONCE() and the
       control-dependent write.
 
   (*) Control dependencies can order prior loads against later stores.
@@ -820,10 +821,10 @@ site: https://www.cl.cam.ac.uk/~pes20/pp
       and WRITE_ONCE() can help to preserve the needed conditional.
 
   (*) Control dependencies require that the compiler avoid reordering the
-      dependency into nonexistence.  Careful use of READ_ONCE_CTRL()
-      or smp_read_barrier_depends() can help to preserve your control
-      dependency.  Please see the Compiler Barrier section for more
-      information.
+      dependency into nonexistence.  Careful use of READ_ONCE_CTRL(),
+      atomic{,64}_read_ctrl() or smp_read_barrier_depends() can help to
+      preserve your control dependency.  Please see the Compiler Barrier
+      section for more information.
 
   (*) Control dependencies pair normally with other types of barriers.
 
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -4,6 +4,24 @@
 #include <asm/atomic.h>
 #include <asm/barrier.h>
 
+#ifndef atomic_read_ctrl
+static inline int atomic_read_ctrl(atomic_t *v)
+{
+	int val = atomic_read(v);
+	smp_read_barrier_depends(); /* Enforce control dependency. */
+	return val;
+}
+#endif
+
+#ifndef atomic64_read_ctrl
+static inline int atomic64_read_ctrl(atomic64_t *v)
+{
+	int val = atomic64_read(v);
+	smp_read_barrier_depends(); /* Enforce control dependency. */
+	return val;
+}
+#endif
+
 /*
  * Relaxed variants of xchg, cmpxchg and some atomic operations.
  *
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227805

On Fri, Sep 18, 2015 at 01:56:37PM +0200, Peter Zijlstra wrote:
> +#ifndef atomic64_read_ctrl
> +static inline int atomic64_read_ctrl(atomic64_t *v)
> +{
> +	int val = atomic64_read(v);

Duh

	long long...

> +	smp_read_barrier_depends(); /* Enforce control dependency. */
> +	return val;
> +}
> +#endif
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227831

On Fri, Sep 18, 2015 at 01:19:20PM +0100, Peter Zijlstra wrote:
> On Fri, Sep 18, 2015 at 01:56:37PM +0200, Peter Zijlstra wrote:
> > +#ifndef atomic64_read_ctrl
> > +static inline int atomic64_read_ctrl(atomic64_t *v)
> > +{
> > +	int val = atomic64_read(v);
> 
> Duh
> 
> 	long long...

...and atomic_long_read_ctrl, too?

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227850

On Fri, Sep 18, 2015 at 01:44:42PM +0100, Will Deacon wrote:
> On Fri, Sep 18, 2015 at 01:19:20PM +0100, Peter Zijlstra wrote:
> > On Fri, Sep 18, 2015 at 01:56:37PM +0200, Peter Zijlstra wrote:
> > > +#ifndef atomic64_read_ctrl
> > > +static inline int atomic64_read_ctrl(atomic64_t *v)
> > > +{
> > > +	int val = atomic64_read(v);
> > 
> > Duh
> > 
> > 	long long...
> 
> ...and atomic_long_read_ctrl, too?

+ATOMIC_LONG_READ_OP(_ctrl)

done.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227876

On 09/18, Peter Zijlstra wrote:
>
> Provide atomic_read_ctrl() to mirror READ_ONCE_CTRL(), such that we can
> more conveniently use atomics in control dependencies.
>
> Since we can assume atomic_read() implies a READ_ONCE(), we must only
> emit an extra smp_read_barrier_depends() in order to upgrade to
> READ_ONCE_CTRL() semantics.

...

> +static inline int atomic_read_ctrl(atomic_t *v)
> +{
> +	int val = atomic_read(v);
> +	smp_read_barrier_depends(); /* Enforce control dependency. */
> +	return val;
> +}

Help. I am starting to think that the control dependencies is even more
hard to understand that memory barriers...

So I assume that if we have

	int X = 0;
	atomic_t Y = ATOMIC_INIT(0);

	void w(void)
	{
		X = 1;
		atomic_inc_return(&Y);
	}

then

	void r(void)
	{
		if (atomic_read_ctrl(&Y))
			BUG_ON(X == 0);
	}

should be correct?  Why?

If not then I am even more confused.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227879

On Fri, Sep 18, 2015 at 03:44:53PM +0200, Oleg Nesterov wrote:
> On 09/18, Peter Zijlstra wrote:
> > +static inline int atomic_read_ctrl(atomic_t *v)
> > +{
> > +	int val = atomic_read(v);
> > +	smp_read_barrier_depends(); /* Enforce control dependency. */
> > +	return val;
> > +}
> 
> Help. I am starting to think that the control dependencies is even more
> hard to understand that memory barriers...

Hehe, think of then as a load-store barrier; due to the 'impossibility'
of speculative stores (we'd see all kinds of random crap if you could
speculate stores).

> So I assume that if we have
> 
> 	int X = 0;
> 	atomic_t Y = ATOMIC_INIT(0);
> 
> 	void w(void)
> 	{
> 		X = 1;
> 		atomic_inc_return(&Y);
> 	}
> 
> then
> 
> 	void r(void)
> 	{
> 		if (atomic_read_ctrl(&Y))
> 			BUG_ON(X == 0);
> 	}
> 
> should be correct?  Why?

Nope, because its (again) a load-load order you have there.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227880

On Fri, Sep 18, 2015 at 3:44 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 09/18, Peter Zijlstra wrote:
>>
>> Provide atomic_read_ctrl() to mirror READ_ONCE_CTRL(), such that we can
>> more conveniently use atomics in control dependencies.
>>
>> Since we can assume atomic_read() implies a READ_ONCE(), we must only
>> emit an extra smp_read_barrier_depends() in order to upgrade to
>> READ_ONCE_CTRL() semantics.
>
> ...
>
>> +static inline int atomic_read_ctrl(atomic_t *v)
>> +{
>> +     int val = atomic_read(v);
>> +     smp_read_barrier_depends(); /* Enforce control dependency. */
>> +     return val;
>> +}
>
> Help. I am starting to think that the control dependencies is even more
> hard to understand that memory barriers...
>
> So I assume that if we have
>
>         int X = 0;
>         atomic_t Y = ATOMIC_INIT(0);
>
>         void w(void)
>         {
>                 X = 1;
>                 atomic_inc_return(&Y);
>         }
>
> then
>
>         void r(void)
>         {
>                 if (atomic_read_ctrl(&Y))
>                         BUG_ON(X == 0);
>         }
>
> should be correct?  Why?
>
> If not then I am even more confused.

This not correct, because "ctrl" barrier affects only
control-dependent stores. For reads processor still can speculate,
that is, speculatively load X before loading Y. Control-dependent
require full read/acquire memory barrier.
What will work is:

// thread 1
                 X = 1;
                 atomic_inc_return(&Y);

// thread 2
                 if (atomic_read_ctrl(&Y)) {
                         X = 2;
                         BUG_ON(X == 2);
                  }

Without the "ctrl" barrier store X=2 could hoist above load of Y (on
Alpha), and then X=1 can happen _after_ X=2, and then BUG_ON could
fail.
With the "ctrl" barrier store X=2 is not allowed to hoist above load of Y.


-- 
Dmitry Vyukov, Software Engineer, dvyukov@google.com
Google Germany GmbH, Dienerstraße 12, 80331, München
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
sind, leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227945

On 09/18, Dmitry Vyukov wrote:
>
> On Fri, Sep 18, 2015 at 3:44 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > So I assume that if we have
> >
> >         int X = 0;
> >         atomic_t Y = ATOMIC_INIT(0);
> >
> >         void w(void)
> >         {
> >                 X = 1;
> >                 atomic_inc_return(&Y);
> >         }
> >
> > then
> >
> >         void r(void)
> >         {
> >                 if (atomic_read_ctrl(&Y))
> >                         BUG_ON(X == 0);
> >         }
> >
> > should be correct?  Why?
> >
> > If not then I am even more confused.
>
> This not correct,

Good. because I wasn't able to understand why this could work.

> // thread 1
>                  X = 1;
>                  atomic_inc_return(&Y);
>
> // thread 2
>                  if (atomic_read_ctrl(&Y)) {
>                          X = 2;
>                          BUG_ON(X == 2);
>                   }

Thanks. This makes perfect sense to me.

And then I agree, atomic_read_ctrl() in put_pid() should fix the
theoretical problem.

Perhaps we can add this example to memory-barriers.txt... Although
perhaps it already explains/documents this case. I am afraid to open
it, it is huge and changes too often so every time it looks like a
new document to me ;)

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229985

Should I wait until atomic_read_ctrl patch is landed and update my patch?


On Fri, Sep 18, 2015 at 1:56 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Sep 18, 2015 at 01:50:01PM +0200, Dmitry Vyukov wrote:
>> > +#ifndef atomic_read_ctrl
>> > +static inline int atomic_read_ctrl(atomic_t *v)
>> > +{
>> > +       int val = atomic_read(v);
>> > +       smp_read_barrier_depends(); /* Enforce control dependency. */
>> > +       return val;
>> > +}
>> > +#endif
>> > +
>> >  /*
>> >   * Relaxed variants of xchg, cmpxchg and some atomic operations.
>> >   *
>>
>> Looks good to me.
>> Should we add atomic64_read_ctrl for completeness? I have not seen
>> cases where it was needed, though.
>
> Sure, and while doing another spin, let me go update the documentation
> too.
>
> ---
> Subject: atomic: Implement atomic_read_ctrl()
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Fri, 18 Sep 2015 13:22:52 +0200
>
> Provide atomic_read_ctrl() to mirror READ_ONCE_CTRL(), such that we can
> more conveniently use atomics in control dependencies.
>
> Since we can assume atomic_read() implies a READ_ONCE(), we must only
> emit an extra smp_read_barrier_depends() in order to upgrade to
> READ_ONCE_CTRL() semantics.
>
> Cc: oleg@redhat.com
> Cc: torvalds@linux-foundation.org
> Cc: will.deacon@arm.com
> Cc: paulmck@linux.vnet.ibm.com
> Requested-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  Documentation/memory-barriers.txt |   17 +++++++++--------
>  include/linux/atomic.h            |   18 ++++++++++++++++++
>  2 files changed, 27 insertions(+), 8 deletions(-)
>
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -637,7 +637,8 @@ to optimize the original example by elim
>         b = p;  /* BUG: Compiler and CPU can both reorder!!! */
>
>  Finally, the READ_ONCE_CTRL() includes an smp_read_barrier_depends()
> -that DEC Alpha needs in order to respect control depedencies.
> +that DEC Alpha needs in order to respect control depedencies. Alternatively
> +use one of atomic{,64}_read_ctrl().
>
>  So don't leave out the READ_ONCE_CTRL().
>
> @@ -796,9 +797,9 @@ site: https://www.cl.cam.ac.uk/~pes20/pp
>
>  In summary:
>
> -  (*) Control dependencies must be headed by READ_ONCE_CTRL().
> -      Or, as a much less preferable alternative, interpose
> -      smp_read_barrier_depends() between a READ_ONCE() and the
> +  (*) Control dependencies must be headed by READ_ONCE_CTRL(),
> +      atomic{,64}_read_ctrl(). Or, as a much less preferable alternative,
> +      interpose smp_read_barrier_depends() between a READ_ONCE() and the
>        control-dependent write.
>
>    (*) Control dependencies can order prior loads against later stores.
> @@ -820,10 +821,10 @@ site: https://www.cl.cam.ac.uk/~pes20/pp
>        and WRITE_ONCE() can help to preserve the needed conditional.
>
>    (*) Control dependencies require that the compiler avoid reordering the
> -      dependency into nonexistence.  Careful use of READ_ONCE_CTRL()
> -      or smp_read_barrier_depends() can help to preserve your control
> -      dependency.  Please see the Compiler Barrier section for more
> -      information.
> +      dependency into nonexistence.  Careful use of READ_ONCE_CTRL(),
> +      atomic{,64}_read_ctrl() or smp_read_barrier_depends() can help to
> +      preserve your control dependency.  Please see the Compiler Barrier
> +      section for more information.
>
>    (*) Control dependencies pair normally with other types of barriers.
>
> --- a/include/linux/atomic.h
> +++ b/include/linux/atomic.h
> @@ -4,6 +4,24 @@
>  #include <asm/atomic.h>
>  #include <asm/barrier.h>
>
> +#ifndef atomic_read_ctrl
> +static inline int atomic_read_ctrl(atomic_t *v)
> +{
> +       int val = atomic_read(v);
> +       smp_read_barrier_depends(); /* Enforce control dependency. */
> +       return val;
> +}
> +#endif
> +
> +#ifndef atomic64_read_ctrl
> +static inline int atomic64_read_ctrl(atomic64_t *v)
> +{
> +       int val = atomic64_read(v);
> +       smp_read_barrier_depends(); /* Enforce control dependency. */
> +       return val;
> +}
> +#endif
> +
>  /*
>   * Relaxed variants of xchg, cmpxchg and some atomic operations.
>   *



-- 
Dmitry Vyukov, Software Engineer, dvyukov@google.com
Google Germany GmbH, Dienerstraße 12, 80331, München
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
sind, leiten Sie diese bitte nicht weiter, informieren Sie den
Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please
do not forward it, please inform the sender, and please erase this
e-mail including any attachments. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1228124

On Fri, 2015-09-18 at 13:22 +0200, Peter Zijlstra wrote:

...

>  
> +#ifndef atomic_read_ctrl
> +static inline int atomic_read_ctrl(atomic_t *v)

const atomic_t *v

> +{
> +	int val = atomic_read(v);
> +	smp_read_barrier_depends(); /* Enforce control dependency. */
> +	return val;
> +}
> +#endif



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1228135

On Fri, Sep 18, 2015 at 09:15:34AM -0700, Eric Dumazet wrote:
> On Fri, 2015-09-18 at 13:22 +0200, Peter Zijlstra wrote:

> > +static inline int atomic_read_ctrl(atomic_t *v)
> 
> const atomic_t *v

Good point, lemme also fix up ATOMIC_LONG_READ_OP().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1228097

On Fri, Sep 18, 2015 at 10:28:20AM +0100, Will Deacon wrote:
> On Fri, Sep 18, 2015 at 10:06:46AM +0100, Dmitry Vyukov wrote:
> > On Fri, Sep 18, 2015 at 10:51 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > As an alternative patch, could we not do:
> > >
> > >   void put_pid(struct pid *pid)
> > >   {
> > >         struct pid_namespace *ns;
> > >
> > >         if (!pid)
> > >                 return;
> > >
> > >         ns = pid->numbers[pid->level].ns;
> > >         if ((atomic_read(&pid->count) == 1) ||
> > >              atomic_dec_and_test(&pid->count)) {
> > >
> > > +               smp_read_barrier_depends(); /* ctrl-dep */
> > >
> > >                 kmem_cache_free(ns->pid_cachep, pid);
> > >                 put_pid_ns(ns);
> > >         }
> > >   }
> > >
> > > That would upgrade the atomic_read() path to a full READ_ONCE_CTRL(),
> > > and thereby avoid any of the kmem_cache_free() stores from leaking out.
> > > And its free, except on Alpha. Whereas the atomic_read_acquire() will
> > > generate a full memory barrier on whole bunch of archs.
> > 
> > 
> > What you propose makes sense.
> > 
> > +Will, Paul
> > 
> > Can we have something along the lines of:
> > 
> > #define atomic_read_ctrl(v) READ_ONCE_CTRL(&(v)->counter)
> 
> Funnily enough, I had this exact same discussion off-list yesterday
> afternoon, since I wrote some code relying on a ctrl dependency from
> an atomic_read to an atomic_xchg_relaxed.
> 
> So I guess I'm for the addition, but at the same time, could we make
> atomic_read and atomic_set generic too?

If we have some places that could use it, by all means let's add it!

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227867

Damn, sorry for noise,

On 09/18, Oleg Nesterov wrote:
>
> Now. In theory this this code is wrong:
>
> 	if (Y) {
> 		BUG_ON(X == 0);
> 	}

Of course without READ_ONCE() or barrier() in between this code
is buggy in any case. But I hope you understand what I tried to
say...

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227870

On 09/18, Peter Zijlstra wrote:
>
> On Thu, Sep 17, 2015 at 08:09:19PM +0200, Oleg Nesterov wrote:
>
> > I need to recheck, but afaics this is not possible. This optimization
> > is fine, but probably needs a comment.
>
> For sure, this code doesn't make any sense to me.

So yes, after a sleep I am starting to agree that in theory this fast-path
check is wrong. I'll write another email..

> As an alternative patch, could we not do:
>
>   void put_pid(struct pid *pid)
>   {
> 	struct pid_namespace *ns;
>
> 	if (!pid)
> 		return;
>
> 	ns = pid->numbers[pid->level].ns;
> 	if ((atomic_read(&pid->count) == 1) ||
> 	     atomic_dec_and_test(&pid->count)) {
>
> +		smp_read_barrier_depends(); /* ctrl-dep */

Not sure... Firstly it is not clear what this barrier pairs with. And I
have to admit that I can not understand if _CTRL() logic applies here.
The same for atomic_read_ctrl().

OK, please forget about put_pid() for the moment. Suppose we have

	X = 1;
	synchronize_sched();
	Y = 1;

Or
	X = 1;
	call_rcu_sched( func => { Y = 1; } );



Now. In theory this this code is wrong:

	if (Y) {
		BUG_ON(X == 0);
	}

But this is correct:

	if (Y) {
		rcu_read_lock_sched();
		rcu_read_unlock_sched();
		BUG_ON(X == 0);
	}

So perhaps something like this

	/*
	 * Comment to explain it is eq to read_lock + read_unlock,
	 * in a sense that this guarantees a full barrier wrt to
	 * the previous synchronize_sched().
	 */
	#define rcu_read_barrier_sched()	barrier()

make sense?


And again, I simply can't understand if this code

	if (READ_ONCE_CTRL(Y))
		BUG_ON(X == 0);

to me it does _not_ look correct in theory.
		
Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227893

On Fri, Sep 18, 2015 at 03:28:44PM +0200, Oleg Nesterov wrote:
> On 09/18, Peter Zijlstra wrote:
> >
> > On Thu, Sep 17, 2015 at 08:09:19PM +0200, Oleg Nesterov wrote:
> >
> > > I need to recheck, but afaics this is not possible. This optimization
> > > is fine, but probably needs a comment.
> >
> > For sure, this code doesn't make any sense to me.
> 
> So yes, after a sleep I am starting to agree that in theory this fast-path
> check is wrong. I'll write another email..

This other mail will include a patch adding comments to pid.c ? That
code didn't want to make sense to me this morning.

> > As an alternative patch, could we not do:
> >
> >   void put_pid(struct pid *pid)
> >   {
> > 	struct pid_namespace *ns;
> >
> > 	if (!pid)
> > 		return;
> >
> > 	ns = pid->numbers[pid->level].ns;
> > 	if ((atomic_read(&pid->count) == 1) ||
> > 	     atomic_dec_and_test(&pid->count)) {
> >
> > +		smp_read_barrier_depends(); /* ctrl-dep */
> 
> Not sure... Firstly it is not clear what this barrier pairs with. And I
> have to admit that I can not understand if _CTRL() logic applies here.
> The same for atomic_read_ctrl().

The control dependency barrier pairs with the full barrier of
atomic_dec_and_test.

So the two put_pid() instances:

	CPU0					CPU1

	pid->foo = 1;
	atomic_dec_and_test() == false		atomic_read_ctrl() == 1
						kmem_cache_free(pid)

CPU0 will modify a pid field and decrement, but not reach 0.
CPU1 finds we're the last, but must also be able to observe our foo
store such that we can rest assured  it is complete before we free the
storage.

The freeing of pid, on CPU1, is stores, these must not happen before we
satisfy the freeing condition, iow a load-store barrier, which is what
the control dependency provides.

> OK, please forget about put_pid() for the moment. Suppose we have
> 
> 	X = 1;
> 	synchronize_sched();
> 	Y = 1;
> 
> Or
> 	X = 1;
> 	call_rcu_sched( func => { Y = 1; } );
> 
> 
> 
> Now. In theory this this code is wrong:
> 
> 	if (Y) {
> 		BUG_ON(X == 0);
> 	}
> 
> But this is correct:
> 
> 	if (Y) {
> 		rcu_read_lock_sched();
> 		rcu_read_unlock_sched();
> 		BUG_ON(X == 0);
> 	}
> 
> So perhaps something like this
> 
> 	/*
> 	 * Comment to explain it is eq to read_lock + read_unlock,
> 	 * in a sense that this guarantees a full barrier wrt to
> 	 * the previous synchronize_sched().
> 	 */
> 	#define rcu_read_barrier_sched()	barrier()
> 
> make sense?
> 
> 
> And again, I simply can't understand if this code
> 
> 	if (READ_ONCE_CTRL(Y))
> 		BUG_ON(X == 0);
> 
> to me it does _not_ look correct in theory.

So control dependencies provide a load-store barrier. Your examples
above rely on a load-load barrier; BUG_ON(X == 0) is a load.

kmem_cache_free() OTOH is stores (we must modify the free list).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1228001

On 09/18, Peter Zijlstra wrote:
>
> On Fri, Sep 18, 2015 at 03:28:44PM +0200, Oleg Nesterov wrote:
> > On 09/18, Peter Zijlstra wrote:
> > >
> > > 	ns = pid->numbers[pid->level].ns;
> > > 	if ((atomic_read(&pid->count) == 1) ||
> > > 	     atomic_dec_and_test(&pid->count)) {
> > >
> > > +		smp_read_barrier_depends(); /* ctrl-dep */
> >
> > Not sure... Firstly it is not clear what this barrier pairs with. And I
> > have to admit that I can not understand if _CTRL() logic applies here.
> > The same for atomic_read_ctrl().
>
> The control dependency barrier pairs with the full barrier of
> atomic_dec_and_test.

Yes thanks. I already got it. I hope ;)

> > OK, please forget about put_pid() for the moment. Suppose we have
> >
> > 	X = 1;
> > 	synchronize_sched();
> > 	Y = 1;
> >
> > Or
> > 	X = 1;
> > 	call_rcu_sched( func => { Y = 1; } );
> >
> >
> >
> > Now. In theory this this code is wrong:
> >
> > 	if (Y) {
> > 		BUG_ON(X == 0);
> > 	}
> >
> > But this is correct:
> >
> > 	if (Y) {
> > 		rcu_read_lock_sched();
> > 		rcu_read_unlock_sched();
> > 		BUG_ON(X == 0);
> > 	}
> >
> > So perhaps something like this
> >
> > 	/*
> > 	 * Comment to explain it is eq to read_lock + read_unlock,
> > 	 * in a sense that this guarantees a full barrier wrt to
> > 	 * the previous synchronize_sched().
> > 	 */
> > 	#define rcu_read_barrier_sched()	barrier()
> >
> > make sense?
> >
> >
> > And again, I simply can't understand if this code
> >
> > 	if (READ_ONCE_CTRL(Y))
> > 		BUG_ON(X == 0);
> >
> > to me it does _not_ look correct in theory.
>
> So control dependencies provide a load-store barrier. Your examples
> above rely on a load-load barrier; BUG_ON(X == 0) is a load.

Yes, yes...

What I tried to say is that we could fix it another way. And even look
at this problem from another angle. No, it is not that I think it would
be better in this particular case, but still...

put_pid() could do

	if (atomic_read(&pid->count) == 1) {

		rcu_read_lock();
		rcu_read_unlock();
		
		kmem_cache_free(pid);
	}

if we observe atomic_read() == 1, we know that we have at least one
gp pass after all other writes to this memory (namely hlist_del_rcu()
which removes it from rcu-list). Because we can see atomic_read() == 1
until delayed_put_pid() (called by RCU) drops its reference.

and perhaps this lock + unlock pair (which is nop at least for _sched)
makes some sense in general...

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1228071

I can't resist. You certainly don't need this spam, but

On 09/18, Peter Zijlstra wrote:
>
> On Fri, Sep 18, 2015 at 03:28:44PM +0200, Oleg Nesterov wrote:
> >
> > And again, I simply can't understand if this code
> >
> > 	if (READ_ONCE_CTRL(Y))
> > 		BUG_ON(X == 0);
> >
> > to me it does _not_ look correct in theory.
>
> So control dependencies provide a load-store barrier. Your examples
> above rely on a load-load barrier; BUG_ON(X == 0) is a load.

Yes, and this is just obvious.

Yet I was confused because reading other emails I misunderstood the
proposed semantics of atomic_read_ctrl(), so I started to suspect
that in fact _CTRL() does more than I used to think.

Nevermind, sorry for noise, thanks to all.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1228141

On Fri, Sep 18, 2015 at 03:46:30PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 18, 2015 at 03:28:44PM +0200, Oleg Nesterov wrote:
> > On 09/18, Peter Zijlstra wrote:
> > >
> > > On Thu, Sep 17, 2015 at 08:09:19PM +0200, Oleg Nesterov wrote:
> > >
> > > > I need to recheck, but afaics this is not possible. This optimization
> > > > is fine, but probably needs a comment.
> > >
> > > For sure, this code doesn't make any sense to me.
> > 
> > So yes, after a sleep I am starting to agree that in theory this fast-path
> > check is wrong. I'll write another email..
> 
> This other mail will include a patch adding comments to pid.c ? That
> code didn't want to make sense to me this morning.
> 
> > > As an alternative patch, could we not do:
> > >
> > >   void put_pid(struct pid *pid)
> > >   {
> > > 	struct pid_namespace *ns;
> > >
> > > 	if (!pid)
> > > 		return;
> > >
> > > 	ns = pid->numbers[pid->level].ns;
> > > 	if ((atomic_read(&pid->count) == 1) ||
> > > 	     atomic_dec_and_test(&pid->count)) {
> > >
> > > +		smp_read_barrier_depends(); /* ctrl-dep */
> > 
> > Not sure... Firstly it is not clear what this barrier pairs with. And I
> > have to admit that I can not understand if _CTRL() logic applies here.
> > The same for atomic_read_ctrl().
> 
> The control dependency barrier pairs with the full barrier of
> atomic_dec_and_test.
> 
> So the two put_pid() instances:
> 
> 	CPU0					CPU1
> 
> 	pid->foo = 1;
> 	atomic_dec_and_test() == false		atomic_read_ctrl() == 1
> 						kmem_cache_free(pid)
> 
> CPU0 will modify a pid field and decrement, but not reach 0.
> CPU1 finds we're the last, but must also be able to observe our foo
> store such that we can rest assured  it is complete before we free the
> storage.
> 
> The freeing of pid, on CPU1, is stores, these must not happen before we
> satisfy the freeing condition, iow a load-store barrier, which is what
> the control dependency provides.
> 
> > OK, please forget about put_pid() for the moment. Suppose we have
> > 
> > 	X = 1;
> > 	synchronize_sched();
> > 	Y = 1;
> > 
> > Or
> > 	X = 1;
> > 	call_rcu_sched( func => { Y = 1; } );
> > 
> > 
> > 
> > Now. In theory this this code is wrong:
> > 
> > 	if (Y) {
> > 		BUG_ON(X == 0);
> > 	}
> > 
> > But this is correct:
> > 
> > 	if (Y) {
> > 		rcu_read_lock_sched();
> > 		rcu_read_unlock_sched();
> > 		BUG_ON(X == 0);
> > 	}
> > 
> > So perhaps something like this
> > 
> > 	/*
> > 	 * Comment to explain it is eq to read_lock + read_unlock,
> > 	 * in a sense that this guarantees a full barrier wrt to
> > 	 * the previous synchronize_sched().
> > 	 */
> > 	#define rcu_read_barrier_sched()	barrier()
> > 
> > make sense?
> > 
> > 
> > And again, I simply can't understand if this code
> > 
> > 	if (READ_ONCE_CTRL(Y))
> > 		BUG_ON(X == 0);
> > 
> > to me it does _not_ look correct in theory.
> 
> So control dependencies provide a load-store barrier. Your examples
> above rely on a load-load barrier; BUG_ON(X == 0) is a load.
> 
> kmem_cache_free() OTOH is stores (we must modify the free list).

And any reads are bogus, so ordering with writes suffices.  Good!

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227261

On Thu, Sep 17, 2015 at 06:41:46PM +0200, Dmitry Vyukov wrote:
> What happens here exactly matches what is described in CONTROL
> DEPENDENCIES section of Documentation/memory-barriers.txt. So all the
> bad things described there are possible here. The document explicitly
> requires usage of rmb/acquire/READ_ONCE_CTRL in such cases. I don't
> know what to add to that.

I suggest explicitly listing the steps leading to a specific instance
of the failure.  This isn't always easy to do, but it can be very helpful.

> Regarding reordering of "ns = pid->numbers[pid->level].ns". If we are
> talking about the thread that releases the last reference via the
> fast-path atomic_read check, then, yes, they can be reordered by both
> compiler and hardware, but only in a non-observable way, and so it
> does not matter. Only in a non-observable way because the freeing
> thread is the same as the thread that does "ns =
> pid->numbers[pid->level].ns" and both compilers and hardware visibly
> preserve program order for single-thread.

Agreed.  If the compiler or the CPU did that reordering in a way that was
visible, that would be a bug even in single-threaded code.  Therefore,
if that reordering matters, both the compiler and the CPU are forbidden
from doing it.

> If we are talking about threads that release all but last reference,
> then, no, they can't be reordered, because those threads execute
> atomic_dec_and_test which is a release operation.

Also agreed.

							Thanx, Paul

> On Thu, Sep 17, 2015 at 6:08 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > Honestly, I can not see how this can happen. So I do not really
> > understand the problem and the fix.
> >
> > And if this can happen I can't understand how this patch can help.
> > What about "ns = pid->numbers[pid->level].ns" ? It can be reordered
> > with atomic_read_acquire().
> >
> > I leave this to other reviewers, but perhaps you can spell the
> > "For example" part of the changelog.
> >
> >
> > On 09/17, Dmitry Vyukov wrote:
> >>
> >> put_pid checks whether the current thread has the only reference
> >> to the pid with atomic_read() which does not have any memory
> >> barriers, and if so proceeds directly to kmem_cache_free().
> >> As the result memory accesses to the object in kmem_cache_free()
> >> or user accesses to the object after reallocation (again without
> >> any memory barriers on fast path) can hoist above the atomic_read()
> >> check and conflict with memory accesses to the pid object in other
> >> threads before they released their references.
> >>
> >> There is a control dependency between the atomic_read() check and
> >> kmem_cache_free(), but control dependencies are disregarded by some
> >> architectures. Documentation/memory-barriers.txt explicitly states:
> >> "A load-load control dependency requires a full read memory barrier.
> >> ... please note that READ_ONCE_CTRL() is not optional! [even for stores]"
> >> in the CONTROL DEPENDENCIES section.
> >>
> >> For example, if store to the first word of the object to build a freelist
> >> in kmem_cache_free() hoists above the check, stores to the first word
> >> in other threads can corrupt the memory allocator freelist.
> >>
> >> Use atomic_read_acquire() for the fast path check to hand off properly
> >> acquired object to memory allocator.
> >>
> >> The data race was found with KernelThreadSanitizer (KTSAN).
> >>
> >> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> >> ---
> >>  kernel/pid.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/pid.c b/kernel/pid.c
> >> index ca36879..3b0b13d 100644
> >> --- a/kernel/pid.c
> >> +++ b/kernel/pid.c
> >> @@ -242,7 +242,7 @@ void put_pid(struct pid *pid)
> >>               return;
> >>
> >>       ns = pid->numbers[pid->level].ns;
> >> -     if ((atomic_read(&pid->count) == 1) ||
> >> +     if ((atomic_read_acquire(&pid->count) == 1) ||
> >>            atomic_dec_and_test(&pid->count)) {
> >>               kmem_cache_free(ns->pid_cachep, pid);
> >>               put_pid_ns(ns);
> >> --
> >> 2.6.0.rc0.131.gf624c3d
> >>
> >
> 
> 
> 
> -- 
> Dmitry Vyukov, Software Engineer, dvyukov@google.com
> Google Germany GmbH, Dienerstraße 12, 80331, München
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat
> sind, leiten Sie diese bitte nicht weiter, informieren Sie den
> Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
> This e-mail is confidential. If you are not the right addressee please
> do not forward it, please inform the sender, and please erase this
> e-mail including any attachments. Thanks.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web