Hi Jinging, > -----Original Message----- > From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Wednesday, September 6, 2017 1:21 PM > To: Gaëtan Rivet <gaetan.ri...@6wind.com>; Matan Azrad > <ma...@mellanox.com> > Cc: Jingjing Wu <jingjing...@intel.com>; dev@dpdk.org; Ori Kam > <or...@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.ri...@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
