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

[Christophe JAILLET]

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@xxxxxxxxxx>
> > ---
> >   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