Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] net: openvswitch: pass NULL for unused parameters
@ 2020-08-30 15:14 trix
  2020-08-30 20:02 ` Andy Shevchenko
  0 siblings, 1 reply; 3+ messages in thread
From: trix @ 2020-08-30 15:14 UTC (permalink / raw)
  To: pshelar, davem, kuba, natechancellor, ndesaulniers
  Cc: netdev, dev, linux-kernel, Tom Rix

From: Tom Rix <trix@redhat.com>

clang static analysis flags these problems

flow_table.c:713:2: warning: The expression is an uninitialized
  value. The computed value will also be garbage
        (*n_mask_hit)++;
        ^~~~~~~~~~~~~~~
flow_table.c:748:5: warning: The expression is an uninitialized
  value. The computed value will also be garbage
                                (*n_cache_hit)++;
                                ^~~~~~~~~~~~~~~~

These are not problems because neither pararmeter is used
by the calling function.

Looking at all of the calling functions, there are many
cases where the results are unused.  Passing unused
parameters is a waste.

To avoid passing unused parameters, rework the
masked_flow_lookup() and flow_lookup() routines to check
for NULL parameters and change the unused parameters to NULL.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 net/openvswitch/flow_table.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index e2235849a57e..18e7fa3aa67e 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -710,7 +710,8 @@ static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
 	ovs_flow_mask_key(&masked_key, unmasked, false, mask);
 	hash = flow_hash(&masked_key, &mask->range);
 	head = find_bucket(ti, hash);
-	(*n_mask_hit)++;
+	if (n_mask_hit)
+		(*n_mask_hit)++;
 
 	hlist_for_each_entry_rcu(flow, head, flow_table.node[ti->node_ver],
 				lockdep_ovsl_is_held()) {
@@ -745,7 +746,8 @@ static struct sw_flow *flow_lookup(struct flow_table *tbl,
 				u64_stats_update_begin(&ma->syncp);
 				usage_counters[*index]++;
 				u64_stats_update_end(&ma->syncp);
-				(*n_cache_hit)++;
+				if (n_cache_hit)
+					(*n_cache_hit)++;
 				return flow;
 			}
 		}
@@ -798,9 +800,8 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
 	*n_cache_hit = 0;
 	if (unlikely(!skb_hash || mc->cache_size == 0)) {
 		u32 mask_index = 0;
-		u32 cache = 0;
 
-		return flow_lookup(tbl, ti, ma, key, n_mask_hit, &cache,
+		return flow_lookup(tbl, ti, ma, key, n_mask_hit, NULL,
 				   &mask_index);
 	}
 
@@ -849,11 +850,9 @@ struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *tbl,
 {
 	struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
 	struct mask_array *ma = rcu_dereference_ovsl(tbl->mask_array);
-	u32 __always_unused n_mask_hit;
-	u32 __always_unused n_cache_hit;
 	u32 index = 0;
 
-	return flow_lookup(tbl, ti, ma, key, &n_mask_hit, &n_cache_hit, &index);
+	return flow_lookup(tbl, ti, ma, key, NULL, NULL, &index);
 }
 
 struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
@@ -865,7 +864,6 @@ struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
 	/* Always called under ovs-mutex. */
 	for (i = 0; i < ma->max; i++) {
 		struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
-		u32 __always_unused n_mask_hit;
 		struct sw_flow_mask *mask;
 		struct sw_flow *flow;
 
@@ -873,7 +871,7 @@ struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
 		if (!mask)
 			continue;
 
-		flow = masked_flow_lookup(ti, match->key, mask, &n_mask_hit);
+		flow = masked_flow_lookup(ti, match->key, mask, NULL);
 		if (flow && ovs_identifier_is_key(&flow->id) &&
 		    ovs_flow_cmp_unmasked_key(flow, match)) {
 			return flow;
-- 
2.18.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] net: openvswitch: pass NULL for unused parameters
  2020-08-30 15:14 [PATCH] net: openvswitch: pass NULL for unused parameters trix
@ 2020-08-30 20:02 ` Andy Shevchenko
  2020-08-30 20:32   ` Tom Rix
  0 siblings, 1 reply; 3+ messages in thread
From: Andy Shevchenko @ 2020-08-30 20:02 UTC (permalink / raw)
  To: trix
  Cc: pshelar, David S. Miller, Jakub Kicinski, Nathan Chancellor,
	Nick Desaulniers, netdev, dev, Linux Kernel Mailing List

On Sun, Aug 30, 2020 at 6:17 PM <trix@redhat.com> wrote:
>
> From: Tom Rix <trix@redhat.com>
>
> clang static analysis flags these problems
>
> flow_table.c:713:2: warning: The expression is an uninitialized
>   value. The computed value will also be garbage
>         (*n_mask_hit)++;
>         ^~~~~~~~~~~~~~~
> flow_table.c:748:5: warning: The expression is an uninitialized
>   value. The computed value will also be garbage
>                                 (*n_cache_hit)++;
>                                 ^~~~~~~~~~~~~~~~
>
> These are not problems because neither pararmeter is used

parameter

> by the calling function.
>
> Looking at all of the calling functions, there are many
> cases where the results are unused.  Passing unused
> parameters is a waste.
>
> To avoid passing unused parameters, rework the
> masked_flow_lookup() and flow_lookup() routines to check
> for NULL parameters and change the unused parameters to NULL.
>
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  net/openvswitch/flow_table.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
> index e2235849a57e..18e7fa3aa67e 100644
> --- a/net/openvswitch/flow_table.c
> +++ b/net/openvswitch/flow_table.c
> @@ -710,7 +710,8 @@ static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
>         ovs_flow_mask_key(&masked_key, unmasked, false, mask);
>         hash = flow_hash(&masked_key, &mask->range);
>         head = find_bucket(ti, hash);
> -       (*n_mask_hit)++;
> +       if (n_mask_hit)
> +               (*n_mask_hit)++;
>
>         hlist_for_each_entry_rcu(flow, head, flow_table.node[ti->node_ver],
>                                 lockdep_ovsl_is_held()) {
> @@ -745,7 +746,8 @@ static struct sw_flow *flow_lookup(struct flow_table *tbl,
>                                 u64_stats_update_begin(&ma->syncp);
>                                 usage_counters[*index]++;
>                                 u64_stats_update_end(&ma->syncp);
> -                               (*n_cache_hit)++;
> +                               if (n_cache_hit)
> +                                       (*n_cache_hit)++;
>                                 return flow;
>                         }
>                 }
> @@ -798,9 +800,8 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
>         *n_cache_hit = 0;

>         if (unlikely(!skb_hash || mc->cache_size == 0)) {
>                 u32 mask_index = 0;
> -               u32 cache = 0;
>
> -               return flow_lookup(tbl, ti, ma, key, n_mask_hit, &cache,
> +               return flow_lookup(tbl, ti, ma, key, n_mask_hit, NULL,
>                                    &mask_index);

Can it be done for mask_index as well?

>         }
>
> @@ -849,11 +850,9 @@ struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *tbl,
>  {
>         struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
>         struct mask_array *ma = rcu_dereference_ovsl(tbl->mask_array);
> -       u32 __always_unused n_mask_hit;
> -       u32 __always_unused n_cache_hit;
>         u32 index = 0;
>

> -       return flow_lookup(tbl, ti, ma, key, &n_mask_hit, &n_cache_hit, &index);
> +       return flow_lookup(tbl, ti, ma, key, NULL, NULL, &index);

Ditto.

>  }
>
>  struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
> @@ -865,7 +864,6 @@ struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
>         /* Always called under ovs-mutex. */
>         for (i = 0; i < ma->max; i++) {
>                 struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
> -               u32 __always_unused n_mask_hit;
>                 struct sw_flow_mask *mask;
>                 struct sw_flow *flow;
>
> @@ -873,7 +871,7 @@ struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
>                 if (!mask)
>                         continue;
>
> -               flow = masked_flow_lookup(ti, match->key, mask, &n_mask_hit);
> +               flow = masked_flow_lookup(ti, match->key, mask, NULL);
>                 if (flow && ovs_identifier_is_key(&flow->id) &&
>                     ovs_flow_cmp_unmasked_key(flow, match)) {
>                         return flow;
> --
> 2.18.1
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] net: openvswitch: pass NULL for unused parameters
  2020-08-30 20:02 ` Andy Shevchenko
