Re: [PATCH] kernel: fix data race in put_pid

[Dmitry Vyukov]

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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> 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@xxxxxxxxxx
> Cc: torvalds@xxxxxxxxxxxxxxxxxxxx
> Cc: will.deacon@xxxxxxx
> Cc: paulmck@xxxxxxxxxxxxxxxxxx
> Requested-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxx
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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/