diff mbox series

[13/13] rcutorture: formal: prepare for ACCESS_ONCE() removal

Message ID 1507573730-8083-14-git-send-email-mark.rutland@arm.com
State New
Headers show
Series Preparatory work to kill off ACCESS_ONCE() | expand

Commit Message

Mark Rutland Oct. 9, 2017, 6:28 p.m. UTC 
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.

The bulk of the kernel code can be transformed via Coccinelle to use
{READ,WRITE}_ONCE(), though this only modifies users of ACCESS_ONCE(),
and not the implementation itself. As such, it has the potential to
break homebrew ACCESS_ONCE() macros seen in some user code in the kernel
tree (e.g. the virtio code, as fixed in commit ea9156fb3b71d9f7).

To avoid fragility if/when that transformation occurs, this patch
reworks the rcutorture formal tests to use an intermediate
__ACCESS_ONCE() helper, which will avoid {READ,WRITE}_ONCE() being
turned into tautological definitions. There should be no functional
change as a result of this patch.

Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>

---
 tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
1.9.1

Comments

Paul E. McKenney Oct. 9, 2017, 7:51 p.m. UTC | #1 
On Mon, Oct 09, 2017 at 07:28:50PM +0100, Mark Rutland wrote:
> 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.

> 

> The bulk of the kernel code can be transformed via Coccinelle to use

> {READ,WRITE}_ONCE(), though this only modifies users of ACCESS_ONCE(),

> and not the implementation itself. As such, it has the potential to

> break homebrew ACCESS_ONCE() macros seen in some user code in the kernel

> tree (e.g. the virtio code, as fixed in commit ea9156fb3b71d9f7).

> 

> To avoid fragility if/when that transformation occurs, this patch

> reworks the rcutorture formal tests to use an intermediate

> __ACCESS_ONCE() helper, which will avoid {READ,WRITE}_ONCE() being

> turned into tautological definitions. There should be no functional

> change as a result of this patch.

> 

> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

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

> ---

>  tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h | 7 ++++---

>  1 file changed, 4 insertions(+), 3 deletions(-)

> 

> diff --git a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> index 6687acc..ee4e4f8 100644

> --- a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> +++ b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> @@ -34,8 +34,9 @@

>  #define rs_smp_mb() do {} while (0)

>  #endif

> 

> -#define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> -#define READ_ONCE(x) ACCESS_ONCE(x)

> -#define WRITE_ONCE(x, val) (ACCESS_ONCE(x) = (val))

> +#define __ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> +#define ACCESS_ONCE(x) __ACCESS_ONCE(x)

> +#define READ_ONCE(x) __ACCESS_ONCE(x)

> +#define WRITE_ONCE(x, val) (__ACCESS_ONCE(x) = (val))


How about something like the following?

#define READ_ONCE(x) (*(volatile typeof(x) *) &(x))
#define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))


							Thanx, Paul
Mark Rutland Oct. 10, 2017, 9:54 a.m. UTC | #2 
On Mon, Oct 09, 2017 at 12:51:12PM -0700, Paul E. McKenney wrote:
> On Mon, Oct 09, 2017 at 07:28:50PM +0100, Mark Rutland wrote:

> > diff --git a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > index 6687acc..ee4e4f8 100644

> > --- a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > +++ b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > @@ -34,8 +34,9 @@

> >  #define rs_smp_mb() do {} while (0)

> >  #endif

> > 

> > -#define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > -#define READ_ONCE(x) ACCESS_ONCE(x)

> > -#define WRITE_ONCE(x, val) (ACCESS_ONCE(x) = (val))

> > +#define __ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > +#define ACCESS_ONCE(x) __ACCESS_ONCE(x)

> > +#define READ_ONCE(x) __ACCESS_ONCE(x)

> > +#define WRITE_ONCE(x, val) (__ACCESS_ONCE(x) = (val))

> 

> How about something like the following?

> 

> #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))


Sure; folded in and pushed out. :)

I've assumed that the ACCESS_ONCE() definition needs to be kept until
that's ripped out treewide. Please shout if that's not the case!

Thanks,
Mark.
Paul E. McKenney Oct. 10, 2017, 12:47 p.m. UTC | #3 
On Tue, Oct 10, 2017 at 10:54:14AM +0100, Mark Rutland wrote:
> On Mon, Oct 09, 2017 at 12:51:12PM -0700, Paul E. McKenney wrote:

> > On Mon, Oct 09, 2017 at 07:28:50PM +0100, Mark Rutland wrote:

> > > diff --git a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > index 6687acc..ee4e4f8 100644

> > > --- a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > +++ b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > @@ -34,8 +34,9 @@

> > >  #define rs_smp_mb() do {} while (0)

> > >  #endif

> > > 

> > > -#define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > -#define READ_ONCE(x) ACCESS_ONCE(x)

> > > -#define WRITE_ONCE(x, val) (ACCESS_ONCE(x) = (val))

> > > +#define __ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > +#define ACCESS_ONCE(x) __ACCESS_ONCE(x)

> > > +#define READ_ONCE(x) __ACCESS_ONCE(x)

> > > +#define WRITE_ONCE(x, val) (__ACCESS_ONCE(x) = (val))

> > 

> > How about something like the following?

> > 

> > #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> > #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

> 

> Sure; folded in and pushed out. :)


Thank you!

> I've assumed that the ACCESS_ONCE() definition needs to be kept until

> that's ripped out treewide. Please shout if that's not the case!


You have it right.  This case is an exception because this code is
used only by RCU, which has long since had ACCESS_ONCE() ripped out.

							Thanx, Paul
Mark Rutland Oct. 10, 2017, 12:50 p.m. UTC | #4 
On Tue, Oct 10, 2017 at 05:47:12AM -0700, Paul E. McKenney wrote:
> On Tue, Oct 10, 2017 at 10:54:14AM +0100, Mark Rutland wrote:

> > On Mon, Oct 09, 2017 at 12:51:12PM -0700, Paul E. McKenney wrote:

> > > On Mon, Oct 09, 2017 at 07:28:50PM +0100, Mark Rutland wrote:

> > > > diff --git a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > index 6687acc..ee4e4f8 100644

> > > > --- a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > +++ b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > @@ -34,8 +34,9 @@

> > > >  #define rs_smp_mb() do {} while (0)

> > > >  #endif

> > > > 

> > > > -#define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > -#define READ_ONCE(x) ACCESS_ONCE(x)

> > > > -#define WRITE_ONCE(x, val) (ACCESS_ONCE(x) = (val))

> > > > +#define __ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > +#define ACCESS_ONCE(x) __ACCESS_ONCE(x)

> > > > +#define READ_ONCE(x) __ACCESS_ONCE(x)

> > > > +#define WRITE_ONCE(x, val) (__ACCESS_ONCE(x) = (val))

> > > 

> > > How about something like the following?

> > > 

> > > #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

> > 

> > Sure; folded in and pushed out. :)

> 

> Thank you!

> 

> > I've assumed that the ACCESS_ONCE() definition needs to be kept until

> > that's ripped out treewide. Please shout if that's not the case!

> 

> You have it right.  This case is an exception because this code is

> used only by RCU, which has long since had ACCESS_ONCE() ripped out.


Sorry; I meant that in this case, I leave this code as:

#define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))
#define READ_ONCE(x) (*(volatile typeof(x) *) &(x))
#define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

... if you mean that we can drop ACCESS_ONCE() in this case, then I can
rip that out.

Thanks,
Mark.
Paul E. McKenney Oct. 10, 2017, 2:52 p.m. UTC | #5 
On Tue, Oct 10, 2017 at 01:50:39PM +0100, Mark Rutland wrote:
> On Tue, Oct 10, 2017 at 05:47:12AM -0700, Paul E. McKenney wrote:

> > On Tue, Oct 10, 2017 at 10:54:14AM +0100, Mark Rutland wrote:

