On 6 Jul 2021, at 17:32, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Eelco Chaudron <echau...@redhat.com>
>> Sent: Tuesday, July 6, 2021 3:19 PM
>> To: Ferriter, Cian <cian.ferri...@intel.com>
>> Cc: ovs-dev@openvswitch.org; f...@sysclose.org; i.maxim...@ovn.org; Van
>> Haaren,
>> Harry <harry.van.haa...@intel.com>; Amber, Kumar <kumar.am...@intel.com>;
>> Stokes, Ian <ian.sto...@intel.com>
>> Subject: Re: [v5 01/11] dpif-netdev: Add command line and function pointer
>> for
>> miniflow extract
>>
>> See comments inline...
>
> <snip many comments, Amber will address smaller/code-level changes>
>
>>> + return;
>>> + }
>>
>>
>> Argument handling is not as it should be, see my previous comment. I think
>> the
>> packets count should only be available for the study option (this might not
>> be the
>> correct patch, but just want to make sure it’s addressed, and I do not
>> forget).
>>
>> So as an example this looks odd trying to set it for a specific PMD:
>>
>> $ ovs-appctl dpif-netdev/miniflow-parser-set autovalidator 15 1
>> Miniflow implementation set to autovalidator, on pmd thread 1
>>
>> Why do I have to put in the dummy value 15. Here is a quote from my previous
>> comment:
>>
>> “
>> We also might need to re-think the command to make sure
>> packet_count_to_study
>> is only needed for the study command.
>> So the help text might become something like:
>>
>> dpif-netdev/miniflow-parser-set {miniflow_implementation_name | study
>> [pkt_cnt]} [dp] [pmd_core]
>
>
> I don't particularly like the "insert extra variable" with PMD_core moving up
> an index if study is used.
> Special casing specific implementations like that (if study then change
> indexes) is nasty.
>
> (Note that based on Flavio's feedback the [dp] argument was removed.)
>
> Thoughts on the this suggestion:
> $ dpif-netdev/miniflow-parser-set miniflow_implementation_name [pmd_core]
> [pkt_cnt]
>
> Notes:
> 1) All arguments are positional, optional arguments at the end
> 2) Based on "power-user-ness", the command required will get longer
> - simple usage is simple, with just $ miniflow-parser-set <impl_name>
> 3) The worst-part is that to specify [pkt_cnt] to study, it also requires
> setting [pmd_core]. Given [pkt_cnt] is a power-user option, I think this is
> the right compromise.
>
> Agree to implement & merge the above?
You are right, and your suggestion eases the general use case but makes the
pkt_cnt option hard to use, as you have to execute the command for each PMD.
I was looking at other commands with the same problem, and they use a -pmd
keyword approach. Some examples:
dpif-netdev/pmd-rxq-show [-pmd core] [dp]
dpif-netdev/pmd-stats-clear [-pmd core] [dp]
dpif-netdev/pmd-stats-show [-pmd core] [dp]
Guess we can do the same here:
dpif-netdev/miniflow-parser-set [-pmd core] miniflow_implementation_name
[pkt_cnt]
> <snip remainder of feedback>.
>
> Regards, -Harry
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
