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

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

On 26-Jan-18 3:41 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 accommodate 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>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---

<snip>

> +
> +static int
> +mp_send(struct rte_mp_msg *msg)
> +{
> +	int ret = 0;
> +	DIR *mp_dir;
> +	struct dirent *ent;
> +
> +	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> +		if (send_msg(eal_mp_socket_path(), msg) < 0)
> +			return -1;
> +		else
> +			return 0;
> +	}
> +
> +	/* broadcast to all secondary processes */
> +	mp_dir = opendir(mp_dir_path);
> +	if (!mp_dir) {
> +		RTE_LOG(ERR, EAL, "Unable to open directory %s\n",
> +				mp_dir_path);
> +		rte_errno = errno;
> +		return -1;
> +	}
> +	while ((ent = readdir(mp_dir))) {
> +		if (fnmatch(mp_filter, ent->d_name, 0) != 0)
> +			continue;
> +
> +		if (send_msg(ent->d_name, msg) < 0)
> +			ret = -1;
> +	}
> +	closedir(mp_dir);
> +
> +	return ret;

Nitpick: you probably don't need ret here, just return 0 as in other places.

> +}
> +
> +static bool
> +check_input(const struct rte_mp_msg *msg)
> +{
> +	if (msg == NULL) {
> +		RTE_LOG(ERR, EAL, "Msg cannot be NULL\n");
> +		rte_errno = -EINVAL;
> +		return false;
> +	}
> +
> +	if (validate_action_name(msg->name))
> +		return false;
> +
> +	if (msg->len_param > RTE_MP_MAX_PARAM_LEN) {
> +		RTE_LOG(ERR, EAL, "Message data is too long\n");
> +		rte_errno = -E2BIG;
> +		return false;
> +	}
> +
> +	if (msg->num_fds > RTE_MP_MAX_FD_NUM) {
> +		RTE_LOG(ERR, EAL, "Cannot send more than %d FDs\n",
> +			RTE_MP_MAX_FD_NUM);
> +		rte_errno = -E2BIG;
> +		return false;

Otherwise, i'm happy with this patch.

-- 
Thanks,
Anatoly