From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Subject: Re: [PATCH v6 1/2] eal: add channel for multi-process
	communication
Date: Fri, 26 Jan 2018 10:25:45 +0000
Message-ID: <e87c69d0-4d03-f500-26d5-55b06b78f307@intel.com>
References: <1512067450-59203-1-git-send-email-jianfeng.tan@intel.com>
 <1516938082-27852-1-git-send-email-jianfeng.tan@intel.com>
 <1516938082-27852-2-git-send-email-jianfeng.tan@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: bruce.richardson@intel.com, konstantin.ananyev@intel.com,
 thomas@monjalon.net
To: Jianfeng Tan <jianfeng.tan@intel.com>, dev@dpdk.org
Return-path: <dev-bounces@dpdk.org>
Received: from mga07.intel.com (mga07.intel.com [134.134.136.100])
 by dpdk.org (Postfix) with ESMTP id E0CE91B650
 for <dev@dpdk.org>; Fri, 26 Jan 2018 11:25:49 +0100 (CET)
In-Reply-To: <1516938082-27852-2-git-send-email-jianfeng.tan@intel.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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