[PATCH 0/2] dpdk header fixes for aarch64

[PATCH 0/2] dpdk header fixes for aarch64

[Andy Green]

On Xenial / 16.04 native aarch64 build with gcc 5.4.0,
lagopus build finds a couple more issues hiding in
dpdk headers.

---

Andy Green (2):
      ring: fix declaration after code
      ring: fix sign conversion warning


 lib/librte_ring/rte_ring.h         |    2 +-
 lib/librte_ring/rte_ring_c11_mem.h |    8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

--
Signature

[PATCH 1/2] ring: fix declaration after code

[Andy Green]

On gcc 5.4.0 / native aarch64 from Ubuntu 16.04:

/home/agreen/lagopus/src/dpdk/build/include/
rte_ring_c11_mem.h: In function
'__rte_ring_move_prod_head':
/home/agreen/lagopus/src/dpdk/build/include/
rte_ring_c11_mem.h:69:3: warning: ISO C90
forbids mixed declarations and code
[-Wdeclaration-after-statement]
   const uint32_t cons_tail = r->cons.tail;
   ^
/home/agreen/lagopus/src/dpdk/build/include/
rte_ring_c11_mem.h: In function
'__rte_ring_move_cons_head':
/home/agreen/lagopus/src/dpdk/build/include/
rte_ring_c11_mem.h:136:3: warning: ISO C90
forbids mixed declarations and code
[-Wdeclaration-after-statement]
   const uint32_t prod_tail = r->prod.tail;
   ^

Signed-off-by: Andy Green <andy@warmcat.com>
Fixes: 39368ebfc6 ("ring: introduce C11 memory model barrier option")
---
 lib/librte_ring/rte_ring_c11_mem.h |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_ring/rte_ring_c11_mem.h b/lib/librte_ring/rte_ring_c11_mem.h
