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

[Amber, Kumar]

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.

> > +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.
 
> > +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.

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