Re: [PATCH v5 2/2] eal: add asynchronous request API to DPDK IPC

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

On 26-Mar-18 3:15 PM, Tan, Jianfeng wrote:
> 
> 
> On 3/24/2018 8:46 PM, Anatoly Burakov wrote:
>> This API is similar to the blocking API that is already present,
>> but reply will be received in a separate callback by the caller
>> (callback specified at the time of request, rather than registering
>> for it in advance).
>>
>> Under the hood, we create a separate thread to deal with replies to
>> asynchronous requests, that will just wait to be notified by the
>> main thread, or woken up on a timer.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Generally, it looks great to me except some trivial nits, so
> 
> Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>

Thanks!

>> +static void
>> +trigger_async_action(struct pending_request *sr)
>> +{
>> +    struct async_request_param *param;
>> +    struct rte_mp_reply *reply;
>> +
>> +    param = sr->async.param;
>> +    reply = &param->user_reply;
>> +
>> +    param->clb(sr->request, reply);
>> +
>> +    /* clean up */
>> +    free(sr->async.param->user_reply.msgs);
> 
> How about simple "free(reply->msgs);"?
> 

I would prefer leaving it as is, as it makes it clear that i'm freeing 
everything to do with sync request.

>> +
>> +    sync_req->type = REQUEST_TYPE_ASYNC;
>> +    strcpy(sync_req->dst, dst);
>> +    sync_req->request = req;
>> +    sync_req->reply = reply_msg;
>> +    sync_req->async.param = param;
>> +
>> +    /* queue already locked by caller */
>> +
>> +    exist = find_sync_request(dst, req->name);
>> +    if (!exist)
>> +        TAILQ_INSERT_TAIL(&pending_requests.requests, sync_req, next);
>> +    if (exist) {
> 
> else?
> 

Will fix in v6

>> @@ -744,9 +1027,155 @@ rte_mp_request(struct rte_mp_msg *req, struct 
>> rte_mp_reply *reply,
>>   }
>>   int __rte_experimental
>> -rte_mp_reply(struct rte_mp_msg *msg, const char *peer)
>> +rte_mp_request_async(struct rte_mp_msg *req, const struct timespec *ts,
>> +        rte_mp_async_reply_t clb)
>>   {
>> +    struct rte_mp_msg *copy;
>> +    struct pending_request *dummy;
>> +    struct async_request_param *param = NULL;
> 
> No need to assign it to NULL.
> 

Will fix in v6.

>> +    /* we have to lock the request queue here, as we will be adding a 
>> bunch
>> +     * of requests to the queue at once, and some of the replies may 
>> arrive
>> +     * before we add all of the requests to the queue.
>> +     */
>> +    pthread_mutex_lock(&pending_requests.lock);
>> +
>> +    /* we have to ensure that callback gets triggered even if we 
>> don't send
>> +     * anything, therefore earlier we have allocated a dummy request. 
>> put it
>> +     * on the queue and fill it. we will remove it once we know we sent
>> +     * something.
>> +     */
> 
> Or we can add this dummy at last if it's necessary, instead of adding 
> firstly and remove if not necessary? No strong option here.
> 

Yep, sure, will fix in v6.

-- 
Thanks,
Anatoly