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

["Tan, Jianfeng" <jianfeng.tan@intel.com>]

On 1/16/2018 7:12 PM, Ananyev, Konstantin wrote:
> Hi Jianfeng,
>
>> -----Original Message-----
>> From: Tan, Jianfeng
>> Sent: Tuesday, January 16, 2018 8:11 AM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org; Burakov, Anatoly <anatoly.burakov@intel.com>
>> Cc: Richardson, Bruce <bruce.richardson@intel.com>; thomas@monjalon.net
>> Subject: Re: [PATCH v2 3/4] eal: add synchronous multi-process communication
>>
>> Thank you, Konstantin and Anatoly firstly. Other comments are well
>> received and I'll send out a new version.
>>
>>
>> On 1/16/2018 8:00 AM, Ananyev, Konstantin 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
>>>> timeout is hard-coded 5 Sec. And the replied message will be copied
>>>> in the parameters of this API so that the caller can decide how
>>>> to translate those information (including params and fds). Note
>>>> if a primary process owns multiple secondary processes, this API
>>>> will fail.
>>>>
>>>> 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.
>>>>
>>>> We use mutex in rte_eal_mp_request() to guarantee that only one
>>>> request is on the fly for one pair of processes.
>>> You don't need to do things in such strange and restrictive way.
>>> Instead you can do something like that:
>>> 1) Introduce new struct, list for it and mutex
>>>    struct sync_request {
>>>         int reply_received;
>>>         char dst[PATH_MAX];
>>>         char reply[...];
>>>         LIST_ENTRY(sync_request) next;
>>> };
>>>
>>> static struct
>>>       LIST_HEAD(list, sync_request);
>>>       pthread_mutex_t lock;
>>>      pthead_cond_t cond;
>>> } sync_requests;
>>>
>>> 2) then at request() call:
>>>     Grab sync_requests.lock
>>>     Check do we already have a pending request for that destination,
>>>     If yes - the release the lock and returns with error.
>>>     - allocate and init new sync_request struct, set reply_received=0
>>>     - do send_msg()
>>>     -then in a cycle:
>>>     pthread_cond_timed_wait(&sync_requests.cond, &sync_request.lock, &timespec);
>>>     - at return from it check if sync_request.reply_received == 1, if not
>>> check if timeout expired and either return a failure or go to the start of the cycle.
>>>
>>> 3) at mp_handler() if REPLY received - grab sync_request.lock,
>>>       search through sync_requests.list for dst[] ,
>>>      if found, then set it's reply_received=1, copy the received message into reply
>>>      and call pthread_cond_braodcast((&sync_requests.cond);
>> The only benefit I can see is that now the sender can request to
>> multiple receivers at the same time. And it makes things more
>> complicated. Do we really need this?
> The benefit is that one thread is blocked waiting for response,
> your mp_handler can still receive and handle other messages.

This can already be done in the original implementation. mp_handler 
listens for msg, request from the other peer(s), and replies the 
requests, which is not affected.

> Plus as you said - other threads can keep sending messages.

For this one, in the original implementation, other threads can still 
send msg, but not request. I suppose the request is not in a fast path, 
why we care to make it fast?

Thanks,
Jianfeng

> Konstantin
>
>> Thanks,
>> Jianfeng