[dpdk-dev] [PATCH] pdump: change to use generic multi-process channel

[dpdk-dev] [PATCH] pdump: change to use generic multi-process channel

[Jianfeng Tan]

The original code replies on the private channel for primary and
secondary communication. Change to use the generic multi-process
channel.

Note with this change, dpdk-pdump will be not compatible with
old version DPDK applications.

Cc: reshma.pattan@intel.com

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 lib/librte_pdump/Makefile    |   3 +-
 lib/librte_pdump/rte_pdump.c | 420 +++++++------------------------------------
 lib/librte_pdump/rte_pdump.h |   1 +
 3 files changed, 66 insertions(+), 358 deletions(-)

diff --git a/lib/librte_pdump/Makefile b/lib/librte_pdump/Makefile
index 98fa752..0ee0fa1 100644
--- a/lib/librte_pdump/Makefile
+++ b/lib/librte_pdump/Makefile
@@ -1,11 +1,12 @@
 # SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2016 Intel Corporation
+# Copyright(c) 2016-2018 Intel Corporation
 
 include $(RTE_SDK)/mk/rte.vars.mk
 
 # library name
 LIB = librte_pdump.a
 
+CFLAGS += -DALLOW_EXPERIMENTAL_API
 CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3
 CFLAGS += -D_GNU_SOURCE
 LDLIBS += -lpthread
diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index ec8a5d8..1dee72f 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -1,16 +1,7 @@
 /* SPDX-License-Identifier: BSD-3-Clause
- * Copyright(c) 2016 Intel Corporation
+ * Copyright(c) 2016-2018 Intel Corporation
  */
 
-#include <sys/socket.h>
-#include <sys/un.h>
-#include <sys/stat.h>
-#include <unistd.h>
-#include <sys/types.h>
-#include <pthread.h>
-#include <stdbool.h>
-#include <stdio.h>
-
 #include <rte_memcpy.h>
 #include <rte_mbuf.h>
 #include <rte_ethdev.h>
@@ -20,12 +11,6 @@
 
 #include "rte_pdump.h"
 
-#define SOCKET_PATH_VAR_RUN "/var/run"
-#define SOCKET_PATH_HOME "HOME"
-#define DPDK_DIR         "/.dpdk"
-#define SOCKET_DIR       "/pdump_sockets"
-#define SERVER_SOCKET "%s/pdump_server_socket"
-#define CLIENT_SOCKET "%s/pdump_client_socket_%d_%u"
 #define DEVICE_ID_SIZE 64
 /* Macros for printing using RTE_LOG */
 #define RTE_LOGTYPE_PDUMP RTE_LOGTYPE_USER1
@@ -39,11 +24,6 @@ enum pdump_version {
 	V1 = 1
 };
 
-static pthread_t pdump_thread;
-static int pdump_socket_fd;
-static char server_socket_dir[PATH_MAX];
-static char client_socket_dir[PATH_MAX];
-
 struct pdump_request {
 	uint16_t ver;
 	uint16_t op;
@@ -307,7 +287,7 @@ pdump_register_tx_callbacks(uint16_t end_q, uint16_t port, uint16_t queue,
 }
 
 static int
-set_pdump_rxtx_cbs(struct pdump_request *p)
+set_pdump_rxtx_cbs(const struct pdump_request *p)
 {
 	uint16_t nb_rx_q = 0, nb_tx_q = 0, end_q, queue;
 	uint16_t port;
@@ -391,313 +371,49 @@ set_pdump_rxtx_cbs(struct pdump_request *p)
 	return ret;
 }
 
-/* get socket path (/var/run if root, $HOME otherwise) */
 static int
-pdump_get_socket_path(char *buffer, int bufsz, enum rte_pdump_socktype type)
+pdump_server(const struct rte_mp_msg *mp_msg, const void *peer)
 {
-	char dpdk_dir[PATH_MAX] = {0};
-	char dir[PATH_MAX] = {0};
-	char *dir_home = NULL;
-	int ret = 0;
-
-	if (type == RTE_PDUMP_SOCKET_SERVER && server_socket_dir[0] != 0)
-		snprintf(dir, sizeof(dir), "%s", server_socket_dir);
-	else if (type == RTE_PDUMP_SOCKET_CLIENT && client_socket_dir[0] != 0)
-		snprintf(dir, sizeof(dir), "%s", client_socket_dir);
-	else {
-		if (getuid() != 0) {
-			dir_home = getenv(SOCKET_PATH_HOME);
-			if (!dir_home) {
-				RTE_LOG(ERR, PDUMP,
-					"Failed to get environment variable"
-					" value for %s, %s:%d\n",
-					SOCKET_PATH_HOME, __func__, __LINE__);
-				return -1;
-			}
-			snprintf(dpdk_dir, sizeof(dpdk_dir), "%s%s",
-					dir_home, DPDK_DIR);
-		} else
-			snprintf(dpdk_dir, sizeof(dpdk_dir), "%s%s",
-					SOCKET_PATH_VAR_RUN, DPDK_DIR);
-
-		mkdir(dpdk_dir, 0700);
-		snprintf(dir, sizeof(dir), "%s%s",
-					dpdk_dir, SOCKET_DIR);
-	}
-
-	ret =  mkdir(dir, 0700);
-	/* if user passed socket path is invalid, return immediately */
-	if (ret < 0 && errno != EEXIST) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create dir:%s:%s\n", dir,
-			strerror(errno));
-		rte_errno = errno;
-		return -1;
-	}
-
-	if (type == RTE_PDUMP_SOCKET_SERVER)
-		snprintf(buffer, bufsz, SERVER_SOCKET, dir);
-	else
-		snprintf(buffer, bufsz, CLIENT_SOCKET, dir, getpid(),
-				rte_sys_gettid());
-
-	return 0;
-}
-
-static int
-pdump_create_server_socket(void)
-{
-	int ret, socket_fd;
-	struct sockaddr_un addr;
-	socklen_t addr_len;
-
-	ret = pdump_get_socket_path(addr.sun_path, sizeof(addr.sun_path),
-				RTE_PDUMP_SOCKET_SERVER);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to get server socket path: %s:%d\n",
-			__func__, __LINE__);
-		return -1;
-	}
-	addr.sun_family = AF_UNIX;
-
-	/* remove if file already exists */
-	unlink(addr.sun_path);
-
-	/* set up a server socket */
-	socket_fd = socket(AF_UNIX, SOCK_DGRAM, 0);
-	if (socket_fd < 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create server socket: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		return -1;
-	}
-
-	addr_len = sizeof(struct sockaddr_un);
-	ret = bind(socket_fd, (struct sockaddr *) &addr, addr_len);
-	if (ret) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to bind to server socket: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		close(socket_fd);
-		return -1;
+	struct rte_mp_msg mp_resp;
+	const struct pdump_request *cli_req;
+	struct pdump_response *resp = (struct pdump_response *)&mp_resp.param;
+
+	/* recv client requests */
+	if (mp_msg->len_param != sizeof(*cli_req)) {
+		RTE_LOG(ERR, PDUMP, "failed to recv from client\n");
+		resp->err_value = EINVAL;
+	} else {
+		cli_req = (const struct pdump_request *)mp_msg->param;
+		resp->ver = cli_req->ver;
+		resp->res_op = cli_req->op;
+		resp->err_value = set_pdump_rxtx_cbs(cli_req);
 	}
 
-	/* save the socket in local configuration */
-	pdump_socket_fd = socket_fd;
+	strcpy(mp_resp.name, "pdump");
+	mp_resp.len_param = sizeof(*resp);
+	mp_resp.num_fds = 0;
+	if (rte_mp_reply(&mp_resp, peer) < 0)
+		RTE_LOG(ERR, PDUMP, "failed to send to client:%s, %s:%d\n",
+			strerror(rte_errno), __func__, __LINE__);
 
 	return 0;
 }
 
-static __attribute__((noreturn)) void *
-pdump_thread_main(__rte_unused void *arg)
-{
-	struct sockaddr_un cli_addr;
-	socklen_t cli_len;
-	struct pdump_request cli_req;
-	struct pdump_response resp;
-	int n;
-	int ret = 0;
-
-	/* host thread, never break out */
-	for (;;) {
-		/* recv client requests */
-		cli_len = sizeof(cli_addr);
-		n = recvfrom(pdump_socket_fd, &cli_req,
-				sizeof(struct pdump_request), 0,
-				(struct sockaddr *)&cli_addr, &cli_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to recv from client:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			continue;
-		}
-
-		ret = set_pdump_rxtx_cbs(&cli_req);
-
-		resp.ver = cli_req.ver;
-		resp.res_op = cli_req.op;
-		resp.err_value = ret;
-		n = sendto(pdump_socket_fd, &resp,
-				sizeof(struct pdump_response),
-				0, (struct sockaddr *)&cli_addr, cli_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to send to client:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-		}
-	}
-}
-
 int
-rte_pdump_init(const char *path)
+rte_pdump_init(const char *path __rte_unused)
 {
-	int ret = 0;
-	char thread_name[RTE_MAX_THREAD_NAME_LEN];
-
-	ret = rte_pdump_set_socket_dir(path, RTE_PDUMP_SOCKET_SERVER);
-	if (ret != 0)
-		return -1;
-
-	ret = pdump_create_server_socket();
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create server socket:%s:%d\n",
-			__func__, __LINE__);
-		return -1;
-	}
-
-	/* create the host thread to wait/handle pdump requests */
-	ret = pthread_create(&pdump_thread, NULL, pdump_thread_main, NULL);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create the pdump thread:%s, %s:%d\n",
-			strerror(ret), __func__, __LINE__);
-		return -1;
-	}
-	/* Set thread_name for aid in debugging. */
-	snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "pdump-thread");
-	ret = rte_thread_setname(pdump_thread, thread_name);
-	if (ret != 0) {
-		RTE_LOG(DEBUG, PDUMP,
-			"Failed to set thread name for pdump handling\n");
-	}
-
-	return 0;
+	return rte_mp_action_register("pdump", pdump_server);
 }
 
 int
 rte_pdump_uninit(void)
 {
-	int ret;
-
-	ret = pthread_cancel(pdump_thread);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to cancel the pdump thread:%s, %s:%d\n",
-			strerror(ret), __func__, __LINE__);
-		return -1;
-	}
-
-	ret = close(pdump_socket_fd);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to close server socket: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		return -1;
-	}
-
-	struct sockaddr_un addr;
-
-	ret = pdump_get_socket_path(addr.sun_path, sizeof(addr.sun_path),
-				RTE_PDUMP_SOCKET_SERVER);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to get server socket path: %s:%d\n",
-			__func__, __LINE__);
-		return -1;
-	}
-	ret = unlink(addr.sun_path);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to remove server socket addr: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		return -1;
-	}
+	rte_mp_action_unregister("pdump");
 
 	return 0;
 }
 
 static int
-pdump_create_client_socket(struct pdump_request *p)
-{
-	int ret, socket_fd;
-	int pid;
-	int n;
-	struct pdump_response server_resp;
-	struct sockaddr_un addr, serv_addr, from;
-	socklen_t addr_len, serv_len;
-
-	pid = getpid();
-
-	socket_fd = socket(AF_UNIX, SOCK_DGRAM, 0);
-	if (socket_fd < 0) {
-		RTE_LOG(ERR, PDUMP,
-			"client socket(): %s:pid(%d):tid(%u), %s:%d\n",
-			strerror(errno), pid, rte_sys_gettid(),
-			__func__, __LINE__);
-		rte_errno = errno;
-		return -1;
-	}
-
-	ret = pdump_get_socket_path(addr.sun_path, sizeof(addr.sun_path),
-				RTE_PDUMP_SOCKET_CLIENT);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to get client socket path: %s:%d\n",
-			__func__, __LINE__);
-		rte_errno = errno;
-		goto exit;
-	}
-	addr.sun_family = AF_UNIX;
-	addr_len = sizeof(struct sockaddr_un);
-
-	do {
-		ret = bind(socket_fd, (struct sockaddr *) &addr, addr_len);
-		if (ret) {
-			RTE_LOG(ERR, PDUMP,
-				"client bind(): %s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			rte_errno = errno;
-			break;
-		}
-
-		serv_len = sizeof(struct sockaddr_un);
-		memset(&serv_addr, 0, sizeof(serv_addr));
-		ret = pdump_get_socket_path(serv_addr.sun_path,
-					sizeof(serv_addr.sun_path),
-					RTE_PDUMP_SOCKET_SERVER);
-		if (ret != 0) {
-			RTE_LOG(ERR, PDUMP,
-				"Failed to get server socket path: %s:%d\n",
-				__func__, __LINE__);
-			rte_errno = errno;
-			break;
-		}
-		serv_addr.sun_family = AF_UNIX;
-
-		n =  sendto(socket_fd, p, sizeof(struct pdump_request), 0,
-				(struct sockaddr *)&serv_addr, serv_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to send to server:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			rte_errno = errno;
-			ret = -1;
-			break;
-		}
-
-		n = recvfrom(socket_fd, &server_resp,
-				sizeof(struct pdump_response), 0,
-				(struct sockaddr *)&from, &serv_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to recv from server:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			rte_errno = errno;
-			ret = -1;
-			break;
-		}
-		ret = server_resp.err_value;
-	} while (0);
-
-exit:
-	close(socket_fd);
-	unlink(addr.sun_path);
-	return ret;
-}
-
-static int
 pdump_validate_ring_mp(struct rte_ring *ring, struct rte_mempool *mp)
 {
 	if (ring == NULL || mp == NULL) {
@@ -768,36 +484,48 @@ pdump_prepare_client_request(char *device, uint16_t queue,
 				struct rte_mempool *mp,
 				void *filter)
 {
-	int ret;
-	struct pdump_request req = {.ver = 1,};
-
-	req.flags = flags;
-	req.op =  operation;
+	int ret = -1;
+	struct rte_mp_msg mp_req, *mp_rep;
+	struct rte_mp_reply mp_reply;
+	struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
+	struct pdump_request *req = (struct pdump_request *)mp_req.param;
+	struct pdump_response *resp;
+
+	req->ver = 1;
+	req->flags = flags;
+	req->op = operation;
 	if ((operation & ENABLE) != 0) {
-		snprintf(req.data.en_v1.device, sizeof(req.data.en_v1.device),
-				"%s", device);
-		req.data.en_v1.queue = queue;
-		req.data.en_v1.ring = ring;
-		req.data.en_v1.mp = mp;
-		req.data.en_v1.filter = filter;
+		snprintf(req->data.en_v1.device,
+			 sizeof(req->data.en_v1.device), "%s", device);
+		req->data.en_v1.queue = queue;
+		req->data.en_v1.ring = ring;
+		req->data.en_v1.mp = mp;
+		req->data.en_v1.filter = filter;
 	} else {
-		snprintf(req.data.dis_v1.device, sizeof(req.data.dis_v1.device),
-				"%s", device);
-		req.data.dis_v1.queue = queue;
-		req.data.dis_v1.ring = NULL;
-		req.data.dis_v1.mp = NULL;
-		req.data.dis_v1.filter = NULL;
+		snprintf(req->data.dis_v1.device,
+			 sizeof(req->data.dis_v1.device), "%s", device);
+		req->data.dis_v1.queue = queue;
+		req->data.dis_v1.ring = NULL;
+		req->data.dis_v1.mp = NULL;
+		req->data.dis_v1.filter = NULL;
+	}
+
+	strcpy(mp_req.name, "pdump");
+	mp_req.len_param = sizeof(*req);
+	mp_req.num_fds = 0;
+	if (rte_mp_request(&mp_req, &mp_reply, &ts) == 0) {
+		mp_rep = &mp_reply.msgs[0];
+		resp = (struct pdump_response *)mp_rep->param;
+		rte_errno = resp->err_value;
+		if (!resp->err_value)
+			ret = 0;
+		free(mp_reply.msgs);
 	}
 
-	ret = pdump_create_client_socket(&req);
-	if (ret < 0) {
+	if (ret < 0)
 		RTE_LOG(ERR, PDUMP,
 			"client request for pdump enable/disable failed\n");
-		rte_errno = ret;
-		return -1;
-	}
-
-	return 0;
+	return ret;
 }
 
 int
@@ -884,30 +612,8 @@ rte_pdump_disable_by_deviceid(char *device_id, uint16_t queue,
 }
 
 int
-rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype type)
+rte_pdump_set_socket_dir(const char *path __rte_unused,
+			 enum rte_pdump_socktype type __rte_unused)
 {
-	int ret, count;
-
-	if (path != NULL) {
-		if (type == RTE_PDUMP_SOCKET_SERVER) {
-			count = sizeof(server_socket_dir);
-			ret = snprintf(server_socket_dir, count, "%s", path);
-		} else {
-			count = sizeof(client_socket_dir);
-			ret = snprintf(client_socket_dir, count, "%s", path);
-		}
-
-		if (ret < 0  || ret >= count) {
-			RTE_LOG(ERR, PDUMP,
-					"Invalid socket path:%s:%d\n",
-					__func__, __LINE__);
-			if (type == RTE_PDUMP_SOCKET_SERVER)
-				server_socket_dir[0] = 0;
-			else
-				client_socket_dir[0] = 0;
-			return -EINVAL;
-		}
-	}
-
 	return 0;
 }
diff --git a/lib/librte_pdump/rte_pdump.h b/lib/librte_pdump/rte_pdump.h
index a7e8372..787adb2 100644
--- a/lib/librte_pdump/rte_pdump.h
+++ b/lib/librte_pdump/rte_pdump.h
@@ -163,6 +163,7 @@ rte_pdump_disable_by_deviceid(char *device_id, uint16_t queue,
 				uint32_t flags);
 
 /**
+ * @deprecated
  * Allows applications to set server and client socket paths.
  * If specified path is null default path will be selected, i.e.
  *"/var/run/" for root user and "$HOME" for non root user.
-- 
2.7.4

Re: [dpdk-dev] [PATCH] pdump: change to use generic multi-process channel

[Pattan, Reshma]

Hi,

> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Sunday, March 4, 2018 3:04 PM
> To: dev@dpdk.org
> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>
> Subject: [PATCH] pdump: change to use generic multi-process channel
> 
> The original code replies on the private channel for primary and secondary
> communication. Change to use the generic multi-process channel.
> 
> Note with this change, dpdk-pdump will be not compatible with old version
> DPDK applications.
> 

Is this the correct time to make this change? As I see the rte_mp APIs are still in experimental stage?
If you wish to change them,

1) I feel ABI breakage has to be addressed  first for change in  rte_pdump_init() .
2)ABI notice for removal of the rte_pdump_set_socket_dir()  and then remove it completely . 
3)Need to do cleanup of the code app/dpdk-pdump. 
4)After all the changes we need to make sure dpdk-pdump works fine without breaking the functionality, validation team should be able to help.
5)Replace strcpy  with snprintf.

Thanks,
Reshma

Re: [dpdk-dev] [PATCH] pdump: change to use generic multi-process channel

[Tan, Jianfeng]

Thank you for the comments!

> -----Original Message-----
> From: Pattan, Reshma
> Sent: Wednesday, March 21, 2018 12:38 AM
> To: Tan, Jianfeng; dev@dpdk.org
> Subject: RE: [PATCH] pdump: change to use generic multi-process channel
> 
> Hi,
> 
> > -----Original Message-----
> > From: Tan, Jianfeng
> > Sent: Sunday, March 4, 2018 3:04 PM
> > To: dev@dpdk.org
> > Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; Pattan, Reshma
> > <reshma.pattan@intel.com>
> > Subject: [PATCH] pdump: change to use generic multi-process channel
> >
> > The original code replies on the private channel for primary and secondary
> > communication. Change to use the generic multi-process channel.
> >
> > Note with this change, dpdk-pdump will be not compatible with old version
> > DPDK applications.
> >
> 
> Is this the correct time to make this change? As I see the rte_mp APIs are still
> in experimental stage?
> If you wish to change them,
> 
> 1) I feel ABI breakage has to be addressed  first for change in rte_pdump_init() . 
> 2)ABI notice for removal of the rte_pdump_set_socket_dir()  and then remove it completely .

This patch itself does not break any ABI. It just puts parameters of rte_pdump_init() not used. And make rte_pdump_set_socket_dir() as a dummy function.

> 3)Need to do cleanup of the code app/dpdk-pdump.

Yes, I understand it's a normal process to announce deprecation firstly, and then do the change.

But here is the thing, with generic mp introduced, we will not be compatible with DPDK versions.
So we want to unify the use of generic mp channel in this release for vfio, pdump, vdev, memory rework.
And in fact, ABI/API changes could be delayed to later releases.

> 4)After all the changes we need to make sure dpdk-pdump works fine
> without breaking the functionality, validation team should be able to help.

I have done a simple test of pdump. Can you suggest where can I get the comprehensive test cases?

> 5)Replace strcpy  with snprintf.

Will do.

Thanks,
Jianfeng

> 
> Thanks,
> Reshma
> 

Re: [dpdk-dev] [PATCH] pdump: change to use generic multi-process channel

[Pattan, Reshma]

Hi,

> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Wednesday, March 21, 2018 2:28 AM
> To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org
> Subject: RE: [PATCH] pdump: change to use generic multi-process channel
> 

Hi,

> > 1) I feel ABI breakage has to be addressed  first for change in
> rte_pdump_init() .
> > 2)ABI notice for removal of the rte_pdump_set_socket_dir()  and then
> remove it completely .
> 
> This patch itself does not break any ABI. It just puts parameters of
> rte_pdump_init() not used. And make rte_pdump_set_socket_dir() as a
> dummy function.
> 

So, for current release you just mark parameters unused and functions set to dummy, in future release you announce 
ABI breakage by removing them completely? If that is agreed plan I don't have any issues.

> > 3)Need to do cleanup of the code app/dpdk-pdump.
> 
> Yes, I understand it's a normal process to announce deprecation firstly, and
> then do the change.
> 
> But here is the thing, with generic mp introduced, we will not be compatible
> with DPDK versions.
> So we want to unify the use of generic mp channel in this release for vfio,
> pdump, vdev, memory rework.
> And in fact, ABI/API changes could be delayed to later releases.

So, you want to remove unnecessary socket  related code from dpdk-pdump in future release itself?  Kind of making sense. 
But dpdk-pdump  tool has socket path related command line options which user still can pass on, isn't it kind of confusion we creating w.r.t
Internal design and usage? 

> 
> > 4)After all the changes we need to make sure dpdk-pdump works fine
> > without breaking the functionality, validation team should be able to help.
> 
> I have done a simple test of pdump. Can you suggest where can I get the
> comprehensive test cases?
> 

Ok, if you have verified and observed packets are been captured successfully, that is good enough.

Thanks,
Reshma

Re: [dpdk-dev] [PATCH] pdump: change to use generic multi-process channel

[Tan, Jianfeng]

> -----Original Message-----
> From: Pattan, Reshma
> Sent: Wednesday, March 21, 2018 5:55 PM
> To: Tan, Jianfeng; dev@dpdk.org
> Subject: RE: [PATCH] pdump: change to use generic multi-process channel
> 
> Hi,
> 
> > -----Original Message-----
> > From: Tan, Jianfeng
> > Sent: Wednesday, March 21, 2018 2:28 AM
> > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org
> > Subject: RE: [PATCH] pdump: change to use generic multi-process channel
> >
> 
> Hi,
> 
> > > 1) I feel ABI breakage has to be addressed  first for change in
> > rte_pdump_init() .
> > > 2)ABI notice for removal of the rte_pdump_set_socket_dir()  and then
> > remove it completely .
> >
> > This patch itself does not break any ABI. It just puts parameters of
> > rte_pdump_init() not used. And make rte_pdump_set_socket_dir() as a
> > dummy function.
> >
> 
> So, for current release you just mark parameters unused and functions set to
> dummy, in future release you announce
> ABI breakage by removing them completely? If that is agreed plan I don't
> have any issues.

Actually, as you commented, we can announce the deprecation with this patch.

> 
> > > 3)Need to do cleanup of the code app/dpdk-pdump.
> >
> > Yes, I understand it's a normal process to announce deprecation firstly, and
> > then do the change.
> >
> > But here is the thing, with generic mp introduced, we will not be
> compatible
> > with DPDK versions.
> > So we want to unify the use of generic mp channel in this release for vfio,
> > pdump, vdev, memory rework.
> > And in fact, ABI/API changes could be delayed to later releases.
> 
> So, you want to remove unnecessary socket  related code from dpdk-pdump
> in future release itself?  Kind of making sense.
> But dpdk-pdump  tool has socket path related command line options which
> user still can pass on, isn't it kind of confusion we creating w.r.t
> Internal design and usage?

AFAIK, these options do not affect anything with this patch even they are set. How about printing a warning saying that these options will be deprecated and take no effect now?

I'll send a v2 for your review.

Thanks,
Jianfeng

> 
> >
> > > 4)After all the changes we need to make sure dpdk-pdump works fine
> > > without breaking the functionality, validation team should be able to help.
> >
> > I have done a simple test of pdump. Can you suggest where can I get the
> > comprehensive test cases?
> >
> 
> Ok, if you have verified and observed packets are been captured successfully,
> that is good enough.
> 
> Thanks,
> Reshma

Re: [dpdk-dev] [PATCH] pdump: change to use generic multi-process channel

[Pattan, Reshma]

Hi,

> >
> > > > 1) I feel ABI breakage has to be addressed  first for change in
> > > rte_pdump_init() .
> >
> > So, you want to remove unnecessary socket  related code from
> > dpdk-pdump in future release itself?  Kind of making sense.
> > But dpdk-pdump  tool has socket path related command line options
> > which user still can pass on, isn't it kind of confusion we creating
> > w.r.t Internal design and usage?
> 
> AFAIK, these options do not affect anything with this patch even they are set.
> How about printing a warning saying that these options will be deprecated
> and take no effect now?

Fine I guess, when the ABI notice is sent to remove all socket path code, that time you can remove the socket path cli options too.

Thanks,
Reshma

[dpdk-dev] [PATCH v2] pdump: change to use generic multi-process channel

[Jianfeng Tan]

The original code replies on the private channel for primary and
secondary communication. Change to use the generic multi-process
channel.

Note with this change, dpdk-pdump will be not compatible with
old version DPDK applications.

Cc: reshma.pattan@intel.com

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
Note this patch needs this patch set: http://dpdk.org/dev/patchwork/patch/36814/
v2:
  - Update doc for deprecation of API, rte_pdump_set_socket_dir,
    and API change for rte_pdump_init.
  - Add notice for known incompatibility issue in doc.
 app/pdump/main.c                       |   6 +-
 doc/guides/rel_notes/deprecation.rst   |   4 +
 doc/guides/rel_notes/release_18_05.rst |   7 +
 lib/librte_pdump/Makefile              |   3 +-
 lib/librte_pdump/rte_pdump.c           | 423 +++++----------------------------
 lib/librte_pdump/rte_pdump.h           |   1 +
 6 files changed, 84 insertions(+), 360 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index f6865bd..3779164 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -155,9 +155,11 @@ pdump_usage(const char *prgname)
 			"[mbuf-size=<mbuf data size>default:2176],"
 			"[total-num-mbufs=<number of mbufs>default:65535]'\n"
 			"[--server-socket-path=<server socket dir>"
-				"default:/var/run/.dpdk/ (or) ~/.dpdk/]\n"
+				" which is deprecated and will be removed soon,"
+				" default:/var/run/.dpdk/ (or) ~/.dpdk/]\n"
 			"[--client-socket-path=<client socket dir>"
-				"default:/var/run/.dpdk/ (or) ~/.dpdk/]\n",
+				" which is deprecated and will be removed soon,"
+				" default:/var/run/.dpdk/ (or) ~/.dpdk/]\n",
 			prgname);
 }
 
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 0c696f7..d55fd05 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -153,3 +153,7 @@ Deprecation Notices
   be added between the producer and consumer structures. The size of the
   structure and the offset of the fields will remain the same on
   platforms with 64B cache line, but will change on other platforms.
+
+* pdump: As we changed to use generic IPC, ``rte_pdump_set_socket_dir`` will be
+  deprecated and removed in subsequent release; the parameter, path, of
+  ``rte_pdump_init`` will also be removed.
diff --git a/doc/guides/rel_notes/release_18_05.rst b/doc/guides/rel_notes/release_18_05.rst
index 9cc77f8..eefb901 100644
--- a/doc/guides/rel_notes/release_18_05.rst
+++ b/doc/guides/rel_notes/release_18_05.rst
@@ -114,6 +114,13 @@ Known Issues
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* **pdump is not compatible with old applications.**
+
+  As we changed to use generic multi-process communication for pdump negotiation
+  instead of previous dedicated unix socket way, pdump applications, including
+  dpdk-pdump example and any other applications using librte_pdump, cannot work
+  with older version DPDK primary applications.
+
 
 Shared Library Versions
 -----------------------
diff --git a/lib/librte_pdump/Makefile b/lib/librte_pdump/Makefile
index 98fa752..0ee0fa1 100644
--- a/lib/librte_pdump/Makefile
+++ b/lib/librte_pdump/Makefile
@@ -1,11 +1,12 @@
 # SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2016 Intel Corporation
+# Copyright(c) 2016-2018 Intel Corporation
 
 include $(RTE_SDK)/mk/rte.vars.mk
 
 # library name
 LIB = librte_pdump.a
 
+CFLAGS += -DALLOW_EXPERIMENTAL_API
 CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3
 CFLAGS += -D_GNU_SOURCE
 LDLIBS += -lpthread
diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index 4fb0b42..37fba47 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -1,16 +1,7 @@
 /* SPDX-License-Identifier: BSD-3-Clause
- * Copyright(c) 2016 Intel Corporation
+ * Copyright(c) 2016-2018 Intel Corporation
  */
 
-#include <sys/socket.h>
-#include <sys/un.h>
-#include <sys/stat.h>
-#include <unistd.h>
-#include <sys/types.h>
-#include <pthread.h>
-#include <stdbool.h>
-#include <stdio.h>
-
 #include <rte_memcpy.h>
 #include <rte_mbuf.h>
 #include <rte_ethdev.h>
@@ -20,16 +11,13 @@
 
 #include "rte_pdump.h"
 
-#define SOCKET_PATH_VAR_RUN "/var/run"
-#define SOCKET_PATH_HOME "HOME"
-#define DPDK_DIR         "/.dpdk"
-#define SOCKET_DIR       "/pdump_sockets"
-#define SERVER_SOCKET "%s/pdump_server_socket"
-#define CLIENT_SOCKET "%s/pdump_client_socket_%d_%u"
 #define DEVICE_ID_SIZE 64
 /* Macros for printing using RTE_LOG */
 #define RTE_LOGTYPE_PDUMP RTE_LOGTYPE_USER1
 
+/* Used for the multi-process communication */
+#define PDUMP_MP	"mp_pdump"
+
 enum pdump_operation {
 	DISABLE = 1,
 	ENABLE = 2
@@ -39,11 +27,6 @@ enum pdump_version {
 	V1 = 1
 };
 
-static pthread_t pdump_thread;
-static int pdump_socket_fd;
-static char server_socket_dir[PATH_MAX];
-static char client_socket_dir[PATH_MAX];
-
 struct pdump_request {
 	uint16_t ver;
 	uint16_t op;
@@ -307,7 +290,7 @@ pdump_register_tx_callbacks(uint16_t end_q, uint16_t port, uint16_t queue,
 }
 
 static int
-set_pdump_rxtx_cbs(struct pdump_request *p)
+set_pdump_rxtx_cbs(const struct pdump_request *p)
 {
 	uint16_t nb_rx_q = 0, nb_tx_q = 0, end_q, queue;
 	uint16_t port;
@@ -391,313 +374,49 @@ set_pdump_rxtx_cbs(struct pdump_request *p)
 	return ret;
 }
 
-/* get socket path (/var/run if root, $HOME otherwise) */
 static int
-pdump_get_socket_path(char *buffer, int bufsz, enum rte_pdump_socktype type)
+pdump_server(const struct rte_mp_msg *mp_msg, const void *peer)
 {
-	char dpdk_dir[PATH_MAX] = {0};
-	char dir[PATH_MAX] = {0};
-	char *dir_home = NULL;
-	int ret = 0;
-
-	if (type == RTE_PDUMP_SOCKET_SERVER && server_socket_dir[0] != 0)
-		snprintf(dir, sizeof(dir), "%s", server_socket_dir);
-	else if (type == RTE_PDUMP_SOCKET_CLIENT && client_socket_dir[0] != 0)
-		snprintf(dir, sizeof(dir), "%s", client_socket_dir);
-	else {
-		if (getuid() != 0) {
-			dir_home = getenv(SOCKET_PATH_HOME);
-			if (!dir_home) {
-				RTE_LOG(ERR, PDUMP,
-					"Failed to get environment variable"
-					" value for %s, %s:%d\n",
-					SOCKET_PATH_HOME, __func__, __LINE__);
-				return -1;
-			}
-			snprintf(dpdk_dir, sizeof(dpdk_dir), "%s%s",
-					dir_home, DPDK_DIR);
-		} else
-			snprintf(dpdk_dir, sizeof(dpdk_dir), "%s%s",
-					SOCKET_PATH_VAR_RUN, DPDK_DIR);
-
-		mkdir(dpdk_dir, 0700);
-		snprintf(dir, sizeof(dir), "%s%s",
-					dpdk_dir, SOCKET_DIR);
-	}
-
-	ret =  mkdir(dir, 0700);
-	/* if user passed socket path is invalid, return immediately */
-	if (ret < 0 && errno != EEXIST) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create dir:%s:%s\n", dir,
-			strerror(errno));
-		rte_errno = errno;
-		return -1;
-	}
-
-	if (type == RTE_PDUMP_SOCKET_SERVER)
-		snprintf(buffer, bufsz, SERVER_SOCKET, dir);
-	else
-		snprintf(buffer, bufsz, CLIENT_SOCKET, dir, getpid(),
-				rte_sys_gettid());
-
-	return 0;
-}
-
-static int
-pdump_create_server_socket(void)
-{
-	int ret, socket_fd;
-	struct sockaddr_un addr;
-	socklen_t addr_len;
-
-	ret = pdump_get_socket_path(addr.sun_path, sizeof(addr.sun_path),
-				RTE_PDUMP_SOCKET_SERVER);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to get server socket path: %s:%d\n",
-			__func__, __LINE__);
-		return -1;
-	}
-	addr.sun_family = AF_UNIX;
-
-	/* remove if file already exists */
-	unlink(addr.sun_path);
-
-	/* set up a server socket */
-	socket_fd = socket(AF_UNIX, SOCK_DGRAM, 0);
-	if (socket_fd < 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create server socket: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		return -1;
-	}
-
-	addr_len = sizeof(struct sockaddr_un);
-	ret = bind(socket_fd, (struct sockaddr *) &addr, addr_len);
-	if (ret) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to bind to server socket: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		close(socket_fd);
-		return -1;
+	struct rte_mp_msg mp_resp;
+	const struct pdump_request *cli_req;
+	struct pdump_response *resp = (struct pdump_response *)&mp_resp.param;
+
+	/* recv client requests */
+	if (mp_msg->len_param != sizeof(*cli_req)) {
+		RTE_LOG(ERR, PDUMP, "failed to recv from client\n");
+		resp->err_value = EINVAL;
+	} else {
+		cli_req = (const struct pdump_request *)mp_msg->param;
+		resp->ver = cli_req->ver;
+		resp->res_op = cli_req->op;
+		resp->err_value = set_pdump_rxtx_cbs(cli_req);
 	}
 
