[dpdk-dev] [PATCH] ipc: stop mp control thread on cleanup

[dpdk-dev] [PATCH] ipc: stop mp control thread on cleanup

[David Marchand]

When calling rte_eal_cleanup, the mp channel cleanup routine only sets
mp_fd to -1 leaving the rte_mp_handle control thread running.
This control thread can spew warnings on reading on an invalid fd.

To handle this situation, sets mp_fd to -1 to signal the control thread
it should exit, but since this thread might be sleeping on the socket,
cancel the thread too.

Fixes: 85d6815fa6d0 ("eal: close multi-process socket during cleanup")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_proc.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/lib/eal/common/eal_common_proc.c b/lib/eal/common/eal_common_proc.c
index dc4a2efa82..2c46c21421 100644
--- a/lib/eal/common/eal_common_proc.c
+++ b/lib/eal/common/eal_common_proc.c
@@ -35,6 +35,7 @@
 #include "eal_internal_cfg.h"
 
 static int mp_fd = -1;
+static pthread_t mp_handle_tid;
 static char mp_filter[PATH_MAX];   /* Filter for secondary process sockets */
 static char mp_dir_path[PATH_MAX]; /* The directory path for all mp sockets */
 static pthread_mutex_t mp_mutex_action = PTHREAD_MUTEX_INITIALIZER;
@@ -383,7 +384,7 @@ mp_handle(void *arg __rte_unused)
 	struct mp_msg_internal msg;
 	struct sockaddr_un sa;
 
-	while (1) {
+	while (mp_fd >= 0) {
 		if (read_msg(&msg, &sa) == 0)
 			process_msg(&msg, &sa);
 	}
@@ -566,25 +567,11 @@ open_socket_fd(void)
 	return mp_fd;
 }
 
