[PATCH] service: avoid worker lcore exit deadlock

[PATCH] service: avoid worker lcore exit deadlock

[Mattias Rönnblom]

Calling rte_exit() from a worker lcore thread causes a deadlock in
rte_service_finalize().

This patch makes rte_service_finalize() deadlock-free by avoiding the
need to synchronize with service lcore threads, which in turn is
achieved by moving service and per-lcore state from the heap to being
statically allocated.

The BSS segment increases with ~156 kB (on x86_64 with default
RTE_MAX_LCORE and RTE_SERVICE_NUM_MAX).

According to the service perf autotest, this change also results in a
slight reduction of service framework overhead.

Fixes: 33666b448f15 ("service: fix crash on exit")
Cc: harry.van.haaren@intel.com
Cc: stable@dpdk.org

Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
 lib/eal/common/rte_service.c | 28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c
index 94e872a08a..2ea0c0c0d9 100644
--- a/lib/eal/common/rte_service.c
+++ b/lib/eal/common/rte_service.c
@@ -15,7 +15,6 @@
 #include <rte_common.h>
 #include <rte_cycles.h>
 #include <rte_atomic.h>
-#include <rte_malloc.h>
 #include <rte_spinlock.h>
 #include <rte_trace_point.h>
 
@@ -74,8 +73,8 @@ struct core_state {
 } __rte_cache_aligned;
 
 static uint32_t rte_service_count;
-static struct rte_service_spec_impl *rte_services;
-static struct core_state *lcore_states;
+static struct rte_service_spec_impl rte_services[RTE_SERVICE_NUM_MAX];
+static struct core_state lcore_states[RTE_MAX_LCORE];
 static uint32_t rte_service_library_initialized;
 
 int32_t
@@ -93,21 +92,6 @@ rte_service_init(void)
 		return -EALREADY;
 	}
 
-	rte_services = rte_calloc("rte_services", RTE_SERVICE_NUM_MAX,
-			sizeof(struct rte_service_spec_impl),
-			RTE_CACHE_LINE_SIZE);
-	if (!rte_services) {
-		RTE_LOG(ERR, EAL, "error allocating rte services array\n");
-		goto fail_mem;
-	}
-
-	lcore_states = rte_calloc("rte_service_core_states", RTE_MAX_LCORE,
-			sizeof(struct core_state), RTE_CACHE_LINE_SIZE);
-	if (!lcore_states) {
-		RTE_LOG(ERR, EAL, "error allocating core states array\n");
-		goto fail_mem;
-	}
-
 	int i;
 	struct rte_config *cfg = rte_eal_get_configuration();
 	for (i = 0; i < RTE_MAX_LCORE; i++) {
@@ -120,10 +104,6 @@ rte_service_init(void)
 
 	rte_service_library_initialized = 1;
 	return 0;
-fail_mem:
-	rte_free(rte_services);
-	rte_free(lcore_states);
-	return -ENOMEM;
 }
 
 void
@@ -133,10 +113,6 @@ rte_service_finalize(void)
 		return;
 
 	rte_service_lcore_reset_all();
-	rte_eal_mp_wait_lcore();
-
-	rte_free(rte_services);
-	rte_free(lcore_states);
 
 	rte_service_library_initialized = 0;
 }
-- 
2.34.1

RE: [PATCH] service: avoid worker lcore exit deadlock

[Van Haaren, Harry]

> -----Original Message-----
> From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> Sent: Tuesday, July 4, 2023 10:44 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>
> Cc: hofors@lysator.liu.se; dev@dpdk.org; Suanming Mou
> <suanmingm@nvidia.com>; thomas@monjalon.net;
> david.marchand@redhat.com; mattias.ronnblom
> <mattias.ronnblom@ericsson.com>; stable@dpdk.org
> Subject: [PATCH] service: avoid worker lcore exit deadlock
> 
> Calling rte_exit() from a worker lcore thread causes a deadlock in
> rte_service_finalize().
> 
> This patch makes rte_service_finalize() deadlock-free by avoiding the
> need to synchronize with service lcore threads, which in turn is
> achieved by moving service and per-lcore state from the heap to being
> statically allocated.

Elegant solution to avoiding the malloc/free in cleanup issue.
Thanks for investigating & implementing the solution!

> The BSS segment increases with ~156 kB (on x86_64 with default
> RTE_MAX_LCORE and RTE_SERVICE_NUM_MAX).
> 
> According to the service perf autotest, this change also results in a
> slight reduction of service framework overhead.
> 
> Fixes: 33666b448f15 ("service: fix crash on exit")
> Cc: harry.van.haaren@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>

Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Re: [PATCH] service: avoid worker lcore exit deadlock

[David Marchand]

Hello,