-	/* save the socket in local configuration */
-	pdump_socket_fd = socket_fd;
+	snprintf(mp_resp.name, RTE_MP_MAX_NAME_LEN, PDUMP_MP);
+	mp_resp.len_param = sizeof(*resp);
+	mp_resp.num_fds = 0;
+	if (rte_mp_reply(&mp_resp, peer) < 0)
+		RTE_LOG(ERR, PDUMP, "failed to send to client:%s, %s:%d\n",
+			strerror(rte_errno), __func__, __LINE__);
 
 	return 0;
 }
 
-static __attribute__((noreturn)) void *
-pdump_thread_main(__rte_unused void *arg)
-{
-	struct sockaddr_un cli_addr;
-	socklen_t cli_len;
-	struct pdump_request cli_req;
-	struct pdump_response resp;
-	int n;
-	int ret = 0;
-
-	/* host thread, never break out */
-	for (;;) {
-		/* recv client requests */
-		cli_len = sizeof(cli_addr);
-		n = recvfrom(pdump_socket_fd, &cli_req,
-				sizeof(struct pdump_request), 0,
-				(struct sockaddr *)&cli_addr, &cli_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to recv from client:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			continue;
-		}
-
-		ret = set_pdump_rxtx_cbs(&cli_req);
-
-		resp.ver = cli_req.ver;
-		resp.res_op = cli_req.op;
-		resp.err_value = ret;
-		n = sendto(pdump_socket_fd, &resp,
-				sizeof(struct pdump_response),
-				0, (struct sockaddr *)&cli_addr, cli_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to send to client:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-		}
-	}
-}
-
 int
