diff mbox series

[ovs-dev,1/2] dpif-netdev : Fix ALB parameters type mismatch.

Message ID MEYP282MB330257D9B87E998F447AE56DCDC49@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:09 p.m. UTC
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(-)

Comments

Mike Pattrick May 10, 2022, 4 p.m. UTC | #1
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
>
Kevin Traynor May 10, 2022, 4:15 p.m. UTC | #2
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;
>       }
Kevin Traynor May 11, 2022, 9:44 a.m. UTC | #3
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
>
lin huang May 13, 2022, 1:43 p.m. UTC | #4
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&amp;data=05%7C01%7C%7Cf6dd9aed789c49da485c08da3332d512%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C
> 0%7C637878590681477268%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=cmtUftXsT3fheNm3%2F1GkjJo39dH8g127A4SlQ%2F8i4Ro%3D&amp;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&amp;data=05%7C01%7C%7Cf6dd9aed789c49da485c08da3332d512%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C
> 0%7C637878590681477268%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=cmtUftXsT3fheNm3%2F1GkjJo39dH8g127A4SlQ%2F8i4Ro%3D&amp;res
> erved=0
> >
diff mbox series

Patch

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;
     }