[dpdk-dev] [PATCH] app/testpmd: fix forward port ids setting

Gaëtan Rivet gaetan.rivet at 6wind.com
Mon Sep 4 10:45:38 CEST 2017

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 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
> 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;
> +		fwd_ports_ids[i++] = pt_id;
>  	nb_cfg_ports = nb_ports;
>  	nb_fwd_ports = nb_ports;
> -- 
> 2.7.4

Gaëtan Rivet

More information about the dev mailing list