-rte_pdump_init(const char *path)
+rte_pdump_init(const char *path __rte_unused)
 {
-	int ret = 0;
-	char thread_name[RTE_MAX_THREAD_NAME_LEN];
-
-	ret = rte_pdump_set_socket_dir(path, RTE_PDUMP_SOCKET_SERVER);
-	if (ret != 0)
-		return -1;
-
-	ret = pdump_create_server_socket();
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create server socket:%s:%d\n",
-			__func__, __LINE__);
-		return -1;
-	}
-
-	/* create the host thread to wait/handle pdump requests */
-	ret = pthread_create(&pdump_thread, NULL, pdump_thread_main, NULL);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create the pdump thread:%s, %s:%d\n",
-			strerror(ret), __func__, __LINE__);
-		return -1;
-	}
-	/* Set thread_name for aid in debugging. */
-	snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "pdump-thread");
-	ret = rte_thread_setname(pdump_thread, thread_name);
-	if (ret != 0) {
-		RTE_LOG(DEBUG, PDUMP,
-			"Failed to set thread name for pdump handling\n");
-	}
-
-	return 0;
+	return rte_mp_action_register(PDUMP_MP, pdump_server);
 }
 
 int
 rte_pdump_uninit(void)
 {
-	int ret;
-
-	ret = pthread_cancel(pdump_thread);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to cancel the pdump thread:%s, %s:%d\n",
-			strerror(ret), __func__, __LINE__);
-		return -1;
-	}
-
-	ret = close(pdump_socket_fd);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to close server socket: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		return -1;
-	}
-
-	struct sockaddr_un addr;
-
-	ret = pdump_get_socket_path(addr.sun_path, sizeof(addr.sun_path),
-				RTE_PDUMP_SOCKET_SERVER);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to get server socket path: %s:%d\n",
-			__func__, __LINE__);
-		return -1;
-	}
-	ret = unlink(addr.sun_path);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to remove server socket addr: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		return -1;
-	}
+	rte_mp_action_unregister(PDUMP_MP);
 
 	return 0;
 }
 
 static int
-pdump_create_client_socket(struct pdump_request *p)
-{
-	int ret, socket_fd;
-	int pid;
-	int n;
-	struct pdump_response server_resp;
-	struct sockaddr_un addr, serv_addr, from;
-	socklen_t addr_len, serv_len;
-
-	pid = getpid();
-
-	socket_fd = socket(AF_UNIX, SOCK_DGRAM, 0);
-	if (socket_fd < 0) {
-		RTE_LOG(ERR, PDUMP,
-			"client socket(): %s:pid(%d):tid(%u), %s:%d\n",
-			strerror(errno), pid, rte_sys_gettid(),
-			__func__, __LINE__);
-		rte_errno = errno;
-		return -1;
-	}
-
-	ret = pdump_get_socket_path(addr.sun_path, sizeof(addr.sun_path),
-				RTE_PDUMP_SOCKET_CLIENT);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to get client socket path: %s:%d\n",
-			__func__, __LINE__);
-		rte_errno = errno;
-		goto exit;
-	}
-	addr.sun_family = AF_UNIX;
-	addr_len = sizeof(struct sockaddr_un);
-
-	do {
-		ret = bind(socket_fd, (struct sockaddr *) &addr, addr_len);
-		if (ret) {
-			RTE_LOG(ERR, PDUMP,
-				"client bind(): %s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			rte_errno = errno;
-			break;
-		}
-
-		serv_len = sizeof(struct sockaddr_un);
-		memset(&serv_addr, 0, sizeof(serv_addr));
-		ret = pdump_get_socket_path(serv_addr.sun_path,
-					sizeof(serv_addr.sun_path),
-					RTE_PDUMP_SOCKET_SERVER);
-		if (ret != 0) {
-			RTE_LOG(ERR, PDUMP,
-				"Failed to get server socket path: %s:%d\n",
-				__func__, __LINE__);
-			rte_errno = errno;
-			break;
-		}
-		serv_addr.sun_family = AF_UNIX;
-
-		n =  sendto(socket_fd, p, sizeof(struct pdump_request), 0,
-				(struct sockaddr *)&serv_addr, serv_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to send to server:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			rte_errno = errno;
-			ret = -1;
-			break;
-		}
-
-		n = recvfrom(socket_fd, &server_resp,
-				sizeof(struct pdump_response), 0,
-				(struct sockaddr *)&from, &serv_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to recv from server:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			rte_errno = errno;
-			ret = -1;
-			break;
-		}
-		ret = server_resp.err_value;
-	} while (0);
-
-exit:
-	close(socket_fd);
-	unlink(addr.sun_path);
-	return ret;
-}
-
-static int
 pdump_validate_ring_mp(struct rte_ring *ring, struct rte_mempool *mp)
 {
 	if (ring == NULL || mp == NULL) {
@@ -768,36 +487,48 @@ pdump_prepare_client_request(char *device, uint16_t queue,
 				struct rte_mempool *mp,
 				void *filter)
 {
-	int ret;
-	struct pdump_request req = {.ver = 1,};
-
-	req.flags = flags;
-	req.op =  operation;
+	int ret = -1;
+	struct rte_mp_msg mp_req, *mp_rep;
+	struct rte_mp_reply mp_reply;
+	struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
+	struct pdump_request *req = (struct pdump_request *)mp_req.param;
+	struct pdump_response *resp;
+
+	req->ver = 1;
+	req->flags = flags;
+	req->op = operation;
 	if ((operation & ENABLE) != 0) {
-		snprintf(req.data.en_v1.device, sizeof(req.data.en_v1.device),
-				"%s", device);
-		req.data.en_v1.queue = queue;
-		req.data.en_v1.ring = ring;
-		req.data.en_v1.mp = mp;
-		req.data.en_v1.filter = filter;
+		snprintf(req->data.en_v1.device,
+			 sizeof(req->data.en_v1.device), "%s", device);
+		req->data.en_v1.queue = queue;
+		req->data.en_v1.ring = ring;
+		req->data.en_v1.mp = mp;
+		req->data.en_v1.filter = filter;
 	} else {
-		snprintf(req.data.dis_v1.device, sizeof(req.data.dis_v1.device),
-				"%s", device);
-		req.data.dis_v1.queue = queue;
-		req.data.dis_v1.ring = NULL;
-		req.data.dis_v1.mp = NULL;
-		req.data.dis_v1.filter = NULL;
+		snprintf(req->data.dis_v1.device,
+			 sizeof(req->data.dis_v1.device), "%s", device);
+		req->data.dis_v1.queue = queue;
+		req->data.dis_v1.ring = NULL;
+		req->data.dis_v1.mp = NULL;
+		req->data.dis_v1.filter = NULL;
+	}
+
+	snprintf(mp_req.name, RTE_MP_MAX_NAME_LEN, PDUMP_MP);
+	mp_req.len_param = sizeof(*req);
+	mp_req.num_fds = 0;
+	if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) == 0) {
+		mp_rep = &mp_reply.msgs[0];
+		resp = (struct pdump_response *)mp_rep->param;
+		rte_errno = resp->err_value;
+		if (!resp->err_value)
+			ret = 0;
+		free(mp_reply.msgs);
 	}
 
-	ret = pdump_create_client_socket(&req);
-	if (ret < 0) {
+	if (ret < 0)
 		RTE_LOG(ERR, PDUMP,
 			"client request for pdump enable/disable failed\n");
-		rte_errno = ret;
-		return -1;
-	}
-
-	return 0;
+	return ret;
 }
 
 int
@@ -884,30 +615,8 @@ rte_pdump_disable_by_deviceid(char *device_id, uint16_t queue,
 }
 
 int
-rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype type)
+rte_pdump_set_socket_dir(const char *path __rte_unused,
+			 enum rte_pdump_socktype type __rte_unused)
 {
-	int ret, count;
-
-	if (path != NULL) {
-		if (type == RTE_PDUMP_SOCKET_SERVER) {
-			count = sizeof(server_socket_dir);
-			ret = snprintf(server_socket_dir, count, "%s", path);
-		} else {
-			count = sizeof(client_socket_dir);
-			ret = snprintf(client_socket_dir, count, "%s", path);
-		}
-
-		if (ret < 0  || ret >= count) {
-			RTE_LOG(ERR, PDUMP,
-					"Invalid socket path:%s:%d\n",
-					__func__, __LINE__);
-			if (type == RTE_PDUMP_SOCKET_SERVER)
-				server_socket_dir[0] = 0;
-			else
-				client_socket_dir[0] = 0;
-			return -EINVAL;
-		}
-	}
-
 	return 0;
 }
diff --git a/lib/librte_pdump/rte_pdump.h b/lib/librte_pdump/rte_pdump.h
index a7e8372..787adb2 100644
--- a/lib/librte_pdump/rte_pdump.h
+++ b/lib/librte_pdump/rte_pdump.h
@@ -163,6 +163,7 @@ rte_pdump_disable_by_deviceid(char *device_id, uint16_t queue,
 				uint32_t flags);
 
 /**
+ * @deprecated
  * Allows applications to set server and client socket paths.
  * If specified path is null default path will be selected, i.e.
  *"/var/run/" for root user and "$HOME" for non root user.
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] pdump: change to use generic multi-process channel

[Pattan, Reshma]

Hi

> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
> Note this patch needs this patch set:
> http://dpdk.org/dev/patchwork/patch/36814/
> v2:
>   - Update doc for deprecation of API, rte_pdump_set_socket_dir,
>     and API change for rte_pdump_init.
>   - Add notice for known incompatibility issue in doc.
>  app/pdump/main.c                       |   6 +-
>  doc/guides/rel_notes/deprecation.rst   |   4 +
>  doc/guides/rel_notes/release_18_05.rst |   7 +
>  lib/librte_pdump/Makefile              |   3 +-
>  lib/librte_pdump/rte_pdump.c           | 423 +++++----------------------------
>  lib/librte_pdump/rte_pdump.h           |   1 +
>  6 files changed, 84 insertions(+), 360 deletions(-)

>diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
> +
> 


> +
> +	/* recv client requests */
> +	if (mp_msg->len_param != sizeof(*cli_req)) {
> +		RTE_LOG(ERR, PDUMP, "failed to recv from client\n");
> +		resp->err_value = EINVAL;

	resp->err_value = -EINVAL

> -	/* save the socket in local configuration */
> -	pdump_socket_fd = socket_fd;
> +	snprintf(mp_resp.name, RTE_MP_MAX_NAME_LEN, PDUMP_MP);
> +	mp_resp.len_param = sizeof(*resp);
> +	mp_resp.num_fds = 0;
> +	if (rte_mp_reply(&mp_resp, peer) < 0)
> +		RTE_LOG(ERR, PDUMP, "failed to send to client:%s, %s:%d\n",
> +			strerror(rte_errno), __func__, __LINE__);
> 

If failed to send the reply should'nt we return -1? 

>  	return 0;
>  }
> 

