> -----Original Message-----
> From: Stokes, Ian <ian.sto...@intel.com>
> Sent: Friday 15 July 2022 11:25
> To: Phelan, Michael <michael.phe...@intel.com>; Pai G, Sunil
> <sunil.pa...@intel.com>; d...@openvswitch.org
> Cc: maxime.coque...@redhat.com; i.maxim...@ovn.org
> Subject: RE: [ovs-dev] [PATCH v2] tests: Add OVS-DPDK QoS unit tests
>
> > Hey Sunil,
> > Thanks for the comments, I've left some responses inline. Let me know
> > what you think.
> >
> > > -----Original Message-----
> > > From: Pai G, Sunil <sunil.pa...@intel.com>
> > > Sent: Wednesday 13 July 2022 14:51
> > > To: Phelan, Michael <michael.phe...@intel.com>; d...@openvswitch.org
> > > Cc: maxime.coque...@redhat.com; i.maxim...@ovn.org
> > > Subject: RE: [ovs-dev] [PATCH v2] tests: Add OVS-DPDK QoS unit tests
> > >
> > > Hi Mike,
> > >
> > > Thanks for adding more testcases !
> > > Patch generally looks good, although it might require a rebase.
> >
> > Yep, will definitely need a rebase before merging.
> >
> > > Minor nits inline.
> > >
> > >
> > > > -----Original Message-----
> > > > From: dev <ovs-dev-boun...@openvswitch.org> On Behalf Of Michael
> > > > Phelan
> > > > Sent: Wednesday, June 29, 2022 4:19 PM
> > > > To: d...@openvswitch.org
> > > > Cc: maxime.coque...@redhat.com; i.maxim...@ovn.org
> > > > Subject: [ovs-dev] [PATCH v2] tests: Add OVS-DPDK QoS unit tests
> > > >
> > > > This adds 4 new unit tests to the 'check-dpdk' subsystem that will
> > > > test Quality of Service (QoS) functionality.
> > > >
> > > > Signed-off-by: Michael Phelan <michael.phe...@intel.com>
> > > >
> > > > ---
> > > > v2:
> > > > - Added check to confirm policer was removed correctly in phy
> > > > and vport tests.
> > > > - Removed unnecessary error catch in phy test.
> > > > - Added egress to various comments to improve clarity.
> > > > ---
> > > > ---
> > > > tests/system-dpdk.at | 147
> > > > +++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 147 insertions(+)
> > > >
> > > > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index
> > > > 7d2715c4a..41987b7b5 100644
> > > > --- a/tests/system-dpdk.at
> > > > +++ b/tests/system-dpdk.at
> > > > @@ -222,6 +222,153 @@ OVS_VSWITCHD_STOP("m4_join([],
> > > > [SYSTEM_DPDK_ALLOWED_LOGS], [ AT_CLEANUP dnl
> > > > ---------------------------
> > > > -----------------------------------------------
> > > >
> > > > +
> > > > +
> > > > +dnl
> > > > +-----------------------------------------------------------------
> > > > +----
> > > > +--
> > > > +---
> > > > +dnl QoS create delete phy port
> > > > +AT_SETUP([OVS-DPDK - QoS create delete phy port])
> > >
> > > Do we want to call this QOS or egress policing ? or both ? (QoS /
> > > Egress
> > policing)
> > > Don’t have a strong preference here but just to make the life of a
> > > tester
> > easier I
> > > would include egress policing in the name.
> > > This way: make check-dpdk TESTSUITEFLAGS="-k policing" would run all
> > policing
> > > test cases(ingress/rate limiting as well as egress/QOS)
> >
> > I agree with you here, I think it make sense to call them egress
> > policing tests to keep it in line with the ingress tests and hopefully
> > make it a bit easier for end users. I will change this is in the next
> > version.
>
> Not sure I agree with this, egress policing is specifically part of QoS which
> is
> handled separately to rate-limiting, even as part of the vswitch.xml these are
> separate concepts and separate in the code base for netdev-dpdk, it's just
> coincidence that there happens to be an egress policer implementation for QoS.
> I would prefer to see a section in the future for QoS specifically that runs
> all QoS
> tests if that makes sense rather than mixing the policers.
No problem, will leave the naming as QoS for the next version.
>
> > >
> > >
> > > > +AT_KEYWORDS([dpdk])
> > > > +
> > > > +OVS_DPDK_PRE_PHY_SKIP()
> > > > +OVS_DPDK_START()
> > > > +
> > > > +dnl Add userspace bridge and attach it to OVS and add egress
> > > > +policer AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > > > +datapath_type=netdev]) AT_CHECK([ovs-vsctl add-port br10 phy0 --
> > set
> > > > +Interface phy0 type=dpdk options:dpdk-devargs=$(cat PCI_ADDR)],
> > > > +[], [stdout], [stderr]) OVS_WAIT_UNTIL([ovs-vsctl set port phy0
> > qos=@newqos
> > > > +-- --id=@newqos create qos type=egress-policer other-
> > config:cir=1250000
> > > > +other-config:cbs=2048]) AT_CHECK([ovs-appctl -t ovs-vswitchd
> > qos/show
> > > > +phy0], [], [stdout]) sleep 2
> > > > +
> > > > +dnl Fail if egress policer could not be created AT_FAIL_IF([grep
> > > > +"Could not create rte meter for egress policer"
> > > > +ovs-vswitchd.log], [],
> > > > +[stdout])
> > > > +
> > > > +dnl remove egress policer
> > >
> > > Please capitalize the first word. [please check this on the entire
> > > patch]
> > >
> > > > +AT_CHECK([ovs-vsctl destroy QoS phy0 -- clear Port phy0 qos])
> > > > +
> > > > +dnl check egress policer was removed correctly
> > > > +AT_CHECK([ovs-appctl -
> > t
> > > > +ovs-vswitchd qos/show phy0], [], [stdout]) AT_CHECK([egrep 'QoS
> > > > +not configured on phy0' stdout], [], [stdout])
> > > > +
> > > > +dnl Clean up
> > > > +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [stdout], [stderr])
> > > > +OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
> > > > +AT_CLEANUP
> > > > +dnl
> > > > +-----------------------------------------------------------------
> > > > +------
> > > > +---
> > > > +
> > > > +
> > > > +
> > > > +dnl
> > > > +-----------------------------------------------------------------
> > > > +------
> > > > +---
> > > > +dnl QoS create delete vport port
> > > > +AT_SETUP([OVS-DPDK - QoS create delete vport port])
> > > > +AT_KEYWORDS([dpdk])
> > > > +
> > > > +OVS_DPDK_PRE_CHECK()
> > > > +OVS_DPDK_START()
> > > > +
> > > > +dnl Add userspace bridge and attach it to OVS and add egress
> > > > +policer AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > > > +datapath_type=netdev]) AT_CHECK([ovs-vsctl add-port br10
> > > > +dpdkvhostuserclient0 -- set Interface dpdkvhostuserclient0
> > > > type=dpdkvhostuserclient options:vhost-server-
> > > > path=$OVS_RUNDIR/dpdkvhostclient0], [], [stdout], [stderr])
> > > > OVS_WAIT_UNTIL([ovs-vsctl set port dpdkvhostuserclient0
> > qos=@newqos -- --
> > > > id=@newqos create qos type=egress-policer other-config:cir=1250000
> > > > \
> > > > + other-config:cbs=2048]) AT_CHECK([ovs-appctl -t
> > > > +ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout]) sleep
> > > > +2
> > > > +
> > > > +dnl Fail if egress policer could not be created AT_FAIL_IF([grep
> > > > +"Could not create rte meter for egress policer"
> > > > +ovs-vswitchd.log], [],
> > > > +[stdout])
> > > > +
> > > > +dnl remove egress policer
> > > > +AT_CHECK([ovs-vsctl destroy QoS dpdkvhostuserclient0 -- clear
> > > > +Port
> > > > +dpdkvhostuserclient0 qos])
> > > > +
> > > > +dnl check egress policer was removed correctly
> > > > +AT_CHECK([ovs-appctl -
> > t
> > > > +ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
> > > > +AT_CHECK([egrep 'QoS not configured on dpdkvhostuserclient0'
> > stdout],
> > > > +[], [stdout])
> > > > +
> > > > +dnl Parse log file
> > > > +AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created"
> > > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "vHost User
> > > > +device 'dpdkvhostuserclient0' created in 'client' mode, using client
> > > > socket"
> > > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "VHOST_CONFIG:
> > > > +$OVS_RUNDIR/dpdkvhostclient0: reconnecting..." ovs-vswitchd.log],
> > > > +[],
> > > > +[stdout])
> > >
> > > Although this sequence works just fine, we could move the above
> > checks(Parse
> > > log file) just after adding the vhost port.
> > > Followed by qos port add and the checks for this addition.
> >
> > Makes sense, I will rearrange these in the next version.
> >
> > >
> > >
> > > > +
> > > > +dnl Clean up
> > > > +AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [],
> > > > +[stdout],
> > > > +[stderr]) OVS_VSWITCHD_STOP("m4_join([],
> > > [SYSTEM_DPDK_ALLOWED_LOGS], [
> > > > +\@VHOST_CONFIG: failed to connect to
> > $OVS_RUNDIR/dpdkvhostclient0:
> > > No
> > > > +such file or directory@d
> > > > +])")
> > > > +AT_CLEANUP
> > > > +dnl
> > > > +-----------------------------------------------------------------
> > > > +------
> > > > +---
> > > > +
> > > > +
> > > > +
> > > > +dnl
> > > > +-----------------------------------------------------------------
> > > > +------
> > > > +---
> > > > +dnl QoS no cir
> > > > +AT_SETUP([OVS-DPDK - QoS no cir])
> > > > +AT_KEYWORDS([dpdk])
> > > > +
> > > > +OVS_DPDK_PRE_CHECK()
> > > > +OVS_DPDK_START()
> > > > +
> > > > +dnl Add userspace bridge and attach it to OVS and add egress
> > > > +policer AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > > > +datapath_type=netdev]) AT_CHECK([ovs-vsctl add-port br10
> > > > +dpdkvhostuserclient0 -- set Interface dpdkvhostuserclient0
> > > > +type=dpdkvhostuserclient
> > > > +options:vhost-server-path=$OVS_RUNDIR/dpdkvhostclient0], [],
> > [stdout],
> > > > +[stderr]) OVS_WAIT_UNTIL([ovs-vsctl set port dpdkvhostuserclient0
> > > > +qos=@newqos -- --id=@newqos create qos type=egress-policer
> > > > +other-config:cbs=2048]) sleep 2
> > > > +
> > > > +dnl check egress policer was not created AT_CHECK([ovs-appctl -t
> > > > +ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
> > > > +AT_CHECK([egrep 'QoS not configured on dpdkvhostuserclient0'
> > stdout],
> > > > +[], [stdout])
> > > > +
> > > > +dnl Parse log file
> > > > +AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created"
> > > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "vHost User
> > > > +device 'dpdkvhostuserclient0' created in 'client' mode, using client
> > > > socket"
> > > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "VHOST_CONFIG:
> > > > +$OVS_RUNDIR/dpdkvhostclient0: reconnecting..." ovs-vswitchd.log],
> > > > +[],
> > > > +[stdout])
> > > > +
> > > > +dnl Clean up
> > > > +AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [],
> > > > +[stdout],
> > > > +[stderr]) OVS_VSWITCHD_STOP("m4_join([],
> > > [SYSTEM_DPDK_ALLOWED_LOGS], [
> > > > +\@VHOST_CONFIG: failed to connect to
> > $OVS_RUNDIR/dpdkvhostclient0:
> > > No
> > > > +such file or directory@d \@Could not create rte meter for egress
> > > > +policer@d \@Failed to set QoS type egress-policer on port
> > > > +dpdkvhostuserclient0: Invalid argument@d
> > > > +])")
> > > > +AT_CLEANUP
> > > > +dnl
> > > > +-----------------------------------------------------------------
> > > > +------
> > >
> > > Maybe we should combine last three test cases into 1 ? WDYT ?
> > > But here we must be careful while we grep the string in
> > > ovs-vswitchd.log
> > as logs
> > > might be repetitive.
> > > We could borrow the following trick used for few testscases in
> > > pmd.at to
> > grep
> > > from the logical end of the prev part of the test:
> > > TMP=$(($(cat ovs-vswitchd.log | wc -l | tr -d [[:blank:]])+1))
> > > OVS_WAIT_UNTIL([tail -n +$TMP ovs-vswitchd.log | grep
> > > "<any_string>"])
> >
> > I'm open to doing that as there is a lot of repetition of commands in
> > the 3 tests but I thought it was better to make unit tests test a
> > specific functionality to allow for easy debugging on a failure. Ilya
> > or Ian do you have any thoughts on the best course of action?
>
> +1 on making unit tests specific functionality, this follows what we have
> already
> for the rate-limiting tests so is in keeping with what is already in place.
Sounds good, I will leave them as is for the next version.
>
>
> >
> > >
> > >
> > > > +---
> > > > +
> > > > +
> > > > +
> > > > +dnl
> > > > +-----------------------------------------------------------------
> > > > +------
> > > > +---
> > > > +dnl QoS no cbs
> > > > +AT_SETUP([OVS-DPDK - QoS no cbs])
> > > > +AT_KEYWORDS([dpdk])
> > > > +
> > > > +OVS_DPDK_PRE_CHECK()
> > > > +OVS_DPDK_START()
> > > > +
> > > > +dnl Add userspace bridge and attach it to OVS and add egress
> > > > +policer AT_CHECK([ovs-vsctl add-br br10 -- set bridge br10
> > > > +datapath_type=netdev]) AT_CHECK([ovs-vsctl add-port br10
> > > > +dpdkvhostuserclient0 -- set Interface dpdkvhostuserclient0
> > > > +type=dpdkvhostuserclient
> > > > +options:vhost-server-path=$OVS_RUNDIR/dpdkvhostclient0], [],
> > [stdout],
> > > > +[stderr]) OVS_WAIT_UNTIL([ovs-vsctl set port dpdkvhostuserclient0
> > > > +qos=@newqos -- --id=@newqos create qos type=egress-policer
> > > > +other-config:cir=1250000]) sleep 2
> > > > +
> > > > +dnl check egress policer was not created AT_CHECK([ovs-appctl -t
> > > > +ovs-vswitchd qos/show dpdkvhostuserclient0], [], [stdout])
> > > > +AT_CHECK([egrep 'QoS not configured on dpdkvhostuserclient0'
> > stdout],
> > > > +[], [stdout])
> > > > +
> > > > +
> > > > +dnl Parse log file
> > > > +AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created"
> > > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "vHost User
> > > > +device 'dpdkvhostuserclient0' created in 'client' mode, using client
> > > > socket"
> > > > +ovs-vswitchd.log], [], [stdout]) AT_CHECK([grep "VHOST_CONFIG:
> > > > +$OVS_RUNDIR/dpdkvhostclient0: reconnecting..." ovs-vswitchd.log],
> > > > +[],
> > > > +[stdout])
> > > > +
> > > > +dnl Clean up
> > > > +AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [],
> > > > +[stdout],
> > > > +[stderr]) OVS_VSWITCHD_STOP("m4_join([],
> > > [SYSTEM_DPDK_ALLOWED_LOGS], [
> > > > +\@VHOST_CONFIG: failed to connect to
> > $OVS_RUNDIR/dpdkvhostclient0:
> > > No
> > > > +such file or directory@d \@Could not create rte meter for egress
> > > > +policer@d \@Failed to set QoS type egress-policer on port
> > > > +dpdkvhostuserclient0: Invalid argument@d
> > > > +])")
> > > > +AT_CLEANUP
> > > > +dnl
> > > > +-----------------------------------------------------------------
> > > > +------
> > > > +---
> > > > +
> > >
> > >
> > > We should also consider adding trtcm-policer tests too in future.
> > > But
> > these are
> > > good additions for now 😊
>
> Agree here, I think this will be targeted for 2.19.
>
> Thanks
> Ian
> > >
> > >
> > > Thanks and regards
> > > Sunil
> > Thanks,
> > Michael.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
