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

[Neil Horman]

On Thu, Dec 21, 2017 at 07:37:06PM +0000, Matan Azrad wrote:
> Hi
> 
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman at tuxdriver.com]
> > Sent: Thursday, December 21, 2017 7:43 PM
> > To: Thomas Monjalon <thomas at monjalon.net>
> > Cc: dev at dpdk.org; Bruce Richardson <bruce.richardson at intel.com>; Matan
> > Azrad <matan at mellanox.com>; Ananyev, Konstantin
> > <konstantin.ananyev at intel.com>; Gaëtan Rivet <gaetan.rivet at 6wind.com>;
> > Wu, Jingjing <jingjing.wu at intel.com>
> > Subject: Re: [dpdk-dev] [PATCH 2/5] ethdev: add port ownership
> > 
> > On Thu, Dec 21, 2017 at 06:06:48PM +0100, Thomas Monjalon wrote:
> > > 08/12/2017 13:31, Neil Horman:
> > > > On Fri, Dec 08, 2017 at 12:35:18PM +0100, Thomas Monjalon wrote:
> > > > > 05/12/2017 11:05, Bruce Richardson:
> > > > > > > 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.
> > > > >
> > > > > I think we need to clearly describe what is the tread-safety
> > > > > policy in DPDK (especially in ethdev as a first example).
> > > > > Let's start with obvious things:
> > > > >
> > > > > 	1/ A queue is not protected for races with multiple Rx or Tx
> > > > > 			- no planned change because of performance
> > purpose
> > > > > 	2/ The list of devices is racy
> > > > > 			- to be fixed with atomics
> > > > > 	3/ The configuration of different devices is thread-safe
> > > > > 			- the configurations are different per-device
> > > > > 	4/ The configuration of a given device is racy
> > > > > 			- can be managed by the owner of the device
> > > > > 	5/ The device ownership is racy
> > > > > 			- to be fixed with atomics
> > > > >
> > > > > What am I missing?
> > > > >
> 
> Thank you Thomas for this order.
> Actually the port ownership is a good opportunity to redefine the synchronization rules in ethdev :)
> 
> > > > There is fan out to consider here:
> > > >
> > > > 1) Is device configuration racy with ownership?  That is to say, can
> > > > I change ownership of a device safely while another thread that
> > > > currently owns it modifies its configuration?
> > >
> > > If an entity steals ownership to another one, either it is agreed
> > > earlier, or it is done by a central authority.
> > > When it is acked that ownership can be moved, there should not be any
> > > configuration in progress.
> > > So it is more a communication issue than a race.
> > >
> > But if thats the case (specifically that mutual exclusion between port
> > ownership and configuration is an exercize left to an application developer),
> > then port ownership itself is largely meaningless within the dpdk, because
> > the notion of who owns the port needs to be codified within the application
> > anyway.
> > 
> 
> Bruce, As I understand it, only the dpdk entity who took ownership of a port successfully can configure the device by default, if other dpdk entities want to configure it too they must to be synchronized with the port owner while it is not recommended after the port ownership integration.
> 
Can you clarify what you mean by "it is not recommended after the port ownership
integration"?  I think there is consensus that the port owner must be the only
entitiy to operate on a port (be that configuration/frame rx/tx, or some other
operation).  Multithreaded operation on a port always means some level of
synchronization between application threads and the dpdk library, but I'm not
sure why that would be different if we introduced a more concrete notion of port
ownership via a new library.

> So, for example,  if the dpdk entity is an application, the application should take ownership of the port and manage the synchronization of this port configuration between the application threads and its EAL host thread callbacks, no other dpdk entity should configure the same port because they should fail when they try to take ownership of the same port too.
Well, failing is one good approach, yes, blocking on port relenquishment could
be another.  I'd recommend an API with the following interface:

rte_port_ownership_claim(int port_id) - blocks execution of the calling thread
until the previous owner releases ownership, then claims it and returns

rte_port_ownership_release(int port_id) - releases ownership of port, or returns
error if the port was not owned by this execution context

rte_port_owernship_try_claim(int port_id) - same as rte_port_ownership_claim,
but fails if the port is already owned.

That would give the option for both semantics.

> Each dpdk entity which wants to take ownership must to be able to synchronize the port configuration in its level. 
Can you elaborate on what you mean by level here?  Are you envisioning a scheme
in which multiple execution contexts might own a port for various
non-conflicting purposes?


> 
> > 
> > > > 2) Is device configuration racy with device addition/removal?  That
> > > > is to say, can one thread remove a device while another configures it.
> > >
> > > I think it is the same as two threads configuring the same device
> > > (item 4/ above). It can be managed with port ownership.
> > >
> > Only if you assert that application is required to have the owning port be
> > responsible for the ports deletion, which we can say, but that leads to the
> > issue above again.
> > 
> > 
> As Thomas said in item 2 the port creation must be synchronized by ethdev and we need to add it there. 
> I think it is obvious that port removal must to be done only by the port owner.  
> 
You say that, but its obvious to you as a developer who has looked extensively
at the code.  It may well be less so to a consumer who is not an active member
of the community, for instance one who obtains the dpdk via pre-built package.

> 
> I think we need to add synchronization for port ownership management in this patch V2 and add port creation synchronization in ethdev in separate patch to fill the new rules Thomas suggested.
I think that makes sense, yes. 

Neil

> 
> What do you think?
> 
> > > > There are probably many subsystem interactions that need to be
> > addressed here.
> > > >
> > > > Neil
> > > >
> > > > > I am also wondering whether the device ownership can be a separate
> > > > > library used in several device class interfaces?
> > >
> > >
> > >
>