From: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
To: Matan Azrad <matan@mellanox.com>
Cc: Jingjing Wu <jingjing.wu@intel.com>,
Thomas Monjalon <thomas@monjalon.net>,
dev@dpdk.org, Ori Kam <orika@mellanox.com>,
stable@dpdk.org
Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix forward port ids setting
Date: Mon, 4 Sep 2017 10:45:38 +0200 [thread overview]
Message-ID: <20170904084538.GB21444@bidouze.vm.6wind.com> (raw)
In-Reply-To: <1504444747-56298-1-git-send-email-matan@mellanox.com>
Hi Matan,
On Sun, Sep 03, 2017 at 04:19:07PM +0300, Matan Azrad wrote:
> The corrupted code didn't check the port availability when
> it was trying to set the forward port IDs array.
> However, when it was counting the number of ports, the availability
> was checked by RTE_ETH_FOREACH_DEV iterator.
>
> Hence, even when ETH devices ports were not in ATTACHED state,
> the testpmd tried to forward traffic by them and got segmentation
> fault at queue access time.
>
> For example:
> When EAL command line parameters include two devices, the first
> is failsafe with two sub devices and the second is any device,
> testpmd gets two devices by the iterator and sets for forwarding
> both, the failsafe device and the failsafe first sub device
> (instead of the second sub device).
> After the first failsafe sub device state was changed to DEFERRED,
> testpmd tries to forward traffic through the deferred device
> because it didn't check the port availability in setting time.
>
> The fix uses the RTE_ETH_FOREACH_DEV iterator for the forward
> port IDs default setting.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
> app/test-pmd/testpmd.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> 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.
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 7d40139..f9bdbf8 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -463,9 +463,10 @@ static void
> set_default_fwd_ports_config(void)
> {
> portid_t pt_id;
> + int i = 0;
>
> - for (pt_id = 0; pt_id < nb_ports; pt_id++)
> - fwd_ports_ids[pt_id] = pt_id;
> + RTE_ETH_FOREACH_DEV(pt_id)
> + fwd_ports_ids[i++] = pt_id;
>
> nb_cfg_ports = nb_ports;
> nb_fwd_ports = nb_ports;
> --
> 2.7.4
>
--
Gaëtan Rivet
6WIND
next prev parent reply other threads:[~2017-09-04 8:45 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-03 13:19 Matan Azrad
2017-09-04 8:45 ` Gaëtan Rivet [this message]
2017-09-04 9:25 ` Matan Azrad
2017-09-04 9:52 ` Gaëtan Rivet
2017-09-07 7:44 ` Wu, Jingjing
2017-10-09 5:13 ` 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=20170904084538.GB21444@bidouze.vm.6wind.com \
--to=gaetan.rivet@6wind.com \
--cc=dev@dpdk.org \
--cc=jingjing.wu@intel.com \
--cc=matan@mellanox.com \
--cc=orika@mellanox.com \
--cc=stable@dpdk.org \
--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).