From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 478B3A0C45; Mon, 25 Oct 2021 06:53:17 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id ACB2541130; Mon, 25 Oct 2021 06:53:05 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 9244841120 for ; Mon, 25 Oct 2021 06:53:02 +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 0D4871FB; Sun, 24 Oct 2021 21:53:02 -0700 (PDT) Received: from qc2400f-1.austin.arm.com (qc2400f-1.austin.arm.com [10.118.12.44]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id ECB043F73D; Sun, 24 Oct 2021 21:53:01 -0700 (PDT) From: Honnappa Nagarahalli To: dev@dpdk.org, honnappa.nagarahalli@arm.com, konstantin.ananyev@intel.com, david.marchand@redhat.com, feifei.wang2@arm.com Cc: ruifeng.wang@arm.com, nd@arm.com Date: Sun, 24 Oct 2021 23:52:36 -0500 Message-Id: <20211025045237.19878-4-honnappa.nagarahalli@arm.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20211025045237.19878-1-honnappa.nagarahalli@arm.com> References: <20210224212018.17576-1-honnappa.nagarahalli@arm.com> <20211025045237.19878-1-honnappa.nagarahalli@arm.com> Subject: [dpdk-dev] [PATCH v3 3/4] eal: use correct memory ordering X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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" Ensure that the memory operations before the call to rte_eal_remote_launch are visible to the worker thread. Use the function pointer to execute in worker thread as the guard variable. Ensure that the memory operations in worker thread, that happen before it returns the status of the assigned function, are visible to the main thread. Use the variable containing the lcore's state as the guard variable. Signed-off-by: Honnappa Nagarahalli Reviewed-by: Ola Liljedahl Reviewed-by: Feifei Wang --- lib/eal/common/eal_common_launch.c | 8 ++---- lib/eal/freebsd/eal_thread.c | 41 ++++++++++++++++++++++++------ lib/eal/linux/eal_thread.c | 40 +++++++++++++++++++++++------ lib/eal/windows/eal_thread.c | 40 +++++++++++++++++++++++------ 4 files changed, 99 insertions(+), 30 deletions(-) diff --git a/lib/eal/common/eal_common_launch.c b/lib/eal/common/eal_common_launch.c index 78fd940267..e95dadffb3 100644 --- a/lib/eal/common/eal_common_launch.c +++ b/lib/eal/common/eal_common_launch.c @@ -23,14 +23,10 @@ int rte_eal_wait_lcore(unsigned worker_id) { - if (lcore_config[worker_id].state == WAIT) - return 0; - - while (lcore_config[worker_id].state != WAIT) + while (__atomic_load_n(&lcore_config[worker_id].state, + __ATOMIC_ACQUIRE) != WAIT) rte_pause(); - rte_rmb(); - return lcore_config[worker_id].ret; } diff --git a/lib/eal/freebsd/eal_thread.c b/lib/eal/freebsd/eal_thread.c index ce314c1dd4..3b18030d73 100644 --- a/lib/eal/freebsd/eal_thread.c +++ b/lib/eal/freebsd/eal_thread.c @@ -39,11 +39,19 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned worker_id) int w2m = lcore_config[worker_id].pipe_worker2main[0]; int rc = -EBUSY; - if (lcore_config[worker_id].state != WAIT) + /* Check if the worker is in 'WAIT' state. Use acquire order + * since 'state' variable is used as the guard variable. + */ + if (__atomic_load_n(&lcore_config[worker_id].state, + __ATOMIC_ACQUIRE) != WAIT) goto finish; - lcore_config[worker_id].f = f; lcore_config[worker_id].arg = arg; + /* Ensure that all the memory operations are completed + * before the worker thread starts running the function. + * Use worker thread function as the guard variable. + */ + __atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE); /* send message */ n = 0; @@ -100,6 +108,7 @@ eal_thread_loop(__rte_unused void *arg) /* read on our pipe to get commands */ while (1) { + lcore_function_t *f; void *fct_arg; /* wait command */ @@ -110,7 +119,11 @@ eal_thread_loop(__rte_unused void *arg) if (n <= 0) rte_panic("cannot read on configuration pipe\n"); - lcore_config[lcore_id].state = RUNNING; + /* Set the state to 'RUNNING'. Use release order + * since 'state' variable is used as the guard variable. + */ + __atomic_store_n(&lcore_config[lcore_id].state, RUNNING, + __ATOMIC_RELEASE); /* send ack */ n = 0; @@ -119,17 +132,29 @@ eal_thread_loop(__rte_unused void *arg) if (n < 0) rte_panic("cannot write on configuration pipe\n"); - if (lcore_config[lcore_id].f == NULL) - rte_panic("NULL function pointer\n"); + /* Load 'f' with acquire order to ensure that + * the memory operations from the main thread + * are accessed only after update to 'f' is visible. + * Wait till the update to 'f' is visible to the worker. + */ + while ((f = __atomic_load_n(&lcore_config[lcore_id].f, + __ATOMIC_ACQUIRE)) == NULL) + rte_pause(); /* call the function and store the return value */ fct_arg = lcore_config[lcore_id].arg; - ret = lcore_config[lcore_id].f(fct_arg); + ret = f(fct_arg); lcore_config[lcore_id].ret = ret; lcore_config[lcore_id].f = NULL; lcore_config[lcore_id].arg = NULL; - rte_wmb(); - lcore_config[lcore_id].state = WAIT; + + /* Store the state with release order to ensure that + * the memory operations from the worker thread + * are completed before the state is updated. + * Use 'state' as the guard variable. + */ + __atomic_store_n(&lcore_config[lcore_id].state, WAIT, + __ATOMIC_RELEASE); } /* never reached */ diff --git a/lib/eal/linux/eal_thread.c b/lib/eal/linux/eal_thread.c index e2c9e5a3f7..c7f0f9b2f7 100644 --- a/lib/eal/linux/eal_thread.c +++ b/lib/eal/linux/eal_thread.c @@ -39,11 +39,19 @@ rte_eal_remote_launch(int (*f)(void *), void *arg, unsigned int worker_id) int w2m = lcore_config[worker_id].pipe_worker2main[0]; int rc = -EBUSY; - if (lcore_config[worker_id].state != WAIT) + /* Check if the worker is in 'WAIT' state. Use acquire order + * since 'state' variable is used as the guard variable. + */ + if (__atomic_load_n(&lcore_config[worker_id].state, + __ATOMIC_ACQUIRE) != WAIT) goto finish; - lcore_config[worker_id].f = f; lcore_config[worker_id].arg = arg; + /* Ensure that all the memory operations are completed + * before the worker thread starts running the function. + * Use worker thread function pointer as the guard variable. + */ + __atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE); /* send message */ n = 0; @@ -100,6 +108,7 @@ eal_thread_loop(__rte_unused void *arg) /* read on our pipe to get commands */ while (1) { + lcore_function_t *f; void *fct_arg; /* wait command */ @@ -110,7 +119,11 @@ eal_thread_loop(__rte_unused void *arg) if (n <= 0) rte_panic("cannot read on configuration pipe\n"); - lcore_config[lcore_id].state = RUNNING; + /* Set the state to 'RUNNING'. Use release order + * since 'state' variable is used as the guard variable. + */ + __atomic_store_n(&lcore_config[lcore_id].state, RUNNING, + __ATOMIC_RELEASE); /* send ack */ n = 0; @@ -119,18 +132,29 @@ eal_thread_loop(__rte_unused void *arg) if (n < 0) rte_panic("cannot write on configuration pipe\n"); - if (lcore_config[lcore_id].f == NULL) - rte_panic("NULL function pointer\n"); + /* Load 'f' with acquire order to ensure that + * the memory operations from the main thread + * are accessed only after update to 'f' is visible. + * Wait till the update to 'f' is visible to the worker. + */ + while ((f = __atomic_load_n(&lcore_config[lcore_id].f, + __ATOMIC_ACQUIRE)) == NULL) + rte_pause(); /* call the function and store the return value */ fct_arg = lcore_config[lcore_id].arg; - ret = lcore_config[lcore_id].f(fct_arg); + ret = f(fct_arg); lcore_config[lcore_id].ret = ret; lcore_config[lcore_id].f = NULL; lcore_config[lcore_id].arg = NULL; - rte_wmb(); - lcore_config[lcore_id].state = WAIT; + /* Store the state with release order to ensure that + * the memory operations from the worker thread + * are completed before the state is updated. + * Use 'state' as the guard variable. + */ + __atomic_store_n(&lcore_config[lcore_id].state, WAIT, + __ATOMIC_RELEASE); } /* never reached */ diff --git a/lib/eal/windows/eal_thread.c b/lib/eal/windows/eal_thread.c index 0028123c95..54fa93fa62 100644 --- a/lib/eal/windows/eal_thread.c +++ b/lib/eal/windows/eal_thread.c @@ -29,11 +29,19 @@ rte_eal_remote_launch(lcore_function_t *f, void *arg, unsigned int worker_id) int m2w = lcore_config[worker_id].pipe_main2worker[1]; int w2m = lcore_config[worker_id].pipe_worker2main[0]; - if (lcore_config[worker_id].state != WAIT) + /* Check if the worker is in 'WAIT' state. Use acquire order + * since 'state' variable is used as the guard variable. + */ + if (__atomic_load_n(&lcore_config[worker_id].state, + __ATOMIC_ACQUIRE) != WAIT) return -EBUSY; - lcore_config[worker_id].f = f; lcore_config[worker_id].arg = arg; + /* Ensure that all the memory operations are completed + * before the worker thread starts running the function. + * Use worker thread function as the guard variable. + */ + __atomic_store_n(&lcore_config[worker_id].f, f, __ATOMIC_RELEASE); /* send message */ n = 0; @@ -84,6 +92,7 @@ eal_thread_loop(void *arg __rte_unused) /* read on our pipe to get commands */ while (1) { + lcore_function_t *f; void *fct_arg; /* wait command */ @@ -94,7 +103,11 @@ eal_thread_loop(void *arg __rte_unused) if (n <= 0) rte_panic("cannot read on configuration pipe\n"); - lcore_config[lcore_id].state = RUNNING; + /* Set the state to 'RUNNING'. Use release order + * since 'state' variable is used as the guard variable. + */ + __atomic_store_n(&lcore_config[lcore_id].state, RUNNING, + __ATOMIC_RELEASE); /* send ack */ n = 0; @@ -103,18 +116,29 @@ eal_thread_loop(void *arg __rte_unused) if (n < 0) rte_panic("cannot write on configuration pipe\n"); - if (lcore_config[lcore_id].f == NULL) - rte_panic("NULL function pointer\n"); + /* Load 'f' with acquire order to ensure that + * the memory operations from the main thread + * are accessed only after update to 'f' is visible. + * Wait till the update to 'f' is visible to the worker. + */ + while ((f = __atomic_load_n(&lcore_config[lcore_id].f, + __ATOMIC_ACQUIRE)) == NULL) + rte_pause(); /* call the function and store the return value */ fct_arg = lcore_config[lcore_id].arg; - ret = lcore_config[lcore_id].f(fct_arg); + ret = f(fct_arg); lcore_config[lcore_id].ret = ret; lcore_config[lcore_id].f = NULL; lcore_config[lcore_id].arg = NULL; - rte_wmb(); - lcore_config[lcore_id].state = WAIT; + /* Store the state with release order to ensure that + * the memory operations from the worker thread + * are completed before the state is updated. + * Use 'state' as the guard variable. + */ + __atomic_store_n(&lcore_config[lcore_id].state, WAIT, + __ATOMIC_RELEASE); } } -- 2.17.1