Received-SPF: pass (google.com: domain of linux-kernel+bounces-209416-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 11 Jun 2024 09:50:16 +0200
From: Marek =?UTF-8?B?QmVow7pu?= <kabel@kernel.org>
To: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
Cc: Andrew Lunn <andrew@lunn.ch>, Florian Fainelli <f.fainelli@gmail.com>,
 Vladimir Oltean <olteanv@gmail.com>, "David S. Miller"
 <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski
 <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH v0] net: dsa: mv88e6xxx: Add FID map cache
Message-ID: <20240611095016.7804b091@dellmb>
In-Reply-To: <20240610050724.2439780-1-aryan.srivastava@alliedtelesis.co.nz>
References: <20240610050724.2439780-1-aryan.srivastava@alliedtelesis.co.nz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 10 Jun 2024 17:07:23 +1200
Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz> 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@alliedtelesis.co.nz>
> ---
>  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() ?