> > > On Mon, Oct 09, 2017 at 12:51:12PM -0700, Paul E. McKenney wrote:

> > > > On Mon, Oct 09, 2017 at 07:28:50PM +0100, Mark Rutland wrote:

> > > > > diff --git a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > > index 6687acc..ee4e4f8 100644

> > > > > --- a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > > +++ b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > > @@ -34,8 +34,9 @@

> > > > >  #define rs_smp_mb() do {} while (0)

> > > > >  #endif

> > > > > 

> > > > > -#define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > > -#define READ_ONCE(x) ACCESS_ONCE(x)

> > > > > -#define WRITE_ONCE(x, val) (ACCESS_ONCE(x) = (val))

> > > > > +#define __ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > > +#define ACCESS_ONCE(x) __ACCESS_ONCE(x)

> > > > > +#define READ_ONCE(x) __ACCESS_ONCE(x)

> > > > > +#define WRITE_ONCE(x, val) (__ACCESS_ONCE(x) = (val))

> > > > 

> > > > How about something like the following?

> > > > 

> > > > #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

> > > 

> > > Sure; folded in and pushed out. :)

> > 

> > Thank you!

> > 

> > > I've assumed that the ACCESS_ONCE() definition needs to be kept until

> > > that's ripped out treewide. Please shout if that's not the case!

> > 

> > You have it right.  This case is an exception because this code is

> > used only by RCU, which has long since had ACCESS_ONCE() ripped out.

> 

> Sorry; I meant that in this case, I leave this code as:

> 

> #define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

> 

> ... if you mean that we can drop ACCESS_ONCE() in this case, then I can

> rip that out.


We can drop ACCESS_ONCE() in this case.

							Thanx, Paul
Mark Rutland Oct. 10, 2017, 4:24 p.m. UTC | #6 
On Tue, Oct 10, 2017 at 07:52:52AM -0700, Paul E. McKenney wrote:
> On Tue, Oct 10, 2017 at 01:50:39PM +0100, Mark Rutland wrote:

> > On Tue, Oct 10, 2017 at 05:47:12AM -0700, Paul E. McKenney wrote:

> > > On Tue, Oct 10, 2017 at 10:54:14AM +0100, Mark Rutland wrote:


> > > > I've assumed that the ACCESS_ONCE() definition needs to be kept until

> > > > that's ripped out treewide. Please shout if that's not the case!

> > > 

> > > You have it right.  This case is an exception because this code is

> > > used only by RCU, which has long since had ACCESS_ONCE() ripped out.

> > 

> > Sorry; I meant that in this case, I leave this code as:

> > 

> > #define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> > #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

> > 

> > ... if you mean that we can drop ACCESS_ONCE() in this case, then I can

> > rip that out.

> 

> We can drop ACCESS_ONCE() in this case.


Done. Sorry for the confusion!

Thanks,
Mark.
Joe Perches Oct. 10, 2017, 4:27 p.m. UTC | #7 
On Tue, 2017-10-10 at 07:52 -0700, Paul E. McKenney wrote:
> On Tue, Oct 10, 2017 at 01:50:39PM +0100, Mark Rutland wrote:

> > On Tue, Oct 10, 2017 at 05:47:12AM -0700, Paul E. McKenney wrote:

> > > On Tue, Oct 10, 2017 at 10:54:14AM +0100, Mark Rutland wrote:

> > > > On Mon, Oct 09, 2017 at 12:51:12PM -0700, Paul E. McKenney wrote:

> > > > > On Mon, Oct 09, 2017 at 07:28:50PM +0100, Mark Rutland wrote:

> > > > > > diff --git a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > > > index 6687acc..ee4e4f8 100644

> > > > > > --- a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > > > +++ b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > > > @@ -34,8 +34,9 @@

> > > > > >  #define rs_smp_mb() do {} while (0)

> > > > > >  #endif

> > > > > > 

> > > > > > -#define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > > > -#define READ_ONCE(x) ACCESS_ONCE(x)

