Replace smp_read_barrier_depends() with lockless_derefrence()

RFC PATCH Applied Pranith Kumar 7 participants 33 messages Nov 13, 2014 – Nov 30, 2014 1 message not in the archive

Subsystems (23)

Networking Network drivers Memory management Documentation +19 more subsystems

lore

Share & tools

lore

Pranith Kumar

author

2014-11-13 19:24 UTC #0 lore raw

cover Replace smp_read_barrier_depends() with lockless_derefrence()

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends().

http://lkml.iu.edu/hypermail/linux/kernel/1410.3/04561.html

The following series tries to do this.

There are still some hard-coded locations which I was not sure how to replace with. I will send in separate patches/questions regarding them.

Pranith Kumar (16):
crypto: caam - Remove unnecessary smp_read_barrier_depends()
doc: memory-barriers.txt: Document use of lockless_dereference()
drivers: dma: Replace smp_read_barrier_depends() with
lockless_dereference()
dcache: Replace smp_read_barrier_depends() with lockless_dereference()
overlayfs: Replace smp_read_barrier_depends() with
lockless_dereference()
assoc_array: Replace smp_read_barrier_depends() with
lockless_dereference()
hyperv: Replace smp_read_barrier_depends() with lockless_dereference()
rcupdate: Replace smp_read_barrier_depends() with
lockless_dereference()
percpu: Replace smp_read_barrier_depends() with lockless_dereference()
perf: Replace smp_read_barrier_depends() with lockless_dereference()
seccomp: Replace smp_read_barrier_depends() with
lockless_dereference()
task_work: Replace smp_read_barrier_depends() with
lockless_dereference()
ksm: Replace smp_read_barrier_depends() with lockless_dereference()
slab: Replace smp_read_barrier_depends() with lockless_dereference()
netfilter: Replace smp_read_barrier_depends() with
lockless_dereference()
rxrpc: Replace smp_read_barrier_depends() with lockless_dereference()

 Documentation/memory-barriers.txt |  2 +-
 drivers/crypto/caam/jr.c          |  3 ---
 drivers/dma/ioat/dma_v2.c         |  3 +--
 drivers/dma/ioat/dma_v3.c         |  3 +--
 fs/dcache.c                       |  7 ++-----
 fs/overlayfs/super.c              |  4 +---
 include/linux/assoc_array_priv.h  | 11 +++++++----
 include/linux/hyperv.h            |  9 ++++-----
 include/linux/percpu-refcount.h   |  4 +---
 include/linux/rcupdate.h          | 10 +++++-----
 kernel/events/core.c              |  3 +--
 kernel/events/uprobes.c           |  8 ++++----
 kernel/seccomp.c                  |  7 +++----
 kernel/task_work.c                |  3 +--
 lib/assoc_array.c                 |  7 -------
 mm/ksm.c                          |  7 +++----
 mm/slab.h                         |  6 +++---
 net/ipv4/netfilter/arp_tables.c   |  3 +--
 net/ipv4/netfilter/ip_tables.c    |  3 +--
 net/ipv6/netfilter/ip6_tables.c   |  3 +--
 net/rxrpc/ar-ack.c                | 22 +++++++++-------------
 security/keys/keyring.c           |  6 ------
 22 files changed, 50 insertions(+), 84 deletions(-)

Signature · 7 lines

1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pranith Kumar

author

2014-11-13 19:24 UTC #1 lore raw

PATCH 1/16 crypto: caam - Remove unnecessary smp_read_barrier_depends()

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 drivers/crypto/caam/jr.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/crypto/caam/jr.c +0 -3

