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

[Matan Azrad]

Hi Jinging,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas at monjalon.net]
> Sent: Wednesday, September 6, 2017 1:21 PM
> To: Gaëtan Rivet <gaetan.rivet at 6wind.com>; Matan Azrad
> <matan at mellanox.com>
> Cc: Jingjing Wu <jingjing.wu at intel.com>; dev at dpdk.org; Ori Kam
> <orika at mellanox.com>
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix forward port ids setting
> 
> 04/09/2017 11:52, Gaëtan Rivet:
> > On Mon, Sep 04, 2017 at 09:25:04AM +0000, Matan Azrad wrote:
> > > From: Gaëtan Rivet [mailto:gaetan.rivet at 6wind.com]
> > > > On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> > > > > 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?
> 
> Please, let's continue this ethdev discussion in a separate thread.
> I've started a new one:
> 	https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F
> %2Fdpdk.org%2Fml%2Farchives%2Fdev%2F2017-
> September%2F074656.html&data=02%7C01%7Corika%40mellanox.com%7C5
> 9017f577e004c8be80c08d4f51104ec%7Ca652971c7d2e4d9ba6a4d149256f461b
> %7C0%7C0%7C636402900840946032&sdata=lPFh1ro1cJTyiYYC7KQtRm7CQ8M
> rkct7i6%2BUBW1HEsM%3D&reserved=0

I think you can acknowledge this fix for the default forward port IDs setting (this patch fixes it).
I will send fix in a separated patch to the  "set portlist" port validation after ethdev discussion will be done.

Regards,
Matan Azrad