[PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Jia He]

for the code as follows:
if (condition)
	rte_smp_rmb();
else
	rte_smp_wmb();
Without this patch, compiler will report this error:
error: 'else' without a previous 'if'

Signed-off-by: Jia He <hejianet@gmail.com>
Signed-off-by: jia.he@hxt-semitech.com
---
 lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
index 0b70d62..38c3393 100644
--- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
+++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
@@ -43,8 +43,8 @@ extern "C" {
 
 #include "generic/rte_atomic.h"
 
-#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
-#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
+#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
+#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
 
 #define rte_mb() dsb(sy)
 
-- 
2.7.4

[PATCH 2/3] ring: guarantee load ordering of cons/prod when doing enqueue/dequeue

[Jia He]

We watched a rte panic of mbuf_autotest in our qualcomm arm64 server.
As for the possible race condition, please refer to [1].

Furthermore, to fix this race, there are 2 options as suggested by Jerin:
1. use rte_smp_rmb
2. use load_acquire/store_release(refer to [2]).
CONFIG_RTE_RING_USE_C11_MEM_MODEL is provided, and by
default it is "y" only on arm64.

The reason why providing 2 options is due to the performance benchmark
difference in different arm machines, please refer to [3].

Already fuctionally tested on the machines as follows:
- on X86
- on arm64 with CONFIG_RTE_RING_USE_C11_MEM_MODEL=y
- on arm64 with CONFIG_RTE_RING_USE_C11_MEM_MODEL=n

We haven't tested the ppc64 case. If anyone verifies it, he can add
CONFIG_RTE_RING_USE_C11_MEM_MODEL=y to ppc64 config files.

[1] http://dpdk.org/ml/archives/dev/2017-October/078366.html
[2] https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L170
[3] http://dpdk.org/ml/archives/dev/2017-October/080861.html

---
Changelog:
V3: arch specific implementation for enqueue/dequeue barrier
V2: let users choose whether using load_acquire/store_release
V1: rte_smp_rmb() between 2 loads

Signed-off-by: Jia He <hejianet@gmail.com>
Signed-off-by: jie2.liu@hxt-semitech.com
Signed-off-by: bing.zhao@hxt-semitech.com
Signed-off-by: jia.he@hxt-semitech.com
Suggested-by: jerin.jacob@caviumnetworks.com
Suggested-by: konstantin.ananyev@intel.com

---
 lib/librte_eventdev/rte_event_ring.h |   6 +-
 lib/librte_ring/Makefile             |   4 +-
 lib/librte_ring/rte_ring.h           | 160 +++--------------------------
 lib/librte_ring/rte_ring_c11_mem.h   | 185 +++++++++++++++++++++++++++++++++
 lib/librte_ring/rte_ring_generic.h   | 192 +++++++++++++++++++++++++++++++++++
 5 files changed, 397 insertions(+), 150 deletions(-)
 create mode 100644 lib/librte_ring/rte_ring_c11_mem.h
 create mode 100644 lib/librte_ring/rte_ring_generic.h

diff --git a/lib/librte_eventdev/rte_event_ring.h b/lib/librte_eventdev/rte_event_ring.h
index ea9b688..3e49458 100644
--- a/lib/librte_eventdev/rte_event_ring.h
+++ b/lib/librte_eventdev/rte_event_ring.h
@@ -126,9 +126,8 @@ rte_event_ring_enqueue_burst(struct rte_event_ring *r,
 		goto end;
 
 	ENQUEUE_PTRS(&r->r, &r[1], prod_head, events, n, struct rte_event);
-	rte_smp_wmb();
 
-	update_tail(&r->r.prod, prod_head, prod_next, 1);
+	update_tail(&r->r.prod, prod_head, prod_next, 1, 1);
 end:
 	if (free_space != NULL)
 		*free_space = free_entries - n;
@@ -168,9 +167,8 @@ rte_event_ring_dequeue_burst(struct rte_event_ring *r,
 		goto end;
 
 	DEQUEUE_PTRS(&r->r, &r[1], cons_head, events, n, struct rte_event);
-	rte_smp_rmb();
 
-	update_tail(&r->r.cons, cons_head, cons_next, 1);
+	update_tail(&r->r.cons, cons_head, cons_next, 1, 0);
 
 end:
 	if (available != NULL)
diff --git a/lib/librte_ring/Makefile b/lib/librte_ring/Makefile
index e34d9d9..a2682e7 100644
--- a/lib/librte_ring/Makefile
+++ b/lib/librte_ring/Makefile
@@ -45,6 +45,8 @@ LIBABIVER := 1
 SRCS-$(CONFIG_RTE_LIBRTE_RING) := rte_ring.c
 
 # install includes
-SYMLINK-$(CONFIG_RTE_LIBRTE_RING)-include := rte_ring.h
+SYMLINK-$(CONFIG_RTE_LIBRTE_RING)-include := rte_ring.h \
+					rte_ring_generic.h \
+					rte_ring_c11_mem.h
 
 include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index 5e9b3b7..53764d9 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -356,85 +356,19 @@ void rte_ring_dump(FILE *f, const struct rte_ring *r);
 	} \
 } while (0)
 
-static __rte_always_inline void
-update_tail(struct rte_ring_headtail *ht, uint32_t old_val, uint32_t new_val,
-		uint32_t single)
-{
-	/*
-	 * If there are other enqueues/dequeues in progress that preceded us,
-	 * we need to wait for them to complete
-	 */
-	if (!single)
-		while (unlikely(ht->tail != old_val))
-			rte_pause();
-
-	ht->tail = new_val;
-}
-
-/**
- * @internal This function updates the producer head for enqueue
- *
- * @param r
- *   A pointer to the ring structure
- * @param is_sp
- *   Indicates whether multi-producer path is needed or not
- * @param n
- *   The number of elements we will want to enqueue, i.e. how far should the
- *   head be moved
- * @param behavior
- *   RTE_RING_QUEUE_FIXED:    Enqueue a fixed number of items from a ring
- *   RTE_RING_QUEUE_VARIABLE: Enqueue as many items as possible from ring
- * @param old_head
- *   Returns head value as it was before the move, i.e. where enqueue starts
- * @param new_head
- *   Returns the current/new head value i.e. where enqueue finishes
- * @param free_entries
- *   Returns the amount of free space in the ring BEFORE head was moved
- * @return
- *   Actual number of objects enqueued.
- *   If behavior == RTE_RING_QUEUE_FIXED, this will be 0 or n only.
- */
-static __rte_always_inline unsigned int
-__rte_ring_move_prod_head(struct rte_ring *r, int is_sp,
-		unsigned int n, enum rte_ring_queue_behavior behavior,
-		uint32_t *old_head, uint32_t *new_head,
-		uint32_t *free_entries)
-{
-	const uint32_t capacity = r->capacity;
-	unsigned int max = n;
-	int success;
-
-	do {
-		/* Reset n to the initial burst count */
-		n = max;
-
-		*old_head = r->prod.head;
-		const uint32_t cons_tail = r->cons.tail;
-		/*
-		 *  The subtraction is done between two unsigned 32bits value
-		 * (the result is always modulo 32 bits even if we have
-		 * *old_head > cons_tail). So 'free_entries' is always between 0
-		 * and capacity (which is < size).
-		 */
-		*free_entries = (capacity + cons_tail - *old_head);
-
-		/* check that we have enough room in ring */
-		if (unlikely(n > *free_entries))
-			n = (behavior == RTE_RING_QUEUE_FIXED) ?
-					0 : *free_entries;
-
-		if (n == 0)
-			return 0;
-
-		*new_head = *old_head + n;
-		if (is_sp)
-			r->prod.head = *new_head, success = 1;
-		else
-			success = rte_atomic32_cmpset(&r->prod.head,
-					*old_head, *new_head);
-	} while (unlikely(success == 0));
-	return n;
-}
+/* Between load and load. there might be cpu reorder in weak model
+ * (powerpc/arm).
+ * There are 2 choices for the users
+ * 1.use rmb() memory barrier
+ * 2.use one-direcion load_acquire/store_release barrier,defined by
+ * CONFIG_RTE_RING_USE_C11_MEM_MODEL=y
+ * It depends on performance test results.
+ */
+#ifdef RTE_RING_USE_C11_MEM_MODEL
+#include "rte_ring_c11_mem.h"
+#else
+#include "rte_ring_generic.h"
+#endif
 
 /**
  * @internal Enqueue several objects on the ring
@@ -470,9 +404,8 @@ __rte_ring_do_enqueue(struct rte_ring *r, void * const *obj_table,
 		goto end;
 
 	ENQUEUE_PTRS(r, &r[1], prod_head, obj_table, n, void *);
-	rte_smp_wmb();
 
-	update_tail(&r->prod, prod_head, prod_next, is_sp);
+	update_tail(&r->prod, prod_head, prod_next, is_sp, 1);
 end:
 	if (free_space != NULL)
 		*free_space = free_entries - n;
@@ -480,68 +413,6 @@ __rte_ring_do_enqueue(struct rte_ring *r, void * const *obj_table,
 }
 
 /**
- * @internal This function updates the consumer head for dequeue
- *
- * @param r
- *   A pointer to the ring structure
- * @param is_sc
- *   Indicates whether multi-consumer path is needed or not
- * @param n
- *   The number of elements we will want to enqueue, i.e. how far should the
- *   head be moved
- * @param behavior
- *   RTE_RING_QUEUE_FIXED:    Dequeue a fixed number of items from a ring
- *   RTE_RING_QUEUE_VARIABLE: Dequeue as many items as possible from ring
- * @param old_head
- *   Returns head value as it was before the move, i.e. where dequeue starts
- * @param new_head
- *   Returns the current/new head value i.e. where dequeue finishes
- * @param entries
- *   Returns the number of entries in the ring BEFORE head was moved
- * @return
- *   - Actual number of objects dequeued.
- *     If behavior == RTE_RING_QUEUE_FIXED, this will be 0 or n only.
- */
-static __rte_always_inline unsigned int
-__rte_ring_move_cons_head(struct rte_ring *r, int is_sc,
-		unsigned int n, enum rte_ring_queue_behavior behavior,
-		uint32_t *old_head, uint32_t *new_head,
-		uint32_t *entries)
-{
-	unsigned int max = n;
-	int success;
-
-	/* move cons.head atomically */
-	do {
-		/* Restore n as it may change every loop */
-		n = max;
-
-		*old_head = r->cons.head;
-		const uint32_t prod_tail = r->prod.tail;
-		/* The subtraction is done between two unsigned 32bits value
-		 * (the result is always modulo 32 bits even if we have
-		 * cons_head > prod_tail). So 'entries' is always between 0
-		 * and size(ring)-1. */
-		*entries = (prod_tail - *old_head);
-
-		/* Set the actual entries for dequeue */
-		if (n > *entries)
-			n = (behavior == RTE_RING_QUEUE_FIXED) ? 0 : *entries;
-
-		if (unlikely(n == 0))
-			return 0;
-
-		*new_head = *old_head + n;
-		if (is_sc)
-			r->cons.head = *new_head, success = 1;
-		else
-			success = rte_atomic32_cmpset(&r->cons.head, *old_head,
-					*new_head);
-	} while (unlikely(success == 0));
-	return n;
-}
-
-/**
  * @internal Dequeue several objects from the ring
  *
  * @param r
@@ -575,9 +446,8 @@ __rte_ring_do_dequeue(struct rte_ring *r, void **obj_table,
 		goto end;
 
 	DEQUEUE_PTRS(r, &r[1], cons_head, obj_table, n, void *);
-	rte_smp_rmb();
 
-	update_tail(&r->cons, cons_head, cons_next, is_sc);
+	update_tail(&r->cons, cons_head, cons_next, is_sc, 0);
 
 end:
 	if (available != NULL)
diff --git a/lib/librte_ring/rte_ring_c11_mem.h b/lib/librte_ring/rte_ring_c11_mem.h
new file mode 100644
index 0000000..f370df9
--- /dev/null
+++ b/lib/librte_ring/rte_ring_c11_mem.h
@@ -0,0 +1,185 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2017 hxt-semitech. All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of hxt-semitech nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_RING_C11_MEM_H_
+#define _RTE_RING_C11_MEM_H_
+
+#define UNUSED_PARAMETER(x) ((void)(x))
+static __rte_always_inline void
+update_tail(struct rte_ring_headtail *ht, uint32_t old_val, uint32_t new_val,
+		uint32_t single, uint32_t enqueue)
+{
+	UNUSED_PARAMETER(enqueue);
+	/*
+	 * If there are other enqueues/dequeues in progress that preceded us,
+	 * we need to wait for them to complete
+	 */
+	if (!single)
+		while (unlikely(ht->tail != old_val))
+			rte_pause();
+
+	__atomic_store_n(&ht->tail, new_val, __ATOMIC_RELEASE);
+}
+
+/**
+ * @internal This function updates the producer head for enqueue
+ *
+ * @param r
+ *   A pointer to the ring structure
+ * @param is_sp
+ *   Indicates whether multi-producer path is needed or not
+ * @param n
+ *   The number of elements we will want to enqueue, i.e. how far should the
+ *   head be moved
+ * @param behavior
+ *   RTE_RING_QUEUE_FIXED:    Enqueue a fixed number of items from a ring
+ *   RTE_RING_QUEUE_VARIABLE: Enqueue as many items as possible from ring
+ * @param old_head
+ *   Returns head value as it was before the move, i.e. where enqueue starts
+ * @param new_head
+ *   Returns the current/new head value i.e. where enqueue finishes
+ * @param free_entries
+ *   Returns the amount of free space in the ring BEFORE head was moved
+ * @return
+ *   Actual number of objects enqueued.
+ *   If behavior == RTE_RING_QUEUE_FIXED, this will be 0 or n only.
+ */
+static __rte_always_inline unsigned int
+__rte_ring_move_prod_head(struct rte_ring *r, int is_sp,
+		unsigned int n, enum rte_ring_queue_behavior behavior,
+		uint32_t *old_head, uint32_t *new_head,
+		uint32_t *free_entries)
+{
+	const uint32_t capacity = r->capacity;
+	unsigned int max = n;
+	int success;
+
+	do {
+		/* Reset n to the initial burst count */
+		n = max;
+
+		*old_head = __atomic_load_n(&r->prod.head,
+					__ATOMIC_ACQUIRE);
+		const uint32_t cons_tail = r->cons.tail;
+		/*
+		 *  The subtraction is done between two unsigned 32bits value
+		 * (the result is always modulo 32 bits even if we have
+		 * *old_head > cons_tail). So 'free_entries' is always between 0
+		 * and capacity (which is < size).
+		 */
+		*free_entries = (capacity + cons_tail - *old_head);
+
+		/* check that we have enough room in ring */
+		if (unlikely(n > *free_entries))
+			n = (behavior == RTE_RING_QUEUE_FIXED) ?
+					0 : *free_entries;
+
+		if (n == 0)
+			return 0;
+
+		*new_head = *old_head + n;
+		if (is_sp)
+			r->prod.head = *new_head, success = 1;
+		else
+			success = __atomic_compare_exchange_n(&r->prod.head,
+					old_head, *new_head,
+					0, __ATOMIC_ACQUIRE,
+					__ATOMIC_RELAXED);
+	} while (unlikely(success == 0));
+	return n;
+}
+
+/**
+ * @internal This function updates the consumer head for dequeue
+ *
+ * @param r
+ *   A pointer to the ring structure
+ * @param is_sc
+ *   Indicates whether multi-consumer path is needed or not
+ * @param n
+ *   The number of elements we will want to enqueue, i.e. how far should the
+ *   head be moved
+ * @param behavior
+ *   RTE_RING_QUEUE_FIXED:    Dequeue a fixed number of items from a ring
+ *   RTE_RING_QUEUE_VARIABLE: Dequeue as many items as possible from ring
+ * @param old_head
+ *   Returns head value as it was before the move, i.e. where dequeue starts
+ * @param new_head
+ *   Returns the current/new head value i.e. where dequeue finishes
+ * @param entries
+ *   Returns the number of entries in the ring BEFORE head was moved
+ * @return
+ *   - Actual number of objects dequeued.
+ *     If behavior == RTE_RING_QUEUE_FIXED, this will be 0 or n only.
+ */
+static __rte_always_inline unsigned int
+__rte_ring_move_cons_head(struct rte_ring *r, int is_sc,
+		unsigned int n, enum rte_ring_queue_behavior behavior,
+		uint32_t *old_head, uint32_t *new_head,
+		uint32_t *entries)
+{
+	unsigned int max = n;
+	int success;
+
+	/* move cons.head atomically */
+	do {
+		/* Restore n as it may change every loop */
+		n = max;
+		*old_head = __atomic_load_n(&r->cons.head,
+					__ATOMIC_ACQUIRE);
+		const uint32_t prod_tail = r->prod.tail;
+		/* The subtraction is done between two unsigned 32bits value
+		 * (the result is always modulo 32 bits even if we have
+		 * cons_head > prod_tail). So 'entries' is always between 0
+		 * and size(ring)-1. */
+		*entries = (prod_tail - *old_head);
+
+		/* Set the actual entries for dequeue */
+		if (n > *entries)
+			n = (behavior == RTE_RING_QUEUE_FIXED) ? 0 : *entries;
+
+		if (unlikely(n == 0))
+			return 0;
+
+		*new_head = *old_head + n;
+		if (is_sc)
+			r->cons.head = *new_head, success = 1;
+		else
+			success = __atomic_compare_exchange_n(&r->cons.head,
+							old_head, *new_head,
+							0, __ATOMIC_ACQUIRE,
+							__ATOMIC_RELAXED);
+	} while (unlikely(success == 0));
+	return n;
+}
+
+#endif /* _RTE_RING_C11_MEM_H_ */
diff --git a/lib/librte_ring/rte_ring_generic.h b/lib/librte_ring/rte_ring_generic.h
new file mode 100644
index 0000000..aa596eb
--- /dev/null
+++ b/lib/librte_ring/rte_ring_generic.h
@@ -0,0 +1,192 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2017 hxt-semitech. All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of hxt-semitech nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_RING_GENERIC_H_
+#define _RTE_RING_GENERIC_H_
+
+static __rte_always_inline void
+update_tail(struct rte_ring_headtail *ht, uint32_t old_val, uint32_t new_val,
+		uint32_t single, uint32_t enqueue)
+{
+	if (enqueue)
+		rte_smp_wmb();
+	else
+		rte_smp_rmb();
+	/*
+	 * If there are other enqueues/dequeues in progress that preceded us,
+	 * we need to wait for them to complete
+	 */
+	if (!single)
+		while (unlikely(ht->tail != old_val))
+			rte_pause();
+
+	ht->tail = new_val;
+}
+
+/**
+ * @internal This function updates the producer head for enqueue
+ *
+ * @param r
+ *   A pointer to the ring structure
+ * @param is_sp
+ *   Indicates whether multi-producer path is needed or not
+ * @param n
+ *   The number of elements we will want to enqueue, i.e. how far should the
+ *   head be moved
+ * @param behavior
+ *   RTE_RING_QUEUE_FIXED:    Enqueue a fixed number of items from a ring
+ *   RTE_RING_QUEUE_VARIABLE: Enqueue as many items as possible from ring
+ * @param old_head
+ *   Returns head value as it was before the move, i.e. where enqueue starts
+ * @param new_head
+ *   Returns the current/new head value i.e. where enqueue finishes
+ * @param free_entries
+ *   Returns the amount of free space in the ring BEFORE head was moved
+ * @return
+ *   Actual number of objects enqueued.
+ *   If behavior == RTE_RING_QUEUE_FIXED, this will be 0 or n only.
+ */
+static __rte_always_inline unsigned int
+__rte_ring_move_prod_head(struct rte_ring *r, int is_sp,
+		unsigned int n, enum rte_ring_queue_behavior behavior,
+		uint32_t *old_head, uint32_t *new_head,
+		uint32_t *free_entries)
+{
+	const uint32_t capacity = r->capacity;
+	unsigned int max = n;
+	int success;
+
+	do {
+		/* Reset n to the initial burst count */
+		n = max;
+
+		*old_head = r->prod.head;
+
+		/* add rmb barrier to avoid load/load reorder in weak
+		 * memory model. It is noop on x86 */
+		rte_smp_rmb();
+
+		const uint32_t cons_tail = r->cons.tail;
+		/*
+		 *  The subtraction is done between two unsigned 32bits value
+		 * (the result is always modulo 32 bits even if we have
+		 * *old_head > cons_tail). So 'free_entries' is always between 0
+		 * and capacity (which is < size).
+		 */
+		*free_entries = (capacity + cons_tail - *old_head);
+
+		/* check that we have enough room in ring */
+		if (unlikely(n > *free_entries))
+			n = (behavior == RTE_RING_QUEUE_FIXED) ?
+					0 : *free_entries;
+
+		if (n == 0)
+			return 0;
+
+		*new_head = *old_head + n;
+		if (is_sp)
+			r->prod.head = *new_head, success = 1;
+		else
+			success = rte_atomic32_cmpset(&r->prod.head,
+					*old_head, *new_head);
+	} while (unlikely(success == 0));
+	return n;
+}
+
+/**
+ * @internal This function updates the consumer head for dequeue
+ *
+ * @param r
+ *   A pointer to the ring structure
+ * @param is_sc
+ *   Indicates whether multi-consumer path is needed or not
+ * @param n
+ *   The number of elements we will want to enqueue, i.e. how far should the
+ *   head be moved
+ * @param behavior
+ *   RTE_RING_QUEUE_FIXED:    Dequeue a fixed number of items from a ring
+ *   RTE_RING_QUEUE_VARIABLE: Dequeue as many items as possible from ring
+ * @param old_head
+ *   Returns head value as it was before the move, i.e. where dequeue starts
+ * @param new_head
+ *   Returns the current/new head value i.e. where dequeue finishes
+ * @param entries
+ *   Returns the number of entries in the ring BEFORE head was moved
+ * @return
+ *   - Actual number of objects dequeued.
+ *     If behavior == RTE_RING_QUEUE_FIXED, this will be 0 or n only.
+ */
+static __rte_always_inline unsigned int
+__rte_ring_move_cons_head(struct rte_ring *r, int is_sc,
+		unsigned int n, enum rte_ring_queue_behavior behavior,
+		uint32_t *old_head, uint32_t *new_head,
+		uint32_t *entries)
+{
+	unsigned int max = n;
+	int success;
+
+	/* move cons.head atomically */
+	do {
+		/* Restore n as it may change every loop */
+		n = max;
+
+		*old_head = r->cons.head;
+
+		/* add rmb barrier to avoid load/load reorder in weak
+		 * memory model. It is noop on x86 */
+		rte_smp_rmb();
+
+		const uint32_t prod_tail = r->prod.tail;
+		/* The subtraction is done between two unsigned 32bits value
+		 * (the result is always modulo 32 bits even if we have
+		 * cons_head > prod_tail). So 'entries' is always between 0
+		 * and size(ring)-1. */
+		*entries = (prod_tail - *old_head);
+
+		/* Set the actual entries for dequeue */
+		if (n > *entries)
+			n = (behavior == RTE_RING_QUEUE_FIXED) ? 0 : *entries;
+
+		if (unlikely(n == 0))
+			return 0;
+
+		*new_head = *old_head + n;
+		if (is_sc)
+			r->cons.head = *new_head, success = 1;
+		else
+			success = rte_atomic32_cmpset(&r->cons.head, *old_head,
+					*new_head);
+	} while (unlikely(success == 0));
+	return n;
+}
+
+#endif /* _RTE_RING_GENERIC_H_ */
-- 
2.7.4