index bae20d8..9b3ef1bc 100644
@@ -181,8 +181,6 @@ static void caam_jr_dequeue(unsigned long devarg)
 		for (i = 0; CIRC_CNT(head, tail + i, JOBR_DEPTH) >= 1; i++) {
 			sw_idx = (tail + i) & (JOBR_DEPTH - 1);
 
-			smp_read_barrier_depends();
-
 			if (jrp->outring[hw_idx].desc ==
 			    jrp->entinfo[sw_idx].desc_addr_dma)
 				break; /* found */

@@ -218,7 +216,6 @@ static void caam_jr_dequeue(unsigned long devarg)
 		if (sw_idx == tail) {
 			do {
 				tail = (tail + 1) & (JOBR_DEPTH - 1);
-				smp_read_barrier_depends();
 			} while (CIRC_CNT(head, tail, JOBR_DEPTH) >= 1 &&
 				 jrp->entinfo[tail].desc_addr_dma == 0);

Signature · 1 line

1.9.1

Paul E. McKenney

2014-11-13 20:10 UTC #2 lore raw

Re: [PATCH 01/16] crypto: caam - Remove unnecessary smp_read_barrier_depends()

Quoted 13 lines · Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
Signed-off-by Pranith Kumar <[email protected]>
---
 drivers/crypto/caam/jr.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/crypto/caam/jr.c +0 -0
index bae20d8..9b3ef1bc 100644

drivers/crypto/caam/jr.c static void caam_jr_dequeue(unsigned long devarg) · line 185

Quoted 5 lines

drivers/crypto/caam/jr.c +0 -2

@@ -181,8 +181,6 @@ static void caam_jr_dequeue(unsigned long devarg)
 		for (i = 0; CIRC_CNT(head, tail + i, JOBR_DEPTH) >= 1; i++) {
 			sw_idx = (tail + i) & (JOBR_DEPTH - 1);
 
-			smp_read_barrier_depends();
-

Did you mean to add a lockless_dereference() somewhere? I would guess that one is required on the load into sw_idx before the "for" loop, but I cannot claim to be familiar with this code.

Thanx, Paul

Quoted 14 lines

drivers/crypto/caam/jr.c +0 -1

 			if (jrp->outring[hw_idx].desc ==
 			    jrp->entinfo[sw_idx].desc_addr_dma)
 				break; /* found */

@@ -218,7 +216,6 @@ static void caam_jr_dequeue(unsigned long devarg)
 		if (sw_idx == tail) {
 			do {
 				tail = (tail + 1) & (JOBR_DEPTH - 1);
-				smp_read_barrier_depends();
 			} while (CIRC_CNT(head, tail, JOBR_DEPTH) >= 1 &&
 				 jrp->entinfo[tail].desc_addr_dma == 0);

Signature · 1 line

1.9.1

A message that is not in the archive (sent off-list or not yet received): [email protected]

Kim Phillips

Reviewed-by

2014-11-14 01:03 UTC #4 lore raw

Re: [PATCH 01/16] crypto: caam - Remove unnecessary smp_read_barrier_depends()

Quoted 25 lines · On Thu, 13 Nov 2014 16:51:12 -0500 Pranith Kumar <[email protected]> wrote:

Quoted 17 lines · Paul E. McKenney
Quoted 16 lines · Pranith Kumar
Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
Signed-off-by Pranith Kumar <[email protected]>
---
 drivers/crypto/caam/jr.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/crypto/caam/jr.c +0 -0
index bae20d8..9b3ef1bc 100644
@@ -181,8 +181,6 @@ static void caam_jr_dequeue(unsigned long devarg)
 		for (i = 0; CIRC_CNT(head, tail + i, JOBR_DEPTH) >= 1; i++) {
 			sw_idx = (tail + i) & (JOBR_DEPTH - 1);
 
Quoted 6 lines
drivers/crypto/caam/jr.c static void caam_jr_dequeue(unsigned long devarg) · line 185
Quoted 2 lines
drivers/crypto/caam/jr.c +0 -2
-			smp_read_barrier_depends();
-
Did you mean to add a lockless_dereference() somewhere? I would guess that one is required on the load into sw_idx before the "for" loop, but I cannot claim to be familiar with this code.

Quoted 3 lines

No, I could not understand why the barrier was here in the first place. The change log does not reflect the subject line.
sw_idx is a local variable and is used to index the entinfo array. It does not seem like the barrier is needed. If not a comment saying why could be added I guess.

I likely misinterpreted the "read index before reading contents at that index" instructions in early circular buffer documentation, and left it in for the benefit of the doubt. Now it looks to me it's not necessary, given both sw_idx and tail are just being computed within a lock, and removing both barriers doesn't affect the compiler output, so:

Reviewed-by Kim Phillips <[email protected]>

Thanks,

Kim

Pranith Kumar

author

2014-11-13 19:30 UTC #5 lore raw

PATCH 2/16 doc: memory-barriers.txt: Document use of lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 Documentation/memory-barriers.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Documentation/memory-barriers.txt +1 -1

index 3d5f49b..841ac36 100644
@@ -203,7 +203,7 @@ There are some minimal guarantees that may be expected of a CPU:
      and always in that order.  On most systems, smp_read_barrier_depends()
      does nothing, but it is required for DEC Alpha.  The ACCESS_ONCE()
      is required to prevent compiler mischief.  Please note that you
-     should normally use something like rcu_dereference() instead of
+     should normally use something like lockless_dereference() instead of
      open-coding smp_read_barrier_depends().
 
  (*) Overlapping loads and stores within a particular CPU will appear to be

Signature · 1 line

1.9.1

Paul E. McKenney

2014-11-13 20:11 UTC #6 lore raw

Re: [PATCH 02/16] doc: memory-barriers.txt: Document use of lockless_dereference()

Quoted 13 lines · Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
Signed-off-by Pranith Kumar <[email protected]>
---
 Documentation/memory-barriers.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Documentation/memory-barriers.txt +0 -0
index 3d5f49b..841ac36 100644

Documentation/memory-barriers.txt There are some minimal guarantees that may be expected of a CPU: · line 206

Quoted 5 lines

Documentation/memory-barriers.txt +1 -1

@@ -203,7 +203,7 @@ There are some minimal guarantees that may be expected of a CPU:
      and always in that order.  On most systems, smp_read_barrier_depends()
      does nothing, but it is required for DEC Alpha.  The ACCESS_ONCE()
      is required to prevent compiler mischief.  Please note that you
-     should normally use something like rcu_dereference() instead of
+     should normally use something like lockless_dereference() instead of

Good catch, but please keep both possibilities, something like "... like rcu_dereference() or lockless_dereference() instead of ..."

Thanx, Paul

Quoted 6 lines

Documentation/memory-barriers.txt +0 -0

      open-coding smp_read_barrier_depends().
 
  (*) Overlapping loads and stores within a particular CPU will appear to be

Signature · 1 line

1.9.1

Pranith Kumar

author

2014-11-13 19:25 UTC #7 lore raw

PATCH 3/16 drivers: dma: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 drivers/dma/ioat/dma_v2.c | 3 +--
 drivers/dma/ioat/dma_v3.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

drivers/dma/ioat/dma_v2.c +1 -2

index 695483e..0f94d72 100644
@@ -142,10 +142,9 @@ static void __cleanup(struct ioat2_dma_chan *ioat, dma_addr_t phys_complete)
 
 	active = ioat2_ring_active(ioat);
 	for (i = 0; i < active && !seen_current; i++) {
-		smp_read_barrier_depends();
 		prefetch(ioat2_get_ring_ent(ioat, idx + i + 1));
 		desc = ioat2_get_ring_ent(ioat, idx + i);
-		tx = &desc->txd;
+		tx = &lockless_dereference(desc)->txd;
 		dump_desc_dbg(ioat, desc);
 		if (tx->cookie) {
 			dma_descriptor_unmap(tx);

drivers/dma/ioat/dma_v3.c +1 -2

index 895f869..cbd0537 100644
@@ -389,7 +389,6 @@ static void __cleanup(struct ioat2_dma_chan *ioat, dma_addr_t phys_complete)
 	for (i = 0; i < active && !seen_current; i++) {
 		struct dma_async_tx_descriptor *tx;
 
-		smp_read_barrier_depends();
 		prefetch(ioat2_get_ring_ent(ioat, idx + i + 1));
 		desc = ioat2_get_ring_ent(ioat, idx + i);
 		dump_desc_dbg(ioat, desc);

@@ -398,7 +397,7 @@ static void __cleanup(struct ioat2_dma_chan *ioat, dma_addr_t phys_complete)
 		if (device->cap & IOAT_CAP_DWBES)
 			desc_get_errstat(ioat, desc);
 
-		tx = &desc->txd;
+		tx = &lockless_dereference(desc)->txd;
 		if (tx->cookie) {
 			dma_cookie_complete(tx);
 			dma_descriptor_unmap(tx);

Signature · 1 line

1.9.1

Pranith Kumar

author

2014-11-13 19:24 UTC #8 lore raw

PATCH 4/16 dcache: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 fs/dcache.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

fs/dcache.c +2 -5

index a6c5d7e..27b8b5b 100644
@@ -230,8 +230,7 @@ static inline int dentry_cmp(const struct dentry *dentry, const unsigned char *c
 	 * early because the data cannot match (there can
 	 * be no NUL in the ct/tcount data)
 	 */
-	cs = ACCESS_ONCE(dentry->d_name.name);
-	smp_read_barrier_depends();
+	cs = lockless_dereference(dentry->d_name.name);
 	return dentry_string_cmp(cs, ct, tcount);
 }

@@ -2714,10 +2713,8 @@ static int prepend(char **buffer, int *buflen, const char *str, int namelen)
 static int prepend_name(char **buffer, int *buflen, struct qstr *name)
 {
 	const char *dname = ACCESS_ONCE(name->name);
-	u32 dlen = ACCESS_ONCE(name->len);
 	char *p;
-
-	smp_read_barrier_depends();
+	u32 dlen = lockless_dereference(name->len);
 
 	*buflen -= dlen + 1;
 	if (*buflen < 0)

Signature · 1 line

1.9.1

Pranith Kumar

author

2014-11-13 19:24 UTC #9 lore raw

PATCH 5/16 overlayfs: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 fs/overlayfs/super.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

fs/overlayfs/super.c +1 -3

index 08b704c..b0f050e 100644
@@ -84,12 +84,10 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
 
 static struct dentry *ovl_upperdentry_dereference(struct ovl_entry *oe)
 {
-	struct dentry *upperdentry = ACCESS_ONCE(oe->__upperdentry);
 	/*
 	 * Make sure to order reads to upperdentry wrt ovl_dentry_update()
 	 */
-	smp_read_barrier_depends();
-	return upperdentry;
+	return lockless_dereference(oe->__upperdentry);
 }
 
 void ovl_path_upper(struct dentry *dentry, struct path *path)

Signature · 1 line

1.9.1

Miklos Szeredi

maintainer

2014-11-17 10:35 UTC #10 lore raw

Re: [PATCH 05/16] overlayfs: Replace smp_read_barrier_depends() with lockless_dereference()

Quoted 27 lines · Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 fs/overlayfs/super.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

fs/overlayfs/super.c +1 -4

index 08b704c..b0f050e 100644
@@ -84,12 +84,10 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
 
 static struct dentry *ovl_upperdentry_dereference(struct ovl_entry *oe)
 {
-       struct dentry *upperdentry = ACCESS_ONCE(oe->__upperdentry);
        /*
         * Make sure to order reads to upperdentry wrt ovl_dentry_update()
         */
-       smp_read_barrier_depends();
-       return upperdentry;
+       return lockless_dereference(oe->__upperdentry);
 }
 
 void ovl_path_upper(struct dentry *dentry, struct path *path)
--

Quoted 2 lines

1.9.1

Thanks, already in overlayfs-next branch of

git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next

Thanks,
Miklos

Pranith Kumar

author

2014-11-13 19:25 UTC #11 lore raw

PATCH 6/16 assoc_array: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

I replaced the inline functions dereferencing pointer 'x' to use lockless_dereference() because of which we do not need to litter the code with smp_read_barrier_depends().

Signed-off-by Pranith Kumar <[email protected]>

---
 include/linux/assoc_array_priv.h | 11 +++++++----
 lib/assoc_array.c                |  7 -------
 security/keys/keyring.c          |  6 ------
 3 files changed, 7 insertions(+), 17 deletions(-)

include/linux/assoc_array_priv.h +7 -4

index 711275e..96449c3 100644
@@ -118,7 +118,8 @@ struct assoc_array_edit {
 
 static inline bool assoc_array_ptr_is_meta(const struct assoc_array_ptr *x)
 {
-	return (unsigned long)x & ASSOC_ARRAY_PTR_TYPE_MASK;
+	return (unsigned long)lockless_dereference(x) &
+						ASSOC_ARRAY_PTR_TYPE_MASK;
 }
 static inline bool assoc_array_ptr_is_leaf(const struct assoc_array_ptr *x)
 {

@@ -126,7 +127,8 @@ static inline bool assoc_array_ptr_is_leaf(const struct assoc_array_ptr *x)
 }
 static inline bool assoc_array_ptr_is_shortcut(const struct assoc_array_ptr *x)
 {
-	return (unsigned long)x & ASSOC_ARRAY_PTR_SUBTYPE_MASK;
+	return (unsigned long)lockless_dereference(x) &
+						ASSOC_ARRAY_PTR_SUBTYPE_MASK;
 }
 static inline bool assoc_array_ptr_is_node(const struct assoc_array_ptr *x)
 {

@@ -135,13 +137,14 @@ static inline bool assoc_array_ptr_is_node(const struct assoc_array_ptr *x)
 
 static inline void *assoc_array_ptr_to_leaf(const struct assoc_array_ptr *x)
 {
-	return (void *)((unsigned long)x & ~ASSOC_ARRAY_PTR_TYPE_MASK);
+	return (void *)((unsigned long)lockless_dereference(x) &
+						~ASSOC_ARRAY_PTR_TYPE_MASK);
 }
 
 static inline
 unsigned long __assoc_array_ptr_to_meta(const struct assoc_array_ptr *x)
 {
-	return (unsigned long)x &
+	return (unsigned long)lockless_dereference(x) &
 		~(ASSOC_ARRAY_PTR_SUBTYPE_MASK | ASSOC_ARRAY_PTR_TYPE_MASK);
 }
 static inline

lib/assoc_array.c +0 -7

index 2404d03..5b62033 100644
@@ -37,12 +37,10 @@ begin_node:
 	if (assoc_array_ptr_is_shortcut(cursor)) {
 		/* Descend through a shortcut */
 		shortcut = assoc_array_ptr_to_shortcut(cursor);
-		smp_read_barrier_depends();
 		cursor = ACCESS_ONCE(shortcut->next_node);
 	}
 
 	node = assoc_array_ptr_to_node(cursor);
-	smp_read_barrier_depends();
 	slot = 0;
 
 	/* We perform two passes of each node.

@@ -85,7 +83,6 @@ begin_node:
 
 continue_node:
 	node = assoc_array_ptr_to_node(cursor);
-	smp_read_barrier_depends();
 
 	for (; slot < ASSOC_ARRAY_FAN_OUT; slot++) {
 		ptr = ACCESS_ONCE(node->slots[slot]);

@@ -104,7 +101,6 @@ finished_node:
 
 	if (assoc_array_ptr_is_shortcut(parent)) {
 		shortcut = assoc_array_ptr_to_shortcut(parent);
-		smp_read_barrier_depends();
 		cursor = parent;
 		parent = ACCESS_ONCE(shortcut->back_pointer);
 		slot = shortcut->parent_slot;

@@ -215,7 +211,6 @@ jumped:
 
 consider_node:
 	node = assoc_array_ptr_to_node(cursor);
-	smp_read_barrier_depends();
 
 	slot = segments >> (level & ASSOC_ARRAY_KEY_CHUNK_MASK);
 	slot &= ASSOC_ARRAY_FAN_MASK;

@@ -253,7 +248,6 @@ consider_node:
 	cursor = ptr;
 follow_shortcut:
 	shortcut = assoc_array_ptr_to_shortcut(cursor);
-	smp_read_barrier_depends();
 	pr_devel("shortcut to %d\n", shortcut->skip_to_level);
 	sc_level = level + ASSOC_ARRAY_LEVEL_STEP;
 	BUG_ON(sc_level > shortcut->skip_to_level);

@@ -343,7 +337,6 @@ void *assoc_array_find(const struct assoc_array *array,
 			 * actually going to dereference it.
 			 */
 			leaf = assoc_array_ptr_to_leaf(ptr);
-			smp_read_barrier_depends();
 			if (ops->compare_object(leaf, index_key))
 				return (void *)leaf;
 		}

security/keys/keyring.c +0 -6

index 8177010..48d3464 100644
@@ -683,7 +683,6 @@ descend_to_keyring:
 		 * doesn't contain any keyring pointers.
 		 */
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		if ((shortcut->index_key[0] & ASSOC_ARRAY_FAN_MASK) != 0)
 			goto not_this_keyring;

@@ -693,7 +692,6 @@ descend_to_keyring:
 	}
 
 	node = assoc_array_ptr_to_node(ptr);
-	smp_read_barrier_depends();
 
 	ptr = node->slots[0];
 	if (!assoc_array_ptr_is_meta(ptr))

@@ -706,7 +704,6 @@ descend_to_node:
 	kdebug("descend");
 	if (assoc_array_ptr_is_shortcut(ptr)) {
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		ptr = ACCESS_ONCE(shortcut->next_node);
 		BUG_ON(!assoc_array_ptr_is_node(ptr));
 	}

@@ -714,7 +711,6 @@ descend_to_node:
 
 begin_node:
 	kdebug("begin_node");
-	smp_read_barrier_depends();
 	slot = 0;
 ascend_to_node:
 	/* Go through the slots in a node */

@@ -762,14 +758,12 @@ ascend_to_node:
 
 	if (ptr && assoc_array_ptr_is_shortcut(ptr)) {
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		ptr = ACCESS_ONCE(shortcut->back_pointer);
 		slot = shortcut->parent_slot;
 	}
 	if (!ptr)
 		goto not_this_keyring;
 	node = assoc_array_ptr_to_node(ptr);
-	smp_read_barrier_depends();
 	slot++;
 
 	/* If we've ascended to the root (zero backpointer), we must have just

Signature · 1 line

1.9.1

David Howells

maintainer

2014-11-13 20:11 UTC #12 lore raw

Re: [PATCH 06/16] assoc_array: Replace smp_read_barrier_depends() with lockless_dereference()

include/linux/assoc_array_priv.h struct assoc_array_edit { · line 123

Quoted 6 lines · Pranith Kumar

static inline bool assoc_array_ptr_is_meta(const struct assoc_array_ptr *x) {
include/linux/assoc_array_priv.h +2 -1
-	return (unsigned long)x & ASSOC_ARRAY_PTR_TYPE_MASK;
+	return (unsigned long)lockless_dereference(x) &
+						ASSOC_ARRAY_PTR_TYPE_MASK;
 }

Nack. x is not being dereferenced here, nor in some of the other places you've added this.

David

Pranith Kumar

author

2014-11-13 19:25 UTC #13 lore raw

PATCH 7/16 hyperv: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 include/linux/hyperv.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

include/linux/hyperv.h +4 -5

index 08cfaff..06418b1 100644
@@ -127,13 +127,12 @@ hv_get_ringbuffer_availbytes(struct hv_ring_buffer_info *rbi,
 			  u32 *read, u32 *write)
 {
 	u32 read_loc, write_loc, dsize;
-
-	smp_read_barrier_depends();
+	struct hv_ring_buffer_info *rbi_p = lockless_dereference(rbi);
 
 	/* Capture the read/write indices before they changed */
-	read_loc = rbi->ring_buffer->read_index;
-	write_loc = rbi->ring_buffer->write_index;
-	dsize = rbi->ring_datasize;
+	read_loc = rbi_p->ring_buffer->read_index;
+	write_loc = rbi_p->ring_buffer->write_index;
+	dsize = rbi_p->ring_datasize;
 
 	*write = write_loc >= read_loc ? dsize - (write_loc - read_loc) :
 		read_loc - write_loc;

Signature · 1 line

1.9.1

Pranith Kumar

author

2014-11-13 19:28 UTC #14 lore raw

PATCH 8/16 rcupdate: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 include/linux/rcupdate.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

include/linux/rcupdate.h +5 -5

index ed4f593..386ba28 100644
@@ -582,11 +582,11 @@ static inline void rcu_preempt_sleep_check(void)
 })
 #define __rcu_dereference_check(p, c, space) \
 ({ \
-	typeof(*p) *_________p1 = (typeof(*p) *__force)ACCESS_ONCE(p); \
+	/* Dependency order vs. p above. */ \
+	typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \
 	rcu_lockdep_assert(c, "suspicious rcu_dereference_check() usage"); \
 	rcu_dereference_sparse(p, space); \
-	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
-	((typeof(*p) __force __kernel *)(_________p1)); \
+	((typeof(*p) __force __kernel *)(________p1)); \
 })
 #define __rcu_dereference_protected(p, c, space) \
 ({ \

@@ -603,10 +603,10 @@ static inline void rcu_preempt_sleep_check(void)
 })
 #define __rcu_dereference_index_check(p, c) \
 ({ \
-	typeof(p) _________p1 = ACCESS_ONCE(p); \
+	/* Dependency order vs. p above. */ \
+	typeof(p) _________p1 = lockless_dereference(p); \
 	rcu_lockdep_assert(c, \
 			   "suspicious rcu_dereference_index_check() usage"); \
-	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
 	(_________p1); \
 })

Signature · 1 line

1.9.1

Paul E. McKenney

Applied

2014-11-18 19:33 UTC #15 lore raw

Re: [PATCH 08/16] rcupdate: Replace smp_read_barrier_depends() with lockless_dereference()

Quoted 4 lines · Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
Signed-off-by Pranith Kumar <[email protected]>

Queued for 3.20, thank you! I was wondering about your removing the "_" from "_________p1", but see that this allowed you to avoid the checkpatch.pl warning. ;-)

Thanx, Paul

Quoted 39 lines

---
 include/linux/rcupdate.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

include/linux/rcupdate.h +5 -5

index ed4f593..386ba28 100644
@@ -582,11 +582,11 @@ static inline void rcu_preempt_sleep_check(void)
 })
 #define __rcu_dereference_check(p, c, space) \
 ({ \
-	typeof(*p) *_________p1 = (typeof(*p) *__force)ACCESS_ONCE(p); \
+	/* Dependency order vs. p above. */ \
+	typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \
 	rcu_lockdep_assert(c, "suspicious rcu_dereference_check() usage"); \
 	rcu_dereference_sparse(p, space); \
-	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
-	((typeof(*p) __force __kernel *)(_________p1)); \
+	((typeof(*p) __force __kernel *)(________p1)); \
 })
 #define __rcu_dereference_protected(p, c, space) \
 ({ \

@@ -603,10 +603,10 @@ static inline void rcu_preempt_sleep_check(void)
 })
 #define __rcu_dereference_index_check(p, c) \
 ({ \
-	typeof(p) _________p1 = ACCESS_ONCE(p); \
+	/* Dependency order vs. p above. */ \
+	typeof(p) _________p1 = lockless_dereference(p); \
 	rcu_lockdep_assert(c, \
 			   "suspicious rcu_dereference_index_check() usage"); \
-	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
 	(_________p1); \
 })

Signature · 1 line

1.9.1

Pranith Kumar

author

2014-11-18 19:38 UTC #16 lore raw

Re: [PATCH 08/16] rcupdate: Replace smp_read_barrier_depends() with lockless_dereference()

Quoted 9 lines · Paul E. McKenney

Quoted 4 lines · Pranith Kumar
Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
Signed-off-by Pranith Kumar <[email protected]>
Queued for 3.20, thank you! I was wondering about your removing the "_" from "_________p1", but see that this allowed you to avoid the checkpatch.pl warning. ;-)

Yes, that was the easiest way I could think of to avoid the warning with adding more lines.

I will send the rest of the patches updated with comments.

Thanks,
Pranith

Pranith Kumar

author

2014-11-13 19:28 UTC #17 lore raw

PATCH 9/16 percpu: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 include/linux/percpu-refcount.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

include/linux/percpu-refcount.h +1 -3

index 494ab05..3cf6759 100644
@@ -128,10 +128,8 @@ static inline void percpu_ref_kill(struct percpu_ref *ref)
 static inline bool __ref_is_percpu(struct percpu_ref *ref,
 					  unsigned long __percpu **percpu_countp)
 {
-	unsigned long percpu_ptr = ACCESS_ONCE(ref->percpu_count_ptr);
-
 	/* paired with smp_store_release() in percpu_ref_reinit() */
-	smp_read_barrier_depends();
+	unsigned long percpu_ptr = lockless_dereference(ref->percpu_count_ptr);
 
 	if (unlikely(percpu_ptr & __PERCPU_REF_ATOMIC))
 		return false;

Signature · 1 line

1.9.1

Tejun Heo

maintainer

2014-11-14 13:14 UTC #18 lore raw

Re: [PATCH 09/16] percpu: Replace smp_read_barrier_depends() with lockless_dereference()

Quoted 4 lines · Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
Signed-off-by Pranith Kumar <[email protected]>

How should this be routed? There's a pending change on the same region of code and if this patch is routed outside percpu tree it'd cause a conflict and I can't route this as lockless_dereference() isn't in the mainline yet. Maybe expose a git branch which contaisn lockless_dereference() so that percpu can pull it in?

Thanks.

Signature · 1 line

tejun

Pranith Kumar

author

2014-11-14 16:02 UTC #19 lore raw

Re: [PATCH 09/16] percpu: Replace smp_read_barrier_depends() with lockless_dereference()

Quoted 7 lines · Tejun Heo

How should this be routed? There's a pending change on the same region of code and if this patch is routed outside percpu tree it'd cause a conflict and I can't route this as lockless_dereference() isn't in the mainline yet. Maybe expose a git branch which contaisn lockless_dereference() so that percpu can pull it in?

I do not have access to git hosting (except github, but I think that is not acceptable). May be once the patches are final, I can send in a version which you can put in a branch on kernel.org?

Signature · 1 line

Pranith

Paul E. McKenney

2014-11-17 04:52 UTC #20 lore raw

Re: [PATCH 09/16] percpu: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Tejun Heo

Quoted 11 lines · Tejun Heo

Quoted 4 lines · Pranith Kumar
Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
Signed-off-by Pranith Kumar <[email protected]>
How should this be routed? There's a pending change on the same region of code and if this patch is routed outside percpu tree it'd cause a conflict and I can't route this as lockless_dereference() isn't in the mainline yet. Maybe expose a git branch which contaisn lockless_dereference() so that percpu can pull it in?

Actually, lockless_dereference() is now in mainline, if that makes it easier.

Thanx, Paul

Pranith Kumar

author

2014-11-13 19:25 UTC #21 lore raw

PATCH 10/16 perf: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 kernel/events/core.c    | 3 +--
 kernel/events/uprobes.c | 8 ++++----
 2 files changed, 5 insertions(+), 6 deletions(-)

kernel/events/core.c +1 -2

index d59fdc0..9dd5920 100644
@@ -3400,14 +3400,13 @@ static void perf_remove_from_owner(struct perf_event *event)
 	struct task_struct *owner;
 
 	rcu_read_lock();
-	owner = ACCESS_ONCE(event->owner);
 	/*
 	 * Matches the smp_wmb() in perf_event_exit_task(). If we observe
 	 * !owner it means the list deletion is complete and we can indeed
 	 * free this event, otherwise we need to serialize on
 	 * owner->perf_event_mutex.
 	 */
-	smp_read_barrier_depends();
+	owner = lockless_dereference(event->owner);
 	if (owner) {
 		/*
 		 * Since delayed_put_task_struct() also drops the last

kernel/events/uprobes.c +4 -4

index 6158a64b..c070949 100644
@@ -1212,8 +1212,8 @@ static struct xol_area *get_xol_area(void)
 	if (!mm->uprobes_state.xol_area)
 		__create_xol_area(0);
 
-	area = mm->uprobes_state.xol_area;
-	smp_read_barrier_depends();	/* pairs with wmb in xol_add_vma() */
+	/* pairs with wmb in xol_add_vma() */
+	area = lockless_dereference(mm->uprobes_state.xol_area);
 	return area;
 }

@@ -1507,8 +1507,8 @@ static unsigned long get_trampoline_vaddr(void)
 	struct xol_area *area;
 	unsigned long trampoline_vaddr = -1;
 
-	area = current->mm->uprobes_state.xol_area;
-	smp_read_barrier_depends();
+	area = lockless_dereference(current->mm->uprobes_state.xol_area);
+
 	if (area)
 		trampoline_vaddr = area->vaddr;

Signature · 1 line

1.9.1

Pranith Kumar

author

2014-11-13 19:27 UTC #22 lore raw

PATCH 11/16 seccomp: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 kernel/seccomp.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

kernel/seccomp.c +3 -4

index 4ef9687..3729b06 100644
@@ -175,17 +175,16 @@ static int seccomp_check_filter(struct sock_filter *filter, unsigned int flen)
  */
 static u32 seccomp_run_filters(struct seccomp_data *sd)
 {
-	struct seccomp_filter *f = ACCESS_ONCE(current->seccomp.filter);
 	struct seccomp_data sd_local;
 	u32 ret = SECCOMP_RET_ALLOW;
+	/* Make sure cross-thread synced filter points somewhere sane. */
+	struct seccomp_filter *f =
+			lockless_dereference(current->seccomp.filter);
 
 	/* Ensure unexpected behavior doesn't result in failing open. */
 	if (unlikely(WARN_ON(f == NULL)))
 		return SECCOMP_RET_KILL;
 
-	/* Make sure cross-thread synced filter points somewhere sane. */
-	smp_read_barrier_depends();
-
 	if (!sd) {
 		populate_seccomp_data(&sd_local);
 		sd = &sd_local;

Signature · 1 line

1.9.1

Pranith Kumar

author

2014-11-13 19:27 UTC #23 lore raw

PATCH 12/16 task_work: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 kernel/task_work.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

kernel/task_work.c +1 -2

index 8727032..b5599ce 100644
@@ -62,8 +62,7 @@ task_work_cancel(struct task_struct *task, task_work_func_t func)
 	 * we raced with task_work_run(), *pprev == NULL/exited.
 	 */
 	raw_spin_lock_irqsave(&task->pi_lock, flags);
-	while ((work = ACCESS_ONCE(*pprev))) {
-		smp_read_barrier_depends();
+	while ((work = lockless_dereference(*pprev))) {
 		if (work->func != func)
 			pprev = &work->next;
 		else if (cmpxchg(pprev, work, work->next) == work)

Signature · 1 line

1.9.1

Pranith Kumar

author

2014-11-13 19:25 UTC #24 lore raw

PATCH 13/16 ksm: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 mm/ksm.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

mm/ksm.c +3 -4

index d247efa..a67de79 100644
@@ -542,15 +542,14 @@ static struct page *get_ksm_page(struct stable_node *stable_node, bool lock_it)
 	expected_mapping = (void *)stable_node +
 				(PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
 again:
-	kpfn = ACCESS_ONCE(stable_node->kpfn);
-	page = pfn_to_page(kpfn);
-
 	/*
 	 * page is computed from kpfn, so on most architectures reading
 	 * page->mapping is naturally ordered after reading node->kpfn,
 	 * but on Alpha we need to be more careful.
 	 */
-	smp_read_barrier_depends();
+	kpfn = lockless_dereference(stable_node->kpfn);
+	page = pfn_to_page(kpfn);
+
 	if (ACCESS_ONCE(page->mapping) != expected_mapping)
 		goto stale;

Signature · 7 lines

1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Hugh Dickins

2014-11-30 22:35 UTC #25 lore raw

Re: [PATCH 13/16] ksm: Replace smp_read_barrier_depends() with lockless_dereference()

Quoted 4 lines · Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
Signed-off-by Pranith Kumar <[email protected]>

Sorry, I don't think your patch is buggy, but I do think it makes this tricky piece of code harder to follow, not easier.

It is certainly not a standard use of lockless_dereference() (kpfn is not a pointer), and it both hides and moves where the barrier is. And then at the end of the function, there's still explicit barriers and ACCESS_ONCE comparison with kpfn, which this makes more obscure.

Unless you are actually fixing a bug (I don't pretend to have tested this on Alpha, and I can get barriers wrong as we all do), or smp_read_barrier_depends() is about to be withdrawn from use, I'd rather say NAK to this patch.

Hugh

Quoted 29 lines

---
 mm/ksm.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

mm/ksm.c +3 -4

index d247efa..a67de79 100644
@@ -542,15 +542,14 @@ static struct page *get_ksm_page(struct stable_node *stable_node, bool lock_it)
 	expected_mapping = (void *)stable_node +
 				(PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
 again:
-	kpfn = ACCESS_ONCE(stable_node->kpfn);
-	page = pfn_to_page(kpfn);
-
 	/*
 	 * page is computed from kpfn, so on most architectures reading
 	 * page->mapping is naturally ordered after reading node->kpfn,
 	 * but on Alpha we need to be more careful.
 	 */
-	smp_read_barrier_depends();
+	kpfn = lockless_dereference(stable_node->kpfn);
+	page = pfn_to_page(kpfn);
+
 	if (ACCESS_ONCE(page->mapping) != expected_mapping)
 		goto stale;

Signature · 1 line

1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pranith Kumar

author

2014-11-13 19:25 UTC #26 lore raw

PATCH 14/16 slab: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 mm/slab.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

mm/slab.h +3 -3

index 3347fd7..1cf40054 100644
@@ -209,15 +209,15 @@ cache_from_memcg_idx(struct kmem_cache *s, int idx)
 
 	rcu_read_lock();
 	params = rcu_dereference(s->memcg_params);
-	cachep = params->memcg_caches[idx];
-	rcu_read_unlock();
 
 	/*
 	 * Make sure we will access the up-to-date value. The code updating
 	 * memcg_caches issues a write barrier to match this (see
 	 * memcg_register_cache()).
 	 */
-	smp_read_barrier_depends();
+	cachep = lockless_dereference(params->memcg_caches[idx]);
+	rcu_read_unlock();
+
 	return cachep;
 }

Signature · 7 lines

1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pranith Kumar

author

2014-11-13 19:24 UTC #27 lore raw

PATCH 15/16 netfilter: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 net/ipv4/netfilter/arp_tables.c | 3 +--
 net/ipv4/netfilter/ip_tables.c  | 3 +--
 net/ipv6/netfilter/ip6_tables.c | 3 +--
 3 files changed, 3 insertions(+), 6 deletions(-)

net/ipv4/netfilter/arp_tables.c +1 -2

index f95b6f9..fc7533d 100644
@@ -270,12 +270,11 @@ unsigned int arpt_do_table(struct sk_buff *skb,
 
 	local_bh_disable();
 	addend = xt_write_recseq_begin();
-	private = table->private;
 	/*
 	 * Ensure we load private-> members after we've fetched the base
 	 * pointer.
 	 */
-	smp_read_barrier_depends();
+	private = lockless_dereference(table->private);
 	table_base = private->entries[smp_processor_id()];
 
 	e = get_entry(table_base, private->hook_entry[hook]);

net/ipv4/netfilter/ip_tables.c +1 -2

index 99e810f..e0fd044 100644
@@ -325,13 +325,12 @@ ipt_do_table(struct sk_buff *skb,
 	IP_NF_ASSERT(table->valid_hooks & (1 << hook));
 	local_bh_disable();
 	addend = xt_write_recseq_begin();
-	private = table->private;
 	cpu        = smp_processor_id();
 	/*
 	 * Ensure we load private-> members after we've fetched the base
 	 * pointer.
 	 */
-	smp_read_barrier_depends();
+	private = lockless_dereference(table->private);
 	table_base = private->entries[cpu];
 	jumpstack  = (struct ipt_entry **)private->jumpstack[cpu];
 	stackptr   = per_cpu_ptr(private->stackptr, cpu);

net/ipv6/netfilter/ip6_tables.c +1 -2

index e080fbb..0459d6a 100644
@@ -348,12 +348,11 @@ ip6t_do_table(struct sk_buff *skb,
 
 	local_bh_disable();
 	addend = xt_write_recseq_begin();
-	private = table->private;
 	/*
 	 * Ensure we load private-> members after we've fetched the base
 	 * pointer.
 	 */
-	smp_read_barrier_depends();
+	private = lockless_dereference(table->private);
 	cpu        = smp_processor_id();
 	table_base = private->entries[cpu];
 	jumpstack  = (struct ip6t_entry **)private->jumpstack[cpu];

Signature · 1 line

1.9.1

Pranith Kumar

author

2014-11-13 19:24 UTC #28 lore raw

PATCH 16/16 rxrpc: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Signed-off-by Pranith Kumar <[email protected]>

---
 net/rxrpc/ar-ack.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

net/rxrpc/ar-ack.c +9 ~~-13~~

index c6be17a..9237448 100644
@@ -234,8 +234,7 @@ static void rxrpc_resend(struct rxrpc_call *call)
 	     loop != call->acks_head || stop;
 	     loop = (loop + 1) &  (call->acks_winsz - 1)
 	     ) {
-		p_txb = call->acks_window + loop;
-		smp_read_barrier_depends();
+		p_txb = lockless_dereference(call)->acks_window + loop;
 		if (*p_txb & 1)
 			continue;

@@ -303,8 +302,7 @@ static void rxrpc_resend_timer(struct rxrpc_call *call)
 	     loop != call->acks_head;
 	     loop = (loop + 1) &  (call->acks_winsz - 1)
 	     ) {
-		p_txb = call->acks_window + loop;
-		smp_read_barrier_depends();
+		p_txb = lockless_dereference(call)->acks_window + loop;
 		txb = (struct sk_buff *) (*p_txb & ~1);
 		sp = rxrpc_skb(txb);

@@ -354,9 +352,10 @@ static int rxrpc_process_soft_ACKs(struct rxrpc_call *call,
 	resend = 0;
 	resend_at = 0;
 	for (loop = 0; loop < ack->nAcks; loop++) {
-		p_txb = call->acks_window;
-		p_txb += (call->acks_tail + loop) & (call->acks_winsz - 1);
-		smp_read_barrier_depends();
+		struct rxrpc_call *callp = lockless_dereference(call);
+
+		p_txb = callp->acks_window;
+		p_txb += (callp->acks_tail + loop) & (callp->acks_winsz - 1);
 		txb = (struct sk_buff *) (*p_txb & ~1);
 		sp = rxrpc_skb(txb);

@@ -385,8 +384,7 @@ static int rxrpc_process_soft_ACKs(struct rxrpc_call *call,
 	     loop != call->acks_head;
 	     loop = (loop + 1) &  (call->acks_winsz - 1)
 	     ) {
-		p_txb = call->acks_window + loop;
-		smp_read_barrier_depends();
+		p_txb = lockless_dereference(call)->acks_window + loop;
 		txb = (struct sk_buff *) (*p_txb & ~1);
 		sp = rxrpc_skb(txb);

@@ -432,8 +430,7 @@ static void rxrpc_rotate_tx_window(struct rxrpc_call *call, u32 hard)
 	ASSERTCMP(hard - call->acks_hard, <=, win);
 
 	while (call->acks_hard < hard) {
-		smp_read_barrier_depends();
-		_skb = call->acks_window[tail] & ~1;
+		_skb = lockless_dereference(call)->acks_window[tail] & ~1;
 		rxrpc_free_skb((struct sk_buff *) _skb);
 		old_tail = tail;
 		tail = (tail + 1) & (call->acks_winsz - 1);

@@ -577,8 +574,7 @@ static void rxrpc_zap_tx_window(struct rxrpc_call *call)
 	call->acks_window = NULL;
 
 	while (CIRC_CNT(call->acks_head, call->acks_tail, winsz) > 0) {
-		tail = call->acks_tail;
-		smp_read_barrier_depends();
+		tail = lockless_dereference(call)->acks_tail;
 		_skb = acks_window[tail] & ~1;
 		smp_mb();
 		call->acks_tail = (call->acks_tail + 1) & (winsz - 1);

Signature · 1 line

1.9.1

David Howells

maintainer

2014-11-13 20:17 UTC #29 lore raw

Re: [PATCH 16/16] rxrpc: Replace smp_read_barrier_depends() with lockless_dereference()

net/rxrpc/ar-ack.c static void rxrpc_resend(struct rxrpc_call *call) · line 237

Quoted 6 lines · Pranith Kumar

net/rxrpc/ar-ack.c +1 -2

 	     loop != call->acks_head || stop;
 	     loop = (loop + 1) &  (call->acks_winsz - 1)
 	     ) {
-		p_txb = call->acks_window + loop;
-		smp_read_barrier_depends();
+		p_txb = lockless_dereference(call)->acks_window + loop;

Nack. You've stuck an implicit barrier on a dereference that doesn't matter. And similar for other hunks of this patch.

David

David Howells

maintainer

2014-11-13 20:47 UTC #30 lore raw

Re: [PATCH 16/16] rxrpc: Replace smp_read_barrier_depends() with lockless_dereference()

In reply to Pranith Kumar

Quoted 2 lines · Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.

Actually, the use of smp_read_barrier_depends() is wrong in circular buffering. See Documentation/circular-buffers.txt

David

Pranith Kumar

author

2014-11-13 21:55 UTC #31 lore raw

Re: [PATCH 16/16] rxrpc: Replace smp_read_barrier_depends() with lockless_dereference()

Quoted 8 lines · David Howells

Quoted 2 lines · Pranith Kumar
Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
Actually, the use of smp_read_barrier_depends() is wrong in circular buffering. See Documentation/circular-buffers.txt

OK. Should I send in a patch removing these barriers then?

--
Pranith

David Howells

maintainer

2014-11-13 23:07 UTC #32 lore raw

Re: [PATCH 16/16] rxrpc: Replace smp_read_barrier_depends() with lockless_dereference()

Quoted 1 line · Pranith Kumar

OK. Should I send in a patch removing these barriers then?

No. There need to be stronger barriers, at least in some of the cases. circular-buffers.txt details what is required, but not all of the cases match the pattern there, so it needs a bit more consideration.

David

Paul E. McKenney

2014-11-13 20:07 UTC #33 lore raw

In reply to Pranith Kumar

Quoted 9 lines · Pranith Kumar

Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends().
http://lkml.iu.edu/hypermail/linux/kernel/1410.3/04561.html
The following series tries to do this.
There are still some hard-coded locations which I was not sure how to replace with. I will send in separate patches/questions regarding them.

Thank you for taking this on! Some questions and comments in response to the individual patches.

Thanx, Paul

Quoted 40 lines

Pranith Kumar (16):
crypto: caam - Remove unnecessary smp_read_barrier_depends()
doc: memory-barriers.txt: Document use of lockless_dereference()
drivers: dma: Replace smp_read_barrier_depends() with
lockless_dereference()
dcache: Replace smp_read_barrier_depends() with lockless_dereference()
overlayfs: Replace smp_read_barrier_depends() with
lockless_dereference()
assoc_array: Replace smp_read_barrier_depends() with
lockless_dereference()
hyperv: Replace smp_read_barrier_depends() with lockless_dereference()
rcupdate: Replace smp_read_barrier_depends() with
lockless_dereference()
percpu: Replace smp_read_barrier_depends() with lockless_dereference()
perf: Replace smp_read_barrier_depends() with lockless_dereference()
seccomp: Replace smp_read_barrier_depends() with
lockless_dereference()
task_work: Replace smp_read_barrier_depends() with
lockless_dereference()
ksm: Replace smp_read_barrier_depends() with lockless_dereference()
slab: Replace smp_read_barrier_depends() with lockless_dereference()
netfilter: Replace smp_read_barrier_depends() with
lockless_dereference()
rxrpc: Replace smp_read_barrier_depends() with lockless_dereference()
 Documentation/memory-barriers.txt |  2 +-
 drivers/crypto/caam/jr.c          |  3 ---
 drivers/dma/ioat/dma_v2.c         |  3 +--
 drivers/dma/ioat/dma_v3.c         |  3 +--
 fs/dcache.c                       |  7 ++-----
 fs/overlayfs/super.c              |  4 +---
 include/linux/assoc_array_priv.h  | 11 +++++++----
 include/linux/hyperv.h            |  9 ++++-----
 include/linux/percpu-refcount.h   |  4 +---
 include/linux/rcupdate.h          | 10 +++++-----
 kernel/events/core.c              |  3 +--
 kernel/events/uprobes.c           |  8 ++++----
 kernel/seccomp.c                  |  7 +++----
 kernel/task_work.c                |  3 +--
 lib/assoc_array.c                 |  7 -------

Quoted 12 lines

 mm/ksm.c                          |  7 +++----
 mm/slab.h                         |  6 +++---
 net/ipv4/netfilter/arp_tables.c   |  3 +--
 net/ipv4/netfilter/ip_tables.c    |  3 +--
 net/ipv6/netfilter/ip6_tables.c   |  3 +--
 net/rxrpc/ar-ack.c                | 22 +++++++++-------------
 security/keys/keyring.c           |  6 ------
 22 files changed, 50 insertions(+), 84 deletions(-)

Signature · 1 line

1.9.1