From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 0CABB325F for ; Sat, 24 Mar 2018 14:22:15 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Mar 2018 06:22:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,355,1517904000"; d="scan'208";a="210968572" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.252.52.185]) ([10.252.52.185]) by orsmga005.jf.intel.com with ESMTP; 24 Mar 2018 06:22:09 -0700 To: "Tan, Jianfeng" , dev@dpdk.org Cc: konstantin.ananyev@intel.com References: <7f5496e8b5fd43dcbf10fe7059ed832107be0720.1520961844.git.anatoly.burakov@intel.com> <8046b5ca-08d7-50f4-4917-e23950ac5856@intel.com> From: "Burakov, Anatoly" Message-ID: <26e4659f-03a2-f426-7062-4e47e6a090c2@intel.com> Date: Sat, 24 Mar 2018 13:22:08 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <8046b5ca-08d7-50f4-4917-e23950ac5856@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v4] eal: add asynchronous request API to DPDK IPC 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: Sat, 24 Mar 2018 13:22:18 -0000 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