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

Groups > linux.kernel > #1404895 > unrolled thread

[PATCH] seqlock: fix raw_read_seqcount_latch()

Back to article view | Back to linux.kernel

Contents

  [PATCH] seqlock: fix raw_read_seqcount_latch() Alexey Dobriyan <adobriyan@gmail.com> - 2016-05-21 22:20 +0200
    Re: [PATCH] seqlock: fix raw_read_seqcount_latch() Peter Zijlstra <peterz@infradead.org> - 2016-05-22 12:50 +0200
      Re: [PATCH] seqlock: fix raw_read_seqcount_latch() Alexey Dobriyan <adobriyan@gmail.com> - 2016-05-22 21:00 +0200
        Re: [PATCH] seqlock: fix raw_read_seqcount_latch() Peter Zijlstra <peterz@infradead.org> - 2016-05-23 11:40 +0200
          Re: [PATCH] seqlock: fix raw_read_seqcount_latch() Tejun Heo <tj@kernel.org> - 2016-05-25 22:00 +0200
            [PATCH] percpu: Revert ("percpu: Replace smp_read_barrier_depends()  with lockless_dereference()") Tejun Heo <tj@kernel.org> - 2016-05-25 22:20 +0200
    Re: [PATCH] seqlock: fix raw_read_seqcount_latch() Peter Zijlstra <peterz@infradead.org> - 2016-05-27 13:20 +0200

#1404895 — [PATCH] seqlock: fix raw_read_seqcount_latch()

