diff mbox series

[ovs-dev,2/2] dpif-netdev : Fix ALB 'rebalance_intvl' max hard limit.

Message ID MEYP282MB3302DB480CBDD801BC9D7CE1CDC49@MEYP282MB3302.AUSP282.PROD.OUTLOOK.COM
State Changes Requested
Headers show
Series Fix ALB parameters type and value mismatch. | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

lin huang May 7, 2022, 5:10 p.m. UTC
Currently the pmd-auto-lb-rebal-interval's value was not been
checked properly.
It maybe a negative, or too big value (>2 weeks between rebalances),
which will be lead to a big unsigned value. So reset it to default
if the value exceeds the max permitted as described in vswitchd.xml.

Fixes: 5bf84282482a ("Adding support for PMD auto load balancing")
Signed-off-by: Lin Huang linhuang@ruijie.com.cn
---
 lib/dpif-netdev.c |  6 +++++-
 tests/alb.at      | 20 +++++++++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

Comments

Kevin Traynor May 10, 2022, 4:16 p.m. UTC | #1
On 07/05/2022 18:10, lin huang wrote:
> Currently the pmd-auto-lb-rebal-interval's value was not been
> checked properly.
> It maybe a negative, or too big value (>2 weeks between rebalances),
> which will be lead to a big unsigned value. So reset it to default
> if the value exceeds the max permitted as described in vswitchd.xml.
> 
> Fixes: 5bf84282482a ("Adding support for PMD auto load balancing")
> Signed-off-by: Lin Huang linhuang@ruijie.com.cn

Similar comment about title and sign-off as patch 1/2.

Fix looks good and thanks for the additional unit tests :) One small 
comment on them below.

> ---
>   lib/dpif-netdev.c |  6 +++++-
>   tests/alb.at      | 20 +++++++++++++++++++-
>   2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 2e4be433c..b358b8b1c 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -93,7 +93,8 @@ VLOG_DEFINE_THIS_MODULE(dpif_netdev);
>   /* Auto Load Balancing Defaults */
>   #define ALB_IMPROVEMENT_THRESHOLD    25
>   #define ALB_LOAD_THRESHOLD           95
> -#define ALB_REBALANCE_INTERVAL       1 /* 1 Min */
> +#define ALB_REBALANCE_INTERVAL       1     /* 1 Min */
> +#define MAX_ALB_REBALANCE_INTERVAL   20000 /* 20000 Min */
>   #define MIN_TO_MSEC                  60000
>   
>   #define FLOW_DUMP_MAX_BATCH 50
> @@ -4881,6 +4882,9 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config)
>   
>       rebalance_intvl = smap_get_ullong(other_config, "pmd-auto-lb-rebal-interval",
>                                         ALB_REBALANCE_INTERVAL);
> +    if (rebalance_intvl > MAX_ALB_REBALANCE_INTERVAL) {
> +        rebalance_intvl = ALB_REBALANCE_INTERVAL;
> +    }
>   
>       /* Input is in min, convert it to msec. */
>       rebalance_intvl =
> diff --git a/tests/alb.at b/tests/alb.at
> index 2bef06f39..17bb754ae 100644
> --- a/tests/alb.at
> +++ b/tests/alb.at
> @@ -197,7 +197,25 @@ get_log_next_line_num
>   AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="0"])
>   CHECK_ALB_PARAM([interval], [1 mins], [+$LINENUM])
>   
> -# No check for above max as it is only a documented max value and not a hard limit
> +# Set new value
> +get_log_next_line_num
> +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="100"])
> +CHECK_ALB_PARAM([interval], [100 mins], [+$LINENUM])
> +
> +# Set above max value
> +get_log_next_line_num
> +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="50000"])
> +CHECK_ALB_PARAM([interval], [1 mins], [+$LINENUM])

As it's checking above the max, might as well check the whole invalid 
range with "20001", either as a replacement for this test or an 
additional test.

thanks,
Kevin.

> +
> +# Set new value
> +get_log_next_line_num
> +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="1000"])
> +CHECK_ALB_PARAM([interval], [1000 mins], [+$LINENUM])
> +
> +# Set Negative value
> +get_log_next_line_num
> +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="-1"])
> +CHECK_ALB_PARAM([interval], [1 mins], [+$LINENUM])
>   
>   OVS_VSWITCHD_STOP
>   AT_CLEANUP
lin huang May 11, 2022, 2:22 p.m. UTC | #2
Hi Kevin,

Thanks for the review, much appreciated.

I will send a new patch later.

