> -----Original Message-----
> From: Stokes, Ian <ian.sto...@intel.com>
> Sent: Wednesday 27 April 2022 16:16
> To: Phelan, Michael <michael.phe...@intel.com>; d...@openvswitch.org
> Cc: acon...@redhat.com; david.march...@redhat.com;
> maxime.coque...@redhat.com; Ryan, Seamus <seamus.r...@intel.com>
> Subject: RE: [PATCH] tests: Add ovs-dpdk rate limiting unit tests.
>
> > > From: Seamus Ryan <seamus.r...@intel.com>
> > >
> > > This adds 4 new unit tests to the 'check-dpdk' subsystem that will
> > > test rate limiting functionality.
> > >
> >
> > Thanks for the patch Michael, a few comments below.
> >
> > I'm still conducting some testing so may have some follow up comments
> > for full review.
>
> Hi Michael, had some more time for testing today and have a few more
> follow up comments inline below for the v2.
Hey Ian,
Thanks for the review, I will incorporate your comments and push a V2 ASAP.
I've also left a couple of comments below on things I wasn't sure on that you
might be able to help clarify.
>
> >
> > > Signed-off-by: Seamus Ryan <seamus.r...@intel.com>
> > Probably for the v2 you should add yourself as a Co-author and add
> > your own sign off tag as well.
> >
> > > ---
> > > NEWS | 3 +-
> > > tests/system-dpdk.at | 128
> > > +++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 130 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/NEWS b/NEWS
> > > index 8fa57836a..e1fbc39ed 100644
> > > --- a/NEWS
> > > +++ b/NEWS
> > > @@ -3,7 +3,8 @@ Post-v2.17.0
> > > - OVSDB:
> > > * 'relay' service model now supports transaction history, i.e.
> > > honors
> the
> > > 'last-txn-id' field in 'monitor_cond_since' requests from clients.
> > > -
> > No need to remove the white space above, You can just add the DPDK
> > section straight after the last line above.
> > There should still be 2 white spaces between this section and the
> > v2.17 section below.
> >
> > > + - DPDK:
> > > + * Add rate limiting unit tests to DPDK testsuite
> > Minor, missing period at end of the sentence above.
> >
> > >
> > > v2.17.0 - 17 Feb 2022
> > > ---------------------
> > > diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at index
> > > 7d2715c4a..6d4707f84 100644
> > > --- a/tests/system-dpdk.at
> > > +++ b/tests/system-dpdk.at
> > > @@ -222,6 +222,134 @@ OVS_VSWITCHD_STOP("m4_join([],
> > > [SYSTEM_DPDK_ALLOWED_LOGS], [ AT_CLEANUP dnl
> > > --------------------------------------------------------------------
> > > ------
> > >
> > Just to keep with the existing style for the majority of the unit
> > tests please ensure 3 whitespaces between the dnl above and below here.
> >
> > > +dnl
> > > +-------------------------------------------------------------------
> > > +-------
> > > +dn1 Rate create delete phy port
> > Should this not be dnl instead of dn1?
> >
> > As this is ingress policing I think it would be better to reflect that
> > in the text throughout, you can have rate limiting technically on the
> > QoS (Egress) and on ingress policing also so it would be good to reflect
> which is being checked here.
> >
> > > +AT_SETUP([OVS-DPDK - Rate create delete phy port])
> > > +AT_KEYWORDS([dpdk])
> > > +
> > > +OVS_DPDK_PRE_PHY_SKIP()
> > > +OVS_DPDK_START()
> > > +
> > > +dnl Add userspace bridge and attach it to OVS and add 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])
> > > +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=10000
> > > ingress_policing_burst=1000])
> > > +AT_CHECK([ovs-vsctl show], [], [stdout])
> >
> > What's the purpose of this check? I'm guessing that you are probably
> > looking to confirm the rate and burst levels but I don't see an explicit
> > check
> for it?
> > If so maybe you need to search for the required fields explicitly
> > rather than just the show command itself?
>
> So just to confirm, show command here wont provide useful infor for the
You're right in saying that the show command doesn't show any specific
information relating to the ingress policer but it appears in all other tests
after creating a port on a bridge so the style was copied for these tests.
Do you know of a reason to do this or is it unnecessary?
> inress policer. However we could check the values for burst and rate with
> something like
>
> ovs-vsctl get interface dpdk0 ingress_policing_burst ovs-vsctl get interface
> dpdk0 ingress_policing_rate
>
> To ensure the expected values are there for the given interface?
> >
> > > +sleep 2
> > > +
> > > +dn1 remove policer
> > dn1 should be dnl
> >
> > > +AT_CHECK([ovs-vsctl set interface phy0 ingress_policing_rate=0
> > > ingress_policing_burst=0])
>
> Again we could confirm the rate/burst after it's set to 0 here to ensure it's
> after being set without error.
>
> > > +
> > > +dnl Clean up
> > > +AT_CHECK([ovs-vsctl del-port br10 phy0], [], [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
> > > +])")
> > Not sure why we need the OVS_VSWITCHD_STOP command above to
> check for
> > vhost client connection error, this test only looks at using a
> > standard DPDK PHY device so we shouldn't see this message would be my
> > thinking as we are not adding a vhost port?
> >
> > > +AT_CLEANUP
> > > +dnl
> > > +-------------------------------------------------------------------
> > > +-------
> > > +
> > > +dnl
> > > +-------------------------------------------------------------------
> > > +-------
> > > +dn1 Rate create delete vport port
> >
> > dn1 -> dnl
> >
> > > +AT_SETUP([OVS-DPDK - Rate 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 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])
> > > +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0
> > > ingress_policing_rate=10000 ingress_policing_burst=1000])
> > > +AT_CHECK([ovs-vsctl show], [], [stdout])
>
> Again here this will not confirm the rate/burst, just the interfaces attached
> to
> the bridge.
> Might be an idea to check the rate/burst as flagged above here also.
>
> > > +sleep 2
> > > +
> > > +dn1 remove policer
> >
> > dn1 -> dnl
> >
> > > +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0
> > > ingress_policing_rate=0 ingress_policing_burst=0])
> Same comment as above to confirm the rate/burst here has been reset to 0.
>
> > > +
> > > +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
> > > +])")
> > > +AT_CLEANUP
> > > +dnl
> > > +-------------------------------------------------------------------
> > > +-------
> > > +
> > 3 whitspaces between dnl above and below to separate the unit tests
> >
> > > +dnl
> > > +-------------------------------------------------------------------
> > > +-------
> > > +dn1 Rate no policing rate
> > dn1 - > dnl
> >
> > > +AT_SETUP([OVS-DPDK - Rate no policing rate])
> > > +AT_KEYWORDS([dpdk])
> > > +
> > > +OVS_DPDK_PRE_CHECK()
> > > +OVS_DPDK_START()
> > > +
> > > +dnl Add userspace bridge and attach it to OVS and add 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])
> > > +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0
> > > ingress_policing_burst=1000])
> So the behavior for no value being set for rate is that the existing value for
> rate remains the same. We could try and confirm this here.
>
> > > +AT_CHECK([ovs-vsctl show], [], [stdout]) sleep 2
> > > +
> > > +dn1 check policer was created correctly
> > dn1 -> dnl
> >
> > > +AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [],
> > > +[stdout]) AT_CHECK([egrep 'ingress_policing_burst: 1000' stdout],
> > > +[], [stdout])
> > > +
> > > +AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [],
> > > +[stdout]) AT_CHECK([egrep 'ingress_policing_rate: 0' 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
> > > +])")
> > > +AT_CLEANUP
> > > +dnl
> > > +-------------------------------------------------------------------
> > > +-------
> > > +
> > 3 whitspaces between dnl above and below to separate the unit tests
> >
> > > +dnl
> > > +-------------------------------------------------------------------
> > > +-------
> > > +dn1 Rate no policing burst
> > > +AT_SETUP([OVS-DPDK - Rate no policing burst])
> > > +AT_KEYWORDS([dpdk])
> > > +
> > > +OVS_DPDK_PRE_CHECK()
> > > +OVS_DPDK_START()
> > > +
> > > +dnl Add userspace bridge and attach it to OVS and add 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])
> > > +AT_CHECK([ovs-vsctl set interface dpdkvhostuserclient0
> > > ingress_policing_rate=10000])
> > > +AT_CHECK([ovs-vsctl show], [], [stdout]) sleep 2
> > > +
> > > +dn1 check policer was created correctly AT_CHECK([ovs-vsctl list
> > > +interface dpdkvhostuserclient0], [], [stdout]) AT_CHECK([egrep
> > > +'ingress_policing_burst: 0' stdout], [], [stdout])
> > > +
> > > +AT_CHECK([ovs-vsctl list interface dpdkvhostuserclient0], [],
> > > +[stdout]) AT_CHECK([egrep 'ingress_policing_rate: 10000' stdout],
> > > +[], [stdout])
> > > +
> > So when burst is not defined it should default to 8000 from what I
> > remember, it would be good if we could check this and confirm it in the
> test?
>
> Actually I'm not sure how we can check this as the value will be 0 in the
> database so its difficult to confirm (I believe from the code the default
> value
> will be set to 8000 but this will not be reflected back in the database).
Yes, I couldn't find any command which showed that the burst had been set to
8000 as a default value, is it best to leave it as is in that case?
Thanks,
Michael.
>
> Thanks
> Ian
>
> >
> > Thanks
> > Ian
> >
> > > +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
> > > +])")
> > > +AT_CLEANUP
> > > +dnl
> > > +-------------------------------------------------------------------
> > > +-------
> > > +
> > > dnl
> > > --------------------------------------------------------------------
> > > ------
> > > dnl Add standard DPDK PHY port
> > > AT_SETUP([OVS-DPDK - MFEX Autovalidator])
> > > --
> > > 2.25.1
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
