[09/19] net: average: Kill off ACCESS_ONCE()

Message ID 1508792849-3115-9-git-send-email-paulmck@linux.vnet.ibm.com
State New
Headers show
Series
  • [01/19] dm integrity: Kill off ACCESS_ONCE()
Related show

Commit Message

Paul E. McKenney Oct. 23, 2017, 9:07 p.m.
From: Mark Rutland <mark.rutland@arm.com>


For several reasons, it is desirable to use {READ,WRITE}_ONCE() in
preference to ACCESS_ONCE(), and new code is expected to use one of the
former. So far, there's been no reason to change most existing uses of
ACCESS_ONCE(), as these aren't currently harmful.

However, for some features it is necessary to instrument reads and
writes separately, which is not possible with ACCESS_ONCE(). This
distinction is critical to correct operation.

It's possible to transform the bulk of kernel code using the Coccinelle
script below. However, this doesn't pick up some uses, including those
in <linux/average.h>. As a preparatory step, this patch converts the
file to use {READ,WRITE}_ONCE() consistently.

At the same time, this patch addds missing includes necessary for
{READ,WRITE}_ONCE(), *BUG_ON*(), and ilog2().

----
virtual patch

@ depends on patch @
expression E1, E2;
@@

- ACCESS_ONCE(E1) = E2
+ WRITE_ONCE(E1, E2)

@ depends on patch @
expression E;
@@

- ACCESS_ONCE(E)
+ READ_ONCE(E)
----

Signed-off-by: Mark Rutland <mark.rutland@arm.com>

Cc: Johannes Berg <johannes.berg@intel.com>
Cc: David S. Miller <davem@davemloft.net>
---
 include/linux/average.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

-- 
2.5.2

Comments

Johannes Berg Oct. 24, 2017, 5:58 a.m. | #1
On Mon, 2017-10-23 at 21:07 +0000, Paul E. McKenney wrote:
> From: Mark Rutland <mark.rutland@arm.com>

> 

> For several reasons, it is desirable to use {READ,WRITE}_ONCE() in

> preference to ACCESS_ONCE(), and new code is expected to use one of the

> former. So far, there's been no reason to change most existing uses of

> ACCESS_ONCE(), as these aren't currently harmful.

> 

> However, for some features it is necessary to instrument reads and

> writes separately, which is not possible with ACCESS_ONCE(). This

> distinction is critical to correct operation.

> 

> It's possible to transform the bulk of kernel code using the Coccinelle

> script below. However, this doesn't pick up some uses, including those

> in <linux/average.h>. As a preparatory step, this patch converts the

> file to use {READ,WRITE}_ONCE() consistently.

> 

> At the same time, this patch addds missing includes necessary for

> {READ,WRITE}_ONCE(), *BUG_ON*(), and ilog2().

> 

> ----

> virtual patch

> 

> @ depends on patch @

> expression E1, E2;

> @@

> 

> - ACCESS_ONCE(E1) = E2

> + WRITE_ONCE(E1, E2)

> 

> @ depends on patch @

> expression E;

> @@

> 

> - ACCESS_ONCE(E)

> + READ_ONCE(E)

> ----

> 

> Signed-off-by: Mark Rutland <mark.rutland@arm.com>

> Cc: Johannes Berg <johannes.berg@intel.com>


Reviewed-by: Johannes Berg <johannes@sipsolutions.net>


Let me know if you want me to apply this, since I seem to be the
average.h maintainer :-)

johannes
Ingo Molnar Oct. 24, 2017, 9:34 a.m. | #2
* Johannes Berg <johannes@sipsolutions.net> wrote:

> On Mon, 2017-10-23 at 21:07 +0000, Paul E. McKenney wrote:

> > From: Mark Rutland <mark.rutland@arm.com>

> > 

> > For several reasons, it is desirable to use {READ,WRITE}_ONCE() in

> > preference to ACCESS_ONCE(), and new code is expected to use one of the

> > former. So far, there's been no reason to change most existing uses of

> > ACCESS_ONCE(), as these aren't currently harmful.

> > 

> > However, for some features it is necessary to instrument reads and

> > writes separately, which is not possible with ACCESS_ONCE(). This

> > distinction is critical to correct operation.

> > 

> > It's possible to transform the bulk of kernel code using the Coccinelle

> > script below. However, this doesn't pick up some uses, including those

> > in <linux/average.h>. As a preparatory step, this patch converts the

> > file to use {READ,WRITE}_ONCE() consistently.

> > 

> > At the same time, this patch addds missing includes necessary for

> > {READ,WRITE}_ONCE(), *BUG_ON*(), and ilog2().