> > > > > > -#define WRITE_ONCE(x, val) (ACCESS_ONCE(x) = (val))

> > > > > > +#define __ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > > > +#define ACCESS_ONCE(x) __ACCESS_ONCE(x)

> > > > > > +#define READ_ONCE(x) __ACCESS_ONCE(x)

> > > > > > +#define WRITE_ONCE(x, val) (__ACCESS_ONCE(x) = (val))

> > > > > 

> > > > > How about something like the following?

> > > > > 

> > > > > #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > > #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

> > > > 

> > > > Sure; folded in and pushed out. :)

> > > 

> > > Thank you!

> > > 

> > > > I've assumed that the ACCESS_ONCE() definition needs to be kept until

> > > > that's ripped out treewide. Please shout if that's not the case!

> > > 

> > > You have it right.  This case is an exception because this code is

> > > used only by RCU, which has long since had ACCESS_ONCE() ripped out.

> > 

> > Sorry; I meant that in this case, I leave this code as:

> > 

> > #define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> > #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

> > 

> > ... if you mean that we can drop ACCESS_ONCE() in this case, then I can

> > rip that out.

> 

> We can drop ACCESS_ONCE() in this case.


Once ACCESS_ONCE is removed from the code in the tree
it can also be removed from checkpatch
---
 scripts/checkpatch.pl | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2a8c6c3c1bdb..440262a87d26 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6224,28 +6224,6 @@ sub process {
 			}
 		}
 
-# whine about ACCESS_ONCE
-		if ($^V && $^V ge 5.10.0 &&
-		    $line =~ /\bACCESS_ONCE\s*$balanced_parens\s*(=(?!=))?\s*($FuncArg)?/) {
-			my $par = $1;
-			my $eq = $2;
-			my $fun = $3;
-			$par =~ s/^\(\s*(.*)\s*\)$/$1/;
-			if (defined($eq)) {
-				if (WARN("PREFER_WRITE_ONCE",
-					 "Prefer WRITE_ONCE(<FOO>, <BAR>) over ACCESS_ONCE(<FOO>) = <BAR>\n" . $herecurr) &&
-				    $fix) {
-					$fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)\s*$eq\s*\Q$fun\E/WRITE_ONCE($par, $fun)/;
-				}
-			} else {
-				if (WARN("PREFER_READ_ONCE",
-					 "Prefer READ_ONCE(<FOO>) over ACCESS_ONCE(<FOO>)\n" . $herecurr) &&
-				    $fix) {
-					$fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)/READ_ONCE($par)/;
-				}
-			}
-		}
-
 # check for mutex_trylock_recursive usage
 		if ($line =~ /mutex_trylock_recursive/) {
 			ERROR("LOCKING",
Paul E. McKenney Oct. 10, 2017, 4:35 p.m. UTC | #8 
On Tue, Oct 10, 2017 at 09:27:54AM -0700, Joe Perches wrote:
> On Tue, 2017-10-10 at 07:52 -0700, Paul E. McKenney wrote:

> > On Tue, Oct 10, 2017 at 01:50:39PM +0100, Mark Rutland wrote:

> > > On Tue, Oct 10, 2017 at 05:47:12AM -0700, Paul E. McKenney wrote:

> > > > On Tue, Oct 10, 2017 at 10:54:14AM +0100, Mark Rutland wrote:

> > > > > On Mon, Oct 09, 2017 at 12:51:12PM -0700, Paul E. McKenney wrote:

> > > > > > On Mon, Oct 09, 2017 at 07:28:50PM +0100, Mark Rutland wrote:

> > > > > > > diff --git a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > > > > index 6687acc..ee4e4f8 100644

> > > > > > > --- a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > > > > +++ b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > > > > @@ -34,8 +34,9 @@

> > > > > > >  #define rs_smp_mb() do {} while (0)

> > > > > > >  #endif

> > > > > > > 

> > > > > > > -#define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > > > > -#define READ_ONCE(x) ACCESS_ONCE(x)

> > > > > > > -#define WRITE_ONCE(x, val) (ACCESS_ONCE(x) = (val))

> > > > > > > +#define __ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > > > > +#define ACCESS_ONCE(x) __ACCESS_ONCE(x)

> > > > > > > +#define READ_ONCE(x) __ACCESS_ONCE(x)

> > > > > > > +#define WRITE_ONCE(x, val) (__ACCESS_ONCE(x) = (val))

> > > > > > 

> > > > > > How about something like the following?

> > > > > > 

> > > > > > #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > > > #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

> > > > > 

> > > > > Sure; folded in and pushed out. :)

> > > > 

> > > > Thank you!

> > > > 

> > > > > I've assumed that the ACCESS_ONCE() definition needs to be kept until

> > > > > that's ripped out treewide. Please shout if that's not the case!

> > > > 

> > > > You have it right.  This case is an exception because this code is

> > > > used only by RCU, which has long since had ACCESS_ONCE() ripped out.

> > > 

> > > Sorry; I meant that in this case, I leave this code as:

> > > 

> > > #define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

> > > 

> > > ... if you mean that we can drop ACCESS_ONCE() in this case, then I can

> > > rip that out.

> > 

> > We can drop ACCESS_ONCE() in this case.

> 

> Once ACCESS_ONCE is removed from the code in the tree

> it can also be removed from checkpatch


Agreed, but only after all ACCESS_ONCE() definitions are removed.

								Thanx, Paul

> ---

>  scripts/checkpatch.pl | 22 ----------------------

>  1 file changed, 22 deletions(-)

> 

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

> index 2a8c6c3c1bdb..440262a87d26 100755

> --- a/scripts/checkpatch.pl

> +++ b/scripts/checkpatch.pl

> @@ -6224,28 +6224,6 @@ sub process {

>  			}

>  		}

