DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 1/2] eal: remove redundant code
@ 2020-06-11 10:24 Phil Yang
  2020-06-11 10:24 ` [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status Phil Yang
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Phil Yang @ 2020-06-11 10:24 UTC (permalink / raw)
  To: dev; +Cc: drc, honnappa.nagarahalli, ruifeng.wang, nd, shahafs, stable

The event status has been cleaned up by the CAS operation when we free
the event data, so there is no need to set it to invalid after that.

Fixes: 49e2f374e45a ("eal/linux: support external Rx interrupt")
Cc: shahafs@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/librte_eal/linux/eal_interrupts.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/librte_eal/linux/eal_interrupts.c b/lib/librte_eal/linux/eal_interrupts.c
index 16e7a7d..2f369dc 100644
--- a/lib/librte_eal/linux/eal_interrupts.c
+++ b/lib/librte_eal/linux/eal_interrupts.c
@@ -1431,7 +1431,6 @@ rte_intr_free_epoll_fd(struct rte_intr_handle *intr_handle)
 		if (rte_epoll_ctl(rev->epfd, EPOLL_CTL_DEL, rev->fd, rev)) {
 			/* force free if the entry valid */
 			eal_epoll_data_safe_free(rev);
-			rev->status = RTE_EPOLL_INVALID;
 		}
 	}
 }
-- 
2.7.4


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
  2020-06-11 10:24 [dpdk-dev] [PATCH 1/2] eal: remove redundant code Phil Yang
@ 2020-06-11 10:24 ` Phil Yang
  2020-07-08  5:24   ` Honnappa Nagarahalli
                     ` (2 more replies)
  2020-07-08  5:14 ` [dpdk-dev] [PATCH 1/2] eal: remove redundant code Honnappa Nagarahalli
  2020-07-09  6:46 ` [dpdk-dev] [PATCH v2] eal: use c11 atomic built-ins for interrupt status Phil Yang
  2 siblings, 3 replies; 20+ messages in thread
From: Phil Yang @ 2020-06-11 10:24 UTC (permalink / raw)
  To: dev; +Cc: drc, honnappa.nagarahalli, ruifeng.wang, nd

The event status is defined as a volatile variable and shared
between threads. Use c11 atomics with explicit ordering instead
of rte_atomic ops which enforce unnecessary barriers on aarch64.

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/librte_eal/include/rte_eal_interrupts.h |  2 +-
 lib/librte_eal/linux/eal_interrupts.c       | 47 ++++++++++++++++++++---------
 2 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/lib/librte_eal/include/rte_eal_interrupts.h b/lib/librte_eal/include/rte_eal_interrupts.h