> > 

> > ----

> > virtual patch

> > 

> > @ depends on patch @

> > expression E1, E2;

> > @@

> > 

> > - ACCESS_ONCE(E1) = E2

> > + WRITE_ONCE(E1, E2)

> > 

> > @ depends on patch @

> > expression E;

> > @@

> > 

> > - ACCESS_ONCE(E)

> > + READ_ONCE(E)

> > ----

> > 

> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>

> > Cc: Johannes Berg <johannes.berg@intel.com>

> 

> Reviewed-by: Johannes Berg <johannes@sipsolutions.net>


Thanks!

> Let me know if you want me to apply this, since I seem to be the

> average.h maintainer :-)


Would be nice to keep these patches together if possible, so that we can remove 
ACCESS_ONCE() - or at least add a build warning.

Thanks,

	Ingo
Paul E. McKenney Oct. 24, 2017, 1:49 p.m. | #3
On Tue, Oct 24, 2017 at 11:34:13AM +0200, Ingo Molnar wrote:
> 

> * Johannes Berg <johannes@sipsolutions.net> wrote:

> 

> > On Mon, 2017-10-23 at 21:07 +0000, Paul E. McKenney wrote:

> > > From: Mark Rutland <mark.rutland@arm.com>

> > > 

> > > For several reasons, it is desirable to use {READ,WRITE}_ONCE() in

> > > preference to ACCESS_ONCE(), and new code is expected to use one of the

> > > former. So far, there's been no reason to change most existing uses of

> > > ACCESS_ONCE(), as these aren't currently harmful.

> > > 

> > > However, for some features it is necessary to instrument reads and

> > > writes separately, which is not possible with ACCESS_ONCE(). This

> > > distinction is critical to correct operation.

> > > 

> > > It's possible to transform the bulk of kernel code using the Coccinelle

> > > script below. However, this doesn't pick up some uses, including those

> > > in <linux/average.h>. As a preparatory step, this patch converts the

> > > file to use {READ,WRITE}_ONCE() consistently.

> > > 

> > > At the same time, this patch addds missing includes necessary for

> > > {READ,WRITE}_ONCE(), *BUG_ON*(), and ilog2().

> > > 

> > > ----

> > > virtual patch

> > > 

> > > @ depends on patch @

> > > expression E1, E2;

> > > @@

> > > 

> > > - ACCESS_ONCE(E1) = E2

> > > + WRITE_ONCE(E1, E2)

> > > 

> > > @ depends on patch @

> > > expression E;

> > > @@

> > > 

> > > - ACCESS_ONCE(E)

> > > + READ_ONCE(E)

> > > ----

> > > 

> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>

> > > Cc: Johannes Berg <johannes.berg@intel.com>

> > 

> > Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

> 

> Thanks!

> 

> > Let me know if you want me to apply this, since I seem to be the

> > average.h maintainer :-)

> 

> Would be nice to keep these patches together if possible, so that we can remove 

> ACCESS_ONCE() - or at least add a build warning.


My goal is to remove it.  After all, I am the guy who added it.  ;-)

							Thanx, Paul

Patch

diff --git a/include/linux/average.h b/include/linux/average.h
index 7ddaf340d2ac..3f462292269c 100644
--- a/include/linux/average.h
+++ b/include/linux/average.h
@@ -1,6 +1,10 @@ 
 #ifndef _LINUX_AVERAGE_H
 #define _LINUX_AVERAGE_H
 
+#include <linux/bug.h>
+#include <linux/compiler.h>
+#include <linux/log2.h>
+
 /*
  * Exponentially weighted moving average (EWMA)
  *
@@ -48,7 +52,7 @@ 
 	static inline void ewma_##name##_add(struct ewma_##name *e,	\
 					     unsigned long val)		\
 	{								\
-		unsigned long internal = ACCESS_ONCE(e->internal);	\
+		unsigned long internal = READ_ONCE(e->internal);	\
 		unsigned long weight_rcp = ilog2(_weight_rcp);		\
 		unsigned long precision = _precision;			\
 									\
@@ -57,10 +61,10 @@ 
 		BUILD_BUG_ON((_precision) > 30);			\
 		BUILD_BUG_ON_NOT_POWER_OF_2(_weight_rcp);		\
 									\
-		ACCESS_ONCE(e->internal) = internal ?			\
+		WRITE_ONCE(e->internal, internal ?			\
 			(((internal << weight_rcp) - internal) +	\
 				(val << precision)) >> weight_rcp :	\
-			(val << precision);				\
+			(val << precision));				\
 	}
 
 #endif /* _LINUX_AVERAGE_H */