>  

> -# whine about ACCESS_ONCE

> -		if ($^V && $^V ge 5.10.0 &&

> -		    $line =~ /\bACCESS_ONCE\s*$balanced_parens\s*(=(?!=))?\s*($FuncArg)?/) {

> -			my $par = $1;

> -			my $eq = $2;

> -			my $fun = $3;

> -			$par =~ s/^\(\s*(.*)\s*\)$/$1/;

> -			if (defined($eq)) {

> -				if (WARN("PREFER_WRITE_ONCE",

> -					 "Prefer WRITE_ONCE(<FOO>, <BAR>) over ACCESS_ONCE(<FOO>) = <BAR>\n" . $herecurr) &&

> -				    $fix) {

> -					$fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)\s*$eq\s*\Q$fun\E/WRITE_ONCE($par, $fun)/;

> -				}

> -			} else {

> -				if (WARN("PREFER_READ_ONCE",

> -					 "Prefer READ_ONCE(<FOO>) over ACCESS_ONCE(<FOO>)\n" . $herecurr) &&

> -				    $fix) {

> -					$fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)/READ_ONCE($par)/;

> -				}

> -			}

> -		}

> -

>  # check for mutex_trylock_recursive usage

>  		if ($line =~ /mutex_trylock_recursive/) {

>  			ERROR("LOCKING",

>
Paul E. McKenney Oct. 10, 2017, 4:35 p.m. UTC | #9 
On Tue, Oct 10, 2017 at 05:24:42PM +0100, Mark Rutland wrote:
> On Tue, Oct 10, 2017 at 07:52:52AM -0700, Paul E. McKenney wrote:

> > On Tue, Oct 10, 2017 at 01:50:39PM +0100, Mark Rutland wrote:

> > > On Tue, Oct 10, 2017 at 05:47:12AM -0700, Paul E. McKenney wrote:

> > > > On Tue, Oct 10, 2017 at 10:54:14AM +0100, Mark Rutland wrote:

> 

> > > > > I've assumed that the ACCESS_ONCE() definition needs to be kept until

> > > > > that's ripped out treewide. Please shout if that's not the case!

> > > > 

> > > > You have it right.  This case is an exception because this code is

> > > > used only by RCU, which has long since had ACCESS_ONCE() ripped out.

> > > 

> > > Sorry; I meant that in this case, I leave this code as:

> > > 

> > > #define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

> > > 

> > > ... if you mean that we can drop ACCESS_ONCE() in this case, then I can

> > > rip that out.

> > 

> > We can drop ACCESS_ONCE() in this case.

> 

> Done. Sorry for the confusion!


Very good, thank you!

							Thanx, Paul
Mark Rutland Oct. 10, 2017, 4:41 p.m. UTC | #10 
On Tue, Oct 10, 2017 at 09:27:54AM -0700, Joe Perches wrote:
> On Tue, 2017-10-10 at 07:52 -0700, Paul E. McKenney wrote:

> > On Tue, Oct 10, 2017 at 01:50:39PM +0100, Mark Rutland wrote:

> > > On Tue, Oct 10, 2017 at 05:47:12AM -0700, Paul E. McKenney wrote:

> > > > On Tue, Oct 10, 2017 at 10:54:14AM +0100, Mark Rutland wrote:

> > > > > On Mon, Oct 09, 2017 at 12:51:12PM -0700, Paul E. McKenney wrote:

> > > > > > On Mon, Oct 09, 2017 at 07:28:50PM +0100, Mark Rutland wrote:

> > > > > > > diff --git a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > > > > index 6687acc..ee4e4f8 100644

> > > > > > > --- a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > > > > +++ b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h

> > > > > > > @@ -34,8 +34,9 @@

> > > > > > >  #define rs_smp_mb() do {} while (0)

> > > > > > >  #endif

> > > > > > > 

> > > > > > > -#define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > > > > -#define READ_ONCE(x) ACCESS_ONCE(x)

> > > > > > > -#define WRITE_ONCE(x, val) (ACCESS_ONCE(x) = (val))

> > > > > > > +#define __ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > > > > +#define ACCESS_ONCE(x) __ACCESS_ONCE(x)

> > > > > > > +#define READ_ONCE(x) __ACCESS_ONCE(x)

> > > > > > > +#define WRITE_ONCE(x, val) (__ACCESS_ONCE(x) = (val))

> > > > > > 

> > > > > > How about something like the following?

> > > > > > 

> > > > > > #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > > > > #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

> > > > > 

> > > > > Sure; folded in and pushed out. :)

> > > > 

> > > > Thank you!

> > > > 

> > > > > I've assumed that the ACCESS_ONCE() definition needs to be kept until

> > > > > that's ripped out treewide. Please shout if that's not the case!

> > > > 

> > > > You have it right.  This case is an exception because this code is

> > > > used only by RCU, which has long since had ACCESS_ONCE() ripped out.

> > > 

> > > Sorry; I meant that in this case, I leave this code as:

> > > 

> > > #define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > #define READ_ONCE(x) (*(volatile typeof(x) *) &(x))

> > > #define WRITE_ONCE(x) ((*(volatile typeof(x) *) &(x)) = (val))

> > > 

> > > ... if you mean that we can drop ACCESS_ONCE() in this case, then I can

> > > rip that out.

> > 

> > We can drop ACCESS_ONCE() in this case.

> 

> Once ACCESS_ONCE is removed from the code in the tree

> it can also be removed from checkpatch


Sure thing. We're expecting to rip that out with the ACCESS_ONCE
definitions in a post-Coccinelle patch.

If you're happy to give a sign-off for the below, we can drop that into
the post-coccienelle patch/series.

Thanks,
Mark.

> ---

>  scripts/checkpatch.pl | 22 ----------------------

>  1 file changed, 22 deletions(-)

> 

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

> index 2a8c6c3c1bdb..440262a87d26 100755

> --- a/scripts/checkpatch.pl

> +++ b/scripts/checkpatch.pl

> @@ -6224,28 +6224,6 @@ sub process {

>  			}

>  		}

