Remove to-be-unneeded smp_read_barrier_depends()

Because READ_ONCE() now implies smp_read_barrier_depends(), this commit removes the now-redundant smp_read_barrier_depends() following the READ_ONCE() in __ref_is_percpu().

Signed-off-by Paul E. McKenney <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Christoph Lameter <[email protected]>

Quoted 7 lines · Paul E. McKenney

Because READ_ONCE() now implies smp_read_barrier_depends(), this commit removes the now-redundant smp_read_barrier_depends() following the READ_ONCE() in __ref_is_percpu().

Signed-off-by Paul E. McKenney <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Christoph Lameter <[email protected]>

Acked-by Tejun Heo <[email protected]>

Please feel free to route with other patches. If this should be routed through the percpu tree, please let me know.

Quoted 2 lines

Please feel free to route with other patches. If this should be routed through the percpu tree, please let me know.

This depends on some other not-yet-finalized patch series, so if you are OK with it, I will route them with that group of serieses.

The comment in tracepoint_add_func() mentions smp_read_barrier_depends(), whose use should be quite restricted. This commit updates the comment to instead mention the smp_store_release() and rcu_dereference_sched() that the current code actually uses.

Signed-off-by Paul E. McKenney <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Steven Rostedt <[email protected]>

---
 kernel/tracepoint.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

kernel/tracepoint.c +4 -5

index 685c50ae6300..671b13457387 100644
@@ -212,11 +212,10 @@ static int tracepoint_add_func(struct tracepoint *tp,
 	}
 
 	/*
-	 * rcu_assign_pointer has a smp_wmb() which makes sure that the new
-	 * probe callbacks array is consistent before setting a pointer to it.
-	 * This array is referenced by __DO_TRACE from
-	 * include/linux/tracepoints.h. A matching smp_read_barrier_depends()
-	 * is used.
+	 * rcu_assign_pointer has as smp_store_release() which makes sure
+	 * that the new probe callbacks array is consistent before setting
+	 * a pointer to it.  This array is referenced by __DO_TRACE from
+	 * include/linux/tracepoint.h using rcu_dereference_sched().
 	 */
 	rcu_assign_pointer(tp->funcs, tp_funcs);
 	if (!static_key_enabled(&tp->key))

[ added Mathieu ]

Quoted 5 lines · On Mon, 9 Oct 2017 17:22:45 -0700 "Paul E. McKenney"…

The comment in tracepoint_add_func() mentions smp_read_barrier_depends(), whose use should be quite restricted. This commit updates the comment to instead mention the smp_store_release() and rcu_dereference_sched() that the current code actually uses.

Quoted 3 lines

Signed-off-by Paul E. McKenney <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Steven Rostedt <[email protected]>

Acked-by Steven Rostedt (VMware) <[email protected]>

-- Steve

Quoted 24 lines

---
 kernel/tracepoint.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

kernel/tracepoint.c +4 -5

index 685c50ae6300..671b13457387 100644
@@ -212,11 +212,10 @@ static int tracepoint_add_func(struct tracepoint *tp,
 	}
 
 	/*
-	 * rcu_assign_pointer has a smp_wmb() which makes sure that the new
-	 * probe callbacks array is consistent before setting a pointer to it.
-	 * This array is referenced by __DO_TRACE from
-	 * include/linux/tracepoints.h. A matching smp_read_barrier_depends()
-	 * is used.
+	 * rcu_assign_pointer has as smp_store_release() which makes sure
+	 * that the new probe callbacks array is consistent before setting
+	 * a pointer to it.  This array is referenced by __DO_TRACE from
+	 * include/linux/tracepoint.h using rcu_dereference_sched().
 	 */
 	rcu_assign_pointer(tp->funcs, tp_funcs);
 	if (!static_key_enabled(&tp->key))

Quoted 10 lines · ----- On Oct 9, 2017, at 8:31 PM, rostedt [email protected] wrote:

[ added Mathieu ]

Quoted 5 lines · On Mon, 9 Oct 2017 17:22:45 -0700 "Paul E. McKenney"…