Re: [PATCH 2/3] ring: guarantee load ordering of cons/prod when doing enqueue/dequeue

[Jerin Jacob]

-----Original Message-----
> Date: Wed,  8 Nov 2017 06:17:11 +0000
> From: Jia He <hejianet@gmail.com>
> To: jerin.jacob@caviumnetworks.com, dev@dpdk.org, olivier.matz@6wind.com
> Cc: konstantin.ananyev@intel.com, bruce.richardson@intel.com,
>  jianbo.liu@arm.com, hemant.agrawal@nxp.com, Jia He <hejianet@gmail.com>,
>  jie2.liu@hxt-semitech.com, bing.zhao@hxt-semitech.com,
>  jia.he@hxt-semitech.com
> Subject: [PATCH 2/3] ring: guarantee load ordering of cons/prod when doing
>  enqueue/dequeue
> X-Mailer: git-send-email 2.7.4
> 

It is the V3 patch and subject line does not reflect that. 
If you send a new version of a patch or series, you must use
--in-reply-to with message id of previous version.

> We watched a rte panic of mbuf_autotest in our qualcomm arm64 server.
> As for the possible race condition, please refer to [1].

Instead of the indirection of [1], add the technical details here to
make git comment useful.

> 
> Furthermore, to fix this race, there are 2 options as suggested by Jerin:
> 1. use rte_smp_rmb
> 2. use load_acquire/store_release(refer to [2]).
> CONFIG_RTE_RING_USE_C11_MEM_MODEL is provided, and by
> default it is "y" only on arm64.
> 
> The reason why providing 2 options is due to the performance benchmark
> difference in different arm machines, please refer to [3].
> 
> Already fuctionally tested on the machines as follows:
> - on X86
> - on arm64 with CONFIG_RTE_RING_USE_C11_MEM_MODEL=y
> - on arm64 with CONFIG_RTE_RING_USE_C11_MEM_MODEL=n
> 
> We haven't tested the ppc64 case. If anyone verifies it, he can add
> CONFIG_RTE_RING_USE_C11_MEM_MODEL=y to ppc64 config files.

