diff mbox series

service: fix early move to inactive status

Message ID 20221020190041.2350842-1-erik.g.carrillo@intel.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers
Series service: fix early move to inactive status |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS

Commit Message

Carrillo, Erik G Oct. 20, 2022, 7 p.m. UTC 
  Assume thread T2 is a service lcore that is in the middle of executing
a service function.  Also, assume thread T1 concurrently calls
rte_service_lcore_stop(), which will set the "service_active_on_lcore"
state to false.  If thread T1 then calls rte_service_may_be_active(),
it can return zero even though T2 is still running the service function.
If T1 then proceeds to free data being used by T2, a crash can ensue.

Move the logic that clears the "service_active_on_lcore" state from the
rte_service_lcore_stop() function to the service_runner_func() to
ensure that we:
- don't let the "service_active_on_lcore" state linger as 1
- don't clear the state early

Fixes: 6550113be62d ("service: fix lingering active status")
Cc: stable@dpdk.org

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
 lib/eal/common/rte_service.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Van Haaren, Harry Oct. 21, 2022, 11:36 a.m. UTC | #1 
> -----Original Message-----
> From: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Sent: Thursday, October 20, 2022 8:01 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: Naga Harish K, S V <s.v.naga.harish.k@intel.com>; dev@dpdk.org;
> stable@dpdk.org
> Subject: [PATCH] service: fix early move to inactive status
> 
> Assume thread T2 is a service lcore that is in the middle of executing
> a service function.  Also, assume thread T1 concurrently calls
> rte_service_lcore_stop(), which will set the "service_active_on_lcore"
> state to false.  If thread T1 then calls rte_service_may_be_active(),
> it can return zero even though T2 is still running the service function.
> If T1 then proceeds to free data being used by T2, a crash can ensue.
> 
> Move the logic that clears the "service_active_on_lcore" state from the
> rte_service_lcore_stop() function to the service_runner_func() to
> ensure that we:
> - don't let the "service_active_on_lcore" state linger as 1
> - don't clear the state early
> 
> Fixes: 6550113be62d ("service: fix lingering active status")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>

+CC Thomas/David, please consider for inclusion in next RC.

Thanks Erik, indeed the "active = off" store is too early in the current implementation, so:
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

<snip patch>
David Marchand Oct. 21, 2022, 1:10 p.m. UTC | #2 
On Fri, Oct 21, 2022 at 1:36 PM Van Haaren, Harry
<harry.van.haaren@intel.com> wrote:
> >
> > Assume thread T2 is a service lcore that is in the middle of executing
> > a service function.  Also, assume thread T1 concurrently calls
> > rte_service_lcore_stop(), which will set the "service_active_on_lcore"
> > state to false.  If thread T1 then calls rte_service_may_be_active(),
> > it can return zero even though T2 is still running the service function.
> > If T1 then proceeds to free data being used by T2, a crash can ensue.
> >
> > Move the logic that clears the "service_active_on_lcore" state from the
> > rte_service_lcore_stop() function to the service_runner_func() to
> > ensure that we:
> > - don't let the "service_active_on_lcore" state linger as 1
> > - don't clear the state early
> >
> > Fixes: 6550113be62d ("service: fix lingering active status")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Applied, thanks.
diff mbox series

Patch

diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c
index 81c9514149..bcc2e19077 100644
--- a/lib/eal/common/rte_service.c
+++ b/lib/eal/common/rte_service.c
@@ -479,6 +479,7 @@  static int32_t
 service_runner_func(void *arg)
 {
 	RTE_SET_USED(arg);
+	uint8_t i;
 	const int lcore = rte_lcore_id();
 	struct core_state *cs = &lcore_states[lcore];
 
@@ -494,7 +495,6 @@  service_runner_func(void *arg)
 		const uint64_t service_mask = cs->service_mask;
 		uint8_t start_id;
 		uint8_t end_id;
-		uint8_t i;
 
 		if (service_mask == 0)
 			continue;
@@ -510,6 +510,12 @@  service_runner_func(void *arg)
 		__atomic_store_n(&cs->loops, cs->loops + 1, __ATOMIC_RELAXED);
 	}
 
+	/* Switch off this core for all services, to ensure that future
+	 * calls to may_be_active() know this core is switched off.
+	 */
+	for (i = 0; i < RTE_SERVICE_NUM_MAX; i++)
+		cs->service_active_on_lcore[i] = 0;
+
 	/* Use SEQ CST memory ordering to avoid any re-ordering around
 	 * this store, ensuring that once this store is visible, the service
 	 * lcore thread really is done in service cores code.
@@ -806,11 +812,6 @@  rte_service_lcore_stop(uint32_t lcore)
 			__atomic_load_n(&rte_services[i].num_mapped_cores,
 				__ATOMIC_RELAXED));
 
-		/* Switch off this core for all services, to ensure that future
-		 * calls to may_be_active() know this core is switched off.
-		 */
-		cs->service_active_on_lcore[i] = 0;
-
 		/* if the core is mapped, and the service is running, and this
 		 * is the only core that is mapped, the service would cease to
 		 * run if this core stopped, so fail instead.