Re: [dpdk-dev] [PATCH v4] eal: add asynchronous request API to DPDK IPC

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

A few of my yesterday's replies made no sense... Lesson learned: don't 
reply to code review comments on a late friday evening :)

On 23-Mar-18 6:21 PM, Burakov, Anatoly wrote:
> On 23-Mar-18 3:38 PM, Tan, Jianfeng wrote:
> 
> We do. However, we have to wait for *something* if there aren't any 
> asynchronous requests pending. There isn't a way to put "wait infinite 
> amount" as a time value, so i opted for next best thing - large enough 
> to not cause any performance issues. The timeout is arbitrary.
> 

Didn't realize we were holding the lock, so we could choose between wait 
and timed wait. Fixed in v5.

>>>   /** Double linked list of actions. */
>>> @@ -60,13 +65,37 @@ struct mp_msg_internal {
>>>       struct rte_mp_msg msg;
>>>   };
>>> +enum mp_request_type {
>>> +    REQUEST_TYPE_SYNC,
>>> +    REQUEST_TYPE_ASYNC
>>> +};
>>> +
>>> +struct async_request_shared_param {
>>> +    struct rte_mp_reply *user_reply;
>>> +    struct timespec *end;
>>
>> Why we have to make these two as pointers? Operating on pointers 
>> introduce unnecessary complexity.
> 
> Because those are shared between different pending requests. Each 
> pending request gets its own entry in the queue (because it expects 
> answer from a particular process), but the request data (callback, 
> number of requests processed, etc.) is shared between all requests for 
> this sync operation. We don't have the luxury of storing all of that in 
> a local variable like we do with synchronous requests :)

Missed the fact that you weren't referring to the need of storing these 
in shared_param but rather to the fact that i was storing 
malloc-allocated values that are shared, as pointers in shared param 
structure, when i could just as easily store actual structs there. Fixed 
in v5.


>> Too many structs are defined? How about just putting it like this:
>>
>> struct pending_request {
>>          TAILQ_ENTRY(sync_request) next;
>>          enum mp_request_type type;
>>          char dst[PATH_MAX];
>>          struct rte_mp_msg *request;
>>          struct rte_mp_msg *reply_msg;
>>          int reply_received;
>>          RTE_STD_C11
>>          union {
>>                  /* for sync request */
>>                  struct {
>>                          pthread_cond_t cond; /* used for mp thread to 
>> wake up requesting thread */
>>                  };
>>
>>                  /* for async request */
>>                  struct {
>>                          struct rte_mp_reply user_reply;
>>                          struct timespec end;
>>                          int n_requests_processed; /* store how 
>> requests */
>>                  };
>>          };
>> };
> 
> That can work, sure. However, i actually think that my approach is 
> clearer, because when you're working with autocomplete and a proper IDE, 
> it's clear which values are for which case (and i would argue it makes 
> the code more readable as well).

Again, didn't realize that you were referring to defining all of the 
structs and values outside of pending request, rather than storing them 
as anonymous structs (which would indeed cause problems with 
autocomplete and readability). Fixed in v5.

Thanks again for your review!

-- 
Thanks,
Anatoly