The testing part can be moved under "---". ie in the comment section
below.

> 
> [1] http://dpdk.org/ml/archives/dev/2017-October/078366.html
> [2] https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L170
> [3] http://dpdk.org/ml/archives/dev/2017-October/080861.html
> 

Please split this patches into three:
- Your original fix
- Introduce rte_ring_generic.h and move the common function to that file
- Introduce CONFIG_RTE_RING_USE_C11_MEM_MODEL and move C11 memory
  implementation to lib/librte_ring/rte_ring_c11_mem.h

> ---
> +#ifndef _RTE_RING_C11_MEM_H_
> +#define _RTE_RING_C11_MEM_H_
> +
> +#define UNUSED_PARAMETER(x) ((void)(x))
> +static __rte_always_inline void
> +update_tail(struct rte_ring_headtail *ht, uint32_t old_val, uint32_t new_val,
> +		uint32_t single, uint32_t enqueue)
> +{
> +	UNUSED_PARAMETER(enqueue);

s/UNUSED_PARAMETER/RTE_SET_USED

Re: [PATCH 2/3] ring: guarantee load ordering of cons/prod when doing enqueue/dequeue

[Ananyev, Konstantin]

> +
> +#define UNUSED_PARAMETER(x) ((void)(x))
> +static __rte_always_inline void
> +update_tail(struct rte_ring_headtail *ht, uint32_t old_val, uint32_t new_val,
> +		uint32_t single, uint32_t enqueue)
> +{
> +	UNUSED_PARAMETER(enqueue);

As a nit you can use __rte_unused parameter attribute or
RTE_SET_USED() macro here.

Konstantin

[PATCH 3/3] config: support C11 memory model for arm64

[Jia He]

by default, CONFIG_RTE_RING_USE_C11_MEM_MODEL is y on arm64

Signed-off-by: Jia He <hejianet@gmail.com>
Signed-off-by: jia.he@hxt-semitech.com
---
 config/common_armv8a_linuxapp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config/common_armv8a_linuxapp b/config/common_armv8a_linuxapp
index 6732d1e..1bf6e4d 100644
--- a/config/common_armv8a_linuxapp
+++ b/config/common_armv8a_linuxapp
@@ -49,3 +49,4 @@ CONFIG_RTE_LIBRTE_SFC_EFX_PMD=n
 CONFIG_RTE_LIBRTE_AVP_PMD=n
 
 CONFIG_RTE_SCHED_VECTOR=n
+CONFIG_RTE_RING_USE_C11_MEM_MODEL=y
-- 
2.7.4

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Jerin Jacob]

-----Original Message-----
> Date: Wed,  8 Nov 2017 06:17:10 +0000
> From: Jia He <hejianet@gmail.com>
> To: jerin.jacob@caviumnetworks.com, dev@dpdk.org, olivier.matz@6wind.com
> Cc: konstantin.ananyev@intel.com, bruce.richardson@intel.com,
>  jianbo.liu@arm.com, hemant.agrawal@nxp.com, Jia He <hejianet@gmail.com>,
>  jia.he@hxt-semitech.com
> Subject: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(),dsb()
> X-Mailer: git-send-email 2.7.4
> 
> for the code as follows:
> if (condition)
> 	rte_smp_rmb();
> else
> 	rte_smp_wmb();
> Without this patch, compiler will report this error:
> error: 'else' without a previous 'if'
> 
> Signed-off-by: Jia He <hejianet@gmail.com>
> Signed-off-by: jia.he@hxt-semitech.com

# If both are same person then IMO, one sign-off is enough.
# Make sure the patches are ./devtools/checkpatches.sh and ./devtools/check-git-log.sh
clean
# Add Fixes: tag.
Example:
http://dpdk.org/ml/archives/dev/2017-November/081510.html

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Bruce Richardson]

On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote:
> for the code as follows:
> if (condition)
> 	rte_smp_rmb();
> else
> 	rte_smp_wmb();
> Without this patch, compiler will report this error:
> error: 'else' without a previous 'if'
> 
> Signed-off-by: Jia He <hejianet@gmail.com>
> Signed-off-by: jia.he@hxt-semitech.com
> ---
>  lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> index 0b70d62..38c3393 100644
> --- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> +++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> @@ -43,8 +43,8 @@ extern "C" {
>  
>  #include "generic/rte_atomic.h"
>  
> -#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
> -#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
> +#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
> +#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
>  

Need to remove the trailing ";" I too I think.
Alternatively, to keep the braces, the standard practice is to use
do { ... } while(0)

	Regards,
	/Bruce

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Jia He]

Hi Bruce


On 11/8/2017 6:28 PM, Bruce Richardson Wrote:
> On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote:
>> for the code as follows:
>> if (condition)
>> 	rte_smp_rmb();
>> else
>> 	rte_smp_wmb();
>> Without this patch, compiler will report this error:
>> error: 'else' without a previous 'if'
>>
>> Signed-off-by: Jia He <hejianet@gmail.com>
>> Signed-off-by: jia.he@hxt-semitech.com
>> ---
>>   lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>> index 0b70d62..38c3393 100644
>> --- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>> +++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>> @@ -43,8 +43,8 @@ extern "C" {
>>   
>>   #include "generic/rte_atomic.h"
>>   
>> -#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
>> -#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
>> +#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
>> +#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
>>   
> Need to remove the trailing ";" I too I think.
> Alternatively, to keep the braces, the standard practice is to use
> do { ... } while(0)
If trailing ";" is not removed
the code:
if (condition)
     rte_smp_rmb();
else
     anything();

will be like below after precompiling:
if (condition)
     asm volatile("dsb " "ld" : : : "memory");;
else
     anything();
Then, the same error - error: 'else' without a previous 'if'

If you choose do/while(0), yes, no errors from compiler.
But the checkpatch will report

WARNING: Single statement macros should not use a do {} while (0) loop
#11: FILE: lib/librte_eal/common/include/arch/arm/rte_atomic_64.h:46:
+#define dsb(opt) do { asm volatile("dsb " #opt : : : "memory"); } while (0)

I searched the kernel codes, the marco dsb() didn't use the do/while(0).
Which one do you think is better for dpdk?

Cheers,
Jia

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Jia He]

