[dpdk-dev] [PATCH] app/testpmd: fix forward port ids setting

Gaëtan Rivet gaetan.rivet at 6wind.com
Mon Sep 4 11:52:32 CEST 2017


On Mon, Sep 04, 2017 at 09:25:04AM +0000, Matan Azrad wrote:
> Hi
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet at 6wind.com]
> > Sent: Monday, September 4, 2017 11:46 AM
> > To: Matan Azrad <matan at mellanox.com>
> > Cc: Jingjing Wu <jingjing.wu at intel.com>; Thomas Monjalon
> > <thomas at monjalon.net>; dev at dpdk.org; Ori Kam <orika at mellanox.com>;
> > stable at dpdk.org
> > Subject: Re: [PATCH] app/testpmd: fix forward port ids setting
> > 
> > Hi Matan,
> > 
> > On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> > > The corrupted code didn't check the port availability when it was
> > > trying to set the forward port IDs array.
> > > However, when it was counting the number of ports, the availability
> > > was checked by RTE_ETH_FOREACH_DEV iterator.
> > >
> > > Hence, even when ETH devices ports were not in ATTACHED state, the
> > > testpmd tried to forward traffic by them and got segmentation fault at
> > > queue access time.
> > >
> > > For example:
> > > When EAL command line parameters include two devices, the first is
> > > failsafe with two sub devices and the second is any device, testpmd
> > > gets two devices by the iterator and sets for forwarding both, the
> > > failsafe device and the failsafe first sub device (instead of the
> > > second sub device).
> > > After the first failsafe sub device state was changed to DEFERRED,
> > > testpmd tries to forward traffic through the deferred device because
> > > it didn't check the port availability in setting time.
> > >
> > > The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward port IDs
> > > default setting.
> > >
> > > Fixes: af75078fece3 ("first public release")
> > > Cc: stable at dpdk.org
> > >
> > > Signed-off-by: Matan Azrad <matan at mellanox.com>
> > > ---
> > >  app/test-pmd/testpmd.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > Hi All
> > > I would like to bring up a discussion to complete this bug fix.
> > >
> > > When user wants to set the list of forwarding ports by "set portlist"
> > > (testpmd command line), the testpmd application checks the
> > > availability of the ports by rte_eth_dev_is_valid_port API.
> > > By this way, it gets the DEFERRED port as valid port and will try to
> > > recieve\send packets via this port.
> > >
> > > This scenario will cause the same error as this patch fixes.
> > >
> > > Should testpmd allow user to run traffic by DEFERRED port directly?
> > >
> > > If any application wants to check a port availability for device usage
> > > (conf\rxtx), Which API should be used?
> > >
> > > According to the patch cb894d99eceb ("ethdev: add deferred
> > > intermediate device state"), DEFERRED ports should be invisible to
> > > application, So maybe the rte_eth_dev_is_valid_port API should be
> > > internal and a new ethdev API should be created for applications.
> > >
> > > What do you think?
> > >
> > 
> > I think that regardless of the semantic of the DEFERRED state or any other
> > port handling, the correct assumption is to consider any port iterated over by
> > RTE_ETH_FOREACH_DEV to be the exact set of devices that are supposed to
> > be usable. In the event of an API evolution regarding port states, this macro
> > should remain correct.
> > 
> > So I think your fix is correct, and that the implication of
> > RTE_ETH_FOREACH_DEV avoiding DEFERRED devices is correct.
> > 
> 
> This patch fixes the default forward ports setting (actually when user don't use --portmask param or "set portlist"),
> But it don't fix the port validation which PMD does for "set portlist" command.
> So, the discussion is how to fix this port validation.

You could make a static rte_eth_dev_is_valid_port with a different name,
declare both RTE_ETH_VALID_PORT* macros within rte_ethdev.c
and introduce a new rte_eth_dev_is_valid_port which would restrict
devices to those ATTACHED.

I'm not sure this would be interesting for applications. Currently this
check is performed internally by the ether layer, I guess most
applications rely on it. Making the "extended" version (ATTACHED +
DEFERRED) private with the strict one public would probably not change
behaviors, thus it would probably have little to no effect.

So my opinion is "why not, but the risk is adding dead code for no real
benefit".

> In current code, testpmd uses  rte_eth_dev_is_valid_port which return the DEFERRED device too for forwarding.
> Should it use the RTE_ETH_FOREACH_DEV  iterator for one port validation? 
> Don't you think we need new API for one port?
> 
> > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > > 7d40139..f9bdbf8 100644
> > > --- a/app/test-pmd/testpmd.c
> > > +++ b/app/test-pmd/testpmd.c
> > > @@ -463,9 +463,10 @@ static void
> > >  set_default_fwd_ports_config(void)
> > >  {
> > >  	portid_t pt_id;
> > > +	int i = 0;
> > >
> > > -	for (pt_id = 0; pt_id < nb_ports; pt_id++)
> > > -		fwd_ports_ids[pt_id] = pt_id;
> > > +	RTE_ETH_FOREACH_DEV(pt_id)
> > > +		fwd_ports_ids[i++] = pt_id;
> > >
> > >  	nb_cfg_ports = nb_ports;
> > >  	nb_fwd_ports = nb_ports;
> > > --
> > > 2.7.4
> > >
> > 
> > --
> > Gaëtan Rivet
> > 6WIND

-- 
Gaëtan Rivet
6WIND


More information about the dev mailing list