diff mbox series

[2/2] eal: use c11 atomics for interrupt status

Message ID 1591871065-12461-2-git-send-email-phil.yang@arm.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers show
Series [1/2] eal: remove redundant code | expand

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Phil Yang June 11, 2020, 10:24 a.m. UTC
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(-)

Comments

Honnappa Nagarahalli July 8, 2020, 5:24 a.m. UTC | #1
<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
Harman Kalra July 8, 2020, 11:41 a.m. UTC | #2
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
>
David Marchand July 8, 2020, 12:29 p.m. UTC | #3
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?
Aaron Conole July 8, 2020, 1:43 p.m. UTC | #4
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.
David Marchand July 8, 2020, 1:59 p.m. UTC | #5
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..
Kinsella, Ray July 8, 2020, 3:04 p.m. UTC | #6
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.
Aaron Conole July 8, 2020, 8:48 p.m. UTC | #7
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.
Phil Yang July 9, 2020, 5:17 a.m. UTC | #8
> -----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>
Phil Yang July 9, 2020, 5:21 a.m. UTC | #9
> -----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
diff mbox series

Patch

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 */