The comment in tracepoint_add_func() mentions smp_read_barrier_depends(), whose use should be quite restricted. This commit updates the comment to instead mention the smp_store_release() and rcu_dereference_sched() that the current code actually uses.

Quoted 6 lines

Quoted 3 lines

Signed-off-by Paul E. McKenney <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Steven Rostedt <[email protected]>

Acked-by Steven Rostedt (VMware) <[email protected]>

Yeah I think we don't need to spell out the implementation of rcu_dereference_sched() there (which actually uses lockless_dereference(), which indeed ends up relying on smp_read_barrier_depends()). The comment was a bit too low-level for the benefit of its audience.

I agree with the change.

Acked-by Mathieu Desnoyers <[email protected]>

Thanks,

Mathieu

Quoted 26 lines

-- Steve

Quoted 24 lines

---
 kernel/tracepoint.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

kernel/tracepoint.c +4 -5

index 685c50ae6300..671b13457387 100644
@@ -212,11 +212,10 @@ static int tracepoint_add_func(struct tracepoint *tp,
 	}
 
 	/*
-	 * rcu_assign_pointer has a smp_wmb() which makes sure that the new
-	 * probe callbacks array is consistent before setting a pointer to it.
-	 * This array is referenced by __DO_TRACE from
-	 * include/linux/tracepoints.h. A matching smp_read_barrier_depends()
-	 * is used.
+	 * rcu_assign_pointer has as smp_store_release() which makes sure
+	 * that the new probe callbacks array is consistent before setting
+	 * a pointer to it.  This array is referenced by __DO_TRACE from
+	 * include/linux/tracepoint.h using rcu_dereference_sched().
 	 */
 	rcu_assign_pointer(tp->funcs, tp_funcs);
 	if (!static_key_enabled(&tp->key))

Signature · 3 lines

Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Now that smp_read_barrier_depends() is implied by READ_ONCE(), adding READ_ONCE() to assoc_array_ptr_to_leaf() and __assoc_array_ptr_to_meta() allows the several smp_read_barrier_depends() calls to be removed from lib/assoc_array.c. This commit makes this change.

Quoted 4 lines

Now that smp_read_barrier_depends() is implied by READ_ONCE(), adding READ_ONCE() to assoc_array_ptr_to_leaf() and __assoc_array_ptr_to_meta() allows the several smp_read_barrier_depends() calls to be removed from lib/assoc_array.c. This commit makes this change.

So arguably this code was broken for not already having READ_ONCE().

include/linux/assoc_array_priv.h static inline bool assoc_array_ptr_is_node(const struct assoc_array_ptr *x) · line 140

Quoted 6 lines · Paul E. McKenney

static inline void *assoc_array_ptr_to_leaf(const struct assoc_array_ptr *x) {

include/linux/assoc_array_priv.h +2 -1

-	return (void *)((unsigned long)x & ~ASSOC_ARRAY_PTR_TYPE_MASK);
+	return (void *)((unsigned long)READ_ONCE(x) & /* Address dependency. */
+		~ASSOC_ARRAY_PTR_TYPE_MASK);
 }

This is the wrong place to do this. assoc_array_ptr_to_leaf() is effectively no more than a special cast; it removes a metadata bit from a pointer. x is the value we're modifying, not *x, and x was read by the caller.

Quoted 12 lines · David Howells

Review

Quoted 6 lines · Paul E. McKenney

static inline void *assoc_array_ptr_to_leaf(const struct assoc_array_ptr *x) {

(quoted diff) +2 -1

-	return (void *)((unsigned long)x & ~ASSOC_ARRAY_PTR_TYPE_MASK);
+	return (void *)((unsigned long)READ_ONCE(x) & /* Address dependency. */
+		~ASSOC_ARRAY_PTR_TYPE_MASK);
 }

This is the wrong place to do this. assoc_array_ptr_to_leaf() is effectively no more than a special cast; it removes a metadata bit from a pointer. x is the value we're modifying, not *x, and x was read by the caller.

