All of lore.kernel.org
 help / color / mirror / Atom feed
* [Patch net-next-2.6] netpoll: disable netpoll when enslave a device
@ 2011-05-18 10:00 Amerigo Wang
  2011-05-18 10:56 ` Neil Horman
  0 siblings, 1 reply; 16+ messages in thread
From: Amerigo Wang @ 2011-05-18 10:00 UTC (permalink / raw)
  To: linux-kernel
  Cc: WANG Cong, Neil Horman, Jay Vosburgh, Andy Gospodarek,
	David S. Miller, Neil Horman, Alexey Dobriyan, Ferenc Wagner,
	Andrew Morton, Paul E. McKenney, Josh Triplett, Ian Campbell,
	netdev

Currently we do nothing when we enslave a net device which is running netconsole.
Neil pointed out that we may get weird results in such case, so let's disable
netpoll on the device being enslaved. I think it is too harsh to prevent
the device being ensalved if it is running netconsole.

By the way, this patch also removes the NETDEV_GOING_DOWN from netconsole
netdev notifier, because netpoll will check if the device is running or not
and we don't handle NETDEV_PRE_UP neither.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Neil Horman <nhorman@redhat.com>

---
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 088fd84..b9c70c5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1640,6 +1640,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		}
 	}
 
+	netdev_bonding_change(slave_dev, NETDEV_ENSLAVE);
+
 	/* If this is the first slave, then we need to set the master's hardware
 	 * address to be the same as the slave's. */
 	if (is_zero_ether_addr(bond->dev->dev_addr))
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index a83e101..0c3e8de 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -621,7 +621,8 @@ static int netconsole_netdev_event(struct notifier_block *this,
 	bool stopped = false;
 
 	if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
-	      event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN))
+	      event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN ||
+	      event == NETDEV_ENSLAVE))
 		goto done;
 
 	spin_lock_irqsave(&target_list_lock, flags);
@@ -650,8 +651,8 @@ restart:
 					goto restart;
 				}
 				/* Fall through */
-			case NETDEV_GOING_DOWN:
 			case NETDEV_BONDING_DESLAVE:
+			case NETDEV_ENSLAVE:
 				nt->enabled = 0;
 				stopped = true;
 				break;
@@ -660,10 +661,21 @@ restart:
 		netconsole_target_put(nt);
 	}
 	spin_unlock_irqrestore(&target_list_lock, flags);
-	if (stopped && (event == NETDEV_UNREGISTER || event == NETDEV_BONDING_DESLAVE))
+	if (stopped) {
 		printk(KERN_INFO "netconsole: network logging stopped on "
-			"interface %s as it %s\n",  dev->name,
-			event == NETDEV_UNREGISTER ? "unregistered" : "released slaves");
+		       "interface %s as it ", dev->name);
+		switch (event) {
+		case NETDEV_UNREGISTER:
+			printk(KERN_CONT "unregistered\n");
+			break;
+		case NETDEV_BONDING_DESLAVE:
+			printk(KERN_CONT "released slaves\n");
+			break;
+		case NETDEV_ENSLAVE:
+			printk(KERN_CONT "is enslaved\n");
+			break;
+		}
+	}
 
 done:
 	return NOTIFY_DONE;
diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index 621dfa1..3d82867 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
 #define NETDEV_UNREGISTER_BATCH 0x0011
 #define NETDEV_BONDING_DESLAVE  0x0012
 #define NETDEV_NOTIFY_PEERS	0x0013
+#define NETDEV_ENSLAVE		0x0014
 
 #define SYS_DOWN	0x0001	/* Notify of system down */
 #define SYS_RESTART	SYS_DOWN

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Patch net-next-2.6] netpoll: disable netpoll when enslave a device
  2011-05-18 10:00 [Patch net-next-2.6] netpoll: disable netpoll when enslave a device Amerigo Wang
@ 2011-05-18 10:56 ` Neil Horman
  2011-05-19  5:13   ` Cong Wang
  2011-05-19  8:39   ` [V2 Patch " Amerigo Wang
  0 siblings, 2 replies; 16+ messages in thread
From: Neil Horman @ 2011-05-18 10:56 UTC (permalink / raw)
  To: Amerigo Wang
  Cc: linux-kernel, Neil Horman, Jay Vosburgh, Andy Gospodarek,
	David S. Miller, Alexey Dobriyan, Ferenc Wagner, Andrew Morton,
	Paul E. McKenney, Josh Triplett, Ian Campbell, netdev

On Wed, May 18, 2011 at 06:00:35PM +0800, Amerigo Wang wrote:
> Currently we do nothing when we enslave a net device which is running netconsole.
> Neil pointed out that we may get weird results in such case, so let's disable
> netpoll on the device being enslaved. I think it is too harsh to prevent
> the device being ensalved if it is running netconsole.
> 
> By the way, this patch also removes the NETDEV_GOING_DOWN from netconsole
> netdev notifier, because netpoll will check if the device is running or not
> and we don't handle NETDEV_PRE_UP neither.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Neil Horman <nhorman@redhat.com>
> 
> ---
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 088fd84..b9c70c5 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1640,6 +1640,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  		}
>  	}
>  
> +	netdev_bonding_change(slave_dev, NETDEV_ENSLAVE);
> +
>  	/* If this is the first slave, then we need to set the master's hardware
>  	 * address to be the same as the slave's. */
>  	if (is_zero_ether_addr(bond->dev->dev_addr))
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index a83e101..0c3e8de 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -621,7 +621,8 @@ static int netconsole_netdev_event(struct notifier_block *this,
>  	bool stopped = false;
>  
>  	if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
> -	      event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN))
> +	      event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN ||
> +	      event == NETDEV_ENSLAVE))
>  		goto done;
>  
>  	spin_lock_irqsave(&target_list_lock, flags);
> @@ -650,8 +651,8 @@ restart:
>  					goto restart;
>  				}
>  				/* Fall through */
> -			case NETDEV_GOING_DOWN:
>  			case NETDEV_BONDING_DESLAVE:
> +			case NETDEV_ENSLAVE:
>  				nt->enabled = 0;
>  				stopped = true;
>  				break;
This wasn't introduced by this patch, but looking at it made me realize that
nt->enabled, if it passes through this code path, doesn't properly track weather
or not netpoll_setup has been called on this interface.  If you look at
drop_netconsole_target, you'll see we only call netpoll_cleanup_target if
nt->enabled is set.  We should probably change the nt->enabled check there, and
in store_enabled to be if (nt->np.dev), like we do in the NETDEV_UNREGISTER case
in netconsole_netdev_event.

