Hi Gaetan > -----Original Message----- > From: Gaëtan Rivet [mailto:gaetan.ri...@6wind.com] > Sent: Thursday, November 30, 2017 5:10 PM > To: Matan Azrad <ma...@mellanox.com> > Cc: Neil Horman <nhor...@tuxdriver.com>; Thomas Monjalon > <tho...@monjalon.net>; Jingjing Wu <jingjing...@intel.com>; > dev@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.ri...@6wind.com] > > > Sent: Thursday, November 30, 2017 3:25 PM > > > To: Neil Horman <nhor...@tuxdriver.com> > > > Cc: Matan Azrad <ma...@mellanox.com>; Thomas Monjalon > > > <tho...@monjalon.net>; Jingjing Wu <jingjing...@intel.com>; > > > dev@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
