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

[Thomas Monjalon <thomas@monjalon.net>]

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@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:
	http://dpdk.org/ml/archives/dev/2017-September/074656.html