diff mbox series

[ovs-dev,v2,2/2] testsuite: add test cases for ingress_policing parameters

Message ID 20210312115917.6838-3-simon.horman@netronome.com
State Changes Requested
Headers show
Series netdev-linux: correct unit of burst parameter | expand

Commit Message

Simon Horman March 12, 2021, 11:59 a.m. UTC 
From: Tianyu Yuan <tianyu.yuan@corigine.com>

* Exercise OVS setting of ingress_policing parameters using ovs-vsctl and
  verify that the correct values are stored on OVSDB.
* Verify the ingress_policing parameters with tc command. Also check
  offload and non-offload in tc software datapath based on tc filter type
  (matchall and basic).

Example invocation:
make check TESTSUITEFLAGS='-k ingress_policing'
make check-offloads TESTSUITEFLAGS='-k ingress_policing'

Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com>
Signed-off-by: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Louis Peens <louis.peens@netronome.com>
---
 tests/ovs-vsctl.at               | 58 ++++++++++++++++++++++++++++++++
 tests/system-offloads-traffic.at | 47 ++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)

Comments

0-day Robot March 12, 2021, 2 p.m. UTC | #1 
Bleep bloop.  Greetings Simon Horman, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Simon Horman <simon.horman@netronome.com>, Louis Peens <louis.peens@netronome.com>
Lines checked: 147, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Tonghao Zhang March 15, 2021, 3:20 a.m. UTC | #2 
On Fri, Mar 12, 2021 at 8:00 PM Simon Horman <simon.horman@netronome.com> wrote:
>
> From: Tianyu Yuan <tianyu.yuan@corigine.com>
>
> * Exercise OVS setting of ingress_policing parameters using ovs-vsctl and
>   verify that the correct values are stored on OVSDB.
> * Verify the ingress_policing parameters with tc command. Also check
>   offload and non-offload in tc software datapath based on tc filter type
>   (matchall and basic).
>
> Example invocation:
> make check TESTSUITEFLAGS='-k ingress_policing'
> make check-offloads TESTSUITEFLAGS='-k ingress_policing'
>
> Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> Signed-off-by: Louis Peens <louis.peens@netronome.com>
> ---
>  tests/ovs-vsctl.at               | 58 ++++++++++++++++++++++++++++++++
>  tests/system-offloads-traffic.at | 47 ++++++++++++++++++++++++++
>  2 files changed, 105 insertions(+)
>
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index d2cb41403..790d46f18 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -1659,3 +1659,61 @@ AT_CHECK([grep "server name" ovsdb-server.log], [0],
>
>  OVS_VSCTL_CLEANUP
>  AT_CLEANUP
> +
> +dnl ----------------------------------------------------------------------
> +AT_BANNER([set ingress policing test])
> +
> +AT_SETUP([set ingress_policing_rate and ingress_policing_burst])
> +AT_KEYWORDS([ingress_policing])
> +OVS_VSCTL_SETUP
> +AT_CHECK([RUN_OVS_VSCTL_TOGETHER(
> +   [add-br a],
> +   [add-port a a1],
> +   [set interface a1 ingress_policing_rate=100],
> +   [set interface a1 ingress_policing_burst=10],
> +   [list interface a1 > configured_interface_a1.txt])],
I think we only care about options "ingress_policing_xxx" and if "cat"
the informations to a file, we should delete it when we don't use it
anymore.
So use the command for example as below, to make the codes more simplified.
ovs-vsctl --columns=ingress_policing_burst,ingress_policing_rate list
interface enp1s0f0
ingress_policing_burst: 10
ingress_policing_rate: 100

> +   [0], [])
> +AT_CHECK(
> +    [uuidfilt configured_interface_a1.txt],
> +    [0],
> +    [[
> +
> +
> +
> +_uuid               : <0>
> +admin_state         : []
> +bfd                 : {}
> +bfd_status          : {}
> +cfm_fault           : []
> +cfm_fault_status    : []
> +cfm_flap_count      : []
> +cfm_health          : []
> +cfm_mpid            : []
> +cfm_remote_mpids    : []
> +cfm_remote_opstate  : []
> +duplex              : []
> +error               : []
> +external_ids        : {}
> +ifindex             : []
> +ingress_policing_burst: 10
> +ingress_policing_rate: 100
> +lacp_current        : []
> +link_resets         : []
> +link_speed          : []
> +link_state          : []
> +lldp                : {}
> +mac                 : []
> +mac_in_use          : []
> +mtu                 : []
> +mtu_request         : []
> +name                : a1
> +ofport              : []
> +ofport_request      : []
> +options             : {}
> +other_config        : {}
> +statistics          : {}
> +status              : {}
> +type                : ""
> +]])
> +OVS_VSCTL_CLEANUP
> +AT_CLEANUP
> diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
> index 4f601ef93..b9cd29c95 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -70,3 +70,50 @@ AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [i
>
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst - offloads disabled])
> +AT_KEYWORDS([ingress_policing])
> +OVS_TRAFFIC_VSWITCHD_START()
> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=false])
> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> +ADD_NAMESPACES(at_ns0)
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_rate=100])
> +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_burst=10])
> +AT_CHECK([ovs-vsctl list open | grep other_config > other_config.txt])
> +AT_CHECK([cat other_config.txt],[0],
> +    [other_config        : {hw-offload="false"}
> +])
> +AT_CHECK([tc -s -d filter show dev ovs-p0 ingress |grep rate| awk '{a=index($0,"rate");b=index($0,"mtu");print substr($0,a,b-a-1)}' > tc_ovs-p0.txt ],[0],[])
> +AT_CHECK([cat tc_ovs-p0.txt],[0],
> +    [rate 100Kbit burst 1280b
I am not sure, should we use the tc command to check offload. if so,
OvS packets will depend iproute2 ?
> +])
> +AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | grep basic | head -n 1 | awk '{a=index($0,"basic");print substr($0,a,5)}'| head -n 1],[0],
> +    [basic
> +])
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +
> +AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst - offloads enabled])
> +AT_KEYWORDS([ingress_policing])
> +OVS_TRAFFIC_VSWITCHD_START()
> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true])
> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> +ADD_NAMESPACES(at_ns0)
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_rate=100])
> +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_burst=10])
> +AT_CHECK([ovs-vsctl list open | grep other_config > other_config.txt])
> +AT_CHECK([cat other_config.txt],[0],
> +    [other_config        : {hw-offload="true"}
> +])
> +AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | grep rate| awk '{a=index($0,"rate");b=index($0,"mtu");print substr($0,a,b-a-1)}' > tc_ovs-p0.txt ],[0],[])
> +AT_CHECK([cat tc_ovs-p0.txt],[0],
> +    [rate 100Kbit burst 1280b
> +])
> +AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | grep matchall | head -n 1 | awk '{a=index($0,"matchall");print substr($0,a,8)}'| head -n 1],[0],
> +    [matchall
> +])
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> --
> 2.20.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Marcelo Leitner March 25, 2021, 7:21 p.m. UTC | #3 
On Fri, Mar 12, 2021 at 12:59:17PM +0100, Simon Horman wrote:
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -70,3 +70,50 @@ AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [i
>  
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst - offloads disabled])
> +AT_KEYWORDS([ingress_policing])
> +OVS_TRAFFIC_VSWITCHD_START()
> +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=false])
> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> +ADD_NAMESPACES(at_ns0)
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_rate=100])
> +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_burst=10])
> +AT_CHECK([ovs-vsctl list open | grep other_config > other_config.txt])
> +AT_CHECK([cat other_config.txt],[0],
> +    [other_config        : {hw-offload="false"}
> +])
> +AT_CHECK([tc -s -d filter show dev ovs-p0 ingress |grep rate| awk '{a=index($0,"rate");b=index($0,"mtu");print substr($0,a,b-a-1)}' > tc_ovs-p0.txt ],[0],[])

