diff mbox series

[net-next] net: dsa: b53: Slightly optimize b53_arl_read()

Message ID c94fb1b4dcd9a04eff08cf9ba2444c348477e554.1682023416.git.christophe.jaillet@wanadoo.fr
State New
Headers
Series [net-next] net: dsa: b53: Slightly optimize b53_arl_read() |

Commit Message

Christophe JAILLET April 20, 2023, 8:44 p.m. UTC 
  When the 'free_bins' bitmap is cleared, it is better to use its full
maximum size instead of only the needed size.
This lets the compiler optimize it because the size is now known at compile
time. B53_ARLTBL_MAX_BIN_ENTRIES is small (i.e. currently 4), so a call to
memset() is saved.

Also, as 'free_bins' is local to the function, the non-atomic __set_bit()
can also safely be used here.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/net/dsa/b53/b53_common.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Florian Fainelli April 21, 2023, 12:40 a.m. UTC | #1 
On 4/20/2023 1:44 PM, Christophe JAILLET wrote:
> When the 'free_bins' bitmap is cleared, it is better to use its full
> maximum size instead of only the needed size.
> This lets the compiler optimize it because the size is now known at compile
> time. B53_ARLTBL_MAX_BIN_ENTRIES is small (i.e. currently 4), so a call to
> memset() is saved.
> 
> Also, as 'free_bins' is local to the function, the non-atomic __set_bit()
> can also safely be used here.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>   drivers/net/dsa/b53/b53_common.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 3464ce5e7470..8c55fe0e0747 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1627,7 +1627,7 @@ static int b53_arl_read(struct b53_device *dev, u64 mac,
>   	if (ret)
>   		return ret;
>   
> -	bitmap_zero(free_bins, dev->num_arl_bins);
> +	bitmap_zero(free_bins, B53_ARLTBL_MAX_BIN_ENTRIES);

That one I am not a big fan, as the number of ARL bins is a function of 
the switch model, and this illustrates it well.

>   
>   	/* Read the bins */
>   	for (i = 0; i < dev->num_arl_bins; i++) {
> @@ -1641,7 +1641,7 @@ static int b53_arl_read(struct b53_device *dev, u64 mac,
>   		b53_arl_to_entry(ent, mac_vid, fwd_entry);
>   
>   		if (!(fwd_entry & ARLTBL_VALID)) {
> -			set_bit(i, free_bins);
> +			__set_bit(i, free_bins);

I would be keen on taking that hunk but keep the other as-is. Does that 
work for you?
--
Florian
Christophe JAILLET April 21, 2023, 5:40 a.m. UTC | #2 
Le 21/04/2023 à 02:40, Florian Fainelli a écrit :
> 
> 
> On 4/20/2023 1:44 PM, Christophe JAILLET wrote:
>> When the 'free_bins' bitmap is cleared, it is better to use its full
>> maximum size instead of only the needed size.
>> This lets the compiler optimize it because the size is now known at 
>> compile
>> time. B53_ARLTBL_MAX_BIN_ENTRIES is small (i.e. currently 4), so a 
>> call to
>> memset() is saved.
>>
>> Also, as 'free_bins' is local to the function, the non-atomic __set_bit()
>> can also safely be used here.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   drivers/net/dsa/b53/b53_common.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/dsa/b53/b53_common.c 
>> b/drivers/net/dsa/b53/b53_common.c
>> index 3464ce5e7470..8c55fe0e0747 100644
>> --- a/drivers/net/dsa/b53/b53_common.c
>> +++ b/drivers/net/dsa/b53/b53_common.c
>> @@ -1627,7 +1627,7 @@ static int b53_arl_read(struct b53_device *dev, 
>> u64 mac,
>>       if (ret)
>>           return ret;
>> -    bitmap_zero(free_bins, dev->num_arl_bins);
>> +    bitmap_zero(free_bins, B53_ARLTBL_MAX_BIN_ENTRIES);
> 
> That one I am not a big fan, as the number of ARL bins is a function of 
> the switch model, and this illustrates it well.

Ok, up to you to take or not what looks the better solution.

 From my point of view, the "for (i = 0; i < dev->num_arl_bins" below 
illustrates it better.


Maybe, another approach to save the memset() call would be remove the 
bitmap_zero() call, and declare 'free_bins' as:

    DECLARE_BITMAP(free_bins, B53_ARLTBL_MAX_BIN_ENTRIES) = { };
(this syntax is already used in b53_configure_vlan())


The compiler should still be able to optimize the initialisation and 
this wouldn't, IMHO, introduce confusion about the intent.

Let me know if you prefer to leave this hunk as-is, or if this other 
alternative pleases you.


CJ

> 
>>       /* Read the bins */
>>       for (i = 0; i < dev->num_arl_bins; i++) {
>> @@ -1641,7 +1641,7 @@ static int b53_arl_read(struct b53_device *dev, 
>> u64 mac,
>>           b53_arl_to_entry(ent, mac_vid, fwd_entry);
>>           if (!(fwd_entry & ARLTBL_VALID)) {
>> -            set_bit(i, free_bins);
>> +            __set_bit(i, free_bins);
> 
> I would be keen on taking that hunk but keep the other as-is. Does that 
> work for you?
> -- 
> Florian
>
diff mbox series

Patch

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 3464ce5e7470..8c55fe0e0747 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1627,7 +1627,7 @@  static int b53_arl_read(struct b53_device *dev, u64 mac,
 	if (ret)
 		return ret;
 
-	bitmap_zero(free_bins, dev->num_arl_bins);
+	bitmap_zero(free_bins, B53_ARLTBL_MAX_BIN_ENTRIES);
 
 	/* Read the bins */
 	for (i = 0; i < dev->num_arl_bins; i++) {
@@ -1641,7 +1641,7 @@  static int b53_arl_read(struct b53_device *dev, u64 mac,
 		b53_arl_to_entry(ent, mac_vid, fwd_entry);
 
 		if (!(fwd_entry & ARLTBL_VALID)) {
-			set_bit(i, free_bins);
+			__set_bit(i, free_bins);
 			continue;
 		}
 		if ((mac_vid & ARLTBL_MAC_MASK) != mac)