[dpdk-dev] [PATCH 2/5] ethdev: add port ownership

Matan Azrad matan at mellanox.com
Tue Dec 5 07:08:35 CET 2017


Hi Neil

> -----Original Message-----
> From: Neil Horman [mailto:nhorman at tuxdriver.com]
> Sent: Tuesday, December 5, 2017 12:31 AM
> To: Matan Azrad <matan at mellanox.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>; Gaëtan Rivet
> <gaetan.rivet at 6wind.com>; Thomas Monjalon <thomas at monjalon.net>;
> Wu, Jingjing <jingjing.wu at intel.com>; dev at 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 at tuxdriver.com]
> > > Sent: Monday, December 4, 2017 6:01 PM
> > > To: Matan Azrad <matan at mellanox.com>
> > > Cc: Ananyev, Konstantin <konstantin.ananyev at intel.com>; Gaëtan Rivet
> > > <gaetan.rivet at 6wind.com>; Thomas Monjalon
> <thomas at monjalon.net>; Wu,
> > > Jingjing <jingjing.wu at intel.com>; dev at 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 at intel.com]
> > > > > Sent: Sunday, December 3, 2017 1:10 PM
> > > > > To: Matan Azrad <matan at mellanox.com>; Neil Horman
> > > > > <nhorman at tuxdriver.com>; Gaëtan Rivet
> <gaetan.rivet at 6wind.com>
> > > > > Cc: Thomas Monjalon <thomas at monjalon.net>; Wu, Jingjing
> > > > > <jingjing.wu at intel.com>; dev at dpdk.org
> > > > > Subject: RE: [dpdk-dev] [PATCH 2/5] ethdev: add port ownership
> > > > >
> > > > >
> > > > >
> > > > > Hi Matan,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Matan
> > > > > > Azrad
> > > > > > Sent: Sunday, December 3, 2017 8:05 AM
> > > > > > To: Neil Horman <nhorman at tuxdriver.com>; Gaëtan Rivet
> > > > > > <gaetan.rivet at 6wind.com>
> > > > > > Cc: Thomas Monjalon <thomas at monjalon.net>; Wu, Jingjing
> > > > > > <jingjing.wu at intel.com>; dev at dpdk.org
> > > > > > Subject: Re: [dpdk-dev] [PATCH 2/5] ethdev: add port ownership
> > > > > >
> > > > > > Hi
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Neil Horman [mailto:nhorman at tuxdriver.com]
> > > > > > > Sent: Friday, December 1, 2017 2:10 PM
> > > > > > > To: Gaëtan Rivet <gaetan.rivet at 6wind.com>
> > > > > > > Cc: Matan Azrad <matan at mellanox.com>; Thomas Monjalon
> > > > > > > <thomas at monjalon.net>; Jingjing Wu <jingjing.wu at intel.com>;
> > > > > > > dev at 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 <matan at mellanox.com>
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > 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.

> 
> Neil



More information about the dev mailing list