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

[Bruce Richardson]

On Tue, Dec 05, 2017 at 06:08:35AM +0000, Matan Azrad wrote:
> 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.
> 
> > 
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