On Tue, Jul 4, 2023 at 11:49 PM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> Calling rte_exit() from a worker lcore thread causes a deadlock in
> rte_service_finalize().
>
> This patch makes rte_service_finalize() deadlock-free by avoiding the
> need to synchronize with service lcore threads, which in turn is
> achieved by moving service and per-lcore state from the heap to being
> statically allocated.
>
> The BSS segment increases with ~156 kB (on x86_64 with default
> RTE_MAX_LCORE and RTE_SERVICE_NUM_MAX).
>
> According to the service perf autotest, this change also results in a
> slight reduction of service framework overhead.
>
> Fixes: 33666b448f15 ("service: fix crash on exit")
> Cc: stable@dpdk.org
>
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Sorry, this fell through the cracks.
CI was complaining about this patch and it was probably a false
positive, but we don't have the (full) logs anymore.

Could you resend it please?


-- 
David Marchand

Re: [PATCH] service: avoid worker lcore exit deadlock

[Tyler Retzlaff]

On Tue, Oct 01, 2024 at 06:26:03PM +0200, Mattias Rönnblom wrote:
> Calling rte_exit() from a worker lcore thread causes a deadlock in
> rte_service_finalize().
> 
> This patch makes rte_service_finalize() deadlock-free by avoiding the
> need to synchronize with service lcore threads, which in turn is
> achieved by moving service and per-lcore state from the heap to being
> statically allocated.
> 
> The BSS segment increases with ~156 kB (on x86_64 with default
> RTE_MAX_LCORE and RTE_SERVICE_NUM_MAX).
> 
> According to the service perf autotest, this change also results in a
> slight reduction of service framework overhead.
> 
> Fixes: 33666b448f15 ("service: fix crash on exit")
> Cc: harry.van.haaren@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> ---
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

[PATCH] service: avoid worker lcore exit deadlock

[Mattias Rönnblom]

Calling rte_exit() from a worker lcore thread causes a deadlock in
rte_service_finalize().

This patch makes rte_service_finalize() deadlock-free by avoiding the
need to synchronize with service lcore threads, which in turn is
achieved by moving service and per-lcore state from the heap to being
statically allocated.

The BSS segment increases with ~156 kB (on x86_64 with default
RTE_MAX_LCORE and RTE_SERVICE_NUM_MAX).

According to the service perf autotest, this change also results in a
slight reduction of service framework overhead.

Fixes: 33666b448f15 ("service: fix crash on exit")
Cc: harry.van.haaren@intel.com
Cc: stable@dpdk.org

Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
 lib/eal/common/rte_service.c | 28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c
index 94e872a08a..2ea0c0c0d9 100644
--- a/lib/eal/common/rte_service.c
+++ b/lib/eal/common/rte_service.c
@@ -15,7 +15,6 @@
 #include <rte_common.h>
 #include <rte_cycles.h>
 #include <rte_atomic.h>
-#include <rte_malloc.h>
 #include <rte_spinlock.h>
 #include <rte_trace_point.h>
 
@@ -74,8 +73,8 @@ struct core_state {
 } __rte_cache_aligned;
 
 static uint32_t rte_service_count;
-static struct rte_service_spec_impl *rte_services;
-static struct core_state *lcore_states;
+static struct rte_service_spec_impl rte_services[RTE_SERVICE_NUM_MAX];
+static struct core_state lcore_states[RTE_MAX_LCORE];
 static uint32_t rte_service_library_initialized;
 
 int32_t
@@ -93,21 +92,6 @@ rte_service_init(void)
 		return -EALREADY;
 	}
 
-	rte_services = rte_calloc("rte_services", RTE_SERVICE_NUM_MAX,
-			sizeof(struct rte_service_spec_impl),
-			RTE_CACHE_LINE_SIZE);
-	if (!rte_services) {
-		RTE_LOG(ERR, EAL, "error allocating rte services array\n");
-		goto fail_mem;
-	}
-
-	lcore_states = rte_calloc("rte_service_core_states", RTE_MAX_LCORE,
-			sizeof(struct core_state), RTE_CACHE_LINE_SIZE);
-	if (!lcore_states) {
-		RTE_LOG(ERR, EAL, "error allocating core states array\n");
-		goto fail_mem;
-	}
-
 	int i;
 	struct rte_config *cfg = rte_eal_get_configuration();
 	for (i = 0; i < RTE_MAX_LCORE; i++) {
@@ -120,10 +104,6 @@ rte_service_init(void)
 
 	rte_service_library_initialized = 1;
 	return 0;
-fail_mem:
-	rte_free(rte_services);
-	rte_free(lcore_states);
-	return -ENOMEM;
 }
 
 void
@@ -133,10 +113,6 @@ rte_service_finalize(void)
 		return;
 
 	rte_service_lcore_reset_all();
-	rte_eal_mp_wait_lcore();
-
-	rte_free(rte_services);
-	rte_free(lcore_states);
 
 	rte_service_library_initialized = 0;
 }
-- 
2.34.1