[PATCH] eal: add worker threads cleanup in rte_eal_cleanup()

[PATCH] eal: add worker threads cleanup in rte_eal_cleanup()

[Gagandeep Singh]

This patch introduces a worker thread cleanup function in the EAL library,
ensuring proper termination of created pthreads and invocation of
registered pthread destructors.
This guarantees the correct cleanup of thread-specific resources,
used by drivers or applications.

Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
---
 lib/eal/linux/eal.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib/eal/linux/eal.c b/lib/eal/linux/eal.c
index a6220524a4..a828905783 100644
--- a/lib/eal/linux/eal.c
+++ b/lib/eal/linux/eal.c
@@ -913,6 +913,32 @@ eal_worker_thread_create(unsigned int lcore_id)
 	return ret;
 }
 
+static void
+eal_worker_thread_cleanup(void)
+{
+	unsigned int lcore_id;
+	int ret = 0;
+
+	/* Cancel all the worker pthreads */
+	RTE_LCORE_FOREACH_WORKER(lcore_id) {
+		/* Check for non zero id */
+		if (!lcore_config[lcore_id].thread_id.opaque_id)
+			continue;
+
+		ret = pthread_cancel((pthread_t)lcore_config[lcore_id].thread_id.opaque_id);
+		if (ret != 0) {
+			EAL_LOG(WARNING, "Pthread cancel fails for lcore %d",
+					lcore_id);
+		}
+		ret = pthread_join((pthread_t)lcore_config[lcore_id].thread_id.opaque_id, NULL);
+		if (ret != 0) {
+			EAL_LOG(WARNING, "Pthread join fails for lcore %d",
+					lcore_id);
+		}
+	}
+	EAL_LOG(DEBUG, "Worker thread clean up done");
+}
+
 /* Launch threads, called at application init(). */
 int
 rte_eal_init(int argc, char **argv)
@@ -1321,6 +1347,7 @@ rte_eal_cleanup(void)
 #endif
 	rte_mp_channel_cleanup();
 	eal_bus_cleanup();
+	eal_worker_thread_cleanup();
 	rte_trace_save();
 	eal_trace_fini();
 	eal_mp_dev_hotplug_cleanup();
-- 
2.25.1

Re: [PATCH] eal: add worker threads cleanup in rte_eal_cleanup()

[Stephen Hemminger]

On Fri, 10 Jan 2025 12:17:17 +0530
Gagandeep Singh <g.singh@nxp.com> wrote:

> This patch introduces a worker thread cleanup function in the EAL library,
> ensuring proper termination of created pthreads and invocation of
> registered pthread destructors.
> This guarantees the correct cleanup of thread-specific resources,
> used by drivers or applications.
> 
> Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> ---

What problem is this trying to solve?

Canceling threads sends signals and can be problematic.
Many of the operations done in drivers are not signal safe.

RE: [PATCH] eal: add worker threads cleanup in rte_eal_cleanup()

[Gagandeep Singh]

Hi,

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, January 10, 2025 10:49 PM
> To: Gagandeep Singh <G.Singh@nxp.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] eal: add worker threads cleanup in rte_eal_cleanup()
> 
> On Fri, 10 Jan 2025 12:17:17 +0530
> Gagandeep Singh <g.singh@nxp.com> wrote:
> 
> > This patch introduces a worker thread cleanup function in the EAL
> > library, ensuring proper termination of created pthreads and
> > invocation of registered pthread destructors.
> > This guarantees the correct cleanup of thread-specific resources, used
> > by drivers or applications.
> >
> > Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> > ---
> 
> What problem is this trying to solve?
> 
> Canceling threads sends signals and can be problematic.
> Many of the operations done in drivers are not signal safe.

To ensure the proper cleanup of thread-specific resources, the DPAA driver initializes pthread-specific destructors using pthread_key_create(). These destructors are executed only when a thread terminates or the key is deleted. However, since threads are not terminated when the application is killed, these destructors are not executed, resulting in resource leaks.
To address this issue, we propose adding thread termination code to rte_eal_cleanup() to ensure that threads are properly terminated, thereby triggering the execution of pthread-specific destructors

Any alternate suggestion in case pthread_cancel is not a better solution? We can add pthread join timeout to avoid blocking on thread stuck or
May be any way to call pthread_exit?

Re: [PATCH] eal: add worker threads cleanup in rte_eal_cleanup()

[Stephen Hemminger]

