[PATCH 0/7] netpoll and netconsole fixes for 3.6

[PATCH 0/7] netpoll and netconsole fixes for 3.6

[Cong Wang]

This patch fixes serval problems in netconsole and netpoll.

I ran this patch in my KVM guest with some netpoll test cases,
even covered with some corner cases, everything worked as expected.

BTW, my kernel config enables the following validatation options:

CONFIG_DEBUG_ATOMIC_SLEEP=y
CONFIG_PROVE_LOCKING=y
CONFIG_PROVE_RCU=y
CONFIG_LOCKDEP=y
CONFIG_RCU_CPU_STALL_TIMEOUT=60
CONFIG_LOCKUP_DETECTOR=y
CONFIG_HARDLOCKUP_DETECTOR=y

Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>

---

Cong Wang (7):
  netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup()
  netpoll: make __netpoll_cleanup non-block
  netconsole: do not release spin_lock when calling __netpoll_cleanup
  bridge: call NETDEV_RELEASE notifier in br_del_if()
  netpoll: take rcu_read_lock_bh() in netpoll_rx()
  netpoll: use netpoll_rx_on() in netpoll_rx()
  netpoll: take rcu_read_lock_bh() in netpoll_send_skb_on_dev()

 drivers/net/bonding/bond_main.c |    6 +--
 drivers/net/netconsole.c        |    5 --
 include/linux/netpoll.h         |   25 +++++++-----
 net/8021q/vlan_dev.c            |    6 +--
 net/bridge/br_device.c          |    6 +--
 net/bridge/br_if.c              |    1 +
 net/core/netpoll.c              |   78 +++++++++++++++++++++++++-------------
 7 files changed, 70 insertions(+), 57 deletions(-)

-- 
1.7.7.6

[PATCH 1/7] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup()

[Cong Wang]

slave_enable_netpoll() and __netpoll_setup() may be called
with read_lock() held, so should use GFP_ATOMIC to allocate
memory.

Cc: "David S. Miller" <davem@davemloft.net>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 drivers/net/bonding/bond_main.c |    2 +-
 net/core/netpoll.c              |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 6fae5f3..ab773d4 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1235,7 +1235,7 @@ static inline int slave_enable_netpoll(struct slave *slave)
 	struct netpoll *np;
 	int err = 0;
 