lockless_dereference() is supposed to take pointer not integer.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 include/linux/seqlock.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -277,7 +277,7 @@ static inline void raw_write_seqcount_barrier(seqcount_t *s)
 
 static inline int raw_read_seqcount_latch(seqcount_t *s)
 {
-	return lockless_dereference(s->sequence);
+	return lockless_dereference(s)->sequence;
 }
 
 /**
@@ -331,7 +331,7 @@ static inline int raw_read_seqcount_latch(seqcount_t *s)
  *	unsigned seq, idx;
  *
  *	do {
- *		seq = lockless_dereference(latch->seq);
+ *		seq = lockless_dereference(latch)->seq;
  *
  *		idx = seq & 0x01;
  *		entry = data_query(latch->data[idx], ...);

[toc] | [next] | [standalone]

#1404994

On Sat, May 21, 2016 at 11:14:49PM +0300, Alexey Dobriyan wrote:
> lockless_dereference() is supposed to take pointer not integer.

Urgh :/

Is there any way we can make lockless_dereference() issue a warning if
we don't feed it a pointer?

Would something like so work? All pointer types should silently cast to
void * while integer (and others) should refuse to.

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index b5ff9881bef8..8886de704d33 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -544,6 +544,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  */
 #define lockless_dereference(p) \
 ({ \
+ 	__maybe_unused void * _________p2 = p; \
 	typeof(p) _________p1 = READ_ONCE(p); \
 	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
 	(_________p1); \

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

#1405032

On Sun, May 22, 2016 at 12:48:27PM +0200, Peter Zijlstra wrote:
> On Sat, May 21, 2016 at 11:14:49PM +0300, Alexey Dobriyan wrote:
> > lockless_dereference() is supposed to take pointer not integer.
> 
> Urgh :/
> 
> Is there any way we can make lockless_dereference() issue a warning if
> we don't feed it a pointer?
> 
> Would something like so work? All pointer types should silently cast to
> void * while integer (and others) should refuse to.

This works (and spammy enough in case of seqlock, which is good)
but not for "unsigned long":

	include/linux/percpu-refcount.h:146:36: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
	  percpu_ptr = lockless_dereference(ref->percpu_count_ptr);

> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -544,6 +544,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
>   */
>  #define lockless_dereference(p) \
>  ({ \
> + 	__maybe_unused void * _________p2 = p; \
>  	typeof(p) _________p1 = READ_ONCE(p); \
>  	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
>  	(_________p1); \

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

#1405197

On Sun, May 22, 2016 at 09:50:40PM +0300, Alexey Dobriyan wrote:
> On Sun, May 22, 2016 at 12:48:27PM +0200, Peter Zijlstra wrote:
> > On Sat, May 21, 2016 at 11:14:49PM +0300, Alexey Dobriyan wrote:
> > > lockless_dereference() is supposed to take pointer not integer.
> > 
> > Urgh :/
> > 
> > Is there any way we can make lockless_dereference() issue a warning if
> > we don't feed it a pointer?
> > 
> > Would something like so work? All pointer types should silently cast to
> > void * while integer (and others) should refuse to.
> 
> This works (and spammy enough in case of seqlock, which is good)
> but not for "unsigned long":
> 
> 	include/linux/percpu-refcount.h:146:36: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
> 	  percpu_ptr = lockless_dereference(ref->percpu_count_ptr);

TJ; would you prefer casting or not using lockless_dereference() here?

> 
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -544,6 +544,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
> >   */
> >  #define lockless_dereference(p) \
> >  ({ \
> > + 	__maybe_unused void * _________p2 = p; \
> >  	typeof(p) _________p1 = READ_ONCE(p); \
> >  	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
> >  	(_________p1); \

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

#1407183

Hello,

On Mon, May 23, 2016 at 11:36:18AM +0200, Peter Zijlstra wrote:
> > 	include/linux/percpu-refcount.h:146:36: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
> > 	  percpu_ptr = lockless_dereference(ref->percpu_count_ptr);
> 
> TJ; would you prefer casting or not using lockless_dereference() here?

Casting is nasty - *(unsigned long __percpu **)& - because the macro
expects an lvalue.  I think it'd be better to revert to opencoding
READ_ONCE() and barrier there.  It's a pretty special case anyway.

Thanks.

-- 
tejun

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

#1407190 — [PATCH] percpu: Revert ("percpu: Replace smp_read_barrier_depends() with lockless_dereference()")

lockless_dereference() is planned to grow a sanity check to ensure
that the input parameter is a pointer.  __ref_is_percpu() passes in an
unsinged long value which is a combination of a pointer and a flag.
While it can be casted to a pointer lvalue, the casting looks messy
and it's a special case anyway.  Let's revert back to open-coding
READ_ONCE() and explicit barrier.

This doesn't cause any functional changes.

Signed-off-by: Tejun Heo <tj@kernel.org>
Link: http://lkml.kernel.org/g/20160522185040.GA23664@p183.telecom.by
Cc: Pranith Kumar <bobby.prani@gmail.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
So, something like this.  Please feel free to include in the series.

Thanks.

 include/linux/percpu-refcount.h |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/linux/percpu-refcount.h b/include/linux/percpu-refcount.h
index 84f542d..1c7eec0 100644
--- a/include/linux/percpu-refcount.h
+++ b/include/linux/percpu-refcount.h
@@ -136,14 +136,12 @@ static inline bool __ref_is_percpu(struct percpu_ref *ref,
 	 * used as a pointer.  If the compiler generates a separate fetch
 	 * when using it as a pointer, __PERCPU_REF_ATOMIC may be set in
 	 * between contaminating the pointer value, meaning that
-	 * ACCESS_ONCE() is required when fetching it.
-	 *
-	 * Also, we need a data dependency barrier to be paired with
-	 * smp_store_release() in __percpu_ref_switch_to_percpu().
-	 *
-	 * Use lockless deref which contains both.
+	 * READ_ONCE() is required when fetching it.
 	 */
-	percpu_ptr = lockless_dereference(ref->percpu_count_ptr);
+	percpu_ptr = READ_ONCE(ref->percpu_count_ptr);
+
+	/* paired with smp_store_release() in __percpu_ref_switch_to_percpu() */
+	smp_read_barrier_depends();
 
 	/*
 	 * Theoretically, the following could test just ATOMIC; however,

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

#1408006

On Sat, May 21, 2016 at 11:14:49PM +0300, Alexey Dobriyan wrote:
> lockless_dereference() is supposed to take pointer not integer.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
> 
>  include/linux/seqlock.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -277,7 +277,7 @@ static inline void raw_write_seqcount_barrier(seqcount_t *s)
>  
>  static inline int raw_read_seqcount_latch(seqcount_t *s)
>  {
> -	return lockless_dereference(s->sequence);
> +	return lockless_dereference(s)->sequence;
>  }
>  
>  /**
> @@ -331,7 +331,7 @@ static inline int raw_read_seqcount_latch(seqcount_t *s)
>   *	unsigned seq, idx;
>   *
>   *	do {
> - *		seq = lockless_dereference(latch->seq);
> + *		seq = lockless_dereference(latch)->seq;
>   *
>   *		idx = seq & 0x01;
>   *		entry = data_query(latch->data[idx], ...);

So while the code was dubious; I it is now wrong, but my head hurts.

I'll queue the below, TJs per-cpu change and the lockless_dereference()
void * cast trick.


---
Subject: seqcount: Re-fix raw_read_seqcount_latch()

Commit 50755bc1c305 ("seqlock: fix raw_read_seqcount_latch()") broke
raw_read_seqcount_latch().

If you look at the comment that was modified; the thing that changes is
the seq count, not the latch pointer.

 * void latch_modify(struct latch_struct *latch, ...)
 * {
 *	smp_wmb();	<- Ensure that the last data[1] update is visible
 *	latch->seq++;
 *	smp_wmb();	<- Ensure that the seqcount update is visible
 *
 *	modify(latch->data[0], ...);
 *
 *	smp_wmb();	<- Ensure that the data[0] update is visible
 *	latch->seq++;
 *	smp_wmb();	<- Ensure that the seqcount update is visible
 *
 *	modify(latch->data[1], ...);
 * }
 *
 * The query will have a form like:
 *
 * struct entry *latch_query(struct latch_struct *latch, ...)
 * {
 *	struct entry *entry;
 *	unsigned seq, idx;
 *
 *	do {
 *		seq = lockless_dereference(latch->seq);

So here we have:

		seq = READ_ONCE(latch->seq);
		smp_read_barrier_depends();

Which is exactly what we want; the new code:

		seq = ({ p = READ_ONCE(latch);
			 smp_read_barrier_depends(); p })->seq;

is just wrong; because it looses the volatile read on seq, which can now
be torn or worse 'optimized'. And the read_depend barrier is also placed
wrong, we want it after the load of seq, to match the above data[]
up-to-date wmb()s.

Such that when we dereference latch->data[] below, we're guaranteed to
observe the right data.

 *
 *		idx = seq & 0x01;
 *		entry = data_query(latch->data[idx], ...);
 *
 *		smp_rmb();
 *	} while (seq != latch->seq);
 *
 *	return entry;
 * }

So yes, not passing a pointer is not pretty, but the code was correct,
and isn't anymore now.

Change to explicit READ_ONCE()+smp_read_barrier_depends() to avoid
confusion and allow strict lockless_dereference() checking.

Fixes: 50755bc1c305 ("seqlock: fix raw_read_seqcount_latch()")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/seqlock.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index 7973a821ac58..f3db247cebc8 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -277,7 +277,9 @@ static inline void raw_write_seqcount_barrier(seqcount_t *s)
 
 static inline int raw_read_seqcount_latch(seqcount_t *s)
 {
-	return lockless_dereference(s)->sequence;
+	int seq = READ_ONCE(s->sequence);
+	smp_read_barrier_depends();
+	return seq;
 }
 
 /**
@@ -331,7 +333,7 @@ static inline int raw_read_seqcount_latch(seqcount_t *s)
  *	unsigned seq, idx;
  *
  *	do {
- *		seq = lockless_dereference(latch)->seq;
+ *		seq = raw_read_seqcount_latch(&latch->seq);
  *
  *		idx = seq & 0x01;
  *		entry = data_query(latch->data[idx], ...);

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web