Hi Kevin I have updated the Scenarios you wanted to check with 'set' and 'get' combinations in V3 😊 even if the dummy-pmd will not changes but still commands can be checked for DPCLS as priority change function will be called no matter what eg:
This command set the max priority and then use the get-command to see if the priority was set : AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set generic 255], [0], [dnl Lookup priority change affected 0 dpcls ports and 0 subtables. ]) AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [], [dnl 255 : generic ]) V3: http://patchwork.ozlabs.org/project/openvswitch/list/?series=259816 Thanks for the get and set idea 😊. And more responses are in-lined. > -----Original Message----- > From: Kevin Traynor <[email protected]> > Sent: Friday, August 27, 2021 3:35 PM > To: Amber, Kumar <[email protected]>; [email protected] > Cc: [email protected] > Subject: Re: [ovs-dev] [PATCH v2] pmd.at: Add test-cases for set and get > commands for DPCLS and DPIF > > 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 <[email protected]> > >> Sent: Thursday, August 26, 2021 8:50 PM > >> To: Amber, Kumar <[email protected]>; [email protected] > >> Cc: [email protected] > >> 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 <[email protected]> > >>> > >>> --- > >>> 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. > Yes fixed in V3. > >>> +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. > > > Now it wont matter we can still verify the functionality using get and set functionality . > 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 > Sure I will . Regards Amber > Kevin. > > > Regards > > Amber > >>> +OVS_VSWITCHD_STOP > >>> +AT_CLEANUP > >>> > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
