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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201120133050.GF1804098@lunn.ch>
Date:   Fri, 20 Nov 2020 14:30:50 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Tobias Waldekranz <tobias@...dekranz.com>, davem@...emloft.net,
        kuba@...nel.org, vivien.didelot@...il.com, olteanv@...il.com,
        j.vosburgh@...il.com, vfalico@...il.com, andy@...yhouse.net,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/4] net: dsa: Link aggregation support

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