>  int
> -rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype
> type)
> +rte_pdump_set_socket_dir(const char *path __rte_unused,
> +			 enum rte_pdump_socktype type __rte_unused)
>  {

What about enum rte_pdump_socktype in header file? When to delete them?

We need to update doxygen comments in header file for rte_pdump_init and rte_pdump_uninit()? What do you say.

Thanks,
Reshma

Re: [dpdk-dev] [PATCH v2] pdump: change to use generic multi-process channel

[Pattan, Reshma]

Hi,

> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Wednesday, April 4, 2018 4:08 PM
> To: dev@dpdk.org
> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>
> Subject: [PATCH v2] pdump: change to use generic multi-process channel
> 
> The original code replies on the private channel for primary and secondary
> communication. Change to use the generic multi-process channel.
> 
> Note with this change, dpdk-pdump will be not compatible with old version
> DPDK applications.
> 
> Cc: reshma.pattan@intel.com
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 0c696f7..d55fd05 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -153,3 +153,7 @@ Deprecation Notices
>    be added between the producer and consumer structures. The size of the
>    structure and the offset of the fields will remain the same on
>    platforms with 64B cache line, but will change on other platforms.
> +
> +* pdump: As we changed to use generic IPC, ``rte_pdump_set_socket_dir``
> +will be
> +  deprecated and removed in subsequent release; the parameter, path, of
> +  ``rte_pdump_init`` will also be removed.

Do we need to mention about deprecation of below enums too?
enum rte_pdump_socktype {
        RTE_PDUMP_SOCKET_SERVER = 1,
        RTE_PDUMP_SOCKET_CLIENT = 2
};

Thanks,
Reshma

Re: [dpdk-dev] [PATCH v2] pdump: change to use generic multi-process channel

[Tan, Jianfeng]

On 4/5/2018 6:37 PM, Pattan, Reshma wrote:
> Hi,
>
>> -----Original Message-----
>> From: Tan, Jianfeng
>> Sent: Wednesday, April 4, 2018 4:08 PM
>> To: dev@dpdk.org
>> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; Pattan, Reshma
>> <reshma.pattan@intel.com>
>> Subject: [PATCH v2] pdump: change to use generic multi-process channel
>>
>> The original code replies on the private channel for primary and secondary
>> communication. Change to use the generic multi-process channel.
>>
>> Note with this change, dpdk-pdump will be not compatible with old version
>> DPDK applications.
>>
>> Cc: reshma.pattan@intel.com
>>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>> ---
>> diff --git a/doc/guides/rel_notes/deprecation.rst
>> b/doc/guides/rel_notes/deprecation.rst
>> index 0c696f7..d55fd05 100644
>> --- a/doc/guides/rel_notes/deprecation.rst
>> +++ b/doc/guides/rel_notes/deprecation.rst
>> @@ -153,3 +153,7 @@ Deprecation Notices
>>     be added between the producer and consumer structures. The size of the
>>     structure and the offset of the fields will remain the same on
>>     platforms with 64B cache line, but will change on other platforms.
>> +
>> +* pdump: As we changed to use generic IPC, ``rte_pdump_set_socket_dir``
>> +will be
>> +  deprecated and removed in subsequent release; the parameter, path, of
>> +  ``rte_pdump_init`` will also be removed.
> Do we need to mention about deprecation of below enums too?
> enum rte_pdump_socktype {
>          RTE_PDUMP_SOCKET_SERVER = 1,
>          RTE_PDUMP_SOCKET_CLIENT = 2
> };

Nice catch, will add in next version.

Thanks,
Jianfeng

>
> Thanks,
> Reshma

[dpdk-dev] [PATCH v3] pdump: change to use generic multi-process channel

[Jianfeng Tan]

The original code replies on the private channel for primary and
secondary communication. Change to use the generic multi-process
channel.

Note with this change, dpdk-pdump will be not compatible with
old version DPDK applications.

Cc: reshma.pattan@intel.com

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
v3:
  - Deprecate the enum as suggested by Reshma.
  - Add __rte_deprecate flag for rte_pdump_set_socket_dir.
  - Delete rte_pdump_set_socket_dir in pdump example.
v2:
  - Update doc for deprecation of API, rte_pdump_set_socket_dir,
    and API change for rte_pdump_init.
  - Add notice for known incompatibility issue in doc.

 app/pdump/main.c                       |  22 +-
 doc/guides/rel_notes/deprecation.rst   |   7 +
 doc/guides/rel_notes/release_18_05.rst |   7 +
 lib/librte_pdump/Makefile              |   3 +-
 lib/librte_pdump/rte_pdump.c           | 423 +++++----------------------------
 lib/librte_pdump/rte_pdump.h           |   3 +-
 6 files changed, 88 insertions(+), 377 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index d29de03..2d0879c 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -156,9 +156,11 @@ pdump_usage(const char *prgname)
 			"[mbuf-size=<mbuf data size>default:2176],"
 			"[total-num-mbufs=<number of mbufs>default:65535]'\n"
 			"[--server-socket-path=<server socket dir>"
-				"default:/var/run/.dpdk/ (or) ~/.dpdk/]\n"
+				" which is deprecated and will be removed soon,"
+				" default:/var/run/.dpdk/ (or) ~/.dpdk/]\n"
 			"[--client-socket-path=<client socket dir>"
-				"default:/var/run/.dpdk/ (or) ~/.dpdk/]\n",
+				" which is deprecated and will be removed soon,"
+				" default:/var/run/.dpdk/ (or) ~/.dpdk/]\n",
 			prgname);
 }
 
@@ -744,22 +746,6 @@ enable_pdump(void)
 	struct pdump_tuples *pt;
 	int ret = 0, ret1 = 0;
 
-	if (server_socket_path[0] != 0)
-		ret = rte_pdump_set_socket_dir(server_socket_path,
-				RTE_PDUMP_SOCKET_SERVER);
-	if (ret == 0 && client_socket_path[0] != 0) {
-		ret = rte_pdump_set_socket_dir(client_socket_path,
-				RTE_PDUMP_SOCKET_CLIENT);
-	}
-	if (ret < 0) {
-		cleanup_pdump_resources();
-		rte_exit(EXIT_FAILURE,
-				"failed to set socket paths of server:%s, "
-				"client:%s\n",
-				server_socket_path,
-				client_socket_path);
-	}
-
 	for (i = 0; i < num_tuples; i++) {
 		pt = &pdump_t[i];
 		if (pt->dir == RTE_PDUMP_FLAG_RXTX) {
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index ec70b5f..857450a 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -150,3 +150,10 @@ Deprecation Notices
   be added between the producer and consumer structures. The size of the
   structure and the offset of the fields will remain the same on
   platforms with 64B cache line, but will change on other platforms.
+
+* pdump: As we changed to use generic IPC, some changes in APIs and structure
+  are expected in subsequent release.
+
+  - ``rte_pdump_set_socket_dir`` will be removed;
+  - The parameter, ``path``, of ``rte_pdump_init`` will be removed;
+  - The enum ``rte_pdump_socktype`` will be removed.
diff --git a/doc/guides/rel_notes/release_18_05.rst b/doc/guides/rel_notes/release_18_05.rst
index e5fac1c..966bd36 100644
--- a/doc/guides/rel_notes/release_18_05.rst
+++ b/doc/guides/rel_notes/release_18_05.rst
@@ -114,6 +114,13 @@ Known Issues
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* **pdump is not compatible with old applications.**
+
+  As we changed to use generic multi-process communication for pdump negotiation
+  instead of previous dedicated unix socket way, pdump applications, including
+  dpdk-pdump example and any other applications using librte_pdump, cannot work
+  with older version DPDK primary applications.
+
 
 Shared Library Versions
 -----------------------
diff --git a/lib/librte_pdump/Makefile b/lib/librte_pdump/Makefile
index 98fa752..0ee0fa1 100644
--- a/lib/librte_pdump/Makefile
+++ b/lib/librte_pdump/Makefile
@@ -1,11 +1,12 @@
 # SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2016 Intel Corporation
+# Copyright(c) 2016-2018 Intel Corporation
 
 include $(RTE_SDK)/mk/rte.vars.mk
 
 # library name
 LIB = librte_pdump.a
 
+CFLAGS += -DALLOW_EXPERIMENTAL_API
 CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3
 CFLAGS += -D_GNU_SOURCE
 LDLIBS += -lpthread
diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index ad6efc6..f7cfaec 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -1,16 +1,7 @@
 /* SPDX-License-Identifier: BSD-3-Clause
- * Copyright(c) 2016 Intel Corporation
+ * Copyright(c) 2016-2018 Intel Corporation
  */
 
-#include <sys/socket.h>
-#include <sys/un.h>
-#include <sys/stat.h>
-#include <unistd.h>
-#include <sys/types.h>
-#include <pthread.h>
-#include <stdbool.h>
-#include <stdio.h>
-
 #include <rte_memcpy.h>
 #include <rte_mbuf.h>
 #include <rte_ethdev.h>
@@ -21,16 +12,13 @@
 
 #include "rte_pdump.h"
 
-#define SOCKET_PATH_VAR_RUN "/var/run"
-#define SOCKET_PATH_HOME "HOME"
-#define DPDK_DIR         "/.dpdk"
-#define SOCKET_DIR       "/pdump_sockets"
-#define SERVER_SOCKET "%s/pdump_server_socket"
-#define CLIENT_SOCKET "%s/pdump_client_socket_%d_%u"
 #define DEVICE_ID_SIZE 64
 /* Macros for printing using RTE_LOG */
 #define RTE_LOGTYPE_PDUMP RTE_LOGTYPE_USER1
 
+/* Used for the multi-process communication */
+#define PDUMP_MP	"mp_pdump"
+
 enum pdump_operation {
 	DISABLE = 1,
 	ENABLE = 2
@@ -40,11 +28,6 @@ enum pdump_version {
 	V1 = 1
 };
 
-static pthread_t pdump_thread;
-static int pdump_socket_fd;
-static char server_socket_dir[PATH_MAX];
-static char client_socket_dir[PATH_MAX];
-
 struct pdump_request {
 	uint16_t ver;
 	uint16_t op;
@@ -308,7 +291,7 @@ pdump_register_tx_callbacks(uint16_t end_q, uint16_t port, uint16_t queue,
 }
 
 static int
-set_pdump_rxtx_cbs(struct pdump_request *p)
+set_pdump_rxtx_cbs(const struct pdump_request *p)
 {
 	uint16_t nb_rx_q = 0, nb_tx_q = 0, end_q, queue;
 	uint16_t port;
@@ -392,313 +375,49 @@ set_pdump_rxtx_cbs(struct pdump_request *p)
 	return ret;
 }
 
-/* get socket path (/var/run if root, $HOME otherwise) */
 static int
-pdump_get_socket_path(char *buffer, int bufsz, enum rte_pdump_socktype type)
+pdump_server(const struct rte_mp_msg *mp_msg, const void *peer)
 {
-	char dpdk_dir[PATH_MAX] = {0};
-	char dir[PATH_MAX] = {0};
-	char *dir_home = NULL;
-	int ret = 0;
-
-	if (type == RTE_PDUMP_SOCKET_SERVER && server_socket_dir[0] != 0)
-		strlcpy(dir, server_socket_dir, sizeof(dir));
-	else if (type == RTE_PDUMP_SOCKET_CLIENT && client_socket_dir[0] != 0)
-		strlcpy(dir, client_socket_dir, sizeof(dir));
-	else {
-		if (getuid() != 0) {
-			dir_home = getenv(SOCKET_PATH_HOME);
-			if (!dir_home) {
-				RTE_LOG(ERR, PDUMP,
-					"Failed to get environment variable"
-					" value for %s, %s:%d\n",
-					SOCKET_PATH_HOME, __func__, __LINE__);
-				return -1;
-			}
-			snprintf(dpdk_dir, sizeof(dpdk_dir), "%s%s",
-					dir_home, DPDK_DIR);
-		} else
-			snprintf(dpdk_dir, sizeof(dpdk_dir), "%s%s",
-					SOCKET_PATH_VAR_RUN, DPDK_DIR);
-
-		mkdir(dpdk_dir, 0700);
-		snprintf(dir, sizeof(dir), "%s%s",
-					dpdk_dir, SOCKET_DIR);
-	}
-
-	ret =  mkdir(dir, 0700);
-	/* if user passed socket path is invalid, return immediately */
-	if (ret < 0 && errno != EEXIST) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create dir:%s:%s\n", dir,
-			strerror(errno));
-		rte_errno = errno;
-		return -1;
-	}
-
-	if (type == RTE_PDUMP_SOCKET_SERVER)
-		snprintf(buffer, bufsz, SERVER_SOCKET, dir);
-	else
-		snprintf(buffer, bufsz, CLIENT_SOCKET, dir, getpid(),
-				rte_sys_gettid());
-
-	return 0;
-}
-
-static int
-pdump_create_server_socket(void)
-{
-	int ret, socket_fd;
-	struct sockaddr_un addr;
-	socklen_t addr_len;
-
-	ret = pdump_get_socket_path(addr.sun_path, sizeof(addr.sun_path),
-				RTE_PDUMP_SOCKET_SERVER);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to get server socket path: %s:%d\n",
-			__func__, __LINE__);
-		return -1;
-	}
-	addr.sun_family = AF_UNIX;
-
-	/* remove if file already exists */
-	unlink(addr.sun_path);
-
-	/* set up a server socket */
-	socket_fd = socket(AF_UNIX, SOCK_DGRAM, 0);
-	if (socket_fd < 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create server socket: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		return -1;
-	}
-
-	addr_len = sizeof(struct sockaddr_un);
-	ret = bind(socket_fd, (struct sockaddr *) &addr, addr_len);
-	if (ret) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to bind to server socket: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		close(socket_fd);
-		return -1;
+	struct rte_mp_msg mp_resp;
+	const struct pdump_request *cli_req;
+	struct pdump_response *resp = (struct pdump_response *)&mp_resp.param;
+
+	/* recv client requests */
+	if (mp_msg->len_param != sizeof(*cli_req)) {
+		RTE_LOG(ERR, PDUMP, "failed to recv from client\n");
+		resp->err_value = EINVAL;
+	} else {
+		cli_req = (const struct pdump_request *)mp_msg->param;
+		resp->ver = cli_req->ver;
+		resp->res_op = cli_req->op;
+		resp->err_value = set_pdump_rxtx_cbs(cli_req);
 	}
 
-	/* save the socket in local configuration */
-	pdump_socket_fd = socket_fd;
+	strlcpy(mp_resp.name, PDUMP_MP, RTE_MP_MAX_NAME_LEN);
+	mp_resp.len_param = sizeof(*resp);
+	mp_resp.num_fds = 0;
+	if (rte_mp_reply(&mp_resp, peer) < 0)
+		RTE_LOG(ERR, PDUMP, "failed to send to client:%s, %s:%d\n",
+			strerror(rte_errno), __func__, __LINE__);
 
 	return 0;
 }
 