> -----Original Message-----
> From: Kevin Traynor <ktraynor@redhat.com>
> Sent: Wednesday, May 11, 2022 12:16 AM
> To: lin huang <miterv@outlook.com>; ovs-dev@openvswitch.org
> Subject: Re: [PATCH 2/2] dpif-netdev : Fix ALB 'rebalance_intvl' max hard limit.
> 
> On 07/05/2022 18:10, lin huang wrote:
> > Currently the pmd-auto-lb-rebal-interval's value was not been
> > checked properly.
> > It maybe a negative, or too big value (>2 weeks between rebalances),
> > which will be lead to a big unsigned value. So reset it to default
> > if the value exceeds the max permitted as described in vswitchd.xml.
> >
> > Fixes: 5bf84282482a ("Adding support for PMD auto load balancing")
> > Signed-off-by: Lin Huang linhuang@ruijie.com.cn
> 
> Similar comment about title and sign-off as patch 1/2.
> 
> Fix looks good and thanks for the additional unit tests :) One small
> comment on them below.
> 
> > ---
> >   lib/dpif-netdev.c |  6 +++++-
> >   tests/alb.at      | 20 +++++++++++++++++++-
> >   2 files changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 2e4be433c..b358b8b1c 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -93,7 +93,8 @@ VLOG_DEFINE_THIS_MODULE(dpif_netdev);
> >   /* Auto Load Balancing Defaults */
> >   #define ALB_IMPROVEMENT_THRESHOLD    25
> >   #define ALB_LOAD_THRESHOLD           95
> > -#define ALB_REBALANCE_INTERVAL       1 /* 1 Min */
> > +#define ALB_REBALANCE_INTERVAL       1     /* 1 Min */
> > +#define MAX_ALB_REBALANCE_INTERVAL   20000 /* 20000 Min */
> >   #define MIN_TO_MSEC                  60000
> >
> >   #define FLOW_DUMP_MAX_BATCH 50
> > @@ -4881,6 +4882,9 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config)
> >
> >       rebalance_intvl = smap_get_ullong(other_config, "pmd-auto-lb-rebal-interval",
> >                                         ALB_REBALANCE_INTERVAL);
> > +    if (rebalance_intvl > MAX_ALB_REBALANCE_INTERVAL) {
> > +        rebalance_intvl = ALB_REBALANCE_INTERVAL;
> > +    }
> >
> >       /* Input is in min, convert it to msec. */
> >       rebalance_intvl =
> > diff --git a/tests/alb.at b/tests/alb.at
> > index 2bef06f39..17bb754ae 100644
> > --- a/tests/alb.at
> > +++ b/tests/alb.at
> > @@ -197,7 +197,25 @@ get_log_next_line_num
> >   AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="0"])
> >   CHECK_ALB_PARAM([interval], [1 mins], [+$LINENUM])
> >
> > -# No check for above max as it is only a documented max value and not a hard limit
> > +# Set new value
> > +get_log_next_line_num
> > +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="100"])
> > +CHECK_ALB_PARAM([interval], [100 mins], [+$LINENUM])
> > +
> > +# Set above max value
> > +get_log_next_line_num
> > +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="50000"])
> > +CHECK_ALB_PARAM([interval], [1 mins], [+$LINENUM])
> 
> As it's checking above the max, might as well check the whole invalid
> range with "20001", either as a replacement for this test or an
> additional test.
> 
> thanks,
> Kevin.
> 
> > +
> > +# Set new value
> > +get_log_next_line_num
> > +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="1000"])
> > +CHECK_ALB_PARAM([interval], [1000 mins], [+$LINENUM])
> > +
> > +# Set Negative value
> > +get_log_next_line_num
> > +AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="-1"])
> > +CHECK_ALB_PARAM([interval], [1 mins], [+$LINENUM])
> >
> >   OVS_VSWITCHD_STOP
> >   AT_CLEANUP
diff mbox series

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 2e4be433c..b358b8b1c 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -93,7 +93,8 @@  VLOG_DEFINE_THIS_MODULE(dpif_netdev);
 /* Auto Load Balancing Defaults */
 #define ALB_IMPROVEMENT_THRESHOLD    25
 #define ALB_LOAD_THRESHOLD           95
-#define ALB_REBALANCE_INTERVAL       1 /* 1 Min */
+#define ALB_REBALANCE_INTERVAL       1     /* 1 Min */
+#define MAX_ALB_REBALANCE_INTERVAL   20000 /* 20000 Min */
 #define MIN_TO_MSEC                  60000
 
 #define FLOW_DUMP_MAX_BATCH 50
@@ -4881,6 +4882,9 @@  dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config)
 
     rebalance_intvl = smap_get_ullong(other_config, "pmd-auto-lb-rebal-interval",
                                       ALB_REBALANCE_INTERVAL);
+    if (rebalance_intvl > MAX_ALB_REBALANCE_INTERVAL) {
+        rebalance_intvl = ALB_REBALANCE_INTERVAL;
+    }
 
     /* Input is in min, convert it to msec. */
     rebalance_intvl =
diff --git a/tests/alb.at b/tests/alb.at
index 2bef06f39..17bb754ae 100644
--- a/tests/alb.at
+++ b/tests/alb.at
@@ -197,7 +197,25 @@  get_log_next_line_num
 AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="0"])
 CHECK_ALB_PARAM([interval], [1 mins], [+$LINENUM])
 
-# No check for above max as it is only a documented max value and not a hard limit
+# Set new value
+get_log_next_line_num
+AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="100"])
+CHECK_ALB_PARAM([interval], [100 mins], [+$LINENUM])
+
+# Set above max value
+get_log_next_line_num
+AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="50000"])
+CHECK_ALB_PARAM([interval], [1 mins], [+$LINENUM])
+
+# Set new value
+get_log_next_line_num
+AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="1000"])
+CHECK_ALB_PARAM([interval], [1000 mins], [+$LINENUM])
+
+# Set Negative value
+get_log_next_line_num
+AT_CHECK([ovs-vsctl set open_vswitch . other_config:pmd-auto-lb-rebal-interval="-1"])
+CHECK_ALB_PARAM([interval], [1 mins], [+$LINENUM])
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP