[patch net-next-2.6] net: make dev->master general

[patch net-next-2.6] net: make dev->master general

[Jiri Pirko]

dev->master is now tightly connected to bonding driver. This patch makes
this pointer more general and ready to be used by others.

 - netdev_set_master() - bond specifics moved to new function
   netdev_set_bond_master()
 - introduced netif_is_bond_slave() to check if device is a bonding slave

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/infiniband/hw/nes/nes.c    |    3 +-
 drivers/infiniband/hw/nes/nes_cm.c |    2 +-
 drivers/net/bonding/bond_main.c    |   10 +++---
 drivers/net/cxgb3/cxgb3_offload.c  |    3 +-
 include/linux/netdevice.h          |    7 +++++
 net/core/dev.c                     |   49 +++++++++++++++++++++++++++---------
 6 files changed, 54 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/hw/nes/nes.c b/drivers/infiniband/hw/nes/nes.c
index 3b4ec32..3d7f366 100644
--- a/drivers/infiniband/hw/nes/nes.c
+++ b/drivers/infiniband/hw/nes/nes.c
@@ -153,7 +153,8 @@ static int nes_inetaddr_event(struct notifier_block *notifier,
 				nesdev, nesdev->netdev[0]->name);
 		netdev = nesdev->netdev[0];
 		nesvnic = netdev_priv(netdev);
-		is_bonded = (netdev->master == event_netdev);
+		is_bonded = netif_is_bond_slave(netdev) &&
+			    (netdev->master == event_netdev);
 		if ((netdev == event_netdev) || is_bonded) {
 			if (nesvnic->rdma_enabled == 0) {
 				nes_debug(NES_DBG_NETDEV, "Returning without processing event for %s since"
diff --git a/drivers/infiniband/hw/nes/nes_cm.c b/drivers/infiniband/hw/nes/nes_cm.c
index 009ec81..ec3aa11 100644
--- a/drivers/infiniband/hw/nes/nes_cm.c
+++ b/drivers/infiniband/hw/nes/nes_cm.c
@@ -1118,7 +1118,7 @@ static int nes_addr_resolve_neigh(struct nes_vnic *nesvnic, u32 dst_ip, int arpi
 		return rc;
 	}
 
-	if (nesvnic->netdev->master)
+	if (netif_is_bond_slave(netdev))
 		netdev = nesvnic->netdev->master;
 	else
 		netdev = nesvnic->netdev;
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 1df9f0e..9f87787 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1594,9 +1594,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		}
 	}
 
-	res = netdev_set_master(slave_dev, bond_dev);
+	res = netdev_set_bond_master(slave_dev, bond_dev);
 	if (res) {
-		pr_debug("Error %d calling netdev_set_master\n", res);
+		pr_debug("Error %d calling netdev_set_bond_master\n", res);
 		goto err_restore_mac;
 	}
 	/* open the slave since the application closed it */
@@ -1812,7 +1812,7 @@ err_close:
 	dev_close(slave_dev);
 
 err_unset_master:
-	netdev_set_master(slave_dev, NULL);
+	netdev_set_bond_master(slave_dev, NULL);
 
 err_restore_mac:
 	if (!bond->params.fail_over_mac) {
@@ -1992,7 +1992,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
 		netif_addr_unlock_bh(bond_dev);
 	}
 
-	netdev_set_master(slave_dev, NULL);
+	netdev_set_bond_master(slave_dev, NULL);
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	read_lock_bh(&bond->lock);
@@ -2114,7 +2114,7 @@ static int bond_release_all(struct net_device *bond_dev)
 			netif_addr_unlock_bh(bond_dev);
 		}
 
-		netdev_set_master(slave_dev, NULL);
+		netdev_set_bond_master(slave_dev, NULL);
 
 		/* close slave before restoring its mac address */
 		dev_close(slave_dev);
diff --git a/drivers/net/cxgb3/cxgb3_offload.c b/drivers/net/cxgb3/cxgb3_offload.c
index 7ea94b5..862804f 100644
--- a/drivers/net/cxgb3/cxgb3_offload.c
+++ b/drivers/net/cxgb3/cxgb3_offload.c
@@ -186,9 +186,10 @@ static struct net_device *get_iff_from_mac(struct adapter *adapter,
 				dev = NULL;
 				if (grp)
 					dev = vlan_group_get_device(grp, vlan);
-			} else
+			} else if (netif_is_bond_slave(dev)) {
 				while (dev->master)
 					dev = dev->master;
+			}
 			return dev;
 		}
 	}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5a5baea..5a42b10 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2377,6 +2377,8 @@ extern int		netdev_max_backlog;
 extern int		netdev_tstamp_prequeue;
 extern int		weight_p;
 extern int		netdev_set_master(struct net_device *dev, struct net_device *master);