On Mon, 13 Jan 2025 05:13:01 +0000
Gagandeep Singh <G.Singh@nxp.com> wrote:

> Hi,
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Friday, January 10, 2025 10:49 PM
> > To: Gagandeep Singh <G.Singh@nxp.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [PATCH] eal: add worker threads cleanup in rte_eal_cleanup()
> > 
> > On Fri, 10 Jan 2025 12:17:17 +0530
> > Gagandeep Singh <g.singh@nxp.com> wrote:
> >   
> > > This patch introduces a worker thread cleanup function in the EAL
> > > library, ensuring proper termination of created pthreads and
> > > invocation of registered pthread destructors.
> > > This guarantees the correct cleanup of thread-specific resources, used
> > > by drivers or applications.
> > >
> > > Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> > > ---  
> > 
> > What problem is this trying to solve?
> > 
> > Canceling threads sends signals and can be problematic.
> > Many of the operations done in drivers are not signal safe.  
> 
> To ensure the proper cleanup of thread-specific resources, the DPAA driver initializes pthread-specific destructors using pthread_key_create(). These destructors are executed only when a thread terminates or the key is deleted. However, since threads are not terminated when the application is killed, these destructors are not executed, resulting in resource leaks.
> To address this issue, we propose adding thread termination code to rte_eal_cleanup() to ensure that threads are properly terminated, thereby triggering the execution of pthread-specific destructors
> 
> Any alternate suggestion in case pthread_cancel is not a better solution? We can add pthread join timeout to avoid blocking on thread stuck or
> May be any way to call pthread_exit?

The DPAA driver is the problem here. It should not be using pthread_key.
Other drivers don't do this. Other drivers do setup on probe and cleanup on close.

An application doing a clean shutdown should do what existing testpmd, l3fwd, do
	- wait for worker threads to go idle
	- stop all ports
	- close all ports
	- call eal cleanup

If DPAA driver needs pthread_key it should handling that in the close.
But it really should be using DPDK thread local storage for this.

RE: [PATCH] eal: add worker threads cleanup in rte_eal_cleanup()

[Gagandeep Singh]

Hi,

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday, January 13, 2025 10:10 PM
> To: Gagandeep Singh <G.Singh@nxp.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] eal: add worker threads cleanup in rte_eal_cleanup()
> 
> On Mon, 13 Jan 2025 05:13:01 +0000
> Gagandeep Singh <G.Singh@nxp.com> wrote:
> 
> > Hi,
> >
> > > -----Original Message-----
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Friday, January 10, 2025 10:49 PM
> > > To: Gagandeep Singh <G.Singh@nxp.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [PATCH] eal: add worker threads cleanup in
> > > rte_eal_cleanup()
> > >
> > > On Fri, 10 Jan 2025 12:17:17 +0530
> > > Gagandeep Singh <g.singh@nxp.com> wrote:
> > >
> > > > This patch introduces a worker thread cleanup function in the EAL
> > > > library, ensuring proper termination of created pthreads and
> > > > invocation of registered pthread destructors.
> > > > This guarantees the correct cleanup of thread-specific resources,
> > > > used by drivers or applications.
> > > >
> > > > Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> > > > ---
> > >
> > > What problem is this trying to solve?
> > >
> > > Canceling threads sends signals and can be problematic.
> > > Many of the operations done in drivers are not signal safe.
> >
> > To ensure the proper cleanup of thread-specific resources, the DPAA driver
> initializes pthread-specific destructors using pthread_key_create(). These
> destructors are executed only when a thread terminates or the key is deleted.
> However, since threads are not terminated when the application is killed, these
> destructors are not executed, resulting in resource leaks.
> > To address this issue, we propose adding thread termination code to
> > rte_eal_cleanup() to ensure that threads are properly terminated,
> > thereby triggering the execution of pthread-specific destructors
> >
> > Any alternate suggestion in case pthread_cancel is not a better
> > solution? We can add pthread join timeout to avoid blocking on thread stuck or
> May be any way to call pthread_exit?
> 
> The DPAA driver is the problem here. It should not be using pthread_key.
> Other drivers don't do this. Other drivers do setup on probe and cleanup on close.
> 
> An application doing a clean shutdown should do what existing testpmd, l3fwd, do
> 	- wait for worker threads to go idle
> 	- stop all ports
> 	- close all ports
> 	- call eal cleanup
> 
> If DPAA driver needs pthread_key it should handling that in the close.
> But it really should be using DPDK thread local storage for this.
> 
This is fine for graceful application termination, but what about non-graceful application termination?
I have a DPAA bus cleanup patch ready, and will submit it soon, But we still need
destructor in case application exit without calling the rte_eal_cleanup().

