From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 903D01D8E for ; Tue, 5 Dec 2017 20:27:38 +0100 (CET) Received: from cpe-2606-a000-111b-423c-e874-da8e-c543-d863.dyn6.twc.com ([2606:a000:111b:423c:e874:da8e:c543:d863] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1eMIs6-0005nJ-Le; Tue, 05 Dec 2017 14:27:19 -0500 Date: Tue, 5 Dec 2017 14:26:40 -0500 From: Neil Horman To: Matan Azrad Cc: "Ananyev, Konstantin" , =?iso-8859-1?Q?Ga=EBtan?= Rivet , Thomas Monjalon , "Wu, Jingjing" , "dev@dpdk.org" Message-ID: <20171205192640.GC10327@hmswarspite.think-freely.org> References: <20171130123611.GA20914@hmswarspite.think-freely.org> <20171130132443.4htutb5gpktcshgh@bidouze.vm.6wind.com> <20171201120946.GA23598@hmswarspite.think-freely.org> <2601191342CEEE43887BDE71AB9772585FAC3E77@irsmsx105.ger.corp.intel.com> <20171204160117.GB25048@hmswarspite.think-freely.org> <20171204223051.GA11618@hmswarspite.think-freely.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.1 (2017-09-22) X-Spam-Score: -2.9 (--) X-Spam-Status: No Subject: Re: [dpdk-dev] [PATCH 2/5] ethdev: add port ownership 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: Tue, 05 Dec 2017 19:27:40 -0000 On Tue, Dec 05, 2017 at 06:08:35AM +0000, Matan Azrad wrote: > > Please look at the code again, secondary process cannot take ownership, I don't allow it! > Actually, I think it must not be like that as no limitation for that in any other ethdev configurations. > Sure you do. Consider the following situation, two tasks, A and B running independently on separate CPUS: TASK A TASK B ================================================================================ calls rte_eth_dev_owner_new (gets id 2)| calls rte_eth_dev_owner_new (gets id 1) | calls rte_eth_dev_owner_set on port 1 | calls rte_eth_dev_owner_set (port 1) | sets port_owner->id = 2 | gets removed from cpu via scheduler | | returns to continue running on cpu | Gets interrupted immediately before | completes rte_eth_dev_owner_set, return 0 statement | setting port_owner->id = 1 | | returns 0 from rte_eth_dev_owner_set is scheduled back on the cpu | | returns 0 from rte_eth_dev_owner_set | in the above scenario, you've allowed two tasks to race through the ownership set routine, and while your intended semantics indicate that task A should have taken ownership of the port, task B actually did, and whats worse, both tasks think they completed successfully. I get that much of dpdk relies on the fact that the application either handles all the locking, or architects itself so that a single thread of execution (or at least only one thread at a time), is responsible for packet processing and port configuration. If you are assuming the former, you've done a disservice to the downstream consumer, because the locking is the intricate part of this operation (i.e. you are requiring that the developer figure out what granularity of locking is required such that you don't serialize too many operations that don't need it, while maintaining enough serialization that you don't corrupt the data that they wanted the api to manage in the first place. If you assert that the application should only be using a single task to do these operations to begin with, then this API isn't overly useful, because theres only one thread pushing data into the library and, by definition it implicitly owns all the ports, or at least knows which ports it shouldn't mess with (e.g subordunate ports in a failsafe device). > > > Any port configuration (for example port creation and release) is not > > internally synchronized between the primary to secondary processes so I > > don't see any reason to synchronize port ownership. > > Yes, and I've never agreed with that design point either, because the fact of > > the matter is that one of two things must be true in relation to port > > configuration: > > > > 1) Either multiple processes will attempt to read/change port > > configuration/ownership > > > > or > > > > 2) port ownership is implicitly given to a single task (be it a primary or > > secondary task), and said ownership is therefore implicitly known by the > > application > > > > Either situation may be true depending on the details of the application being > > built, but regardless, if (2) is true, then this api isn't really needed at all, > > because the application implicitly has been designed to have a port be > > owned by a given task. > > Here I think you miss something, the port ownership is not mainly for process DPDK entities, > The entity can be also PMD, library, application in same process. > For MP it is nice to have, the secondary just can see the primary owners and take decision accordingly (please read my answer to Konstatin above). > But the former is just a case of the latter (in fact worse). if you expect another execution context out of the control of the application to query this API, then you are in an MP situation, and definately need to provide mutually exclusive access to your data. If instead you expect all other execution contexts to suspend (or otherwise refrain from accessing this API) while a single task makes changes, then by definition you have already had to create some syncrnoization between those contexts and are capable of informing them of of the new ownership scheme The bottom line is, either you expect multiple access, or you dont. If you expect multiple access, and you belive that said access are not completely under the control of your application, you need to protect those accesses against one another. If you don't expect multiple access (or expect your application to architect itself to enforce single access), then you've created a environment in which the single accessor already has to know all the information regarding port ownership that you store in the API. > If (1) is true, then all the locking required to access > > the data of port ownership needs to be adhered to. > > > > And all the port configurations! > I think it is behind to this thread. > > > > I understand that you are taking Thomas' words to mean that all paths are > > lockless in the DPDK, and that is true as a statement of fact (in that the DPDK > > doesn't synchronize access to internal data). What it does do, is leave that > > locking as an exercize for the downstream consumer of the library. While I > > don't agree with it, I can see that such an argument is valid for hot paths such > > as transmission and reception where you may perhaps want to minimize > > your locking by assigning a single task to do that work, but port configuration > > and ownership isn't a hot path. If you mean to allow potentially multiple > > tasks to access configuration (including port ownership), be it frequently or > > just occasionaly, why are you making a downstream developer recognize the > > need for locking here outside the library? It just seems like very bad general > > practice to me. > > > > > If the primary-secondary process want to manage(configure) same port in > > same time, they must be synchronized by the applications, so this is the case > > in port ownership too (actually I don't think this synchronization is realistic > > because many configurations of the port are not in shared memory). > > Yes, it is the case, my question is, why? We're not talking about a time > > sensitive execution path here. And by avoiding it you're just making work > > that has to be repeated by every downstream consumer. > > I think you suggest to make all the ethdev configuration race safe, it is behind to this thread. > Current ethdev implementation leave the race management to applications, so port ownership as any other port configurations should be designed in the same method. > I would like to make ethdev configuration race safe, but thats a fight I've lost, but I strongly disagree that just because some parts of the dpdk leave race safety to the user, doesn't mean you have to. Its just silly not to here. We're not talking about a hot execution path here, we're talking about one time / rare configuration changes. To insert a lock (or other lightweight atomic operation) costs nothing in terms of execution time, and saves downstream consumers significant time figuring out what the best mutual exclusion strategy is. Why wouldn't you do that? Neil > > > > Neil > >