DPDK patches and discussions
 help / color / mirror / Atom feed
From: Stefan Puiu <stefan.puiu@gmail.com>
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@arm.com, Ruifeng.Wang@arm.com,  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
Date: Thu, 9 Jul 2020 11:02:25 +0300	[thread overview]
Message-ID: <CACKs7VBFoe1TrpZiFh8wKyaGNWRmB3H3k_+fCbMR6_1Eox62HQ@mail.gmail.com> (raw)
In-Reply-To: <1594277162-15021-1-git-send-email-phil.yang@arm.com>

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
>

  reply	other threads:[~2020-07-09  8:02 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACKs7VBFoe1TrpZiFh8wKyaGNWRmB3H3k_+fCbMR6_1Eox62HQ@mail.gmail.com \
    --to=stefan.puiu@gmail.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=Ruifeng.Wang@arm.com \
    --cc=aconole@redhat.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=dodji@redhat.com \
    --cc=drc@linux.vnet.ibm.com \
    --cc=hkalra@marvell.com \
    --cc=mdr@ashroe.eu \
    --cc=nd@arm.com \
    --cc=nhorman@tuxdriver.com \
    --cc=phil.yang@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).