On Sun, 29 Sep 2019 at 04:36, Johannes Berg [off-list ref] wrote:

Hi,

quoted

 int netdev_walk_all_upper_dev_rcu(struct net_device *dev,
                                int (*fn)(struct net_device *dev,
                                          void *data),
                                void *data)
 {

[...]

quoted

      }

      return 0;
+
 }

that seems like an oversight, probably from editing the patch in
different versions?

I will fix this in a v5 patch.

quoted

+static int __netdev_update_upper_level(struct net_device *dev, void *data)
+{
+     dev->upper_level = __netdev_upper_depth(dev) + 1;
+     return 0;
+}
+
+static int __netdev_update_lower_level(struct net_device *dev, void *data)
+{
+     dev->lower_level = __netdev_lower_depth(dev) + 1;
+     return 0;
+}

Is there any point in the return value here? You don't really use it,
afaict? I guess I might see the point if it was used for tail-call
optimisation or such?

These functions are used as a callback function of
netdev_walk_all_{upper/lower}_dev(). So these return types are needed.

Also, I dunno, I guess netdevs aren't as much under pressure as SKBs :-)
but do we actually gain much from storing the nesting level at all? You
have to maintain it all the time anyway when adding/removing and that's
the only place where you also check it, so perhaps it wouldn't be that
bad to just count at that time?

But then again the counting would probably be recursive again ...

quoted

      return 0;
+
 }
 EXPORT_SYMBOL_GPL(netdev_walk_all_lower_dev_rcu);

same nit as above

I will fix this in a v5 patch too.

quoted

+     __netdev_update_upper_level(dev, NULL);
+     netdev_walk_all_lower_dev(dev, __netdev_update_upper_level, NULL);
+
+     __netdev_update_lower_level(upper_dev, NULL);
+     netdev_walk_all_upper_dev(upper_dev, __netdev_update_lower_level, NULL);

Actually, if I'm reading this correctly you already walk all the levels
anyway? Then couldn't you calculate the depth at this time and return
it, instead of storing it? Though, if it actually overflowed then you'd
have to walk *again* to undo that?

Hmm, actually, if you don't store the value you don't even need to walk
here I guess, or at least you would only have to do it to verify you
*can* attach, but wouldn't have to in detach?

So it looks to me like on attach (i.e. this code, quoted from
__netdev_upper_dev_link) you're already walking the entire graph to
update the level values, and could probably instead calculate the
nesting depth to validate it?
And then on netdev_upper_dev_unlink() you wouldn't even have to walk the
graph at all, since you only need that to update the values that you
stored.

But maybe I'm misinterpreting this completely?

Without storing level storing, a walking graph routine is needed only
once. The routine would work as a nesting depth validator.
So that the detach routine doesn't need to walk the graph.
Whereas, in this patch, both attach and detach routine need to
walk graph. So, storing nesting variable way is slower than without
storing nesting variable way because of the detach routine's updating
upper and lower level routine.

But I'm sure that storing nesting variables is useful because other
modules already using nesting level values.
Please look at vlan_get_encap_level() and usecases.
If we don't provide nesting level variables, they should calculate
every time when they need it and this way is easier way to get a
nesting level. There are use-cases of lower_level variable
in the 11th patch.

Thank you

Thanks,
johannes