Re: [PATCH] documentation: Fix two-CPU control-dependency example

[Boqun Feng]

On Sat, Jul 22, 2017 at 09:43:00PM -0700, Paul E. McKenney wrote:
[...]
> > Your priority seemed to be in reducing the chance of the "if" statement
> > to be optimized away.  So I suggested to use "extern" as a compromise.
> 

Hi Akira,

The problem is that, such a compromise doesn't help *developers* write
good concurrent code. The document should serve as a reference book for
the developers, and with the compromise you suggest, the developers will
possibly add "extern" to their shared variables. This is not only
unrealistic but also wrong, because "extern" means external for
translation units(compiling units), not external for execution
units(CPUs).

And as I said, the proper semantics of READ_ONCE() should work well
without using "extern", if we find a 'volatile' load doesn't work, we
can find another way (writing in asm or use asm volatile("" : "+m"(var));
to indicate @var changed). And the compromise just changes the
semantics... To me, it's not worth changing the semantics because the
implementation might be broken in the feature ;-)


> If the various tools accept the "extern", this might not be a bad thing
> to do.
> 
> But what this really means is that I need to take another tilt at
> the "volatile" windmill in the committee.
> 
> > Another way would be to express the ">=" version in a pseudo-asm form.
> > 
> > 	CPU 0                     CPU 1
> > 	=======================   =======================
> > 	r1 = LOAD x               r2 = LOAD y
> > 	if (r1 >= 0)              if (r2 >= 0)
> > 	  STORE y = 1               STORE x = 1
> > 
> > 	assert(!(r1 == 1 && r2 == 1));
> > 
> > This should eliminate any concern of compiler optimization.
> > In this final part of CONTROL DEPENDENCIES section, separating the
> > problem of optimization and transitivity would clarify the point
> > (at least for me).
> 
> The problem is that people really do use C-language control dependencies
> in the Linux kernel, so we need to describe them.  Maybe someday it
> will be necessary to convert them to asm, but I am hoping that we can
> avoid that.
> 
> > Thoughts?
> 
> My hope is that the memory model can help here, but that will in any
> case take time.

Hi Paul,

I add some comments for READ_ONCE() to emphasize compilers should honor
the return value, in the future, we may need a separate document for the
use/definition of volatile in kernel, but I think the comment of
READ_ONCE() is good enough now?

Regards,
Boqun

----------------->8
Subject: [PATCH] kernel: Emphasize the return value of READ_ONCE() is honored

READ_ONCE() is used around in kernel to provide a control dependency,
and to make the control dependency valid, we must 1) make the load of
READ_ONCE() actually happen and 2) make sure compilers take the return
value of READ_ONCE() serious. 1) is already done and commented,
and in current implementation, 2) is also considered done in the
same way as 1): a 'volatile' load.

Whereas, Akira Yokosawa recently reported a problem that would be
triggered if 2) is not achieved. Moreover, according to Paul Mckenney,
using volatile might not actually give us what we want for 2) depending
on compiler writers' definition of 'volatile'. Therefore it's necessary
to emphasize 2) as a part of the semantics of READ_ONCE(), this not only
fits the conceptual semantics we have been using, but also makes the
implementation requirement more accurate.

In the future, we can either make compiler writers accept our use of
'volatile', or(if that fails) find another way to provide this
guarantee.

Cc: Akira Yokosawa <akiyks@xxxxxxxxx>
Cc: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx>
---
 include/linux/compiler.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 219f82f3ec1a..8094f594427c 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -305,6 +305,31 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  * mutilate accesses that either do not require ordering or that interact
  * with an explicit memory barrier or atomic instruction that provides the
  * required ordering.
+ *
+ * The return value of READ_ONCE() should be honored by compilers, IOW,
+ * compilers must treat the return value of READ_ONCE() as an unknown value at
+ * compile time, i.e. no optimization should be done based on the value of a
+ * READ_ONCE(). For example, the following code snippet:
+ *
+ * 	int a = 0;
+ * 	int x = 0;
+ *
+ * 	void some_func() {
+ * 		int t = READ_ONCE(a);
+ * 		if (!t)
+ * 			WRITE_ONCE(x, 1);
+ * 	}
+ *
+ * , should never be optimized as:
+ *
+ * 	void some_func() {
+ * 		WRITE_ONCE(x, 1);
+ * 	}
+ *
+ * because the compiler is 'smart' enough to think the value of 'a' is never
+ * changed.
+ *
+ * We provide this guarantee by making READ_ONCE() a *volatile* load.
  */
 
 #define __READ_ONCE(x, check)						\
-- 
2.13.3