Re: [PATCH net-next 2/4] net: dsa: Link aggregation support

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Nov 19, 2020 at 06:43:38PM -0800, Florian Fainelli wrote:
> 
> 
> On 11/19/2020 4:30 PM, Andrew Lunn wrote:
> >> +static struct dsa_lag *dsa_lag_get(struct dsa_switch_tree *dst,
> >> +				   struct net_device *dev)
> >> +{
> >> +	unsigned long busy = 0;
> >> +	struct dsa_lag *lag;
> >> +	int id;
> >> +
> >> +	list_for_each_entry(lag, &dst->lags, list) {
> >> +		set_bit(lag->id, &busy);
> >> +
> >> +		if (lag->dev == dev) {
> >> +			kref_get(&lag->refcount);
> >> +			return lag;
> >> +		}
> >> +	}
> >> +
> >> +	id = find_first_zero_bit(&busy, BITS_PER_LONG);
> >> +	if (id >= BITS_PER_LONG)
> >> +		return ERR_PTR(-ENOSPC);
> >> +
> >> +	lag = kzalloc(sizeof(*lag), GFP_KERNEL);
> >> +	if (!lag)
> >> +		return ERR_PTR(-ENOMEM);
> > 
> > Hi Tobias
> > 
> > My comment last time was to statically allocated them at probe
> > time. Worse case scenario is each port is alone in a LAG. Pointless,
> > but somebody could configure it. In dsa_tree_setup_switches() you can
> > count the number of ports and then allocate an array, or while setting
> > up a port, add one more lag to the list of lags.
> 
> The allocation is allowed to sleep (have not checked the calling context
> of dsa_lag_get() whether this is OK) so what would be the upside of
> doing upfront dsa_lag allocation which could be wasteful?

Hi Florian

It fits the pattern for the rest of the DSA core. We never allocate
anything at runtime. That keeps the error handling simple, we don't
need to deal with ENOMEM errors, undoing whatever we might of done,
implementing transactions etc.

And the memory involved here is small. I guess around 80 bytes per
lag? So even for a 10 port switch, we are only talking about 800
bytes. That is not a lot.

       Andrew