On 11/9/2017 9:22 AM, Jia He Wrote:
> Hi Bruce
>
>
> On 11/8/2017 6:28 PM, Bruce Richardson Wrote:
>> On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote:
>>> for the code as follows:
>>> if (condition)
>>>     rte_smp_rmb();
>>> else
>>>     rte_smp_wmb();
>>> Without this patch, compiler will report this error:
>>> error: 'else' without a previous 'if'
>>>
>>> Signed-off-by: Jia He <hejianet@gmail.com>
>>> Signed-off-by: jia.he@hxt-semitech.com
>>> ---
>>>   lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h 
>>> b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>>> index 0b70d62..38c3393 100644
>>> --- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>>> +++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>>> @@ -43,8 +43,8 @@ extern "C" {
>>>     #include "generic/rte_atomic.h"
>>>   -#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
>>> -#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
>>> +#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
>>> +#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
>> Need to remove the trailing ";" I too I think.
>> Alternatively, to keep the braces, the standard practice is to use
>> do { ... } while(0)
> If trailing ";" is not removed
> the code:
> if (condition)
>     rte_smp_rmb();
> else
>     anything();
>
> will be like below after precompiling:
> if (condition)
>     asm volatile("dsb " "ld" : : : "memory");;
> else
>     anything();
> Then, the same error - error: 'else' without a previous 'if'
>
Ignore my words above，thanks
sorry for the inconvenience.
And I've sent out v4 in this mail thread. The ";" has been removed.
If no more comments, I will send out v5 (2 patch sets for 17 and 18)

-- 
Cheers,
Jia


-- 
Cheers,
Jia

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Jianbo Liu]

The 11/09/2017 11:14, Jia He wrote:
>
>
> On 11/9/2017 9:22 AM, Jia He Wrote:
> >Hi Bruce
> >
> >
> >On 11/8/2017 6:28 PM, Bruce Richardson Wrote:
> >>On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote:
> >>>for the code as follows:
> >>>if (condition)
> >>>    rte_smp_rmb();
> >>>else
> >>>    rte_smp_wmb();
> >>>Without this patch, compiler will report this error:
> >>>error: 'else' without a previous 'if'
> >>>
> >>>Signed-off-by: Jia He <hejianet@gmail.com>
> >>>Signed-off-by: jia.he@hxt-semitech.com
> >>>---
> >>>  lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>>diff --git
> >>>a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>index 0b70d62..38c3393 100644
> >>>--- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>+++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>@@ -43,8 +43,8 @@ extern "C" {
> >>>    #include "generic/rte_atomic.h"
> >>>  -#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
> >>>-#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
> >>>+#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
> >>>+#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
> >>Need to remove the trailing ";" I too I think.
> >>Alternatively, to keep the braces, the standard practice is to use
> >>do { ... } while(0)
> >If trailing ";" is not removed
> >the code:
> >if (condition)
> >    rte_smp_rmb();
> >else
> >    anything();
> >

Sorry, why not use two different functions as your conditions passed in
are fixed in the calling functions.

> >will be like below after precompiling:
> >if (condition)
> >    asm volatile("dsb " "ld" : : : "memory");;
> >else
> >    anything();
> >Then, the same error - error: 'else' without a previous 'if'
> >
> Ignore my words above，thanks
> sorry for the inconvenience.
> And I've sent out v4 in this mail thread. The ";" has been removed.
> If no more comments, I will send out v5 (2 patch sets for 17 and 18)
>
> --
> Cheers,
> Jia
>
>
> --
> Cheers,
> Jia
>

--
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Jia He]

Hi Jianbo


On 11/9/2017 11:21 AM, Jianbo Liu Wrote:
> The 11/09/2017 11:14, Jia He wrote:
>>
>> On 11/9/2017 9:22 AM, Jia He Wrote:
>>> Hi Bruce
>>>
>>>
>>> On 11/8/2017 6:28 PM, Bruce Richardson Wrote:
>>>> On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote:
>>>>> for the code as follows:
>>>>> if (condition)
>>>>>      rte_smp_rmb();
>>>>> else
>>>>>      rte_smp_wmb();
>>>>> Without this patch, compiler will report this error:
>>>>> error: 'else' without a previous 'if'
>>>>>
>>>>> Signed-off-by: Jia He <hejianet@gmail.com>
>>>>> Signed-off-by: jia.he@hxt-semitech.com
>>>>> ---
>>>>>    lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git
>>>>> a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>>>>> b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>>>>> index 0b70d62..38c3393 100644
>>>>> --- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>>>>> +++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>>>>> @@ -43,8 +43,8 @@ extern "C" {
>>>>>      #include "generic/rte_atomic.h"
>>>>>    -#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
>>>>> -#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
>>>>> +#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
>>>>> +#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
>>>> Need to remove the trailing ";" I too I think.
>>>> Alternatively, to keep the braces, the standard practice is to use
>>>> do { ... } while(0)
>>> If trailing ";" is not removed
>>> the code:
>>> if (condition)
>>>      rte_smp_rmb();
>>> else
>>>      anything();
>>>
> Sorry, why not use two different functions as your conditions passed in
> are fixed in the calling functions.
Do you mean to split update_tail() into update_tail_enqueue() and 
update_tail_dequeue()?
Cheers,
Jia

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Jianbo Liu]

The 11/09/2017 12:43, Jia He wrote:
> Hi Jianbo
>
>
> On 11/9/2017 11:21 AM, Jianbo Liu Wrote:
> >The 11/09/2017 11:14, Jia He wrote:
> >>
> >>On 11/9/2017 9:22 AM, Jia He Wrote:
> >>>Hi Bruce
> >>>
> >>>
> >>>On 11/8/2017 6:28 PM, Bruce Richardson Wrote:
> >>>>On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote:
> >>>>>for the code as follows:
> >>>>>if (condition)
> >>>>>     rte_smp_rmb();
> >>>>>else
> >>>>>     rte_smp_wmb();
> >>>>>Without this patch, compiler will report this error:
> >>>>>error: 'else' without a previous 'if'
> >>>>>
> >>>>>Signed-off-by: Jia He <hejianet@gmail.com>
> >>>>>Signed-off-by: jia.he@hxt-semitech.com
> >>>>>---
> >>>>>   lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
> >>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>>diff --git
> >>>>>a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>>>b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>>>index 0b70d62..38c3393 100644
> >>>>>--- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>>>+++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>>>@@ -43,8 +43,8 @@ extern "C" {
> >>>>>     #include "generic/rte_atomic.h"
> >>>>>   -#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
> >>>>>-#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
> >>>>>+#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
> >>>>>+#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
> >>>>Need to remove the trailing ";" I too I think.
> >>>>Alternatively, to keep the braces, the standard practice is to use
> >>>>do { ... } while(0)
> >>>If trailing ";" is not removed
> >>>the code:
> >>>if (condition)
> >>>     rte_smp_rmb();
> >>>else
> >>>     anything();
> >>>
> >Sorry, why not use two different functions as your conditions passed in
> >are fixed in the calling functions.
> Do you mean to split update_tail() into update_tail_enqueue() and
> update_tail_dequeue()?

Yes. So it's not need to change dsb/dmb.

Jianbo
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Ananyev, Konstantin]

> -----Original Message-----
> From: Jianbo Liu [mailto:jianbo.liu@arm.com]
> Sent: Thursday, November 9, 2017 4:56 AM
> To: Jia He <hejianet@gmail.com>
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; jerin.jacob@caviumnetworks.com; dev@dpdk.org; olivier.matz@6wind.com;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>; hemant.agrawal@nxp.com; jia.he@hxt-semitech.com
> Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(),dsb()
> 
> The 11/09/2017 12:43, Jia He wrote:
> > Hi Jianbo
> >
> >
> > On 11/9/2017 11:21 AM, Jianbo Liu Wrote:
> > >The 11/09/2017 11:14, Jia He wrote:
> > >>
> > >>On 11/9/2017 9:22 AM, Jia He Wrote:
> > >>>Hi Bruce
> > >>>
> > >>>
> > >>>On 11/8/2017 6:28 PM, Bruce Richardson Wrote:
> > >>>>On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote:
> > >>>>>for the code as follows:
> > >>>>>if (condition)
> > >>>>>     rte_smp_rmb();
> > >>>>>else
> > >>>>>     rte_smp_wmb();
> > >>>>>Without this patch, compiler will report this error:
> > >>>>>error: 'else' without a previous 'if'
> > >>>>>
> > >>>>>Signed-off-by: Jia He <hejianet@gmail.com>
> > >>>>>Signed-off-by: jia.he@hxt-semitech.com
> > >>>>>---
> > >>>>>   lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
> > >>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
> > >>>>>
> > >>>>>diff --git
> > >>>>>a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> > >>>>>b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> > >>>>>index 0b70d62..38c3393 100644
> > >>>>>--- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> > >>>>>+++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> > >>>>>@@ -43,8 +43,8 @@ extern "C" {
> > >>>>>     #include "generic/rte_atomic.h"
> > >>>>>   -#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
> > >>>>>-#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
> > >>>>>+#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
> > >>>>>+#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
> > >>>>Need to remove the trailing ";" I too I think.
> > >>>>Alternatively, to keep the braces, the standard practice is to use
> > >>>>do { ... } while(0)
> > >>>If trailing ";" is not removed
> > >>>the code:
> > >>>if (condition)
> > >>>     rte_smp_rmb();
> > >>>else
> > >>>     anything();
> > >>>
> > >Sorry, why not use two different functions as your conditions passed in
> > >are fixed in the calling functions.
> > Do you mean to split update_tail() into update_tail_enqueue() and
> > update_tail_dequeue()?
> 
> Yes. So it's not need to change dsb/dmb.

That's a good idea - but you still might hit the same problem in 
Some different place in future...
Why not to convert these macros into 'always_inline' functions then?
Konstantin

> 
> Jianbo
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the
> intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or
> store or copy the information in any medium. Thank you.

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Jianbo Liu]

The 11/09/2017 09:38, Ananyev, Konstantin wrote:
>
>
> > -----Original Message-----
> > From: Jianbo Liu [mailto:jianbo.liu@arm.com]
> > Sent: Thursday, November 9, 2017 4:56 AM
> > To: Jia He <hejianet@gmail.com>
> > Cc: Richardson, Bruce <bruce.richardson@intel.com>; jerin.jacob@caviumnetworks.com; dev@dpdk.org; olivier.matz@6wind.com;
> > Ananyev, Konstantin <konstantin.ananyev@intel.com>; hemant.agrawal@nxp.com; jia.he@hxt-semitech.com
> > Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(),dsb()
> >
> > The 11/09/2017 12:43, Jia He wrote:
> > > Hi Jianbo
> > >
> > >
> > > On 11/9/2017 11:21 AM, Jianbo Liu Wrote:
> > > >The 11/09/2017 11:14, Jia He wrote:
> > > >>
> > > >>On 11/9/2017 9:22 AM, Jia He Wrote:
> > > >>>Hi Bruce
> > > >>>
> > > >>>
> > > >>>On 11/8/2017 6:28 PM, Bruce Richardson Wrote:
> > > >>>>On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote:
> > > >>>>>for the code as follows:
> > > >>>>>if (condition)
> > > >>>>>     rte_smp_rmb();
> > > >>>>>else
> > > >>>>>     rte_smp_wmb();
> > > >>>>>Without this patch, compiler will report this error:
> > > >>>>>error: 'else' without a previous 'if'
> > > >>>>>
> > > >>>>>Signed-off-by: Jia He <hejianet@gmail.com>
> > > >>>>>Signed-off-by: jia.he@hxt-semitech.com
> > > >>>>>---
> > > >>>>>   lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
> > > >>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
> > > >>>>>
> > > >>>>>diff --git
> > > >>>>>a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> > > >>>>>b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> > > >>>>>index 0b70d62..38c3393 100644
> > > >>>>>--- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> > > >>>>>+++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> > > >>>>>@@ -43,8 +43,8 @@ extern "C" {
> > > >>>>>     #include "generic/rte_atomic.h"
> > > >>>>>   -#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
> > > >>>>>-#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
> > > >>>>>+#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
> > > >>>>>+#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
> > > >>>>Need to remove the trailing ";" I too I think.
> > > >>>>Alternatively, to keep the braces, the standard practice is to use
> > > >>>>do { ... } while(0)
> > > >>>If trailing ";" is not removed
> > > >>>the code:
> > > >>>if (condition)
> > > >>>     rte_smp_rmb();
> > > >>>else
> > > >>>     anything();
> > > >>>
> > > >Sorry, why not use two different functions as your conditions passed in
> > > >are fixed in the calling functions.
> > > Do you mean to split update_tail() into update_tail_enqueue() and
> > > update_tail_dequeue()?
> >
> > Yes. So it's not need to change dsb/dmb.
>
> That's a good idea - but you still might hit the same problem in
> Some different place in future...

I think there is no any issue if we add {} for if/else sections.

-       if (enqueue)
+       if (enqueue) {
                rte_smp_wmb();
-       else
+       } else {
                rte_smp_rmb();
+       }

It's not a good way to ommit {}.

Jianbo

> Why not to convert these macros into 'always_inline' functions then?
> Konstantin
>
> >
> > Jianbo
> > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the
> > intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or
> > store or copy the information in any medium. Thank you.

--
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Ananyev, Konstantin]

