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: sta...@dpdk.org
>
> Signed-off-by: Matan Azrad <ma...@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
