[PATCH 00/13] Preparatory work to kill off ACCESS_ONCE()

[PATCH 00/13] Preparatory work to kill off ACCESS_ONCE()

[Mark Rutland]

Hi all,

There's a general want to kill off ACCESS_ONCE(), which is required to kill off
smp_read_barrier_depends(), and to support debug features which require
instrumenting reads and writes separately.

Thanks to preparatory work by a number of people, it's largely possible to
script this with the Coccinelle patch below. However, this breaks a handful of
cases, and renders some comments stale.

This series fixes up said cases, and comments. Where fixups have been made,
I've converted the entire file for consistency. The remaining code can be
converted by Coccinelle script, allowing for the subsequent removal of
ACCESS_ONCE().

I've pushed this series, complete with an example treewide conversion of
v4.14-rc4 to my core/access-once branch [1,2].

Thanks,
Mark.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git core/access-once
[2] https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=core/access-once

----
// 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)
*** BLURB HERE ***
----

Mark Rutland (13):
  dm integrity: kill off ACCESS_ONCE()
  EDAC, altera: kill off ACCESS_ONCE()
  firmware/ivc: kill off ACCESS_ONCE()
  fs: dcache: kill off ACCESS_ONCE()
  fs: ncpfs: kill off ACCESS_ONCE()
  media: dvb_ringbuffer: kill off ACCESS_ONCE()
  net: netlink/netfilter: kill off ACCESS_ONCE()
  net/ipv4/tcp_input.c: kill off ACCESS_ONCE()
  net: average: kill off ACCESS_ONCE()
  samples: mic/mpssd/mpssd.c: kill off ACCESS_ONCE()
  selftests/powerpc: kill off ACCESS_ONCE()
  workqueue: kill off ACCESS_ONCE()
  rcutorture: formal: prepare for ACCESS_ONCE() removal

 drivers/edac/altera_edac.c                         | 10 ++++-----
 drivers/firmware/tegra/ivc.c                       | 24 +++++++++++-----------
 drivers/md/dm-integrity.c                          | 15 +++++++-------
 drivers/media/dvb-core/dvb_ringbuffer.c            |  8 ++++----
 fs/dcache.c                                        | 18 ++++++++--------
 fs/ncpfs/dir.c                                     |  9 --------
 include/linux/average.h                            | 10 ++++++---
 include/linux/dcache.h                             |  4 ++--
 include/linux/genetlink.h                          |  2 +-
 include/linux/netfilter/nfnetlink.h                |  2 +-
 include/linux/rtnetlink.h                          |  2 +-
 include/net/netfilter/nf_tables.h                  |  4 ++--
 kernel/workqueue.c                                 |  4 ++--
 net/ipv4/tcp_input.c                               |  6 +++---
 net/netfilter/ipvs/ip_vs_sync.c                    |  2 +-
 net/netfilter/nfnetlink_queue.c                    |  4 ++--
 samples/mic/mpssd/mpssd.c                          |  6 +++---
 tools/testing/selftests/powerpc/dscr/dscr.h        |  2 +-
 .../selftests/powerpc/dscr/dscr_default_test.c     |  2 +-
 .../rcutorture/formal/srcu-cbmc/src/barriers.h     |  7 ++++---
 20 files changed, 69 insertions(+), 72 deletions(-)

-- 
1.9.1

[PATCH 01/13] dm integrity: kill off ACCESS_ONCE()

[Mark Rutland]

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 dm-integrity.c. As a preparatory step, this patch converts the driver
to use {READ,WRITE}_ONCE() consistently.

At the same time, this patch adds the missing include of
<linux/compiler.h> necessary for the {READ,WRITE}_ONCE() definitions.

----
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: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-integrity.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 096fe9b..8c5756e 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -6,6 +6,7 @@
  * This file is released under the GPL.
  */
 
+#include <linux/compiler.h>
 #include <linux/module.h>
 #include <linux/device-mapper.h>
 #include <linux/dm-io.h>
