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 <[email protected]>
Cc: Neil Horman <[email protected]>
---
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
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 <[email protected]>
> Cc: Neil Horman <[email protected]>
>
> ---
> 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
于 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.
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 <[email protected]>
Cc: Neil Horman <[email protected]>
---
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
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 <[email protected]>
Cc: Neil Horman <[email protected]>
---
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;
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!
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.
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
On Thu, 19 May 2011 18:24:17 +0800
Amerigo Wang <[email protected]> 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 <[email protected]>
> Cc: Neil Horman <[email protected]>
>
Is there a usage for this? What listens for this notification?
On Thu, 19 May 2011 08:12:13 -0700
Stephen Hemminger <[email protected]> wrote:
> On Thu, 19 May 2011 18:24:17 +0800
> Amerigo Wang <[email protected]> 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 <[email protected]>
> > Cc: Neil Horman <[email protected]>
> >
>
> 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.
于 2011年05月20日 00:04, Stephen Hemminger 写道:
> On Thu, 19 May 2011 08:12:13 -0700
> Stephen Hemminger<[email protected]> wrote:
>
>> On Thu, 19 May 2011 18:24:17 +0800
>> Amerigo Wang<[email protected]> 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<[email protected]>
>>> Cc: Neil Horman<[email protected]>
>>>
>>
>> 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.
于 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.