Hi Kevin,
Thanks for the review, much appreciated.
I will send a new patch later.
> -----Original Message-----
> From: Kevin Traynor <ktray...@redhat.com>
> Sent: Wednesday, May 11, 2022 12:16 AM
> To: lin huang <mit...@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 linhu...@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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