>  

> -# whine about ACCESS_ONCE

> -		if ($^V && $^V ge 5.10.0 &&

> -		    $line =~ /\bACCESS_ONCE\s*$balanced_parens\s*(=(?!=))?\s*($FuncArg)?/) {

> -			my $par = $1;

> -			my $eq = $2;

> -			my $fun = $3;

> -			$par =~ s/^\(\s*(.*)\s*\)$/$1/;

> -			if (defined($eq)) {

> -				if (WARN("PREFER_WRITE_ONCE",

> -					 "Prefer WRITE_ONCE(<FOO>, <BAR>) over ACCESS_ONCE(<FOO>) = <BAR>\n" . $herecurr) &&

> -				    $fix) {

> -					$fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)\s*$eq\s*\Q$fun\E/WRITE_ONCE($par, $fun)/;

> -				}

> -			} else {

> -				if (WARN("PREFER_READ_ONCE",

> -					 "Prefer READ_ONCE(<FOO>) over ACCESS_ONCE(<FOO>)\n" . $herecurr) &&

> -				    $fix) {

> -					$fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)/READ_ONCE($par)/;

> -				}

> -			}

> -		}

> -

>  # check for mutex_trylock_recursive usage

>  		if ($line =~ /mutex_trylock_recursive/) {

>  			ERROR("LOCKING",

>
Joe Perches Oct. 10, 2017, 4:57 p.m. UTC | #11 
On Tue, 2017-10-10 at 17:41 +0100, Mark Rutland wrote:
> On Tue, Oct 10, 2017 at 09:27:54AM -0700, Joe Perches wrote:

> > Once ACCESS_ONCE is removed from the code in the tree

> > it can also be removed from checkpatch

> 

> Sure thing. We're expecting to rip that out with the ACCESS_ONCE

> definitions in a post-Coccinelle patch.


This treewide series is one of the types that would be nice to
have done at -rc1 via some external automated script service.
Mark Rutland Oct. 10, 2017, 5:06 p.m. UTC | #12 
On Tue, Oct 10, 2017 at 09:57:25AM -0700, Joe Perches wrote:
> On Tue, 2017-10-10 at 17:41 +0100, Mark Rutland wrote:

> > On Tue, Oct 10, 2017 at 09:27:54AM -0700, Joe Perches wrote:

> > > Once ACCESS_ONCE is removed from the code in the tree

> > > it can also be removed from checkpatch

> > 

> > Sure thing. We're expecting to rip that out with the ACCESS_ONCE

> > definitions in a post-Coccinelle patch.

> 

> This treewide series is one of the types that would be nice to

> have done at -rc1 via some external automated script service.


Agreed, and this is exactly the plan.

As mentioned above, the treewide conversion is scripted with Coccinelle
(script below). The plan would be to regenerate this at an rc boundary,
once the preparatory patches (which cannot be scripted) have been
merged.

The ACCESS_ONCE() definitions and checkpatch bits would be removed in a
subsequent patch or series of patches.

Thanks,
Mark.

----
// Convert trivial ACCESS_ONCE() uses to equivalent READ_ONCE() and
// WRITE_ONCE()

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)
----
diff mbox series

Patch

diff --git a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h
index 6687acc..ee4e4f8 100644
--- a/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h
+++ b/tools/testing/selftests/rcutorture/formal/srcu-cbmc/src/barriers.h
@@ -34,8 +34,9 @@ 
 #define rs_smp_mb() do {} while (0)
 #endif
 
-#define ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))
-#define READ_ONCE(x) ACCESS_ONCE(x)
-#define WRITE_ONCE(x, val) (ACCESS_ONCE(x) = (val))
+#define __ACCESS_ONCE(x) (*(volatile typeof(x) *) &(x))
+#define ACCESS_ONCE(x) __ACCESS_ONCE(x)
+#define READ_ONCE(x) __ACCESS_ONCE(x)
+#define WRITE_ONCE(x, val) (__ACCESS_ONCE(x) = (val))
 
 #endif