@@ -80,13 +81,13 @@ struct journal_entry {
 #define journal_entry_tag(ic, je)		((__u8 *)&(je)->last_bytes[(ic)->sectors_per_block])
 
 #if BITS_PER_LONG == 64
-#define journal_entry_set_sector(je, x)		do { smp_wmb(); ACCESS_ONCE((je)->u.sector) = cpu_to_le64(x); } while (0)
+#define journal_entry_set_sector(je, x)		do { smp_wmb(); WRITE_ONCE((je)->u.sector, cpu_to_le64(x)); } while (0)
 #define journal_entry_get_sector(je)		le64_to_cpu((je)->u.sector)
 #elif defined(CONFIG_LBDAF)
-#define journal_entry_set_sector(je, x)		do { (je)->u.s.sector_lo = cpu_to_le32(x); smp_wmb(); ACCESS_ONCE((je)->u.s.sector_hi) = cpu_to_le32((x) >> 32); } while (0)
+#define journal_entry_set_sector(je, x)		do { (je)->u.s.sector_lo = cpu_to_le32(x); smp_wmb(); WRITE_ONCE((je)->u.s.sector_hi, cpu_to_le32((x) >> 32)); } while (0)
 #define journal_entry_get_sector(je)		le64_to_cpu((je)->u.sector)
 #else
-#define journal_entry_set_sector(je, x)		do { (je)->u.s.sector_lo = cpu_to_le32(x); smp_wmb(); ACCESS_ONCE((je)->u.s.sector_hi) = cpu_to_le32(0); } while (0)
+#define journal_entry_set_sector(je, x)		do { (je)->u.s.sector_lo = cpu_to_le32(x); smp_wmb(); WRITE_ONCE((je)->u.s.sector_hi, cpu_to_le32(0)); } while (0)
 #define journal_entry_get_sector(je)		le32_to_cpu((je)->u.s.sector_lo)
 #endif
 #define journal_entry_is_unused(je)		((je)->u.s.sector_hi == cpu_to_le32(-1))
@@ -320,7 +321,7 @@ static void dm_integrity_io_error(struct dm_integrity_c *ic, const char *msg, in
 
 static int dm_integrity_failed(struct dm_integrity_c *ic)
 {
-	return ACCESS_ONCE(ic->failed);
+	return READ_ONCE(ic->failed);
 }
 
 static commit_id_t dm_integrity_commit_id(struct dm_integrity_c *ic, unsigned i,
@@ -1545,7 +1546,7 @@ static bool __journal_read_write(struct dm_integrity_io *dio, struct bio *bio,
 		smp_mb();
 		if (unlikely(waitqueue_active(&ic->copy_to_journal_wait)))
 			wake_up(&ic->copy_to_journal_wait);
-		if (ACCESS_ONCE(ic->free_sectors) <= ic->free_sectors_threshold) {
+		if (READ_ONCE(ic->free_sectors) <= ic->free_sectors_threshold) {
 			queue_work(ic->commit_wq, &ic->commit_work);
 		} else {
 			schedule_autocommit(ic);
@@ -1798,7 +1799,7 @@ static void integrity_commit(struct work_struct *w)
 	ic->n_committed_sections += commit_sections;
 	spin_unlock_irq(&ic->endio_wait.lock);
 
-	if (ACCESS_ONCE(ic->free_sectors) <= ic->free_sectors_threshold)
+	if (READ_ONCE(ic->free_sectors) <= ic->free_sectors_threshold)
 		queue_work(ic->writer_wq, &ic->writer_work);
 
 release_flush_bios:
@@ -1980,7 +1981,7 @@ static void integrity_writer(struct work_struct *w)
 	unsigned prev_free_sectors;
 
 	/* the following test is not needed, but it tests the replay code */
-	if (ACCESS_ONCE(ic->suspending))
+	if (READ_ONCE(ic->suspending))
 		return;
 
 	spin_lock_irq(&ic->endio_wait.lock);
-- 
1.9.1

[PATCH 02/13] EDAC, altera: kill off ACCESS_ONCE()

[Mark Rutland]

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 handle comments, leaving references
to ACCESS_ONCE() instances which have been removed. As a preparatory
step, this patch converts the Altera EDAC code and comments to use
{READ,WRITE}_ONCE() consistently.

----
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: Borislav Petkov <bp@alien8.de>
Cc: Thor Thayer <thor.thayer@linux.intel.com>
---
 drivers/edac/altera_edac.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 346c498..11d6419 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -175,11 +175,11 @@ static ssize_t altr_sdr_mc_err_inject_write(struct file *file,
 	/*
 	 * To trigger the error, we need to read the data back
 	 * (the data was written with errors above).
-	 * The ACCESS_ONCE macros and printk are used to prevent the
+	 * The READ_ONCE macros and printk are used to prevent the
 	 * the compiler optimizing these reads out.
 	 */
-	reg = ACCESS_ONCE(ptemp[0]);
-	read_reg = ACCESS_ONCE(ptemp[1]);
+	reg = READ_ONCE(ptemp[0]);
+	read_reg = READ_ONCE(ptemp[1]);
 	/* Force Read */
 	rmb();
 
@@ -618,7 +618,7 @@ static ssize_t altr_edac_device_trig(struct file *file,
 	for (i = 0; i < (priv->trig_alloc_sz / sizeof(*ptemp)); i++) {
 		/* Read data so we're in the correct state */
 		rmb();
-		if (ACCESS_ONCE(ptemp[i]))
+		if (READ_ONCE(ptemp[i]))
 			result = -1;
 		/* Toggle Error bit (it is latched), leave ECC enabled */
 		writel(error_mask, (drvdata->base + priv->set_err_ofst));
@@ -635,7 +635,7 @@ static ssize_t altr_edac_device_trig(struct file *file,
 
 	/* Read out written data. ECC error caused here */
 	for (i = 0; i < ALTR_TRIGGER_READ_WRD_CNT; i++)
-		if (ACCESS_ONCE(ptemp[i]) != i)
+		if (READ_ONCE(ptemp[i]) != i)
 			edac_printk(KERN_ERR, EDAC_DEVICE,
 				    "Read doesn't match written data\n");
 
-- 
1.9.1

Re: [PATCH 02/13] EDAC, altera: kill off ACCESS_ONCE()

[Thor Thayer]

On 10/09/2017 01:28 PM, 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.
> 
> It's possible to transform the bulk of kernel code using the Coccinelle
> script below. However, this doesn't handle comments, leaving references
> to ACCESS_ONCE() instances which have been removed. As a preparatory
> step, this patch converts the Altera EDAC code and comments to use
> {READ,WRITE}_ONCE() consistently.
> 
> ----
> 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: Borislav Petkov <bp@alien8.de>
> Cc: Thor Thayer <thor.thayer@linux.intel.com>
> ---
>   drivers/edac/altera_edac.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index 346c498..11d6419 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
> @@ -175,11 +175,11 @@ static ssize_t altr_sdr_mc_err_inject_write(struct file *file,
>   	/*
>   	 * To trigger the error, we need to read the data back
>   	 * (the data was written with errors above).
> -	 * The ACCESS_ONCE macros and printk are used to prevent the
> +	 * The READ_ONCE macros and printk are used to prevent the
>   	 * the compiler optimizing these reads out.
>   	 */
> -	reg = ACCESS_ONCE(ptemp[0]);
> -	read_reg = ACCESS_ONCE(ptemp[1]);
> +	reg = READ_ONCE(ptemp[0]);
> +	read_reg = READ_ONCE(ptemp[1]);
>   	/* Force Read */
>   	rmb();
>   
> @@ -618,7 +618,7 @@ static ssize_t altr_edac_device_trig(struct file *file,
>   	for (i = 0; i < (priv->trig_alloc_sz / sizeof(*ptemp)); i++) {
>   		/* Read data so we're in the correct state */
>   		rmb();
> -		if (ACCESS_ONCE(ptemp[i]))
> +		if (READ_ONCE(ptemp[i]))
>   			result = -1;
>   		/* Toggle Error bit (it is latched), leave ECC enabled */
>   		writel(error_mask, (drvdata->base + priv->set_err_ofst));
> @@ -635,7 +635,7 @@ static ssize_t altr_edac_device_trig(struct file *file,
>   
>   	/* Read out written data. ECC error caused here */
>   	for (i = 0; i < ALTR_TRIGGER_READ_WRD_CNT; i++)
> -		if (ACCESS_ONCE(ptemp[i]) != i)
> +		if (READ_ONCE(ptemp[i]) != i)
>   			edac_printk(KERN_ERR, EDAC_DEVICE,
>   				    "Read doesn't match written data\n");
>   
> 

Tested on Cyclone5 and Arria10. Thanks!
Acked-by: Thor Thayer <thor.thayer@linux.intel.com>

Re: [PATCH 02/13] EDAC, altera: kill off ACCESS_ONCE()

[Mark Rutland]

On Mon, Oct 09, 2017 at 06:12:10PM -0500, Thor Thayer wrote:
> On 10/09/2017 01:28 PM, Mark Rutland wrote:
> >diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> >index 346c498..11d6419 100644
> >--- a/drivers/edac/altera_edac.c
> >+++ b/drivers/edac/altera_edac.c
> >@@ -175,11 +175,11 @@ static ssize_t altr_sdr_mc_err_inject_write(struct file *file,
> >  	/*
> >  	 * To trigger the error, we need to read the data back
> >  	 * (the data was written with errors above).
> >-	 * The ACCESS_ONCE macros and printk are used to prevent the
> >+	 * The READ_ONCE macros and printk are used to prevent the
> >  	 * the compiler optimizing these reads out.
> >  	 */
> >-	reg = ACCESS_ONCE(ptemp[0]);
> >-	read_reg = ACCESS_ONCE(ptemp[1]);
> >+	reg = READ_ONCE(ptemp[0]);
> >+	read_reg = READ_ONCE(ptemp[1]);
> >  	/* Force Read */
> >  	rmb();
> >@@ -618,7 +618,7 @@ static ssize_t altr_edac_device_trig(struct file *file,
> >  	for (i = 0; i < (priv->trig_alloc_sz / sizeof(*ptemp)); i++) {
> >  		/* Read data so we're in the correct state */
> >  		rmb();
> >-		if (ACCESS_ONCE(ptemp[i]))
> >+		if (READ_ONCE(ptemp[i]))
> >  			result = -1;
> >  		/* Toggle Error bit (it is latched), leave ECC enabled */
> >  		writel(error_mask, (drvdata->base + priv->set_err_ofst));
> >@@ -635,7 +635,7 @@ static ssize_t altr_edac_device_trig(struct file *file,
> >  	/* Read out written data. ECC error caused here */
> >  	for (i = 0; i < ALTR_TRIGGER_READ_WRD_CNT; i++)
> >-		if (ACCESS_ONCE(ptemp[i]) != i)
> >+		if (READ_ONCE(ptemp[i]) != i)
> >  			edac_printk(KERN_ERR, EDAC_DEVICE,
> >  				    "Read doesn't match written data\n");
> >
> 
> Tested on Cyclone5 and Arria10. Thanks!
> Acked-by: Thor Thayer <thor.thayer@linux.intel.com>

Thanks, that's much appreciated!

Mark.

[PATCH 03/13] firmware/ivc: kill off ACCESS_ONCE()

[Mark Rutland]

workqueue: kill off ACCESS_ONCE()

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 handle comments, leaving references
to ACCESS_ONCE() instances which have been removed. As a preparatory
step, this patch converts the Tegra IVC code and comments to use
{READ,WRITE}_ONCE() consistently.

----
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: Jonathan Hunter <jonathanh@nvidia.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/firmware/tegra/ivc.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/tegra/ivc.c b/drivers/firmware/tegra/ivc.c
index a01461d..00de793 100644
--- a/drivers/firmware/tegra/ivc.c
+++ b/drivers/firmware/tegra/ivc.c
@@ -99,11 +99,11 @@ static inline bool tegra_ivc_empty(struct tegra_ivc *ivc,
 {
 	/*
 	 * This function performs multiple checks on the same values with
-	 * security implications, so create snapshots with ACCESS_ONCE() to
+	 * security implications, so create snapshots with READ_ONCE() to
 	 * ensure that these checks use the same values.
 	 */
-	u32 tx = ACCESS_ONCE(header->tx.count);
-	u32 rx = ACCESS_ONCE(header->rx.count);
+	u32 tx = READ_ONCE(header->tx.count);
+	u32 rx = READ_ONCE(header->rx.count);
 
 	/*
 	 * Perform an over-full check to prevent denial of service attacks
@@ -124,8 +124,8 @@ static inline bool tegra_ivc_empty(struct tegra_ivc *ivc,
 static inline bool tegra_ivc_full(struct tegra_ivc *ivc,
 				  struct tegra_ivc_header *header)
 {
-	u32 tx = ACCESS_ONCE(header->tx.count);
-	u32 rx = ACCESS_ONCE(header->rx.count);
+	u32 tx = READ_ONCE(header->tx.count);
+	u32 rx = READ_ONCE(header->rx.count);
 
 	/*
 	 * Invalid cases where the counters indicate that the queue is over
@@ -137,8 +137,8 @@ static inline bool tegra_ivc_full(struct tegra_ivc *ivc,
 static inline u32 tegra_ivc_available(struct tegra_ivc *ivc,
 				      struct tegra_ivc_header *header)
 {
-	u32 tx = ACCESS_ONCE(header->tx.count);
-	u32 rx = ACCESS_ONCE(header->rx.count);
+	u32 tx = READ_ONCE(header->tx.count);
+	u32 rx = READ_ONCE(header->rx.count);
 
 	/*
 	 * This function isn't expected to be used in scenarios where an
@@ -151,8 +151,8 @@ static inline u32 tegra_ivc_available(struct tegra_ivc *ivc,
 
 static inline void tegra_ivc_advance_tx(struct tegra_ivc *ivc)
 {
-	ACCESS_ONCE(ivc->tx.channel->tx.count) =
-		ACCESS_ONCE(ivc->tx.channel->tx.count) + 1;
+	WRITE_ONCE(ivc->tx.channel->tx.count,
+		   READ_ONCE(ivc->tx.channel->tx.count) + 1);
 
 	if (ivc->tx.position == ivc->num_frames - 1)
 		ivc->tx.position = 0;
@@ -162,8 +162,8 @@ static inline void tegra_ivc_advance_tx(struct tegra_ivc *ivc)
 
 static inline void tegra_ivc_advance_rx(struct tegra_ivc *ivc)
 {
-	ACCESS_ONCE(ivc->rx.channel->rx.count) =
-		ACCESS_ONCE(ivc->rx.channel->rx.count) + 1;
+	WRITE_ONCE(ivc->rx.channel->rx.count,
+		   READ_ONCE(ivc->rx.channel->rx.count) + 1);
 
 	if (ivc->rx.position == ivc->num_frames - 1)
 		ivc->rx.position = 0;
@@ -428,7 +428,7 @@ int tegra_ivc_notified(struct tegra_ivc *ivc)
 
 	/* Copy the receiver's state out of shared memory. */
 	tegra_ivc_invalidate(ivc, ivc->rx.phys + offset);
-	state = ACCESS_ONCE(ivc->rx.channel->tx.state);
+	state = READ_ONCE(ivc->rx.channel->tx.state);
 
 	if (state == TEGRA_IVC_STATE_SYNC) {
 		offset = offsetof(struct tegra_ivc_header, tx.count);
-- 
1.9.1

[PATCH 04/13] fs: dcache: kill off ACCESS_ONCE()

[Mark Rutland]

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 handle comments, leaving references
to ACCESS_ONCE() instances which have been removed. As a preparatory
step, this patch converts the dcache code and comments to use
{READ,WRITE}_ONCE() consistently.

----
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: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 fs/dcache.c            | 18 +++++++++---------
 include/linux/dcache.h |  4 ++--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index f901413..f283cd3 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -630,7 +630,7 @@ static inline struct dentry *lock_parent(struct dentry *dentry)
 	rcu_read_lock();
 	spin_unlock(&dentry->d_lock);
 again:
-	parent = ACCESS_ONCE(dentry->d_parent);
+	parent = READ_ONCE(dentry->d_parent);
 	spin_lock(&parent->d_lock);
 	/*
 	 * We can't blindly lock dentry until we are sure
@@ -721,7 +721,7 @@ static inline bool fast_dput(struct dentry *dentry)
 	 * around with a zero refcount.
 	 */
 	smp_rmb();
-	d_flags = ACCESS_ONCE(dentry->d_flags);
+	d_flags = READ_ONCE(dentry->d_flags);
 	d_flags &= DCACHE_REFERENCED | DCACHE_LRU_LIST | DCACHE_DISCONNECTED;
 
 	/* Nothing to do? Dropping the reference was all we needed? */
@@ -850,11 +850,11 @@ struct dentry *dget_parent(struct dentry *dentry)
 	 * locking.
 	 */
 	rcu_read_lock();
-	ret = ACCESS_ONCE(dentry->d_parent);
+	ret = READ_ONCE(dentry->d_parent);
 	gotref = lockref_get_not_zero(&ret->d_lockref);
 	rcu_read_unlock();
 	if (likely(gotref)) {
-		if (likely(ret == ACCESS_ONCE(dentry->d_parent)))
+		if (likely(ret == READ_ONCE(dentry->d_parent)))
 			return ret;
 		dput(ret);
 	}
@@ -3040,7 +3040,7 @@ static int prepend(char **buffer, int *buflen, const char *str, int namelen)
  * @buflen: allocated length of the buffer
  * @name:   name string and length qstr structure
  *
- * With RCU path tracing, it may race with d_move(). Use ACCESS_ONCE() to
+ * With RCU path tracing, it may race with d_move(). Use READ_ONCE() to
  * make sure that either the old or the new name pointer and length are
  * fetched. However, there may be mismatch between length and pointer.
  * The length cannot be trusted, we need to copy it byte-by-byte until
@@ -3054,8 +3054,8 @@ static int prepend(char **buffer, int *buflen, const char *str, int namelen)
  */
 static int prepend_name(char **buffer, int *buflen, const struct qstr *name)
 {
-	const char *dname = ACCESS_ONCE(name->name);
-	u32 dlen = ACCESS_ONCE(name->len);
+	const char *dname = READ_ONCE(name->name);
+	u32 dlen = READ_ONCE(name->len);
 	char *p;
 
 	smp_read_barrier_depends();
@@ -3120,7 +3120,7 @@ static int prepend_path(const struct path *path,
 		struct dentry * parent;
 
 		if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
-			struct mount *parent = ACCESS_ONCE(mnt->mnt_parent);
+			struct mount *parent = READ_ONCE(mnt->mnt_parent);
 			/* Escaped? */
 			if (dentry != vfsmnt->mnt_root) {
 				bptr = *buffer;
@@ -3130,7 +3130,7 @@ static int prepend_path(const struct path *path,
 			}
 			/* Global root? */
 			if (mnt != parent) {
-				dentry = ACCESS_ONCE(mnt->mnt_mountpoint);
+				dentry = READ_ONCE(mnt->mnt_mountpoint);
 				mnt = parent;
 				vfsmnt = &mnt->mnt;
 				continue;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index ed1a7cf..1d8f581 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -519,7 +519,7 @@ static inline struct inode *d_inode(const struct dentry *dentry)
 }
 
 /**
- * d_inode_rcu - Get the actual inode of this dentry with ACCESS_ONCE()
+ * d_inode_rcu - Get the actual inode of this dentry with READ_ONCE()
  * @dentry: The dentry to query
  *
  * This is the helper normal filesystems should use to get at their own inodes
@@ -527,7 +527,7 @@ static inline struct inode *d_inode(const struct dentry *dentry)
  */
 static inline struct inode *d_inode_rcu(const struct dentry *dentry)
 {
-	return ACCESS_ONCE(dentry->d_inode);
+	return READ_ONCE(dentry->d_inode);
 }
 
 /**
-- 
1.9.1

[PATCH 05/13] fs: ncpfs: kill off ACCESS_ONCE()

[Mark Rutland]

The NCPFS code has some stale comments regarding ACCESS_ONCE() uses
which were removed a long time ago.

Let's remove the stale comments.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Petr Vandrovec <petr@vandrovec.name>
---
 fs/ncpfs/dir.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
index 088f524..72cfaa2 100644
--- a/fs/ncpfs/dir.c
+++ b/fs/ncpfs/dir.c
@@ -119,10 +119,6 @@ static inline int ncp_case_sensitive(const struct inode *i)
 /*
  * Note: leave the hash unchanged if the directory
  * is case-sensitive.
- *
- * Accessing the parent inode can be racy under RCU pathwalking.
- * Use ACCESS_ONCE() to make sure we use _one_ particular inode,
- * the callers will handle races.
  */
 static int 
 ncp_hash_dentry(const struct dentry *dentry, struct qstr *this)
@@ -147,11 +143,6 @@ static inline int ncp_case_sensitive(const struct inode *i)
 	return 0;
 }
 
-/*
- * Accessing the parent inode can be racy under RCU pathwalking.
- * Use ACCESS_ONCE() to make sure we use _one_ particular inode,
- * the callers will handle races.
- */
 static int
 ncp_compare_dentry(const struct dentry *dentry,
 		unsigned int len, const char *str, const struct qstr *name)
-- 
1.9.1

[PATCH 06/13] media: dvb_ringbuffer: kill off ACCESS_ONCE()

[Mark Rutland]

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 handle comments, leaving references
to ACCESS_ONCE() instances which have been removed. As a preparatory
step, this patch converts the DVB ringbuffer code and comments to use
{READ,WRITE}_ONCE() consistently.

----
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: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/dvb-core/dvb_ringbuffer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_ringbuffer.c b/drivers/media/dvb-core/dvb_ringbuffer.c
index 2322af1..5301162 100644
--- a/drivers/media/dvb-core/dvb_ringbuffer.c
+++ b/drivers/media/dvb-core/dvb_ringbuffer.c
@@ -66,12 +66,12 @@ ssize_t dvb_ringbuffer_free(struct dvb_ringbuffer *rbuf)
 {
 	ssize_t free;
 
-	/* ACCESS_ONCE() to load read pointer on writer side
+	/* READ_ONCE() to load read pointer on writer side
 	 * this pairs with smp_store_release() in dvb_ringbuffer_read(),
 	 * dvb_ringbuffer_read_user(), dvb_ringbuffer_flush(),
 	 * or dvb_ringbuffer_reset()
 	 */
-	free = ACCESS_ONCE(rbuf->pread) - rbuf->pwrite;
+	free = READ_ONCE(rbuf->pread) - rbuf->pwrite;
 	if (free <= 0)
 		free += rbuf->size;
 	return free-1;
@@ -143,7 +143,7 @@ ssize_t dvb_ringbuffer_read_user(struct dvb_ringbuffer *rbuf, u8 __user *buf, si
 		todo -= split;
 		/* smp_store_release() for read pointer update to ensure
 		 * that buf is not overwritten until read is complete,
-		 * this pairs with ACCESS_ONCE() in dvb_ringbuffer_free()
+		 * this pairs with READ_ONCE() in dvb_ringbuffer_free()
 		 */
 		smp_store_release(&rbuf->pread, 0);
 	}
@@ -168,7 +168,7 @@ void dvb_ringbuffer_read(struct dvb_ringbuffer *rbuf, u8 *buf, size_t len)
 		todo -= split;
 		/* smp_store_release() for read pointer update to ensure
 		 * that buf is not overwritten until read is complete,
-		 * this pairs with ACCESS_ONCE() in dvb_ringbuffer_free()
+		 * this pairs with READ_ONCE() in dvb_ringbuffer_free()
 		 */
 		smp_store_release(&rbuf->pread, 0);
 	}
-- 
1.9.1

[PATCH 07/13] net: netlink/netfilter: kill off ACCESS_ONCE()

[Mark Rutland]

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 handle comments, leaving references
to ACCESS_ONCE() instances which have been removed. As a preparatory
step, this patch converts netlink and netfilter code and comments to use
{READ,WRITE}_ONCE() consistently.

----
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: David S. Miller <davem@davemloft.net>
Cc: Florian Westphal <fw@strlen.de>
Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/linux/genetlink.h           | 2 +-
 include/linux/netfilter/nfnetlink.h | 2 +-
 include/linux/rtnetlink.h           | 2 +-
 include/net/netfilter/nf_tables.h   | 4 ++--
 net/netfilter/ipvs/ip_vs_sync.c     | 2 +-
 net/netfilter/nfnetlink_queue.c     | 4 ++--
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/genetlink.h b/include/linux/genetlink.h
index a4c61cb..0e694cf 100644
--- a/include/linux/genetlink.h
+++ b/include/linux/genetlink.h
@@ -30,7 +30,7 @@
  * @p: The pointer to read, prior to dereferencing
  *
  * Return the value of the specified RCU-protected pointer, but omit
- * both the smp_read_barrier_depends() and the ACCESS_ONCE(), because
+ * both the smp_read_barrier_depends() and the READ_ONCE(), because
  * caller holds genl mutex.
  */
 #define genl_dereference(p)					\
diff --git a/include/linux/netfilter/nfnetlink.h b/include/linux/netfilter/nfnetlink.h
index 41d04e9..0f47a4a 100644
--- a/include/linux/netfilter/nfnetlink.h
+++ b/include/linux/netfilter/nfnetlink.h
@@ -66,7 +66,7 @@ static inline bool lockdep_nfnl_is_held(__u8 subsys_id)
  * @ss: The nfnetlink subsystem ID
  *
  * Return the value of the specified RCU-protected pointer, but omit
- * both the smp_read_barrier_depends() and the ACCESS_ONCE(), because
+ * both the smp_read_barrier_depends() and the READ_ONCE(), because
  * caller holds the NFNL subsystem mutex.
  */
 #define nfnl_dereference(p, ss)					\
diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index dea59c8..765f7b9 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -67,7 +67,7 @@ static inline bool lockdep_rtnl_is_held(void)
  * @p: The pointer to read, prior to dereferencing
  *
  * Return the value of the specified RCU-protected pointer, but omit
- * both the smp_read_barrier_depends() and the ACCESS_ONCE(), because
+ * both the smp_read_barrier_depends() and the READ_ONCE(), because
  * caller holds RTNL.
  */
 #define rtnl_dereference(p)					\
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 0f5b12a..5c68e27 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1164,8 +1164,8 @@ static inline u8 nft_genmask_next(const struct net *net)
 
 static inline u8 nft_genmask_cur(const struct net *net)
 {
-	/* Use ACCESS_ONCE() to prevent refetching the value for atomicity */
-	return 1 << ACCESS_ONCE(net->nft.gencursor);
+	/* Use READ_ONCE() to prevent refetching the value for atomicity */
+	return 1 << READ_ONCE(net->nft.gencursor);
 }
 
 #define NFT_GENMASK_ANY		((1 << 0) | (1 << 1))
diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 0e5b64a..1cfffd4 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -457,7 +457,7 @@ static inline bool in_persistence(struct ip_vs_conn *cp)
 static int ip_vs_sync_conn_needed(struct netns_ipvs *ipvs,
 				  struct ip_vs_conn *cp, int pkts)
 {
-	unsigned long orig = ACCESS_ONCE(cp->sync_endtime);
+	unsigned long orig = READ_ONCE(cp->sync_endtime);
 	unsigned long now = jiffies;
 	unsigned long n = (now + cp->timeout) & ~3UL;
 	unsigned int sync_refresh_period;
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index c979662..a16356c 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -401,7 +401,7 @@ static int nfqnl_put_bridge(struct nf_queue_entry *entry, struct sk_buff *skb)
 
 	outdev = entry->state.out;
 
-	switch ((enum nfqnl_config_mode)ACCESS_ONCE(queue->copy_mode)) {
+	switch ((enum nfqnl_config_mode)READ_ONCE(queue->copy_mode)) {
 	case NFQNL_COPY_META:
 	case NFQNL_COPY_NONE:
 		break;
@@ -412,7 +412,7 @@ static int nfqnl_put_bridge(struct nf_queue_entry *entry, struct sk_buff *skb)
 		    skb_checksum_help(entskb))
 			return NULL;
 
-		data_len = ACCESS_ONCE(queue->copy_range);
+		data_len = READ_ONCE(queue->copy_range);
 		if (data_len > entskb->len)
 			data_len = entskb->len;
 
-- 
1.9.1

[PATCH 08/13] net/ipv4/tcp_input.c: kill off ACCESS_ONCE()

[Mark Rutland]

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 handle comments, leaving references
to ACCESS_ONCE() instances which have been removed. As a preparatory
step, this patch converts the IPv4 TCP input code and comments to use
{READ,WRITE}_ONCE() consistently.

----
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: David S. Miller <davem@davemloft.net>
---
 net/ipv4/tcp_input.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index c5d7656..0b3bb19 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -815,12 +815,12 @@ static void tcp_update_pacing_rate(struct sock *sk)
 	if (likely(tp->srtt_us))
 		do_div(rate, tp->srtt_us);
 
-	/* ACCESS_ONCE() is needed because sch_fq fetches sk_pacing_rate
+	/* WRITE_ONCE() is needed because sch_fq fetches sk_pacing_rate
 	 * without any lock. We want to make sure compiler wont store
 	 * intermediate values in this location.
 	 */
-	ACCESS_ONCE(sk->sk_pacing_rate) = min_t(u64, rate,
-						sk->sk_max_pacing_rate);
+	WRITE_ONCE(sk->sk_pacing_rate) = min_t(u64, rate,
+					       sk->sk_max_pacing_rate);
 }
 
 /* Calculate rto without backoff.  This is the second half of Van Jacobson's
-- 
1.9.1

Re: [PATCH 08/13] net/ipv4/tcp_input.c: kill off ACCESS_ONCE()

[Mark Rutland]

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

> -	ACCESS_ONCE(sk->sk_pacing_rate) = min_t(u64, rate,
> -						sk->sk_max_pacing_rate);
> +	WRITE_ONCE(sk->sk_pacing_rate) = min_t(u64, rate,
> +					       sk->sk_max_pacing_rate);

Sorry, I messed this up when attempting to fix the horizontal alignment
of the min_t() parameters.

I've pushed out a corrected version to my core/access-once branch [1].

Thanks,
Mark.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git core/access-once

[PATCH 09/13] net: average: kill off ACCESS_ONCE()

[Mark Rutland]

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(-)

diff --git a/include/linux/average.h b/include/linux/average.h
index 7ddaf34..7a72de4 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/ilog2.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 */
-- 
1.9.1

Re: [PATCH 09/13] net: average: kill off ACCESS_ONCE()

[Mark Rutland]

On Mon, Oct 09, 2017 at 07:28:46PM +0100, Mark Rutland wrote:
> diff --git a/include/linux/average.h b/include/linux/average.h
> index 7ddaf34..7a72de4 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/ilog2.h>

This should've been <linux/log2.h>. I've fixed that locally and pushed
out to my core/access-once branch.

Thanks,
Mark.

[PATCH 10/13] samples: mic/mpssd/mpssd.c: kill off ACCESS_ONCE()

[Mark Rutland]

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, and to align with
the preferred usage of {READ,WRITE}_ONCE(), this patch updates the MPSSD
sample code to use READ_ONCE() rather than ACCESS_ONCE(). There should
be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Shuah Khan <shuah@kernel.org>
---
 samples/mic/mpssd/mpssd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/samples/mic/mpssd/mpssd.c b/samples/mic/mpssd/mpssd.c
index 49db1de..f42ce55 100644
--- a/samples/mic/mpssd/mpssd.c
+++ b/samples/mic/mpssd/mpssd.c
@@ -65,7 +65,7 @@
 /* to align the pointer to the (next) page boundary */
 #define PAGE_ALIGN(addr)        _ALIGN(addr, PAGE_SIZE)
 
-#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
+#define READ_ONCE(x) (*(volatile typeof(x) *)&(x))
 
 #define GSO_ENABLED		1
 #define MAX_GSO_SIZE		(64 * 1024)
@@ -382,7 +382,7 @@ static inline void verify_out_len(struct mic_info *mic,
 
 static inline __u16 read_avail_idx(struct mic_vring *vr)
 {
-	return ACCESS_ONCE(vr->info->avail_idx);
+	return READ_ONCE(vr->info->avail_idx);
 }
 
 static inline void txrx_prepare(int type, bool tx, struct mic_vring *vr,
@@ -523,7 +523,7 @@ static inline unsigned _vring_size(unsigned int num, unsigned long align)
 {
 	__u16 avail_idx = read_avail_idx(vr);
 
-	while (avail_idx == le16toh(ACCESS_ONCE(vr->vr.avail->idx))) {
+	while (avail_idx == le16toh(READ_ONCE(vr->vr.avail->idx))) {
 #ifdef DEBUG
 		mpsslog("%s %s waiting for desc avail %d info_avail %d\n",
 			mic->name, __func__,
-- 
1.9.1

[PATCH 11/13] selftests/powerpc: kill off ACCESS_ONCE()

[Mark Rutland]

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, and to align with
the preferred usage of {READ,WRITE}_ONCE(), this patch updates the DSCR
selftest code to use READ_ONCE() rather than ACCESS_ONCE(). There should
be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Shuah Khan <shuah@kernel.org>
---
 tools/testing/selftests/powerpc/dscr/dscr.h              | 2 +-
 tools/testing/selftests/powerpc/dscr/dscr_default_test.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/powerpc/dscr/dscr.h b/tools/testing/selftests/powerpc/dscr/dscr.h
index 18ea223b..cdb840b 100644
--- a/tools/testing/selftests/powerpc/dscr/dscr.h
+++ b/tools/testing/selftests/powerpc/dscr/dscr.h
@@ -39,7 +39,7 @@
 #define rmb()  asm volatile("lwsync":::"memory")
 #define wmb()  asm volatile("lwsync":::"memory")
 
-#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
+#define READ_ONCE(x) (*(volatile typeof(x) *)&(x))
 
 /* Prilvilege state DSCR access */
 inline unsigned long get_dscr(void)
diff --git a/tools/testing/selftests/powerpc/dscr/dscr_default_test.c b/tools/testing/selftests/powerpc/dscr/dscr_default_test.c
index df17c3b..9e1a37e 100644
--- a/tools/testing/selftests/powerpc/dscr/dscr_default_test.c
+++ b/tools/testing/selftests/powerpc/dscr/dscr_default_test.c
@@ -27,7 +27,7 @@ static void *do_test(void *in)
 		unsigned long d, cur_dscr, cur_dscr_usr;
 		unsigned long s1, s2;
 
-		s1 = ACCESS_ONCE(sequence);
+		s1 = READ_ONCE(sequence);
 		if (s1 & 1)
 			continue;
 		rmb();
-- 
1.9.1

Re: [PATCH 11/13] selftests/powerpc: kill off ACCESS_ONCE()

[Michael Ellerman]

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

> 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, and to align with
> the preferred usage of {READ,WRITE}_ONCE(), this patch updates the DSCR
> selftest code to use READ_ONCE() rather than ACCESS_ONCE(). There should
> be no functional change as a result of this patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Shuah Khan <shuah@kernel.org>
> ---
>  tools/testing/selftests/powerpc/dscr/dscr.h              | 2 +-
>  tools/testing/selftests/powerpc/dscr/dscr_default_test.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Acked-by: Michael Ellerman <mpe@ellerman.id.au>

cheers

Re: [PATCH 11/13] selftests/powerpc: kill off ACCESS_ONCE()

[Mark Rutland]

On Tue, Oct 10, 2017 at 11:45:04AM +1100, Michael Ellerman wrote:
> Mark Rutland <mark.rutland@arm.com> writes:
> 
> > 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, and to align with
> > the preferred usage of {READ,WRITE}_ONCE(), this patch updates the DSCR
> > selftest code to use READ_ONCE() rather than ACCESS_ONCE(). There should
> > be no functional change as a result of this patch.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Shuah Khan <shuah@kernel.org>
> > ---
> >  tools/testing/selftests/powerpc/dscr/dscr.h              | 2 +-
> >  tools/testing/selftests/powerpc/dscr/dscr_default_test.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Acked-by: Michael Ellerman <mpe@ellerman.id.au>

Thanks!

Mark.

[PATCH 12/13] workqueue: kill off ACCESS_ONCE()

[Mark Rutland]

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 handle comments, leaving references
to ACCESS_ONCE() instances which have been removed. As a preparatory
step, this patch converts the workqueue code and comments to use
{READ,WRITE}_ONCE() consistently.

----
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: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
---
 kernel/workqueue.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 64d0edf..39831b2 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4647,7 +4647,7 @@ static void rebind_workers(struct worker_pool *pool)
 		 * concurrency management.  Note that when or whether
 		 * @worker clears REBOUND doesn't affect correctness.
 		 *
-		 * ACCESS_ONCE() is necessary because @worker->flags may be
+		 * WRITE_ONCE() is necessary because @worker->flags may be
 		 * tested without holding any lock in
 		 * wq_worker_waking_up().  Without it, NOT_RUNNING test may
 		 * fail incorrectly leading to premature concurrency
@@ -4656,7 +4656,7 @@ static void rebind_workers(struct worker_pool *pool)
 		WARN_ON_ONCE(!(worker_flags & WORKER_UNBOUND));
 		worker_flags |= WORKER_REBOUND;
 		worker_flags &= ~WORKER_UNBOUND;
-		ACCESS_ONCE(worker->flags) = worker_flags;
+		WRITE_ONCE(worker->flags, worker_flags);
 	}
 
 	spin_unlock_irq(&pool->lock);
-- 
1.9.1

Re: [PATCH 12/13] workqueue: kill off ACCESS_ONCE()

[Tejun Heo]

On Mon, Oct 09, 2017 at 07:28:49PM +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.
> 
> It's possible to transform the bulk of kernel code using the Coccinelle
> script below. However, this doesn't handle comments, leaving references
> to ACCESS_ONCE() instances which have been removed. As a preparatory
> step, this patch converts the workqueue code and comments to use
> {READ,WRITE}_ONCE() consistently.
...
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> Cc: Tejun Heo <tj@kernel.org>

Acked-by: Tejun Heo <tj@kernel.org>

If you want me to route it through the workqueue tree, please let me
know.

Thanks.

-- 
tejun

Re: [PATCH 12/13] workqueue: kill off ACCESS_ONCE()

[Mark Rutland]

On Tue, Oct 10, 2017 at 07:06:29AM -0700, Tejun Heo wrote:
> On Mon, Oct 09, 2017 at 07:28:49PM +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.
> > 
> > It's possible to transform the bulk of kernel code using the Coccinelle
> > script below. However, this doesn't handle comments, leaving references
> > to ACCESS_ONCE() instances which have been removed. As a preparatory
> > step, this patch converts the workqueue code and comments to use
> > {READ,WRITE}_ONCE() consistently.
> ...
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> > Cc: Tejun Heo <tj@kernel.org>
> 
> Acked-by: Tejun Heo <tj@kernel.org>

Thanks!

> If you want me to route it through the workqueue tree, please let me
> know.

I'm not sure what the plan is for merging just yet, but I will let you
know as soon as that's figured out.

Thanks,
Mark.

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

[Mark Rutland]

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))
 
 #endif
-- 
1.9.1

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

[Paul E. McKenney]

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

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

[Mark Rutland]

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.

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

[Paul E. McKenney]

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

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

[Mark Rutland]

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.

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

[Paul E. McKenney]

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

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

[Mark Rutland]

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.

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

[Paul E. McKenney]

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

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

[Joe Perches]

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",

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

[Paul E. McKenney]

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",
> 

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

[Mark Rutland]

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",
> 

Re: [Ksummit-discuss] [PATCH 13/13] rcutorture: formal: prepare for ACCESS_ONCE() removal

[Joe Perches]

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.

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

[Joe Perches]

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.

Re: [Ksummit-discuss] [PATCH 13/13] rcutorture: formal: prepare for ACCESS_ONCE() removal

[Mark Rutland]

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)
----

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

[Mark Rutland]

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)
----

Re: [PATCH 00/13] Preparatory work to kill off ACCESS_ONCE()

[Paul E. McKenney]

On Mon, Oct 09, 2017 at 07:28:37PM +0100, Mark Rutland wrote:
> Hi all,
> 
> There's a general want to kill off ACCESS_ONCE(), which is required to kill off
> smp_read_barrier_depends(), and to support debug features which require
> instrumenting reads and writes separately.
> 
> Thanks to preparatory work by a number of people, it's largely possible to
> script this with the Coccinelle patch below. However, this breaks a handful of
> cases, and renders some comments stale.
> 
> This series fixes up said cases, and comments. Where fixups have been made,
> I've converted the entire file for consistency. The remaining code can be
> converted by Coccinelle script, allowing for the subsequent removal of
> ACCESS_ONCE().
> 
> I've pushed this series, complete with an example treewide conversion of
> v4.14-rc4 to my core/access-once branch [1,2].

And this all that is left, aside from definitions and comments associated
with those definitions.  Cool!

Feel free to pull this into your preparation series.

							Thanx, Paul

------------------------------------------------------------------------

commit ed940234966f4857e23dd5f16aa8f200fc85dac6
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Mon Oct 9 12:59:18 2017 -0700

    treewide: Kill off remaining ACCESS_ONCE()
    
    This commit removes instances of ACCESS_ONCE() not located by Mark Rutland's
    coccinelle script, for example, those in complex macro definitions and
    those in comments.  Removing ACCESS_ONCE() in favor of READ_ONCE() and
    WRITE_ONCE() provides tools such as KTSAN the read/write distinction that
    they need to better detect concurrency bugs.  In addition, removing
    ACCESS_ONCE() is one necessary step towards removing special cases for
    DEC Alpha from the Linux-kernel memory model.
    
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/Documentation/filesystems/path-lookup.md b/Documentation/filesystems/path-lookup.md
index 1b39e084a2b2..50ea32a51626 100644
--- a/Documentation/filesystems/path-lookup.md
+++ b/Documentation/filesystems/path-lookup.md
@@ -826,9 +826,9 @@ If the filesystem may need to revalidate dcache entries, then
 *is* passed the dentry but does not have access to the `inode` or the
 `seq` number from the `nameidata`, so it needs to be extra careful
 when accessing fields in the dentry.  This "extra care" typically
-involves using `ACCESS_ONCE()` or the newer [`READ_ONCE()`] to access
-fields, and verifying the result is not NULL before using it.  This
-pattern can be see in `nfs_lookup_revalidate()`.
+involves using [`READ_ONCE()`] to access fields, and verifying the
+result is not NULL before using it.  This pattern can be see in
+`nfs_lookup_revalidate()`.
 
 A pair of patterns
 ------------------
diff --git a/mm/memory.c b/mm/memory.c
index a728bed16c20..cae514e7dcfc 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3891,9 +3891,9 @@ static int handle_pte_fault(struct vm_fault *vmf)
 		/*
 		 * some architectures can have larger ptes than wordsize,
 		 * e.g.ppc44x-defconfig has CONFIG_PTE_64BIT=y and
-		 * CONFIG_32BIT=y, so READ_ONCE or ACCESS_ONCE cannot guarantee
-		 * atomic accesses.  The code below just needs a consistent
-		 * view for the ifs and we later double check anyway with the
+		 * CONFIG_32BIT=y, so READ_ONCE cannot guarantee atomic
+		 * accesses.  The code below just needs a consistent view
+		 * for the ifs and we later double check anyway with the
 		 * ptl lock held. So here a barrier will do.
 		 */
 		barrier();