> @@ -660,10 +661,21 @@ restart:
>  		netconsole_target_put(nt);
>  	}
>  	spin_unlock_irqrestore(&target_list_lock, flags);
> -	if (stopped && (event == NETDEV_UNREGISTER || event == NETDEV_BONDING_DESLAVE))
> +	if (stopped) {
>  		printk(KERN_INFO "netconsole: network logging stopped on "
> -			"interface %s as it %s\n",  dev->name,
> -			event == NETDEV_UNREGISTER ? "unregistered" : "released slaves");
> +		       "interface %s as it ", dev->name);
> +		switch (event) {
> +		case NETDEV_UNREGISTER:
> +			printk(KERN_CONT "unregistered\n");
> +			break;
> +		case NETDEV_BONDING_DESLAVE:
> +			printk(KERN_CONT "released slaves\n");
> +			break;
> +		case NETDEV_ENSLAVE:
> +			printk(KERN_CONT "is enslaved\n");
> +			break;
> +		}
> +	}
>  
>  done:
>  	return NOTIFY_DONE;
> diff --git a/include/linux/notifier.h b/include/linux/notifier.h
> index 621dfa1..3d82867 100644
> --- a/include/linux/notifier.h
> +++ b/include/linux/notifier.h
> @@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
>  #define NETDEV_UNREGISTER_BATCH 0x0011
>  #define NETDEV_BONDING_DESLAVE  0x0012
>  #define NETDEV_NOTIFY_PEERS	0x0013
> +#define NETDEV_ENSLAVE		0x0014
>  
Nit:
Shouldn't this be NETDEV_BONDING_ENSLAVE, to keep it in line with
NETDEV_BONDING_DESLAVE above?

>  #define SYS_DOWN	0x0001	/* Notify of system down */
>  #define SYS_RESTART	SYS_DOWN
> 