@ 2020-08-30 20:32   ` Tom Rix
  0 siblings, 0 replies; 3+ messages in thread
From: Tom Rix @ 2020-08-30 20:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: pshelar, David S. Miller, Jakub Kicinski, Nathan Chancellor,
	Nick Desaulniers, netdev, dev, Linux Kernel Mailing List


On 8/30/20 1:02 PM, Andy Shevchenko wrote:
> On Sun, Aug 30, 2020 at 6:17 PM <trix@redhat.com> wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> clang static analysis flags these problems
>>
>> flow_table.c:713:2: warning: The expression is an uninitialized
>>   value. The computed value will also be garbage
>>         (*n_mask_hit)++;
>>         ^~~~~~~~~~~~~~~
>> flow_table.c:748:5: warning: The expression is an uninitialized
>>   value. The computed value will also be garbage
>>                                 (*n_cache_hit)++;
>>                                 ^~~~~~~~~~~~~~~~
>>
>> These are not problems because neither pararmeter is used
> parameter
Too many ar's, it must be talk like a pirate day.
>
>> by the calling function.
>>
>> Looking at all of the calling functions, there are many
>> cases where the results are unused.  Passing unused
>> parameters is a waste.
>>
>> To avoid passing unused parameters, rework the
>> masked_flow_lookup() and flow_lookup() routines to check
>> for NULL parameters and change the unused parameters to NULL.
>>
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>>  net/openvswitch/flow_table.c | 16 +++++++---------
>>  1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
>> index e2235849a57e..18e7fa3aa67e 100644
>> --- a/net/openvswitch/flow_table.c
>> +++ b/net/openvswitch/flow_table.c
>> @@ -710,7 +710,8 @@ static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
>>         ovs_flow_mask_key(&masked_key, unmasked, false, mask);
>>         hash = flow_hash(&masked_key, &mask->range);
>>         head = find_bucket(ti, hash);
>> -       (*n_mask_hit)++;
>> +       if (n_mask_hit)
>> +               (*n_mask_hit)++;
>>
>>         hlist_for_each_entry_rcu(flow, head, flow_table.node[ti->node_ver],
>>                                 lockdep_ovsl_is_held()) {
>> @@ -745,7 +746,8 @@ static struct sw_flow *flow_lookup(struct flow_table *tbl,
>>                                 u64_stats_update_begin(&ma->syncp);
>>                                 usage_counters[*index]++;
>>                                 u64_stats_update_end(&ma->syncp);
>> -                               (*n_cache_hit)++;
>> +                               if (n_cache_hit)
>> +                                       (*n_cache_hit)++;
>>                                 return flow;
>>                         }
>>                 }
>> @@ -798,9 +800,8 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
>>         *n_cache_hit = 0;
>>         if (unlikely(!skb_hash || mc->cache_size == 0)) {
>>                 u32 mask_index = 0;
>> -               u32 cache = 0;
>>
>> -               return flow_lookup(tbl, ti, ma, key, n_mask_hit, &cache,
>> +               return flow_lookup(tbl, ti, ma, key, n_mask_hit, NULL,
>>                                    &mask_index);
> Can it be done for mask_index as well?

Yes, it's a bit more complicated but doable.

Tom


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-08-30 20:32 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-30 15:14 [PATCH] net: openvswitch: pass NULL for unused parameters trix
2020-08-30 20:02 ` Andy Shevchenko
2020-08-30 20:32   ` Tom Rix

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).