Also, x is not a pointer you can read from, so if READ_ONCE(x) ever effects a memory access, you might get an oops.

Review

Quoted 6 lines · Paul E. McKenney

static inline void *assoc_array_ptr_to_leaf(const struct assoc_array_ptr *x) {

(quoted diff) +2 -1

-	return (void *)((unsigned long)x & ~ASSOC_ARRAY_PTR_TYPE_MASK);
+	return (void *)((unsigned long)READ_ONCE(x) & /* Address dependency. */
+		~ASSOC_ARRAY_PTR_TYPE_MASK);
 }

This is the wrong place to do this. assoc_array_ptr_to_leaf() is effectively no more than a special cast; it removes a metadata bit from a pointer. x is the value we're modifying, not *x, and x was read by the caller.

Review

Quoted 7 lines

static inline
unsigned long __assoc_array_ptr_to_meta(const struct assoc_array_ptr *x) {

(quoted diff) +1 -1

-	return (unsigned long)x &
+	return (unsigned long)READ_ONCE(x) & /* Address dependency. */
 		~(ASSOC_ARRAY_PTR_SUBTYPE_MASK | ASSOC_ARRAY_PTR_TYPE_MASK);
 }

Ditto.

Review

Quoted 10 lines

(quoted diff) +1 -7