-	np = kzalloc(sizeof(*np), GFP_KERNEL);
+	np = kzalloc(sizeof(*np), GFP_ATOMIC);
 	err = -ENOMEM;
 	if (!np)
 		goto out;
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index b4c90e4..c78a966 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -734,7 +734,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	}
 
 	if (!ndev->npinfo) {
-		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
+		npinfo = kmalloc(sizeof(*npinfo), GFP_ATOMIC);
 		if (!npinfo) {
 			err = -ENOMEM;
 			goto out;
-- 
1.7.7.6

Re: [PATCH 1/7] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup()

[Eric Dumazet]

On Fri, 2012-07-27 at 23:37 +0800, Cong Wang wrote:
> slave_enable_netpoll() and __netpoll_setup() may be called
> with read_lock() held, so should use GFP_ATOMIC to allocate
> memory.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> ---
>  drivers/net/bonding/bond_main.c |    2 +-
>  net/core/netpoll.c              |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 6fae5f3..ab773d4 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1235,7 +1235,7 @@ static inline int slave_enable_netpoll(struct slave *slave)
>  	struct netpoll *np;
>  	int err = 0;
>  
> -	np = kzalloc(sizeof(*np), GFP_KERNEL);
> +	np = kzalloc(sizeof(*np), GFP_ATOMIC);
>  	err = -ENOMEM;
>  	if (!np)
>  		goto out;
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index b4c90e4..c78a966 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -734,7 +734,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
>  	}
>  
>  	if (!ndev->npinfo) {
> -		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
> +		npinfo = kmalloc(sizeof(*npinfo), GFP_ATOMIC);
>  		if (!npinfo) {
>  			err = -ENOMEM;
>  			goto out;

Yes this works, but maybe you instead could pass/add a gfp_t flags
argument to __netpoll_setup() ?

Management tasks should allow GFP_KERNEL allocations to have less
failure risks.

Its sad bonding uses the rwlock here instead of a mutex

Re: [PATCH 1/7] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup()

[Cong Wang]

On Fri, 2012-08-03 at 11:17 +0200, Eric Dumazet wrote:
> On Fri, 2012-07-27 at 23:37 +0800, Cong Wang wrote:
> > slave_enable_netpoll() and __netpoll_setup() may be called
> > with read_lock() held, so should use GFP_ATOMIC to allocate
> > memory.
> > 
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Cong Wang <amwang@redhat.com>
> > ---
> >  drivers/net/bonding/bond_main.c |    2 +-
> >  net/core/netpoll.c              |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index 6fae5f3..ab773d4 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -1235,7 +1235,7 @@ static inline int slave_enable_netpoll(struct slave *slave)
> >  	struct netpoll *np;
> >  	int err = 0;
> >  
> > -	np = kzalloc(sizeof(*np), GFP_KERNEL);
> > +	np = kzalloc(sizeof(*np), GFP_ATOMIC);
> >  	err = -ENOMEM;
> >  	if (!np)
> >  		goto out;
> > diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> > index b4c90e4..c78a966 100644
> > --- a/net/core/netpoll.c
> > +++ b/net/core/netpoll.c
> > @@ -734,7 +734,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
> >  	}
> >  
> >  	if (!ndev->npinfo) {
> > -		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
> > +		npinfo = kmalloc(sizeof(*npinfo), GFP_ATOMIC);
> >  		if (!npinfo) {
> >  			err = -ENOMEM;
> >  			goto out;
> 
> Yes this works, but maybe you instead could pass/add a gfp_t flags
> argument to __netpoll_setup() ?
> 
> Management tasks should allow GFP_KERNEL allocations to have less
> failure risks.
> 
> Its sad bonding uses the rwlock here instead of a mutex
> 

Yup, that is a good idea. I will update this patch.

Thanks!

Re: [PATCH 1/7] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup()

[Eric Dumazet]

On Fri, 2012-08-03 at 17:34 +0800, Cong Wang wrote:
> On Fri, 2012-08-03 at 11:17 +0200, Eric Dumazet wrote:
> > On Fri, 2012-07-27 at 23:37 +0800, Cong Wang wrote:
> > > slave_enable_netpoll() and __netpoll_setup() may be called
> > > with read_lock() held, so should use GFP_ATOMIC to allocate
> > > memory.
> > > 
> > > Cc: "David S. Miller" <davem@davemloft.net>
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: Cong Wang <amwang@redhat.com>
> > > ---
> > >  drivers/net/bonding/bond_main.c |    2 +-
> > >  net/core/netpoll.c              |    2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > index 6fae5f3..ab773d4 100644
> > > --- a/drivers/net/bonding/bond_main.c
> > > +++ b/drivers/net/bonding/bond_main.c
> > > @@ -1235,7 +1235,7 @@ static inline int slave_enable_netpoll(struct slave *slave)
> > >  	struct netpoll *np;
> > >  	int err = 0;
> > >  
> > > -	np = kzalloc(sizeof(*np), GFP_KERNEL);
> > > +	np = kzalloc(sizeof(*np), GFP_ATOMIC);
> > >  	err = -ENOMEM;
> > >  	if (!np)
> > >  		goto out;
> > > diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> > > index b4c90e4..c78a966 100644
> > > --- a/net/core/netpoll.c
> > > +++ b/net/core/netpoll.c
> > > @@ -734,7 +734,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
> > >  	}
> > >  
> > >  	if (!ndev->npinfo) {
> > > -		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
> > > +		npinfo = kmalloc(sizeof(*npinfo), GFP_ATOMIC);
> > >  		if (!npinfo) {
> > >  			err = -ENOMEM;
> > >  			goto out;
> > 
> > Yes this works, but maybe you instead could pass/add a gfp_t flags
> > argument to __netpoll_setup() ?
> > 
> > Management tasks should allow GFP_KERNEL allocations to have less
> > failure risks.
> > 
> > Its sad bonding uses the rwlock here instead of a mutex
> > 
> 
> Yup, that is a good idea. I will update this patch.
> 
> Thanks!
> 

I did this , just take it ;)

 drivers/net/bonding/bond_main.c |    6 +++---
 drivers/net/team/team.c         |   14 +++++++-------
 include/linux/netdevice.h       |    2 +-
 include/linux/netpoll.h         |    2 +-
 net/8021q/vlan_dev.c            |    6 +++---
 net/bridge/br_device.c          |   10 +++++-----
 net/bridge/br_if.c              |    2 +-
 net/bridge/br_private.h         |    4 ++--
 net/core/netpoll.c              |    8 ++++----
 9 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 6fae5f3..ccff590 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1235,12 +1235,12 @@ static inline int slave_enable_netpoll(struct slave *slave)
 	struct netpoll *np;
 	int err = 0;
 
-	np = kzalloc(sizeof(*np), GFP_KERNEL);
+	np = kzalloc(sizeof(*np), GFP_ATOMIC);
 	err = -ENOMEM;
 	if (!np)
 		goto out;
 
-	err = __netpoll_setup(np, slave->dev);
+	err = __netpoll_setup(np, slave->dev, GFP_ATOMIC);
 	if (err) {
 		kfree(np);
 		goto out;
@@ -1292,7 +1292,7 @@ static void bond_netpoll_cleanup(struct net_device *bond_dev)
 	read_unlock(&bond->lock);
 }
 
-static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni)
+static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni, gfp_t flags)
 {
 	struct bonding *bond = netdev_priv(dev);
 	struct slave *slave;
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 87707ab..3177d6b 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -795,16 +795,16 @@ static void team_port_leave(struct team *team, struct team_port *port)
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
-static int team_port_enable_netpoll(struct team *team, struct team_port *port)
+static int team_port_enable_netpoll(struct team *team, struct team_port *port, gfp_t flags)
 {
 	struct netpoll *np;
 	int err;
 
-	np = kzalloc(sizeof(*np), GFP_KERNEL);
+	np = kzalloc(sizeof(*np), flags);
 	if (!np)
 		return -ENOMEM;
 
-	err = __netpoll_setup(np, port->dev);
+	err = __netpoll_setup(np, port->dev, flags);
 	if (err) {
 		kfree(np);
 		return err;
@@ -833,7 +833,7 @@ static struct netpoll_info *team_netpoll_info(struct team *team)
 }
 
 #else
-static int team_port_enable_netpoll(struct team *team, struct team_port *port)
+static int team_port_enable_netpoll(struct team *team, struct team_port *port, gfp_t flags)
 {
 	return 0;
 }
@@ -913,7 +913,7 @@ static int team_port_add(struct team *team, struct net_device *port_dev)
 	}
 
 	if (team_netpoll_info(team)) {
-		err = team_port_enable_netpoll(team, port);
+		err = team_port_enable_netpoll(team, port, GFP_KERNEL);
 		if (err) {
 			netdev_err(dev, "Failed to enable netpoll on device %s\n",
 				   portname);
@@ -1443,7 +1443,7 @@ static void team_netpoll_cleanup(struct net_device *dev)
 }
 
 static int team_netpoll_setup(struct net_device *dev,
-			      struct netpoll_info *npifo)
+			      struct netpoll_info *npifo, gfp_t flags)
 {
 	struct team *team = netdev_priv(dev);
 	struct team_port *port;
@@ -1451,7 +1451,7 @@ static int team_netpoll_setup(struct net_device *dev,
 
 	mutex_lock(&team->lock);
 	list_for_each_entry(port, &team->port_list, list) {
-		err = team_port_enable_netpoll(team, port);
+		err = team_port_enable_netpoll(team, port, flags);
 		if (err) {
 			__team_netpoll_cleanup(team);
 			break;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a9db4f3..2ad76e3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -953,7 +953,7 @@ struct net_device_ops {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	void                    (*ndo_poll_controller)(struct net_device *dev);
 	int			(*ndo_netpoll_setup)(struct net_device *dev,
-						     struct netpoll_info *info);
+						     struct netpoll_info *info, gfp_t flags);
 	void			(*ndo_netpoll_cleanup)(struct net_device *dev);
 #endif
 	int			(*ndo_set_vf_mac)(struct net_device *dev,
diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index 28f5389..d67d4be3 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -43,7 +43,7 @@ struct netpoll_info {
 void netpoll_send_udp(struct netpoll *np, const char *msg, int len);
 void netpoll_print_options(struct netpoll *np);
 int netpoll_parse_options(struct netpoll *np, char *opt);
-int __netpoll_setup(struct netpoll *np, struct net_device *ndev);
+int __netpoll_setup(struct netpoll *np, struct net_device *ndev, gfp_t flags);
 int netpoll_setup(struct netpoll *np);
 int netpoll_trap(void);
 void netpoll_set_trap(int trap);
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 73a2a83..4ca0b39 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -669,19 +669,19 @@ static void vlan_dev_poll_controller(struct net_device *dev)
 	return;
 }
 
-static int vlan_dev_netpoll_setup(struct net_device *dev, struct netpoll_info *npinfo)
+static int vlan_dev_netpoll_setup(struct net_device *dev, struct netpoll_info *npinfo, gfp_t flags)
 {
 	struct vlan_dev_priv *info = vlan_dev_priv(dev);
 	struct net_device *real_dev = info->real_dev;
 	struct netpoll *netpoll;
 	int err = 0;
 
-	netpoll = kzalloc(sizeof(*netpoll), GFP_KERNEL);
+	netpoll = kzalloc(sizeof(*netpoll), flags);
 	err = -ENOMEM;
 	if (!netpoll)
 		goto out;
 
-	err = __netpoll_setup(netpoll, real_dev);
+	err = __netpoll_setup(netpoll, real_dev, flags);
 	if (err) {
 		kfree(netpoll);
 		goto out;
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 3334845..efcd36c 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -213,7 +213,7 @@ static void br_netpoll_cleanup(struct net_device *dev)
 	}
 }
 
-static int br_netpoll_setup(struct net_device *dev, struct netpoll_info *ni)
+static int br_netpoll_setup(struct net_device *dev, struct netpoll_info *ni, gfp_t flags)
 {
 	struct net_bridge *br = netdev_priv(dev);
 	struct net_bridge_port *p, *n;
@@ -223,7 +223,7 @@ static int br_netpoll_setup(struct net_device *dev, struct netpoll_info *ni)
 		if (!p->dev)
 			continue;
 
-		err = br_netpoll_enable(p);
+		err = br_netpoll_enable(p, flags);
 		if (err)
 			goto fail;
 	}
@@ -236,17 +236,17 @@ fail:
 	goto out;
 }
 
-int br_netpoll_enable(struct net_bridge_port *p)
+int br_netpoll_enable(struct net_bridge_port *p, gfp_t flags)
 {
 	struct netpoll *np;
 	int err = 0;
 
-	np = kzalloc(sizeof(*p->np), GFP_KERNEL);
+	np = kzalloc(sizeof(*p->np), flags);
 	err = -ENOMEM;
 	if (!np)
 		goto out;
 
-	err = __netpoll_setup(np, p->dev);
+	err = __netpoll_setup(np, p->dev, flags);
 	if (err) {
 		kfree(np);
 		goto out;
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index e1144e1..171fd6b 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -361,7 +361,7 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 	if (err)
 		goto err2;
 
-	if (br_netpoll_info(br) && ((err = br_netpoll_enable(p))))
+	if (br_netpoll_info(br) && ((err = br_netpoll_enable(p, GFP_KERNEL))))
 		goto err3;
 
 	err = netdev_set_master(dev, br->dev);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index a768b24..bfdb5ab 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -316,7 +316,7 @@ static inline void br_netpoll_send_skb(const struct net_bridge_port *p,
 		netpoll_send_skb(np, skb);
 }
 
-extern int br_netpoll_enable(struct net_bridge_port *p);
+extern int br_netpoll_enable(struct net_bridge_port *p, gfp_t flags);
 extern void br_netpoll_disable(struct net_bridge_port *p);
 #else
 static inline struct netpoll_info *br_netpoll_info(struct net_bridge *br)
@@ -329,7 +329,7 @@ static inline void br_netpoll_send_skb(const struct net_bridge_port *p,
 {
 }
 
-static inline int br_netpoll_enable(struct net_bridge_port *p)
+static inline int br_netpoll_enable(struct net_bridge_port *p, gfp_t flags)
 {
 	return 0;
 }
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index b4c90e4..37cc854 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -715,7 +715,7 @@ int netpoll_parse_options(struct netpoll *np, char *opt)
 }
 EXPORT_SYMBOL(netpoll_parse_options);
 
-int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
+int __netpoll_setup(struct netpoll *np, struct net_device *ndev, gfp_t gfp)
 {
 	struct netpoll_info *npinfo;
 	const struct net_device_ops *ops;
@@ -734,7 +734,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	}
 
 	if (!ndev->npinfo) {
-		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
+		npinfo = kmalloc(sizeof(*npinfo), gfp);
 		if (!npinfo) {
 			err = -ENOMEM;
 			goto out;
@@ -752,7 +752,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 
 		ops = np->dev->netdev_ops;
 		if (ops->ndo_netpoll_setup) {
-			err = ops->ndo_netpoll_setup(ndev, npinfo);
+			err = ops->ndo_netpoll_setup(ndev, npinfo, gfp);
 			if (err)
 				goto free_npinfo;
 		}
@@ -857,7 +857,7 @@ int netpoll_setup(struct netpoll *np)
 	refill_skbs();
 
 	rtnl_lock();
-	err = __netpoll_setup(np, ndev);
+	err = __netpoll_setup(np, ndev, GFP_KERNEL);
 	rtnl_unlock();
 
 	if (err)

Re: [PATCH 1/7] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup()

[Cong Wang]

On Fri, 2012-08-03 at 12:10 +0200, Eric Dumazet wrote: 
> 
> I did this , just take it ;)

Do we have to pass gfp to ->ndo_netpoll_setup() too? It seems no, so far
I don't think we have to do that.

Thanks.

Re: [PATCH 1/7] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup()

[Eric Dumazet]

On Mon, 2012-08-06 at 17:08 +0800, Cong Wang wrote:
> On Fri, 2012-08-03 at 12:10 +0200, Eric Dumazet wrote: 
> > 
> > I did this , just take it ;)
> 
> Do we have to pass gfp to ->ndo_netpoll_setup() too? It seems no, so far
> I don't think we have to do that.
> 
> Thanks.
> 

It is needed.

->ndo_netpoll_setup() is called from __netpoll_setup()

So it would make no sense to allow GFP_KERNEL allocations in
ndo_netpoll_setup() hgandlers if we called __netpoll_setup() with
GFP_ATOMIC gfp

Re: [PATCH 1/7] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup()

[Cong Wang]

On Mon, 2012-08-06 at 11:44 +0200, Eric Dumazet wrote:
> On Mon, 2012-08-06 at 17:08 +0800, Cong Wang wrote:
> > On Fri, 2012-08-03 at 12:10 +0200, Eric Dumazet wrote: 
> > > 
> > > I did this , just take it ;)
> > 
> > Do we have to pass gfp to ->ndo_netpoll_setup() too? It seems no, so far
> > I don't think we have to do that.
> > 
> > Thanks.
> > 
> 
> It is needed.
> 
> ->ndo_netpoll_setup() is called from __netpoll_setup()
> 
> So it would make no sense to allow GFP_KERNEL allocations in
> ndo_netpoll_setup() hgandlers if we called __netpoll_setup() with
> GFP_ATOMIC gfp
> 

I see your point. Thanks!

[PATCH 2/7] netpoll: make __netpoll_cleanup non-block

[Cong Wang]

Like the previous patch, slave_disable_netpoll() and __netpoll_cleanup()
may be called with read_lock() held too, so we should make them
non-block, by moving the cleanup and kfree() to call_rcu_bh() callbacks.

Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 drivers/net/bonding/bond_main.c |    4 +--
 include/linux/netpoll.h         |    3 ++
 net/8021q/vlan_dev.c            |    6 +----
 net/bridge/br_device.c          |    6 +----
 net/core/netpoll.c              |   42 +++++++++++++++++++++++++++++---------
 5 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index ab773d4..9907889 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1257,9 +1257,7 @@ static inline void slave_disable_netpoll(struct slave *slave)
 		return;
 
 	slave->np = NULL;
-	synchronize_rcu_bh();
-	__netpoll_cleanup(np);
-	kfree(np);
+	__netpoll_free_rcu(np);
 }
 static inline bool slave_dev_support_netpoll(struct net_device *slave_dev)
 {
diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index 28f5389..5011d74 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -23,6 +23,7 @@ struct netpoll {
 	u8 remote_mac[ETH_ALEN];
 
 	struct list_head rx; /* rx_np list element */
+	struct rcu_head rcu;
 };
 
 struct netpoll_info {
@@ -38,6 +39,7 @@ struct netpoll_info {
 	struct delayed_work tx_work;
 
 	struct netpoll *netpoll;
+	struct rcu_head rcu;
 };
 
 void netpoll_send_udp(struct netpoll *np, const char *msg, int len);
@@ -48,6 +50,7 @@ int netpoll_setup(struct netpoll *np);
 int netpoll_trap(void);
 void netpoll_set_trap(int trap);
 void __netpoll_cleanup(struct netpoll *np);
+void __netpoll_free_rcu(struct netpoll *np);
 void netpoll_cleanup(struct netpoll *np);
 int __netpoll_rx(struct sk_buff *skb);
 void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 73a2a83..43e14a2 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -703,11 +703,7 @@ static void vlan_dev_netpoll_cleanup(struct net_device *dev)
 
 	info->netpoll = NULL;
 
-        /* Wait for transmitting packets to finish before freeing. */
-        synchronize_rcu_bh();
-
-        __netpoll_cleanup(netpoll);
-        kfree(netpoll);
+	__netpoll_free_rcu(netpoll);
 }
 #endif /* CONFIG_NET_POLL_CONTROLLER */
 
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 3334845..bb6a5dd 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -267,11 +267,7 @@ void br_netpoll_disable(struct net_bridge_port *p)
 
 	p->np = NULL;
 
-	/* Wait for transmitting packets to finish before freeing. */
-	synchronize_rcu_bh();
-
-	__netpoll_cleanup(np);
-	kfree(np);
+	__netpoll_free_rcu(np);
 }
 
 #endif
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index c78a966..19dddef 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -878,6 +878,24 @@ static int __init netpoll_init(void)
 }
 core_initcall(netpoll_init);
 
+static void rcu_cleanup_netpoll_info(struct rcu_head *rcu_head)
+{
+	struct netpoll_info *npinfo =
+			container_of(rcu_head, struct netpoll_info, rcu);
+
+	skb_queue_purge(&npinfo->arp_tx);
+	skb_queue_purge(&npinfo->txq);
+
+	/* we can't call cancel_delayed_work_sync here, as we are in softirq*/
+	cancel_delayed_work(&npinfo->tx_work);
+
+	/* clean after last, unfinished work */
+	__skb_queue_purge(&npinfo->txq);
+	/* now cancel it again */
+	cancel_delayed_work(&npinfo->tx_work);
+	kfree(npinfo);
+}
+
 void __netpoll_cleanup(struct netpoll *np)
 {
 	struct netpoll_info *npinfo;
@@ -903,20 +921,24 @@ void __netpoll_cleanup(struct netpoll *np)
 			ops->ndo_netpoll_cleanup(np->dev);
 
 		RCU_INIT_POINTER(np->dev->npinfo, NULL);
+		call_rcu_bh(&npinfo->rcu, rcu_cleanup_netpoll_info);
+	}
+}
+EXPORT_SYMBOL_GPL(__netpoll_cleanup);
 
-		/* avoid racing with NAPI reading npinfo */
-		synchronize_rcu_bh();
+static void rcu_cleanup_netpoll(struct rcu_head *rcu_head)
+{
+	struct netpoll *np = container_of(rcu_head, struct netpoll, rcu);
 
-		skb_queue_purge(&npinfo->arp_tx);
-		skb_queue_purge(&npinfo->txq);
-		cancel_delayed_work_sync(&npinfo->tx_work);
+	__netpoll_cleanup(np);
+	kfree(np);
+}
 
-		/* clean after last, unfinished work */
-		__skb_queue_purge(&npinfo->txq);
-		kfree(npinfo);
-	}
+void __netpoll_free_rcu(struct netpoll *np)
+{
+	call_rcu_bh(&np->rcu, rcu_cleanup_netpoll);
 }
-EXPORT_SYMBOL_GPL(__netpoll_cleanup);
+EXPORT_SYMBOL_GPL(__netpoll_free_rcu);
 
 void netpoll_cleanup(struct netpoll *np)
 {
-- 
1.7.7.6

Re: [PATCH 2/7] netpoll: make __netpoll_cleanup non-block

[Neil Horman]

On Fri, Jul 27, 2012 at 11:37:59PM +0800, Cong Wang wrote:
> Like the previous patch, slave_disable_netpoll() and __netpoll_cleanup()
> may be called with read_lock() held too, so we should make them
> non-block, by moving the cleanup and kfree() to call_rcu_bh() callbacks.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> ---
>  drivers/net/bonding/bond_main.c |    4 +--
>  include/linux/netpoll.h         |    3 ++
>  net/8021q/vlan_dev.c            |    6 +----
>  net/bridge/br_device.c          |    6 +----
>  net/core/netpoll.c              |   42 +++++++++++++++++++++++++++++---------
>  5 files changed, 38 insertions(+), 23 deletions(-)
><snip>

>  	struct netpoll_info *npinfo;
> @@ -903,20 +921,24 @@ void __netpoll_cleanup(struct netpoll *np)
>  			ops->ndo_netpoll_cleanup(np->dev);
>  
>  		RCU_INIT_POINTER(np->dev->npinfo, NULL);
> +		call_rcu_bh(&npinfo->rcu, rcu_cleanup_netpoll_info);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(__netpoll_cleanup);
>  
> -		/* avoid racing with NAPI reading npinfo */
> -		synchronize_rcu_bh();
> +static void rcu_cleanup_netpoll(struct rcu_head *rcu_head)
> +{
> +	struct netpoll *np = container_of(rcu_head, struct netpoll, rcu);
>  
> -		skb_queue_purge(&npinfo->arp_tx);
> -		skb_queue_purge(&npinfo->txq);
> -		cancel_delayed_work_sync(&npinfo->tx_work);
> +	__netpoll_cleanup(np);
> +	kfree(np);
> +}
>  
> -		/* clean after last, unfinished work */
> -		__skb_queue_purge(&npinfo->txq);
> -		kfree(npinfo);
> -	}
> +void __netpoll_free_rcu(struct netpoll *np)
> +{
> +	call_rcu_bh(&np->rcu, rcu_cleanup_netpoll);
Here, and above I see you using an rcu_head to defer cleanup, until after all
pointer uses are dropped, but I don't see any modification of code points that
dereference any struct netpoll pointers to include
rcu_read_lock()/rcu_read_unlock().  Without those using rcu to defer cleanup is
pointless, as the rcu code won't know when its safe to run.  You're no better
off that you would be just calling __netpoll_cleanup directly.
Neil

>  }
> -EXPORT_SYMBOL_GPL(__netpoll_cleanup);
> +EXPORT_SYMBOL_GPL(__netpoll_free_rcu);
>  
>  void netpoll_cleanup(struct netpoll *np)
>  {
> -- 
> 1.7.7.6
> 
> 

Re: [PATCH 2/7] netpoll: make __netpoll_cleanup non-block

[Cong Wang]

On Fri, 2012-07-27 at 14:40 -0400, Neil Horman wrote:
> Here, and above I see you using an rcu_head to defer cleanup, until after all
> pointer uses are dropped, but I don't see any modification of code points that
> dereference any struct netpoll pointers to include
> rcu_read_lock()/rcu_read_unlock().  Without those using rcu to defer cleanup is
> pointless, as the rcu code won't know when its safe to run.  You're no better
> off that you would be just calling __netpoll_cleanup directly.

Hi, Neil,

Actually they are protected by rcu_read_lock_bh() which is implied by
local_irq_disable(), see:

commit f0f9deae9e7c421fa0c1c627beb8e174325e1ba7
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Fri Sep 17 16:55:03 2010 -0700

    netpoll: Disable IRQ around RCU dereference in netpoll_rx
    
    We cannot use rcu_dereference_bh safely in netpoll_rx as we may
    be called with IRQs disabled.  We could however simply disable
    IRQs as that too causes BH to be disabled and is safe in either
    case.
    
    Thanks to John Linville for discovering this bug and providing
    a patch.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
    Signed-off-by: David S. Miller <davem@davemloft.net>


Thanks.

[PATCH 3/7] netconsole: do not release spin_lock before calling __netpoll_cleanup

[Cong Wang]

With the previous patch applied, __netpoll_cleanup() is non-block now,
so we don't need to release the spin_lock before calling it.

Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 drivers/net/netconsole.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index f9347ea..f0ad56c 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -640,12 +640,7 @@ static int netconsole_netdev_event(struct notifier_block *this,
 				 * rtnl_lock already held
 				 */
 				if (nt->np.dev) {
-					spin_unlock_irqrestore(
-							      &target_list_lock,
-							      flags);
 					__netpoll_cleanup(&nt->np);
-					spin_lock_irqsave(&target_list_lock,
-							  flags);
 					dev_put(nt->np.dev);
 					nt->np.dev = NULL;
 					netconsole_target_put(nt);
-- 
1.7.7.6

[PATCH 4/7] bridge: call NETDEV_RELEASE notifier in br_del_if()

[Cong Wang]

When a bridge interface deletes its underlying ports, it should
notify netconsole too, like what bonding interface does.

Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 net/bridge/br_if.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index e1144e1..d243914 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -427,6 +427,7 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
 	if (!p || p->br != br)
 		return -EINVAL;
 
+	call_netdevice_notifiers(NETDEV_RELEASE, br->dev);
 	del_nbp(p);
 
 	spin_lock_bh(&br->lock);
-- 
1.7.7.6

Re: [PATCH 4/7] bridge: call NETDEV_RELEASE notifier in br_del_if()

[Stephen Hemminger]

On Fri, 27 Jul 2012 23:38:01 +0800
Cong Wang <amwang@redhat.com> wrote:

> When a bridge interface deletes its underlying ports, it should
> notify netconsole too, like what bonding interface does.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> ---
>  net/bridge/br_if.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index e1144e1..d243914 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -427,6 +427,7 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
>  	if (!p || p->br != br)
>  		return -EINVAL;
>  
> +	call_netdevice_notifiers(NETDEV_RELEASE, br->dev);
>  	del_nbp(p);
>  
>  	spin_lock_bh(&br->lock);

Since you can have multiple ports attached to the bridge, this
doesn't seem correct. Don't you want the netconsole to keep going
on the other ports of the bridge?

What exactly is the problem with having netconsole persist?

Re: [PATCH 4/7] bridge: call NETDEV_RELEASE notifier in br_del_if()

[Cong Wang]

On Fri, 2012-07-27 at 08:50 -0700, Stephen Hemminger wrote:
> On Fri, 27 Jul 2012 23:38:01 +0800
> Cong Wang <amwang@redhat.com> wrote:
> 
> > When a bridge interface deletes its underlying ports, it should
> > notify netconsole too, like what bonding interface does.
> > 
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Signed-off-by: Cong Wang <amwang@redhat.com>
> > ---
> >  net/bridge/br_if.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> > index e1144e1..d243914 100644
> > --- a/net/bridge/br_if.c
> > +++ b/net/bridge/br_if.c
> > @@ -427,6 +427,7 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
> >  	if (!p || p->br != br)
> >  		return -EINVAL;
> >  
> > +	call_netdevice_notifiers(NETDEV_RELEASE, br->dev);
> >  	del_nbp(p);
> >  
> >  	spin_lock_bh(&br->lock);
> 
> Since you can have multiple ports attached to the bridge, this
> doesn't seem correct. Don't you want the netconsole to keep going
> on the other ports of the bridge?
> 
> What exactly is the problem with having netconsole persist?

Hmm, I saw an incorrect log message when deleting the last port from the
bridge when netconsole is setup on it. After rethinking it today, you
are right we should not simply disable netconsole when one port is
detached, as we have no way to know if that port is used to reach the
netconsole server.

So, please ignore this patch.

Thanks.

[PATCH 5/7] netpoll: take rcu_read_lock_bh() in netpoll_rx()

[Cong Wang]

In __netpoll_rx(), it dereferences ->npinfo without rcu_dereference_bh(),
this patch fixes it by using the 'npinfo' passed from netpoll_rx()
where it is already dereferenced with rcu_dereference_bh().

Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 include/linux/netpoll.h |    4 ++--
 net/core/netpoll.c      |    3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index 5011d74..36f938c 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -52,7 +52,7 @@ void netpoll_set_trap(int trap);
 void __netpoll_cleanup(struct netpoll *np);
 void __netpoll_free_rcu(struct netpoll *np);
 void netpoll_cleanup(struct netpoll *np);
-int __netpoll_rx(struct sk_buff *skb);
+int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo);
 void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
 			     struct net_device *dev);
 static inline void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
@@ -77,7 +77,7 @@ static inline bool netpoll_rx(struct sk_buff *skb)
 
 	spin_lock(&npinfo->rx_lock);
 	/* check rx_flags again with the lock held */
-	if (npinfo->rx_flags && __netpoll_rx(skb))
+	if (npinfo->rx_flags && __netpoll_rx(skb, npinfo))
 		ret = true;
 	spin_unlock(&npinfo->rx_lock);
 
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 19dddef..3965fdb 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -543,13 +543,12 @@ static void arp_reply(struct sk_buff *skb)
 	spin_unlock_irqrestore(&npinfo->rx_lock, flags);
 }
 
-int __netpoll_rx(struct sk_buff *skb)
+int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
 {
 	int proto, len, ulen;
 	int hits = 0;
 	const struct iphdr *iph;
 	struct udphdr *uh;
-	struct netpoll_info *npinfo = skb->dev->npinfo;
 	struct netpoll *np, *tmp;
 
 	if (list_empty(&npinfo->rx_np))
-- 
1.7.7.6

[PATCH 6/7] netpoll: use netpoll_rx_on() in netpoll_rx()

[Cong Wang]

The logic of the code is same, just call netpoll_rx_on().

Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 include/linux/netpoll.h |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index 36f938c..f654fa3 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -63,6 +63,13 @@ static inline void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
 
 
 #ifdef CONFIG_NETPOLL
+static inline int netpoll_rx_on(struct sk_buff *skb)
+{
+	struct netpoll_info *npinfo = rcu_dereference_bh(skb->dev->npinfo);
+
+	return npinfo && (!list_empty(&npinfo->rx_np) || npinfo->rx_flags);
+}
+
 static inline bool netpoll_rx(struct sk_buff *skb)
 {
 	struct netpoll_info *npinfo;
@@ -70,11 +77,11 @@ static inline bool netpoll_rx(struct sk_buff *skb)
 	bool ret = false;
 
 	local_irq_save(flags);
-	npinfo = rcu_dereference_bh(skb->dev->npinfo);
 
-	if (!npinfo || (list_empty(&npinfo->rx_np) && !npinfo->rx_flags))
+	if (!netpoll_rx_on(skb))
 		goto out;
 
+	npinfo = rcu_dereference_bh(skb->dev->npinfo);
 	spin_lock(&npinfo->rx_lock);
 	/* check rx_flags again with the lock held */
 	if (npinfo->rx_flags && __netpoll_rx(skb, npinfo))
@@ -86,13 +93,6 @@ out:
 	return ret;
 }
 
-static inline int netpoll_rx_on(struct sk_buff *skb)
-{
-	struct netpoll_info *npinfo = rcu_dereference_bh(skb->dev->npinfo);
-
-	return npinfo && (!list_empty(&npinfo->rx_np) || npinfo->rx_flags);
-}
-
 static inline int netpoll_receive_skb(struct sk_buff *skb)
 {
 	if (!list_empty(&skb->dev->napi_list))
-- 
1.7.7.6

[PATCH 7/7] netpoll: take rcu_read_lock_bh() in netpoll_send_skb_on_dev()

[Cong Wang]

This patch fixes several problems in the call path of
netpoll_send_skb_on_dev():

1. We already disable IRQ's before calling netpoll_send_skb_on_dev(),
   so we don't need to disable IRQ's again.
2. All the callees of netpoll_send_skb_on_dev() should use
   rcu_dereference_bh() to dereference ->npinfo.
3. Rename arp_reply() to netpoll_arp_reply(), the former is too generic.

Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
 net/core/netpoll.c |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 3965fdb..d6e192b 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -54,7 +54,7 @@ static atomic_t trapped;
 	 MAX_UDP_CHUNK)
 
 static void zap_completion_queue(void);
-static void arp_reply(struct sk_buff *skb);
+static void netpoll_arp_reply(struct sk_buff *skb, struct netpoll_info *npinfo);
 
 static unsigned int carrier_timeout = 4;
 module_param(carrier_timeout, uint, 0644);
@@ -170,7 +170,8 @@ static void poll_napi(struct net_device *dev)
 	list_for_each_entry(napi, &dev->napi_list, dev_list) {
 		if (napi->poll_owner != smp_processor_id() &&
 		    spin_trylock(&napi->poll_lock)) {
-			budget = poll_one_napi(dev->npinfo, napi, budget);
+			budget = poll_one_napi(rcu_dereference_bh(dev->npinfo),
+					       napi, budget);
 			spin_unlock(&napi->poll_lock);
 
 			if (!budget)
@@ -185,13 +186,14 @@ static void service_arp_queue(struct netpoll_info *npi)
 		struct sk_buff *skb;
 
 		while ((skb = skb_dequeue(&npi->arp_tx)))
-			arp_reply(skb);
+			netpoll_arp_reply(skb, npi);
 	}
 }
 
 static void netpoll_poll_dev(struct net_device *dev)
 {
 	const struct net_device_ops *ops;
+	struct netpoll_info *ni = rcu_dereference_bh(dev->npinfo);
 
 	if (!dev || !netif_running(dev))
 		return;
@@ -206,17 +208,18 @@ static void netpoll_poll_dev(struct net_device *dev)
 	poll_napi(dev);
 
 	if (dev->flags & IFF_SLAVE) {
-		if (dev->npinfo) {
+		if (ni) {
 			struct net_device *bond_dev = dev->master;
 			struct sk_buff *skb;
-			while ((skb = skb_dequeue(&dev->npinfo->arp_tx))) {
+			struct netpoll_info *bond_ni = rcu_dereference_bh(bond_dev->npinfo);
+			while ((skb = skb_dequeue(&ni->arp_tx))) {
 				skb->dev = bond_dev;
-				skb_queue_tail(&bond_dev->npinfo->arp_tx, skb);
+				skb_queue_tail(&bond_ni->arp_tx, skb);
 			}
 		}
 	}
 
-	service_arp_queue(dev->npinfo);
+	service_arp_queue(ni);
 
 	zap_completion_queue();
 }
@@ -302,6 +305,7 @@ static int netpoll_owner_active(struct net_device *dev)
 	return 0;
 }
 
+/* call with IRQ disabled */
 void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
 			     struct net_device *dev)
 {
@@ -309,8 +313,11 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
 	unsigned long tries;
 	const struct net_device_ops *ops = dev->netdev_ops;
 	/* It is up to the caller to keep npinfo alive. */
-	struct netpoll_info *npinfo = np->dev->npinfo;
+	struct netpoll_info *npinfo;
+
+	WARN_ON_ONCE(!irqs_disabled());
 
+	npinfo = rcu_dereference_bh(np->dev->npinfo);
 	if (!npinfo || !netif_running(dev) || !netif_device_present(dev)) {
 		__kfree_skb(skb);
 		return;
@@ -319,11 +326,9 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
 	/* don't get messages out of order, and no recursion */
 	if (skb_queue_len(&npinfo->txq) == 0 && !netpoll_owner_active(dev)) {
 		struct netdev_queue *txq;
-		unsigned long flags;
 
 		txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
 
-		local_irq_save(flags);
 		/* try until next clock tick */
 		for (tries = jiffies_to_usecs(1)/USEC_PER_POLL;
 		     tries > 0; --tries) {
@@ -347,10 +352,9 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
 		}
 
 		WARN_ONCE(!irqs_disabled(),
-			"netpoll_send_skb(): %s enabled interrupts in poll (%pF)\n",
+			"netpoll_send_skb_on_dev(): %s enabled interrupts in poll (%pF)\n",
 			dev->name, ops->ndo_start_xmit);
 
-		local_irq_restore(flags);
 	}
 
 	if (status != NETDEV_TX_OK) {
@@ -423,9 +427,8 @@ void netpoll_send_udp(struct netpoll *np, const char *msg, int len)
 }
 EXPORT_SYMBOL(netpoll_send_udp);
 
-static void arp_reply(struct sk_buff *skb)
+static void netpoll_arp_reply(struct sk_buff *skb, struct netpoll_info *npinfo)
 {
-	struct netpoll_info *npinfo = skb->dev->npinfo;
 	struct arphdr *arp;
 	unsigned char *arp_ptr;
 	int size, type = ARPOP_REPLY, ptype = ETH_P_ARP;
-- 
1.7.7.6

Re: [PATCH 0/7] netpoll and netconsole fixes for 3.6

[Cong Wang]

Ping... Any more reviews?

On Fri, 2012-07-27 at 23:37 +0800, Cong Wang wrote:
> This patch fixes serval problems in netconsole and netpoll.
> 
> I ran this patch in my KVM guest with some netpoll test cases,
> even covered with some corner cases, everything worked as expected.
> 
> BTW, my kernel config enables the following validatation options:
> 
> CONFIG_DEBUG_ATOMIC_SLEEP=y
> CONFIG_PROVE_LOCKING=y
> CONFIG_PROVE_RCU=y
> CONFIG_LOCKDEP=y
> CONFIG_RCU_CPU_STALL_TIMEOUT=60
> CONFIG_LOCKUP_DETECTOR=y
> CONFIG_HARDLOCKUP_DETECTOR=y
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> 
> ---
> 
> Cong Wang (7):
>   netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup()
>   netpoll: make __netpoll_cleanup non-block
>   netconsole: do not release spin_lock when calling __netpoll_cleanup
>   bridge: call NETDEV_RELEASE notifier in br_del_if()
>   netpoll: take rcu_read_lock_bh() in netpoll_rx()
>   netpoll: use netpoll_rx_on() in netpoll_rx()
>   netpoll: take rcu_read_lock_bh() in netpoll_send_skb_on_dev()
> 
>  drivers/net/bonding/bond_main.c |    6 +--
>  drivers/net/netconsole.c        |    5 --
>  include/linux/netpoll.h         |   25 +++++++-----
>  net/8021q/vlan_dev.c            |    6 +--
>  net/bridge/br_device.c          |    6 +--
>  net/bridge/br_if.c              |    1 +
>  net/core/netpoll.c              |   78 +++++++++++++++++++++++++-------------
>  7 files changed, 70 insertions(+), 57 deletions(-)
>