From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH] app/testpmd: fix forward port ids setting Date: Mon, 4 Sep 2017 11:52:32 +0200 Message-ID: <20170904095231.GD21444@bidouze.vm.6wind.com> References: <1504444747-56298-1-git-send-email-matan@mellanox.com> <20170904084538.GB21444@bidouze.vm.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: Jingjing Wu , Thomas Monjalon , "dev@dpdk.org" , Ori Kam , "stable@dpdk.org" To: Matan Azrad Return-path: Received: from mail-wm0-f43.google.com (mail-wm0-f43.google.com [74.125.82.43]) by dpdk.org (Postfix) with ESMTP id C191E5A6E for ; Mon, 4 Sep 2017 11:52:44 +0200 (CEST) Received: by mail-wm0-f43.google.com with SMTP id 187so116962wmn.1 for ; Mon, 04 Sep 2017 02:52:44 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Sep 04, 2017 at 09:25:04AM +0000, Matan Azrad wrote: > Hi > > > -----Original Message----- > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com] > > Sent: Monday, September 4, 2017 11:46 AM > > To: Matan Azrad > > Cc: Jingjing Wu ; Thomas Monjalon > > ; dev@dpdk.org; Ori Kam ; > > stable@dpdk.org > > Subject: Re: [PATCH] app/testpmd: fix forward port ids setting > > > > 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 > > > --- > > > 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. > > > > 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? > > > > 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 -- Gaëtan Rivet 6WIND