From patchwork Thu Jul 9 08:34:35 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Phil Yang X-Patchwork-Id: 73618 X-Patchwork-Delegate: david.marchand@redhat.com Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2DAE8A0526; Thu, 9 Jul 2020 10:35:25 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7EB461DE39; Thu, 9 Jul 2020 10:35:24 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by dpdk.org (Postfix) with ESMTP id 60C4F1DDCF for ; Thu, 9 Jul 2020 10:35:22 +0200 (CEST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id ACFAE31B; Thu, 9 Jul 2020 01:35:21 -0700 (PDT) Received: from phil-VirtualBox.arm.com (A010647.Arm.com [10.170.226.139]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 2C0F93F887; Thu, 9 Jul 2020 01:35:17 -0700 (PDT) From: Phil Yang To: david.marchand@redhat.com, dev@dpdk.org Cc: stefan.puiu@gmail.com, 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, nhorman@tuxdriver.com, hkalra@marvell.com Date: Thu, 9 Jul 2020 16:34:35 +0800 Message-Id: <1594283675-15965-1-git-send-email-phil.yang@arm.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1594277162-15021-1-git-send-email-phil.yang@arm.com> References: <1594277162-15021-1-git-send-email-phil.yang@arm.com> Subject: [dpdk-dev] [PATCH v3] 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" 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 --- 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 #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 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; } } }