-static __attribute__((noreturn)) void *
-pdump_thread_main(__rte_unused void *arg)
-{
-	struct sockaddr_un cli_addr;
-	socklen_t cli_len;
-	struct pdump_request cli_req;
-	struct pdump_response resp;
-	int n;
-	int ret = 0;
-
-	/* host thread, never break out */
-	for (;;) {
-		/* recv client requests */
-		cli_len = sizeof(cli_addr);
-		n = recvfrom(pdump_socket_fd, &cli_req,
-				sizeof(struct pdump_request), 0,
-				(struct sockaddr *)&cli_addr, &cli_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to recv from client:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			continue;
-		}
-
-		ret = set_pdump_rxtx_cbs(&cli_req);
-
-		resp.ver = cli_req.ver;
-		resp.res_op = cli_req.op;
-		resp.err_value = ret;
-		n = sendto(pdump_socket_fd, &resp,
-				sizeof(struct pdump_response),
-				0, (struct sockaddr *)&cli_addr, cli_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to send to client:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-		}
-	}
-}
-
 int
-rte_pdump_init(const char *path)
+rte_pdump_init(const char *path __rte_unused)
 {
-	int ret = 0;
-	char thread_name[RTE_MAX_THREAD_NAME_LEN];
-
-	ret = rte_pdump_set_socket_dir(path, RTE_PDUMP_SOCKET_SERVER);
-	if (ret != 0)
-		return -1;
-
-	ret = pdump_create_server_socket();
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create server socket:%s:%d\n",
-			__func__, __LINE__);
-		return -1;
-	}
-
-	/* create the host thread to wait/handle pdump requests */
-	ret = pthread_create(&pdump_thread, NULL, pdump_thread_main, NULL);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create the pdump thread:%s, %s:%d\n",
-			strerror(ret), __func__, __LINE__);
-		return -1;
-	}
-	/* Set thread_name for aid in debugging. */
-	snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "pdump-thread");
-	ret = rte_thread_setname(pdump_thread, thread_name);
-	if (ret != 0) {
-		RTE_LOG(DEBUG, PDUMP,
-			"Failed to set thread name for pdump handling\n");
-	}
-
-	return 0;
+	return rte_mp_action_register(PDUMP_MP, pdump_server);
 }
 
 int
 rte_pdump_uninit(void)
 {
-	int ret;
-
-	ret = pthread_cancel(pdump_thread);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to cancel the pdump thread:%s, %s:%d\n",
-			strerror(ret), __func__, __LINE__);
-		return -1;
-	}
-
-	ret = close(pdump_socket_fd);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to close server socket: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		return -1;
-	}
-
-	struct sockaddr_un addr;
-
-	ret = pdump_get_socket_path(addr.sun_path, sizeof(addr.sun_path),
-				RTE_PDUMP_SOCKET_SERVER);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to get server socket path: %s:%d\n",
-			__func__, __LINE__);
-		return -1;
-	}
-	ret = unlink(addr.sun_path);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to remove server socket addr: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		return -1;
-	}
+	rte_mp_action_unregister(PDUMP_MP);
 
 	return 0;
 }
 
 static int
-pdump_create_client_socket(struct pdump_request *p)
-{
-	int ret, socket_fd;
-	int pid;
-	int n;
-	struct pdump_response server_resp;
-	struct sockaddr_un addr, serv_addr, from;
-	socklen_t addr_len, serv_len;
-
-	pid = getpid();
-
-	socket_fd = socket(AF_UNIX, SOCK_DGRAM, 0);
-	if (socket_fd < 0) {
-		RTE_LOG(ERR, PDUMP,
-			"client socket(): %s:pid(%d):tid(%u), %s:%d\n",
-			strerror(errno), pid, rte_sys_gettid(),
-			__func__, __LINE__);
-		rte_errno = errno;
-		return -1;
-	}
-
-	ret = pdump_get_socket_path(addr.sun_path, sizeof(addr.sun_path),
-				RTE_PDUMP_SOCKET_CLIENT);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to get client socket path: %s:%d\n",
-			__func__, __LINE__);
-		rte_errno = errno;
-		goto exit;
-	}
-	addr.sun_family = AF_UNIX;
-	addr_len = sizeof(struct sockaddr_un);
-
-	do {
-		ret = bind(socket_fd, (struct sockaddr *) &addr, addr_len);
-		if (ret) {
-			RTE_LOG(ERR, PDUMP,
-				"client bind(): %s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			rte_errno = errno;
-			break;
-		}
-
-		serv_len = sizeof(struct sockaddr_un);
-		memset(&serv_addr, 0, sizeof(serv_addr));
-		ret = pdump_get_socket_path(serv_addr.sun_path,
-					sizeof(serv_addr.sun_path),
-					RTE_PDUMP_SOCKET_SERVER);
-		if (ret != 0) {
-			RTE_LOG(ERR, PDUMP,
-				"Failed to get server socket path: %s:%d\n",
-				__func__, __LINE__);
-			rte_errno = errno;
-			break;
-		}
-		serv_addr.sun_family = AF_UNIX;
-
-		n =  sendto(socket_fd, p, sizeof(struct pdump_request), 0,
-				(struct sockaddr *)&serv_addr, serv_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to send to server:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			rte_errno = errno;
-			ret = -1;
-			break;
-		}
-
-		n = recvfrom(socket_fd, &server_resp,
-				sizeof(struct pdump_response), 0,
-				(struct sockaddr *)&from, &serv_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to recv from server:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			rte_errno = errno;
-			ret = -1;
-			break;
-		}
-		ret = server_resp.err_value;
-	} while (0);
-
-exit:
-	close(socket_fd);
-	unlink(addr.sun_path);
-	return ret;
-}
-
-static int
 pdump_validate_ring_mp(struct rte_ring *ring, struct rte_mempool *mp)
 {
 	if (ring == NULL || mp == NULL) {
@@ -769,36 +488,48 @@ pdump_prepare_client_request(char *device, uint16_t queue,
 				struct rte_mempool *mp,
 				void *filter)
 {
-	int ret;
-	struct pdump_request req = {.ver = 1,};
-
-	req.flags = flags;
-	req.op =  operation;
+	int ret = -1;
+	struct rte_mp_msg mp_req, *mp_rep;
+	struct rte_mp_reply mp_reply;
+	struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
+	struct pdump_request *req = (struct pdump_request *)mp_req.param;
+	struct pdump_response *resp;
+
+	req->ver = 1;
+	req->flags = flags;
+	req->op = operation;
 	if ((operation & ENABLE) != 0) {
-		snprintf(req.data.en_v1.device, sizeof(req.data.en_v1.device),
-				"%s", device);
-		req.data.en_v1.queue = queue;
-		req.data.en_v1.ring = ring;
-		req.data.en_v1.mp = mp;
-		req.data.en_v1.filter = filter;
+		snprintf(req->data.en_v1.device,
+			 sizeof(req->data.en_v1.device), "%s", device);
+		req->data.en_v1.queue = queue;
+		req->data.en_v1.ring = ring;
+		req->data.en_v1.mp = mp;
+		req->data.en_v1.filter = filter;
 	} else {
-		snprintf(req.data.dis_v1.device, sizeof(req.data.dis_v1.device),
-				"%s", device);
-		req.data.dis_v1.queue = queue;
-		req.data.dis_v1.ring = NULL;
-		req.data.dis_v1.mp = NULL;
-		req.data.dis_v1.filter = NULL;
+		snprintf(req->data.dis_v1.device,
+			 sizeof(req->data.dis_v1.device), "%s", device);
+		req->data.dis_v1.queue = queue;
+		req->data.dis_v1.ring = NULL;
+		req->data.dis_v1.mp = NULL;
+		req->data.dis_v1.filter = NULL;
+	}
+
+	strlcpy(mp_req.name, PDUMP_MP, RTE_MP_MAX_NAME_LEN);
+	mp_req.len_param = sizeof(*req);
+	mp_req.num_fds = 0;
+	if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) == 0) {
+		mp_rep = &mp_reply.msgs[0];
+		resp = (struct pdump_response *)mp_rep->param;
+		rte_errno = resp->err_value;
+		if (!resp->err_value)
+			ret = 0;
+		free(mp_reply.msgs);
 	}
 
-	ret = pdump_create_client_socket(&req);
-	if (ret < 0) {
+	if (ret < 0)
 		RTE_LOG(ERR, PDUMP,
 			"client request for pdump enable/disable failed\n");
-		rte_errno = ret;
-		return -1;
-	}
-
-	return 0;
+	return ret;
 }
 
 int
@@ -885,30 +616,8 @@ rte_pdump_disable_by_deviceid(char *device_id, uint16_t queue,
 }
 
 int
-rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype type)
+rte_pdump_set_socket_dir(const char *path __rte_unused,
+			 enum rte_pdump_socktype type __rte_unused)
 {
-	int ret, count;
-
-	if (path != NULL) {
-		if (type == RTE_PDUMP_SOCKET_SERVER) {
-			count = sizeof(server_socket_dir);
-			ret = strlcpy(server_socket_dir, path, count);
-		} else {
-			count = sizeof(client_socket_dir);
-			ret = strlcpy(client_socket_dir, path, count);
-		}
-
-		if (ret < 0  || ret >= count) {
-			RTE_LOG(ERR, PDUMP,
-					"Invalid socket path:%s:%d\n",
-					__func__, __LINE__);
-			if (type == RTE_PDUMP_SOCKET_SERVER)
-				server_socket_dir[0] = 0;
-			else
-				client_socket_dir[0] = 0;
-			return -EINVAL;
-		}
-	}
-
 	return 0;
 }
diff --git a/lib/librte_pdump/rte_pdump.h b/lib/librte_pdump/rte_pdump.h
index a7e8372..c72f72f 100644
--- a/lib/librte_pdump/rte_pdump.h
+++ b/lib/librte_pdump/rte_pdump.h
@@ -163,6 +163,7 @@ rte_pdump_disable_by_deviceid(char *device_id, uint16_t queue,
 				uint32_t flags);
 
 /**
+ * @deprecated
  * Allows applications to set server and client socket paths.
  * If specified path is null default path will be selected, i.e.
  *"/var/run/" for root user and "$HOME" for non root user.
@@ -181,7 +182,7 @@ rte_pdump_disable_by_deviceid(char *device_id, uint16_t queue,
  * 0 on success, -EINVAL on error
  *
  */
-int
+__rte_deprecated int
 rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype type);
 
 #ifdef __cplusplus
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] pdump: change to use generic multi-process channel

[Tan, Jianfeng]

Sorry, it seems that I missed this email.