+extern int netdev_set_bond_master(struct net_device *dev,
+				  struct net_device *master);
 extern int skb_checksum_help(struct sk_buff *skb);
 extern struct sk_buff *skb_gso_segment(struct sk_buff *skb, u32 features);
 #ifdef CONFIG_BUG
@@ -2437,6 +2439,11 @@ static inline void netif_set_gso_max_size(struct net_device *dev,
 	dev->gso_max_size = size;
 }
 
+static inline int netif_is_bond_slave(struct net_device *dev)
+{
+	return dev->flags & IFF_SLAVE && dev->priv_flags & IFF_BONDING;
+}
+
 extern struct pernet_operations __net_initdata loopback_net_ops;
 
 static inline int dev_ethtool_get_settings(struct net_device *dev,
diff --git a/net/core/dev.c b/net/core/dev.c
index d874fd1..a413276 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3146,7 +3146,6 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	struct packet_type *ptype, *pt_prev;
 	rx_handler_func_t *rx_handler;
 	struct net_device *orig_dev;
-	struct net_device *master;
 	struct net_device *null_or_orig;
 	struct net_device *orig_or_bond;
 	int ret = NET_RX_DROP;
@@ -3173,15 +3172,19 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	 */
 	null_or_orig = NULL;
 	orig_dev = skb->dev;
-	master = ACCESS_ONCE(orig_dev->master);
 	if (skb->deliver_no_wcard)
 		null_or_orig = orig_dev;
-	else if (master) {
-		if (__skb_bond_should_drop(skb, master)) {
-			skb->deliver_no_wcard = 1;
-			null_or_orig = orig_dev; /* deliver only exact match */
-		} else
-			skb->dev = master;
+	else if (netif_is_bond_slave(orig_dev)) {
+		struct net_device *bond_master = ACCESS_ONCE(orig_dev->master);
+
+		if (likely(bond_master)) {
+			if (__skb_bond_should_drop(skb, bond_master)) {
+				skb->deliver_no_wcard = 1;
+				/* deliver only exact match */
+				null_or_orig = orig_dev;
+			} else
+				skb->dev = bond_master;
+		}
 	}
 
 	__this_cpu_inc(softnet_data.processed);
@@ -4346,15 +4349,14 @@ static int __init dev_proc_init(void)
 
 
 /**
- *	netdev_set_master	-	set up master/slave pair
+ *	netdev_set_master	-	set up master pointer
  *	@slave: slave device
  *	@master: new master device
  *
  *	Changes the master device of the slave. Pass %NULL to break the
  *	bonding. The caller must hold the RTNL semaphore. On a failure
  *	a negative errno code is returned. On success the reference counts
- *	are adjusted, %RTM_NEWLINK is sent to the routing socket and the
- *	function returns zero.
+ *	are adjusted and the function returns zero.
  */
 int netdev_set_master(struct net_device *slave, struct net_device *master)
 {
@@ -4374,6 +4376,29 @@ int netdev_set_master(struct net_device *slave, struct net_device *master)
 		synchronize_net();
 		dev_put(old);
 	}
+	return 0;
+}
+EXPORT_SYMBOL(netdev_set_master);
+
+/**
+ *	netdev_set_bond_master	-	set up bonding master/slave pair
+ *	@slave: slave device
+ *	@master: new master device
+ *
+ *	Changes the master device of the slave. Pass %NULL to break the
+ *	bonding. The caller must hold the RTNL semaphore. On a failure
+ *	a negative errno code is returned. On success %RTM_NEWLINK is sent
+ *	to the routing socket and the function returns zero.
+ */
+int netdev_set_bond_master(struct net_device *slave, struct net_device *master)
+{
+	int err;
+
+	ASSERT_RTNL();
+
+	err = netdev_set_master(slave, master);
+	if (err)
+		return err;
 	if (master)
 		slave->flags |= IFF_SLAVE;
 	else
@@ -4382,7 +4407,7 @@ int netdev_set_master(struct net_device *slave, struct net_device *master)
 	rtmsg_ifinfo(RTM_NEWLINK, slave, IFF_SLAVE);
 	return 0;
 }
-EXPORT_SYMBOL(netdev_set_master);
+EXPORT_SYMBOL(netdev_set_bond_master);
 
 static void dev_change_rx_flags(struct net_device *dev, int flags)
 {
-- 
1.7.3.4

Re: [patch net-next-2.6] net: make dev->master general

[David Miller]

From: Jiri Pirko <jpirko@redhat.com>
Date: Sat, 12 Feb 2011 17:48:36 +0100

> dev->master is now tightly connected to bonding driver. This patch makes
> this pointer more general and ready to be used by others.
> 
>  - netdev_set_master() - bond specifics moved to new function
>    netdev_set_bond_master()
>  - introduced netif_is_bond_slave() to check if device is a bonding slave
> 
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>

Applied.

Re: [patch net-next-2.6] net: make dev->master general

[Nicolas de Pesloüan]

Le 12/02/2011 17:48, Jiri Pirko a écrit :
> dev->master is now tightly connected to bonding driver. This patch makes
> this pointer more general and ready to be used by others.
>
>   - netdev_set_master() - bond specifics moved to new function
>     netdev_set_bond_master()
>   - introduced netif_is_bond_slave() to check if device is a bonding slave
>
> Signed-off-by: Jiri Pirko<jpirko@redhat.com>

Hi Jiri,

Even if DaveM already applied your patch, I'm not comfortable with it.

What is the rational behind it? Do you have anything in mind to use the now "more general" master 
field of net_device?

Of course, I won't advocate for every fields having only a single possible usage, but, using master 
for several different things might jeopardize our ability to share an interface between several 
logical interface systems:

Due to the current usage of the rx_handler field in net_device, the code suggest that an interface 
cannot be part of a bridge and of a macvlan at the same time. Even if bridge provide an hook for 
ebtables to ignore an skb and allow other to get it, macvlan cannot be registered on the same lower 
interface as a bridge, because rx_handler can only hold a single value.

By giving master a more general meaning, I think we might face a similar problem. It might disallow 
an interface to be enslaved to bonding and part of another logical interface at the same time, if 
such logical interface also use the master field.

It doesn't mean I don't support the general idea, but I would like to clearly understand the 
possible unexpected impact: do we want to stop interfaces to be "enslaved" to several logical 
interface at the same time?

> ---
>   drivers/infiniband/hw/nes/nes.c    |    3 +-
>   drivers/infiniband/hw/nes/nes_cm.c |    2 +-
>   drivers/net/bonding/bond_main.c    |   10 +++---
>   drivers/net/cxgb3/cxgb3_offload.c  |    3 +-
>   include/linux/netdevice.h          |    7 +++++
>   net/core/dev.c                     |   49 +++++++++++++++++++++++++++---------
>   6 files changed, 54 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/infiniband/hw/nes/nes.c b/drivers/infiniband/hw/nes/nes.c
> index 3b4ec32..3d7f366 100644
> --- a/drivers/infiniband/hw/nes/nes.c
> +++ b/drivers/infiniband/hw/nes/nes.c
> @@ -153,7 +153,8 @@ static int nes_inetaddr_event(struct notifier_block *notifier,
>   				nesdev, nesdev->netdev[0]->name);
>   		netdev = nesdev->netdev[0];
>   		nesvnic = netdev_priv(netdev);
> -		is_bonded = (netdev->master == event_netdev);
> +		is_bonded = netif_is_bond_slave(netdev)&&
> +			    (netdev->master == event_netdev);
>   		if ((netdev == event_netdev) || is_bonded) {
>   			if (nesvnic->rdma_enabled == 0) {
>   				nes_debug(NES_DBG_NETDEV, "Returning without processing event for %s since"
> diff --git a/drivers/infiniband/hw/nes/nes_cm.c b/drivers/infiniband/hw/nes/nes_cm.c
> index 009ec81..ec3aa11 100644
> --- a/drivers/infiniband/hw/nes/nes_cm.c
> +++ b/drivers/infiniband/hw/nes/nes_cm.c
> @@ -1118,7 +1118,7 @@ static int nes_addr_resolve_neigh(struct nes_vnic *nesvnic, u32 dst_ip, int arpi
>   		return rc;
>   	}
>
> -	if (nesvnic->netdev->master)
> +	if (netif_is_bond_slave(netdev))
>   		netdev = nesvnic->netdev->master;
>   	else
>   		netdev = nesvnic->netdev;
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 1df9f0e..9f87787 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1594,9 +1594,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>   		}
>   	}
>
> -	res = netdev_set_master(slave_dev, bond_dev);
> +	res = netdev_set_bond_master(slave_dev, bond_dev);
>   	if (res) {
> -		pr_debug("Error %d calling netdev_set_master\n", res);
> +		pr_debug("Error %d calling netdev_set_bond_master\n", res);
>   		goto err_restore_mac;
>   	}
>   	/* open the slave since the application closed it */
> @@ -1812,7 +1812,7 @@ err_close:
>   	dev_close(slave_dev);
>
>   err_unset_master:
> -	netdev_set_master(slave_dev, NULL);
> +	netdev_set_bond_master(slave_dev, NULL);
>
>   err_restore_mac:
>   	if (!bond->params.fail_over_mac) {
> @@ -1992,7 +1992,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
>   		netif_addr_unlock_bh(bond_dev);
>   	}
>
> -	netdev_set_master(slave_dev, NULL);
> +	netdev_set_bond_master(slave_dev, NULL);
>
>   #ifdef CONFIG_NET_POLL_CONTROLLER
>   	read_lock_bh(&bond->lock);
> @@ -2114,7 +2114,7 @@ static int bond_release_all(struct net_device *bond_dev)
>   			netif_addr_unlock_bh(bond_dev);
>   		}
>
> -		netdev_set_master(slave_dev, NULL);
> +		netdev_set_bond_master(slave_dev, NULL);
>
>   		/* close slave before restoring its mac address */
>   		dev_close(slave_dev);
> diff --git a/drivers/net/cxgb3/cxgb3_offload.c b/drivers/net/cxgb3/cxgb3_offload.c
> index 7ea94b5..862804f 100644
> --- a/drivers/net/cxgb3/cxgb3_offload.c
> +++ b/drivers/net/cxgb3/cxgb3_offload.c
> @@ -186,9 +186,10 @@ static struct net_device *get_iff_from_mac(struct adapter *adapter,
>   				dev = NULL;
>   				if (grp)
>   					dev = vlan_group_get_device(grp, vlan);
> -			} else
> +			} else if (netif_is_bond_slave(dev)) {
>   				while (dev->master)
>   					dev = dev->master;

The while() here suggests that nesting bonding is possible. This is not true (even if not enforced 
yet). And even if it was true, then you needed to use netif_is_bond_slave(dev) inside the while() 
loop, to be consistent.

> +			}
>   			return dev;
>   		}
>   	}
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 5a5baea..5a42b10 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2377,6 +2377,8 @@ extern int		netdev_max_backlog;
>   extern int		netdev_tstamp_prequeue;
>   extern int		weight_p;
>   extern int		netdev_set_master(struct net_device *dev, struct net_device *master);
> +extern int netdev_set_bond_master(struct net_device *dev,
> +				  struct net_device *master);
>   extern int skb_checksum_help(struct sk_buff *skb);
>   extern struct sk_buff *skb_gso_segment(struct sk_buff *skb, u32 features);
>   #ifdef CONFIG_BUG
> @@ -2437,6 +2439,11 @@ static inline void netif_set_gso_max_size(struct net_device *dev,
>   	dev->gso_max_size = size;
>   }
>
> +static inline int netif_is_bond_slave(struct net_device *dev)
> +{
> +	return dev->flags&  IFF_SLAVE&&  dev->priv_flags&  IFF_BONDING;
> +}
> +

