Hi Kevin, Thanks a lot for the Reviews. Responses are inlined.
> -----Original Message----- > From: Kevin Traynor <ktray...@redhat.com> > Sent: Thursday, August 26, 2021 8:50 PM > To: Amber, Kumar <kumar.am...@intel.com>; ovs-dev@openvswitch.org > Cc: i.maxim...@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.am...@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@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 > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev