From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id B5DB229CB for ; Wed, 28 Mar 2018 12:42:27 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Mar 2018 03:42:24 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,371,1517904000"; d="scan'208";a="38798780" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.112]) ([10.237.220.112]) by orsmga003.jf.intel.com with ESMTP; 28 Mar 2018 03:42:23 -0700 To: Thomas Monjalon , "Tan, Jianfeng" Cc: "Van Haaren, Harry" , "dev@dpdk.org" , "Ananyev, Konstantin" References: <944ebaf4-6dc3-5069-1d7e-2ee7bbcd8adc@intel.com> <9c6ae3bf-6f45-8148-40c0-757c02fe2102@intel.com> <3288381.S1gyJAqTvN@xps> From: "Burakov, Anatoly" Message-ID: <5a7e8c12-9336-678a-ccdb-4938bb64e9c2@intel.com> Date: Wed, 28 Mar 2018 11:42:23 +0100 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: <3288381.S1gyJAqTvN@xps> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 2/2] 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: Wed, 28 Mar 2018 10:42:29 -0000 On 28-Mar-18 10:53 AM, Thomas Monjalon wrote: > 28/03/2018 11:21, Burakov, Anatoly: >> On 28-Mar-18 9:55 AM, Tan, Jianfeng wrote: >>> On 3/28/2018 4:22 PM, Van Haaren, Harry wrote: >>>> From: Thomas Monjalon [mailto:thomas@monjalon.net] >>>>> 28/03/2018 04:08, Tan, Jianfeng: >>>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net] >>>>>>> 27/03/2018 15:59, Anatoly Burakov: >>>>>>>> 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. >>>>>>> >>>>>>> I really don't like that a library is creating a thread. >>>>>>> We don't even know where the thread is created (which core). >>>>>>> Can it be a rte_service? or in the interrupt thread? >>>>>> >>>>>> Agree that we'd better not adding so many threads in a library. >>>>>> >>>>>> I was considering to merge all the threads into the interrupt thread, >>>>> however, we don't have an interrupt thread in freebsd. Further, we don't >>>>> implement alarm API in freebsd. That's why I tend to current >>>>> implementation, >>>>> and optimize it later. >>>>> >>>>> I would prefer we improve the current code now instead of polluting more >>>>> with more uncontrolled threads. >>>>> >>>>>> For rte_service, it may be not a good idea to reply on it as it needs >>>>> explicit API calls to setup. >>>>> >>>>> I don't see the issue of the explicit API. >>>>> The IPC is a new service. >>> >>> My concern is that not every DPDK application sets up rte_service, but >>> IPC will be used for very fundamental functions, like memory allocation. >>> We could not possibly ask all DPDK applications to add rte_service now. >>> >>> And also take Harry's comments below into consideration, most likely, we >>> will move these threads into interrupt thread now by adding >>> >>>> Although I do like to see new services, if we want to enable "core" >>>> dpdk functionality with Services, we need a proper designed solution >>>> for that. Service cores is not intended for "occasional" work - there >>>> is no method to block and sleep on a specific service until work >>>> becomes available, so this would imply a busy-polling. Using a service >>>> (hence busy polling) for rte_malloc()-based memory mapping requests is >>>> inefficient, and total overkill :) >>>> >>>> For this patch I suggest to use some blocking-read capable mechanism. >>> >>> The problem here is that we add too many threads; blocking-read does not >>> decrease # of threads. >>> >>>> >>>> The above said, in the longer term it would be good to have a design >>>> that allows new file-descriptors to be added to a "dpdk core" thread, >>>> which performs occasional lengthy work if the FD has data available. >>> >>> Interrupt thread vs rte_service, which is the direction to go? We >>> actually have some others threads, in vhost and even virtio-user; we can >>> also avoid those threads if we have a clear direction. >>> >>> Thanks, >>> Jianfeng >>> >> >> Hi all, >> >> First of all, @Thomas, this is not a "new library" - it's part of EAL. > > I did not say it is a new library. > >> We're going to be removing a few threads from EAL as it is because of >> IPC (Jianfeng has already submitted patches for those), > > I don't understand. > Which threads are you going to remove? Which patch? There are separate patches that get rid of some EAL threads we have from before (e.g. VFIO thread) - this is the reason IPC was created in the first place. These aren't relevant to this patchset per se, i'm just saying that it's not like we're adding to the pile of already existing threads without taking anything away :) > >> so i don't think >> it's such a big deal to have two IPC threads instead of one. I'm open to >> suggestions on how to make this work without a second thread, but i >> don't see it. > > I am not against the second thread. > I am against both threads :) Well, the first thread is already in DPDK. To provide some context, first implementation for DPDK IPC was suggested for 17.11, and (without many conceptual changes) was merged in 18.02. I think it's a bit late to be against both threads :) > >> We've discussed possibility of using rte_service internally, but decided >> against it for reasons already outlined by Harry - it's not a suitable >> mechanism for this kind of thing, not as it is. >> >> Using interrupt thread for this _will_ work, however this will require a >> a lot more changes, as currently alarm API allocates everything through >> rte_malloc, while we want to use IPC for rte_malloc (which would make it >> a circular dependency). So it'll probably be more API and more >> complexity for dealing with malloc vs rte_malloc allocations. Hence the >> least-bad approach taken here: a new thread. > > If everybody is happy enough with "least bad" design and not trying > to improve the core design, what can I say? I'm not against trying to improve the core design. I'm just saying that, had this kind of feedback been provided just a bit earlier, I would've had time to fix it in time for deadlines. However, because memory rework patchset depends on this API, i would suggest merging it in now, as is, and commit to a roadmap of improvements for next release(s). For starters, we could plan on removing alarm thread's dependency on rte_malloc and just use regular malloc API's in there, and rework asynchronous IPC API to use that instead. This shouldn't be much work, and will presumably make you halfway happy, as one of the threads will be gone :) We can then look into removing the second thread and moving the entirety of DPDK IPC into the interrupt thread. I'm not too sure how would that work, but i haven't looked at it in any detail, so maybe it is feasible. Can we agree on this? It would be great to do everything perfectly from the first try, but having a goal in sight and working towards it is fine too, even if not all of the steps we take are perfect. -- Thanks, Anatoly