Re: [PATCH 11/18] seqcount: Introduce raw_write_seqcount_barrier()

[Paul E. McKenney]

On Wed, Jun 17, 2015 at 02:29:24PM +0200, Peter Zijlstra wrote:
> On Thu, Jun 11, 2015 at 02:45:57PM -0700, Paul E. McKenney wrote:
> > Color me slow and stupid.  Maybe due to reviewing a patch too early in
> > the morning, who knows?
> > 
> > There is nothing above that prevents the compiler and the CPU from
> > reordering the assignments to X and Y with the increment of s->sequence++.
> > One fix would be as follows:
> > 
> > 	static inline void raw_write_seqcount_barrier(seqcount_t *s)
> > 	{
> > 		smp_wmb();
> > 		s->sequence++;
> > 		smp_wmb();
> > 		s->sequence++;
> > 		smp_wmb();
> > 	}
> > 
> > Of course, this assumes that the accesses surrounding the call to
> > raw_write_seqcount_barrier() are writes.  If they can be a reads,
> > the two added smp_wmb() calls need to be full barriers.
> 
> I have updated the Changelog to hopefully explain things better.
> 
> I did leave off the READ/WRITE ONCE stuff, because I could not come up
> with a scenario where it makes a difference -- I appreciate paranoia,
> but I also think we should not overdo the thing.

I can only conclude that you have not read this document:

	http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4455.html

Specifically, please keep in mind that unless you mark either the variable
or the memory access, the compiler is within its rights to assume that
there are no concurrent accesses to that variable.  For but one example,
if you do a normal store to a given variable, then the compiler is
within its rights to use that variable as temporary storage prior to
that store.  And yes, you can reasonably argue that no sane compiler
would store something else to s->sequence given that it could free up
a register by storing the incremented value, but the fact remains that
you have given it permission to do so if it wants.

							Thanx, Paul

> ---
> Subject: seqcount: Introduce raw_write_seqcount_barrier()
> From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Date: Thu Jun 11 12:35:48 CEST 2015
> 
> Introduce raw_write_seqcount_barrier(), a new construct that can be
> used to provide write barrier semantics in seqcount read loops instead
> of the usual consistency guarantee.
> 
> raw_write_seqcount_barier() is equivalent to:
> 
> 	raw_write_seqcount_begin();
> 	raw_write_seqcount_end();
> 
> But avoids issueing two back-to-back smp_wmb() instructions.
> 
> This construct works because the read side will 'stall' when observing
> odd values. This means that -- referring to the example in the comment
> below -- even though there is no (matching) read barrier between the
> loads of X and Y, we cannot observe !x && !y, because:
> 
>  - if we observe Y == false we must observe the first sequence
>    increment, which makes us loop, until
> 
>  - we observe !(seq & 1) -- the second sequence increment -- at which
>    time we must also observe T == true.
> 
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Paul McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> Suggested-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
>  include/linux/seqlock.h |   42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -233,6 +233,47 @@ static inline void raw_write_seqcount_en
>  	s->sequence++;
>  }
> 
> +/**
> + * raw_write_seqcount_barrier - do a seq write barrier
> + * @s: pointer to seqcount_t
> + *
> + * This can be used to provide an ordering guarantee instead of the
> + * usual consistency guarantee. It is one wmb cheaper, because we can
> + * collapse the two back-to-back wmb()s.
> + *
> + *      seqcount_t seq;
> + *      bool X = true, Y = false;
> + *
> + *      void read(void)
> + *      {
> + *              bool x, y;
> + *
> + *              do {
> + *                      int s = read_seqcount_begin(&seq);
> + *
> + *                      x = X; y = Y;
> + *
> + *              } while (read_seqcount_retry(&seq, s));
> + *
> + *              BUG_ON(!x && !y);
> + *      }
> + *
> + *      void write(void)
> + *      {
> + *              Y = true;
> + *
> + *              raw_write_seqcount_barrier(seq);
> + *
> + *              X = false;
> + *      }
> + */
> +static inline void raw_write_seqcount_barrier(seqcount_t *s)
> +{
> +	s->sequence++;
> +	smp_wmb();
> +	s->sequence++;
> +}
> +
>  /*
>   * raw_write_seqcount_latch - redirect readers to even/odd copy
>   * @s: pointer to seqcount_t
> 

--
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/