[dpdk-dev] [PATCH] app/testpmd: fix forward port ids setting
gaetan.rivet at 6wind.com
Mon Sep 4 10:45:38 CEST 2017
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 at dpdk.org
> Signed-off-by: Matan Azrad <matan at 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
> 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
> 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
> 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;
More information about the dev