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 AADC9325B for ; Mon, 4 Dec 2017 23:32:01 +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 1eLzGn-0004Zg-Af; Mon, 04 Dec 2017 17:31:46 -0500 Date: Mon, 4 Dec 2017 17:30:51 -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: <20171204223051.GA11618@hmswarspite.think-freely.org> References: <1511870281-15282-1-git-send-email-matan@mellanox.com> <1511870281-15282-3-git-send-email-matan@mellanox.com> <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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit 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: Mon, 04 Dec 2017 22:32:02 -0000 On Mon, Dec 04, 2017 at 06:10:56PM +0000, Matan Azrad wrote: > Hi Neil > > > -----Original Message----- > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > Sent: Monday, December 4, 2017 6:01 PM > > To: Matan Azrad > > Cc: Ananyev, Konstantin ; Gaëtan Rivet > > ; Thomas Monjalon ; > > Wu, Jingjing ; dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH 2/5] ethdev: add port ownership > > > > On Sun, Dec 03, 2017 at 01:46:49PM +0000, Matan Azrad wrote: > > > Hi Konstantine > > > > > > > -----Original Message----- > > > > From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com] > > > > Sent: Sunday, December 3, 2017 1:10 PM > > > > To: Matan Azrad ; Neil Horman > > > > ; Gaëtan Rivet > > > > Cc: Thomas Monjalon ; Wu, Jingjing > > > > ; dev@dpdk.org > > > > Subject: RE: [dpdk-dev] [PATCH 2/5] ethdev: add port ownership > > > > > > > > > > > > > > > > Hi Matan, > > > > > > > > > -----Original Message----- > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Matan Azrad > > > > > Sent: Sunday, December 3, 2017 8:05 AM > > > > > To: Neil Horman ; Gaëtan Rivet > > > > > > > > > > Cc: Thomas Monjalon ; Wu, Jingjing > > > > > ; dev@dpdk.org > > > > > Subject: Re: [dpdk-dev] [PATCH 2/5] ethdev: add port ownership > > > > > > > > > > Hi > > > > > > > > > > > -----Original Message----- > > > > > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > > > > > Sent: Friday, December 1, 2017 2:10 PM > > > > > > To: Gaëtan Rivet > > > > > > Cc: Matan Azrad ; Thomas Monjalon > > > > > > ; Jingjing Wu ; > > > > > > dev@dpdk.org > > > > > > Subject: Re: [dpdk-dev] [PATCH 2/5] ethdev: add port ownership > > > > > > > > > > > > On Thu, Nov 30, 2017 at 02:24:43PM +0100, Gaëtan Rivet wrote: > > > > > > > Hello Matan, Neil, > > > > > > > > > > > > > > I like the port ownership concept. I think it is needed to > > > > > > > clarify some operations and should be useful to several subsystems. > > > > > > > > > > > > > > This patch could certainly be sub-divided however, and your > > > > > > > current > > > > > > > 1/5 should probably come after this one. > > > > > > > > > > > > > > Some comments inline. > > > > > > > > > > > > > > On Thu, Nov 30, 2017 at 07:36:11AM -0500, Neil Horman wrote: > > > > > > > > On Tue, Nov 28, 2017 at 11:57:58AM +0000, Matan Azrad wrote: > > > > > > > > > The ownership of a port is implicit in DPDK. > > > > > > > > > Making it explicit is better from the next reasons: > > > > > > > > > 1. It may be convenient for multi-process applications to > > > > > > > > > know > > > > which > > > > > > > > > process is in charge of a port. > > > > > > > > > 2. A library could work on top of a port. > > > > > > > > > 3. A port can work on top of another port. > > > > > > > > > > > > > > > > > > Also in the fail-safe case, an issue has been met in testpmd. > > > > > > > > > We need to check that the user is not trying to use a port > > > > > > > > > which is already managed by fail-safe. > > > > > > > > > > > > > > > > > > Add ownership mechanism to DPDK Ethernet devices to avoid > > > > > > > > > multiple management of a device by different DPDK entities. > > > > > > > > > > > > > > > > > > A port owner is built from owner id(number) and owner > > > > > > > > > name(string) while the owner id must be unique to > > > > > > > > > distinguish between two identical entity instances and the > > > > > > > > > owner name can be > > > > any name. > > > > > > > > > The name helps to logically recognize the owner by > > > > > > > > > different DPDK entities and allows easy debug. > > > > > > > > > Each DPDK entity can allocate an owner unique identifier > > > > > > > > > and can use it and its preferred name to owns valid ethdev > > ports. > > > > > > > > > Each DPDK entity can get any port owner status to decide > > > > > > > > > if it can manage the port or not. > > > > > > > > > > > > > > > > > > The current ethdev internal port management is not > > > > > > > > > affected by this feature. > > > > > > > > > > > > > > > > > > > > > > > The internal port management is not affected, but the external > > > > > > > interface is, however. In order to respect port ownership, > > > > > > > applications are forced to modify their port iterator, as > > > > > > > shown by your > > > > > > testpmd patch. > > > > > > > > > > > > > > I think it would be better to modify the current > > > > > > > RTE_ETH_FOREACH_DEV to call RTE_FOREACH_DEV_OWNED_BY, > > and > > > > > > > introduce a default owner that would represent the application > > > > > > > itself (probably with the ID 0 and an owner string ""). Only > > > > > > > with specific additional configuration should this default > > > > > > > subset of ethdev be > > > > divided. > > > > > > > > > > > > > > This would make this evolution seamless for applications, at > > > > > > > no cost to the complexity of the design. > > > > > > > > > > > > > > > > Signed-off-by: Matan Azrad > > > > > > > > > > > > > > > > > > > > > > > > This seems fairly racy. What if one thread attempts to set > > > > > > > > ownership on a port, while another is checking it on another > > > > > > > > cpu in parallel. It doesn't seem like it will protect against that at all. > > > > > > > > It also doesn't protect against the possibility of multiple > > > > > > > > threads attempting to poll for rx in parallel, which I think > > > > > > > > was part of Thomas's origional statement regarding port > > > > > > > > ownership (he noted that the lockless design implied only a > > > > > > > > single thread should be allowed to poll > > > > > > for receive or make configuration changes at a time. > > > > > > > > > > > > > > > > Neil > > > > > > > > > > > > > > > > > > > > > > Isn't this race already there for any configuration operation > > > > > > > / polling function? The DPDK arch is expecting applications to solve > > it. > > > > > > > Why should port ownership be designed differently from other > > > > > > > DPDK > > > > > > components? > > > > > > > > > > > > > Yes, but that doesn't mean it should exist in purpituity, nor > > > > > > does it mean that your new api should contain it as well. > > > > > > > > > > > > > Embedding checks for port ownership within operations will > > > > > > > force everyone to bear their costs, even those not interested > > > > > > > in using this API. These checks should be kept outside, within > > > > > > > the entity claiming ownership of the port, in the form of > > > > > > > using the proper port iterator IMO. > > > > > > > > > > > > > No. At the very least, you need to make the API itself exclusive. > > > > > > That is to say, you should at least ensure that a port ownership > > > > > > get call doesn't race with a port ownership set call. It seems > > > > > > rediculous to just leave that sort of locking as an exercize to the user. > > > > > > > > > > > > Neil > > > > > > > > > > > Neil, > > > > > As Thomas mentioned, a DPDK port is designed to be managed by only > > > > > one thread (or synchronized DPDK entity). > > > > > So all the port management includes port ownership shouldn't be > > > > > synchronized, i.e. locks are not needed. > > > > > If some application want to do dual thread port management, the > > > > > responsibility to synchronize the port ownership or any other port > > > > > management is on this application. > > > > > Port ownership doesn't come to allow synchronized management of > > > > > the port by two DPDK entities in parallel, it is just nice way to > > > > > answer next > > > > questions: > > > > > 1. Is the port already owned by some DPDK entity(in early control > > > > path)? > > > > > 2. If yes, Who is the owner? > > > > > If the answer to the first question is no, the current entity can > > > > > take the ownership without any lock(1 thread). > > > > > If the answer to the first question is yes, you can recognize the > > > > > owner and take decisions accordingly, sometimes you can decide to > > > > > use the port because you logically know what the current owner > > > > > does and you can be logically synchronized with it, sometimes you > > > > > can just leave this port because you have not any deal with this owner. > > > > > > > > If the goal is just to have an ability to recognize is that device > > > > is managed by another device (failsafe, bonding, etc.), then I > > > > think all we need is a pointer to rte_eth_dev_data of the owner (NULL > > would mean no owner). > > > > > > I think string is better than a pointer from the next reasons: > > > 1. It is more human friendly than pointers for debug and printing. > > > 2. it is flexible and allows to forward logical owner message to other DPDK > > entities. > > > > > > > Also I think if we'd like to introduce that mechanism, then it needs > > > > to be > > > > - mandatory (control API just don't allow changes to the device > > > > configuration if caller is not an owner). > > > > > > But what if 2 DPDK entities should manage the same port \ using it and they > > are synchronized? > > > > > > > - transparent to the user (no API changes). > > > > > > For now, there is not API change but new suggested API to use for port > > iteration. > > > > > > > - set/get owner ops need to be atomic if we want this mechanism to > > > > be usable for MP. > > > > > > But also without atomic this mechanism is usable in MP. > > > For example: > > > PRIMARY application can set its owner with string "primary A". > > > SECONDARY process (which attach to the ports only after the primary > > created them )is not allowed to set owner(As you can see in the code) but it > > can read the owner string and see that the port owner is the primary > > application. > > > The "A" can just sign specific port type to the SECONDARY that this port > > works with logic A which means, for example, primary should send the > > packets and secondary should receive the packets. > > > > > But thats just the point, the operations that you are describing are not atomic > > at all. If the primary process is interrupted during its setting of a ports owner > > ship after its read the current owner field, its entirely possible for the > > secondary proces to set the owner as itself, and then have the primary > > process set it back. Without locking, its just broken. I know that the dpdk > > likes to say its lockless, because it has no locks, but here we're just kicking the > > can down the road, by making the application add the locks for the library. > > > > Neil > > > As I wrote before and as is in the code you can understand that secondary process should not take ownership of ports. But you allow for it, and if you do, you should write your api to be safe for such opperations. > 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. If (1) is true, then all the locking required to access the data of port ownership needs to be adhered to. 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. Neil