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 | show |
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 */