Hi Kevin, Thanks for the review.
One comment below. > -----Original Message----- > From: Kevin Traynor <[email protected]> > Sent: Wednesday, May 11, 2022 5:44 PM > To: Mike Pattrick <[email protected]>; lin huang <[email protected]> > Cc: [email protected] > 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 <[email protected]> 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 [email protected] > >> --- > >> 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 > >> [email protected] > >> > 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 > > [email protected] > > > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
