[ovs-dev] [PATCH v2] pmd.at: Add test-cases for set and get commands for DPCLS and DPIF

[Kevin Traynor]

Hi Amber,

On 26/08/2021 18:05, Amber, Kumar wrote:
> Hi Kevin,
> 
> Thanks a lot for the Reviews.
> Responses are inlined.
> 
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor at redhat.com>
>> Sent: Thursday, August 26, 2021 8:50 PM
>> To: Amber, Kumar <kumar.amber at intel.com>; ovs-dev at openvswitch.org
>> Cc: i.maximets at ovn.org
>> Subject: Re: [ovs-dev] [PATCH v2] pmd.at: Add test-cases for set and get
>> commands for DPCLS and DPIF
>>
>> On 25/08/2021 17:36, Kumar Amber wrote:
>>
>> Please shorten the commit title as it is too long and should have "." at the end.
>>
>>> Added 2 separate test-cases for DPCLS and DPIF commands:
>>> 1018: PMD - DPCLS Configuration
>>> 1017: PMD - DPIF Configuration
>>>
>>> Signed-off-by: Kumar Amber <kumar.amber at intel.com>
>>>
>>> ---
>>> v2:
>>> - Moved the test-cases to pmd.at from dpdk suit.
>>> - Removed avx512 specific set commands as per discussion.
>>> ---
>>>  tests/pmd.at | 36 ++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 36 insertions(+)
>>>
>>> diff --git a/tests/pmd.at b/tests/pmd.at index 225d4ee3a..09ae5bcd6
>>> 100644
>>> --- a/tests/pmd.at
>>> +++ b/tests/pmd.at
>>> @@ -1068,3 +1068,39 @@ AT_CHECK([ovs-appctl dpctl/del-dp dummy at dp0],
>>> [0], [dnl
>>>
>>>  OVS_VSWITCHD_STOP
>>>  AT_CLEANUP
>>> +
>>> +AT_SETUP([PMD - DPIF Configuration])
>>
>> minor comment: the other pmd tests avoid using capitals (after "PMD -") and
>> sentence case, so for consistency maybe it can be "PMD - dpif configuration". At
>> least make "configuration" lower case.
>>
> 
> Will fix these in v3.
> 
>>> +OVS_VSWITCHD_START(
>>> +  [], [], [], [--dummy-numa 0,0])
>>> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
>>
>> Are dbg level logs needed?
>>
> 
> Can be disabled not a urgent need.
>  
>>> +AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1
>>> +type=dummy-pmd])
>>> +
>>> +AT_CHECK([ovs-vsctl show], [], [stdout]) AT_CHECK([ovs-appctl
>>> +dpif-netdev/dpif-impl-get], [], [stdout])
>>> +
>>
>> This ensures that the dpif-impl-get command succeeded. Why not also check the
>> output to ensure it is correct.
>>
> 
> Well, it depends on the system. Like if the system has AVX512 than your output will looks different hence avoided as IIya wanted a test-case
> That call all the commands both set and set but those should not be dependent on the specific ISAs.
> 

I agree it should not be dependent on ISA. The question is, are there
important parts of the output/defaults that are common regardless of ISA
that could be checked? e.g. should it always select dpif_scalar for
those pmds by default and that line can that be checked, regardless of
other options that may be available depending on ISA in the output.

>>> +AT_CHECK([ovs-appctl dpif-netdev/dpif-impl-set dpif_scalar], [0],
>>> +[dnl DPIF implementation set to dpif_scalar.
>>> +])
>>> +
>>> +OVS_VSWITCHD_STOP
>>> +AT_CLEANUP
>>> +
>>> +AT_SETUP([PMD - DPCLS Configuration])
>>
>> same comment as above re naming
>>
> 
> Sure will fix it in v3.
>  
>>> +OVS_VSWITCHD_START(
>>> +  [], [], [], [--dummy-numa 0,0])
>>> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
>>> +AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1
>>> +type=dummy-pmd])
>>> +
>>> +AT_CHECK([ovs-vsctl show], [], [stdout]) AT_CHECK([ovs-appctl
>>> +dpif-netdev/subtable-lookup-prio-get], [], [stdout])
>>> +
>>
>> This ensures that the command succeeds. Why not also check the output to
>> ensure correct options and default priorities (maybe it should only check for
>> presence of autovalidator and generic)
>>
> 
> Yes sure will add those checks here .
> 
>>> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set
>>> +autovalidator 3], [0], [dnl Lookup priority change affected 0 dpcls ports and 0
>> subtables.
>>> +])
>>> +
>>
>> As above, you can use dpif-netdev/subtable-lookup-prio-get to check it has been
>> set correctly.
>>
> 
> True if you look at the v1 we were matching on the output but those were like not dummy-pmd tests.
> Dummy-pmd does not allow the switching of DPCLS at all but that should not a hinderance as the motive of
> The test-case remains to test that the commands are called and does not return a crash or error.
>  

ok, I didn't check that, but if it doesn't have an effect with
dummy-pmds then i agree there is no need to check the output.

>>> +AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 4],
>>> +[0], [dnl Lookup priority change affected 0 dpcls ports and 0 subtables.
>>> +])
>>> +
>>
>> As above, you can use dpif-netdev/subtable-lookup-prio-get to check it has been
>> set correctly.
>>
>> You could check the behaviour in additional ways. e.g.
>> - Check priorities can be changed again
>> - Check if priorities can be the same
>> - Check working with min prio, check behaviour with below prio
>> - Check working with max prio, check behaviour with above prio
>>
> 
> Yes all the combinations are possible but on dummy-pmd I doubt DPCLS fn-pointer switching works and hence kept it just test the basic commands.
> 

Right, i'm not sure about it either, did you try it? You could run the
test with debug (make check TESTSUITEFLAGS='1019 -d -v') and check the
logs in ./tests/testsuite.dir/1019

Kevin.

> Regards
> Amber
>>> +OVS_VSWITCHD_STOP
>>> +AT_CLEANUP
>>>
>