Re: [ovs-dev] [PATCH 1/1] tests: Add PMD auto load balance unit tests.
On 09/03/2021 15:21, David Marchand wrote:
> On Thu, Feb 18, 2021 at 11:48 AM Kevin Traynor wrote:
>>
>> These tests focus on enabling/disabling and user parameters.
>>
>> Signed-off-by: Kevin Traynor
>
> The patch looks good to me and I tested it with no issue.
>
> Just two small comments, see below.
>
>
>> ---
>> tests/alb.at | 256 +
>> tests/automake.mk | 1 +
>> tests/testsuite.at | 1 +
>> 3 files changed, 258 insertions(+)
>> create mode 100644 tests/alb.at
>>
>> diff --git a/tests/alb.at b/tests/alb.at
>> new file mode 100644
>> index 0..136c26c44
>> --- /dev/null
>> +++ b/tests/alb.at
>> @@ -0,0 +1,256 @@
>> +AT_BANNER([PMD Auto Load Balance])
>> +
>> +m4_divert_push([PREPARE_TESTS])
>> +
>> +get_log_line_num () {
>> +LINENUM=$(cat ovs-vswitchd.log | wc -l | tr -d [[:blank:]])
>> +}
>> +
>> +m4_divert_pop([PREPARE_TESTS])
>> +
>> +m4_define([DUMMY_NUMA], [--dummy-numa="0,0"])
>> +
>> +dnl CHECK_INTERVAL_PARAM([interval_time], [+line])
>> +dnl
>> +dnl Waits for alb interval time in logs and checks if time matches
>> +dnl $1. Checking starts from line number 'line' in ovs-vswithd.log .
>> +m4_define([CHECK_INTERVAL_PARAM], [
>> +PATTERN="PMD auto load balance interval set to [[0-9]]* mins"
>> +line_st=$2
>> +if [[ -z "$line_st" ]]
>> +then
>> +line_st="+0"
>> +fi
>> +OVS_WAIT_UNTIL([tail -n $line_st ovs-vswitchd.log | grep "$PATTERN"])
>> +TIME=$(tail -n $line_st ovs-vswitchd.log | grep "$PATTERN" | tail -1 |
>> sed -e 's/.* \([[0-9]]*\) mins/\1/')
>> +AT_CHECK([test "$TIME" -eq "$1"])
>
> The ALB log messages follow a common format, so I'd suggest combining
> those 3 helpers as a single.
> That is with the hope we will conform to this format for new parameters later.
>
> dnl CHECK_ALB_PARAM([param], [value], [+line])
> dnl
> dnl Waits for ALB logs for 'param' in logs and checks if value matches
> dnl 'value'. Checking starts from line number 'line' in ovs-vswithd.log.
> m4_define([CHECK_ALB_PARAM], [
> line_st=$3
> if [[ -z "$line_st" ]]
> then
> line_st="+0"
> fi
> OVS_WAIT_UNTIL([tail -n $line_st ovs-vswitchd.log | grep "PMD auto
> load balance $1 set to"])
> AT_CHECK([tail -n $line_st ovs-vswitchd.log | sed -n "s#.*\(PMD
> auto load balance $1 set to.*\)#\1#p" | tail -1], [0], [dnl
> PMD auto load balance $1 set to $2
> ])
> ])
>
> [snip]
>
>
>> +AT_SETUP([ALB - interval param])
>> +OVS_VSWITCHD_START
>> +OVS_WAIT_UNTIL([grep "PMD auto load balance is disabled" ovs-vswitchd.log])
>> +
>> +# Check default
>> +CHECK_INTERVAL_PARAM([1], [])
>
> Here, it becomes:
> CHECK_ALB_PARAM([interval], [1 mins], [])
>
> etc..
>
> [snip]
>
Thanks, this is neater. I'll incorporate it into v2.
>> +# TODO 2 is documented as max value but it seems arbitary
>> +# Setting to 20001 works, should it be checked and rejected?
>> +# For now add a dummy test above the max value that accepts it
>
> Afaiu, from ovsdb pov, other_config is a simple map of strings with no
> constraint on the content of the strings.
> I understand the constraints expressed for sub fields like
> pmd-auto-lb-XX parameters in vswitchd/vswitchd.xml as only for
> documentation.
>
> I don't see the benefit of checking values 2 or 20001.
>
>
As it stands, without this limit enforced, it is implicitly limited to
INT_MAX and even that should not cause any problems with usage, though
traffic patterns may have changed a bit during the 4K years, so it may
not be optimal :-)
I'll remove this these tests and if we add enforcement on this limit in
the future we can add a UT at that point.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] tests: Add PMD auto load balance unit tests.
> >> Afaiu, from ovsdb pov, other_config is a simple map of strings with > >> no constraint on the content of the strings. > >> I understand the constraints expressed for sub fields like > >> pmd-auto-lb-XX parameters in vswitchd/vswitchd.xml as only for > documentation. > >> > >> I don't see the benefit of checking values 2 or 20001. > > > > I couldn't find the doc where the max value is mentioned. > > Would you mind pointing it ? > > Or its calculated somehow ? > > > > Thanks Sunil (and David). The max is mentioned here: > https://github.com/openvswitch/ovs/blob/master/vswitchd/vswitch.xml#L6 > 77-L679 Thanks Kevin , I somehow overlooked the file mentioned by David. > > We probably need to think a bit more about the units and max for this > parameter in general, but that's what's currently documented. I agree. > > Kevin. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] tests: Add PMD auto load balance unit tests.
On 12/03/2021 13:21, Pai G, Sunil wrote: > Hey Kevin, > > Tested the patch , works fine for me too :) > >> >> The ALB log messages follow a common format, so I'd suggest combining >> those 3 helpers as a single. >> That is with the hope we will conform to this format for new parameters >> later. >> >> dnl CHECK_ALB_PARAM([param], [value], [+line]) dnl dnl Waits for ALB logs >> for 'param' in logs and checks if value matches dnl 'value'. Checking starts >> from line number 'line' in ovs-vswithd.log. >> m4_define([CHECK_ALB_PARAM], [ >> line_st=$3 >> if [[ -z "$line_st" ]] >> then >> line_st="+0" >> fi >> OVS_WAIT_UNTIL([tail -n $line_st ovs-vswitchd.log | grep "PMD auto load >> balance $1 set to"]) >> AT_CHECK([tail -n $line_st ovs-vswitchd.log | sed -n "s#.*\(PMD auto load >> balance $1 set to.*\)#\1#p" | tail -1], [0], [dnl PMD auto load balance $1 >> set >> to $2 >> ]) >> ]) >> > > I agree with David on this one. It might be good to create a helper like > CHECK_LOAD_PARAM. > >> >> >>> +AT_SETUP([ALB - interval param]) >>> +OVS_VSWITCHD_START >>> +OVS_WAIT_UNTIL([grep "PMD auto load balance is disabled" >>> +ovs-vswitchd.log]) >>> + >>> +# Check default >>> +CHECK_INTERVAL_PARAM([1], []) >> >> Here, it becomes: >> CHECK_ALB_PARAM([interval], [1 mins], []) >> >> etc.. >> >> [snip] >> >>> +# TODO 2 is documented as max value but it seems arbitary # >>> +Setting to 20001 works, should it be checked and rejected? >>> +# For now add a dummy test above the max value that accepts it >> >> Afaiu, from ovsdb pov, other_config is a simple map of strings with no >> constraint on the content of the strings. >> I understand the constraints expressed for sub fields like pmd-auto-lb-XX >> parameters in vswitchd/vswitchd.xml as only for documentation. >> >> I don't see the benefit of checking values 2 or 20001. > > I couldn't find the doc where the max value is mentioned. > Would you mind pointing it ? > Or its calculated somehow ? > Thanks Sunil (and David). The max is mentioned here: https://github.com/openvswitch/ovs/blob/master/vswitchd/vswitch.xml#L677-L679 We probably need to think a bit more about the units and max for this parameter in general, but that's what's currently documented. Kevin. > The rest looks to good to me! > > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] tests: Add PMD auto load balance unit tests.
Hey Kevin, Tested the patch , works fine for me too :) > > The ALB log messages follow a common format, so I'd suggest combining > those 3 helpers as a single. > That is with the hope we will conform to this format for new parameters > later. > > dnl CHECK_ALB_PARAM([param], [value], [+line]) dnl dnl Waits for ALB logs > for 'param' in logs and checks if value matches dnl 'value'. Checking starts > from line number 'line' in ovs-vswithd.log. > m4_define([CHECK_ALB_PARAM], [ > line_st=$3 > if [[ -z "$line_st" ]] > then > line_st="+0" > fi > OVS_WAIT_UNTIL([tail -n $line_st ovs-vswitchd.log | grep "PMD auto load > balance $1 set to"]) > AT_CHECK([tail -n $line_st ovs-vswitchd.log | sed -n "s#.*\(PMD auto load > balance $1 set to.*\)#\1#p" | tail -1], [0], [dnl PMD auto load balance $1 set > to $2 > ]) > ]) > I agree with David on this one. It might be good to create a helper like CHECK_LOAD_PARAM. > > > > +AT_SETUP([ALB - interval param]) > > +OVS_VSWITCHD_START > > +OVS_WAIT_UNTIL([grep "PMD auto load balance is disabled" > > +ovs-vswitchd.log]) > > + > > +# Check default > > +CHECK_INTERVAL_PARAM([1], []) > > Here, it becomes: > CHECK_ALB_PARAM([interval], [1 mins], []) > > etc.. > > [snip] > > > +# TODO 2 is documented as max value but it seems arbitary # > > +Setting to 20001 works, should it be checked and rejected? > > +# For now add a dummy test above the max value that accepts it > > Afaiu, from ovsdb pov, other_config is a simple map of strings with no > constraint on the content of the strings. > I understand the constraints expressed for sub fields like pmd-auto-lb-XX > parameters in vswitchd/vswitchd.xml as only for documentation. > > I don't see the benefit of checking values 2 or 20001. I couldn't find the doc where the max value is mentioned. Would you mind pointing it ? Or its calculated somehow ? The rest looks to good to me! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/1] tests: Add PMD auto load balance unit tests.
On Thu, Feb 18, 2021 at 11:48 AM Kevin Traynor wrote:
>
> These tests focus on enabling/disabling and user parameters.
>
> Signed-off-by: Kevin Traynor
The patch looks good to me and I tested it with no issue.
Just two small comments, see below.
> ---
> tests/alb.at | 256 +
> tests/automake.mk | 1 +
> tests/testsuite.at | 1 +
> 3 files changed, 258 insertions(+)
> create mode 100644 tests/alb.at
>
> diff --git a/tests/alb.at b/tests/alb.at
> new file mode 100644
> index 0..136c26c44
> --- /dev/null
> +++ b/tests/alb.at
> @@ -0,0 +1,256 @@
> +AT_BANNER([PMD Auto Load Balance])
> +
> +m4_divert_push([PREPARE_TESTS])
> +
> +get_log_line_num () {
> +LINENUM=$(cat ovs-vswitchd.log | wc -l | tr -d [[:blank:]])
> +}
> +
> +m4_divert_pop([PREPARE_TESTS])
> +
> +m4_define([DUMMY_NUMA], [--dummy-numa="0,0"])
> +
> +dnl CHECK_INTERVAL_PARAM([interval_time], [+line])
> +dnl
> +dnl Waits for alb interval time in logs and checks if time matches
> +dnl $1. Checking starts from line number 'line' in ovs-vswithd.log .
> +m4_define([CHECK_INTERVAL_PARAM], [
> +PATTERN="PMD auto load balance interval set to [[0-9]]* mins"
> +line_st=$2
> +if [[ -z "$line_st" ]]
> +then
> +line_st="+0"
> +fi
> +OVS_WAIT_UNTIL([tail -n $line_st ovs-vswitchd.log | grep "$PATTERN"])
> +TIME=$(tail -n $line_st ovs-vswitchd.log | grep "$PATTERN" | tail -1 |
> sed -e 's/.* \([[0-9]]*\) mins/\1/')
> +AT_CHECK([test "$TIME" -eq "$1"])
The ALB log messages follow a common format, so I'd suggest combining
those 3 helpers as a single.
That is with the hope we will conform to this format for new parameters later.
dnl CHECK_ALB_PARAM([param], [value], [+line])
dnl
dnl Waits for ALB logs for 'param' in logs and checks if value matches
dnl 'value'. Checking starts from line number 'line' in ovs-vswithd.log.
m4_define([CHECK_ALB_PARAM], [
line_st=$3
if [[ -z "$line_st" ]]
then
line_st="+0"
fi
OVS_WAIT_UNTIL([tail -n $line_st ovs-vswitchd.log | grep "PMD auto
load balance $1 set to"])
AT_CHECK([tail -n $line_st ovs-vswitchd.log | sed -n "s#.*\(PMD
auto load balance $1 set to.*\)#\1#p" | tail -1], [0], [dnl
PMD auto load balance $1 set to $2
])
])
[snip]
> +AT_SETUP([ALB - interval param])
> +OVS_VSWITCHD_START
> +OVS_WAIT_UNTIL([grep "PMD auto load balance is disabled" ovs-vswitchd.log])
> +
> +# Check default
> +CHECK_INTERVAL_PARAM([1], [])
Here, it becomes:
CHECK_ALB_PARAM([interval], [1 mins], [])
etc..
[snip]
> +# TODO 2 is documented as max value but it seems arbitary
> +# Setting to 20001 works, should it be checked and rejected?
> +# For now add a dummy test above the max value that accepts it
Afaiu, from ovsdb pov, other_config is a simple map of strings with no
constraint on the content of the strings.
I understand the constraints expressed for sub fields like
pmd-auto-lb-XX parameters in vswitchd/vswitchd.xml as only for
documentation.
I don't see the benefit of checking values 2 or 20001.
--
David Marchand
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/1] tests: Add PMD auto load balance unit tests.
These tests focus on enabling/disabling and user parameters.
Signed-off-by: Kevin Traynor
---
tests/alb.at | 256 +
tests/automake.mk | 1 +
tests/testsuite.at | 1 +
3 files changed, 258 insertions(+)
create mode 100644 tests/alb.at
diff --git a/tests/alb.at b/tests/alb.at
new file mode 100644
index 0..136c26c44
--- /dev/null
+++ b/tests/alb.at
@@ -0,0 +1,256 @@
+AT_BANNER([PMD Auto Load Balance])
+
+m4_divert_push([PREPARE_TESTS])
+
+get_log_line_num () {
+LINENUM=$(cat ovs-vswitchd.log | wc -l | tr -d [[:blank:]])
+}
+
+m4_divert_pop([PREPARE_TESTS])
+
+m4_define([DUMMY_NUMA], [--dummy-numa="0,0"])
+
+dnl CHECK_INTERVAL_PARAM([interval_time], [+line])
+dnl
+dnl Waits for alb interval time in logs and checks if time matches
+dnl $1. Checking starts from line number 'line' in ovs-vswithd.log .
+m4_define([CHECK_INTERVAL_PARAM], [
+PATTERN="PMD auto load balance interval set to [[0-9]]* mins"
+line_st=$2
+if [[ -z "$line_st" ]]
+then
+line_st="+0"
+fi
+OVS_WAIT_UNTIL([tail -n $line_st ovs-vswitchd.log | grep "$PATTERN"])
+TIME=$(tail -n $line_st ovs-vswitchd.log | grep "$PATTERN" | tail -1 | sed
-e 's/.* \([[0-9]]*\) mins/\1/')
+AT_CHECK([test "$TIME" -eq "$1"])
+])
+
+dnl CHECK_LOAD_PARAM([load], [+line])
+dnl
+dnl Waits for alb load threshold in logs and checks if threshold matches
+dnl $1. Checking starts from line number 'line' in ovs-vswithd.log .
+m4_define([CHECK_LOAD_PARAM], [
+PATTERN="PMD auto load balance load threshold set to [[0-9]]*"
+line_st=$2
+if [[ -z "$line_st" ]]
+then
+line_st="+0"
+fi
+OVS_WAIT_UNTIL([tail -n $line_st ovs-vswitchd.log | grep "$PATTERN"])
+LOAD=$(tail -n $line_st ovs-vswitchd.log | grep "$PATTERN" | tail -1 | tr
-d '%' | sed -e 's/.* \([[0-9]]*\)/\1/')
+AT_CHECK([test "$LOAD" -eq "$1"])
+])
+
+dnl CHECK_IMPROVE_PARAM([load], [+line])
+dnl
+dnl Waits for alb improve threshold in logs and checks if threshold matches
+dnl $1. Checking starts from line number 'line' in ovs-vswithd.log .
+m4_define([CHECK_IMPROVE_PARAM], [
+PATTERN="PMD auto load balance improvement threshold set to [[0-9]]*"
+line_st=$3
+if [[ -z "$line_st" ]]
+then
+line_st="+0"
+fi
+OVS_WAIT_UNTIL([tail -n $line_st ovs-vswitchd.log | grep "$PATTERN"])
+IMPROVE=$(tail -n $line_st ovs-vswitchd.log | grep "$PATTERN" | tail -1 |
tr -d '%' | sed -e 's/.* \([[0-9]]*\)/\1/')
+AT_CHECK([test "$IMPROVE" -eq "$1"])
+])
+
+
+AT_SETUP([ALB - default state])
+OVS_VSWITCHD_START
+OVS_WAIT_UNTIL([grep "PMD auto load balance is disabled" ovs-vswitchd.log])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ALB - enable/disable])
+OVS_VSWITCHD_START([add-port br0 p0 \
+-- set Interface p0 type=dummy-pmd options:n_rxq=3 \
+-- set Open_vSwitch . other_config:pmd-cpu-mask=3 \
+-- set open_vswitch . other_config:pmd-auto-lb="true"],
+ [], [], [DUMMY_NUMA])
+OVS_WAIT_UNTIL([grep "PMD auto load balance is enabled" ovs-vswitchd.log])
+
+get_log_line_num
+AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="false"])
+OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load
balance is disabled"])
+
+get_log_line_num
+AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="true"])
+OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load
balance is enabled"])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ALB - min num PMD/RxQ])
+OVS_VSWITCHD_START([add-port br0 p0 \
+-- set Interface p0 type=dummy-pmd options:n_rxq=2 \
+-- set Open_vSwitch . other_config:pmd-cpu-mask=1 \
+-- set open_vswitch . other_config:pmd-auto-lb="true"],
+ [], [], [DUMMY_NUMA])
+OVS_WAIT_UNTIL([grep "PMD auto load balance is disabled" ovs-vswitchd.log])
+
+# Add more PMD
+AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=0x3])
+OVS_WAIT_UNTIL([grep "There are 2 pmd threads on numa node" ovs-vswitchd.log])
+
+# Add one more rxq to have 2 rxq on a PMD
+get_log_line_num
+AT_CHECK([ovs-vsctl set interface p0 options:n_rxq=3])
+OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load
balance is enabled"])
+
+# Reduce PMD
+get_log_line_num
+AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=0x1])
+OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load
balance is disabled"])
+
+# Check logs when try to enable but min PMD/RxQ prevents
+get_log_line_num
+AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="false"])
+OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load
balance is disabled"])
+get_log_line_num
+AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb="true"])
+OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log | grep "PMD auto load
balance is
