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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Thursday, January 25, 2018 12:00 PM
> To: Tan, Jianfeng <jianfeng.tan@intel.com>; dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; thomas@monjalon.net
> Subject: Re: [PATCH v3 2/3] eal: add synchronous multi-process communication
> 
> 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.

Yes, I also think it is good to return number of sends.
Then caller can compare number of sended requests with number of
received replies and decide should it be considered a failure or no.

> > +		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.


As I understand - because we receive a number of sended requests.
Number of received replies  will be available in reply->nb_msgs.
Same below.
Konstantin

> 
> > +	}
> > +
> > +	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