index 773a34a..b1e8a29 100644
--- a/lib/librte_eal/include/rte_eal_interrupts.h
+++ b/lib/librte_eal/include/rte_eal_interrupts.h
@@ -59,7 +59,7 @@ enum {
 
 /** interrupt epoll event obj, taken by epoll_event.ptr */
 struct rte_epoll_event {
-	volatile uint32_t status;  /**< OUT: event status */
+	uint32_t status;           /**< OUT: event status */
 	int fd;                    /**< OUT: event fd */
 	int epfd;       /**< OUT: epoll instance the ev associated with */
 	struct rte_epoll_data epdata;
diff --git a/lib/librte_eal/linux/eal_interrupts.c b/lib/librte_eal/linux/eal_interrupts.c
index 2f369dc..1486acf 100644
--- a/lib/librte_eal/linux/eal_interrupts.c
+++ b/lib/librte_eal/linux/eal_interrupts.c
@@ -26,7 +26,6 @@
 #include <rte_eal.h>
 #include <rte_per_lcore.h>
 #include <rte_lcore.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_debug.h>
 #include <rte_log.h>
@@ -1221,11 +1220,18 @@ eal_epoll_process_event(struct epoll_event *evs, unsigned int n,
 {
 	unsigned int i, count = 0;
 	struct rte_epoll_event *rev;
+	uint32_t valid_status;
 
 	for (i = 0; i < n; i++) {
 		rev = evs[i].data.ptr;
-		if (!rev || !rte_atomic32_cmpset(&rev->status, RTE_EPOLL_VALID,
-						 RTE_EPOLL_EXEC))
+		valid_status =  RTE_EPOLL_VALID;
+		/* ACQUIRE memory ordering here pairs with RELEASE
+		 * ordering bellow acting as a lock to synchronize
+		 * the event data updating.
+		 */
+		if (!rev || !__atomic_compare_exchange_n(&rev->status,
+				    &valid_status, RTE_EPOLL_EXEC, 0,
+				    __ATOMIC_ACQUIRE, __ATOMIC_RELAXED))
 			continue;
 
 		events[count].status        = RTE_EPOLL_VALID;
@@ -1237,8 +1243,11 @@ eal_epoll_process_event(struct epoll_event *evs, unsigned int n,
 			rev->epdata.cb_fun(rev->fd,
 					   rev->epdata.cb_arg);
 
-		rte_compiler_barrier();
-		rev->status = RTE_EPOLL_VALID;
+		/* the status update should be observed after
+		 * the other fields changes.
+		 */
+		__atomic_store_n(&rev->status, RTE_EPOLL_VALID,
+				__ATOMIC_RELEASE);
 		count++;
 	}
 	return count;
@@ -1308,10 +1317,14 @@ rte_epoll_wait(int epfd, struct rte_epoll_event *events,
 static inline void
 eal_epoll_data_safe_free(struct rte_epoll_event *ev)
 {
-	while (!rte_atomic32_cmpset(&ev->status, RTE_EPOLL_VALID,
-				    RTE_EPOLL_INVALID))
-		while (ev->status != RTE_EPOLL_VALID)
+	uint32_t valid_status = RTE_EPOLL_VALID;
+	while (!__atomic_compare_exchange_n(&ev->status, &valid_status,
+		    RTE_EPOLL_INVALID, 0, __ATOMIC_ACQUIRE, __ATOMIC_RELAXED)) {
+		while (__atomic_load_n(&ev->status,
+				__ATOMIC_RELAXED) != RTE_EPOLL_VALID)
 			rte_pause();
+		valid_status = RTE_EPOLL_VALID;
+	}
 	memset(&ev->epdata, 0, sizeof(ev->epdata));
 	ev->fd = -1;
 	ev->epfd = -1;
@@ -1333,7 +1346,8 @@ rte_epoll_ctl(int epfd, int op, int fd,
 		epfd = rte_intr_tls_epfd();
 
 	if (op == EPOLL_CTL_ADD) {
-		event->status = RTE_EPOLL_VALID;
+		__atomic_store_n(&event->status, RTE_EPOLL_VALID,
+				__ATOMIC_RELAXED);
 		event->fd = fd;  /* ignore fd in event */
 		event->epfd = epfd;
 		ev.data.ptr = (void *)event;
@@ -1345,11 +1359,13 @@ rte_epoll_ctl(int epfd, int op, int fd,
 			op, fd, strerror(errno));
 		if (op == EPOLL_CTL_ADD)
 			/* rollback status when CTL_ADD fail */
-			event->status = RTE_EPOLL_INVALID;
+			__atomic_store_n(&event->status, RTE_EPOLL_INVALID,
+					__ATOMIC_RELAXED);
 		return -1;
 	}
 
-	if (op == EPOLL_CTL_DEL && event->status != RTE_EPOLL_INVALID)
+	if (op == EPOLL_CTL_DEL && __atomic_load_n(&event->status,
+			__ATOMIC_RELAXED) != RTE_EPOLL_INVALID)
 		eal_epoll_data_safe_free(event);
 
 	return 0;
@@ -1378,7 +1394,8 @@ rte_intr_rx_ctl(struct rte_intr_handle *intr_handle, int epfd,
 	case RTE_INTR_EVENT_ADD:
 		epfd_op = EPOLL_CTL_ADD;
 		rev = &intr_handle->elist[efd_idx];
-		if (rev->status != RTE_EPOLL_INVALID) {
+		if (__atomic_load_n(&rev->status,
+				__ATOMIC_RELAXED) != RTE_EPOLL_INVALID) {
 			RTE_LOG(INFO, EAL, "Event already been added.\n");
 			return -EEXIST;
 		}
@@ -1401,7 +1418,8 @@ rte_intr_rx_ctl(struct rte_intr_handle *intr_handle, int epfd,
 	case RTE_INTR_EVENT_DEL:
 		epfd_op = EPOLL_CTL_DEL;
 		rev = &intr_handle->elist[efd_idx];
-		if (rev->status == RTE_EPOLL_INVALID) {
+		if (__atomic_load_n(&rev->status,
+				__ATOMIC_RELAXED) == RTE_EPOLL_INVALID) {
 			RTE_LOG(INFO, EAL, "Event does not exist.\n");
 			return -EPERM;
 		}
@@ -1426,7 +1444,8 @@ rte_intr_free_epoll_fd(struct rte_intr_handle *intr_handle)
 
 	for (i = 0; i < intr_handle->nb_efd; i++) {
 		rev = &intr_handle->elist[i];
-		if (rev->status == RTE_EPOLL_INVALID)
+		if (__atomic_load_n(&rev->status,
+				__ATOMIC_RELAXED) == RTE_EPOLL_INVALID)
 			continue;
 		if (rte_epoll_ctl(rev->epfd, EPOLL_CTL_DEL, rev->fd, rev)) {
 			/* force free if the entry valid */
-- 
2.7.4


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] eal: remove redundant code
  2020-06-11 10:24 [dpdk-dev] [PATCH 1/2] eal: remove redundant code Phil Yang
  2020-06-11 10:24 ` [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status Phil Yang
@ 2020-07-08  5:14 ` Honnappa Nagarahalli
  2020-07-08  5:20   ` Phil Yang
  2020-07-09  6:46 ` [dpdk-dev] [PATCH v2] eal: use c11 atomic built-ins for interrupt status Phil Yang
  2 siblings, 1 reply; 20+ messages in thread
From: Honnappa Nagarahalli @ 2020-07-08  5:14 UTC (permalink / raw)
  To: Phil Yang, dev
  Cc: drc, Ruifeng Wang, nd, shahafs, stable, Honnappa Nagarahalli, nd

<snip>

> Subject: [PATCH 1/2] eal: remove redundant code
> 
> The event status has been cleaned up by the CAS operation when we free
> the event data, so there is no need to set it to invalid after that.
> 
> Fixes: 49e2f374e45a ("eal/linux: support external Rx interrupt")
> Cc: shahafs@mellanox.com
> Cc: stable@dpdk.org
This is not a bug fix. I think it is not worth backporting, suggest removing this.

> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Otherwise

Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

> ---
>  lib/librte_eal/linux/eal_interrupts.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/lib/librte_eal/linux/eal_interrupts.c
> b/lib/librte_eal/linux/eal_interrupts.c
> index 16e7a7d..2f369dc 100644
> --- a/lib/librte_eal/linux/eal_interrupts.c
> +++ b/lib/librte_eal/linux/eal_interrupts.c
> @@ -1431,7 +1431,6 @@ rte_intr_free_epoll_fd(struct rte_intr_handle
> *intr_handle)
>  		if (rte_epoll_ctl(rev->epfd, EPOLL_CTL_DEL, rev->fd, rev)) {
>  			/* force free if the entry valid */
>  			eal_epoll_data_safe_free(rev);
> -			rev->status = RTE_EPOLL_INVALID;
>  		}
>  	}
>  }
> --
> 2.7.4


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] eal: remove redundant code
  2020-07-08  5:14 ` [dpdk-dev] [PATCH 1/2] eal: remove redundant code Honnappa Nagarahalli
@ 2020-07-08  5:20   ` Phil Yang
  0 siblings, 0 replies; 20+ messages in thread
From: Phil Yang @ 2020-07-08  5:20 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev; +Cc: drc, Ruifeng Wang, nd, shahafs, stable, nd

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Wednesday, July 8, 2020 1:15 PM
> To: Phil Yang <Phil.Yang@arm.com>; dev@dpdk.org
> Cc: drc@linux.vnet.ibm.com; Ruifeng Wang <Ruifeng.Wang@arm.com>; nd
> <nd@arm.com>; shahafs@mellanox.com; stable@dpdk.org; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH 1/2] eal: remove redundant code
> 
> <snip>
> 
> > Subject: [PATCH 1/2] eal: remove redundant code
> >
> > The event status has been cleaned up by the CAS operation when we free
> > the event data, so there is no need to set it to invalid after that.
> >
> > Fixes: 49e2f374e45a ("eal/linux: support external Rx interrupt")
> > Cc: shahafs@mellanox.com
> > Cc: stable@dpdk.org
> This is not a bug fix. I think it is not worth backporting, suggest removing this.

OK.

> 
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Otherwise
> 
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> 
> > ---
> >  lib/librte_eal/linux/eal_interrupts.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/lib/librte_eal/linux/eal_interrupts.c
> > b/lib/librte_eal/linux/eal_interrupts.c
> > index 16e7a7d..2f369dc 100644
> > --- a/lib/librte_eal/linux/eal_interrupts.c
> > +++ b/lib/librte_eal/linux/eal_interrupts.c
> > @@ -1431,7 +1431,6 @@ rte_intr_free_epoll_fd(struct rte_intr_handle
> > *intr_handle)
> >  		if (rte_epoll_ctl(rev->epfd, EPOLL_CTL_DEL, rev->fd, rev)) {
> >  			/* force free if the entry valid */
> >  			eal_epoll_data_safe_free(rev);
> > -			rev->status = RTE_EPOLL_INVALID;
> >  		}
> >  	}
> >  }
> > --
> > 2.7.4


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
  2020-06-11 10:24 ` [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status Phil Yang
@ 2020-07-08  5:24   ` Honnappa Nagarahalli
  2020-07-08 11:41   ` Harman Kalra
  2020-07-08 12:29   ` David Marchand
  2 siblings, 0 replies; 20+ messages in thread
From: Honnappa Nagarahalli @ 2020-07-08  5:24 UTC (permalink / raw)
  To: Phil Yang, dev; +Cc: drc, Ruifeng Wang, nd, Honnappa Nagarahalli, nd

<snip>

> Subject: [PATCH 2/2] eal: use c11 atomics for interrupt status
nit, C11 atomic built-ins              ^^^^^^^^^^
> 
> The event status is defined as a volatile variable and shared between threads.
> Use c11 atomics with explicit ordering instead of rte_atomic ops which
nit,      ^^^^^^^^^^ same here
> enforce unnecessary barriers on aarch64.
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

Otherwise, it looks fine.

Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

> ---
>  lib/librte_eal/include/rte_eal_interrupts.h |  2 +-
>  lib/librte_eal/linux/eal_interrupts.c       | 47 ++++++++++++++++++++---------
>  2 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/librte_eal/include/rte_eal_interrupts.h
> b/lib/librte_eal/include/rte_eal_interrupts.h
> index 773a34a..b1e8a29 100644
> --- a/lib/librte_eal/include/rte_eal_interrupts.h
> +++ b/lib/librte_eal/include/rte_eal_interrupts.h
> @@ -59,7 +59,7 @@ enum {
> 
>  /** interrupt epoll event obj, taken by epoll_event.ptr */  struct
> rte_epoll_event {
> -	volatile uint32_t status;  /**< OUT: event status */
> +	uint32_t status;           /**< OUT: event status */
>  	int fd;                    /**< OUT: event fd */
>  	int epfd;       /**< OUT: epoll instance the ev associated with */
>  	struct rte_epoll_data epdata;
> diff --git a/lib/librte_eal/linux/eal_interrupts.c
> b/lib/librte_eal/linux/eal_interrupts.c
> index 2f369dc..1486acf 100644
> --- a/lib/librte_eal/linux/eal_interrupts.c
> +++ b/lib/librte_eal/linux/eal_interrupts.c
> @@ -26,7 +26,6 @@
>  #include <rte_eal.h>
>  #include <rte_per_lcore.h>
>  #include <rte_lcore.h>
> -#include <rte_atomic.h>
>  #include <rte_branch_prediction.h>
>  #include <rte_debug.h>
>  #include <rte_log.h>
> @@ -1221,11 +1220,18 @@ eal_epoll_process_event(struct epoll_event
> *evs, unsigned int n,  {
>  	unsigned int i, count = 0;
>  	struct rte_epoll_event *rev;
> +	uint32_t valid_status;
> 
>  	for (i = 0; i < n; i++) {
>  		rev = evs[i].data.ptr;
> -		if (!rev || !rte_atomic32_cmpset(&rev->status,
> RTE_EPOLL_VALID,
> -						 RTE_EPOLL_EXEC))
> +		valid_status =  RTE_EPOLL_VALID;
> +		/* ACQUIRE memory ordering here pairs with RELEASE
> +		 * ordering bellow acting as a lock to synchronize
> +		 * the event data updating.
> +		 */
> +		if (!rev || !__atomic_compare_exchange_n(&rev->status,
> +				    &valid_status, RTE_EPOLL_EXEC, 0,
> +				    __ATOMIC_ACQUIRE,
> __ATOMIC_RELAXED))
>  			continue;
> 
>  		events[count].status        = RTE_EPOLL_VALID;
> @@ -1237,8 +1243,11 @@ eal_epoll_process_event(struct epoll_event *evs,
> unsigned int n,
>  			rev->epdata.cb_fun(rev->fd,
>  					   rev->epdata.cb_arg);
> 
> -		rte_compiler_barrier();
> -		rev->status = RTE_EPOLL_VALID;
> +		/* the status update should be observed after
> +		 * the other fields changes.
> +		 */
> +		__atomic_store_n(&rev->status, RTE_EPOLL_VALID,
> +				__ATOMIC_RELEASE);
>  		count++;
>  	}
>  	return count;
> @@ -1308,10 +1317,14 @@ rte_epoll_wait(int epfd, struct rte_epoll_event
> *events,  static inline void  eal_epoll_data_safe_free(struct rte_epoll_event
> *ev)  {
> -	while (!rte_atomic32_cmpset(&ev->status, RTE_EPOLL_VALID,
> -				    RTE_EPOLL_INVALID))
> -		while (ev->status != RTE_EPOLL_VALID)
> +	uint32_t valid_status = RTE_EPOLL_VALID;
> +	while (!__atomic_compare_exchange_n(&ev->status, &valid_status,
> +		    RTE_EPOLL_INVALID, 0, __ATOMIC_ACQUIRE,
> __ATOMIC_RELAXED)) {
> +		while (__atomic_load_n(&ev->status,
> +				__ATOMIC_RELAXED) != RTE_EPOLL_VALID)
>  			rte_pause();
> +		valid_status = RTE_EPOLL_VALID;
> +	}
>  	memset(&ev->epdata, 0, sizeof(ev->epdata));
>  	ev->fd = -1;
>  	ev->epfd = -1;
> @@ -1333,7 +1346,8 @@ rte_epoll_ctl(int epfd, int op, int fd,
>  		epfd = rte_intr_tls_epfd();
> 
>  	if (op == EPOLL_CTL_ADD) {
> -		event->status = RTE_EPOLL_VALID;
> +		__atomic_store_n(&event->status, RTE_EPOLL_VALID,
> +				__ATOMIC_RELAXED);
>  		event->fd = fd;  /* ignore fd in event */
>  		event->epfd = epfd;
>  		ev.data.ptr = (void *)event;
> @@ -1345,11 +1359,13 @@ rte_epoll_ctl(int epfd, int op, int fd,
>  			op, fd, strerror(errno));
>  		if (op == EPOLL_CTL_ADD)
>  			/* rollback status when CTL_ADD fail */
> -			event->status = RTE_EPOLL_INVALID;
> +			__atomic_store_n(&event->status,
> RTE_EPOLL_INVALID,
> +					__ATOMIC_RELAXED);
>  		return -1;
>  	}
> 
> -	if (op == EPOLL_CTL_DEL && event->status != RTE_EPOLL_INVALID)
> +	if (op == EPOLL_CTL_DEL && __atomic_load_n(&event->status,
> +			__ATOMIC_RELAXED) != RTE_EPOLL_INVALID)
>  		eal_epoll_data_safe_free(event);
> 
>  	return 0;
> @@ -1378,7 +1394,8 @@ rte_intr_rx_ctl(struct rte_intr_handle *intr_handle,
> int epfd,
>  	case RTE_INTR_EVENT_ADD:
>  		epfd_op = EPOLL_CTL_ADD;
>  		rev = &intr_handle->elist[efd_idx];
> -		if (rev->status != RTE_EPOLL_INVALID) {
> +		if (__atomic_load_n(&rev->status,
> +				__ATOMIC_RELAXED) != RTE_EPOLL_INVALID)
> {
>  			RTE_LOG(INFO, EAL, "Event already been added.\n");
>  			return -EEXIST;
>  		}
> @@ -1401,7 +1418,8 @@ rte_intr_rx_ctl(struct rte_intr_handle *intr_handle,
> int epfd,
>  	case RTE_INTR_EVENT_DEL:
>  		epfd_op = EPOLL_CTL_DEL;
>  		rev = &intr_handle->elist[efd_idx];
> -		if (rev->status == RTE_EPOLL_INVALID) {
> +		if (__atomic_load_n(&rev->status,
> +				__ATOMIC_RELAXED) ==
> RTE_EPOLL_INVALID) {
>  			RTE_LOG(INFO, EAL, "Event does not exist.\n");
>  			return -EPERM;
>  		}
> @@ -1426,7 +1444,8 @@ rte_intr_free_epoll_fd(struct rte_intr_handle
> *intr_handle)
> 
>  	for (i = 0; i < intr_handle->nb_efd; i++) {
>  		rev = &intr_handle->elist[i];
> -		if (rev->status == RTE_EPOLL_INVALID)
> +		if (__atomic_load_n(&rev->status,
> +				__ATOMIC_RELAXED) ==
> RTE_EPOLL_INVALID)
>  			continue;
>  		if (rte_epoll_ctl(rev->epfd, EPOLL_CTL_DEL, rev->fd, rev)) {
>  			/* force free if the entry valid */
> --
> 2.7.4


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
  2020-06-11 10:24 ` [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status Phil Yang
  2020-07-08  5:24   ` Honnappa Nagarahalli
@ 2020-07-08 11:41   ` Harman Kalra
  2020-07-09  5:17     ` Phil Yang
  2020-07-08 12:29   ` David Marchand
  2 siblings, 1 reply; 20+ messages in thread
From: Harman Kalra @ 2020-07-08 11:41 UTC (permalink / raw)
  To: Phil Yang; +Cc: dev, drc, honnappa.nagarahalli, ruifeng.wang, nd

On Thu, Jun 11, 2020 at 06:24:25PM +0800, Phil Yang wrote:
> The event status is defined as a volatile variable and shared
> between threads. Use c11 atomics with explicit ordering instead
> of rte_atomic ops which enforce unnecessary barriers on aarch64.
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---

Patches looks fine to me with commit message changes suggested
by Honnappa, i.e. using C11 atomic built-ins.
Since first patch of the series will not be backported, so I
think both patches can be combined into one.

Reviewed-by: Harman Kalra <hkalra@marvell.com>

>  lib/librte_eal/include/rte_eal_interrupts.h |  2 +-
>  lib/librte_eal/linux/eal_interrupts.c       | 47 ++++++++++++++++++++---------
>  2 files changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/librte_eal/include/rte_eal_interrupts.h b/lib/librte_eal/include/rte_eal_interrupts.h
> index 773a34a..b1e8a29 100644
> --- a/lib/librte_eal/include/rte_eal_interrupts.h
> +++ b/lib/librte_eal/include/rte_eal_interrupts.h
> @@ -59,7 +59,7 @@ enum {
>  
>  /** interrupt epoll event obj, taken by epoll_event.ptr */
>  struct rte_epoll_event {
> -	volatile uint32_t status;  /**< OUT: event status */
> +	uint32_t status;           /**< OUT: event status */
>  	int fd;                    /**< OUT: event fd */
>  	int epfd;       /**< OUT: epoll instance the ev associated with */
>  	struct rte_epoll_data epdata;
> diff --git a/lib/librte_eal/linux/eal_interrupts.c b/lib/librte_eal/linux/eal_interrupts.c
> index 2f369dc..1486acf 100644
> --- a/lib/librte_eal/linux/eal_interrupts.c
> +++ b/lib/librte_eal/linux/eal_interrupts.c
> @@ -26,7 +26,6 @@
>  #include <rte_eal.h>
>  #include <rte_per_lcore.h>
>  #include <rte_lcore.h>
> -#include <rte_atomic.h>
>  #include <rte_branch_prediction.h>
>  #include <rte_debug.h>
>  #include <rte_log.h>
> @@ -1221,11 +1220,18 @@ eal_epoll_process_event(struct epoll_event *evs, unsigned int n,
>  {
>  	unsigned int i, count = 0;
>  	struct rte_epoll_event *rev;
> +	uint32_t valid_status;
>  
>  	for (i = 0; i < n; i++) {
>  		rev = evs[i].data.ptr;
> -		if (!rev || !rte_atomic32_cmpset(&rev->status, RTE_EPOLL_VALID,
> -						 RTE_EPOLL_EXEC))
> +		valid_status =  RTE_EPOLL_VALID;
> +		/* ACQUIRE memory ordering here pairs with RELEASE
> +		 * ordering bellow acting as a lock to synchronize
> +		 * the event data updating.
> +		 */
> +		if (!rev || !__atomic_compare_exchange_n(&rev->status,
> +				    &valid_status, RTE_EPOLL_EXEC, 0,
> +				    __ATOMIC_ACQUIRE, __ATOMIC_RELAXED))
>  			continue;
>  
>  		events[count].status        = RTE_EPOLL_VALID;
> @@ -1237,8 +1243,11 @@ eal_epoll_process_event(struct epoll_event *evs, unsigned int n,
>  			rev->epdata.cb_fun(rev->fd,
>  					   rev->epdata.cb_arg);
>  
> -		rte_compiler_barrier();
> -		rev->status = RTE_EPOLL_VALID;
> +		/* the status update should be observed after
> +		 * the other fields changes.
> +		 */
> +		__atomic_store_n(&rev->status, RTE_EPOLL_VALID,
> +				__ATOMIC_RELEASE);
>  		count++;
>  	}
>  	return count;
> @@ -1308,10 +1317,14 @@ rte_epoll_wait(int epfd, struct rte_epoll_event *events,
>  static inline void
>  eal_epoll_data_safe_free(struct rte_epoll_event *ev)
>  {
> -	while (!rte_atomic32_cmpset(&ev->status, RTE_EPOLL_VALID,
> -				    RTE_EPOLL_INVALID))
> -		while (ev->status != RTE_EPOLL_VALID)
> +	uint32_t valid_status = RTE_EPOLL_VALID;
> +	while (!__atomic_compare_exchange_n(&ev->status, &valid_status,
> +		    RTE_EPOLL_INVALID, 0, __ATOMIC_ACQUIRE, __ATOMIC_RELAXED)) {
> +		while (__atomic_load_n(&ev->status,
> +				__ATOMIC_RELAXED) != RTE_EPOLL_VALID)
>  			rte_pause();
> +		valid_status = RTE_EPOLL_VALID;
> +	}
>  	memset(&ev->epdata, 0, sizeof(ev->epdata));
>  	ev->fd = -1;
>  	ev->epfd = -1;
> @@ -1333,7 +1346,8 @@ rte_epoll_ctl(int epfd, int op, int fd,
>  		epfd = rte_intr_tls_epfd();
>  
>  	if (op == EPOLL_CTL_ADD) {
> -		event->status = RTE_EPOLL_VALID;
> +		__atomic_store_n(&event->status, RTE_EPOLL_VALID,
> +				__ATOMIC_RELAXED);
>  		event->fd = fd;  /* ignore fd in event */
>  		event->epfd = epfd;
>  		ev.data.ptr = (void *)event;
> @@ -1345,11 +1359,13 @@ rte_epoll_ctl(int epfd, int op, int fd,
>  			op, fd, strerror(errno));
>  		if (op == EPOLL_CTL_ADD)
>  			/* rollback status when CTL_ADD fail */
> -			event->status = RTE_EPOLL_INVALID;
> +			__atomic_store_n(&event->status, RTE_EPOLL_INVALID,
> +					__ATOMIC_RELAXED);
>  		return -1;
>  	}
>  
> -	if (op == EPOLL_CTL_DEL && event->status != RTE_EPOLL_INVALID)
> +	if (op == EPOLL_CTL_DEL && __atomic_load_n(&event->status,
> +			__ATOMIC_RELAXED) != RTE_EPOLL_INVALID)
>  		eal_epoll_data_safe_free(event);
>  
>  	return 0;
> @@ -1378,7 +1394,8 @@ rte_intr_rx_ctl(struct rte_intr_handle *intr_handle, int epfd,
>  	case RTE_INTR_EVENT_ADD:
>  		epfd_op = EPOLL_CTL_ADD;
>  		rev = &intr_handle->elist[efd_idx];
> -		if (rev->status != RTE_EPOLL_INVALID) {
> +		if (__atomic_load_n(&rev->status,
> +				__ATOMIC_RELAXED) != RTE_EPOLL_INVALID) {
>  			RTE_LOG(INFO, EAL, "Event already been added.\n");
>  			return -EEXIST;
>  		}
> @@ -1401,7 +1418,8 @@ rte_intr_rx_ctl(struct rte_intr_handle *intr_handle, int epfd,
>  	case RTE_INTR_EVENT_DEL:
>  		epfd_op = EPOLL_CTL_DEL;
>  		rev = &intr_handle->elist[efd_idx];
> -		if (rev->status == RTE_EPOLL_INVALID) {
> +		if (__atomic_load_n(&rev->status,
> +				__ATOMIC_RELAXED) == RTE_EPOLL_INVALID) {
>  			RTE_LOG(INFO, EAL, "Event does not exist.\n");
>  			return -EPERM;
>  		}
> @@ -1426,7 +1444,8 @@ rte_intr_free_epoll_fd(struct rte_intr_handle *intr_handle)
>  
>  	for (i = 0; i < intr_handle->nb_efd; i++) {
>  		rev = &intr_handle->elist[i];
> -		if (rev->status == RTE_EPOLL_INVALID)
> +		if (__atomic_load_n(&rev->status,
> +				__ATOMIC_RELAXED) == RTE_EPOLL_INVALID)
>  			continue;
>  		if (rte_epoll_ctl(rev->epfd, EPOLL_CTL_DEL, rev->fd, rev)) {
>  			/* force free if the entry valid */
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
  2020-06-11 10:24 ` [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status Phil Yang
  2020-07-08  5:24   ` Honnappa Nagarahalli
  2020-07-08 11:41   ` Harman Kalra
@ 2020-07-08 12:29   ` David Marchand
  2020-07-08 13:43     ` Aaron Conole
  2020-07-08 15:04     ` Kinsella, Ray
  2 siblings, 2 replies; 20+ messages in thread
From: David Marchand @ 2020-07-08 12:29 UTC (permalink / raw)
  To: Phil Yang, Aaron Conole
  Cc: dev, David Christensen, Honnappa Nagarahalli,
	Ruifeng Wang (Arm Technology China),
	nd, Dodji Seketeli, Neil Horman, Ray Kinsella, Harman Kalra

On Thu, Jun 11, 2020 at 12:25 PM Phil Yang <phil.yang@arm.com> wrote:
>
> The event status is defined as a volatile variable and shared
> between threads. Use c11 atomics with explicit ordering instead
> of rte_atomic ops which enforce unnecessary barriers on aarch64.
>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>  lib/librte_eal/include/rte_eal_interrupts.h |  2 +-
>  lib/librte_eal/linux/eal_interrupts.c       | 47 ++++++++++++++++++++---------
>  2 files changed, 34 insertions(+), 15 deletions(-)
>
> diff --git a/lib/librte_eal/include/rte_eal_interrupts.h b/lib/librte_eal/include/rte_eal_interrupts.h
> index 773a34a..b1e8a29 100644
> --- a/lib/librte_eal/include/rte_eal_interrupts.h
> +++ b/lib/librte_eal/include/rte_eal_interrupts.h
> @@ -59,7 +59,7 @@ enum {
>
>  /** interrupt epoll event obj, taken by epoll_event.ptr */
>  struct rte_epoll_event {
> -       volatile uint32_t status;  /**< OUT: event status */
> +       uint32_t status;           /**< OUT: event status */
>         int fd;                    /**< OUT: event fd */
>         int epfd;       /**< OUT: epoll instance the ev associated with */
>         struct rte_epoll_data epdata;

I got a reject from the ABI check in my env.

1 function with some indirect sub-type change:

  [C]'function int rte_pci_ioport_map(rte_pci_device*, int,
rte_pci_ioport*)' at pci.c:756:1 has some indirect sub-type changes:
    parameter 1 of type 'rte_pci_device*' has sub-type changes:
      in pointed to type 'struct rte_pci_device' at rte_bus_pci.h:57:1:
        type size hasn't changed
        1 data member changes (2 filtered):
         type of 'rte_intr_handle rte_pci_device::intr_handle' changed:
           type size hasn't changed
           1 data member change:
            type of 'rte_epoll_event rte_intr_handle::elist[512]' changed:
              array element type 'struct rte_epoll_event' changed:
                type size hasn't changed
                1 data member change:
                 type of 'volatile uint32_t rte_epoll_event::status' changed:
                   entity changed from 'volatile uint32_t' to 'typedef
uint32_t' at stdint-uintn.h:26:1
                   type size hasn't changed

              type size hasn't changed


This is probably harmless in our case (going from volatile to non
volatile), but it won't pass the check in the CI without an exception
rule.

Note: checking on the test-report ml, I saw nothing, but ovsrobot did
catch the issue with this change too, Aaron?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
  2020-07-08 12:29   ` David Marchand
@ 2020-07-08 13:43     ` Aaron Conole
  2020-07-08 13:59       ` David Marchand
  2020-07-08 15:04     ` Kinsella, Ray
  1 sibling, 1 reply; 20+ messages in thread
From: Aaron Conole @ 2020-07-08 13:43 UTC (permalink / raw)
  To: David Marchand
  Cc: Phil Yang, dev, David Christensen, Honnappa Nagarahalli,
	Ruifeng Wang \(Arm Technology China\),
	nd, Dodji Seketeli, Neil Horman, Ray Kinsella, Harman Kalra

David Marchand <david.marchand@redhat.com> writes:

> On Thu, Jun 11, 2020 at 12:25 PM Phil Yang <phil.yang@arm.com> wrote:
>>
>> The event status is defined as a volatile variable and shared
>> between threads. Use c11 atomics with explicit ordering instead
>> of rte_atomic ops which enforce unnecessary barriers on aarch64.
>>
>> Signed-off-by: Phil Yang <phil.yang@arm.com>
>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
>> ---
>>  lib/librte_eal/include/rte_eal_interrupts.h |  2 +-
>>  lib/librte_eal/linux/eal_interrupts.c       | 47 ++++++++++++++++++++---------
>>  2 files changed, 34 insertions(+), 15 deletions(-)
>>
>> diff --git a/lib/librte_eal/include/rte_eal_interrupts.h b/lib/librte_eal/include/rte_eal_interrupts.h
>> index 773a34a..b1e8a29 100644
>> --- a/lib/librte_eal/include/rte_eal_interrupts.h
>> +++ b/lib/librte_eal/include/rte_eal_interrupts.h
>> @@ -59,7 +59,7 @@ enum {
>>
>>  /** interrupt epoll event obj, taken by epoll_event.ptr */
>>  struct rte_epoll_event {
>> -       volatile uint32_t status;  /**< OUT: event status */
>> +       uint32_t status;           /**< OUT: event status */
>>         int fd;                    /**< OUT: event fd */
>>         int epfd;       /**< OUT: epoll instance the ev associated with */
>>         struct rte_epoll_data epdata;
>
> I got a reject from the ABI check in my env.
>
> 1 function with some indirect sub-type change:
>
>   [C]'function int rte_pci_ioport_map(rte_pci_device*, int,
> rte_pci_ioport*)' at pci.c:756:1 has some indirect sub-type changes:
>     parameter 1 of type 'rte_pci_device*' has sub-type changes:
>       in pointed to type 'struct rte_pci_device' at rte_bus_pci.h:57:1:
>         type size hasn't changed
>         1 data member changes (2 filtered):
>          type of 'rte_intr_handle rte_pci_device::intr_handle' changed:
>            type size hasn't changed
>            1 data member change:
>             type of 'rte_epoll_event rte_intr_handle::elist[512]' changed:
>               array element type 'struct rte_epoll_event' changed:
>                 type size hasn't changed
>                 1 data member change:
>                  type of 'volatile uint32_t rte_epoll_event::status' changed:
>                    entity changed from 'volatile uint32_t' to 'typedef
> uint32_t' at stdint-uintn.h:26:1
>                    type size hasn't changed
>
>               type size hasn't changed
>
>
> This is probably harmless in our case (going from volatile to non
> volatile), but it won't pass the check in the CI without an exception
> rule.
>
> Note: checking on the test-report ml, I saw nothing, but ovsrobot did
> catch the issue with this change too, Aaron?

I don't have archives back to Jun 11 on the robot server.  I think it
doesn't preserve forever (and the archives seem to go back only until
Jul 03).  I will update it.

I do see that we have a failed travis job:

https://travis-ci.org/github/ovsrobot/dpdk/builds/697180855

I'm surprised this didn't go out.  Have we seen other failures to report
of the ovs robot recently?  I can double check the job config.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
  2020-07-08 13:43     ` Aaron Conole
@ 2020-07-08 13:59       ` David Marchand
  2020-07-08 20:48         ` Aaron Conole
  0 siblings, 1 reply; 20+ messages in thread
From: David Marchand @ 2020-07-08 13:59 UTC (permalink / raw)
  To: Aaron Conole
  Cc: Phil Yang, dev, David Christensen, Honnappa Nagarahalli,
	Ruifeng Wang (Arm Technology China),
	nd, Dodji Seketeli, Neil Horman, Ray Kinsella, Harman Kalra

On Wed, Jul 8, 2020 at 3:43 PM Aaron Conole <aconole@redhat.com> wrote:
> > Note: checking on the test-report ml, I saw nothing, but ovsrobot did
> > catch the issue with this change too, Aaron?
>
> I don't have archives back to Jun 11 on the robot server.  I think it
> doesn't preserve forever (and the archives seem to go back only until
> Jul 03).  I will update it.
>
> I do see that we have a failed travis job:
>
> https://travis-ci.org/github/ovsrobot/dpdk/builds/697180855
>
> I'm surprised this didn't go out.  Have we seen other failures to report
> of the ovs robot recently?  I can double check the job config.
>

We do receive reports from the robot atm.
And I found one failure reported this morning.
http://mails.dpdk.org/archives/test-report/2020-July/142421.html

Maybe transient failures wrt mail delivery?
Just throwing an idea..


-- 
David Marchand


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
  2020-07-08 12:29   ` David Marchand
  2020-07-08 13:43     ` Aaron Conole
@ 2020-07-08 15:04     ` Kinsella, Ray
  2020-07-09  5:21       ` Phil Yang
  1 sibling, 1 reply; 20+ messages in thread
From: Kinsella, Ray @ 2020-07-08 15:04 UTC (permalink / raw)
  To: David Marchand, Phil Yang, Aaron Conole
  Cc: dev, David Christensen, Honnappa Nagarahalli,
	Ruifeng Wang (Arm Technology China),
	nd, Dodji Seketeli, Neil Horman, Harman Kalra



On 08/07/2020 13:29, David Marchand wrote:
> On Thu, Jun 11, 2020 at 12:25 PM Phil Yang <phil.yang@arm.com> wrote:
>>
>> The event status is defined as a volatile variable and shared
>> between threads. Use c11 atomics with explicit ordering instead
>> of rte_atomic ops which enforce unnecessary barriers on aarch64.
>>
>> Signed-off-by: Phil Yang <phil.yang@arm.com>
>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
>> ---
>>  lib/librte_eal/include/rte_eal_interrupts.h |  2 +-
>>  lib/librte_eal/linux/eal_interrupts.c       | 47 ++++++++++++++++++++---------
>>  2 files changed, 34 insertions(+), 15 deletions(-)
>>
>> diff --git a/lib/librte_eal/include/rte_eal_interrupts.h b/lib/librte_eal/include/rte_eal_interrupts.h
>> index 773a34a..b1e8a29 100644
>> --- a/lib/librte_eal/include/rte_eal_interrupts.h
>> +++ b/lib/librte_eal/include/rte_eal_interrupts.h
>> @@ -59,7 +59,7 @@ enum {
>>
>>  /** interrupt epoll event obj, taken by epoll_event.ptr */
>>  struct rte_epoll_event {
>> -       volatile uint32_t status;  /**< OUT: event status */
>> +       uint32_t status;           /**< OUT: event status */
>>         int fd;                    /**< OUT: event fd */
>>         int epfd;       /**< OUT: epoll instance the ev associated with */
>>         struct rte_epoll_data epdata;
> 
> I got a reject from the ABI check in my env.
> 
> 1 function with some indirect sub-type change:
> 
>   [C]'function int rte_pci_ioport_map(rte_pci_device*, int,
> rte_pci_ioport*)' at pci.c:756:1 has some indirect sub-type changes:
>     parameter 1 of type 'rte_pci_device*' has sub-type changes:
>       in pointed to type 'struct rte_pci_device' at rte_bus_pci.h:57:1:
>         type size hasn't changed
>         1 data member changes (2 filtered):
>          type of 'rte_intr_handle rte_pci_device::intr_handle' changed:
>            type size hasn't changed
>            1 data member change:
>             type of 'rte_epoll_event rte_intr_handle::elist[512]' changed:
>               array element type 'struct rte_epoll_event' changed:
>                 type size hasn't changed
>                 1 data member change:
>                  type of 'volatile uint32_t rte_epoll_event::status' changed:
>                    entity changed from 'volatile uint32_t' to 'typedef
> uint32_t' at stdint-uintn.h:26:1
>                    type size hasn't changed
> 
>               type size hasn't changed
> 
> 
> This is probably harmless in our case (going from volatile to non
> volatile), but it won't pass the check in the CI without an exception
> rule.
> 
> Note: checking on the test-report ml, I saw nothing, but ovsrobot did
> catch the issue with this change too, Aaron?
> 
> 
Agreed, probably harmless and requires something in libagigail.ignore. 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
  2020-07-08 13:59       ` David Marchand
@ 2020-07-08 20:48         ` Aaron Conole
  0 siblings, 0 replies; 20+ messages in thread
From: Aaron Conole @ 2020-07-08 20:48 UTC (permalink / raw)
  To: David Marchand
  Cc: Phil Yang, dev, David Christensen, Honnappa Nagarahalli,
	Ruifeng Wang \(Arm Technology China\),
	nd, Dodji Seketeli, Neil Horman, Ray Kinsella, Harman Kalra

David Marchand <david.marchand@redhat.com> writes:

> On Wed, Jul 8, 2020 at 3:43 PM Aaron Conole <aconole@redhat.com> wrote:
>> > Note: checking on the test-report ml, I saw nothing, but ovsrobot did
>> > catch the issue with this change too, Aaron?
>>
>> I don't have archives back to Jun 11 on the robot server.  I think it
>> doesn't preserve forever (and the archives seem to go back only until
>> Jul 03).  I will update it.
>>
>> I do see that we have a failed travis job:
>>
>> https://travis-ci.org/github/ovsrobot/dpdk/builds/697180855
>>
>> I'm surprised this didn't go out.  Have we seen other failures to report
>> of the ovs robot recently?  I can double check the job config.
>>
>
> We do receive reports from the robot atm.
> And I found one failure reported this morning.
> http://mails.dpdk.org/archives/test-report/2020-July/142421.html
>
> Maybe transient failures wrt mail delivery?
> Just throwing an idea..

It's possible.  I'll check out the mail server.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
  2020-07-08 11:41   ` Harman Kalra
@ 2020-07-09  5:17     ` Phil Yang
  0 siblings, 0 replies; 20+ messages in thread
From: Phil Yang @ 2020-07-09  5:17 UTC (permalink / raw)
  To: Harman Kalra; +Cc: dev, drc, Honnappa Nagarahalli, Ruifeng Wang, nd

> -----Original Message-----
> From: Harman Kalra <hkalra@marvell.com>
> Sent: Wednesday, July 8, 2020 7:41 PM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: dev@dpdk.org; drc@linux.vnet.ibm.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
> 
> On Thu, Jun 11, 2020 at 06:24:25PM +0800, Phil Yang wrote:
> > The event status is defined as a volatile variable and shared
> > between threads. Use c11 atomics with explicit ordering instead
> > of rte_atomic ops which enforce unnecessary barriers on aarch64.
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> 
> Patches looks fine to me with commit message changes suggested
> by Honnappa, i.e. using C11 atomic built-ins.
> Since first patch of the series will not be backported, so I
> think both patches can be combined into one.

OK. Thanks. 
I will update in the next version.

Thanks,
Phil

> 
> Reviewed-by: Harman Kalra <hkalra@marvell.com>
> 
<snip>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
  2020-07-08 15:04     ` Kinsella, Ray
@ 2020-07-09  5:21       ` Phil Yang
  0 siblings, 0 replies; 20+ messages in thread
From: Phil Yang @ 2020-07-09  5:21 UTC (permalink / raw)
  To: Kinsella, Ray, David Marchand, Aaron Conole
  Cc: dev, David Christensen, Honnappa Nagarahalli, Ruifeng Wang, nd,
	Dodji Seketeli, Neil Horman, Harman Kalra

> -----Original Message-----
> From: Kinsella, Ray <mdr@ashroe.eu>
> Sent: Wednesday, July 8, 2020 11:05 PM
> To: David Marchand <david.marchand@redhat.com>; Phil Yang
> <Phil.Yang@arm.com>; Aaron Conole <aconole@redhat.com>
> Cc: dev <dev@dpdk.org>; David Christensen <drc@linux.vnet.ibm.com>;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; Dodji Seketeli
> <dodji@redhat.com>; Neil Horman <nhorman@tuxdriver.com>; Harman
> Kalra <hkalra@marvell.com>
> Subject: Re: [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status
> 
> 
> 
> On 08/07/2020 13:29, David Marchand wrote:
> > On Thu, Jun 11, 2020 at 12:25 PM Phil Yang <phil.yang@arm.com> wrote:
> >>
> >> The event status is defined as a volatile variable and shared
> >> between threads. Use c11 atomics with explicit ordering instead
> >> of rte_atomic ops which enforce unnecessary barriers on aarch64.
> >>
> >> Signed-off-by: Phil Yang <phil.yang@arm.com>
> >> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> >> ---
> >>  lib/librte_eal/include/rte_eal_interrupts.h |  2 +-
> >>  lib/librte_eal/linux/eal_interrupts.c       | 47 ++++++++++++++++++++----
> -----
> >>  2 files changed, 34 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/lib/librte_eal/include/rte_eal_interrupts.h
> b/lib/librte_eal/include/rte_eal_interrupts.h
> >> index 773a34a..b1e8a29 100644
> >> --- a/lib/librte_eal/include/rte_eal_interrupts.h
> >> +++ b/lib/librte_eal/include/rte_eal_interrupts.h
> >> @@ -59,7 +59,7 @@ enum {
> >>
> >>  /** interrupt epoll event obj, taken by epoll_event.ptr */
> >>  struct rte_epoll_event {
> >> -       volatile uint32_t status;  /**< OUT: event status */
> >> +       uint32_t status;           /**< OUT: event status */
> >>         int fd;                    /**< OUT: event fd */
> >>         int epfd;       /**< OUT: epoll instance the ev associated with */
> >>         struct rte_epoll_data epdata;
> >
> > I got a reject from the ABI check in my env.
> >
> > 1 function with some indirect sub-type change:
> >
> >   [C]'function int rte_pci_ioport_map(rte_pci_device*, int,
> > rte_pci_ioport*)' at pci.c:756:1 has some indirect sub-type changes:
> >     parameter 1 of type 'rte_pci_device*' has sub-type changes:
> >       in pointed to type 'struct rte_pci_device' at rte_bus_pci.h:57:1:
> >         type size hasn't changed
> >         1 data member changes (2 filtered):
> >          type of 'rte_intr_handle rte_pci_device::intr_handle' changed:
> >            type size hasn't changed
> >            1 data member change:
> >             type of 'rte_epoll_event rte_intr_handle::elist[512]' changed:
> >               array element type 'struct rte_epoll_event' changed:
> >                 type size hasn't changed
> >                 1 data member change:
> >                  type of 'volatile uint32_t rte_epoll_event::status' changed:
> >                    entity changed from 'volatile uint32_t' to 'typedef
> > uint32_t' at stdint-uintn.h:26:1
> >                    type size hasn't changed
> >
> >               type size hasn't changed
> >
> >
> > This is probably harmless in our case (going from volatile to non
> > volatile), but it won't pass the check in the CI without an exception
> > rule.
> >
> > Note: checking on the test-report ml, I saw nothing, but ovsrobot did
> > catch the issue with this change too, Aaron?
> >
> >
> Agreed, probably harmless and requires something in libagigail.ignore.

OK. Will update libagigail.ignore in the next version.

Thanks,
Phil


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH v2] eal: use c11 atomic built-ins for interrupt status
  2020-06-11 10:24 [dpdk-dev] [PATCH 1/2] eal: remove redundant code Phil Yang
  2020-06-11 10:24 ` [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status Phil Yang
  2020-07-08  5:14 ` [dpdk-dev] [PATCH 1/2] eal: remove redundant code Honnappa Nagarahalli
@ 2020-07-09  6:46 ` Phil Yang
  2020-07-09  8:02   ` Stefan Puiu
  2020-07-09  8:34   ` [dpdk-dev] [PATCH v3] " Phil Yang
  2 siblings, 2 replies; 20+ messages in thread
From: Phil Yang @ 2020-07-09  6:46 UTC (permalink / raw)
  To: david.marchand, dev
  Cc: mdr, aconole, drc, Honnappa.Nagarahalli, Ruifeng.Wang, nd, dodji,
	nhorman, hkalra

The event status is defined as a volatile variable and shared between
threads. Use c11 atomic built-ins with explicit ordering instead of
rte_atomic ops which enforce unnecessary barriers on aarch64.

The event status has been cleaned up by the compare-and-swap operation
when we free the event data, so there is no need to set it to invalid
after that.

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Harman Kalra <hkalra@marvell.com>
---
v2:
1. Fixed typo.
2. Updated libabigail.abignore to pass ABI check.
3. Merged v1 two patches into one patch.

 devtools/libabigail.abignore                |  4 +++
 lib/librte_eal/include/rte_eal_interrupts.h |  2 +-
 lib/librte_eal/linux/eal_interrupts.c       | 48 ++++++++++++++++++++---------
 3 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index 0133f75..daa4631 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -48,6 +48,10 @@
         changed_enumerators = RTE_CRYPTO_AEAD_LIST_END
 [suppress_variable]
         name = rte_crypto_aead_algorithm_strings
+; Ignore updates of epoll event
+[suppress_type]
+        type_kind = struct
+        name = rte_epoll_event
 
 ;;;;;;;;;;;;;;;;;;;;;;
 ; Temporary exceptions till DPDK 20.11
diff --git a/lib/librte_eal/include/rte_eal_interrupts.h b/lib/librte_eal/include/rte_eal_interrupts.h
index 773a34a..b1e8a29 100644
--- a/lib/librte_eal/include/rte_eal_interrupts.h
+++ b/lib/librte_eal/include/rte_eal_interrupts.h
@@ -59,7 +59,7 @@ enum {
 
 /** interrupt epoll event obj, taken by epoll_event.ptr */
 struct rte_epoll_event {
-	volatile uint32_t status;  /**< OUT: event status */
+	uint32_t status;           /**< OUT: event status */
 	int fd;                    /**< OUT: event fd */
 	int epfd;       /**< OUT: epoll instance the ev associated with */
 	struct rte_epoll_data epdata;
diff --git a/lib/librte_eal/linux/eal_interrupts.c b/lib/librte_eal/linux/eal_interrupts.c
index 84eeaa1..7a50869 100644
--- a/lib/librte_eal/linux/eal_interrupts.c
+++ b/lib/librte_eal/linux/eal_interrupts.c
@@ -26,7 +26,6 @@
 #include <rte_eal.h>
 #include <rte_per_lcore.h>
 #include <rte_lcore.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_debug.h>
 #include <rte_log.h>
@@ -1221,11 +1220,18 @@ eal_epoll_process_event(struct epoll_event *evs, unsigned int n,
 {
 	unsigned int i, count = 0;
 	struct rte_epoll_event *rev;
+	uint32_t valid_status;
 
 	for (i = 0; i < n; i++) {
 		rev = evs[i].data.ptr;
-		if (!rev || !rte_atomic32_cmpset(&rev->status, RTE_EPOLL_VALID,
-						 RTE_EPOLL_EXEC))
+		valid_status =  RTE_EPOLL_VALID;
+		/* ACQUIRE memory ordering here pairs with RELEASE
+		 * ordering bellow acting as a lock to synchronize
+		 * the event data updating.
+		 */
+		if (!rev || !__atomic_compare_exchange_n(&rev->status,
+				    &valid_status, RTE_EPOLL_EXEC, 0,
+				    __ATOMIC_ACQUIRE, __ATOMIC_RELAXED))
 			continue;
 
 		events[count].status        = RTE_EPOLL_VALID;
@@ -1237,8 +1243,11 @@ eal_epoll_process_event(struct epoll_event *evs, unsigned int n,
 			rev->epdata.cb_fun(rev->fd,
 					   rev->epdata.cb_arg);
 
-		rte_compiler_barrier();
-		rev->status = RTE_EPOLL_VALID;
+		/* the status update should be observed after
+		 * the other fields changes.
+		 */
+		__atomic_store_n(&rev->status, RTE_EPOLL_VALID,
+				__ATOMIC_RELEASE);
 		count++;
 	}
 	return count;
@@ -1308,10 +1317,14 @@ rte_epoll_wait(int epfd, struct rte_epoll_event *events,
 static inline void
 eal_epoll_data_safe_free(struct rte_epoll_event *ev)
 {
-	while (!rte_atomic32_cmpset(&ev->status, RTE_EPOLL_VALID,
-				    RTE_EPOLL_INVALID))
-		while (ev->status != RTE_EPOLL_VALID)
+	uint32_t valid_status = RTE_EPOLL_VALID;
+	while (!__atomic_compare_exchange_n(&ev->status, &valid_status,
+		    RTE_EPOLL_INVALID, 0, __ATOMIC_ACQUIRE, __ATOMIC_RELAXED)) {
+		while (__atomic_load_n(&ev->status,
+				__ATOMIC_RELAXED) != RTE_EPOLL_VALID)
 			rte_pause();
+		valid_status = RTE_EPOLL_VALID;
+	}
 	memset(&ev->epdata, 0, sizeof(ev->epdata));
 	ev->fd = -1;
 	ev->epfd = -1;
@@ -1333,7 +1346,8 @@ rte_epoll_ctl(int epfd, int op, int fd,
 		epfd = rte_intr_tls_epfd();
 
 	if (op == EPOLL_CTL_ADD) {
-		event->status = RTE_EPOLL_VALID;
+		__atomic_store_n(&event->status, RTE_EPOLL_VALID,
+				__ATOMIC_RELAXED);
 		event->fd = fd;  /* ignore fd in event */
 		event->epfd = epfd;
 		ev.data.ptr = (void *)event;
@@ -1345,11 +1359,13 @@ rte_epoll_ctl(int epfd, int op, int fd,
 			op, fd, strerror(errno));
 		if (op == EPOLL_CTL_ADD)
 			/* rollback status when CTL_ADD fail */
-			event->status = RTE_EPOLL_INVALID;
+			__atomic_store_n(&event->status, RTE_EPOLL_INVALID,
+					__ATOMIC_RELAXED);
 		return -1;
 	}
 
-	if (op == EPOLL_CTL_DEL && event->status != RTE_EPOLL_INVALID)
+	if (op == EPOLL_CTL_DEL && __atomic_load_n(&event->status,
+			__ATOMIC_RELAXED) != RTE_EPOLL_INVALID)
 		eal_epoll_data_safe_free(event);
 
 	return 0;
@@ -1378,7 +1394,8 @@ rte_intr_rx_ctl(struct rte_intr_handle *intr_handle, int epfd,
 	case RTE_INTR_EVENT_ADD:
 		epfd_op = EPOLL_CTL_ADD;
 		rev = &intr_handle->elist[efd_idx];
-		if (rev->status != RTE_EPOLL_INVALID) {
+		if (__atomic_load_n(&rev->status,
+				__ATOMIC_RELAXED) != RTE_EPOLL_INVALID) {
 			RTE_LOG(INFO, EAL, "Event already been added.\n");
 			return -EEXIST;
 		}
@@ -1401,7 +1418,8 @@ rte_intr_rx_ctl(struct rte_intr_handle *intr_handle, int epfd,
 	case RTE_INTR_EVENT_DEL:
 		epfd_op = EPOLL_CTL_DEL;
 		rev = &intr_handle->elist[efd_idx];
-		if (rev->status == RTE_EPOLL_INVALID) {
+		if (__atomic_load_n(&rev->status,
+				__ATOMIC_RELAXED) == RTE_EPOLL_INVALID) {
 			RTE_LOG(INFO, EAL, "Event does not exist.\n");
 			return -EPERM;
 		}
@@ -1426,12 +1444,12 @@ rte_intr_free_epoll_fd(struct rte_intr_handle *intr_handle)
 
 	for (i = 0; i < intr_handle->nb_efd; i++) {
 		rev = &intr_handle->elist[i];
-		if (rev->status == RTE_EPOLL_INVALID)
+		if (__atomic_load_n(&rev->status,
+				__ATOMIC_RELAXED) == RTE_EPOLL_INVALID)
 			continue;
 		if (rte_epoll_ctl(rev->epfd, EPOLL_CTL_DEL, rev->fd, rev)) {
 			/* force free if the entry valid */
 			eal_epoll_data_safe_free(rev);
-			rev->status = RTE_EPOLL_INVALID;
 		}
 	}
 }
-- 
2.7.4


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v2] eal: use c11 atomic built-ins for interrupt status
  2020-07-09  6:46 ` [dpdk-dev] [PATCH v2] eal: use c11 atomic built-ins for interrupt status Phil Yang
@ 2020-07-09  8:02   ` Stefan Puiu
  2020-07-09  8:07     ` Phil Yang
  2020-07-09  8:34   ` [dpdk-dev] [PATCH v3] " Phil Yang
  1 sibling, 1 reply; 20+ messages in thread
From: Stefan Puiu @ 2020-07-09  8:02 UTC (permalink / raw)
  To: Phil Yang
  Cc: david.marchand, dev, mdr, aconole, drc, Honnappa.Nagarahalli,
	Ruifeng.Wang, nd, dodji, Neil Horman, hkalra

Hi,

Noticed 2 typos:

On Thu, Jul 9, 2020 at 9:46 AM Phil Yang <phil.yang@arm.com> wrote:
>
> The event status is defined as a volatile variable and shared between
> threads. Use c11 atomic built-ins with explicit ordering instead of
> rte_atomic ops which enforce unnecessary barriers on aarch64.
>
> The event status has been cleaned up by the compare-and-swap operation
> when we free the event data, so there is no need to set it to invalid
> after that.
>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Harman Kalra <hkalra@marvell.com>
> ---
> v2:
> 1. Fixed typo.
> 2. Updated libabigail.abignore to pass ABI check.
> 3. Merged v1 two patches into one patch.
>
>  devtools/libabigail.abignore                |  4 +++
>  lib/librte_eal/include/rte_eal_interrupts.h |  2 +-
>  lib/librte_eal/linux/eal_interrupts.c       | 48 ++++++++++++++++++++---------
>  3 files changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index 0133f75..daa4631 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -48,6 +48,10 @@
>          changed_enumerators = RTE_CRYPTO_AEAD_LIST_END
>  [suppress_variable]
>          name = rte_crypto_aead_algorithm_strings
> +; Ignore updates of epoll event
> +[suppress_type]
> +        type_kind = struct
> +        name = rte_epoll_event
>
>  ;;;;;;;;;;;;;;;;;;;;;;
>  ; Temporary exceptions till DPDK 20.11
> diff --git a/lib/librte_eal/include/rte_eal_interrupts.h b/lib/librte_eal/include/rte_eal_interrupts.h
> index 773a34a..b1e8a29 100644
> --- a/lib/librte_eal/include/rte_eal_interrupts.h
> +++ b/lib/librte_eal/include/rte_eal_interrupts.h
> @@ -59,7 +59,7 @@ enum {
>
>  /** interrupt epoll event obj, taken by epoll_event.ptr */
>  struct rte_epoll_event {
> -       volatile uint32_t status;  /**< OUT: event status */
> +       uint32_t status;           /**< OUT: event status */
>         int fd;                    /**< OUT: event fd */
>         int epfd;       /**< OUT: epoll instance the ev associated with */
>         struct rte_epoll_data epdata;
> diff --git a/lib/librte_eal/linux/eal_interrupts.c b/lib/librte_eal/linux/eal_interrupts.c
> index 84eeaa1..7a50869 100644
> --- a/lib/librte_eal/linux/eal_interrupts.c
> +++ b/lib/librte_eal/linux/eal_interrupts.c
> @@ -26,7 +26,6 @@
>  #include <rte_eal.h>
>  #include <rte_per_lcore.h>
>  #include <rte_lcore.h>
> -#include <rte_atomic.h>
>  #include <rte_branch_prediction.h>
>  #include <rte_debug.h>
>  #include <rte_log.h>
> @@ -1221,11 +1220,18 @@ eal_epoll_process_event(struct epoll_event *evs, unsigned int n,
>  {
>         unsigned int i, count = 0;
>         struct rte_epoll_event *rev;
> +       uint32_t valid_status;
>
>         for (i = 0; i < n; i++) {
>                 rev = evs[i].data.ptr;
> -               if (!rev || !rte_atomic32_cmpset(&rev->status, RTE_EPOLL_VALID,
> -                                                RTE_EPOLL_EXEC))
> +               valid_status =  RTE_EPOLL_VALID;
> +               /* ACQUIRE memory ordering here pairs with RELEASE
> +                * ordering bellow acting as a lock to synchronize
s/bellow/below

> +                * the event data updating.
> +                */
> +               if (!rev || !__atomic_compare_exchange_n(&rev->status,
> +                                   &valid_status, RTE_EPOLL_EXEC, 0,
> +                                   __ATOMIC_ACQUIRE, __ATOMIC_RELAXED))
>                         continue;
>
>                 events[count].status        = RTE_EPOLL_VALID;
> @@ -1237,8 +1243,11 @@ eal_epoll_process_event(struct epoll_event *evs, unsigned int n,
>                         rev->epdata.cb_fun(rev->fd,
>                                            rev->epdata.cb_arg);
>
> -               rte_compiler_barrier();
> -               rev->status = RTE_EPOLL_VALID;
> +               /* the status update should be observed after
> +                * the other fields changes.
s/fields changes/fields change/

Thanks,
Stefan.

> +                */
> +               __atomic_store_n(&rev->status, RTE_EPOLL_VALID,
> +                               __ATOMIC_RELEASE);
>                 count++;
>         }
>         return count;
> @@ -1308,10 +1317,14 @@ rte_epoll_wait(int epfd, struct rte_epoll_event *events,
>  static inline void
>  eal_epoll_data_safe_free(struct rte_epoll_event *ev)
>  {
> -       while (!rte_atomic32_cmpset(&ev->status, RTE_EPOLL_VALID,
> -                                   RTE_EPOLL_INVALID))
> -               while (ev->status != RTE_EPOLL_VALID)
> +       uint32_t valid_status = RTE_EPOLL_VALID;
> +       while (!__atomic_compare_exchange_n(&ev->status, &valid_status,
> +                   RTE_EPOLL_INVALID, 0, __ATOMIC_ACQUIRE, __ATOMIC_RELAXED)) {
> +               while (__atomic_load_n(&ev->status,
> +                               __ATOMIC_RELAXED) != RTE_EPOLL_VALID)
>                         rte_pause();
> +               valid_status = RTE_EPOLL_VALID;
> +       }
>         memset(&ev->epdata, 0, sizeof(ev->epdata));
>         ev->fd = -1;
>         ev->epfd = -1;
> @@ -1333,7 +1346,8 @@ rte_epoll_ctl(int epfd, int op, int fd,
>                 epfd = rte_intr_tls_epfd();
>
>         if (op == EPOLL_CTL_ADD) {
> -               event->status = RTE_EPOLL_VALID;
> +               __atomic_store_n(&event->status, RTE_EPOLL_VALID,
> +                               __ATOMIC_RELAXED);
>                 event->fd = fd;  /* ignore fd in event */
>                 event->epfd = epfd;
>                 ev.data.ptr = (void *)event;
> @@ -1345,11 +1359,13 @@ rte_epoll_ctl(int epfd, int op, int fd,
>                         op, fd, strerror(errno));
>                 if (op == EPOLL_CTL_ADD)
>                         /* rollback status when CTL_ADD fail */
> -                       event->status = RTE_EPOLL_INVALID;
> +                       __atomic_store_n(&event->status, RTE_EPOLL_INVALID,
> +                                       __ATOMIC_RELAXED);
>                 return -1;
>         }
>
> -       if (op == EPOLL_CTL_DEL && event->status != RTE_EPOLL_INVALID)
> +       if (op == EPOLL_CTL_DEL && __atomic_load_n(&event->status,
> +                       __ATOMIC_RELAXED) != RTE_EPOLL_INVALID)
>                 eal_epoll_data_safe_free(event);
>
>         return 0;
> @@ -1378,7 +1394,8 @@ rte_intr_rx_ctl(struct rte_intr_handle *intr_handle, int epfd,
>         case RTE_INTR_EVENT_ADD:
>                 epfd_op = EPOLL_CTL_ADD;
>                 rev = &intr_handle->elist[efd_idx];
> -               if (rev->status != RTE_EPOLL_INVALID) {
> +               if (__atomic_load_n(&rev->status,
> +                               __ATOMIC_RELAXED) != RTE_EPOLL_INVALID) {
>                         RTE_LOG(INFO, EAL, "Event already been added.\n");
>                         return -EEXIST;
>                 }
> @@ -1401,7 +1418,8 @@ rte_intr_rx_ctl(struct rte_intr_handle *intr_handle, int epfd,
>         case RTE_INTR_EVENT_DEL:
>                 epfd_op = EPOLL_CTL_DEL;
>                 rev = &intr_handle->elist[efd_idx];
> -               if (rev->status == RTE_EPOLL_INVALID) {
> +               if (__atomic_load_n(&rev->status,
> +                               __ATOMIC_RELAXED) == RTE_EPOLL_INVALID) {
>                         RTE_LOG(INFO, EAL, "Event does not exist.\n");
>                         return -EPERM;
>                 }
> @@ -1426,12 +1444,12 @@ rte_intr_free_epoll_fd(struct rte_intr_handle *intr_handle)
>
>         for (i = 0; i < intr_handle->nb_efd; i++) {
>                 rev = &intr_handle->elist[i];
> -               if (rev->status == RTE_EPOLL_INVALID)
> +               if (__atomic_load_n(&rev->status,
> +                               __ATOMIC_RELAXED) == RTE_EPOLL_INVALID)
>                         continue;
>                 if (rte_epoll_ctl(rev->epfd, EPOLL_CTL_DEL, rev->fd, rev)) {
>                         /* force free if the entry valid */
>                         eal_epoll_data_safe_free(rev);
> -                       rev->status = RTE_EPOLL_INVALID;
>                 }
>         }
>  }
> --
> 2.7.4
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v2] eal: use c11 atomic built-ins for interrupt status
  2020-07-09  8:02   ` Stefan Puiu
@ 2020-07-09  8:07     ` Phil Yang
  0 siblings, 0 replies; 20+ messages in thread
From: Phil Yang @ 2020-07-09  8:07 UTC (permalink / raw)
  To: Stefan Puiu
  Cc: david.marchand, dev, mdr, aconole, drc, Honnappa Nagarahalli,
	Ruifeng Wang, nd, dodji, Neil Horman, hkalra

> -----Original Message-----
> From: Stefan Puiu <stefan.puiu@gmail.com>
> Sent: Thursday, July 9, 2020 4:02 PM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: david.marchand@redhat.com; dev@dpdk.org; mdr@ashroe.eu;
> aconole@redhat.com; drc@linux.vnet.ibm.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; dodji@redhat.com; Neil
> Horman <nhorman@tuxdriver.com>; hkalra@marvell.com
> Subject: Re: [dpdk-dev] [PATCH v2] eal: use c11 atomic built-ins for interrupt
> status
> 
> Hi,
> 
> Noticed 2 typos:
Hi Stefan,

Thanks for your feedback.
Will do.

Thanks,
Phil

<snip>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH v3] eal: use c11 atomic built-ins for interrupt status
  2020-07-09  6:46 ` [dpdk-dev] [PATCH v2] eal: use c11 atomic built-ins for interrupt status Phil Yang
  2020-07-09  8:02   ` Stefan Puiu
@ 2020-07-09  8:34   ` Phil Yang
  2020-07-09 10:30     ` David Marchand
  2020-07-10  6:32     ` David Marchand
  1 sibling, 2 replies; 20+ messages in thread
From: Phil Yang @ 2020-07-09  8:34 UTC (permalink / raw)
  To: david.marchand, dev
  Cc: stefan.puiu, mdr, aconole, drc, Honnappa.Nagarahalli,
	Ruifeng.Wang, nd, dodji, nhorman, hkalra

The event status is defined as a volatile variable and shared between
threads. Use c11 atomic built-ins with explicit ordering instead of
rte_atomic ops which enforce unnecessary barriers on aarch64.

The event status has been cleaned up by the compare-and-swap operation
when we free the event data, so there is no need to set it to invalid
after that.

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Harman Kalra <hkalra@marvell.com>
---
v3:
Fixed typo.

v2:
1. Fixed typo.
2. Updated libabigail.abignore to pass ABI check.
3. Merged v1 two patches into one patch.

 devtools/libabigail.abignore                |  4 +++
 lib/librte_eal/include/rte_eal_interrupts.h |  2 +-
 lib/librte_eal/linux/eal_interrupts.c       | 48 ++++++++++++++++++++---------
 3 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index 0133f75..daa4631 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -48,6 +48,10 @@
         changed_enumerators = RTE_CRYPTO_AEAD_LIST_END
 [suppress_variable]
         name = rte_crypto_aead_algorithm_strings
+; Ignore updates of epoll event
+[suppress_type]
+        type_kind = struct
+        name = rte_epoll_event
 
 ;;;;;;;;;;;;;;;;;;;;;;
 ; Temporary exceptions till DPDK 20.11
diff --git a/lib/librte_eal/include/rte_eal_interrupts.h b/lib/librte_eal/include/rte_eal_interrupts.h
index 773a34a..b1e8a29 100644
--- a/lib/librte_eal/include/rte_eal_interrupts.h
+++ b/lib/librte_eal/include/rte_eal_interrupts.h
@@ -59,7 +59,7 @@ enum {
 
 /** interrupt epoll event obj, taken by epoll_event.ptr */
 struct rte_epoll_event {
-	volatile uint32_t status;  /**< OUT: event status */
+	uint32_t status;           /**< OUT: event status */
 	int fd;                    /**< OUT: event fd */
 	int epfd;       /**< OUT: epoll instance the ev associated with */
 	struct rte_epoll_data epdata;
diff --git a/lib/librte_eal/linux/eal_interrupts.c b/lib/librte_eal/linux/eal_interrupts.c
index 84eeaa1..ad09049 100644
--- a/lib/librte_eal/linux/eal_interrupts.c
+++ b/lib/librte_eal/linux/eal_interrupts.c
@@ -26,7 +26,6 @@
 #include <rte_eal.h>
 #include <rte_per_lcore.h>
 #include <rte_lcore.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_debug.h>
 #include <rte_log.h>
@@ -1221,11 +1220,18 @@ eal_epoll_process_event(struct epoll_event *evs, unsigned int n,
 {
 	unsigned int i, count = 0;
 	struct rte_epoll_event *rev;
+	uint32_t valid_status;
 
 	for (i = 0; i < n; i++) {
 		rev = evs[i].data.ptr;
-		if (!rev || !rte_atomic32_cmpset(&rev->status, RTE_EPOLL_VALID,
-						 RTE_EPOLL_EXEC))
+		valid_status =  RTE_EPOLL_VALID;
+		/* ACQUIRE memory ordering here pairs with RELEASE
+		 * ordering below acting as a lock to synchronize
+		 * the event data updating.
+		 */
+		if (!rev || !__atomic_compare_exchange_n(&rev->status,
+				    &valid_status, RTE_EPOLL_EXEC, 0,
+				    __ATOMIC_ACQUIRE, __ATOMIC_RELAXED))
 			continue;
 
 		events[count].status        = RTE_EPOLL_VALID;
@@ -1237,8 +1243,11 @@ eal_epoll_process_event(struct epoll_event *evs, unsigned int n,
 			rev->epdata.cb_fun(rev->fd,
 					   rev->epdata.cb_arg);
 
-		rte_compiler_barrier();
-		rev->status = RTE_EPOLL_VALID;
+		/* the status update should be observed after
+		 * the other fields change.
+		 */
+		__atomic_store_n(&rev->status, RTE_EPOLL_VALID,
+				__ATOMIC_RELEASE);
 		count++;
 	}
 	return count;
@@ -1308,10 +1317,14 @@ rte_epoll_wait(int epfd, struct rte_epoll_event *events,
 static inline void
 eal_epoll_data_safe_free(struct rte_epoll_event *ev)
 {
-	while (!rte_atomic32_cmpset(&ev->status, RTE_EPOLL_VALID,
-				    RTE_EPOLL_INVALID))
-		while (ev->status != RTE_EPOLL_VALID)
+	uint32_t valid_status = RTE_EPOLL_VALID;
+	while (!__atomic_compare_exchange_n(&ev->status, &valid_status,
+		    RTE_EPOLL_INVALID, 0, __ATOMIC_ACQUIRE, __ATOMIC_RELAXED)) {
+		while (__atomic_load_n(&ev->status,
+				__ATOMIC_RELAXED) != RTE_EPOLL_VALID)
 			rte_pause();
+		valid_status = RTE_EPOLL_VALID;
+	}
 	memset(&ev->epdata, 0, sizeof(ev->epdata));
 	ev->fd = -1;
 	ev->epfd = -1;
@@ -1333,7 +1346,8 @@ rte_epoll_ctl(int epfd, int op, int fd,
 		epfd = rte_intr_tls_epfd();
 
 	if (op == EPOLL_CTL_ADD) {
-		event->status = RTE_EPOLL_VALID;
+		__atomic_store_n(&event->status, RTE_EPOLL_VALID,
+				__ATOMIC_RELAXED);
 		event->fd = fd;  /* ignore fd in event */
 		event->epfd = epfd;
 		ev.data.ptr = (void *)event;
@@ -1345,11 +1359,13 @@ rte_epoll_ctl(int epfd, int op, int fd,
 			op, fd, strerror(errno));
 		if (op == EPOLL_CTL_ADD)
 			/* rollback status when CTL_ADD fail */
-			event->status = RTE_EPOLL_INVALID;
+			__atomic_store_n(&event->status, RTE_EPOLL_INVALID,
+					__ATOMIC_RELAXED);
 		return -1;
 	}
 
-	if (op == EPOLL_CTL_DEL && event->status != RTE_EPOLL_INVALID)
+	if (op == EPOLL_CTL_DEL && __atomic_load_n(&event->status,
+			__ATOMIC_RELAXED) != RTE_EPOLL_INVALID)
 		eal_epoll_data_safe_free(event);
 
 	return 0;
@@ -1378,7 +1394,8 @@ rte_intr_rx_ctl(struct rte_intr_handle *intr_handle, int epfd,
 	case RTE_INTR_EVENT_ADD:
 		epfd_op = EPOLL_CTL_ADD;
 		rev = &intr_handle->elist[efd_idx];
-		if (rev->status != RTE_EPOLL_INVALID) {
+		if (__atomic_load_n(&rev->status,
+				__ATOMIC_RELAXED) != RTE_EPOLL_INVALID) {
 			RTE_LOG(INFO, EAL, "Event already been added.\n");
 			return -EEXIST;
 		}
@@ -1401,7 +1418,8 @@ rte_intr_rx_ctl(struct rte_intr_handle *intr_handle, int epfd,
 	case RTE_INTR_EVENT_DEL:
 		epfd_op = EPOLL_CTL_DEL;
 		rev = &intr_handle->elist[efd_idx];
-		if (rev->status == RTE_EPOLL_INVALID) {
+		if (__atomic_load_n(&rev->status,
+				__ATOMIC_RELAXED) == RTE_EPOLL_INVALID) {
 			RTE_LOG(INFO, EAL, "Event does not exist.\n");
 			return -EPERM;
 		}
@@ -1426,12 +1444,12 @@ rte_intr_free_epoll_fd(struct rte_intr_handle *intr_handle)
 
 	for (i = 0; i < intr_handle->nb_efd; i++) {
 		rev = &intr_handle->elist[i];
-		if (rev->status == RTE_EPOLL_INVALID)
+		if (__atomic_load_n(&rev->status,
+				__ATOMIC_RELAXED) == RTE_EPOLL_INVALID)
 			continue;
 		if (rte_epoll_ctl(rev->epfd, EPOLL_CTL_DEL, rev->fd, rev)) {
 			/* force free if the entry valid */
 			eal_epoll_data_safe_free(rev);
-			rev->status = RTE_EPOLL_INVALID;
 		}
 	}
 }
-- 
2.7.4


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v3] eal: use c11 atomic built-ins for interrupt status
  2020-07-09  8:34   ` [dpdk-dev] [PATCH v3] " Phil Yang
@ 2020-07-09 10:30     ` David Marchand
  2020-07-10  7:18       ` Dodji Seketeli
  2020-07-10  6:32     ` David Marchand
  1 sibling, 1 reply; 20+ messages in thread
From: David Marchand @ 2020-07-09 10:30 UTC (permalink / raw)
  To: Phil Yang, Ray Kinsella, Harman Kalra
  Cc: dev, stefan.puiu, Aaron Conole, David Christensen,
	Honnappa Nagarahalli, Ruifeng Wang (Arm Technology China),
	nd, Dodji Seketeli, Neil Horman

On Thu, Jul 9, 2020 at 10:35 AM Phil Yang <phil.yang@arm.com> wrote:
>
> The event status is defined as a volatile variable and shared between
> threads. Use c11 atomic built-ins with explicit ordering instead of
> rte_atomic ops which enforce unnecessary barriers on aarch64.
>
> The event status has been cleaned up by the compare-and-swap operation
> when we free the event data, so there is no need to set it to invalid
> after that.
>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Harman Kalra <hkalra@marvell.com>
> ---
> v3:
> Fixed typo.
>
> v2:
> 1. Fixed typo.
> 2. Updated libabigail.abignore to pass ABI check.
> 3. Merged v1 two patches into one patch.
>
>  devtools/libabigail.abignore                |  4 +++
>  lib/librte_eal/include/rte_eal_interrupts.h |  2 +-
>  lib/librte_eal/linux/eal_interrupts.c       | 48 ++++++++++++++++++++---------
>  3 files changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index 0133f75..daa4631 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -48,6 +48,10 @@
>          changed_enumerators = RTE_CRYPTO_AEAD_LIST_END
>  [suppress_variable]
>          name = rte_crypto_aead_algorithm_strings
> +; Ignore updates of epoll event
> +[suppress_type]
> +        type_kind = struct
> +        name = rte_epoll_event

In general, ignoring all changes on a structure is risky.
But the risk is acceptable as long as we remember this for the rest of
the 20.08 release (and we will start from scratch for 20.11).


Without any comment from others, I'll merge this by the end of (my) day.

Thanks.

-- 
David Marchand


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v3] eal: use c11 atomic built-ins for interrupt status
  2020-07-09  8:34   ` [dpdk-dev] [PATCH v3] " Phil Yang
  2020-07-09 10:30     ` David Marchand
@ 2020-07-10  6:32     ` David Marchand
  1 sibling, 0 replies; 20+ messages in thread
From: David Marchand @ 2020-07-10  6:32 UTC (permalink / raw)
  To: Phil Yang
  Cc: dev, stefan.puiu, Ray Kinsella, Aaron Conole, David Christensen,
	Honnappa Nagarahalli, Ruifeng Wang (Arm Technology China),
	nd, Dodji Seketeli, Neil Horman, Harman Kalra

On Thu, Jul 9, 2020 at 10:35 AM Phil Yang <phil.yang@arm.com> wrote:
>
> The event status is defined as a volatile variable and shared between
> threads. Use c11 atomic built-ins with explicit ordering instead of
> rte_atomic ops which enforce unnecessary barriers on aarch64.
>
> The event status has been cleaned up by the compare-and-swap operation
> when we free the event data, so there is no need to set it to invalid
> after that.
>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Harman Kalra <hkalra@marvell.com>

Applied, thanks.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH v3] eal: use c11 atomic built-ins for interrupt status
  2020-07-09 10:30     ` David Marchand
@ 2020-07-10  7:18       ` Dodji Seketeli
  0 siblings, 0 replies; 20+ messages in thread
From: Dodji Seketeli @ 2020-07-10  7:18 UTC (permalink / raw)
  To: David Marchand
  Cc: Phil Yang, Ray Kinsella, Harman Kalra, dev, stefan.puiu,
	Aaron Conole, David Christensen, Honnappa Nagarahalli,
	Ruifeng Wang \(Arm Technology China\),
	nd, Neil Horman

David Marchand <david.marchand@redhat.com> writes:

[...]

>> --- a/devtools/libabigail.abignore
>> +++ b/devtools/libabigail.abignore
>> @@ -48,6 +48,10 @@
>>          changed_enumerators = RTE_CRYPTO_AEAD_LIST_END
>>  [suppress_variable]
>>          name = rte_crypto_aead_algorithm_strings
>> +; Ignore updates of epoll event
>> +[suppress_type]
>> +        type_kind = struct
>> +        name = rte_epoll_event
>
> In general, ignoring all changes on a structure is risky.
> But the risk is acceptable as long as we remember this for the rest of
> the 20.08 release (and we will start from scratch for 20.11).

Right, I thought about this too when I saw that change.  If that struct
is inherently *not* part of the logically exposed ABI, the risk is
really minimal as well.  In that case, maybe a comment saying so in the
.abignore file could be useful for future reference.

[...]

Cheers,

-- 
		Dodji


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-07-10  7:19 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-11 10:24 [dpdk-dev] [PATCH 1/2] eal: remove redundant code Phil Yang
2020-06-11 10:24 ` [dpdk-dev] [PATCH 2/2] eal: use c11 atomics for interrupt status Phil Yang
2020-07-08  5:24   ` Honnappa Nagarahalli
2020-07-08 11:41   ` Harman Kalra
2020-07-09  5:17     ` Phil Yang
2020-07-08 12:29   ` David Marchand
2020-07-08 13:43     ` Aaron Conole
2020-07-08 13:59       ` David Marchand
2020-07-08 20:48         ` Aaron Conole
2020-07-08 15:04     ` Kinsella, Ray
2020-07-09  5:21       ` Phil Yang
2020-07-08  5:14 ` [dpdk-dev] [PATCH 1/2] eal: remove redundant code Honnappa Nagarahalli
2020-07-08  5:20   ` Phil Yang
2020-07-09  6:46 ` [dpdk-dev] [PATCH v2] eal: use c11 atomic built-ins for interrupt status Phil Yang
2020-07-09  8:02   ` Stefan Puiu
2020-07-09  8:07     ` Phil Yang
2020-07-09  8:34   ` [dpdk-dev] [PATCH v3] " Phil Yang
2020-07-09 10:30     ` David Marchand
2020-07-10  7:18       ` Dodji Seketeli
2020-07-10  6:32     ` David Marchand

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git