Other than those two points, this looks good to me
Thanks!
Neil


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch net-next-2.6] netpoll: disable netpoll when enslave a device
  2011-05-18 10:56 ` Neil Horman
@ 2011-05-19  5:13   ` Cong Wang
  2011-05-19 11:03     ` Neil Horman
  2011-05-19  8:39   ` [V2 Patch " Amerigo Wang
  1 sibling, 1 reply; 16+ messages in thread
From: Cong Wang @ 2011-05-19  5:13 UTC (permalink / raw)
  To: Neil Horman
  Cc: linux-kernel, Neil Horman, Jay Vosburgh, Andy Gospodarek,
	David S. Miller, Alexey Dobriyan, Ferenc Wagner, Andrew Morton,
	Paul E. McKenney, Josh Triplett, Ian Campbell, netdev

于 2011年05月18日 18:56, Neil Horman 写道:
> On Wed, May 18, 2011 at 06:00:35PM +0800, Amerigo Wang wrote:
...
>> -			case NETDEV_GOING_DOWN:
>>   			case NETDEV_BONDING_DESLAVE:
>> +			case NETDEV_ENSLAVE:
>>   				nt->enabled = 0;
>>   				stopped = true;
>>   				break;
> This wasn't introduced by this patch, but looking at it made me realize that
> nt->enabled, if it passes through this code path, doesn't properly track weather
> or not netpoll_setup has been called on this interface.  If you look at
> drop_netconsole_target, you'll see we only call netpoll_cleanup_target if
> nt->enabled is set.  We should probably change the nt->enabled check there, and
> in store_enabled to be if (nt->np.dev), like we do in the NETDEV_UNREGISTER case
> in netconsole_netdev_event.

Yeah, also note that we can change ->enabled via configfs too.
I guess we probably need to fix this in another patch...


>> +#define NETDEV_ENSLAVE		0x0014
>>
> Nit:
> Shouldn't this be NETDEV_BONDING_ENSLAVE, to keep it in line with
> NETDEV_BONDING_DESLAVE above?

Actually that is my first thought, but I plan to use this in bridge
case too, because using netconsole on a device underlying a bridge
makes little sense too. Thus, I prefer NETDEV_ENSLAVE to
NETDEV_BONDING_ENSLAVE.

>
>>   #define SYS_DOWN	0x0001	/* Notify of system down */
>>   #define SYS_RESTART	SYS_DOWN
>>
>
>
> Other than those two points, this looks good to me

Thanks for review.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [V2 Patch net-next-2.6] netpoll: disable netpoll when enslave a device
  2011-05-18 10:56 ` Neil Horman
  2011-05-19  5:13   ` Cong Wang
@ 2011-05-19  8:39   ` Amerigo Wang
  2011-05-19 10:24       ` [Bridge] " Amerigo Wang
  2011-05-19 11:31     ` [V2 Patch net-next-2.6] netpoll: disable netpoll when enslave a device Andy Gospodarek
  1 sibling, 2 replies; 16+ messages in thread
From: Amerigo Wang @ 2011-05-19  8:39 UTC (permalink / raw)
  To: linux-kernel
  Cc: akpm, WANG Cong, Neil Horman, Jay Vosburgh, David S. Miller,
	Ian Campbell, Paul E. McKenney, Josh Triplett, netdev

Currently we do nothing when we enslave a net device which is running netconsole.
Neil pointed out that we may get weird results in such case, so let's disable
netpoll on the device being enslaved. I think it is too harsh to prevent
the device being ensalved if it is running netconsole.

By the way, this patch also removes the NETDEV_GOING_DOWN from netconsole
netdev notifier, because netpoll will check if the device is running or not
and we don't handle NETDEV_PRE_UP neither.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Neil Horman <nhorman@redhat.com>

---
 drivers/net/bonding/bond_main.c |    2 ++
 drivers/net/netconsole.c        |   26 +++++++++++++++++---------
 include/linux/notifier.h        |    1 +
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 088fd84..b9c70c5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1640,6 +1640,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		}
 	}
 
+	netdev_bonding_change(slave_dev, NETDEV_ENSLAVE);
+
 	/* If this is the first slave, then we need to set the master's hardware
 	 * address to be the same as the slave's. */
 	if (is_zero_ether_addr(bond->dev->dev_addr))
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index a83e101..c2a8230 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -621,11 +621,10 @@ static int netconsole_netdev_event(struct notifier_block *this,
 	bool stopped = false;
 
 	if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
-	      event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN))
+	      event == NETDEV_BONDING_DESLAVE || event == NETDEV_ENSLAVE))
 		goto done;
 
 	spin_lock_irqsave(&target_list_lock, flags);
-restart:
 	list_for_each_entry(nt, &target_list, list) {
 		netconsole_target_get(nt);
 		if (nt->np.dev == dev) {
@@ -633,6 +632,8 @@ restart:
 			case NETDEV_CHANGENAME:
 				strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ);
 				break;
+			case NETDEV_BONDING_DESLAVE:
+			case NETDEV_ENSLAVE:
 			case NETDEV_UNREGISTER:
 				/*
 				 * rtnl_lock already held
@@ -647,11 +648,7 @@ restart:
 					dev_put(nt->np.dev);
 					nt->np.dev = NULL;
 					netconsole_target_put(nt);
-					goto restart;
 				}
-				/* Fall through */
-			case NETDEV_GOING_DOWN:
-			case NETDEV_BONDING_DESLAVE:
 				nt->enabled = 0;
 				stopped = true;
 				break;
@@ -660,10 +657,21 @@ restart:
 		netconsole_target_put(nt);
 	}
 	spin_unlock_irqrestore(&target_list_lock, flags);
-	if (stopped && (event == NETDEV_UNREGISTER || event == NETDEV_BONDING_DESLAVE))
+	if (stopped) {
 		printk(KERN_INFO "netconsole: network logging stopped on "
-			"interface %s as it %s\n",  dev->name,
-			event == NETDEV_UNREGISTER ? "unregistered" : "released slaves");
+		       "interface %s as it ", dev->name);
+		switch (event) {
+		case NETDEV_UNREGISTER:
+			printk(KERN_CONT "unregistered\n");
+			break;
+		case NETDEV_BONDING_DESLAVE:
+			printk(KERN_CONT "released slaves\n");
+			break;
+		case NETDEV_ENSLAVE:
+			printk(KERN_CONT "is enslaved\n");
+			break;
+		}
+	}
 
 done:
 	return NOTIFY_DONE;
diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index 621dfa1..3d82867 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
 #define NETDEV_UNREGISTER_BATCH 0x0011
 #define NETDEV_BONDING_DESLAVE  0x0012
 #define NETDEV_NOTIFY_PEERS	0x0013
+#define NETDEV_ENSLAVE		0x0014
 
 #define SYS_DOWN	0x0001	/* Notify of system down */
 #define SYS_RESTART	SYS_DOWN

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave
  2011-05-19  8:39   ` [V2 Patch " Amerigo Wang
@ 2011-05-19 10:24       ` Amerigo Wang
  2011-05-19 11:31     ` [V2 Patch net-next-2.6] netpoll: disable netpoll when enslave a device Andy Gospodarek
  1 sibling, 0 replies; 16+ messages in thread
From: Amerigo Wang @ 2011-05-19 10:24 UTC (permalink / raw)
  To: linux-kernel
  Cc: akpm, WANG Cong, Neil Horman, Jay Vosburgh, Stephen Hemminger,
	David S. Miller, netdev, bridge

In the previous patch I added NETDEV_ENSLAVE, now
we can notify netconsole when adding a device to a bridge too.

By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
bond_main.c, since this is not bonding specific.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Neil Horman <nhorman@redhat.com>

---
 drivers/net/bonding/bond_main.c |    2 +-
 net/bridge/br_if.c              |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b9c70c5..765fdcf 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1640,7 +1640,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		}
 	}
 