On 4/5/2018 5:45 PM, Pattan, Reshma wrote:
> Hi
>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>> ---
>> Note this patch needs this patch set:
>> http://dpdk.org/dev/patchwork/patch/36814/
>> v2:
>>    - Update doc for deprecation of API, rte_pdump_set_socket_dir,
>>      and API change for rte_pdump_init.
>>    - Add notice for known incompatibility issue in doc.
>>   app/pdump/main.c                       |   6 +-
>>   doc/guides/rel_notes/deprecation.rst   |   4 +
>>   doc/guides/rel_notes/release_18_05.rst |   7 +
>>   lib/librte_pdump/Makefile              |   3 +-
>>   lib/librte_pdump/rte_pdump.c           | 423 +++++----------------------------
>>   lib/librte_pdump/rte_pdump.h           |   1 +
>>   6 files changed, 84 insertions(+), 360 deletions(-)
>> diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
>> +
>>
>
>> +
>> +	/* recv client requests */
>> +	if (mp_msg->len_param != sizeof(*cli_req)) {
>> +		RTE_LOG(ERR, PDUMP, "failed to recv from client\n");
>> +		resp->err_value = EINVAL;
> 	resp->err_value = -EINVAL

OK, this keeps consistent with other places in this file.

>
>> -	/* save the socket in local configuration */
>> -	pdump_socket_fd = socket_fd;
>> +	snprintf(mp_resp.name, RTE_MP_MAX_NAME_LEN, PDUMP_MP);
>> +	mp_resp.len_param = sizeof(*resp);
>> +	mp_resp.num_fds = 0;
>> +	if (rte_mp_reply(&mp_resp, peer) < 0)
>> +		RTE_LOG(ERR, PDUMP, "failed to send to client:%s, %s:%d\n",
>> +			strerror(rte_errno), __func__, __LINE__);
>>
> If failed to send the reply should'nt we return -1?

Yes, will fix it.

>
>>   	return 0;
>>   }
>>
>>   int
>> -rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype
>> type)
>> +rte_pdump_set_socket_dir(const char *path __rte_unused,
>> +			 enum rte_pdump_socktype type __rte_unused)
>>   {
> What about enum rte_pdump_socktype in header file? When to delete them?

Will deprecate it.

>
> We need to update doxygen comments in header file for rte_pdump_init and rte_pdump_uninit()? What do you say.

I'll try to find a flag for deprecating a param. If you know how to do 
that, please let me know.

Thanks,
Jianfeng

>
> Thanks,
> Reshma

[dpdk-dev] [PATCH v4] pdump: change to use generic multi-process channel

[Jianfeng Tan]

The original code replies on the private channel for primary and
secondary communication. Change to use the generic multi-process
channel.

Note with this change, dpdk-pdump will be not compatible with
old version DPDK applications.

Cc: reshma.pattan@intel.com

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
v4:
  - Fix API note in header file.
  - Some other issues (EINVAL, ret, etc).
v3:
  - Deprecate the enum as suggested by Reshma.
  - Add __rte_deprecate flag for rte_pdump_set_socket_dir.
  - Delete rte_pdump_set_socket_dir in pdump example.
v2:
  - Update doc for deprecation of API, rte_pdump_set_socket_dir,
    and API change for rte_pdump_init.
  - Add notice for known incompatibility issue in doc.

 app/pdump/main.c                       |  22 +-
 doc/guides/rel_notes/deprecation.rst   |   7 +
 doc/guides/rel_notes/release_18_05.rst |   7 +
 lib/librte_pdump/Makefile              |   3 +-
 lib/librte_pdump/rte_pdump.c           | 425 ++++++---------------------------
 lib/librte_pdump/rte_pdump.h           |   9 +-
 6 files changed, 93 insertions(+), 380 deletions(-)

diff --git a/app/pdump/main.c b/app/pdump/main.c
index d29de03..2d0879c 100644
--- a/app/pdump/main.c
+++ b/app/pdump/main.c
@@ -156,9 +156,11 @@ pdump_usage(const char *prgname)
 			"[mbuf-size=<mbuf data size>default:2176],"
 			"[total-num-mbufs=<number of mbufs>default:65535]'\n"
 			"[--server-socket-path=<server socket dir>"
-				"default:/var/run/.dpdk/ (or) ~/.dpdk/]\n"
+				" which is deprecated and will be removed soon,"
+				" default:/var/run/.dpdk/ (or) ~/.dpdk/]\n"
 			"[--client-socket-path=<client socket dir>"
-				"default:/var/run/.dpdk/ (or) ~/.dpdk/]\n",
+				" which is deprecated and will be removed soon,"
+				" default:/var/run/.dpdk/ (or) ~/.dpdk/]\n",
 			prgname);
 }
 
@@ -744,22 +746,6 @@ enable_pdump(void)
 	struct pdump_tuples *pt;
 	int ret = 0, ret1 = 0;
 
-	if (server_socket_path[0] != 0)
-		ret = rte_pdump_set_socket_dir(server_socket_path,
-				RTE_PDUMP_SOCKET_SERVER);
-	if (ret == 0 && client_socket_path[0] != 0) {
-		ret = rte_pdump_set_socket_dir(client_socket_path,
-				RTE_PDUMP_SOCKET_CLIENT);
-	}
-	if (ret < 0) {
-		cleanup_pdump_resources();
-		rte_exit(EXIT_FAILURE,
-				"failed to set socket paths of server:%s, "
-				"client:%s\n",
-				server_socket_path,
-				client_socket_path);
-	}
-
 	for (i = 0; i < num_tuples; i++) {
 		pt = &pdump_t[i];
 		if (pt->dir == RTE_PDUMP_FLAG_RXTX) {
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index ec70b5f..857450a 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -150,3 +150,10 @@ Deprecation Notices
   be added between the producer and consumer structures. The size of the
   structure and the offset of the fields will remain the same on
   platforms with 64B cache line, but will change on other platforms.
+
+* pdump: As we changed to use generic IPC, some changes in APIs and structure
+  are expected in subsequent release.
+
+  - ``rte_pdump_set_socket_dir`` will be removed;
+  - The parameter, ``path``, of ``rte_pdump_init`` will be removed;
+  - The enum ``rte_pdump_socktype`` will be removed.
diff --git a/doc/guides/rel_notes/release_18_05.rst b/doc/guides/rel_notes/release_18_05.rst
index e5fac1c..966bd36 100644
--- a/doc/guides/rel_notes/release_18_05.rst
+++ b/doc/guides/rel_notes/release_18_05.rst
@@ -114,6 +114,13 @@ Known Issues
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* **pdump is not compatible with old applications.**
+
+  As we changed to use generic multi-process communication for pdump negotiation
+  instead of previous dedicated unix socket way, pdump applications, including
+  dpdk-pdump example and any other applications using librte_pdump, cannot work
+  with older version DPDK primary applications.
+
 
 Shared Library Versions
 -----------------------
diff --git a/lib/librte_pdump/Makefile b/lib/librte_pdump/Makefile
index 98fa752..0ee0fa1 100644
--- a/lib/librte_pdump/Makefile
+++ b/lib/librte_pdump/Makefile
@@ -1,11 +1,12 @@
 # SPDX-License-Identifier: BSD-3-Clause
-# Copyright(c) 2016 Intel Corporation
+# Copyright(c) 2016-2018 Intel Corporation
 
 include $(RTE_SDK)/mk/rte.vars.mk
 
 # library name
 LIB = librte_pdump.a
 
+CFLAGS += -DALLOW_EXPERIMENTAL_API
 CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3
 CFLAGS += -D_GNU_SOURCE
 LDLIBS += -lpthread
diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index ad6efc6..6c3a885 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -1,16 +1,7 @@
 /* SPDX-License-Identifier: BSD-3-Clause
- * Copyright(c) 2016 Intel Corporation
+ * Copyright(c) 2016-2018 Intel Corporation
  */
 
-#include <sys/socket.h>
-#include <sys/un.h>
-#include <sys/stat.h>
-#include <unistd.h>
-#include <sys/types.h>
-#include <pthread.h>
-#include <stdbool.h>
-#include <stdio.h>
-
 #include <rte_memcpy.h>
 #include <rte_mbuf.h>
 #include <rte_ethdev.h>
@@ -21,16 +12,13 @@
 
 #include "rte_pdump.h"
 
-#define SOCKET_PATH_VAR_RUN "/var/run"
-#define SOCKET_PATH_HOME "HOME"
-#define DPDK_DIR         "/.dpdk"
-#define SOCKET_DIR       "/pdump_sockets"
-#define SERVER_SOCKET "%s/pdump_server_socket"
-#define CLIENT_SOCKET "%s/pdump_client_socket_%d_%u"
 #define DEVICE_ID_SIZE 64
 /* Macros for printing using RTE_LOG */
 #define RTE_LOGTYPE_PDUMP RTE_LOGTYPE_USER1
 
+/* Used for the multi-process communication */
+#define PDUMP_MP	"mp_pdump"
+
 enum pdump_operation {
 	DISABLE = 1,
 	ENABLE = 2
@@ -40,11 +28,6 @@ enum pdump_version {
 	V1 = 1
 };
 
-static pthread_t pdump_thread;
-static int pdump_socket_fd;
-static char server_socket_dir[PATH_MAX];
-static char client_socket_dir[PATH_MAX];
-
 struct pdump_request {
 	uint16_t ver;
 	uint16_t op;
@@ -308,7 +291,7 @@ pdump_register_tx_callbacks(uint16_t end_q, uint16_t port, uint16_t queue,
 }
 
 static int
-set_pdump_rxtx_cbs(struct pdump_request *p)
+set_pdump_rxtx_cbs(const struct pdump_request *p)
 {
 	uint16_t nb_rx_q = 0, nb_tx_q = 0, end_q, queue;
 	uint16_t port;
@@ -392,313 +375,51 @@ set_pdump_rxtx_cbs(struct pdump_request *p)
 	return ret;
 }
 
-/* get socket path (/var/run if root, $HOME otherwise) */
 static int
-pdump_get_socket_path(char *buffer, int bufsz, enum rte_pdump_socktype type)
+pdump_server(const struct rte_mp_msg *mp_msg, const void *peer)
 {
-	char dpdk_dir[PATH_MAX] = {0};
-	char dir[PATH_MAX] = {0};
-	char *dir_home = NULL;
-	int ret = 0;
-
-	if (type == RTE_PDUMP_SOCKET_SERVER && server_socket_dir[0] != 0)
-		strlcpy(dir, server_socket_dir, sizeof(dir));
-	else if (type == RTE_PDUMP_SOCKET_CLIENT && client_socket_dir[0] != 0)
-		strlcpy(dir, client_socket_dir, sizeof(dir));
-	else {
-		if (getuid() != 0) {
-			dir_home = getenv(SOCKET_PATH_HOME);
-			if (!dir_home) {
-				RTE_LOG(ERR, PDUMP,
-					"Failed to get environment variable"
-					" value for %s, %s:%d\n",
-					SOCKET_PATH_HOME, __func__, __LINE__);
-				return -1;
-			}
-			snprintf(dpdk_dir, sizeof(dpdk_dir), "%s%s",
-					dir_home, DPDK_DIR);
-		} else
-			snprintf(dpdk_dir, sizeof(dpdk_dir), "%s%s",
-					SOCKET_PATH_VAR_RUN, DPDK_DIR);
-
-		mkdir(dpdk_dir, 0700);
-		snprintf(dir, sizeof(dir), "%s%s",
-					dpdk_dir, SOCKET_DIR);
-	}
-
-	ret =  mkdir(dir, 0700);
-	/* if user passed socket path is invalid, return immediately */
-	if (ret < 0 && errno != EEXIST) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create dir:%s:%s\n", dir,
-			strerror(errno));
-		rte_errno = errno;
-		return -1;
-	}
-
-	if (type == RTE_PDUMP_SOCKET_SERVER)
-		snprintf(buffer, bufsz, SERVER_SOCKET, dir);
-	else
-		snprintf(buffer, bufsz, CLIENT_SOCKET, dir, getpid(),
-				rte_sys_gettid());
-
-	return 0;
-}
-
-static int
-pdump_create_server_socket(void)
-{
-	int ret, socket_fd;
-	struct sockaddr_un addr;
-	socklen_t addr_len;
-
-	ret = pdump_get_socket_path(addr.sun_path, sizeof(addr.sun_path),
-				RTE_PDUMP_SOCKET_SERVER);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to get server socket path: %s:%d\n",
-			__func__, __LINE__);
-		return -1;
-	}
-	addr.sun_family = AF_UNIX;
-
-	/* remove if file already exists */
-	unlink(addr.sun_path);
-
-	/* set up a server socket */
-	socket_fd = socket(AF_UNIX, SOCK_DGRAM, 0);
-	if (socket_fd < 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create server socket: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		return -1;
-	}
-
-	addr_len = sizeof(struct sockaddr_un);
-	ret = bind(socket_fd, (struct sockaddr *) &addr, addr_len);
-	if (ret) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to bind to server socket: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		close(socket_fd);
+	struct rte_mp_msg mp_resp;
+	const struct pdump_request *cli_req;
+	struct pdump_response *resp = (struct pdump_response *)&mp_resp.param;
+
+	/* recv client requests */
+	if (mp_msg->len_param != sizeof(*cli_req)) {
+		RTE_LOG(ERR, PDUMP, "failed to recv from client\n");
+		resp->err_value = -EINVAL;
+	} else {
+		cli_req = (const struct pdump_request *)mp_msg->param;
+		resp->ver = cli_req->ver;
+		resp->res_op = cli_req->op;
+		resp->err_value = set_pdump_rxtx_cbs(cli_req);
+	}
+
+	strlcpy(mp_resp.name, PDUMP_MP, RTE_MP_MAX_NAME_LEN);
+	mp_resp.len_param = sizeof(*resp);
+	mp_resp.num_fds = 0;
+	if (rte_mp_reply(&mp_resp, peer) < 0) {
+		RTE_LOG(ERR, PDUMP, "failed to send to client:%s, %s:%d\n",
+			strerror(rte_errno), __func__, __LINE__);
 		return -1;
 	}
 
-	/* save the socket in local configuration */
-	pdump_socket_fd = socket_fd;
-
 	return 0;
 }
 
