| Message ID | MEYP282MB3302DB480CBDD801BC9D7CE1CDC49@MEYP282MB3302.AUSP282.PROD.OUTLOOK.COM |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix ALB parameters type and value mismatch. | expand |
| 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 |
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
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 --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
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(-)