From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id BE02BA49A for ; Thu, 25 Jan 2018 13:00:15 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jan 2018 04:00:14 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,412,1511856000"; d="scan'208";a="26256268" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.145]) ([10.237.220.145]) by orsmga001.jf.intel.com with ESMTP; 25 Jan 2018 04:00:13 -0800 To: Jianfeng Tan , dev@dpdk.org Cc: bruce.richardson@intel.com, konstantin.ananyev@intel.com, thomas@monjalon.net References: <1512067450-59203-1-git-send-email-jianfeng.tan@intel.com> <1516853783-108023-1-git-send-email-jianfeng.tan@intel.com> <1516853783-108023-3-git-send-email-jianfeng.tan@intel.com> From: "Burakov, Anatoly" Message-ID: <93ea032e-e2da-f087-567a-2397fad7ff02@intel.com> Date: Thu, 25 Jan 2018 12:00:12 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <1516853783-108023-3-git-send-email-jianfeng.tan@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 2/3] eal: add synchronous multi-process communication X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Jan 2018 12:00:16 -0000 On the overall patch, Reviewed-by: Anatoly Burakov 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 , we guarantee that only one such request > is on the fly. > > Suggested-by: Anatoly Burakov > Suggested-by: Konstantin Ananyev > Signed-off-by: Jianfeng Tan > --- > 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 > #include > #include > +#include > #include > #include > #include > @@ -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); > > + 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; > + > /** > * @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