Re: [patch net-next V5] net: introduce ethernet teaming device

[Eric Dumazet <eric.dumazet@gmail.com>]

Le mardi 25 octobre 2011 à 15:03 +0200, Jiri Pirko a écrit :
> This patch introduces new network device called team. It supposes to be
> very fast, simple, userspace-driven alternative to existing bonding
> driver.
> 
> Userspace library called libteam with couple of demo apps is available
> here:
> https://github.com/jpirko/libteam
> Note it's still in its dipers atm.
> 
> team<->libteam use generic netlink for communication. That and rtnl
> suppose to be the only way to configure team device, no sysfs etc.
> 
> Python binding basis for libteam was recently introduced (some need
> still need to be done on it though). Daemon providing arpmon/miimon
> active-backup functionality will be introduced shortly.
> All what's necessary is already implemented in kernel team driver.
> 
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
> 
> v4->v5:
> 	- team_change_mtu() uses team->lock while travesing though port
> 	  list
> 	- mac address changes are moved completely to jurisdiction of
> 	  userspace daemon. This way the daemon can do FOM1, FOM2 and
> 	  possibly other weird things with mac addresses.
> 	  Only round-robin mode sets up all ports to bond's address then
> 	  enslaved.
> 	- Extended Kconfig text


Following is not called under rcu, but rtnl or team spinlock held.

Therefore, team_get_port_by_index_rcu() is not the right thing.

+static void __reconstruct_port_hlist(struct team *team, int rm_index)
+{
+       int i;
+       struct team_port *port;
+
+       for (i = rm_index + 1; i < team->port_count; i++) {
+               port = team_get_port_by_index_rcu(team, i);
+               hlist_del_rcu(&port->hlist);
+               port->index--;
+               hlist_add_head_rcu(&port->hlist,
+                                  team_port_index_hash(team, port->index));
+       }
+}
+

In fact, I claim most of your rcu_read_lock() in management side are
bogus and obfuscate code.

RCU is an exact science, not a commodity.

When RCU is used, rcu_read_lock()/rcu_read_unlock() are used by readers,
not managers :

They should use a spin/mutex(rtnl usually in network land)
and normal reads, no need for rcu_something

Only writes must take care of concurrent readers (aka rcu_assign_pointer())

For example:

team_nl_fill_port_list_get_changed() should not use
	list_for_each_entry_rcu(port, &team->port_list, list) {
but a regular
	list_for_each_entry(port, &team->port_list, list) {


team_nl_team_get() should not play with RCU either.
	It can use __dev_get_by_index() instead of dev_get_by_index_rcu()

Comment in front of team_nl_team_get() is bogus as well :

/*
 * Netlink cmd functions should be locked by following two functions.
 * To ensure team_uninit would not be called in between, hold rcu_read_lock
 * all the time.
 */

How can holding rcu_read_lock() can prevent another cpu doing whatever he wants ?

It seems you believe rcu_read_lock() is a read_lock(), but it isnt.

Using right API is essential to get appropriate LOCKDEP semantic and
code maintainability.