From: Matan Azrad <matan@mellanox.com>
To: Jingjing Wu <jingjing.wu@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "Ori Kam" <orika@mellanox.com>,
"Thomas Monjalon" <thomas@monjalon.net>,
"Gaëtan Rivet" <gaetan.rivet@6wind.com>
Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix forward port ids setting
Date: Wed, 6 Sep 2017 11:09:20 +0000 [thread overview]
Message-ID: <DB6PR0502MB30487C7F0A8D23C1938864DED2970@DB6PR0502MB3048.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <1766803.K7SpHi0fzz@xps>
Hi Jinging,
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, September 6, 2017 1:21 PM
> To: Gaëtan Rivet <gaetan.rivet@6wind.com>; Matan Azrad
> <matan@mellanox.com>
> Cc: Jingjing Wu <jingjing.wu@intel.com>; dev@dpdk.org; Ori Kam
> <orika@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@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
next prev parent reply other threads:[~2017-09-06 11:09 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-03 13:19 [dpdk-dev] " Matan Azrad
2017-09-04 8:45 ` Gaëtan Rivet
2017-09-04 9:25 ` Matan Azrad
2017-09-04 9:52 ` Gaëtan Rivet
2017-09-06 10:21 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2017-09-06 11:09 ` Matan Azrad [this message]
2017-09-07 7:44 ` [dpdk-dev] " Wu, Jingjing
2017-10-09 5:13 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DB6PR0502MB30487C7F0A8D23C1938864DED2970@DB6PR0502MB3048.eurprd05.prod.outlook.com \
--to=matan@mellanox.com \
--cc=dev@dpdk.org \
--cc=gaetan.rivet@6wind.com \
--cc=jingjing.wu@intel.com \
--cc=orika@mellanox.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).