Uhh.. I wanted to say "just use tc -json ... | jq ..." to avoid all this
parsing, but the action police doesn't support it yet. :-(

> +AT_CHECK([cat tc_ovs-p0.txt],[0],
> +    [rate 100Kbit burst 1280b

This ties back to my previous email. Is 1280b really what is expected
here? (for both flavors)
If 100K was kept as 100K, seems 'K' is what is wanted.
Then, the burst would be 1250b.

While reading the code on this, now noticed that in
netdev_dpdk_policer_construct() it uses 1000 for both.

Anyhow, good that this is now getting asserted.

Other than this and comments already made by Tonghao Zhang, I have no
further comments.

Thanks,
Marcelo
Simon Horman April 1, 2021, 7:08 a.m. UTC | #4 
On Thu, Mar 25, 2021 at 04:21:34PM -0300, Marcelo Ricardo Leitner wrote:
> On Fri, Mar 12, 2021 at 12:59:17PM +0100, Simon Horman wrote:
> > --- a/tests/system-offloads-traffic.at
> > +++ b/tests/system-offloads-traffic.at
> > @@ -70,3 +70,50 @@ AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [i
> >  
> >  OVS_TRAFFIC_VSWITCHD_STOP
> >  AT_CLEANUP
> > +
> > +AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst - offloads disabled])
> > +AT_KEYWORDS([ingress_policing])
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=false])
> > +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> > +ADD_NAMESPACES(at_ns0)
> > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> > +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_rate=100])
> > +AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_burst=10])
> > +AT_CHECK([ovs-vsctl list open | grep other_config > other_config.txt])
> > +AT_CHECK([cat other_config.txt],[0],
> > +    [other_config        : {hw-offload="false"}
> > +])
> > +AT_CHECK([tc -s -d filter show dev ovs-p0 ingress |grep rate| awk '{a=index($0,"rate");b=index($0,"mtu");print substr($0,a,b-a-1)}' > tc_ovs-p0.txt ],[0],[])
> 
> Uhh.. I wanted to say "just use tc -json ... | jq ..." to avoid all this
> parsing, but the action police doesn't support it yet. :-(
> 
> > +AT_CHECK([cat tc_ovs-p0.txt],[0],
> > +    [rate 100Kbit burst 1280b
> 
> This ties back to my previous email. Is 1280b really what is expected
> here? (for both flavors)
> If 100K was kept as 100K, seems 'K' is what is wanted.
> Then, the burst would be 1250b.
> 
> While reading the code on this, now noticed that in
> netdev_dpdk_policer_construct() it uses 1000 for both.
> 
> Anyhow, good that this is now getting asserted.
> 
> Other than this and comments already made by Tonghao Zhang, I have no
> further comments.

Thanks. I did not notice Tonghao Zhang's comments until after I sent my
"ping, please review" email to you. But we do see them. And we are working
on resolving that problem - the test should be skipped if not supported
by the environment, AFAIC.
diff mbox series

Patch

diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index d2cb41403..790d46f18 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -1659,3 +1659,61 @@  AT_CHECK([grep "server name" ovsdb-server.log], [0],
 
 OVS_VSCTL_CLEANUP
 AT_CLEANUP
+
+dnl ----------------------------------------------------------------------
+AT_BANNER([set ingress policing test])
+
+AT_SETUP([set ingress_policing_rate and ingress_policing_burst])
+AT_KEYWORDS([ingress_policing])
+OVS_VSCTL_SETUP
+AT_CHECK([RUN_OVS_VSCTL_TOGETHER(
+   [add-br a],
+   [add-port a a1],
+   [set interface a1 ingress_policing_rate=100],
+   [set interface a1 ingress_policing_burst=10],
+   [list interface a1 > configured_interface_a1.txt])],
+   [0], [])
+AT_CHECK(
+    [uuidfilt configured_interface_a1.txt],
+    [0],
+    [[
+
+
+
+_uuid               : <0>
+admin_state         : []
+bfd                 : {}
+bfd_status          : {}
+cfm_fault           : []
+cfm_fault_status    : []
+cfm_flap_count      : []
+cfm_health          : []
+cfm_mpid            : []
+cfm_remote_mpids    : []
+cfm_remote_opstate  : []
+duplex              : []
+error               : []
+external_ids        : {}
+ifindex             : []
+ingress_policing_burst: 10
+ingress_policing_rate: 100
+lacp_current        : []
+link_resets         : []
+link_speed          : []
+link_state          : []
+lldp                : {}
+mac                 : []
+mac_in_use          : []
+mtu                 : []
+mtu_request         : []
+name                : a1
+ofport              : []
+ofport_request      : []
+options             : {}
+other_config        : {}
+statistics          : {}
+status              : {}
+type                : ""
+]])
+OVS_VSCTL_CLEANUP
+AT_CLEANUP
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index 4f601ef93..b9cd29c95 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -70,3 +70,50 @@  AT_CHECK([ovs-appctl upcall/show | grep -E "offloaded flows : [[1-9]]"], [0], [i
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst - offloads disabled])
+AT_KEYWORDS([ingress_policing])
+OVS_TRAFFIC_VSWITCHD_START()
+AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=false])
+AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
+ADD_NAMESPACES(at_ns0)
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_rate=100])
+AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_burst=10])
+AT_CHECK([ovs-vsctl list open | grep other_config > other_config.txt])
+AT_CHECK([cat other_config.txt],[0],
+    [other_config        : {hw-offload="false"}
+])
+AT_CHECK([tc -s -d filter show dev ovs-p0 ingress |grep rate| awk '{a=index($0,"rate");b=index($0,"mtu");print substr($0,a,b-a-1)}' > tc_ovs-p0.txt ],[0],[])
+AT_CHECK([cat tc_ovs-p0.txt],[0],
+    [rate 100Kbit burst 1280b
+])
+AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | grep basic | head -n 1 | awk '{a=index($0,"basic");print substr($0,a,5)}'| head -n 1],[0],
+    [basic
+])
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
+
+AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst - offloads enabled])
+AT_KEYWORDS([ingress_policing])
+OVS_TRAFFIC_VSWITCHD_START()
+AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true])
+AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
+ADD_NAMESPACES(at_ns0)
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_rate=100])
+AT_CHECK([ovs-vsctl set interface ovs-p0 ingress_policing_burst=10])
+AT_CHECK([ovs-vsctl list open | grep other_config > other_config.txt])
+AT_CHECK([cat other_config.txt],[0],
+    [other_config        : {hw-offload="true"}
+])
+AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | grep rate| awk '{a=index($0,"rate");b=index($0,"mtu");print substr($0,a,b-a-1)}' > tc_ovs-p0.txt ],[0],[])
+AT_CHECK([cat tc_ovs-p0.txt],[0],
+    [rate 100Kbit burst 1280b
+])
+AT_CHECK([tc -s -d filter show dev ovs-p0 ingress | grep matchall | head -n 1 | awk '{a=index($0,"matchall");print substr($0,a,8)}'| head -n 1],[0],
+    [matchall
+])
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP