diff mbox

[dpdk-dev] testpmd: fix fdir command on MAC and tunnel modes

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

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

Thomas Monjalon Sept. 23, 2016, 6:37 p.m. UTC | #1 
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>
Jingjing Wu Sept. 27, 2016, 2:42 a.m. UTC | #2 
> -----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
Frederico Cadete Sept. 27, 2016, 9:01 a.m. UTC | #3 
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
Thomas Monjalon Oct. 25, 2016, 9:14 p.m. UTC | #4 
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 mbox

Patch

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