Does this means you also consider IFF_SLAVE as a "more general" flag now? Wasn't IFF_SLAVE a 
sufficient evidence of the interface being enslaved to a bonding interface, before?

>   extern struct pernet_operations __net_initdata loopback_net_ops;
>
>   static inline int dev_ethtool_get_settings(struct net_device *dev,
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d874fd1..a413276 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3146,7 +3146,6 @@ static int __netif_receive_skb(struct sk_buff *skb)
>   	struct packet_type *ptype, *pt_prev;
>   	rx_handler_func_t *rx_handler;
>   	struct net_device *orig_dev;
> -	struct net_device *master;
>   	struct net_device *null_or_orig;
>   	struct net_device *orig_or_bond;
>   	int ret = NET_RX_DROP;
> @@ -3173,15 +3172,19 @@ static int __netif_receive_skb(struct sk_buff *skb)
>   	 */
>   	null_or_orig = NULL;
>   	orig_dev = skb->dev;
> -	master = ACCESS_ONCE(orig_dev->master);
>   	if (skb->deliver_no_wcard)
>   		null_or_orig = orig_dev;
> -	else if (master) {
> -		if (__skb_bond_should_drop(skb, master)) {
> -			skb->deliver_no_wcard = 1;
> -			null_or_orig = orig_dev; /* deliver only exact match */
> -		} else
> -			skb->dev = master;
> +	else if (netif_is_bond_slave(orig_dev)) {
> +		struct net_device *bond_master = ACCESS_ONCE(orig_dev->master);
> +
> +		if (likely(bond_master)) {
> +			if (__skb_bond_should_drop(skb, bond_master)) {
> +				skb->deliver_no_wcard = 1;
> +				/* deliver only exact match */
> +				null_or_orig = orig_dev;
> +			} else
> +				skb->dev = bond_master;
> +		}

I think we need an "else" here. If orig_dev->master is NULL, while netif_is_bond_slave(orig_dev) is 
TRUE, we apparently face an unexpected situation and we should at least issue a warning.

>   	}
>
>   	__this_cpu_inc(softnet_data.processed);
> @@ -4346,15 +4349,14 @@ static int __init dev_proc_init(void)
>
>
>   /**
> - *	netdev_set_master	-	set up master/slave pair
> + *	netdev_set_master	-	set up master pointer
>    *	@slave: slave device
>    *	@master: new master device
>    *
>    *	Changes the master device of the slave. Pass %NULL to break the
>    *	bonding. The caller must hold the RTNL semaphore. On a failure
>    *	a negative errno code is returned. On success the reference counts
> - *	are adjusted, %RTM_NEWLINK is sent to the routing socket and the
> - *	function returns zero.
> + *	are adjusted and the function returns zero.
>    */
>   int netdev_set_master(struct net_device *slave, struct net_device *master)
>   {
> @@ -4374,6 +4376,29 @@ int netdev_set_master(struct net_device *slave, struct net_device *master)
>   		synchronize_net();
>   		dev_put(old);
>   	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(netdev_set_master);
> +
> +/**
> + *	netdev_set_bond_master	-	set up bonding master/slave pair
> + *	@slave: slave device
> + *	@master: new master device
> + *
> + *	Changes the master device of the slave. Pass %NULL to break the
> + *	bonding. The caller must hold the RTNL semaphore. On a failure
> + *	a negative errno code is returned. On success %RTM_NEWLINK is sent
> + *	to the routing socket and the function returns zero.
> + */
> +int netdev_set_bond_master(struct net_device *slave, struct net_device *master)
> +{
> +	int err;
> +
> +	ASSERT_RTNL();
> +
> +	err = netdev_set_master(slave, master);
> +	if (err)
> +		return err;
>   	if (master)
>   		slave->flags |= IFF_SLAVE;
>   	else
> @@ -4382,7 +4407,7 @@ int netdev_set_master(struct net_device *slave, struct net_device *master)
>   	rtmsg_ifinfo(RTM_NEWLINK, slave, IFF_SLAVE);
>   	return 0;
>   }
> -EXPORT_SYMBOL(netdev_set_master);
> +EXPORT_SYMBOL(netdev_set_bond_master);
>
>   static void dev_change_rx_flags(struct net_device *dev, int flags)
>   {

Re: [patch net-next-2.6] net: make dev->master general

[Jiri Pirko]

Mon, Feb 14, 2011 at 09:48:44AM CET, nicolas.2p.debian@gmail.com wrote:
>Le 12/02/2011 17:48, Jiri Pirko a écrit :
>>dev->master is now tightly connected to bonding driver. This patch makes
>>this pointer more general and ready to be used by others.
>>
>>  - netdev_set_master() - bond specifics moved to new function
>>    netdev_set_bond_master()
>>  - introduced netif_is_bond_slave() to check if device is a bonding slave
>>
>>Signed-off-by: Jiri Pirko<jpirko@redhat.com>
>
>Hi Jiri,
>
>Even if DaveM already applied your patch, I'm not comfortable with it.
>
>What is the rational behind it? Do you have anything in mind to use
>the now "more general" master field of net_device?
>
>Of course, I won't advocate for every fields having only a single
>possible usage, but, using master for several different things might
>jeopardize our ability to share an interface between several logical
>interface systems:
>
>Due to the current usage of the rx_handler field in net_device, the
>code suggest that an interface cannot be part of a bridge and of a
>macvlan at the same time. Even if bridge provide an hook for ebtables
>to ignore an skb and allow other to get it, macvlan cannot be
>registered on the same lower interface as a bridge, because
>rx_handler can only hold a single value.
>
>By giving master a more general meaning, I think we might face a
>similar problem. It might disallow an interface to be enslaved to
>bonding and part of another logical interface at the same time, if
>such logical interface also use the master field.

That is true. I think that it makes no sense to have iface enslaved in
bond and bridge at the same time. Do you have a scenario where it makes
sense?

>
>It doesn't mean I don't support the general idea, but I would like to
>clearly understand the possible unexpected impact: do we want to stop
>interfaces to be "enslaved" to several logical interface at the same
>time?
>
>>---
>>  drivers/infiniband/hw/nes/nes.c    |    3 +-
>>  drivers/infiniband/hw/nes/nes_cm.c |    2 +-
>>  drivers/net/bonding/bond_main.c    |   10 +++---
>>  drivers/net/cxgb3/cxgb3_offload.c  |    3 +-
>>  include/linux/netdevice.h          |    7 +++++
>>  net/core/dev.c                     |   49 +++++++++++++++++++++++++++---------
>>  6 files changed, 54 insertions(+), 20 deletions(-)
>>
>>diff --git a/drivers/infiniband/hw/nes/nes.c b/drivers/infiniband/hw/nes/nes.c
>>index 3b4ec32..3d7f366 100644
>>--- a/drivers/infiniband/hw/nes/nes.c
>>+++ b/drivers/infiniband/hw/nes/nes.c
>>@@ -153,7 +153,8 @@ static int nes_inetaddr_event(struct notifier_block *notifier,
>>  				nesdev, nesdev->netdev[0]->name);
>>  		netdev = nesdev->netdev[0];
>>  		nesvnic = netdev_priv(netdev);
>>-		is_bonded = (netdev->master == event_netdev);
>>+		is_bonded = netif_is_bond_slave(netdev)&&
>>+			    (netdev->master == event_netdev);
>>  		if ((netdev == event_netdev) || is_bonded) {
>>  			if (nesvnic->rdma_enabled == 0) {
>>  				nes_debug(NES_DBG_NETDEV, "Returning without processing event for %s since"
>>diff --git a/drivers/infiniband/hw/nes/nes_cm.c b/drivers/infiniband/hw/nes/nes_cm.c
>>index 009ec81..ec3aa11 100644
>>--- a/drivers/infiniband/hw/nes/nes_cm.c
>>+++ b/drivers/infiniband/hw/nes/nes_cm.c
>>@@ -1118,7 +1118,7 @@ static int nes_addr_resolve_neigh(struct nes_vnic *nesvnic, u32 dst_ip, int arpi
>>  		return rc;
>>  	}
>>
>>-	if (nesvnic->netdev->master)
>>+	if (netif_is_bond_slave(netdev))
>>  		netdev = nesvnic->netdev->master;
>>  	else
>>  		netdev = nesvnic->netdev;
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index 1df9f0e..9f87787 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1594,9 +1594,9 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>  		}
>>  	}
>>
>>-	res = netdev_set_master(slave_dev, bond_dev);
>>+	res = netdev_set_bond_master(slave_dev, bond_dev);
>>  	if (res) {
>>-		pr_debug("Error %d calling netdev_set_master\n", res);
>>+		pr_debug("Error %d calling netdev_set_bond_master\n", res);
>>  		goto err_restore_mac;
>>  	}
>>  	/* open the slave since the application closed it */
>>@@ -1812,7 +1812,7 @@ err_close:
>>  	dev_close(slave_dev);
>>
>>  err_unset_master:
>>-	netdev_set_master(slave_dev, NULL);
>>+	netdev_set_bond_master(slave_dev, NULL);
>>
>>  err_restore_mac:
>>  	if (!bond->params.fail_over_mac) {
>>@@ -1992,7 +1992,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
>>  		netif_addr_unlock_bh(bond_dev);
>>  	}
>>
>>-	netdev_set_master(slave_dev, NULL);
>>+	netdev_set_bond_master(slave_dev, NULL);
>>
>>  #ifdef CONFIG_NET_POLL_CONTROLLER
>>  	read_lock_bh(&bond->lock);
>>@@ -2114,7 +2114,7 @@ static int bond_release_all(struct net_device *bond_dev)
>>  			netif_addr_unlock_bh(bond_dev);
>>  		}
>>
>>-		netdev_set_master(slave_dev, NULL);
>>+		netdev_set_bond_master(slave_dev, NULL);
>>
>>  		/* close slave before restoring its mac address */
>>  		dev_close(slave_dev);
>>diff --git a/drivers/net/cxgb3/cxgb3_offload.c b/drivers/net/cxgb3/cxgb3_offload.c
>>index 7ea94b5..862804f 100644
>>--- a/drivers/net/cxgb3/cxgb3_offload.c
>>+++ b/drivers/net/cxgb3/cxgb3_offload.c
>>@@ -186,9 +186,10 @@ static struct net_device *get_iff_from_mac(struct adapter *adapter,
>>  				dev = NULL;
>>  				if (grp)
>>  					dev = vlan_group_get_device(grp, vlan);
>>-			} else
>>+			} else if (netif_is_bond_slave(dev)) {
>>  				while (dev->master)
>>  					dev = dev->master;
>
>The while() here suggests that nesting bonding is possible. This is
>not true (even if not enforced yet). And even if it was true, then
>you needed to use netif_is_bond_slave(dev) inside the while() loop,
>to be consistent.
>
>>+			}
>>  			return dev;
>>  		}
>>  	}
>>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>index 5a5baea..5a42b10 100644
>>--- a/include/linux/netdevice.h
>>+++ b/include/linux/netdevice.h
>>@@ -2377,6 +2377,8 @@ extern int		netdev_max_backlog;
>>  extern int		netdev_tstamp_prequeue;
>>  extern int		weight_p;
>>  extern int		netdev_set_master(struct net_device *dev, struct net_device *master);
>>+extern int netdev_set_bond_master(struct net_device *dev,
>>+				  struct net_device *master);
>>  extern int skb_checksum_help(struct sk_buff *skb);
>>  extern struct sk_buff *skb_gso_segment(struct sk_buff *skb, u32 features);
>>  #ifdef CONFIG_BUG
>>@@ -2437,6 +2439,11 @@ static inline void netif_set_gso_max_size(struct net_device *dev,
>>  	dev->gso_max_size = size;
>>  }
>>
>>+static inline int netif_is_bond_slave(struct net_device *dev)
>>+{
>>+	return dev->flags&  IFF_SLAVE&&  dev->priv_flags&  IFF_BONDING;
>>+}
>>+
>
>Does this means you also consider IFF_SLAVE as a "more general" flag
>now? Wasn't IFF_SLAVE a sufficient evidence of the interface being
>enslaved to a bonding interface, before?
>
>>  extern struct pernet_operations __net_initdata loopback_net_ops;
>>
>>  static inline int dev_ethtool_get_settings(struct net_device *dev,
>>diff --git a/net/core/dev.c b/net/core/dev.c
>>index d874fd1..a413276 100644
>>--- a/net/core/dev.c
>>+++ b/net/core/dev.c
>>@@ -3146,7 +3146,6 @@ static int __netif_receive_skb(struct sk_buff *skb)
>>  	struct packet_type *ptype, *pt_prev;
>>  	rx_handler_func_t *rx_handler;
>>  	struct net_device *orig_dev;
>>-	struct net_device *master;
>>  	struct net_device *null_or_orig;
>>  	struct net_device *orig_or_bond;
>>  	int ret = NET_RX_DROP;
>>@@ -3173,15 +3172,19 @@ static int __netif_receive_skb(struct sk_buff *skb)
>>  	 */
>>  	null_or_orig = NULL;
>>  	orig_dev = skb->dev;
>>-	master = ACCESS_ONCE(orig_dev->master);
>>  	if (skb->deliver_no_wcard)
>>  		null_or_orig = orig_dev;
>>-	else if (master) {
>>-		if (__skb_bond_should_drop(skb, master)) {
>>-			skb->deliver_no_wcard = 1;
>>-			null_or_orig = orig_dev; /* deliver only exact match */
>>-		} else
>>-			skb->dev = master;
>>+	else if (netif_is_bond_slave(orig_dev)) {
>>+		struct net_device *bond_master = ACCESS_ONCE(orig_dev->master);
>>+
>>+		if (likely(bond_master)) {
>>+			if (__skb_bond_should_drop(skb, bond_master)) {
>>+				skb->deliver_no_wcard = 1;
>>+				/* deliver only exact match */
>>+				null_or_orig = orig_dev;
>>+			} else
>>+				skb->dev = bond_master;
>>+		}
>
>I think we need an "else" here. If orig_dev->master is NULL, while
>netif_is_bond_slave(orig_dev) is TRUE, we apparently face an
>unexpected situation and we should at least issue a warning.
>
>>  	}
>>
>>  	__this_cpu_inc(softnet_data.processed);
>>@@ -4346,15 +4349,14 @@ static int __init dev_proc_init(void)
>>
>>
>>  /**
>>- *	netdev_set_master	-	set up master/slave pair
>>+ *	netdev_set_master	-	set up master pointer
>>   *	@slave: slave device
>>   *	@master: new master device
>>   *
>>   *	Changes the master device of the slave. Pass %NULL to break the
>>   *	bonding. The caller must hold the RTNL semaphore. On a failure
>>   *	a negative errno code is returned. On success the reference counts
>>- *	are adjusted, %RTM_NEWLINK is sent to the routing socket and the
>>- *	function returns zero.
>>+ *	are adjusted and the function returns zero.
>>   */
>>  int netdev_set_master(struct net_device *slave, struct net_device *master)
>>  {
>>@@ -4374,6 +4376,29 @@ int netdev_set_master(struct net_device *slave, struct net_device *master)
>>  		synchronize_net();
>>  		dev_put(old);
>>  	}
>>+	return 0;
>>+}
>>+EXPORT_SYMBOL(netdev_set_master);
>>+
>>+/**
>>+ *	netdev_set_bond_master	-	set up bonding master/slave pair
>>+ *	@slave: slave device
>>+ *	@master: new master device
>>+ *
>>+ *	Changes the master device of the slave. Pass %NULL to break the
>>+ *	bonding. The caller must hold the RTNL semaphore. On a failure
>>+ *	a negative errno code is returned. On success %RTM_NEWLINK is sent
>>+ *	to the routing socket and the function returns zero.
>>+ */
>>+int netdev_set_bond_master(struct net_device *slave, struct net_device *master)
>>+{
>>+	int err;
>>+
>>+	ASSERT_RTNL();
>>+
>>+	err = netdev_set_master(slave, master);
>>+	if (err)
>>+		return err;
>>  	if (master)
>>  		slave->flags |= IFF_SLAVE;
>>  	else
>>@@ -4382,7 +4407,7 @@ int netdev_set_master(struct net_device *slave, struct net_device *master)
>>  	rtmsg_ifinfo(RTM_NEWLINK, slave, IFF_SLAVE);
>>  	return 0;
>>  }
>>-EXPORT_SYMBOL(netdev_set_master);
>>+EXPORT_SYMBOL(netdev_set_bond_master);
>>
>>  static void dev_change_rx_flags(struct net_device *dev, int flags)
>>  {
>

Re: [patch net-next-2.6] net: make dev->master general

[Nicolas de Pesloüan]

Le 14/02/2011 10:01, Jiri Pirko a écrit :
> Mon, Feb 14, 2011 at 09:48:44AM CET, nicolas.2p.debian@gmail.com wrote:
>> Le 12/02/2011 17:48, Jiri Pirko a écrit :
>>> dev->master is now tightly connected to bonding driver. This patch makes
>>> this pointer more general and ready to be used by others.
>>>
>>>   - netdev_set_master() - bond specifics moved to new function
>>>     netdev_set_bond_master()
>>>   - introduced netif_is_bond_slave() to check if device is a bonding slave
>>>
>>> Signed-off-by: Jiri Pirko<jpirko@redhat.com>
>>
>> Hi Jiri,
>>
>> Even if DaveM already applied your patch, I'm not comfortable with it.
>>
>> What is the rational behind it? Do you have anything in mind to use
>> the now "more general" master field of net_device?
>>
>> Of course, I won't advocate for every fields having only a single
>> possible usage, but, using master for several different things might
>> jeopardize our ability to share an interface between several logical
>> interface systems:
>>
>> Due to the current usage of the rx_handler field in net_device, the
>> code suggest that an interface cannot be part of a bridge and of a
>> macvlan at the same time. Even if bridge provide an hook for ebtables
>> to ignore an skb and allow other to get it, macvlan cannot be
>> registered on the same lower interface as a bridge, because
>> rx_handler can only hold a single value.
>>
>> By giving master a more general meaning, I think we might face a
>> similar problem. It might disallow an interface to be enslaved to
>> bonding and part of another logical interface at the same time, if
>> such logical interface also use the master field.
>
> That is true. I think that it makes no sense to have iface enslaved in
> bond and bridge at the same time. Do you have a scenario where it makes
> sense?

Agreed for bonding/bridge, because both tend to eat all skb, even if bridge has a way to give skb to 
others, as stated above. Bonding might/should do the same.

But, for macvlan or vlan for example, it is different. They will ignore skb not matching the correct 
dst_mac (macvlan) or vlan_id (vlan) and give a chance to other to use the skb.

Many setups involving several logical ifaces sharing a physical iface make sense:

- bridge+vlan (see "2.6.37 regression: adding main interface to a bridge breaks vlan interface RX")
- bride+macvlan (In particular because bridge might no know about the other dst_macs that should be 
considered local. I didn't check that particular point in detail.)
- bonding+vlan
- bonding+macvlan

So, would master be used only for ifaces that "eat all skb"?

	Nicolas.

Re: [patch net-next-2.6] net: make dev->master general

[Patrick McHardy]

Am 14.02.2011 15:11, schrieb Nicolas de Pesloüan:
> Le 14/02/2011 10:01, Jiri Pirko a écrit :
>> That is true. I think that it makes no sense to have iface enslaved in
>> bond and bridge at the same time. Do you have a scenario where it makes
>> sense?
> 
> Agreed for bonding/bridge, because both tend to eat all skb, even if
> bridge has a way to give skb to others, as stated above. Bonding
> might/should do the same.
> 
> But, for macvlan or vlan for example, it is different. They will ignore
> skb not matching the correct dst_mac (macvlan) or vlan_id (vlan) and
> give a chance to other to use the skb.

Both are not slaves of the device, so its not really related to this
patch. Slaves sit below the device, macvlan and vlan sit above the
device. Their link to the "real" device is through ->iflink.