From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5AB14A0526; Thu, 9 Jul 2020 10:02:39 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0CAE61DCDF; Thu, 9 Jul 2020 10:02:39 +0200 (CEST) Received: from mail-pl1-f195.google.com (mail-pl1-f195.google.com [209.85.214.195]) by dpdk.org (Postfix) with ESMTP id 1D2A81DCDA for ; Thu, 9 Jul 2020 10:02:38 +0200 (CEST) Received: by mail-pl1-f195.google.com with SMTP id k5so505432plk.13 for ; Thu, 09 Jul 2020 01:02:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=O0odiepyXVwKlzBUrlNporSIx+JK7HIVFHdtVm8byfI=; b=Vah0kS6Jl9qjAHRUV7JORcLmPVmTR7Z6J3mE/hln61azespRnrWUp9qBr+YpWAeIYX txs17lm5cHnF4yXtYCiBVdS7mtZ8hSxRtlbO+lurwHIXk2UymPJ5vOZqYa6+Bf/5fTiO 6Y/K6XmgtwCk18kdGrRjpkRSVXEIFV5Vpez4Iyg5oBTxXs4AA+ofNGb+PwhKBJYqQIKc KmKleNU95rNTuBn0RBt4t87mJstu5nsgerHNo6kdNHgt0GtedizRb7nOUfBAgLnlOLfs KUYmx2FJJkRDvM223EdXN5N0nK39ic1roR8A/6hL4wqxxgin05K5c/aRofveqEZMKWhq mL5w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=O0odiepyXVwKlzBUrlNporSIx+JK7HIVFHdtVm8byfI=; b=l0nZzEyNiSXoucoJP0FuGydB0bh3wJBY0xNq4vrqO8z9niL2bR4NH2I+WFs8x6k8Uc HJYFPxbN67MpBaWdhI91mx2o26DnZ3v1oh5dzK8RO9Icqgqo8ExeJ10bkecYSPM64Ptq sSQAvWZ9Adtry6S+3eZISEhNxKOsQMTCJQRhcU6RMCUHj8t0bW1l+3rFI0r9Y/k1EGWP ZrQl+wG6Cztl9dJOB2LroaQIHEYnbenUuTY4CdvC2V+z0rSF9jgJ0y68moHttG+7zJ5g YAWC8FAZ1CO5TFVtdF5r5Y0432lJBC3QMP5JfMEpSg0kHM2UQe9C6Rr5PY/Zxanqbhk8 EsEw== X-Gm-Message-State: AOAM53322jdPn3LqDqMvMc1LevH3B91nfkLaPhDIb44AldzS/i4BuFcq tu7iPxWldttnGsK0EgZBCbUPq6J2tEso8U6s43U= X-Google-Smtp-Source: ABdhPJwD5uRLIGQknR047zQh1n2jr8n3Xcz9BazdrOKeVhthxr4qqVX8lMtpGLGvF0bXSlNCkKhmXEkeup+I4SOlNv4= X-Received: by 2002:a17:90a:e2c7:: with SMTP id fr7mr13498231pjb.103.1594281757143; Thu, 09 Jul 2020 01:02:37 -0700 (PDT) MIME-Version: 1.0 References: <1591871065-12461-1-git-send-email-phil.yang@arm.com> <1594277162-15021-1-git-send-email-phil.yang@arm.com> In-Reply-To: <1594277162-15021-1-git-send-email-phil.yang@arm.com> From: Stefan Puiu Date: Thu, 9 Jul 2020 11:02:25 +0300 Message-ID: To: Phil Yang 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 , hkalra@marvell.com Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v2] eal: use c11 atomic built-ins for interrupt status X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, Noticed 2 typos: On Thu, Jul 9, 2020 at 9:46 AM Phil Yang 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 > Reviewed-by: Ruifeng Wang > Reviewed-by: Honnappa Nagarahalli > Reviewed-by: Harman Kalra > --- > 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 > #include > #include > -#include > #include > #include > #include > @@ -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 >