> -----Original Message-----
> From: Jianbo Liu [mailto:jianbo.liu@arm.com]
> Sent: Thursday, November 9, 2017 9:58 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Jia He <hejianet@gmail.com>; Richardson, Bruce <bruce.richardson@intel.com>; jerin.jacob@caviumnetworks.com; dev@dpdk.org;
> olivier.matz@6wind.com; hemant.agrawal@nxp.com; jia.he@hxt-semitech.com
> Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(),dsb()
> 
> The 11/09/2017 09:38, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jianbo Liu [mailto:jianbo.liu@arm.com]
> > > Sent: Thursday, November 9, 2017 4:56 AM
> > > To: Jia He <hejianet@gmail.com>
> > > Cc: Richardson, Bruce <bruce.richardson@intel.com>; jerin.jacob@caviumnetworks.com; dev@dpdk.org; olivier.matz@6wind.com;
> > > Ananyev, Konstantin <konstantin.ananyev@intel.com>; hemant.agrawal@nxp.com; jia.he@hxt-semitech.com
> > > Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(),dsb()
> > >
> > > The 11/09/2017 12:43, Jia He wrote:
> > > > Hi Jianbo
> > > >
> > > >
> > > > On 11/9/2017 11:21 AM, Jianbo Liu Wrote:
> > > > >The 11/09/2017 11:14, Jia He wrote:
> > > > >>
> > > > >>On 11/9/2017 9:22 AM, Jia He Wrote:
> > > > >>>Hi Bruce
> > > > >>>
> > > > >>>
> > > > >>>On 11/8/2017 6:28 PM, Bruce Richardson Wrote:
> > > > >>>>On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote:
> > > > >>>>>for the code as follows:
> > > > >>>>>if (condition)
> > > > >>>>>     rte_smp_rmb();
> > > > >>>>>else
> > > > >>>>>     rte_smp_wmb();
> > > > >>>>>Without this patch, compiler will report this error:
> > > > >>>>>error: 'else' without a previous 'if'
> > > > >>>>>
> > > > >>>>>Signed-off-by: Jia He <hejianet@gmail.com>
> > > > >>>>>Signed-off-by: jia.he@hxt-semitech.com
> > > > >>>>>---
> > > > >>>>>   lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
> > > > >>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >>>>>
> > > > >>>>>diff --git
> > > > >>>>>a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> > > > >>>>>b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> > > > >>>>>index 0b70d62..38c3393 100644
> > > > >>>>>--- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> > > > >>>>>+++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> > > > >>>>>@@ -43,8 +43,8 @@ extern "C" {
> > > > >>>>>     #include "generic/rte_atomic.h"
> > > > >>>>>   -#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
> > > > >>>>>-#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
> > > > >>>>>+#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
> > > > >>>>>+#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
> > > > >>>>Need to remove the trailing ";" I too I think.
> > > > >>>>Alternatively, to keep the braces, the standard practice is to use
> > > > >>>>do { ... } while(0)
> > > > >>>If trailing ";" is not removed
> > > > >>>the code:
> > > > >>>if (condition)
> > > > >>>     rte_smp_rmb();
> > > > >>>else
> > > > >>>     anything();
> > > > >>>
> > > > >Sorry, why not use two different functions as your conditions passed in
> > > > >are fixed in the calling functions.
> > > > Do you mean to split update_tail() into update_tail_enqueue() and
> > > > update_tail_dequeue()?
> > >
> > > Yes. So it's not need to change dsb/dmb.
> >
> > That's a good idea - but you still might hit the same problem in
> > Some different place in future...
> 
> I think there is no any issue if we add {} for if/else sections.
> 
> -       if (enqueue)
> +       if (enqueue) {
>                 rte_smp_wmb();
> -       else
> +       } else {
>                 rte_smp_rmb();
> +       }
> 
> It's not a good way to ommit {}.

That's the preferred DPDK coding style these days :)
http://dpdk.org/doc/guides-16.04/contributing/coding_style.html
1.6.2. Control Statements and Loops

Include a space after keywords (if, while, for, return, switch).
Do not use braces ({ and }) for control statements with zero or just a single statement, unless that statement is more than a single line in which case the braces are permitted.

Konstantin

> 
> Jianbo
> 
> > Why not to convert these macros into 'always_inline' functions then?
> > Konstantin
> >
> > >
> > > Jianbo
> > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the
> > > intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or
> > > store or copy the information in any medium. Thank you.
> 
> --
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the
> intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or
> store or copy the information in any medium. Thank you.

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Jia He]

On 11/9/2017 5:38 PM, Ananyev, Konstantin Wrote:
>
>> -----Original Message-----
>> From: Jianbo Liu [mailto:jianbo.liu@arm.com]
>> Sent: Thursday, November 9, 2017 4:56 AM
>> To: Jia He <hejianet@gmail.com>
>> Cc: Richardson, Bruce <bruce.richardson@intel.com>; jerin.jacob@caviumnetworks.com; dev@dpdk.org; olivier.matz@6wind.com;
>> Ananyev, Konstantin <konstantin.ananyev@intel.com>; hemant.agrawal@nxp.com; jia.he@hxt-semitech.com
>> Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(),dsb()
>>
>> The 11/09/2017 12:43, Jia He wrote:
>>> Hi Jianbo
>>>
>>>
>>> On 11/9/2017 11:21 AM, Jianbo Liu Wrote:
>>>> The 11/09/2017 11:14, Jia He wrote:
>>>>> On 11/9/2017 9:22 AM, Jia He Wrote:
>>>>>> Hi Bruce
>>>>>>
>>>>>>
>>>>>> On 11/8/2017 6:28 PM, Bruce Richardson Wrote:
>>>>>>> On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote:
>>>>>>>> for the code as follows:
>>>>>>>> if (condition)
>>>>>>>>      rte_smp_rmb();
>>>>>>>> else
>>>>>>>>      rte_smp_wmb();
>>>>>>>> Without this patch, compiler will report this error:
>>>>>>>> error: 'else' without a previous 'if'
>>>>>>>>
>>>>>>>> Signed-off-by: Jia He <hejianet@gmail.com>
>>>>>>>> Signed-off-by: jia.he@hxt-semitech.com
>>>>>>>> ---
>>>>>>>>    lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
>>>>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>>>>>>>> b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>>>>>>>> index 0b70d62..38c3393 100644
>>>>>>>> --- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>>>>>>>> +++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
>>>>>>>> @@ -43,8 +43,8 @@ extern "C" {
>>>>>>>>      #include "generic/rte_atomic.h"
>>>>>>>>    -#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
>>>>>>>> -#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
>>>>>>>> +#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
>>>>>>>> +#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
>>>>>>> Need to remove the trailing ";" I too I think.
>>>>>>> Alternatively, to keep the braces, the standard practice is to use
>>>>>>> do { ... } while(0)
>>>>>> If trailing ";" is not removed
>>>>>> the code:
>>>>>> if (condition)
>>>>>>      rte_smp_rmb();
>>>>>> else
>>>>>>      anything();
>>>>>>
>>>> Sorry, why not use two different functions as your conditions passed in
>>>> are fixed in the calling functions.
>>> Do you mean to split update_tail() into update_tail_enqueue() and
>>> update_tail_dequeue()?
>> Yes. So it's not need to change dsb/dmb.
> That's a good idea - but you still might hit the same problem in
> Some different place in future...
> Why not to convert these macros into 'always_inline' functions then?
> Konstantin
>
It makes things more complex
opt needs to be redefined with types
such as : __attribute__((always_inline)) void dsb( char* opt)
and the input paramenter shoud be
#define sy "sy"
#define ld "ld"

And the "#" in asm codes needs to be considerred more.

IMO, the kernel way is simple and clean, isn't it?
#define dmb(opt) asm volatile("dmb " #opt : : : "memory")
Another choice is adding the do/while.

@Ananyev @Jianbo
Any thoughts?

-- 
Cheers,
Jia

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Jianbo Liu]

