From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f178.google.com (mail-wr0-f178.google.com [209.85.128.178]) by dpdk.org (Postfix) with ESMTP id E7E9A7CC0 for ; Mon, 4 Sep 2017 10:45:49 +0200 (CEST) Received: by mail-wr0-f178.google.com with SMTP id y15so12796667wrc.2 for ; Mon, 04 Sep 2017 01:45:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=tHGRFBxbufFcWLtBjC0sMrSMYVJ45d3yc50EBzNlZm4=; b=Fw5W1lEAOPpVOxPgQTeTA334h/lyQuHY8XMOO0O8z3jtlxciZeSDLmglmA9L2Q4yPd Tn2WbKFRLWlpdyaVxoaShF3U1lrIVaY0yRBSa9uzEVKx3HFmbvDSqsnHCPsKFasa8ZBe my/KpAgl+PNKTsSaCTZy0R5q9GS1muO0u94dg/3oO2XJ7rKfzJqQRCCs5D4DZNXtpQBa wQ/36BCSfdHpiu/b5kd5Ovv5/9Kj7Rou3lcD6076YQ+offB5LNh5Y6whmYoFif6t7OUp HBRiFGmWd3BoRa96JIAcuq4NmlIFbkxvobiYKCffCE8SyhIEP/rgvbcqqzoiSm5rj0Mz LAPg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=tHGRFBxbufFcWLtBjC0sMrSMYVJ45d3yc50EBzNlZm4=; b=c5NpLy+dWamqGYdj5h9dPEW1GyA1M1SuCvPmBD8zEOP6WE83p4Nv6yV+6QOwlX4DPF tzvrjKg6vPsNdbnY2ZiRhapwOAeWA0cbxhknrpS/7Gx/nBU3fCw257YsC4FNyfUUIebV DZ67zm4PaiTFZL2DrX1g7dnLqwzxkB6YkOFqv43mjZvP7+NOhzB29pfJIcworHDMRst7 KIS6UaRBmwtFmJa4nrFsnwMHlevS8q7XapW44QvRjXLQO9/hphIvtckK/JXxLoq+Ux9k 3XF5hrkCKWLlpV0fdKeCBZISFczKRvnv1Cu54hiXrqQfqKTtzrdXJbsq2AV9IbvYJrFj 2csg== X-Gm-Message-State: AHPjjUjcEoraWaPgJYHwlLNbF1CRWDRe6ZgaNpdWlrXuI90FqctBxRgR 1SxpobLum+sUacNZ X-Google-Smtp-Source: ADKCNb7840YO0LAU3VUxBFivvvsdi63mzZKoHci6SYQCBKNFJdagmJZdVZfDUGCbFJASIKK+4ZwcHw== X-Received: by 10.223.171.70 with SMTP id r6mr5034834wrc.63.1504514749311; Mon, 04 Sep 2017 01:45:49 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id r14sm3893522wra.80.2017.09.04.01.45.47 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 04 Sep 2017 01:45:48 -0700 (PDT) Date: Mon, 4 Sep 2017 10:45:38 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Matan Azrad Cc: Jingjing Wu , Thomas Monjalon , dev@dpdk.org, Ori Kam , stable@dpdk.org Message-ID: <20170904084538.GB21444@bidouze.vm.6wind.com> References: <1504444747-56298-1-git-send-email-matan@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1504444747-56298-1-git-send-email-matan@mellanox.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix forward port ids setting X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Sep 2017 08:45:50 -0000 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. > 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