Re: [dpdk-dev] [PATCH v3 1/3] eal: add channel for multi-process communication

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

Overall on this patch:

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

There are a few nitpicks below in comments.

Also, as a general note, i would prefer for sendmsg API's to return 0 on 
success and -1 on failure, as number of sent messages is not only 
meaningless to the user (since there's no way to tell if the value 
returned is the value we expected), but also makes the API unintuitive 
and prone to usage errors when using common "if (sendmsg()) {// error}" 
idiom. However, i'm fine with leaving it as is, if everyone else is. 
It's an experimental API, so we can change it later if need be.

On 25-Jan-18 4:16 AM, Jianfeng Tan wrote:
> Previouly, there are three channels for multi-process
> (i.e., primary/secondary) communication.
>    1. Config-file based channel, in which, the primary process writes
>       info into a pre-defined config file, and the secondary process
>       reads the info out.
>    2. vfio submodule has its own channel based on unix socket for the
>       secondary process to get container fd and group fd from the
>       primary process.
>    3. pdump submodule also has its own channel based on unix socket for
>       packet dump.
> 
> It'd be good to have a generic communication channel for multi-process
> communication to accomodate the requirements including:
>    a. Secondary wants to send info to primary, for example, secondary
>       would like to send request (about some specific vdev to primary).
>    b. Sending info at any time, instead of just initialization time.
>    c. Share FDs with the other side, for vdev like vhost, related FDs
>       (memory region, kick) should be shared.
>    d. A send message request needs the other side to response immediately.
> 
> This patch proposes to create a communication channel, based on datagram
> unix socket, for above requirements. Each process will block on a unix
> socket waiting for messages from the peers.
> 
> Three new APIs are added:
> 
>    1. rte_eal_mp_action_register() is used to register an action,
>       indexed by a string, when a component at receiver side would like
>       to response the messages from the peer processe.
>    2. rte_eal_mp_action_unregister() is used to unregister the action
>       if the calling component does not want to response the messages.
>    3. rte_eal_mp_sendmsg() is used to send a message, and returns
>       immediately. If there are n secondary processes, the primary
>       process will send n messages.
> 
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>   lib/librte_eal/common/eal_common_proc.c | 390 +++++++++++++++++++++++++++++++-
>   lib/librte_eal/common/eal_filesystem.h  |  17 ++
>   lib/librte_eal/common/eal_private.h     |  10 +
>   lib/librte_eal/common/include/rte_eal.h |  75 ++++++
>   lib/librte_eal/linuxapp/eal/eal.c       |   8 +
>   lib/librte_eal/rte_eal_version.map      |   3 +
>   6 files changed, 502 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
> index 40fa982..baeb7d1 100644
> --- a/lib/librte_eal/common/eal_common_proc.c
> +++ b/lib/librte_eal/common/eal_common_proc.c
> @@ -2,14 +2,48 @@
>    * Copyright(c) 2016 Intel Corporation

Nitpicking - making substantial changes to this files should probably 
update copyright year (2016-2018?).

>    */
>   
> -#include <stdio.h>
> +#include <dirent.h>
> +#include <errno.h>
>   #include <fcntl.h>
> +#include <fnmatch.h>
> +#include <libgen.h>
> +#include <limits.h>
> +#include <pthread.h>
> +#include <stdio.h>
>   #include <stdlib.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <unistd.h>
> +

<snip>

> +int
> +rte_eal_mp_channel_init(void)
> +{
> +	char thread_name[RTE_MAX_THREAD_NAME_LEN];
> +	char *path;
> +	pthread_t tid;
> +
> +	snprintf(mp_filter, PATH_MAX, ".%s_unix_*",
> +		 internal_config.hugefile_prefix);
> +
> +	path = strdup(eal_mp_socket_path());
> +	snprintf(mp_dir_path, PATH_MAX, "%s", dirname(path));
> +	free(path);
> +
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +		unlink_sockets();
> +
> +	if (open_socket_fd() < 0)
> +		return -1;
> +
> +	snprintf(thread_name, RTE_MAX_THREAD_NAME_LEN, "rte_mp_handle");
> +
> +	if (pthread_create(&tid, NULL, mp_handle, NULL) == 0) {
> +		/* try best to set thread name */
> +		rte_thread_setname(tid, thread_name);
> +		return 0;
> +	}
> +
> +	RTE_LOG(ERR, EAL, "failed to create mp thead: %s\n", strerror(errno));
> +	close(mp_fd);
> +	mp_fd = -1;
> +	return -1;

Nitpicking: looks weird, usually early exit is for failures, not 
success. Maybe move the error part under (pthread_create() != 0).

> +}
> +
> +static int
> +send_msg(const char *dst_path, struct rte_mp_msg *msg)
> +{
> +	int snd;
> +	struct iovec iov;
> +	struct msghdr msgh;
> +	struct cmsghdr *cmsg;
> +	struct sockaddr_un dst;
> +	int fd_size = msg->num_fds * sizeof(int);
> +	char control[CMSG_SPACE(fd_size)];
> +
> +	memset(&dst, 0, sizeof(dst));

-- 
Thanks,
Anatoly