-		ptr = READ_ONCE(node->slots[slot]);
+		ptr = READ_ONCE(node->slots[slot]); /* Address dependency. */
 		has_meta |= (unsigned long)ptr;
 		if (ptr && assoc_array_ptr_is_leaf(ptr)) {
-			/* We need a barrier between the read of the pointer
-			 * and dereferencing the pointer - but only if we are
-			 * actually going to dereference it.
-			 */
-			smp_read_barrier_depends();
-

For example, you can see the READ_ONCE() here; that is already done.

Can you also not just delete the comment, but rephrase it to indicate that a barrier is necessary and it's done by READ_ONCE(), please?

lib/assoc_array.c void *assoc_array_find(const struct assoc_array *array, · line 333

Quoted 8 lines · Paul E. McKenney

lib/assoc_array.c +2 -0

+			/* We need a barrier between the read of the pointer,
+			 * which is supplied by the above READ_ONCE().
 			 */
 
 			/* We need a barrier between the read of the pointer
 			 * and dereferencing the pointer - but only if we are
 			 * actually going to dereference it.
 			 */

Could you also fix those "disgusting drug-induced crap" comment styles while you're there anyway?

http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html

lib/assoc_array.c void *assoc_array_find(const struct assoc_array *array, · line 322

Quoted 3 lines · Paul E. McKenney

lib/assoc_array.c +1 -2

-	node = result.terminal_node.node;
-	smp_read_barrier_depends();
+	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */

The main problem I have with this method of annotation is that it's not obvious there's a barrier there or which side the barrier is.

I think one of the trickiest issues is that a barrier is typically between two things and we're not making it clear what those two things actually are.

Also, I would say that the most natural interpretation of READ_ONCE() is that the implicit barrier comes after the read, e.g.:

	f = READ_ONCE(stuff->foo);
	/* Implied barrier */
	look_at(f->a);
	look_at(f->b);

I.e. READ_ONCE() prevents stuff->foo from being reread whilst you access f and orders LOAD(stuff->foo) before LOAD(f->a) and LOAD(f->b).

Quoted 20 lines · David Howells

lib/assoc_array.c void *assoc_array_find(const struct assoc_array *array, · line 322

Quoted 3 lines · Paul E. McKenney

lib/assoc_array.c +1 -2

-	node = result.terminal_node.node;
-	smp_read_barrier_depends();
+	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */

The main problem I have with this method of annotation is that it's not obvious there's a barrier there or which side the barrier is.

I think one of the trickiest issues is that a barrier is typically between two things and we're not making it clear what those two things actually are.

Also, I would say that the most natural interpretation of READ_ONCE() is that the implicit barrier comes after the read, e.g.:

	f = READ_ONCE(stuff->foo);
	/* Implied barrier */
	look_at(f->a);
	look_at(f->b);

Quoted 2 lines

I.e. READ_ONCE() prevents stuff->foo from being reread whilst you access f and orders LOAD(stuff->foo) before LOAD(f->a) and LOAD(f->b).

FWIW, that's exactly what my patches do, this fixup looks a bit weird because it removes a prior barrier which suggests that either (a) it's in the wrong place to start with, or (b) we're annotating the wrong load.

Quoted 21 lines · Will Deacon

Quoted 20 lines · David Howells

lib/assoc_array.c void *assoc_array_find(const struct assoc_array *array, · line 322

Quoted 3 lines · Paul E. McKenney

lib/assoc_array.c +1 -2

-	node = result.terminal_node.node;
-	smp_read_barrier_depends();
+	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */

The main problem I have with this method of annotation is that it's not obvious there's a barrier there or which side the barrier is.

I think one of the trickiest issues is that a barrier is typically between two things and we're not making it clear what those two things actually are.

Also, I would say that the most natural interpretation of READ_ONCE() is that the implicit barrier comes after the read, e.g.:

	f = READ_ONCE(stuff->foo);
	/* Implied barrier */
	look_at(f->a);
	look_at(f->b);

Quoted 6 lines

Quoted 2 lines

I.e. READ_ONCE() prevents stuff->foo from being reread whilst you access f and orders LOAD(stuff->foo) before LOAD(f->a) and LOAD(f->b).

FWIW, that's exactly what my patches do, this fixup looks a bit weird because it removes a prior barrier which suggests that either (a) it's in the wrong place to start with, or (b) we're annotating the wrong load.

You lost me on this one. Here is the side-by-side change, minus the comment:

node = result.terminal_node.node; node = READ_ONCE(result.terminal_node.node);
smp_read_barrier_depends();

The barrier was after the load that got annotated.

Quoted 35 lines · Paul E. McKenney

Quoted 21 lines · Will Deacon

Quoted 20 lines · David Howells

lib/assoc_array.c void *assoc_array_find(const struct assoc_array *array, · line 322

Quoted 3 lines · Paul E. McKenney

lib/assoc_array.c +1 -2

-	node = result.terminal_node.node;
-	smp_read_barrier_depends();
+	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */

The main problem I have with this method of annotation is that it's not obvious there's a barrier there or which side the barrier is.

I think one of the trickiest issues is that a barrier is typically between two things and we're not making it clear what those two things actually are.

Also, I would say that the most natural interpretation of READ_ONCE() is that the implicit barrier comes after the read, e.g.:

	f = READ_ONCE(stuff->foo);
	/* Implied barrier */
	look_at(f->a);
	look_at(f->b);

Quoted 6 lines

Quoted 2 lines

I.e. READ_ONCE() prevents stuff->foo from being reread whilst you access f and orders LOAD(stuff->foo) before LOAD(f->a) and LOAD(f->b).

FWIW, that's exactly what my patches do, this fixup looks a bit weird because it removes a prior barrier which suggests that either (a) it's in the wrong place to start with, or (b) we're annotating the wrong load.

You lost me on this one. Here is the side-by-side change, minus the comment:

node = result.terminal_node.node; node = READ_ONCE(result.terminal_node.node);
smp_read_barrier_depends();

Quoted 1 line

The barrier was after the load that got annotated.

Yes, sorry, I completely lost my ability to read diff. Looking again, I don't actually know what's being ordered by the smp_read_barrier_depends() in the snippet above, given that assigning "node" is a load from the stack afaict.

FWIW, that's exactly what my patches do, this fixup looks a bit weird because it removes a prior barrier which suggests that either (a) it's in the wrong place to start with, or (b) we're annotating the wrong load.

Quoted 3 lines · Will Deacon

FWIW, that's exactly what my patches do, this fixup looks a bit weird because it removes a prior barrier which suggests that either (a) it's in the wrong place to start with, or (b) we're annotating the wrong load.

There is a loop involved. The barrier is against the read in the previous iteration of the loop. IIRC, the reason I did it this way is to avoid the need for the barrier if there's nothing on the 'after-side' - ie. we examine the pointer and see that it's NULL or a leaf. However, I'm not sure that's a particularly necessary optimisation.

So if READ_ONCE() issues a smp_read_barrier_depends() after the read, then I've no problem with the removal of these explicit barriers.

I will, however, quibble with the appropriateness of the name READ_ONCE()... I still think it's not sufficiently obvious that this is a barrier and the barrier is after. Maybe READ_AND_BARRIER()?

Also, does WRITE_ONCE() imply a preceding barrier?

It does not. In most cases, the barriered version would be smp_store_release().

Quoted 2 lines · Paul E. McKenney

It does not. In most cases, the barriered version would be smp_store_release().

Ummm... Is that good enough? Is:

	WRITE_ONCE(x, 1);
	WRITE_ONCE(x, 2);

equivalent to:

	smp_store_release(x, 1);
	smp_store_release(x, 2);

if CONFIG_SMP=n?

(Consider what happens if an interrupt messes with x).

If it is good enough, should we be using smp_load_acquire() rather than READ_ONCE()?

Quoted 2 lines · Paul E. McKenney

It does not. In most cases, the barriered version would be smp_store_release().

Ummm... Is that good enough? Is:

	WRITE_ONCE(x, 1);
	WRITE_ONCE(x, 2);

equivalent to:

	smp_store_release(x, 1);
	smp_store_release(x, 2);

if CONFIG_SMP=n?

(Consider what happens if an interrupt messes with x).

If it is good enough, should we be using smp_load_acquire() rather than READ_ONCE()?

Quoted 3 lines · David Howells

I will, however, quibble with the appropriateness of the name READ_ONCE()... I still think it's not sufficiently obvious that this is a barrier and the barrier is after. Maybe READ_AND_BARRIER()?

Linus was unhappy with READ_ONCE_CTRL() to tag control dependencies, but indicated that he might consider it if it helped code-analysis tools. Adding Dmitry Vyukov for his thoughts on whether tagging READ_ONCE() for dependencies would help. Me, I would suggest READ_ONCE_DEP(), but let's figure out if the bikeshed needs to be painted before arguing over the color. ;-)

Quoted 11 lines · Paul E. McKenney

Quoted 3 lines · David Howells

I will, however, quibble with the appropriateness of the name READ_ONCE()... I still think it's not sufficiently obvious that this is a barrier and the barrier is after. Maybe READ_AND_BARRIER()?

Linus was unhappy with READ_ONCE_CTRL() to tag control dependencies, but indicated that he might consider it if it helped code-analysis tools. Adding Dmitry Vyukov for his thoughts on whether tagging READ_ONCE() for dependencies would help. Me, I would suggest READ_ONCE_DEP(), but let's figure out if the bikeshed needs to be painted before arguing over the color. ;-)

Count me one vote for the READ_ONCE() name. This is about dependent reads, which are nothing special on anything except Alpha.

We want to remove the exception/specialness from the memory model; and therefore have to fix up all primitives that could possibly be used for these reads to unconditionally issue the barrier (on Alpha). The alternative is: rm -rf arch/alpha.

Adding something like READ_ONCE_DEP() does not rid us of the idea that dependent reads are special and thus defeats the purpose, we might as well retain lockless_dereference().

Now; any user of dependent reads must use READ_ONCE() in any case, to avoid load tearing and reloads. So using READ_ONCE() for the dependent reads is not extra or additional (note we'll also have to add the barrier to all our relaxed and release atomics and anything else that implies READ_ONCE and doesn't already imply smp_mb() after).

Quoted 25 lines · Peter Zijlstra

Quoted 11 lines · Paul E. McKenney

Quoted 3 lines · David Howells

I will, however, quibble with the appropriateness of the name READ_ONCE()... I still think it's not sufficiently obvious that this is a barrier and the barrier is after. Maybe READ_AND_BARRIER()?

Linus was unhappy with READ_ONCE_CTRL() to tag control dependencies, but indicated that he might consider it if it helped code-analysis tools. Adding Dmitry Vyukov for his thoughts on whether tagging READ_ONCE() for dependencies would help. Me, I would suggest READ_ONCE_DEP(), but let's figure out if the bikeshed needs to be painted before arguing over the color. ;-)

Count me one vote for the READ_ONCE() name. This is about dependent reads, which are nothing special on anything except Alpha.

We want to remove the exception/specialness from the memory model; and therefore have to fix up all primitives that could possibly be used for these reads to unconditionally issue the barrier (on Alpha). The alternative is: rm -rf arch/alpha.

Adding something like READ_ONCE_DEP() does not rid us of the idea that dependent reads are special and thus defeats the purpose, we might as well retain lockless_dereference().

Quoted 5 lines

Now; any user of dependent reads must use READ_ONCE() in any case, to avoid load tearing and reloads. So using READ_ONCE() for the dependent reads is not extra or additional (note we'll also have to add the barrier to all our relaxed and release atomics and anything else that implies READ_ONCE and doesn't already imply smp_mb() after).

Add the per-cpu ops to that list, they imply READ_ONCE(). Consider for example this example:

	for_each_possible_cpu(cpu)
		smp_store_release(per_cpu_ptr(&foo, cpu), obj);

-vs-

	obj = this_cpu_read(foo);
	if (obj->ponies)
		fart_rainbow(obj);

Interesting. Do we currently have any dependencies headed by this_cpu_read()?

Quoted 2 lines · Paul E. McKenney

Interesting. Do we currently have any dependencies headed by this_cpu_read()?

Nope, but almost, look for: cpufreq_update_util_data.

This, you mean?

data = rcu_dereference_sched(*this_cpu_ptr(&cpufreq_update_util_data));

Quoted 3 lines · Paul E. McKenney

This, you mean?

data = rcu_dereference_sched(*this_cpu_ptr(&cpufreq_update_util_data));

Yep, that one. Although in my tree it now appears to look like:

data = rcu_dereference_sched(*per_cpu_ptr(&cpufreq_update_util_data,
cpu_of(rq)));

Quoted 8 lines · Peter Zijlstra

Quoted 3 lines · Paul E. McKenney

This, you mean?

data = rcu_dereference_sched(*this_cpu_ptr(&cpufreq_update_util_data));

Yep, that one. Although in my tree it now appears to look like:

Quoted 2 lines

data = rcu_dereference_sched(*per_cpu_ptr(&cpufreq_update_util_data,
cpu_of(rq)));

So in the non-RCU case, we could simply replace rcu_dereference_sched() with ACCESS_ONCE(), right? Thus no need to change the per-CPU primitives. Especially given that most uses of per-CPU variables don't even need protection from load/store tearing, let alone protection from Alpha.

A long thread and I lost track somewhat, but, yes, from KTSAN (data race detector) point of view we will need a way to understand when things are ordered due to data and control dependencies (i.e.effectively acquire but only wrt the dependent stuff). There is a logic level and physical level, it's perfectly fine if on physical level all these _DEP/_CTRL variants are no-op (the same as READ_ONCE, at least on todays archs with todays compilers). But on logical level they represent a well defined, meaningful thing.

Quoted 9 lines · Dmitry Vyukov

A long thread and I lost track somewhat, but, yes, from KTSAN (data race detector) point of view we will need a way to understand when things are ordered due to data and control dependencies (i.e.effectively acquire but only wrt the dependent stuff). There is a logic level and physical level, it's perfectly fine if on physical level all these _DEP/_CTRL variants are no-op (the same as READ_ONCE, at least on todays archs with todays compilers). But on logical level they represent a well defined, meaningful thing.

No, they do not.

Do you believe in fairies and Santa Claus?

Because quite frankly, the likelihood of either of those being true is _way_ higher than the likelihood of any normal human ever getting those things right.

So asking a programmer to annotate whether two memory accesses have a data dependency or a control dependency is completely inappropriate. You won't get people understanding it to begin with, much less then figure out subtle things like whether a control dependency is an actual branch, or might be turned into a data dependency through select, or whatever.

We've had really smart people who wrote core code that couldn't get it right, and that weren't sure if a control dependency was really guaranteed or not.

That is *exactly* the kinds of thing that _automation_ should handle. Not some human. Figure the data/control dependencies out from the code, not from some logical level.

I saw the contortions that the ISO C people tried to go through just to describe control and data dependencies. It was awful. It should have never been described on that kind of level to begin with, when it would have been much easier to just describe it to compiler people as "this is a consume relationship". The same rules apply here. Don't make it about some high-level thing and humans annontating things. Make it about the actual generated code.

Quoted 9 lines · Dmitry Vyukov

A long thread and I lost track somewhat, but, yes, from KTSAN (data race detector) point of view we will need a way to understand when things are ordered due to data and control dependencies (i.e.effectively acquire but only wrt the dependent stuff). There is a logic level and physical level, it's perfectly fine if on physical level all these _DEP/_CTRL variants are no-op (the same as READ_ONCE, at least on todays archs with todays compilers). But on logical level they represent a well defined, meaningful thing.

No, they do not.

Do you believe in fairies and Santa Claus?

Because quite frankly, the likelihood of either of those being true is _way_ higher than the likelihood of any normal human ever getting those things right.

So asking a programmer to annotate whether two memory accesses have a data dependency or a control dependency is completely inappropriate. You won't get people understanding it to begin with, much less then figure out subtle things like whether a control dependency is an actual branch, or might be turned into a data dependency through select, or whatever.

We've had really smart people who wrote core code that couldn't get it right, and that weren't sure if a control dependency was really guaranteed or not.

That is *exactly* the kinds of thing that _automation_ should handle. Not some human. Figure the data/control dependencies out from the code, not from some logical level.

I saw the contortions that the ISO C people tried to go through just to describe control and data dependencies. It was awful. It should have never been described on that kind of level to begin with, when it would have been much easier to just describe it to compiler people as "this is a consume relationship". The same rules apply here. Don't make it about some high-level thing and humans annontating things. Make it about the actual generated code.

Quoted 4 lines · Peter Zijlstra

Quoted 2 lines · Paul E. McKenney

Interesting. Do we currently have any dependencies headed by this_cpu_read()?

Quoted 1 line

Nope, but almost, look for: cpufreq_update_util_data.

This, you mean?

data = rcu_dereference_sched(*this_cpu_ptr(&cpufreq_update_util_data));

True, a non-RCU dependency use case would want to use ACCESS_ONCE() in the new regime.

Quoted 25 lines · Peter Zijlstra

Quoted 11 lines · Paul E. McKenney

Quoted 3 lines · David Howells

I will, however, quibble with the appropriateness of the name READ_ONCE()... I still think it's not sufficiently obvious that this is a barrier and the barrier is after. Maybe READ_AND_BARRIER()?

Linus was unhappy with READ_ONCE_CTRL() to tag control dependencies, but indicated that he might consider it if it helped code-analysis tools. Adding Dmitry Vyukov for his thoughts on whether tagging READ_ONCE() for dependencies would help. Me, I would suggest READ_ONCE_DEP(), but let's figure out if the bikeshed needs to be painted before arguing over the color. ;-)

Count me one vote for the READ_ONCE() name. This is about dependent reads, which are nothing special on anything except Alpha.

We want to remove the exception/specialness from the memory model; and therefore have to fix up all primitives that could possibly be used for these reads to unconditionally issue the barrier (on Alpha). The alternative is: rm -rf arch/alpha.

Adding something like READ_ONCE_DEP() does not rid us of the idea that dependent reads are special and thus defeats the purpose, we might as well retain lockless_dereference().

Quoted 5 lines

Now; any user of dependent reads must use READ_ONCE() in any case, to avoid load tearing and reloads. So using READ_ONCE() for the dependent reads is not extra or additional (note we'll also have to add the barrier to all our relaxed and release atomics and anything else that implies READ_ONCE and doesn't already imply smp_mb() after).

Add the per-cpu ops to that list, they imply READ_ONCE(). Consider for example this example:

	for_each_possible_cpu(cpu)
		smp_store_release(per_cpu_ptr(&foo, cpu), obj);

-vs-

	obj = this_cpu_read(foo);
	if (obj->ponies)
		fart_rainbow(obj);

Quoted 11 lines · Paul E. McKenney

Quoted 3 lines · David Howells

I will, however, quibble with the appropriateness of the name READ_ONCE()... I still think it's not sufficiently obvious that this is a barrier and the barrier is after. Maybe READ_AND_BARRIER()?

Linus was unhappy with READ_ONCE_CTRL() to tag control dependencies, but indicated that he might consider it if it helped code-analysis tools. Adding Dmitry Vyukov for his thoughts on whether tagging READ_ONCE() for dependencies would help. Me, I would suggest READ_ONCE_DEP(), but let's figure out if the bikeshed needs to be painted before arguing over the color. ;-)

Count me one vote for the READ_ONCE() name. This is about dependent reads, which are nothing special on anything except Alpha.

lib/assoc_array.c void *assoc_array_find(const struct assoc_array *array, · line 322

Quoted 3 lines · Paul E. McKenney

lib/assoc_array.c +1 -2

-	node = result.terminal_node.node;
-	smp_read_barrier_depends();
+	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */

The main problem I have with this method of annotation is that it's not obvious there's a barrier there or which side the barrier is.

I think one of the trickiest issues is that a barrier is typically between two things and we're not making it clear what those two things actually are.

Also, I would say that the most natural interpretation of READ_ONCE() is that the implicit barrier comes after the read, e.g.:

	f = READ_ONCE(stuff->foo);
	/* Implied barrier */
	look_at(f->a);
	look_at(f->b);

I.e. READ_ONCE() prevents stuff->foo from being reread whilst you access f and orders LOAD(stuff->foo) before LOAD(f->a) and LOAD(f->b).

Placing the comment on the same line makes it less likely that some later change will move the comment away from the load that it applies to.

Quoted 2 lines · Paul E. McKenney

Placing the comment on the same line makes it less likely that some later change will move the comment away from the load that it applies to.

The problem with your 'address dep' comment is that it's not particularly useful.

Either your comment needs to say "dep between X and Y", but if the following is always the dep:

	Y = READ_ONCE(X)
	access(*Y)

then the comment is superfluous.

If it's not always true then your comment needs to indicate what the dependency is.

The other thing your comment could/should say is where the other barrier is - barriers always have to be paired as a general rule. (I know I haven't put these comments in here - but I've been doing that recently).

READ_ONCE() now implies smp_read_barrier_depends(), which means that the instances in arpt_do_table(), ipt_do_table(), and ip6t_do_table() are now redundant. This commit removes them and adjusts the comments.

Quoted 3 lines · Paul E. McKenney

READ_ONCE() now implies smp_read_barrier_depends(), which means that the instances in arpt_do_table(), ipt_do_table(), and ip6t_do_table() are now redundant. This commit removes them and adjusts the comments.

Similar to the previous patch, the lack of READ_ONCE() in the original code is a pre-existing bug. It would allow the compiler to tear the load and observe a composite of two difference pointer values, or reload the private pointer and result in table_base and jumpstacl being part of different objects.

It would be good to point out this actually fixes a bug in the code.

Will Deacon has proposed adding smp_read_barrier_depends() to READ_ONCE(), which would mean that quite a few instances of smp_read_barrier_depends() would become redundant.

Quoted 3 lines · Paul E. McKenney

Will Deacon has proposed adding smp_read_barrier_depends() to READ_ONCE(), which would mean that quite a few instances of smp_read_barrier_depends() would become redundant.

It's not clear from you description where the barrier is added in relation to the read: before, after or both?