Re: [dpdk-dev] [PATCH v3 2/3] eal: add synchronous multi-process communication

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

On the overall patch,

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

For request(), returning number of replies received actually makes 
sense, because now we get use the value to read our replies, if we were 
a primary process sending messages to secondary processes.

Few comments below.

On 25-Jan-18 4:16 AM, Jianfeng Tan wrote:
> We need the synchronous way for multi-process communication,
> i.e., blockingly waiting for reply message when we send a request
> to the peer process.
> 
> We add two APIs rte_eal_mp_request() and rte_eal_mp_reply() for
> such use case. By invoking rte_eal_mp_request(), a request message
> is sent out, and then it waits there for a reply message. The caller
> can specify the timeout. And the response messages will be collected
> and returned so that the caller can decide how to translate them.
> 
> The API rte_eal_mp_reply() is always called by an mp action handler.
> Here we add another parameter for rte_eal_mp_t so that the action
> handler knows which peer address to reply.
> 
>         sender-process                receiver-process
>     ----------------------            ----------------
> 
>      thread-n
>       |_rte_eal_mp_request() ----------> mp-thread
>          |_timedwait()                    |_process_msg()
>                                             |_action()
>                                                 |_rte_eal_mp_reply()
> 	        mp_thread  <---------------------|
>                    |_process_msg()
>                       |_signal(send_thread)
>      thread-m <----------|
>       |_collect-reply
> 
>   * A secondary process is only allowed to talk to the primary process.
>   * If there are multiple secondary processes for the primary proces,
>     it will send request to peer1, collect response from peer1; then
>     send request to peer2, collect reponse from peer2, and so on.
>   * When thread-n is sending request, thread-m of that process can send
>     request at the same time.
>   * For pair <action_name, peer>, we guarantee that only one such request
>     is on the fly.
> 
> Suggested-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>   doc/guides/rel_notes/release_18_02.rst  |  15 ++
>   lib/librte_eal/common/eal_common_proc.c | 237 +++++++++++++++++++++++++++++---
>   lib/librte_eal/common/include/rte_eal.h |  58 +++++++-
>   lib/librte_eal/rte_eal_version.map      |   3 +
>   4 files changed, 295 insertions(+), 18 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_18_02.rst b/doc/guides/rel_notes/release_18_02.rst
> index 00b3224..f6ed666 100644
> --- a/doc/guides/rel_notes/release_18_02.rst
> +++ b/doc/guides/rel_notes/release_18_02.rst
> @@ -151,6 +151,21 @@ New Features
>     renamed the application from SW PMD specific ``eventdev_pipeline_sw_pmd``
>     to PMD agnostic ``eventdev_pipeline``.
>   
> +* **Added new multi-process communication channel**
> +
> +  Added a generic channel in EAL for multi-process (primary/secondary) synchronous
> +  and asynchronous communication. Each component who wants to reponse a message
> +  shall register the action; and each process has a thread to receive the message
> +  and invokes the registered action. The list of new APIs:
> +
> +  * ``rte_eal_mp_register``
> +  * ``rte_eal_mp_unregister``
> +  * ``rte_eal_mp_sendmsg``
> +  * ``rte_eal_mp_request``
> +  * ``rte_eal_mp_reply``
> +
> +  Note as we changed to use the new channel for communication, applications cannot
> +  talk with old version through the old (private) communication channel.

Some of this should've probably been added into previous patch.

>   
>   API Changes
>   -----------
> diff --git a/lib/librte_eal/common/eal_common_proc.c b/lib/librte_eal/common/eal_common_proc.c
> index baeb7d1..69df943 100644
> --- a/lib/librte_eal/common/eal_common_proc.c
> +++ b/lib/librte_eal/common/eal_common_proc.c
> @@ -12,6 +12,7 @@
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <string.h>
> +#include <sys/time.h>
>   #include <sys/types.h>
>   #include <sys/socket.h>
>   #include <sys/un.h>
> @@ -44,6 +45,50 @@ TAILQ_HEAD(action_entry_list, action_entry);
>   static struct action_entry_list action_entry_list =
>   	TAILQ_HEAD_INITIALIZER(action_entry_list);
>   

<snip>

