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 <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. >
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 >>> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev