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

[Frederico Cadete <frederico@cadete.eu>]

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