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 068881D8E for ; Tue, 5 Dec 2017 11:05:47 +0100 (CET) Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Dec 2017 02:05:46 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,363,1508828400"; d="scan'208";a="208113" Received: from bricha3-mobl3.ger.corp.intel.com ([10.252.4.226]) by fmsmga007.fm.intel.com with SMTP; 05 Dec 2017 02:05:43 -0800 Received: by (sSMTP sendmail emulation); Tue, 05 Dec 2017 10:05:42 +0000 Date: Tue, 5 Dec 2017 10:05:42 +0000 From: Bruce Richardson To: Matan Azrad Cc: Neil Horman , "Ananyev, Konstantin" , =?iso-8859-1?Q?Ga=EBtan?= Rivet , Thomas Monjalon , "Wu, Jingjing" , "dev@dpdk.org" Message-ID: <20171205100542.GA7492@bricha3-MOBL3.ger.corp.intel.com> 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=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.9.1 (2017-09-22) 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 10:05:48 -0000 On Tue, Dec 05, 2017 at 06:08:35AM +0000, Matan Azrad wrote: > Hi Neil > > > -----Original Message----- From: Neil Horman > > [mailto:nhorman@tuxdriver.com] Sent: Tuesday, December 5, 2017 12:31 > > AM 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 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. > > 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. > > > > 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). > > 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. > > > One key difference, though, being that port ownership itself could be used to manage the thread-safety of the ethdev configuration. It's also a little different from other APIs in that I find it hard to come up with a scenario where it would be very useful to an application without also having some form of locking present in it. For other config/control APIs we can have the control plane APIs work without locks e.g. by having a single designated thread/process manage all configuration updates. However, as Neil points out, in such a scenario, the ownership concept doesn't provide any additional benefit so can be skipped completely. I'd view it a bit like the reference counting of mbufs - we can provide a lockless/non-atomic version, but for just about every real use-case, you want the atomic version. Regards, /Bruce