Re: [PATCH net-next 1/8] net: move netdev_upper to netdevice.h

[Jiri Pirko <jiri@resnulli.us>]

Mon, Aug 26, 2013 at 06:55:35PM CEST, vfalico@redhat.com wrote:
>On Mon, Aug 26, 2013 at 06:41:15PM +0200, Jiri Pirko wrote:
>>Mon, Aug 26, 2013 at 06:28:46PM CEST, vfalico@redhat.com wrote:
>...snip...
>>>+struct netdev_upper {
>>>+	struct net_device *dev;
>>>+	bool master;
>>>+	struct list_head list;
>>>+	struct rcu_head rcu;
>>>+	struct list_head search_list;
>>>+};
>>>+
>>
>>
>>I like your patchset.  However I'm not entirely comfortable with exposing
>>this struct. I would love to have it "under control" in net/core/dev.c
>
>I've taken this approach first, however the change to non-bonding stuff
>became a bit too big to justify the (only) bonding use.
>
>bonding only reads from it, and there are already primitives in dev.c to
>modify it, so if they will be used for it it's still the dev.c who controls
>it (if someone writes directly to it - it's a bug, and can be NAKed).
>
>>
>>I'm thinking of some getter/iterator for this use. It can work by
>>type as well so you would be able to remove the checks from bonding
>>code.
>
>There are 3 checks in bonding - looking for vlan devs, for a specific dev
>and for a specific ip address. list_for_each_entry() fits here perfectly
>for each case, otherwise the best way to do this would be to
>
>while ((next_dev = netdev_upper_get_next_dev(dev, next_dev)))

I was imagine something like:

struct list_head *iter;
struct net_device *dev, *upper;

netdev_for_each_upper_dev(dev, upper, iter) {

}

This macro can be easily implented using netdev_upper_get_next_dev()
from dev.c

Not much of added overhead other than netdev_upper_get_next_dev calls
(without any search when using list_head iter).
	
>
>or something like that, which adds quite a bit of overhead (looking for the
>previous dev and then returning the next one on each iteration), and looks
>ugly.
>
>So, given that it's a plain list actually, and any modification to this
>list can (and should be) done via functions from dev.c, while reading can
>be done with standard list_for_each_entry(_rcu)(), I think it's better to
>expose it this way.