[dpdk-dev] [PATCH v3 7/7] app/testpmd: adjust ethdev port ownership

[Matan Azrad]

Hi Konstantin

From: Ananyev, Konstantin, Friday, January 19, 2018 3:09 PM
> > -----Original Message-----
> > From: Matan Azrad [mailto:matan at mellanox.com]
> > Sent: Friday, January 19, 2018 12:52 PM
> > To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; Thomas
> > Monjalon <thomas at monjalon.net>; Gaetan Rivet
> <gaetan.rivet at 6wind.com>;
> > Wu, Jingjing <jingjing.wu at intel.com>
> > Cc: dev at dpdk.org; Neil Horman <nhorman at tuxdriver.com>; Richardson,
> > Bruce <bruce.richardson at intel.com>
> > Subject: RE: [PATCH v3 7/7] app/testpmd: adjust ethdev port ownership
> >
> > Hi Konstantin
> >
> > From: Ananyev, Konstantin, Friday, January 19, 2018 2:38 PM
> > > To: Matan Azrad <matan at mellanox.com>; Thomas Monjalon
> > > <thomas at monjalon.net>; Gaetan Rivet <gaetan.rivet at 6wind.com>;
> Wu,
> > > Jingjing <jingjing.wu at intel.com>
> > > Cc: dev at dpdk.org; Neil Horman <nhorman at tuxdriver.com>; Richardson,
> > > Bruce <bruce.richardson at intel.com>
> > > Subject: RE: [PATCH v3 7/7] app/testpmd: adjust ethdev port
> > > ownership
> > >
> > > Hi Matan,
> > >
> > > > -----Original Message-----
> > > > From: Matan Azrad [mailto:matan at mellanox.com]
> > > > Sent: Thursday, January 18, 2018 4:35 PM
> > > > To: Thomas Monjalon <thomas at monjalon.net>; Gaetan Rivet
> > > > <gaetan.rivet at 6wind.com>; Wu, Jingjing <jingjing.wu at intel.com>
> > > > Cc: dev at dpdk.org; Neil Horman <nhorman at tuxdriver.com>;
> Richardson,
> > > > Bruce <bruce.richardson at intel.com>; Ananyev, Konstantin
> > > > <konstantin.ananyev at intel.com>
> > > > Subject: [PATCH v3 7/7] app/testpmd: adjust ethdev port ownership
> > > >
> > > > Testpmd should not use ethdev ports which are managed by other
> > > > DPDK entities.
> > > >
> > > > Set Testpmd ownership to each port which is not used by other
> > > > entity and prevent any usage of ethdev ports which are not owned by
> Testpmd.
> > > >
> > > > Signed-off-by: Matan Azrad <matan at mellanox.com>
> > > > ---
> > > >  app/test-pmd/cmdline.c      | 89 +++++++++++++++++++-----------------
> ----
> > > -----
> > > >  app/test-pmd/cmdline_flow.c |  2 +-
> > > >  app/test-pmd/config.c       | 37 ++++++++++---------
> > > >  app/test-pmd/parameters.c   |  4 +-
> > > >  app/test-pmd/testpmd.c      | 63 ++++++++++++++++++++------------
> > > >  app/test-pmd/testpmd.h      |  3 ++
> > > >  6 files changed, 103 insertions(+), 95 deletions(-)
> > > >
> > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > > > 31919ba..6199c64 100644
> > > > --- a/app/test-pmd/cmdline.c
> > > > +++ b/app/test-pmd/cmdline.c
> > > > @@ -1394,7 +1394,7 @@ struct cmd_config_speed_all {
> > > >  			&link_speed) < 0)
> > > >  		return;
> > > >
> > > > -	RTE_ETH_FOREACH_DEV(pid) {
> > > > +	RTE_ETH_FOREACH_DEV_OWNED_BY(pid, my_owner.id) {
> > >
> > > Why do we need all these changes?
> > > As I understand you changed definition of RTE_ETH_FOREACH_DEV(), so
> > > no testpmd should work ok default (no_owner case).
> > > Am I missing something here?
> >
> > Now, After Gaetan suggestion RTE_ETH_FOREACH_DEV(pid) will iterate
> over all valid and ownerless ports.
> 
> Yes.
> 
> > Here Testpmd wants to iterate over its owned ports.
> 
> Why? Why it can't just iterate over all valid and ownerless ports?
> As I understand it would be enough to fix current problems and would allow
> us to avoid any changes in testmpd (which I think is a good thing).

Yes, I understand that this big change is very daunted, But I think the current a lot of bugs in testpmd(regarding port ownership) even more daunted.

Look,
Testpmd initiates some of its internal databases depends on specific port iteration,
In some time someone may take ownership of Testpmd ports and testpmd will continue to touch them.

In addition
Using the old iterator in some places in testpmd will cause a race for run-time new ports(can be created by failsafe or any hotplug code):
- testpmd finds an ownerless port(just now created) by the old iterator and start traffic there,
- failsafe takes ownership of this new port and start traffic there.
Problem!
 
In addition
As a good example for well-done application (free from ownership bugs) I tried here to adjust Tespmd to the new rules and BTW to fix a lot of bugs.    


So actually applications which are not aware to the port ownership still are exposed to races, but if there use the old iterator(with the new change) the amount of races decreases. 

Thanks, Matan.
> Konstantin
> 
> >
> > I added to Testpmd ability to take an ownership of ports as the new
> > ownership and synchronization rules suggested, Since Tespmd is a DPDK
> > entity which wants that no one will touch its owned ports, It must allocate
> an unique ID, set owner for its ports (see in main function) and recognizes
> them by its owner ID.
> >
> > > Konstantin
> > >