> +		return 0;
> +	}
> +
> +	if (send_msg(dst, req, MP_REQ) != 1) {
> +		RTE_LOG(ERR, EAL, "Fail to send request %s:%s\n",
> +			dst, req->name);
> +		return 0;
> +	}
> +
> +	pthread_mutex_lock(&sync_requests.lock);
> +	do {
> +		pthread_cond_timedwait(&sync_req.cond, &sync_requests.lock, ts);
> +		/* Check spurious wakeups */
> +		if (sync_req.reply_received == 1)
> +			break;
> +		/* Check if time is out */
> +		if (gettimeofday(&now, NULL) < 0)
> +			break;
> +		if (now.tv_sec < ts->tv_sec)
> +			break;
> +		else if (now.tv_sec == ts->tv_sec &&
> +			 now.tv_usec * 1000 < ts->tv_nsec)
> +			break;
> +	} while (1);
> +	/* We got the lock now */
> +	TAILQ_REMOVE(&sync_requests.requests, &sync_req, next);
> +	pthread_mutex_unlock(&sync_requests.lock);
> +
> +	if (sync_req.reply_received == 0) {
> +		RTE_LOG(ERR, EAL, "Fail to recv reply for request %s:%s\n",
> +			dst, req->name);
> +		return 1;

Why are we returning 1 here? There was no reply, so no reply structure 
was allocated. This looks like a potential buffer overflow on trying to 
read replies if one of them wasn't delivered.

> +	}
> +
> +	tmp = realloc(reply->msgs, sizeof(msg) * (reply->nb_msgs + 1));
> +	if (!tmp) {
> +		RTE_LOG(ERR, EAL, "Fail to alloc reply for request %s:%s\n",
> +			dst, req->name);
> +		return 1;
> +	}

Same here - we couldn't allocate a reply, so it won't get to the user. 
Why return 1 here?

> +	memcpy(&tmp[reply->nb_msgs], &msg, sizeof(msg));
> +	reply->msgs = tmp;
> +	reply->nb_msgs++;
> +	return 1;
> +}
> +
> +int
> +rte_eal_mp_request(struct rte_mp_msg *req,
> +		   struct rte_mp_reply *reply,
> +		   const struct timespec *ts)
> +{
> +	DIR *mp_dir;
> +	struct dirent *ent;
> +	int nb_snds = 0;
> +	struct timeval now;
> +	struct timespec end;
> +

<snip>

>   /**
>    * @warning
> @@ -262,6 +268,56 @@ void rte_eal_mp_action_unregister(const char *name);
>   int rte_eal_mp_sendmsg(struct rte_mp_msg *msg);
>   
>   /**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Send a request to the peer process and expect a reply.
> + *
> + * This function sends a request message to the peer process, and will
> + * block until receiving reply message from the peer process.
> + *
> + * @note The caller is responsible to free reply->replies.
> + *
> + * @param req
> + *   The req argument contains the customized request message.
> + *
> + * @param reply
> + *   The reply argument will be for storing all the replied messages;
> + *   the caller is responsible for free reply->replies.
> + *
> + * @param ts
> + *   The ts argument specifies how long we can wait for the peer(s) to reply.
> + *
> + * @return
> + *  - (<0) on invalid parameters;
> + *  - (>=0) as the number of messages being sent successfully.
> + */
> +int rte_eal_mp_request(struct rte_mp_msg *req,
> +			struct rte_mp_reply *reply, const struct timespec *ts);

See above: it would be much more useful to return number of replies 
received, rather than number of messages sent, as that's the number we 
are most interested in. Otherwise, if we e.g. sent 5 messages but 
received 1 reply, you're essentially not telling the user how far can he 
index the reply pointer.

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Send a reply to the peer process.
> + *
> + * This function will send a reply message in response to a request message
> + * received previously.
> + *
> + * @param msg
> + *   The msg argument contains the customized message.
> + *
> + * @param peer
> + *   The peer argument is the pointer to the peer socket path.
> + *
> + * @return
> + *  - (1) on success;
> + *  - (0) on failure;
> + *  - (<0) on invalid parameters.
> + */
> +int rte_eal_mp_reply(struct rte_mp_msg *msg, const char *peer);

I don't think there's much point in making distinction between invalid 
parameters and failure.

> +
> +/**
>    * Usage function typedef used by the application usage function.
>    *
>    * Use this function typedef to define and call rte_set_application_usage_hook()
> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index adeadfb..3015bc6 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -220,6 +220,9 @@ EXPERIMENTAL {
>   	rte_eal_mp_action_register;
>   	rte_eal_mp_action_unregister;
>   	rte_eal_mp_sendmsg;
> +	rte_eal_mp_request;
> +	rte_eal_mp_reply;
> +	rte_eal_mp_sendmsg;

You're adding rte_eal_mp_sendmsg twice.

>   	rte_service_attr_get;
>   	rte_service_attr_reset_all;
>   	rte_service_component_register;
> 


-- 
Thanks,
Anatoly