On the overall patch,
Reviewed-by: Anatoly Burakov <anatoly.bura...@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.bura...@intel.com>
Suggested-by: Konstantin Ananyev <konstantin.anan...@intel.com>
Signed-off-by: Jianfeng Tan <jianfeng....@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