-	netdev_bonding_change(slave_dev, NETDEV_ENSLAVE);
+	call_netdevice_notifiers(NETDEV_ENSLAVE, slave_dev);
 
 	/* If this is the first slave, then we need to set the master's hardware
 	 * address to be the same as the slave's. */
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 5dbdfdf..b44fae5 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -338,6 +338,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 	if (IS_ERR(p))
 		return PTR_ERR(p);
 
+	call_netdevice_notifiers(NETDEV_ENSLAVE, dev);
+
 	err = dev_set_promiscuity(dev, 1);
 	if (err)
 		goto put_back;

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Bridge] [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave
@ 2011-05-19 10:24       ` Amerigo Wang
  0 siblings, 0 replies; 16+ messages in thread
From: Amerigo Wang @ 2011-05-19 10:24 UTC (permalink / raw)
  To: linux-kernel
  Cc: Neil Horman, bridge, WANG Cong, netdev, Jay Vosburgh, akpm,
	David S. Miller

In the previous patch I added NETDEV_ENSLAVE, now
we can notify netconsole when adding a device to a bridge too.

By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
bond_main.c, since this is not bonding specific.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Neil Horman <nhorman@redhat.com>

---
 drivers/net/bonding/bond_main.c |    2 +-
 net/bridge/br_if.c              |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b9c70c5..765fdcf 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1640,7 +1640,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		}
 	}
 
-	netdev_bonding_change(slave_dev, NETDEV_ENSLAVE);
+	call_netdevice_notifiers(NETDEV_ENSLAVE, slave_dev);
 
 	/* If this is the first slave, then we need to set the master's hardware
 	 * address to be the same as the slave's. */
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 5dbdfdf..b44fae5 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -338,6 +338,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 	if (IS_ERR(p))
 		return PTR_ERR(p);
 
+	call_netdevice_notifiers(NETDEV_ENSLAVE, dev);
+
 	err = dev_set_promiscuity(dev, 1);
 	if (err)
 		goto put_back;

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Patch net-next-2.6] netpoll: disable netpoll when enslave a device
  2011-05-19  5:13   ` Cong Wang
@ 2011-05-19 11:03     ` Neil Horman
  0 siblings, 0 replies; 16+ messages in thread
From: Neil Horman @ 2011-05-19 11:03 UTC (permalink / raw)
  To: Cong Wang
  Cc: Neil Horman, linux-kernel, Jay Vosburgh, Andy Gospodarek,
	David S. Miller, Alexey Dobriyan, Ferenc Wagner, Andrew Morton,
	Paul E. McKenney, Josh Triplett, Ian Campbell, netdev

On Thu, May 19, 2011 at 01:13:29PM +0800, Cong Wang wrote:
> 于 2011年05月18日 18:56, Neil Horman 写道:
> >On Wed, May 18, 2011 at 06:00:35PM +0800, Amerigo Wang wrote:
> ...
> >>-			case NETDEV_GOING_DOWN:
> >>  			case NETDEV_BONDING_DESLAVE:
> >>+			case NETDEV_ENSLAVE:
> >>  				nt->enabled = 0;
> >>  				stopped = true;
> >>  				break;
> >This wasn't introduced by this patch, but looking at it made me realize that
> >nt->enabled, if it passes through this code path, doesn't properly track weather
> >or not netpoll_setup has been called on this interface.  If you look at
> >drop_netconsole_target, you'll see we only call netpoll_cleanup_target if
> >nt->enabled is set.  We should probably change the nt->enabled check there, and
> >in store_enabled to be if (nt->np.dev), like we do in the NETDEV_UNREGISTER case
> >in netconsole_netdev_event.
> 
> Yeah, also note that we can change ->enabled via configfs too.
> I guess we probably need to fix this in another patch...
> 
Yeah, or you can roll it into this one, I think this is the only location that
needs fixing.

> 
> >>+#define NETDEV_ENSLAVE		0x0014
> >>
> >Nit:
> >Shouldn't this be NETDEV_BONDING_ENSLAVE, to keep it in line with
> >NETDEV_BONDING_DESLAVE above?
> 
> Actually that is my first thought, but I plan to use this in bridge
> case too, because using netconsole on a device underlying a bridge
> makes little sense too. Thus, I prefer NETDEV_ENSLAVE to
> NETDEV_BONDING_ENSLAVE.
> 
That seems reasonable, but if its going to be more generic, could you change
NETDEV_BONDING_DESLAVE to NETDEV_DESLAVE?

> >
> >>  #define SYS_DOWN	0x0001	/* Notify of system down */
> >>  #define SYS_RESTART	SYS_DOWN
> >>
> >
> >
> >Other than those two points, this looks good to me
> 
> Thanks for review.
Thank you!


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [V2 Patch net-next-2.6] netpoll: disable netpoll when enslave a device
  2011-05-19  8:39   ` [V2 Patch " Amerigo Wang
  2011-05-19 10:24       ` [Bridge] " Amerigo Wang
@ 2011-05-19 11:31     ` Andy Gospodarek
  2011-05-19 13:25       ` Neil Horman
  1 sibling, 1 reply; 16+ messages in thread
From: Andy Gospodarek @ 2011-05-19 11:31 UTC (permalink / raw)
  To: Amerigo Wang
  Cc: linux-kernel, akpm, Neil Horman, Jay Vosburgh, David S. Miller,
	Ian Campbell, Paul E. McKenney, Josh Triplett, netdev

On Thu, May 19, 2011 at 04:39:53PM +0800, Amerigo Wang wrote:
[...]
> diff --git a/include/linux/notifier.h b/include/linux/notifier.h
> index 621dfa1..3d82867 100644
> --- a/include/linux/notifier.h
> +++ b/include/linux/notifier.h
> @@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
>  #define NETDEV_UNREGISTER_BATCH 0x0011
>  #define NETDEV_BONDING_DESLAVE  0x0012
>  #define NETDEV_NOTIFY_PEERS	0x0013
> +#define NETDEV_ENSLAVE		0x0014
>  
>  #define SYS_DOWN	0x0001	/* Notify of system down */
>  #define SYS_RESTART	SYS_DOWN

Neil just noted the same concern I had -- the asymmetry between
NETDEV_ENSLAVE and NETDEV_BONDING_DESLAVE bothers me a bit.  I also
don't really like the followup patch that uses 'ENSLAVE' in the bridging
code when we typically use that language for bonding only.

What about changing NETDEV_BONDING_DESLAVE to NETDEV_RELEASE and create
NETDEV_JOIN instead of NETDEV_ENSLAVE?  I would prefer that or something
else that might use more generic language that could be applied to all
for stacked interfaces.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [V2 Patch net-next-2.6] netpoll: disable netpoll when enslave a device
  2011-05-19 11:31     ` [V2 Patch net-next-2.6] netpoll: disable netpoll when enslave a device Andy Gospodarek
@ 2011-05-19 13:25       ` Neil Horman
  2011-05-20  3:10         ` Cong Wang
  0 siblings, 1 reply; 16+ messages in thread
From: Neil Horman @ 2011-05-19 13:25 UTC (permalink / raw)
  To: Andy Gospodarek
  Cc: Amerigo Wang, linux-kernel, akpm, Jay Vosburgh, David S. Miller,
	Ian Campbell, Paul E. McKenney, Josh Triplett, netdev

On Thu, May 19, 2011 at 07:31:27AM -0400, Andy Gospodarek wrote:
> On Thu, May 19, 2011 at 04:39:53PM +0800, Amerigo Wang wrote:
> [...]
> > diff --git a/include/linux/notifier.h b/include/linux/notifier.h
> > index 621dfa1..3d82867 100644
> > --- a/include/linux/notifier.h
> > +++ b/include/linux/notifier.h
> > @@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
> >  #define NETDEV_UNREGISTER_BATCH 0x0011
> >  #define NETDEV_BONDING_DESLAVE  0x0012
> >  #define NETDEV_NOTIFY_PEERS	0x0013
> > +#define NETDEV_ENSLAVE		0x0014
> >  
> >  #define SYS_DOWN	0x0001	/* Notify of system down */
> >  #define SYS_RESTART	SYS_DOWN
> 
> Neil just noted the same concern I had -- the asymmetry between
> NETDEV_ENSLAVE and NETDEV_BONDING_DESLAVE bothers me a bit.  I also
> don't really like the followup patch that uses 'ENSLAVE' in the bridging
> code when we typically use that language for bonding only.
> 
> What about changing NETDEV_BONDING_DESLAVE to NETDEV_RELEASE and create
> NETDEV_JOIN instead of NETDEV_ENSLAVE?  I would prefer that or something
> else that might use more generic language that could be applied to all
> for stacked interfaces.
JOIN and RELEASE (or perhaps LEAVE) sounds good to me.
Neil


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave
  2011-05-19 10:24       ` [Bridge] " Amerigo Wang
@ 2011-05-19 15:12         ` Stephen Hemminger
  -1 siblings, 0 replies; 16+ messages in thread
From: Stephen Hemminger @ 2011-05-19 15:12 UTC (permalink / raw)
  To: Amerigo Wang
  Cc: linux-kernel, akpm, Neil Horman, Jay Vosburgh, David S. Miller,
	netdev, bridge

On Thu, 19 May 2011 18:24:17 +0800
Amerigo Wang <amwang@redhat.com> wrote:

> In the previous patch I added NETDEV_ENSLAVE, now
> we can notify netconsole when adding a device to a bridge too.
> 
> By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
> bond_main.c, since this is not bonding specific.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Neil Horman <nhorman@redhat.com>
> 

Is there a usage for this? What listens for this notification?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Bridge] [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave
@ 2011-05-19 15:12         ` Stephen Hemminger
  0 siblings, 0 replies; 16+ messages in thread
From: Stephen Hemminger @ 2011-05-19 15:12 UTC (permalink / raw)
  To: Amerigo Wang
  Cc: Neil Horman, bridge, netdev, Jay Vosburgh, linux-kernel, akpm,
	David S. Miller

On Thu, 19 May 2011 18:24:17 +0800
Amerigo Wang <amwang@redhat.com> wrote:

> In the previous patch I added NETDEV_ENSLAVE, now
> we can notify netconsole when adding a device to a bridge too.
> 
> By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
> bond_main.c, since this is not bonding specific.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Neil Horman <nhorman@redhat.com>
> 

Is there a usage for this? What listens for this notification?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Bridge] [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave
  2011-05-19 15:12         ` [Bridge] " Stephen Hemminger
@ 2011-05-19 16:04           ` Stephen Hemminger
  -1 siblings, 0 replies; 16+ messages in thread
From: Stephen Hemminger @ 2011-05-19 16:04 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Amerigo Wang, Neil Horman, bridge, netdev, Jay Vosburgh,
	linux-kernel, akpm, David S. Miller

On Thu, 19 May 2011 08:12:13 -0700
Stephen Hemminger <shemminger@linux-foundation.org> wrote:

> On Thu, 19 May 2011 18:24:17 +0800
> Amerigo Wang <amwang@redhat.com> wrote:
> 
> > In the previous patch I added NETDEV_ENSLAVE, now
> > we can notify netconsole when adding a device to a bridge too.
> > 
> > By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
> > bond_main.c, since this is not bonding specific.
> > 
> > Signed-off-by: WANG Cong <amwang@redhat.com>
> > Cc: Neil Horman <nhorman@redhat.com>
> > 
> 
> Is there a usage for this? What listens for this notification?

Never mind it was in the first patch which you did not send.
You should always put a number on group of patches and send
to all parties.

Also, sending networking patches to LKML is a waste of bandwidth
please don't bother.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Bridge] [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave
@ 2011-05-19 16:04           ` Stephen Hemminger
  0 siblings, 0 replies; 16+ messages in thread
From: Stephen Hemminger @ 2011-05-19 16:04 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Neil Horman, Jay Vosburgh, Amerigo Wang, netdev, bridge,
	linux-kernel, akpm, David S. Miller

On Thu, 19 May 2011 08:12:13 -0700
Stephen Hemminger <shemminger@linux-foundation.org> wrote:

> On Thu, 19 May 2011 18:24:17 +0800
> Amerigo Wang <amwang@redhat.com> wrote:
> 
> > In the previous patch I added NETDEV_ENSLAVE, now
> > we can notify netconsole when adding a device to a bridge too.
> > 
> > By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
> > bond_main.c, since this is not bonding specific.
> > 
> > Signed-off-by: WANG Cong <amwang@redhat.com>
> > Cc: Neil Horman <nhorman@redhat.com>
> > 
> 
> Is there a usage for this? What listens for this notification?

Never mind it was in the first patch which you did not send.
You should always put a number on group of patches and send
to all parties.

Also, sending networking patches to LKML is a waste of bandwidth
please don't bother.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Bridge] [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave
  2011-05-19 16:04           ` Stephen Hemminger
@ 2011-05-20  3:06             ` Cong Wang
  -1 siblings, 0 replies; 16+ messages in thread
From: Cong Wang @ 2011-05-20  3:06 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Neil Horman, bridge, netdev, Jay Vosburgh, linux-kernel, akpm,
	David S. Miller

于 2011年05月20日 00:04, Stephen Hemminger 写道:
> On Thu, 19 May 2011 08:12:13 -0700
> Stephen Hemminger<shemminger@linux-foundation.org>  wrote:
>
>> On Thu, 19 May 2011 18:24:17 +0800
>> Amerigo Wang<amwang@redhat.com>  wrote:
>>
>>> In the previous patch I added NETDEV_ENSLAVE, now
>>> we can notify netconsole when adding a device to a bridge too.
>>>
>>> By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
>>> bond_main.c, since this is not bonding specific.
>>>
>>> Signed-off-by: WANG Cong<amwang@redhat.com>
>>> Cc: Neil Horman<nhorman@redhat.com>
>>>
>>
>> Is there a usage for this? What listens for this notification?
>
> Never mind it was in the first patch which you did not send.
> You should always put a number on group of patches and send
> to all parties.

Ah, sorry, my script simply run get_maintainers.pl to
get the Cc list, so bridge list was not in included in
the first patch.

>
> Also, sending networking patches to LKML is a waste of bandwidth
> please don't bother.

Ok, will fix my script.

Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Bridge] [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave
@ 2011-05-20  3:06             ` Cong Wang
  0 siblings, 0 replies; 16+ messages in thread
From: Cong Wang @ 2011-05-20  3:06 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Neil Horman, Jay Vosburgh, netdev, bridge, linux-kernel, akpm,
	David S. Miller

于 2011年05月20日 00:04, Stephen Hemminger 写道:
> On Thu, 19 May 2011 08:12:13 -0700
> Stephen Hemminger<shemminger@linux-foundation.org>  wrote:
>
>> On Thu, 19 May 2011 18:24:17 +0800
>> Amerigo Wang<amwang@redhat.com>  wrote:
>>
>>> In the previous patch I added NETDEV_ENSLAVE, now
>>> we can notify netconsole when adding a device to a bridge too.
>>>
>>> By the way, s/netdev_bonding_change/call_netdevice_notifiers/ in
>>> bond_main.c, since this is not bonding specific.
>>>
>>> Signed-off-by: WANG Cong<amwang@redhat.com>
>>> Cc: Neil Horman<nhorman@redhat.com>
>>>
>>
>> Is there a usage for this? What listens for this notification?
>
> Never mind it was in the first patch which you did not send.
> You should always put a number on group of patches and send
> to all parties.

Ah, sorry, my script simply run get_maintainers.pl to
get the Cc list, so bridge list was not in included in
the first patch.

>
> Also, sending networking patches to LKML is a waste of bandwidth
> please don't bother.

Ok, will fix my script.

Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [V2 Patch net-next-2.6] netpoll: disable netpoll when enslave a device
  2011-05-19 13:25       ` Neil Horman
@ 2011-05-20  3:10         ` Cong Wang
  0 siblings, 0 replies; 16+ messages in thread
From: Cong Wang @ 2011-05-20  3:10 UTC (permalink / raw)
  To: Neil Horman
  Cc: Andy Gospodarek, linux-kernel, akpm, Jay Vosburgh,
	David S. Miller, Ian Campbell, Paul E. McKenney, Josh Triplett,
	netdev

于 2011年05月19日 21:25, Neil Horman 写道:
> On Thu, May 19, 2011 at 07:31:27AM -0400, Andy Gospodarek wrote:
>> On Thu, May 19, 2011 at 04:39:53PM +0800, Amerigo Wang wrote:
>> [...]
>>> diff --git a/include/linux/notifier.h b/include/linux/notifier.h
>>> index 621dfa1..3d82867 100644
>>> --- a/include/linux/notifier.h
>>> +++ b/include/linux/notifier.h
>>> @@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
>>>   #define NETDEV_UNREGISTER_BATCH 0x0011
>>>   #define NETDEV_BONDING_DESLAVE  0x0012
>>>   #define NETDEV_NOTIFY_PEERS	0x0013
>>> +#define NETDEV_ENSLAVE		0x0014
>>>
>>>   #define SYS_DOWN	0x0001	/* Notify of system down */
>>>   #define SYS_RESTART	SYS_DOWN
>>
>> Neil just noted the same concern I had -- the asymmetry between
>> NETDEV_ENSLAVE and NETDEV_BONDING_DESLAVE bothers me a bit.  I also
>> don't really like the followup patch that uses 'ENSLAVE' in the bridging
>> code when we typically use that language for bonding only.
>>
>> What about changing NETDEV_BONDING_DESLAVE to NETDEV_RELEASE and create
>> NETDEV_JOIN instead of NETDEV_ENSLAVE?  I would prefer that or something
>> else that might use more generic language that could be applied to all
>> for stacked interfaces.
> JOIN and RELEASE (or perhaps LEAVE) sounds good to me.

Thanks, Andy and Neil! I will rename them to JOIN and RELEASE.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2011-05-20  3:10 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-18 10:00 [Patch net-next-2.6] netpoll: disable netpoll when enslave a device Amerigo Wang
2011-05-18 10:56 ` Neil Horman
2011-05-19  5:13   ` Cong Wang
2011-05-19 11:03     ` Neil Horman
2011-05-19  8:39   ` [V2 Patch " Amerigo Wang
2011-05-19 10:24     ` [Patch] bridge: call NETDEV_ENSLAVE notifiers when adding a slave Amerigo Wang
2011-05-19 10:24       ` [Bridge] " Amerigo Wang
2011-05-19 15:12       ` Stephen Hemminger
2011-05-19 15:12         ` [Bridge] " Stephen Hemminger
2011-05-19 16:04         ` Stephen Hemminger
2011-05-19 16:04           ` Stephen Hemminger
2011-05-20  3:06           ` Cong Wang
2011-05-20  3:06             ` Cong Wang
2011-05-19 11:31     ` [V2 Patch net-next-2.6] netpoll: disable netpoll when enslave a device Andy Gospodarek
2011-05-19 13:25       ` Neil Horman
2011-05-20  3:10         ` Cong Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.