-static void
-close_socket_fd(void)
-{
-	char path[PATH_MAX];
-
-	if (mp_fd < 0)
-		return;
-
-	close(mp_fd);
-	create_socket_path(peer_name, path, sizeof(path));
-	unlink(path);
-}
-
 int
 rte_mp_channel_init(void)
 {
 	char path[PATH_MAX];
 	int dir_fd;
-	pthread_t mp_handle_tid;
 	const struct internal_config *internal_conf =
 		eal_get_internal_configuration();
 
@@ -645,7 +632,19 @@ rte_mp_channel_init(void)
 void
 rte_mp_channel_cleanup(void)
 {
-	close_socket_fd();
+	char path[PATH_MAX];
+	int fd;
+
+	if (mp_fd < 0)
+		return;
+
+	fd = mp_fd;
+	mp_fd = -1;
+	pthread_cancel(mp_handle_tid);
+	pthread_join(mp_handle_tid, NULL);
+	close(fd);
+	create_socket_path(peer_name, path, sizeof(path));
+	unlink(path);
 }
 
 /**
-- 
2.23.0

Re: [dpdk-dev] [dpdk-stable] [PATCH] ipc: stop mp control thread on cleanup

[David Marchand]

On Mon, Jun 14, 2021 at 11:13 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> When calling rte_eal_cleanup, the mp channel cleanup routine only sets
> mp_fd to -1 leaving the rte_mp_handle control thread running.
> This control thread can spew warnings on reading on an invalid fd.
>
> To handle this situation, sets mp_fd to -1 to signal the control thread
> it should exit, but since this thread might be sleeping on the socket,
> cancel the thread too.
>
> Fixes: 85d6815fa6d0 ("eal: close multi-process socket during cleanup")
> Cc: stable@dpdk.org
>

Reported-by: Owen Hilyard <ohilyard@iol.unh.edu>

> Signed-off-by: David Marchand <david.marchand@redhat.com>

Anatoly, review please.


-- 
David Marchand

[dpdk-dev] [PATCH v2] ipc: stop mp control thread on cleanup

[David Marchand]

When calling rte_eal_cleanup, the mp channel cleanup routine only sets
mp_fd to -1 leaving the rte_mp_handle control thread running.
This control thread can spew warnings on reading on an invalid fd.
This is especially noticed with ASAN enabled.

To handle this situation, set mp_fd to -1 to signal the control thread
it should exit, but since this thread might be sleeping on the socket,
cancel the thread too.

Fixes: 85d6815fa6d0 ("eal: close multi-process socket during cleanup")
Cc: stable@dpdk.org

Reported-by: Owen Hilyard <ohilyard@iol.unh.edu>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- no functional change, but left close_socket_fd() helper to keep
  symmetry with rte_mp_channel_init()/open_socket_fd(),

---
 lib/eal/common/eal_common_proc.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/lib/eal/common/eal_common_proc.c b/lib/eal/common/eal_common_proc.c
index dc4a2efa82..ebd0f6673b 100644
--- a/lib/eal/common/eal_common_proc.c
+++ b/lib/eal/common/eal_common_proc.c
@@ -35,6 +35,7 @@
 #include "eal_internal_cfg.h"
 
 static int mp_fd = -1;
+static pthread_t mp_handle_tid;
 static char mp_filter[PATH_MAX];   /* Filter for secondary process sockets */
 static char mp_dir_path[PATH_MAX]; /* The directory path for all mp sockets */
 static pthread_mutex_t mp_mutex_action = PTHREAD_MUTEX_INITIALIZER;
@@ -383,7 +384,7 @@ mp_handle(void *arg __rte_unused)
 	struct mp_msg_internal msg;
 	struct sockaddr_un sa;
 
-	while (1) {
+	while (mp_fd >= 0) {
 		if (read_msg(&msg, &sa) == 0)
 			process_msg(&msg, &sa);
 	}
@@ -567,14 +568,11 @@ open_socket_fd(void)
 }
 
 static void
-close_socket_fd(void)
+close_socket_fd(int fd)
 {
 	char path[PATH_MAX];
 
-	if (mp_fd < 0)
-		return;
-
-	close(mp_fd);
+	close(fd);
 	create_socket_path(peer_name, path, sizeof(path));
 	unlink(path);
 }
@@ -584,7 +582,6 @@ rte_mp_channel_init(void)
 {
 	char path[PATH_MAX];
 	int dir_fd;
-	pthread_t mp_handle_tid;
 	const struct internal_config *internal_conf =
 		eal_get_internal_configuration();
 
@@ -645,7 +642,16 @@ rte_mp_channel_init(void)
 void
 rte_mp_channel_cleanup(void)
 {
-	close_socket_fd();
+	int fd;
+
+	if (mp_fd < 0)
+		return;
+
+	fd = mp_fd;
+	mp_fd = -1;
+	pthread_cancel(mp_handle_tid);
+	pthread_join(mp_handle_tid, NULL);
+	close_socket_fd(fd);
 }
 
 /**
-- 
2.23.0

Re: [dpdk-dev] [PATCH v2] ipc: stop mp control thread on cleanup

[Maxime Coquelin]

On 7/7/21 1:02 PM, David Marchand wrote:
> When calling rte_eal_cleanup, the mp channel cleanup routine only sets
> mp_fd to -1 leaving the rte_mp_handle control thread running.
> This control thread can spew warnings on reading on an invalid fd.
> This is especially noticed with ASAN enabled.
> 
> To handle this situation, set mp_fd to -1 to signal the control thread
> it should exit, but since this thread might be sleeping on the socket,
> cancel the thread too.
> 
> Fixes: 85d6815fa6d0 ("eal: close multi-process socket during cleanup")
> Cc: stable@dpdk.org
> 
> Reported-by: Owen Hilyard <ohilyard@iol.unh.edu>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - no functional change, but left close_socket_fd() helper to keep
>   symmetry with rte_mp_channel_init()/open_socket_fd(),
> 
> ---
>  lib/eal/common/eal_common_proc.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v2] ipc: stop mp control thread on cleanup

[David Marchand]

On Thu, Jul 8, 2021 at 9:23 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 7/7/21 1:02 PM, David Marchand wrote:
> > When calling rte_eal_cleanup, the mp channel cleanup routine only sets
> > mp_fd to -1 leaving the rte_mp_handle control thread running.
> > This control thread can spew warnings on reading on an invalid fd.
> > This is especially noticed with ASAN enabled.
> >
> > To handle this situation, set mp_fd to -1 to signal the control thread
> > it should exit, but since this thread might be sleeping on the socket,
> > cancel the thread too.
> >
> > Fixes: 85d6815fa6d0 ("eal: close multi-process socket during cleanup")
> > Cc: stable@dpdk.org
> >
> > Reported-by: Owen Hilyard <ohilyard@iol.unh.edu>
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Applied, thanks.


-- 
David Marchand