From: Thomas Monjalon <thomas@monjalon.net>
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-dev] [dpdk-stable] [PATCH] app/testpmd: fix forward port ids setting
Date: Wed, 06 Sep 2017 12:21:18 +0200 [thread overview]
Message-ID: <1766803.K7SpHi0fzz@xps> (raw)
In-Reply-To: <20170904095231.GD21444@bidouze.vm.6wind.com>
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
next prev parent reply other threads:[~2017-09-06 10:21 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 ` Thomas Monjalon [this message]
2017-09-06 11:09 ` [dpdk-dev] [dpdk-stable] " Matan Azrad
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=1766803.K7SpHi0fzz@xps \
--to=thomas@monjalon.net \
--cc=dev@dpdk.org \
--cc=gaetan.rivet@6wind.com \
--cc=jingjing.wu@intel.com \
--cc=matan@mellanox.com \
--cc=orika@mellanox.com \
/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).