| Message ID | MEYP282MB330257D9B87E998F447AE56DCDC49@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 Sat, May 7, 2022 at 1:10 PM lin huang <miterv@outlook.com> wrote: > > The ALB parameters should never be negative. > So it's to use smap_get_ulonglong() or smap_get_uint() to get it properly. > > Fixes: 5bf84282482a ("Adding support for PMD auto load balancing") > Signed-off-by: Lin Huang linhuang@ruijie.com.cn > --- > lib/dpif-netdev.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 88a5459cc..2e4be433c 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -4879,8 +4879,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) > > struct pmd_auto_lb *pmd_alb = &dp->pmd_alb; > > - rebalance_intvl = smap_get_int(other_config, "pmd-auto-lb-rebal-interval", > - ALB_REBALANCE_INTERVAL); > + rebalance_intvl = smap_get_ullong(other_config, "pmd-auto-lb-rebal-interval", > + ALB_REBALANCE_INTERVAL); > > /* Input is in min, convert it to msec. */ > rebalance_intvl = > @@ -4893,9 +4893,9 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) > log_autolb = true; > } > > - rebalance_improve = smap_get_int(other_config, > - "pmd-auto-lb-improvement-threshold", > - ALB_IMPROVEMENT_THRESHOLD); > + rebalance_improve = smap_get_uint(other_config, > + "pmd-auto-lb-improvement-threshold", > + ALB_IMPROVEMENT_THRESHOLD); > if (rebalance_improve > 100) { > rebalance_improve = ALB_IMPROVEMENT_THRESHOLD; > } > @@ -4906,8 +4906,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) > log_autolb = true; > } > > - rebalance_load = smap_get_int(other_config, "pmd-auto-lb-load-threshold", > - ALB_LOAD_THRESHOLD); > + rebalance_load = smap_get_uint(other_config, "pmd-auto-lb-load-threshold", > + ALB_LOAD_THRESHOLD); > if (rebalance_load > 100) { > rebalance_load = ALB_LOAD_THRESHOLD; > } Shouldn't rebalance_load and rebalance_improve be defined as uint types? The truncation from int to char may produce unintended results. Cheers, M > -- > 2.27.0 > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 07/05/2022 18:09, lin huang wrote: > The ALB parameters should never be negative. > So it's to use smap_get_ulonglong() or smap_get_uint() to get it properly. > Hi Lin. The code lgtm. Just a couple of minor things on the commit message/formatting. The function name is smap_get_ullong() instead of smap_get_ulonglong(). If you want you could just say you are using the 'unsigned smap_get versions' and not mention the function names. Either way is fine. Also, to be consistent with the previous commits, the title should be 'dpif-netdev:' instead of 'dpif-netdev :'. > Fixes: 5bf84282482a ("Adding support for PMD auto load balancing") > Signed-off-by: Lin Huang linhuang@ruijie.com.cn Emails normally have angle brackets, e.g. Signed-off-by: Lin Huang <linhuang@ruijie.com.cn> > --- > lib/dpif-netdev.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 88a5459cc..2e4be433c 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -4879,8 +4879,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) > > struct pmd_auto_lb *pmd_alb = &dp->pmd_alb; > > - rebalance_intvl = smap_get_int(other_config, "pmd-auto-lb-rebal-interval", > - ALB_REBALANCE_INTERVAL); > + rebalance_intvl = smap_get_ullong(other_config, "pmd-auto-lb-rebal-interval", > + ALB_REBALANCE_INTERVAL); > > /* Input is in min, convert it to msec. */ > rebalance_intvl = > @@ -4893,9 +4893,9 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) > log_autolb = true; > } > > - rebalance_improve = smap_get_int(other_config, > - "pmd-auto-lb-improvement-threshold", > - ALB_IMPROVEMENT_THRESHOLD); > + rebalance_improve = smap_get_uint(other_config, > + "pmd-auto-lb-improvement-threshold", > + ALB_IMPROVEMENT_THRESHOLD); > if (rebalance_improve > 100) { > rebalance_improve = ALB_IMPROVEMENT_THRESHOLD; > } > @@ -4906,8 +4906,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) > log_autolb = true; > } > > - rebalance_load = smap_get_int(other_config, "pmd-auto-lb-load-threshold", > - ALB_LOAD_THRESHOLD); > + rebalance_load = smap_get_uint(other_config, "pmd-auto-lb-load-threshold", > + ALB_LOAD_THRESHOLD); > if (rebalance_load > 100) { > rebalance_load = ALB_LOAD_THRESHOLD; > }
On 10/05/2022 17:00, Mike Pattrick wrote: > On Sat, May 7, 2022 at 1:10 PM lin huang <miterv@outlook.com> wrote: >> >> The ALB parameters should never be negative. >> So it's to use smap_get_ulonglong() or smap_get_uint() to get it properly. >> >> Fixes: 5bf84282482a ("Adding support for PMD auto load balancing") >> Signed-off-by: Lin Huang linhuang@ruijie.com.cn >> --- >> lib/dpif-netdev.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index 88a5459cc..2e4be433c 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -4879,8 +4879,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) >> >> struct pmd_auto_lb *pmd_alb = &dp->pmd_alb; >> >> - rebalance_intvl = smap_get_int(other_config, "pmd-auto-lb-rebal-interval", >> - ALB_REBALANCE_INTERVAL); >> + rebalance_intvl = smap_get_ullong(other_config, "pmd-auto-lb-rebal-interval", >> + ALB_REBALANCE_INTERVAL); >> >> /* Input is in min, convert it to msec. */ >> rebalance_intvl = >> @@ -4893,9 +4893,9 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) >> log_autolb = true; >> } >> >> - rebalance_improve = smap_get_int(other_config, >> - "pmd-auto-lb-improvement-threshold", >> - ALB_IMPROVEMENT_THRESHOLD); >> + rebalance_improve = smap_get_uint(other_config, >> + "pmd-auto-lb-improvement-threshold", >> + ALB_IMPROVEMENT_THRESHOLD); >> if (rebalance_improve > 100) { >> rebalance_improve = ALB_IMPROVEMENT_THRESHOLD; >> } >> @@ -4906,8 +4906,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) >> log_autolb = true; >> } >> >> - rebalance_load = smap_get_int(other_config, "pmd-auto-lb-load-threshold", >> - ALB_LOAD_THRESHOLD); >> + rebalance_load = smap_get_uint(other_config, "pmd-auto-lb-load-threshold", >> + ALB_LOAD_THRESHOLD); >> if (rebalance_load > 100) { >> rebalance_load = ALB_LOAD_THRESHOLD; >> } > > Shouldn't rebalance_load and rebalance_improve be defined as uint > types? The truncation from int to char may produce unintended results. > That's a fair point. In that case, how about: store from smap_get_uint() to local uint, check/adjust to valid range (0-100), then store/atomic_store into pmd_alb struct member with a uint8_t cast to avoid any -Wconversion warnings. Update VLOG formatting. Sounds ok? > Cheers, > M > >> -- >> 2.27.0 >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Hi Kevin, Thanks for the review. One comment below. > -----Original Message----- > From: Kevin Traynor <ktraynor@redhat.com> > Sent: Wednesday, May 11, 2022 5:44 PM > To: Mike Pattrick <mkp@redhat.com>; lin huang <miterv@outlook.com> > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH 1/2] dpif-netdev : Fix ALB parameters type mismatch. > > On 10/05/2022 17:00, Mike Pattrick wrote: > > On Sat, May 7, 2022 at 1:10 PM lin huang <miterv@outlook.com> wrote: > >> > >> The ALB parameters should never be negative. > >> So it's to use smap_get_ulonglong() or smap_get_uint() to get it properly. > >> > >> Fixes: 5bf84282482a ("Adding support for PMD auto load balancing") > >> Signed-off-by: Lin Huang linhuang@ruijie.com.cn > >> --- > >> lib/dpif-netdev.c | 14 +++++++------- > >> 1 file changed, 7 insertions(+), 7 deletions(-) > >> > >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > >> index 88a5459cc..2e4be433c 100644 > >> --- a/lib/dpif-netdev.c > >> +++ b/lib/dpif-netdev.c > >> @@ -4879,8 +4879,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) > >> > >> struct pmd_auto_lb *pmd_alb = &dp->pmd_alb; > >> > >> - rebalance_intvl = smap_get_int(other_config, "pmd-auto-lb-rebal-interval", > >> - ALB_REBALANCE_INTERVAL); > >> + rebalance_intvl = smap_get_ullong(other_config, "pmd-auto-lb-rebal-interval", > >> + ALB_REBALANCE_INTERVAL); > >> > >> /* Input is in min, convert it to msec. */ > >> rebalance_intvl = > >> @@ -4893,9 +4893,9 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) > >> log_autolb = true; > >> } > >> > >> - rebalance_improve = smap_get_int(other_config, > >> - "pmd-auto-lb-improvement-threshold", > >> - ALB_IMPROVEMENT_THRESHOLD); > >> + rebalance_improve = smap_get_uint(other_config, > >> + "pmd-auto-lb-improvement-threshold", > >> + ALB_IMPROVEMENT_THRESHOLD); > >> if (rebalance_improve > 100) { > >> rebalance_improve = ALB_IMPROVEMENT_THRESHOLD; > >> } > >> @@ -4906,8 +4906,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) > >> log_autolb = true; > >> } > >> > >> - rebalance_load = smap_get_int(other_config, "pmd-auto-lb-load-threshold", > >> - ALB_LOAD_THRESHOLD); > >> + rebalance_load = smap_get_uint(other_config, "pmd-auto-lb-load-threshold", > >> + ALB_LOAD_THRESHOLD); > >> if (rebalance_load > 100) { > >> rebalance_load = ALB_LOAD_THRESHOLD; > >> } > > > > Shouldn't rebalance_load and rebalance_improve be defined as uint > > types? The truncation from int to char may produce unintended results. > > > > That's a fair point. In that case, how about: store from smap_get_uint() > to local uint, check/adjust to valid range (0-100), then > store/atomic_store into pmd_alb struct member with a uint8_t cast to > avoid any -Wconversion warnings. Update VLOG formatting. Sounds ok? > I think it's ok to define load/improve params as uint32_t type, and update VLOG formatting to PRIu32. But there is no need to set uint8_t cast to void any -Wconversion warnings. Here is the code. - uint8_t rebalance_load, cur_rebalance_load; - uint8_t rebalance_improve; + uint8_t cur_rebalance_load; + uint32_t rebalance_load, rebalance_improve; - rebalance_improve = smap_get_int(other_config, - "pmd-auto-lb-improvement-threshold", - ALB_IMPROVEMENT_THRESHOLD); + rebalance_improve = smap_get_uint(other_config, + "pmd-auto-lb-improvement-threshold", + ALB_IMPROVEMENT_THRESHOLD); VLOG_INFO("PMD auto load balance improvement threshold set to " - "%"PRIu8"%%", rebalance_improve); + "%"PRIu32"%%", rebalance_improve); log_autolb = true; - rebalance_load = smap_get_int(other_config, "pmd-auto-lb-load-threshold", - ALB_LOAD_THRESHOLD); + rebalance_load = smap_get_uint(other_config, "pmd-auto-lb-load-threshold", + ALB_LOAD_THRESHOLD); if (rebalance_load > 100) { rebalance_load = ALB_LOAD_THRESHOLD; } @@ -4915,7 +4915,7 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) if (rebalance_load != cur_rebalance_load) { atomic_store_relaxed(&pmd_alb->rebalance_load_thresh, rebalance_load); - VLOG_INFO("PMD auto load balance load threshold set to %"PRIu8"%%", + VLOG_INFO("PMD auto load balance load threshold set to %"PRIu32"%%", rebalance_load); After modifying the code, I compiled it with no warning messages. > > Cheers, > > M > > > >> -- > >> 2.27.0 > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> > https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fo > vs- > dev&data=05%7C01%7C%7Cf6dd9aed789c49da485c08da3332d512%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C > 0%7C637878590681477268%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC > JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=cmtUftXsT3fheNm3%2F1GkjJo39dH8g127A4SlQ%2F8i4Ro%3D&res > erved=0 > >> > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > > https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fo > vs- > dev&data=05%7C01%7C%7Cf6dd9aed789c49da485c08da3332d512%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C > 0%7C637878590681477268%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC > JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=cmtUftXsT3fheNm3%2F1GkjJo39dH8g127A4SlQ%2F8i4Ro%3D&res > erved=0 > >
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 88a5459cc..2e4be433c 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4879,8 +4879,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) struct pmd_auto_lb *pmd_alb = &dp->pmd_alb; - rebalance_intvl = smap_get_int(other_config, "pmd-auto-lb-rebal-interval", - ALB_REBALANCE_INTERVAL); + rebalance_intvl = smap_get_ullong(other_config, "pmd-auto-lb-rebal-interval", + ALB_REBALANCE_INTERVAL); /* Input is in min, convert it to msec. */ rebalance_intvl = @@ -4893,9 +4893,9 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) log_autolb = true; } - rebalance_improve = smap_get_int(other_config, - "pmd-auto-lb-improvement-threshold", - ALB_IMPROVEMENT_THRESHOLD); + rebalance_improve = smap_get_uint(other_config, + "pmd-auto-lb-improvement-threshold", + ALB_IMPROVEMENT_THRESHOLD); if (rebalance_improve > 100) { rebalance_improve = ALB_IMPROVEMENT_THRESHOLD; } @@ -4906,8 +4906,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) log_autolb = true; } - rebalance_load = smap_get_int(other_config, "pmd-auto-lb-load-threshold", - ALB_LOAD_THRESHOLD); + rebalance_load = smap_get_uint(other_config, "pmd-auto-lb-load-threshold", + ALB_LOAD_THRESHOLD); if (rebalance_load > 100) { rebalance_load = ALB_LOAD_THRESHOLD; }
The ALB parameters should never be negative. So it's to use smap_get_ulonglong() or smap_get_uint() to get it properly. Fixes: 5bf84282482a ("Adding support for PMD auto load balancing") Signed-off-by: Lin Huang linhuang@ruijie.com.cn --- lib/dpif-netdev.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)