I can see DPDK is giving an API rte_thread_key_create() which is defined only in UNIX and windows.
Why not for Linux?

I know DPAA driver is using pthread_key which it should not use but instead use
rte_thread_key_create() provided by DPDK which we can replace.
Having pthread destructor is still a valid case in my opinion.

Re: [PATCH] eal: add worker threads cleanup in rte_eal_cleanup()

[Stephen Hemminger]

On Tue, 14 Jan 2025 04:53:51 +0000
Gagandeep Singh <G.Singh@nxp.com> wrote:

> Hi,
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Monday, January 13, 2025 10:10 PM
> > To: Gagandeep Singh <G.Singh@nxp.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [PATCH] eal: add worker threads cleanup in rte_eal_cleanup()
> > 
> > On Mon, 13 Jan 2025 05:13:01 +0000
> > Gagandeep Singh <G.Singh@nxp.com> wrote:
> >   
> > > Hi,
> > >  
> > > > -----Original Message-----
> > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > Sent: Friday, January 10, 2025 10:49 PM
> > > > To: Gagandeep Singh <G.Singh@nxp.com>
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [PATCH] eal: add worker threads cleanup in
> > > > rte_eal_cleanup()
> > > >
> > > > On Fri, 10 Jan 2025 12:17:17 +0530
> > > > Gagandeep Singh <g.singh@nxp.com> wrote:
> > > >  
> > > > > This patch introduces a worker thread cleanup function in the EAL
> > > > > library, ensuring proper termination of created pthreads and
> > > > > invocation of registered pthread destructors.
> > > > > This guarantees the correct cleanup of thread-specific resources,
> > > > > used by drivers or applications.
> > > > >
> > > > > Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> > > > > ---  
> > > >
> > > > What problem is this trying to solve?
> > > >
> > > > Canceling threads sends signals and can be problematic.
> > > > Many of the operations done in drivers are not signal safe.  
> > >
> > > To ensure the proper cleanup of thread-specific resources, the DPAA driver  
> > initializes pthread-specific destructors using pthread_key_create(). These
> > destructors are executed only when a thread terminates or the key is deleted.
> > However, since threads are not terminated when the application is killed, these
> > destructors are not executed, resulting in resource leaks.  
> > > To address this issue, we propose adding thread termination code to
> > > rte_eal_cleanup() to ensure that threads are properly terminated,
> > > thereby triggering the execution of pthread-specific destructors
> > >
> > > Any alternate suggestion in case pthread_cancel is not a better
> > > solution? We can add pthread join timeout to avoid blocking on thread stuck or  
> > May be any way to call pthread_exit?
> > 
> > The DPAA driver is the problem here. It should not be using pthread_key.
> > Other drivers don't do this. Other drivers do setup on probe and cleanup on close.
> > 
> > An application doing a clean shutdown should do what existing testpmd, l3fwd, do
> > 	- wait for worker threads to go idle
> > 	- stop all ports
> > 	- close all ports
> > 	- call eal cleanup
> > 
> > If DPAA driver needs pthread_key it should handling that in the close.
> > But it really should be using DPDK thread local storage for this.
> >   
> This is fine for graceful application termination, but what about non-graceful application termination?
> I have a DPAA bus cleanup patch ready, and will submit it soon, But we still need
> destructor in case application exit without calling the rte_eal_cleanup().

If application crashes, the code isn't going to be called  so the pthread destructor won't help.
You need to build any driver so that if application ungracefully exits, the hardware can be
reset on restart. Trying to rely on application doing anything while crashing is not going
to work. Alternatively, having a distinct separate watcher process (or using systemd) that can
do a forceful cleanup on application crash can work. The point is you can't handle non-graceful
shutdown cleanup in a DPDK driver and have it work reliably.


> 
> I can see DPDK is giving an API rte_thread_key_create() which is defined only in UNIX and windows.
> Why not for Linux?

I was thinking of the recent per-thread allocator.

> 
> I know DPAA driver is using pthread_key which it should not use but instead use
> rte_thread_key_create() provided by DPDK which we can replace.
> Having pthread destructor is still a valid case in my opinion.