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

[Matan Azrad]

Hi Gaetan

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet at 6wind.com]
> Sent: Thursday, November 30, 2017 5:10 PM
> To: Matan Azrad <matan at mellanox.com>
> Cc: Neil Horman <nhorman at tuxdriver.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: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.
> 

When you suggest improvement I think you need to propose another method\idea.
The author probably thought about it and arrived to his conclusions.
 Exactly as you are doing now in naming.
If you have a specific question, I'm here to answer :) 

I agree with unset name instead of remove, will change it in V2.
  
> > 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.
> 

It is not, rte_eth_dev_data is internal.

> > 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.
> 

If we will break the ABI later users can use RTE_ETH_FOREACH_DEV_OWNED_BY(p,0)
to do it. RTE_ETH_FOREACH_DEV will be unnecessary anymore but maybe is too much to applications to change also the API.
I can agree with it.

> > 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