| Message ID | 1471943445-20696-1-git-send-email-Frederico.Cadete-ext@oneaccess-net.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Thomas Monjalon |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 243F63792; Tue, 23 Aug 2016 11:10:58 +0200 (CEST) Received: from mail.oneaccess-net.com (mail2.belgium.oneaccess-net.com [91.183.184.101]) by dpdk.org (Postfix) with ESMTP id DBB28378B for <dev@dpdk.org>; Tue, 23 Aug 2016 11:10:56 +0200 (CEST) Received: from frederico-HP-8100.oneaccess.intra (10.0.21.149) by oab1mx2.oneaccess.intra (10.0.24.95) with Microsoft SMTP Server id 14.2.347.0; Tue, 23 Aug 2016 11:10:56 +0200 From: <Frederico.Cadete-ext@oneaccess-net.com> To: <dev@dpdk.org> CC: <frederico@cadete.eu>, Frederico Cadete <Frederico.Cadete-ext@oneaccess-net.com> Date: Tue, 23 Aug 2016 11:10:45 +0200 Message-ID: <1471943445-20696-1-git-send-email-Frederico.Cadete-ext@oneaccess-net.com> X-Mailer: git-send-email 2.7.4 MIME-Version: 1.0 Content-Type: text/plain X-Originating-IP: [10.0.21.149] Subject: [dpdk-dev] [PATCH] testpmd: fix fdir command on MAC and tunnel modes X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Frederico.Cadete-ext@oneaccess-net.com
Aug. 23, 2016, 9:10 a.m. UTC
From: Frederico Cadete <Frederico.Cadete-ext@oneaccess-net.com> The flow_director_filter commands has a pf|vf option for most modes except for MAC-VLAN and tunnel. On Intel NIC's these modes are not supported under virtualized environments. But the application was checking that this field was parsed for these cases, even though this token is not registered with the cmdline parser. This patch skips checking of this field for the commands that don't accept it. Signed-off-by: Frederico Cadete <Frederico.Cadete-ext@oneaccess-net.com> --- app/test-pmd/cmdline.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
Comments
Anyone to review please? 2016-08-23 11:10, Frederico.Cadete-ext@oneaccess-net.com: > The flow_director_filter commands has a pf|vf option for most modes > except for MAC-VLAN and tunnel. On Intel NIC's these modes are not > supported under virtualized environments. > But the application was checking that this field was parsed for these cases, > even though this token is not registered with the cmdline parser. > > This patch skips checking of this field for the commands that don't > accept it. > > Signed-off-by: Frederico Cadete <Frederico.Cadete-ext@oneaccess-net.com>
> -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Frederico.Cadete- > ext@oneaccess-net.com > Sent: Tuesday, August 23, 2016 5:11 PM > To: dev@dpdk.org > Cc: frederico@cadete.eu; Frederico Cadete > Subject: [dpdk-dev] [PATCH] testpmd: fix fdir command on MAC and tunnel > modes > > From: Frederico Cadete <Frederico.Cadete-ext@oneaccess-net.com> > > The flow_director_filter commands has a pf|vf option for most modes > except for MAC-VLAN and tunnel. On Intel NIC's these modes are not > supported under virtualized environments. > But the application was checking that this field was parsed for these cases, > even though this token is not registered with the cmdline parser. > > This patch skips checking of this field for the commands that don't accept it. > > Signed-off-by: Frederico Cadete <Frederico.Cadete-ext@oneaccess-net.com> > --- > app/test-pmd/cmdline.c | 32 ++++++++++++++++++-------------- > 1 file changed, 18 insertions(+), 14 deletions(-) > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index > f90befc..f516b1b 100644 > --- a/app/test-pmd/cmdline.c > +++ b/app/test-pmd/cmdline.c > @@ -8502,24 +8502,28 @@ cmd_flow_director_filter_parsed(void > *parsed_result, > else > entry.action.behavior = RTE_ETH_FDIR_ACCEPT; > > - if (!strcmp(res->pf_vf, "pf")) > - entry.input.flow_ext.is_vf = 0; > - else if (!strncmp(res->pf_vf, "vf", 2)) { > - struct rte_eth_dev_info dev_info; > + if (fdir_conf.mode != RTE_FDIR_MODE_PERFECT_MAC_VLAN && > + fdir_conf.mode != RTE_FDIR_MODE_PERFECT_TUNNEL){ > > - memset(&dev_info, 0, sizeof(dev_info)); > - rte_eth_dev_info_get(res->port_id, &dev_info); > - errno = 0; > - vf_id = strtoul(res->pf_vf + 2, &end, 10); > - if (errno != 0 || *end != '\0' || vf_id >= dev_info.max_vfs) { > + if (!strcmp(res->pf_vf, "pf")) > + entry.input.flow_ext.is_vf = 0; > + else if (!strncmp(res->pf_vf, "vf", 2)) { > + struct rte_eth_dev_info dev_info; > + > + memset(&dev_info, 0, sizeof(dev_info)); > + rte_eth_dev_info_get(res->port_id, &dev_info); > + errno = 0; > + vf_id = strtoul(res->pf_vf + 2, &end, 10); > + if (errno != 0 || *end != '\0' || vf_id >= > dev_info.max_vfs) { > + printf("invalid parameter %s.\n", res->pf_vf); > + return; > + } > + entry.input.flow_ext.is_vf = 1; > + entry.input.flow_ext.dst_id = (uint16_t)vf_id; > + } else { > printf("invalid parameter %s.\n", res->pf_vf); > return; > } > - entry.input.flow_ext.is_vf = 1; > - entry.input.flow_ext.dst_id = (uint16_t)vf_id; > - } else { > - printf("invalid parameter %s.\n", res->pf_vf); > - return; > } Thanks for the patch. But with this change the field of pf_vf cannot omit either. I think it still looks confused because it will allow any meaningless string. In MAC_VLAN or TUNNEL mode, why not just use pf. Maybe an optional field supporting on DPDK cmdline library is exactly what we Are waiting for :) Thanks Jingjing
On Tue, Sep 27, 2016 at 4:42 AM, Wu, Jingjing <jingjing.wu@intel.com> wrote: > > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Frederico.Cadete- >> ext@oneaccess-net.com >> Sent: Tuesday, August 23, 2016 5:11 PM >> To: dev@dpdk.org >> Cc: frederico@cadete.eu; Frederico Cadete >> Subject: [dpdk-dev] [PATCH] testpmd: fix fdir command on MAC and tunnel >> modes >> >> From: Frederico Cadete <Frederico.Cadete-ext@oneaccess-net.com> >> >> The flow_director_filter commands has a pf|vf option for most modes >> except for MAC-VLAN and tunnel. On Intel NIC's these modes are not >> supported under virtualized environments. >> But the application was checking that this field was parsed for these cases, >> even though this token is not registered with the cmdline parser. >> >> This patch skips checking of this field for the commands that don't accept it. >> >> Signed-off-by: Frederico Cadete <Frederico.Cadete-ext@oneaccess-net.com> >> --- >> app/test-pmd/cmdline.c | 32 ++++++++++++++++++-------------- >> 1 file changed, 18 insertions(+), 14 deletions(-) >> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index >> f90befc..f516b1b 100644 >> --- a/app/test-pmd/cmdline.c >> +++ b/app/test-pmd/cmdline.c >> @@ -8502,24 +8502,28 @@ cmd_flow_director_filter_parsed(void >> *parsed_result, >> else >> entry.action.behavior = RTE_ETH_FDIR_ACCEPT; >> >> - if (!strcmp(res->pf_vf, "pf")) >> - entry.input.flow_ext.is_vf = 0; >> - else if (!strncmp(res->pf_vf, "vf", 2)) { >> - struct rte_eth_dev_info dev_info; >> + if (fdir_conf.mode != RTE_FDIR_MODE_PERFECT_MAC_VLAN && >> + fdir_conf.mode != RTE_FDIR_MODE_PERFECT_TUNNEL){ >> >> - memset(&dev_info, 0, sizeof(dev_info)); >> - rte_eth_dev_info_get(res->port_id, &dev_info); >> - errno = 0; >> - vf_id = strtoul(res->pf_vf + 2, &end, 10); >> - if (errno != 0 || *end != '\0' || vf_id >= dev_info.max_vfs) { >> + if (!strcmp(res->pf_vf, "pf")) >> + entry.input.flow_ext.is_vf = 0; >> + else if (!strncmp(res->pf_vf, "vf", 2)) { >> + struct rte_eth_dev_info dev_info; >> + >> + memset(&dev_info, 0, sizeof(dev_info)); >> + rte_eth_dev_info_get(res->port_id, &dev_info); >> + errno = 0; >> + vf_id = strtoul(res->pf_vf + 2, &end, 10); >> + if (errno != 0 || *end != '\0' || vf_id >= >> dev_info.max_vfs) { >> + printf("invalid parameter %s.\n", res->pf_vf); >> + return; >> + } >> + entry.input.flow_ext.is_vf = 1; >> + entry.input.flow_ext.dst_id = (uint16_t)vf_id; >> + } else { >> printf("invalid parameter %s.\n", res->pf_vf); >> return; >> } >> - entry.input.flow_ext.is_vf = 1; >> - entry.input.flow_ext.dst_id = (uint16_t)vf_id; >> - } else { >> - printf("invalid parameter %s.\n", res->pf_vf); >> - return; >> } > > Thanks for the patch. And thanks a lot for the review. > But with this change the field of pf_vf cannot omit either. > I think it still looks confused because it will allow any meaningless string. Sorry, I am not aware that it can be omitted. For MAC/VLAN and tunnel mode it does not and will not allow any meaningless string. At least that was my intention :) The cmdline parser expects "... flexbytes (flexbytes_value) (drop|fwd) queue ..." . This is what is documented [1] and the command's cmdline_parse_inst_t [2] matches this. If you put something in-between "(drop|fwd)" and "queue" it is rejected by the parser in librte_cmdline. > In MAC_VLAN or TUNNEL mode, why not just use pf. With the current code, because if you write that in the command, it is rejected by the parser :) Do you mean it would be preferable to make these commands always take such an argument, and only at the NIC driver check that it must equal PF for MAC_VLAN or TUNNEL mode? The command becomes a bit more complicated for the current intel NIC's, but as I understand it currently does not work anyway. Unless I'm missing something else. > > Maybe an optional field supporting on DPDK cmdline library is exactly what we > Are waiting for :) Laudable goal! My excuses but it's beyond my current skills and bandwith :/ Regards, Frederico [1] http://dpdk.org/browse/dpdk/tree/app/test-pmd/cmdline.c#n703 [2] http://dpdk.org/browse/dpdk/tree/app/test-pmd/cmdline.c#n8821 > > > Thanks > Jingjing
2016-09-27 11:01, Frederico Cadete: > On Tue, Sep 27, 2016 at 4:42 AM, Wu, Jingjing <jingjing.wu@intel.com> wrote: > > From: Frederico.Cadete- > >> The flow_director_filter commands has a pf|vf option for most modes > >> except for MAC-VLAN and tunnel. On Intel NIC's these modes are not > >> supported under virtualized environments. > >> But the application was checking that this field was parsed for these cases, > >> even though this token is not registered with the cmdline parser. > >> > >> This patch skips checking of this field for the commands that don't accept it. > >> > >> Signed-off-by: Frederico Cadete <Frederico.Cadete-ext@oneaccess-net.com> [...] > > > > Thanks for the patch. > > And thanks a lot for the review. > > > But with this change the field of pf_vf cannot omit either. > > I think it still looks confused because it will allow any meaningless string. > > Sorry, I am not aware that it can be omitted. > For MAC/VLAN and tunnel mode it does not and will not allow any > meaningless string. > At least that was my intention :) > > The cmdline parser expects "... flexbytes (flexbytes_value) (drop|fwd) > queue ..." . > This is what is documented [1] and the command's cmdline_parse_inst_t > [2] matches this. > If you put something in-between "(drop|fwd)" and "queue" it is > rejected by the parser > in librte_cmdline. > > > In MAC_VLAN or TUNNEL mode, why not just use pf. > > With the current code, because if you write that in the command, it is > rejected by the parser :) > > Do you mean it would be preferable to make these commands always take > such an argument, > and only at the NIC driver check that it must equal PF for MAC_VLAN or > TUNNEL mode? > The command becomes a bit more complicated for the current intel > NIC's, but as I understand > it currently does not work anyway. Unless I'm missing something else. > > > > > Maybe an optional field supporting on DPDK cmdline library is exactly what we > > Are waiting for :) > > Laudable goal! My excuses but it's beyond my current skills and bandwith :/ Thanks Frederico. Your approach has been re-submitted and fixed by Wenzhuo: http://dpdk.org/patch/16679
diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index f90befc..f516b1b 100644 --- a/app/test-pmd/cmdline.c +++ b/app/test-pmd/cmdline.c @@ -8502,24 +8502,28 @@ cmd_flow_director_filter_parsed(void *parsed_result, else entry.action.behavior = RTE_ETH_FDIR_ACCEPT; - if (!strcmp(res->pf_vf, "pf")) - entry.input.flow_ext.is_vf = 0; - else if (!strncmp(res->pf_vf, "vf", 2)) { - struct rte_eth_dev_info dev_info; + if (fdir_conf.mode != RTE_FDIR_MODE_PERFECT_MAC_VLAN && + fdir_conf.mode != RTE_FDIR_MODE_PERFECT_TUNNEL){ - memset(&dev_info, 0, sizeof(dev_info)); - rte_eth_dev_info_get(res->port_id, &dev_info); - errno = 0; - vf_id = strtoul(res->pf_vf + 2, &end, 10); - if (errno != 0 || *end != '\0' || vf_id >= dev_info.max_vfs) { + if (!strcmp(res->pf_vf, "pf")) + entry.input.flow_ext.is_vf = 0; + else if (!strncmp(res->pf_vf, "vf", 2)) { + struct rte_eth_dev_info dev_info; + + memset(&dev_info, 0, sizeof(dev_info)); + rte_eth_dev_info_get(res->port_id, &dev_info); + errno = 0; + vf_id = strtoul(res->pf_vf + 2, &end, 10); + if (errno != 0 || *end != '\0' || vf_id >= dev_info.max_vfs) { + printf("invalid parameter %s.\n", res->pf_vf); + return; + } + entry.input.flow_ext.is_vf = 1; + entry.input.flow_ext.dst_id = (uint16_t)vf_id; + } else { printf("invalid parameter %s.\n", res->pf_vf); return; } - entry.input.flow_ext.is_vf = 1; - entry.input.flow_ext.dst_id = (uint16_t)vf_id; - } else { - printf("invalid parameter %s.\n", res->pf_vf); - return; } /* set to report FD ID by default */