Re: [PATCH v0] net: dsa: mv88e6xxx: Add FID map cache

[Aryan Srivastava]

Hi Marek,
On Tue, 2024-06-11 at 09:50 +0200, Marek Behún wrote:
> On Mon, 10 Jun 2024 17:07:23 +1200
> Aryan Srivastava <aryan.srivastava@xxxxxxxxxxxxxxxxxxx> wrote:
> 
> > Add a cached FID bitmap. This mitigates the need to
> > walk all VTU entries to find the next free FID.
> > 
> > Walk VTU once, then store read FID map into bitmap. Use
> > and manipulate this bitmap from now on, instead of re-reading
> > HW for the FID map.
> > 
> > The repeatedly VTU walks are costly can result in taking ~40 mins
> > if ~4000 vlans are added. Caching the FID map reduces this time
> > to <2 mins.
> > 
> > Signed-off-by: Aryan Srivastava
> > <aryan.srivastava@xxxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/net/dsa/mv88e6xxx/chip.c | 25 +++++++++++++++++++------
> >  drivers/net/dsa/mv88e6xxx/chip.h |  4 ++++
> >  2 files changed, 23 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c
> > b/drivers/net/dsa/mv88e6xxx/chip.c
> > index e5bac87941f6..91816e3e35ed 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -1815,14 +1815,17 @@ int mv88e6xxx_fid_map(struct mv88e6xxx_chip
> > *chip, unsigned long *fid_bitmap)
> >  
> >  static int mv88e6xxx_atu_new(struct mv88e6xxx_chip *chip, u16
> > *fid)
> >  {
> > -       DECLARE_BITMAP(fid_bitmap, MV88E6XXX_N_FID);
> >         int err;
> >  
> > -       err = mv88e6xxx_fid_map(chip, fid_bitmap);
> > -       if (err)
> > -               return err;
> > +       if (!chip->fid_populated) {
> > +               err = mv88e6xxx_fid_map(chip, chip->fid_bitmap);
> > +               if (err)
> > +                       return err;
> >  
> > -       *fid = find_first_zero_bit(fid_bitmap, MV88E6XXX_N_FID);
> > +               chip->fid_populated = true;
> > +       }
> > +
> > +       *fid = find_first_zero_bit(chip->fid_bitmap,
> > MV88E6XXX_N_FID);
> >         if (unlikely(*fid >= mv88e6xxx_num_databases(chip)))
> >                 return -ENOSPC;
> >  
> > @@ -2529,6 +2532,9 @@ static int mv88e6xxx_port_vlan_join(struct
> > mv88e6xxx_chip *chip, int port,
> >                          port, vid);
> >         }
> >  
> > +       /* Record FID used in SW FID map */
> > +       bitmap_set(chip->fid_bitmap, vlan.fid, 1);
> > +
> 
> wouldn't it make more sense to do this bit setting in
> mv88e6xxx_atu_new() and clearingin a new function
> mv88e6xxx_atu_delete/drop() ?
The reason I did not do this is to ensure that we only clear/set the SW
bit if the HW ops pass, i.e. I want to make sure calls like
mv88e6xxx_vtu_loadpurge and mv88e6xxx_g1_atu_remove pass before editing
the SW cache.

Thanks,
Aryan.