index cb3f82b1a..3cc339558 100644
--- a/lib/librte_ring/rte_ring_c11_mem.h
+++ b/lib/librte_ring/rte_ring_c11_mem.h
@@ -61,12 +61,14 @@ __rte_ring_move_prod_head(struct rte_ring *r, unsigned int is_sp,
 	int success;
 
 	do {
+		const uint32_t cons_tail = r->cons.tail;
+
 		/* 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
@@ -129,11 +131,13 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc,
 
 	/* move cons.head atomically */
 	do {
+		const uint32_t prod_tail = r->prod.tail;
+
 		/* 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

Re: [PATCH 1/2] ring: fix declaration after code

[Gavin Hu]

Hi Andy,

See my inline comments.

-Gavin

-----Original Message-----
From: dev <dev-bounces@dpdk.org> On Behalf Of Andy Green
Sent: Monday, May 28, 2018 10:29 AM
To: dev@dpdk.org
Subject: [dpdk-dev] [PATCH 1/2] ring: fix declaration after code

On gcc 5.4.0 / native aarch64 from Ubuntu 16.04:

/home/agreen/lagopus/src/dpdk/build/include/
rte_ring_c11_mem.h: In function
'__rte_ring_move_prod_head':
/home/agreen/lagopus/src/dpdk/build/include/
rte_ring_c11_mem.h:69:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
   const uint32_t cons_tail = r->cons.tail;
   ^
/home/agreen/lagopus/src/dpdk/build/include/
rte_ring_c11_mem.h: In function
'__rte_ring_move_cons_head':
/home/agreen/lagopus/src/dpdk/build/include/
rte_ring_c11_mem.h:136:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
   const uint32_t prod_tail = r->prod.tail;
   ^

Signed-off-by: Andy Green <andy@warmcat.com>
Fixes: 39368ebfc6 ("ring: introduce C11 memory model barrier option")
---
 lib/librte_ring/rte_ring_c11_mem.h |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_ring/rte_ring_c11_mem.h b/lib/librte_ring/rte_ring_c11_mem.h
index cb3f82b1a..3cc339558 100644
--- a/lib/librte_ring/rte_ring_c11_mem.h
+++ b/lib/librte_ring/rte_ring_c11_mem.h
@@ -61,12 +61,14 @@ __rte_ring_move_prod_head(struct rte_ring *r, unsigned int is_sp,
 int success;

 do {
+const uint32_t cons_tail = r->cons.tail;
+
 /* 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;
+
[Gavin Hu] The ACQUIRE and RELEASE pair protects anything that between the two must be visible to other threads when they perform an acquire operation on the same memory address. Your changes broke this semantics.  I advise to move the declaration before and keep the assignment in the old place.
 /*
  *  The subtraction is done between two unsigned 32bits value
  * (the result is always modulo 32 bits even if we have @@ -129,11 +131,13 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc,

 /* move cons.head atomically */
 do {
+const uint32_t prod_tail = r->prod.tail;
+
 /* 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

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/2] ring: fix declaration after code

[Andy Green]

On 05/28/2018 04:15 PM, Gavin Hu wrote:

>   do {
> +const uint32_t cons_tail = r->cons.tail;
> +
>   /* 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;
> +
> [Gavin Hu] The ACQUIRE and RELEASE pair protects anything that between the two must be visible to other threads when they perform an acquire operation on the same memory address. Your changes broke this semantics.  I advise to move the declaration before and keep the assignment in the old place.

I see, thanks for the tip.

How about just get rid of this temp altogether if access to it is locked 
during this sequence anyway?  It's not like we had to sample it after 
the lock then, or it's bringing anything else to the party.

So instead of cons_tail / prod_tail at all, replace directly with 
r->cons.tail / r->prod.tail at the single usage for each.

-Andy

Re: [PATCH 1/2] ring: fix declaration after code

[Gavin Hu]

-----Original Message-----
From: Andy Green <andy@warmcat.com>
Sent: Monday, May 28, 2018 4:47 PM
To: Gavin Hu <Gavin.Hu@arm.com>; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 1/2] ring: fix declaration after code



On 05/28/2018 04:15 PM, Gavin Hu wrote:

>   do {
> +const uint32_t cons_tail = r->cons.tail;
> +
>   /* 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;
> +
> [Gavin Hu] The ACQUIRE and RELEASE pair protects anything that between the two must be visible to other threads when they perform an acquire operation on the same memory address. Your changes broke this semantics.  I advise to move the declaration before and keep the assignment in the old place.

I see, thanks for the tip.

How about just get rid of this temp altogether if access to it is locked during this sequence anyway?  It's not like we had to sample it after the lock then, or it's bringing anything else to the party.

So instead of cons_tail / prod_tail at all, replace directly with
r->cons.tail / r->prod.tail at the single usage for each.
[Gavin Hu] I think it is ok.

-Andy

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.

[PATCH 2/2] ring: fix sign conversion warning

[Andy Green]

On gcc 5.4.0 / native aarch64 from Ubuntu 16.04:

/home/agreen/lagopus/src/dpdk/build/include/rte_ring.h:
In function '__rte_ring_do_dequeue':
/home/agreen/lagopus/src/dpdk/build/include/rte_ring.h:
 385:35: warning: conversion to 'int' from 'unsigned int'
may change the sign of the result [-Wsign-conversion]
  n = __rte_ring_move_cons_head(r, is_sc, n, behavior,
                                   ^

Signed-off-by: Andy Green <andy@warmcat.com>
Fixes: e8ed5056c8 ("ring: remove signed type flip-flopping")
                                   ^
---
 lib/librte_ring/rte_ring.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index 124582251..de1a5f366 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -382,7 +382,7 @@ __rte_ring_do_dequeue(struct rte_ring *r, void **obj_table,
 	uint32_t cons_head, cons_next;
 	uint32_t entries;
 
-	n = __rte_ring_move_cons_head(r, is_sc, n, behavior,
+	n = __rte_ring_move_cons_head(r, (int)is_sc, n, behavior,
 			&cons_head, &cons_next, &entries);
 	if (n == 0)
 		goto end;

[PATCH v2 0/2] dpdk header fixes for aarch64

[Andy Green]

On Xenial / 16.04 native aarch64 build with gcc 5.4.0,
lagopus build finds a couple more issues hiding in
dpdk headers.

---

Andy Green (2):
      ring: fix declaration after code
      ring: fix sign conversion warning


 lib/librte_ring/rte_ring.h         |    2 +-
 lib/librte_ring/rte_ring_c11_mem.h |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

--
Signature

[PATCH v2 1/2] ring: fix declaration after code

[Andy Green]

On gcc 5.4.0 / native aarch64 from Ubuntu 16.04:

/home/agreen/lagopus/src/dpdk/build/include/
rte_ring_c11_mem.h: In function
'__rte_ring_move_prod_head':
/home/agreen/lagopus/src/dpdk/build/include/
rte_ring_c11_mem.h:69:3: warning: ISO C90
forbids mixed declarations and code
[-Wdeclaration-after-statement]
   const uint32_t cons_tail = r->cons.tail;
   ^
/home/agreen/lagopus/src/dpdk/build/include/
rte_ring_c11_mem.h: In function
'__rte_ring_move_cons_head':
/home/agreen/lagopus/src/dpdk/build/include/
rte_ring_c11_mem.h:136:3: warning: ISO C90
forbids mixed declarations and code
[-Wdeclaration-after-statement]
   const uint32_t prod_tail = r->prod.tail;
   ^

Signed-off-by: Andy Green <andy@warmcat.com>
Fixes: 39368ebfc6 ("ring: introduce C11 memory model barrier option")
---
 lib/librte_ring/rte_ring_c11_mem.h |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ring/rte_ring_c11_mem.h b/lib/librte_ring/rte_ring_c11_mem.h
index cb3f82b1a..94df3c4a6 100644
--- a/lib/librte_ring/rte_ring_c11_mem.h
+++ b/lib/librte_ring/rte_ring_c11_mem.h
@@ -66,14 +66,14 @@ __rte_ring_move_prod_head(struct rte_ring *r, unsigned int is_sp,
 
 		*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);
+		*free_entries = (capacity + r->cons.tail - *old_head);
 
 		/* check that we have enough room in ring */
 		if (unlikely(n > *free_entries))
@@ -133,13 +133,13 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc,
 		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);
+		*entries = (r->prod.tail - *old_head);
 
 		/* Set the actual entries for dequeue */
 		if (n > *entries)

Re: [PATCH v2 1/2] ring: fix declaration after code

[Olivier Matz]

On Mon, May 28, 2018 at 05:03:38PM +0800, Andy Green wrote:
> On gcc 5.4.0 / native aarch64 from Ubuntu 16.04:
> 
> /home/agreen/lagopus/src/dpdk/build/include/
> rte_ring_c11_mem.h: In function
> '__rte_ring_move_prod_head':
> /home/agreen/lagopus/src/dpdk/build/include/
> rte_ring_c11_mem.h:69:3: warning: ISO C90
> forbids mixed declarations and code
> [-Wdeclaration-after-statement]
>    const uint32_t cons_tail = r->cons.tail;
>    ^
> /home/agreen/lagopus/src/dpdk/build/include/
> rte_ring_c11_mem.h: In function
> '__rte_ring_move_cons_head':
> /home/agreen/lagopus/src/dpdk/build/include/
> rte_ring_c11_mem.h:136:3: warning: ISO C90
> forbids mixed declarations and code
> [-Wdeclaration-after-statement]
>    const uint32_t prod_tail = r->prod.tail;
>    ^
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> Fixes: 39368ebfc6 ("ring: introduce C11 memory model barrier option")

Acked-by: Olivier Matz <olivier.matz@6wind.com>

[PATCH v2 2/2] ring: fix sign conversion warning

[Andy Green]

On gcc 5.4.0 / native aarch64 from Ubuntu 16.04:

/home/agreen/lagopus/src/dpdk/build/include/rte_ring.h:
In function '__rte_ring_do_dequeue':
/home/agreen/lagopus/src/dpdk/build/include/rte_ring.h:
 385:35: warning: conversion to 'int' from 'unsigned int'
may change the sign of the result [-Wsign-conversion]
  n = __rte_ring_move_cons_head(r, is_sc, n, behavior,
                                   ^

Signed-off-by: Andy Green <andy@warmcat.com>
Fixes: e8ed5056c8 ("ring: remove signed type flip-flopping")
                                   ^
---
 lib/librte_ring/rte_ring.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
index 124582251..de1a5f366 100644
--- a/lib/librte_ring/rte_ring.h
+++ b/lib/librte_ring/rte_ring.h
@@ -382,7 +382,7 @@ __rte_ring_do_dequeue(struct rte_ring *r, void **obj_table,
 	uint32_t cons_head, cons_next;
 	uint32_t entries;
 
-	n = __rte_ring_move_cons_head(r, is_sc, n, behavior,
+	n = __rte_ring_move_cons_head(r, (int)is_sc, n, behavior,
 			&cons_head, &cons_next, &entries);
 	if (n == 0)
 		goto end;

Re: [PATCH v2 2/2] ring: fix sign conversion warning

[Olivier Matz]

On Mon, May 28, 2018 at 05:03:43PM +0800, Andy Green wrote:
> On gcc 5.4.0 / native aarch64 from Ubuntu 16.04:
> 
> /home/agreen/lagopus/src/dpdk/build/include/rte_ring.h:
> In function '__rte_ring_do_dequeue':
> /home/agreen/lagopus/src/dpdk/build/include/rte_ring.h:
>  385:35: warning: conversion to 'int' from 'unsigned int'
> may change the sign of the result [-Wsign-conversion]
>   n = __rte_ring_move_cons_head(r, is_sc, n, behavior,
>                                    ^
> 
> Signed-off-by: Andy Green <andy@warmcat.com>
> Fixes: e8ed5056c8 ("ring: remove signed type flip-flopping")

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [PATCH v2 0/2] dpdk header fixes for aarch64

[Thomas Monjalon]

> Andy Green (2):
>       ring: fix declaration after code
>       ring: fix sign conversion warning

Applied, thanks