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

[Gaëtan Rivet]

On Thu, Nov 30, 2017 at 02:30:20PM +0000, Matan Azrad wrote:
> Hi all
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet at 6wind.com]
> > Sent: Thursday, November 30, 2017 3:25 PM
> > To: Neil Horman <nhorman at tuxdriver.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
> > 
> > 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.
> 
> Can you suggest how to divide it?
> 

Adding first the API to add / remove owners, then in a second patch
set / get / unset. (by the way, remove / delete is pretty confusing, I'd
suggest renaming those.) You can also separate the introduction of the
new owner-wise iterator.

Ultimately, you are the author, it's your job to help us review your
work.

> 1/5 could be actually outside of this series, it is just better behavior to use the right function to do release port :)
> 
> > 
> > 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.
> 
> As you can see in patch code and in testpmd example I added option to iterate
> over all valid ownerless ports which should be more relevant by owner_id = 0.
> So maybe the RTE_ETH_FOREACH_DEV should be changed to run this by the new iterator.

That is precisely what I am suggesting.
Ideally, you should not have to change anything in testpmd, beside some
bug fixing regarding port iteration to avoid those with a specific
owner.

RTE_ETH_FOREACH_DEV must stay valid, and should iterate over ownerless
ports (read: port owned by the default owner).

> By this way current applications don't need to build their owners but the ABI will be broken.
> 

ABI is broken anyway as you will add the owner to rte_eth_dev_data.

> Actually, I think the old iterator RTE_ETH_FOREACH_DEV should be unexposed or just removed,

I don't think so. Using RTE_ETH_FOREACH_DEV should allow keeping the
current behavior of iterating over ownerless ports. Applications that do
not care for this API should not have to change anything to their code.

> also the DEFFERED state should be removed,

Of course.

> I don't really see any usage to iterate over all valid ports by DPDK entities different from ethdev itself.
> I just don't want to break it now.
> 

[snip]

-- 
Gaëtan Rivet
6WIND