The 11/10/2017 10:06, Jia He wrote:
>
>
> On 11/9/2017 5:38 PM, Ananyev, Konstantin Wrote:
> >
> >>-----Original Message-----
> >>From: Jianbo Liu [mailto:jianbo.liu@arm.com]
> >>Sent: Thursday, November 9, 2017 4:56 AM
> >>To: Jia He <hejianet@gmail.com>
> >>Cc: Richardson, Bruce <bruce.richardson@intel.com>; jerin.jacob@caviumnetworks.com; dev@dpdk.org; olivier.matz@6wind.com;
> >>Ananyev, Konstantin <konstantin.ananyev@intel.com>; hemant.agrawal@nxp.com; jia.he@hxt-semitech.com
> >>Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(),dsb()
> >>
> >>The 11/09/2017 12:43, Jia He wrote:
> >>>Hi Jianbo
> >>>
> >>>
> >>>On 11/9/2017 11:21 AM, Jianbo Liu Wrote:
> >>>>The 11/09/2017 11:14, Jia He wrote:
> >>>>>On 11/9/2017 9:22 AM, Jia He Wrote:
> >>>>>>Hi Bruce
> >>>>>>
> >>>>>>
> >>>>>>On 11/8/2017 6:28 PM, Bruce Richardson Wrote:
> >>>>>>>On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote:
> >>>>>>>>for the code as follows:
> >>>>>>>>if (condition)
> >>>>>>>>     rte_smp_rmb();
> >>>>>>>>else
> >>>>>>>>     rte_smp_wmb();
> >>>>>>>>Without this patch, compiler will report this error:
> >>>>>>>>error: 'else' without a previous 'if'
> >>>>>>>>
> >>>>>>>>Signed-off-by: Jia He <hejianet@gmail.com>
> >>>>>>>>Signed-off-by: jia.he@hxt-semitech.com
> >>>>>>>>---
> >>>>>>>>   lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
> >>>>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>>>
> >>>>>>>>diff --git
> >>>>>>>>a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>>>>>>b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>>>>>>index 0b70d62..38c3393 100644
> >>>>>>>>--- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>>>>>>+++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>>>>>>@@ -43,8 +43,8 @@ extern "C" {
> >>>>>>>>     #include "generic/rte_atomic.h"
> >>>>>>>>   -#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
> >>>>>>>>-#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
> >>>>>>>>+#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
> >>>>>>>>+#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
> >>>>>>>Need to remove the trailing ";" I too I think.
> >>>>>>>Alternatively, to keep the braces, the standard practice is to use
> >>>>>>>do { ... } while(0)
> >>>>>>If trailing ";" is not removed
> >>>>>>the code:
> >>>>>>if (condition)
> >>>>>>     rte_smp_rmb();
> >>>>>>else
> >>>>>>     anything();
> >>>>>>
> >>>>Sorry, why not use two different functions as your conditions passed in
> >>>>are fixed in the calling functions.
> >>>Do you mean to split update_tail() into update_tail_enqueue() and
> >>>update_tail_dequeue()?
> >>Yes. So it's not need to change dsb/dmb.
> >That's a good idea - but you still might hit the same problem in
> >Some different place in future...
> >Why not to convert these macros into 'always_inline' functions then?
> >Konstantin
> >
> It makes things more complex
> opt needs to be redefined with types
> such as : __attribute__((always_inline)) void dsb( char* opt)
> and the input paramenter shoud be
> #define sy "sy"
> #define ld "ld"
>
> And the "#" in asm codes needs to be considerred more.
>
> IMO, the kernel way is simple and clean, isn't it?
> #define dmb(opt) asm volatile("dmb " #opt : : : "memory")
> Another choice is adding the do/while.
>
> @Ananyev @Jianbo
> Any thoughts?
>

I think it's a good to keep your v4.

Jianbo
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(), dsb()

[Ananyev, Konstantin]

Hi Jia,

> -----Original Message-----
> From: Jia He [mailto:hejianet@gmail.com]
> Sent: Friday, November 10, 2017 2:06 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Jianbo Liu <jianbo.liu@arm.com>
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; jerin.jacob@caviumnetworks.com; dev@dpdk.org; olivier.matz@6wind.com;
> hemant.agrawal@nxp.com; jia.he@hxt-semitech.com
> Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(),dsb()
> 
> 
> 
> On 11/9/2017 5:38 PM, Ananyev, Konstantin Wrote:
> >
> >> -----Original Message-----
> >> From: Jianbo Liu [mailto:jianbo.liu@arm.com]
> >> Sent: Thursday, November 9, 2017 4:56 AM
> >> To: Jia He <hejianet@gmail.com>
> >> Cc: Richardson, Bruce <bruce.richardson@intel.com>; jerin.jacob@caviumnetworks.com; dev@dpdk.org; olivier.matz@6wind.com;
> >> Ananyev, Konstantin <konstantin.ananyev@intel.com>; hemant.agrawal@nxp.com; jia.he@hxt-semitech.com
> >> Subject: Re: [PATCH 1/3] eal/arm64: remove the braces {} for dmb(),dsb()
> >>
> >> The 11/09/2017 12:43, Jia He wrote:
> >>> Hi Jianbo
> >>>
> >>>
> >>> On 11/9/2017 11:21 AM, Jianbo Liu Wrote:
> >>>> The 11/09/2017 11:14, Jia He wrote:
> >>>>> On 11/9/2017 9:22 AM, Jia He Wrote:
> >>>>>> Hi Bruce
> >>>>>>
> >>>>>>
> >>>>>> On 11/8/2017 6:28 PM, Bruce Richardson Wrote:
> >>>>>>> On Wed, Nov 08, 2017 at 06:17:10AM +0000, Jia He wrote:
> >>>>>>>> for the code as follows:
> >>>>>>>> if (condition)
> >>>>>>>>      rte_smp_rmb();
> >>>>>>>> else
> >>>>>>>>      rte_smp_wmb();
> >>>>>>>> Without this patch, compiler will report this error:
> >>>>>>>> error: 'else' without a previous 'if'
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jia He <hejianet@gmail.com>
> >>>>>>>> Signed-off-by: jia.he@hxt-semitech.com
> >>>>>>>> ---
> >>>>>>>>    lib/librte_eal/common/include/arch/arm/rte_atomic_64.h | 4 ++--
> >>>>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git
> >>>>>>>> a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>>>>>> b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>>>>>> index 0b70d62..38c3393 100644
> >>>>>>>> --- a/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>>>>>> +++ b/lib/librte_eal/common/include/arch/arm/rte_atomic_64.h
> >>>>>>>> @@ -43,8 +43,8 @@ extern "C" {
> >>>>>>>>      #include "generic/rte_atomic.h"
> >>>>>>>>    -#define dsb(opt)  { asm volatile("dsb " #opt : : : "memory"); }
> >>>>>>>> -#define dmb(opt)  { asm volatile("dmb " #opt : : : "memory"); }
> >>>>>>>> +#define dsb(opt) asm volatile("dsb " #opt : : : "memory");
> >>>>>>>> +#define dmb(opt) asm volatile("dmb " #opt : : : "memory");
> >>>>>>> Need to remove the trailing ";" I too I think.
> >>>>>>> Alternatively, to keep the braces, the standard practice is to use
> >>>>>>> do { ... } while(0)
> >>>>>> If trailing ";" is not removed
> >>>>>> the code:
> >>>>>> if (condition)
> >>>>>>      rte_smp_rmb();
> >>>>>> else
> >>>>>>      anything();
> >>>>>>
> >>>> Sorry, why not use two different functions as your conditions passed in
> >>>> are fixed in the calling functions.
> >>> Do you mean to split update_tail() into update_tail_enqueue() and
> >>> update_tail_dequeue()?
> >> Yes. So it's not need to change dsb/dmb.
> > That's a good idea - but you still might hit the same problem in
> > Some different place in future...
> > Why not to convert these macros into 'always_inline' functions then?
> > Konstantin
> >
> It makes things more complex
> opt needs to be redefined with types
> such as : __attribute__((always_inline)) void dsb( char* opt)
> and the input paramenter shoud be
> #define sy "sy"
> #define ld "ld"
> 
> And the "#" in asm codes needs to be considerred more.
> 
> IMO, the kernel way is simple and clean, isn't it?
> #define dmb(opt) asm volatile("dmb " #opt : : : "memory")

Fine by me.
Konstantin

> Another choice is adding the do/while.
> 
> @Ananyev @Jianbo
> Any thoughts?
> 
> --
> Cheers,
> Jia