-static __attribute__((noreturn)) void *
-pdump_thread_main(__rte_unused void *arg)
-{
-	struct sockaddr_un cli_addr;
-	socklen_t cli_len;
-	struct pdump_request cli_req;
-	struct pdump_response resp;
-	int n;
-	int ret = 0;
-
-	/* host thread, never break out */
-	for (;;) {
-		/* recv client requests */
-		cli_len = sizeof(cli_addr);
-		n = recvfrom(pdump_socket_fd, &cli_req,
-				sizeof(struct pdump_request), 0,
-				(struct sockaddr *)&cli_addr, &cli_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to recv from client:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			continue;
-		}
-
-		ret = set_pdump_rxtx_cbs(&cli_req);
-
-		resp.ver = cli_req.ver;
-		resp.res_op = cli_req.op;
-		resp.err_value = ret;
-		n = sendto(pdump_socket_fd, &resp,
-				sizeof(struct pdump_response),
-				0, (struct sockaddr *)&cli_addr, cli_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to send to client:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-		}
-	}
-}
-
 int
-rte_pdump_init(const char *path)
+rte_pdump_init(const char *path __rte_unused)
 {
-	int ret = 0;
-	char thread_name[RTE_MAX_THREAD_NAME_LEN];
-
-	ret = rte_pdump_set_socket_dir(path, RTE_PDUMP_SOCKET_SERVER);
-	if (ret != 0)
-		return -1;
-
-	ret = pdump_create_server_socket();
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create server socket:%s:%d\n",
-			__func__, __LINE__);
-		return -1;
-	}
-
-	/* create the host thread to wait/handle pdump requests */
-	ret = pthread_create(&pdump_thread, NULL, pdump_thread_main, NULL);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to create the pdump thread:%s, %s:%d\n",
-			strerror(ret), __func__, __LINE__);
-		return -1;
-	}
-	/* Set thread_name for aid in debugging. */
-	snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "pdump-thread");
-	ret = rte_thread_setname(pdump_thread, thread_name);
-	if (ret != 0) {
-		RTE_LOG(DEBUG, PDUMP,
-			"Failed to set thread name for pdump handling\n");
-	}
-
-	return 0;
+	return rte_mp_action_register(PDUMP_MP, pdump_server);
 }
 
 int
 rte_pdump_uninit(void)
 {
-	int ret;
-
-	ret = pthread_cancel(pdump_thread);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to cancel the pdump thread:%s, %s:%d\n",
-			strerror(ret), __func__, __LINE__);
-		return -1;
-	}
-
-	ret = close(pdump_socket_fd);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to close server socket: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		return -1;
-	}
-
-	struct sockaddr_un addr;
-
-	ret = pdump_get_socket_path(addr.sun_path, sizeof(addr.sun_path),
-				RTE_PDUMP_SOCKET_SERVER);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to get server socket path: %s:%d\n",
-			__func__, __LINE__);
-		return -1;
-	}
-	ret = unlink(addr.sun_path);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to remove server socket addr: %s, %s:%d\n",
-			strerror(errno), __func__, __LINE__);
-		return -1;
-	}
+	rte_mp_action_unregister(PDUMP_MP);
 
 	return 0;
 }
 
 static int
-pdump_create_client_socket(struct pdump_request *p)
-{
-	int ret, socket_fd;
-	int pid;
-	int n;
-	struct pdump_response server_resp;
-	struct sockaddr_un addr, serv_addr, from;
-	socklen_t addr_len, serv_len;
-
-	pid = getpid();
-
-	socket_fd = socket(AF_UNIX, SOCK_DGRAM, 0);
-	if (socket_fd < 0) {
-		RTE_LOG(ERR, PDUMP,
-			"client socket(): %s:pid(%d):tid(%u), %s:%d\n",
-			strerror(errno), pid, rte_sys_gettid(),
-			__func__, __LINE__);
-		rte_errno = errno;
-		return -1;
-	}
-
-	ret = pdump_get_socket_path(addr.sun_path, sizeof(addr.sun_path),
-				RTE_PDUMP_SOCKET_CLIENT);
-	if (ret != 0) {
-		RTE_LOG(ERR, PDUMP,
-			"Failed to get client socket path: %s:%d\n",
-			__func__, __LINE__);
-		rte_errno = errno;
-		goto exit;
-	}
-	addr.sun_family = AF_UNIX;
-	addr_len = sizeof(struct sockaddr_un);
-
-	do {
-		ret = bind(socket_fd, (struct sockaddr *) &addr, addr_len);
-		if (ret) {
-			RTE_LOG(ERR, PDUMP,
-				"client bind(): %s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			rte_errno = errno;
-			break;
-		}
-
-		serv_len = sizeof(struct sockaddr_un);
-		memset(&serv_addr, 0, sizeof(serv_addr));
-		ret = pdump_get_socket_path(serv_addr.sun_path,
-					sizeof(serv_addr.sun_path),
-					RTE_PDUMP_SOCKET_SERVER);
-		if (ret != 0) {
-			RTE_LOG(ERR, PDUMP,
-				"Failed to get server socket path: %s:%d\n",
-				__func__, __LINE__);
-			rte_errno = errno;
-			break;
-		}
-		serv_addr.sun_family = AF_UNIX;
-
-		n =  sendto(socket_fd, p, sizeof(struct pdump_request), 0,
-				(struct sockaddr *)&serv_addr, serv_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to send to server:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			rte_errno = errno;
-			ret = -1;
-			break;
-		}
-
-		n = recvfrom(socket_fd, &server_resp,
-				sizeof(struct pdump_response), 0,
-				(struct sockaddr *)&from, &serv_len);
-		if (n < 0) {
-			RTE_LOG(ERR, PDUMP,
-				"failed to recv from server:%s, %s:%d\n",
-				strerror(errno), __func__, __LINE__);
-			rte_errno = errno;
-			ret = -1;
-			break;
-		}
-		ret = server_resp.err_value;
-	} while (0);
-
-exit:
-	close(socket_fd);
-	unlink(addr.sun_path);
-	return ret;
-}
-
-static int
 pdump_validate_ring_mp(struct rte_ring *ring, struct rte_mempool *mp)
 {
 	if (ring == NULL || mp == NULL) {
@@ -769,36 +490,48 @@ pdump_prepare_client_request(char *device, uint16_t queue,
 				struct rte_mempool *mp,
 				void *filter)
 {
-	int ret;
-	struct pdump_request req = {.ver = 1,};
-
-	req.flags = flags;
-	req.op =  operation;
+	int ret = -1;
+	struct rte_mp_msg mp_req, *mp_rep;
+	struct rte_mp_reply mp_reply;
+	struct timespec ts = {.tv_sec = 5, .tv_nsec = 0};
+	struct pdump_request *req = (struct pdump_request *)mp_req.param;
+	struct pdump_response *resp;
+
+	req->ver = 1;
+	req->flags = flags;
+	req->op = operation;
 	if ((operation & ENABLE) != 0) {
-		snprintf(req.data.en_v1.device, sizeof(req.data.en_v1.device),
-				"%s", device);
-		req.data.en_v1.queue = queue;
-		req.data.en_v1.ring = ring;
-		req.data.en_v1.mp = mp;
-		req.data.en_v1.filter = filter;
+		snprintf(req->data.en_v1.device,
+			 sizeof(req->data.en_v1.device), "%s", device);
+		req->data.en_v1.queue = queue;
+		req->data.en_v1.ring = ring;
+		req->data.en_v1.mp = mp;
+		req->data.en_v1.filter = filter;
 	} else {
-		snprintf(req.data.dis_v1.device, sizeof(req.data.dis_v1.device),
-				"%s", device);
-		req.data.dis_v1.queue = queue;
-		req.data.dis_v1.ring = NULL;
-		req.data.dis_v1.mp = NULL;
-		req.data.dis_v1.filter = NULL;
+		snprintf(req->data.dis_v1.device,
+			 sizeof(req->data.dis_v1.device), "%s", device);
+		req->data.dis_v1.queue = queue;
+		req->data.dis_v1.ring = NULL;
+		req->data.dis_v1.mp = NULL;
+		req->data.dis_v1.filter = NULL;
+	}
+
+	strlcpy(mp_req.name, PDUMP_MP, RTE_MP_MAX_NAME_LEN);
+	mp_req.len_param = sizeof(*req);
+	mp_req.num_fds = 0;
+	if (rte_mp_request_sync(&mp_req, &mp_reply, &ts) == 0) {
+		mp_rep = &mp_reply.msgs[0];
+		resp = (struct pdump_response *)mp_rep->param;
+		rte_errno = resp->err_value;
+		if (!resp->err_value)
+			ret = 0;
+		free(mp_reply.msgs);
 	}
 
-	ret = pdump_create_client_socket(&req);
-	if (ret < 0) {
+	if (ret < 0)
 		RTE_LOG(ERR, PDUMP,
 			"client request for pdump enable/disable failed\n");
-		rte_errno = ret;
-		return -1;
-	}
-
-	return 0;
+	return ret;
 }
 
 int
@@ -885,30 +618,8 @@ rte_pdump_disable_by_deviceid(char *device_id, uint16_t queue,
 }
 
 int
-rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype type)
+rte_pdump_set_socket_dir(const char *path __rte_unused,
+			 enum rte_pdump_socktype type __rte_unused)
 {
-	int ret, count;
-
-	if (path != NULL) {
-		if (type == RTE_PDUMP_SOCKET_SERVER) {
-			count = sizeof(server_socket_dir);
-			ret = strlcpy(server_socket_dir, path, count);
-		} else {
-			count = sizeof(client_socket_dir);
-			ret = strlcpy(client_socket_dir, path, count);
-		}
-
-		if (ret < 0  || ret >= count) {
-			RTE_LOG(ERR, PDUMP,
-					"Invalid socket path:%s:%d\n",
-					__func__, __LINE__);
-			if (type == RTE_PDUMP_SOCKET_SERVER)
-				server_socket_dir[0] = 0;
-			else
-				client_socket_dir[0] = 0;
-			return -EINVAL;
-		}
-	}
-
 	return 0;
 }
diff --git a/lib/librte_pdump/rte_pdump.h b/lib/librte_pdump/rte_pdump.h
index a7e8372..96fa585 100644
--- a/lib/librte_pdump/rte_pdump.h
+++ b/lib/librte_pdump/rte_pdump.h
@@ -37,10 +37,10 @@ enum rte_pdump_socktype {
 /**
  * Initialize packet capturing handling
  *
- * Creates pthread and server socket for handling clients
- * requests to enable/disable rxtx callbacks.
+ * Register the IPC action for communication with target (primary) process.
  *
  * @param path
+ * This parameter is going to be deprecated; it was used for specifiying the
  * directory path for server socket.
  *
  * @return
@@ -52,7 +52,7 @@ rte_pdump_init(const char *path);
 /**
  * Un initialize packet capturing handling
  *
- * Cancels pthread, close server socket, removes server socket address.
+ * Unregister the IPC action for communication with target (primary) process.
  *
  * @return
  *    0 on success, -1 on error
@@ -163,6 +163,7 @@ rte_pdump_disable_by_deviceid(char *device_id, uint16_t queue,
 				uint32_t flags);
 
 /**
+ * @deprecated
  * Allows applications to set server and client socket paths.
  * If specified path is null default path will be selected, i.e.
  *"/var/run/" for root user and "$HOME" for non root user.
@@ -181,7 +182,7 @@ rte_pdump_disable_by_deviceid(char *device_id, uint16_t queue,
  * 0 on success, -EINVAL on error
  *
  */
-int
+__rte_deprecated int
 rte_pdump_set_socket_dir(const char *path, enum rte_pdump_socktype type);
 
 #ifdef __cplusplus
-- 
2.7.4

Re: [dpdk-dev] [PATCH v4] pdump: change to use generic multi-process channel

[Pattan, Reshma]

> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Thursday, April 5, 2018 1:29 PM
> To: dev@dpdk.org
> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>
> Subject: [PATCH v4] pdump: change to use generic multi-process channel
> 
> The original code replies on the private channel for primary and secondary
> communication. Change to use the generic multi-process channel.
> 
> Note with this change, dpdk-pdump will be not compatible with old version
> DPDK applications.
> 
> Cc: reshma.pattan@intel.com
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Acked-by: Reshma Pattan <reshma.pattan@intel.com>

Re: [dpdk-dev] [PATCH v4] pdump: change to use generic multi-process channel

[Thomas Monjalon]

> > The original code replies on the private channel for primary and secondary
> > communication. Change to use the generic multi-process channel.
> > 
> > Note with this change, dpdk-pdump will be not compatible with old version
> > DPDK applications.
> > 
> > Cc: reshma.pattan@intel.com
> > 
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Acked-by: Reshma Pattan <reshma.pattan@intel.com>

Added "allow_experimental_apis = true" in meson.build
and applied, thanks.