On Thu, Nov 19, 2020 at 06:43:38PM -0800, Florian Fainelli wrote:

On 11/19/2020 4:30 PM, Andrew Lunn wrote:

quoted

quoted

+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