* [PATCH 1/8] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup()
2012-08-06 14:23 [PATCH v2 0/8] some netpoll and netconsole fixes Cong Wang
@ 2012-08-06 14:23 ` Cong Wang
2012-08-06 15:20 ` Eric Dumazet
2012-08-06 14:23 ` [PATCH 2/8] netpoll: make __netpoll_cleanup non-block Cong Wang
` (7 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Cong Wang @ 2012-08-06 14:23 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Eric Dumazet, David S. Miller
slave_enable_netpoll() and __netpoll_setup() may be called
with read_lock() held, so should use GFP_ATOMIC to allocate
memory. Eric suggested to pass gfp flags to __netpoll_setup().
Cc: Eric Dumazet <eric.dumazet@gmail.com>
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 | 6 +++---
drivers/net/team/team.c | 16 +++++++++-------
include/linux/netdevice.h | 3 ++-
include/linux/netpoll.h | 2 +-
net/8021q/vlan_dev.c | 7 ++++---
net/bridge/br_device.c | 12 ++++++------
net/bridge/br_if.c | 2 +-
net/bridge/br_private.h | 4 ++--
net/core/netpoll.c | 8 ++++----
9 files changed, 32 insertions(+), 28 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 6fae5f3..8697136 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 gfp)
{
struct bonding *bond = netdev_priv(dev);
struct slave *slave;
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index ba10c46..bbb8524 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -913,16 +913,17 @@ 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 gfp)
{
struct netpoll *np;
int err;
- np = kzalloc(sizeof(*np), GFP_KERNEL);
+ np = kzalloc(sizeof(*np), gfp);
if (!np)
return -ENOMEM;
- err = __netpoll_setup(np, port->dev);
+ err = __netpoll_setup(np, port->dev, gfp);
if (err) {
kfree(np);
return err;
@@ -951,7 +952,8 @@ 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 gfp)
{
return 0;
}
@@ -1032,7 +1034,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);
@@ -1627,7 +1629,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 gfp)
{
struct team *team = netdev_priv(dev);
struct team_port *port;
@@ -1635,7 +1637,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, gfp);
if (err) {
__team_netpoll_cleanup(team);
break;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a9db4f3..3560d68 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -953,7 +953,8 @@ 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 gfp);
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..bf2d51e 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 gfp);
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..ee4ae09 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -669,19 +669,20 @@ 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 gfp)
{
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), gfp);
err = -ENOMEM;
if (!netpoll)
goto out;
- err = __netpoll_setup(netpoll, real_dev);
+ err = __netpoll_setup(netpoll, real_dev, gfp);
if (err) {
kfree(netpoll);
goto out;
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 3334845..ed0e0f9 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -213,7 +213,8 @@ 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 gfp)
{
struct net_bridge *br = netdev_priv(dev);
struct net_bridge_port *p, *n;
@@ -222,8 +223,7 @@ static int br_netpoll_setup(struct net_device *dev, struct netpoll_info *ni)
list_for_each_entry_safe(p, n, &br->port_list, list) {
if (!p->dev)
continue;
-
- err = br_netpoll_enable(p);
+ err = br_netpoll_enable(p, gfp);
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 gfp)
{
struct netpoll *np;
int err = 0;
- np = kzalloc(sizeof(*p->np), GFP_KERNEL);
+ np = kzalloc(sizeof(*p->np), gfp);
err = -ENOMEM;
if (!np)
goto out;
- err = __netpoll_setup(np, p->dev);
+ err = __netpoll_setup(np, p->dev, gfp);
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..f507d2a 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 gfp);
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 gfp)
{
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)
--
1.7.7.6
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/8] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup()
2012-08-06 14:23 ` [PATCH 1/8] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup() Cong Wang
@ 2012-08-06 15:20 ` Eric Dumazet
2012-08-07 2:28 ` Cong Wang
0 siblings, 1 reply; 21+ messages in thread
From: Eric Dumazet @ 2012-08-06 15:20 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, David S. Miller
On Mon, 2012-08-06 at 22:23 +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. Eric suggested to pass gfp flags to __netpoll_setup().
>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> ---
You based this on net-next, but didnt add the net-next suffix in your
[PATCH ...] description.
Signed-off-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/8] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup()
2012-08-06 15:20 ` Eric Dumazet
@ 2012-08-07 2:28 ` Cong Wang
0 siblings, 0 replies; 21+ messages in thread
From: Cong Wang @ 2012-08-07 2:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David S. Miller
On Mon, 2012-08-06 at 17:20 +0200, Eric Dumazet wrote:
> On Mon, 2012-08-06 at 22:23 +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. Eric suggested to pass gfp flags to __netpoll_setup().
> >
> > Cc: Eric Dumazet <eric.dumazet@gmail.com>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Cong Wang <amwang@redhat.com>
> > ---
>
> You based this on net-next, but didnt add the net-next suffix in your
> [PATCH ...] description.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
>
Oh, it is default, isn't it? :-D Anyway, I will add it the next time
when I send network patches.
Thanks!
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/8] netpoll: make __netpoll_cleanup non-block
2012-08-06 14:23 [PATCH v2 0/8] some netpoll and netconsole fixes Cong Wang
2012-08-06 14:23 ` [PATCH 1/8] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup() Cong Wang
@ 2012-08-06 14:23 ` Cong Wang
2012-08-06 14:23 ` [PATCH 3/8] netconsole: do not release spin_lock when calling __netpoll_cleanup Cong Wang
` (6 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Cong Wang @ 2012-08-06 14:23 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, David S. Miller
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 8697136..e428916 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 bf2d51e..907812e 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 ee4ae09..b65623f 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -704,11 +704,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 ed0e0f9..f41ba40 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 37cc854..dc17f1d 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/8] netconsole: do not release spin_lock when calling __netpoll_cleanup
2012-08-06 14:23 [PATCH v2 0/8] some netpoll and netconsole fixes Cong Wang
2012-08-06 14:23 ` [PATCH 1/8] netpoll: use GFP_ATOMIC in slave_enable_netpoll() and __netpoll_setup() Cong Wang
2012-08-06 14:23 ` [PATCH 2/8] netpoll: make __netpoll_cleanup non-block Cong Wang
@ 2012-08-06 14:23 ` Cong Wang
2012-08-06 14:23 ` [PATCH 4/8] netpoll: take rcu_read_lock_bh() in netpoll_rx() Cong Wang
` (5 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Cong Wang @ 2012-08-06 14:23 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, David S. Miller
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/8] netpoll: take rcu_read_lock_bh() in netpoll_rx()
2012-08-06 14:23 [PATCH v2 0/8] some netpoll and netconsole fixes Cong Wang
` (2 preceding siblings ...)
2012-08-06 14:23 ` [PATCH 3/8] netconsole: do not release spin_lock when calling __netpoll_cleanup Cong Wang
@ 2012-08-06 14:23 ` Cong Wang
2012-08-06 14:23 ` [PATCH 5/8] netpoll: use netpoll_rx_on() " Cong Wang
` (4 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Cong Wang @ 2012-08-06 14:23 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, David S. Miller
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 907812e..5d881c3 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 dc17f1d..d055bb0 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 5/8] netpoll: use netpoll_rx_on() in netpoll_rx()
2012-08-06 14:23 [PATCH v2 0/8] some netpoll and netconsole fixes Cong Wang
` (3 preceding siblings ...)
2012-08-06 14:23 ` [PATCH 4/8] netpoll: take rcu_read_lock_bh() in netpoll_rx() Cong Wang
@ 2012-08-06 14:23 ` Cong Wang
2012-08-06 14:23 ` [PATCH 6/8] netpoll: take rcu_read_lock_bh() in netpoll_send_skb_on_dev() Cong Wang
` (3 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Cong Wang @ 2012-08-06 14:23 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, David S. Miller
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 5d881c3..2d178ba 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 6/8] netpoll: take rcu_read_lock_bh() in netpoll_send_skb_on_dev()
2012-08-06 14:23 [PATCH v2 0/8] some netpoll and netconsole fixes Cong Wang
` (4 preceding siblings ...)
2012-08-06 14:23 ` [PATCH 5/8] netpoll: use netpoll_rx_on() " Cong Wang
@ 2012-08-06 14:23 ` Cong Wang
2012-08-06 14:23 ` [PATCH 7/8] bridge: add some comments for NETDEV_RELEASE Cong Wang
` (2 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Cong Wang @ 2012-08-06 14:23 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, David S. Miller
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 d055bb0..174346a 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 7/8] bridge: add some comments for NETDEV_RELEASE
2012-08-06 14:23 [PATCH v2 0/8] some netpoll and netconsole fixes Cong Wang
` (5 preceding siblings ...)
2012-08-06 14:23 ` [PATCH 6/8] netpoll: take rcu_read_lock_bh() in netpoll_send_skb_on_dev() Cong Wang
@ 2012-08-06 14:23 ` Cong Wang
2012-08-06 20:50 ` David Miller
2012-08-06 14:23 ` [PATCH 8/8] bridge: use list_for_each_entry() in netpoll functions Cong Wang
2012-08-07 6:58 ` [PATCH v2 0/8] some netpoll and netconsole fixes Cong Wang
8 siblings, 1 reply; 21+ messages in thread
From: Cong Wang @ 2012-08-06 14:23 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Stephen Hemminger
Add comments on why we don't notify NETDEV_RELEASE.
Cc: Stephen Hemminger <shemminger@vyatta.com>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
net/bridge/br_if.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 171fd6b..910f4aa 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -427,6 +427,10 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
if (!p || p->br != br)
return -EINVAL;
+ /*
+ * We don't notify NETDEV_RELEASE event, as this will
+ * stop netconsole on the bridge.
+ */
del_nbp(p);
spin_lock_bh(&br->lock);
--
1.7.7.6
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 7/8] bridge: add some comments for NETDEV_RELEASE
2012-08-06 14:23 ` [PATCH 7/8] bridge: add some comments for NETDEV_RELEASE Cong Wang
@ 2012-08-06 20:50 ` David Miller
2012-08-06 23:39 ` Joe Perches
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: David Miller @ 2012-08-06 20:50 UTC (permalink / raw)
To: amwang; +Cc: netdev, shemminger
From: Cong Wang <amwang@redhat.com>
Date: Mon, 6 Aug 2012 22:23:31 +0800
> + /*
> + * We don't notify NETDEV_RELEASE event, as this will
> + * stop netconsole on the bridge.
> + */
Please format comments in the networking:
/* Like
* this.
*/
Thanks.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/8] bridge: add some comments for NETDEV_RELEASE
2012-08-06 20:50 ` David Miller
@ 2012-08-06 23:39 ` Joe Perches
2012-08-10 10:59 ` Jan Engelhardt
2012-08-07 2:27 ` Cong Wang
2012-08-07 5:59 ` [Resend PATCH " Cong Wang
2 siblings, 1 reply; 21+ messages in thread
From: Joe Perches @ 2012-08-06 23:39 UTC (permalink / raw)
To: David Miller; +Cc: amwang, netdev, shemminger
On Mon, 2012-08-06 at 13:50 -0700, David Miller wrote:
> From: Cong Wang <amwang@redhat.com>
> Date: Mon, 6 Aug 2012 22:23:31 +0800
>
> > + /*
> > + * We don't notify NETDEV_RELEASE event, as this will
> > + * stop netconsole on the bridge.
> > + */
>
> Please format comments in the networking:
>
> /* Like
> * this.
> */
Maybe add a networking specific block comment style
to checkpatch. This doesn't trigger on files outside
of drivers/net and net.
---
scripts/checkpatch.pl | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 913d6bd..560f012 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1873,6 +1873,13 @@ sub process {
"No space is necessary after a cast\n" . $hereprev);
}
+ if ($realfile =~ m@^(drivers/net/|net/)@ &&
+ $rawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
+ $prevrawline =~ /^\+[ \t]*$/) {
+ WARN("NETWORKING_BLOCK_COMMENT_STYLE",
+ "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
+ }
+
# check for spaces at the beginning of a line.
# Exceptions:
# 1) within comments
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 7/8] bridge: add some comments for NETDEV_RELEASE
2012-08-06 23:39 ` Joe Perches
@ 2012-08-10 10:59 ` Jan Engelhardt
2012-08-10 11:13 ` Joe Perches
0 siblings, 1 reply; 21+ messages in thread
From: Jan Engelhardt @ 2012-08-10 10:59 UTC (permalink / raw)
To: Joe Perches; +Cc: David Miller, amwang, netdev, shemminger
On Tuesday 2012-08-07 01:39, Joe Perches wrote:
>On Mon, 2012-08-06 at 13:50 -0700, David Miller wrote:
>> > + /*
>> > + * We don't notify NETDEV_RELEASE event, as this will
>> > + * stop netconsole on the bridge.
>> > + */
>>
>> Please format comments in the networking:
>>
>> /* Like
>> * this.
>> */
>
>Maybe add a networking specific block comment style
>to checkpatch. This doesn't trigger on files outside
>of drivers/net and net.
Is there any particular reason why net/ wants to be special?
Before we know it, coding style is like laws across US states..
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/8] bridge: add some comments for NETDEV_RELEASE
2012-08-10 10:59 ` Jan Engelhardt
@ 2012-08-10 11:13 ` Joe Perches
0 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2012-08-10 11:13 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: David Miller, amwang, netdev, shemminger
On Fri, 2012-08-10 at 12:59 +0200, Jan Engelhardt wrote:
> On Tuesday 2012-08-07 01:39, Joe Perches wrote:
> >On Mon, 2012-08-06 at 13:50 -0700, David Miller wrote:
> >> > + /*
> >> > + * We don't notify NETDEV_RELEASE event, as this will
> >> > + * stop netconsole on the bridge.
> >> > + */
> >>
> >> Please format comments in the networking:
> >>
> >> /* Like
> >> * this.
> >> */
> >
> >Maybe add a networking specific block comment style
> >to checkpatch. This doesn't trigger on files outside
> >of drivers/net and net.
>
> Is there any particular reason why net/ wants to be special?
A determined maintainer with a strong desire
for consistency using an idiomatic style.
A thread about it:
https://lkml.org/lkml/2012/4/16/443
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 7/8] bridge: add some comments for NETDEV_RELEASE
2012-08-06 20:50 ` David Miller
2012-08-06 23:39 ` Joe Perches
@ 2012-08-07 2:27 ` Cong Wang
2012-08-07 5:59 ` [Resend PATCH " Cong Wang
2 siblings, 0 replies; 21+ messages in thread
From: Cong Wang @ 2012-08-07 2:27 UTC (permalink / raw)
To: David Miller; +Cc: netdev, shemminger
On Mon, 2012-08-06 at 13:50 -0700, David Miller wrote:
> From: Cong Wang <amwang@redhat.com>
> Date: Mon, 6 Aug 2012 22:23:31 +0800
>
> > + /*
> > + * We don't notify NETDEV_RELEASE event, as this will
> > + * stop netconsole on the bridge.
> > + */
>
> Please format comments in the networking:
>
> /* Like
> * this.
> */
>
> Thanks.
Ok, I will update and resend this single patch. :)
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Resend PATCH 7/8] bridge: add some comments for NETDEV_RELEASE
2012-08-06 20:50 ` David Miller
2012-08-06 23:39 ` Joe Perches
2012-08-07 2:27 ` Cong Wang
@ 2012-08-07 5:59 ` Cong Wang
2012-08-07 7:10 ` David Miller
2 siblings, 1 reply; 21+ messages in thread
From: Cong Wang @ 2012-08-07 5:59 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, David Miller, Stephen Hemminger
Add comments on why we don't notify NETDEV_RELEASE.
Cc: David Miller <davem@davemloft.net>
Cc: Stephen Hemminger <shemminger@vyatta.com>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 171fd6b..bf47d4f 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -427,6 +427,9 @@ int br_del_if(struct net_bridge *br, struct net_device *dev)
if (!p || p->br != br)
return -EINVAL;
+ /* We don't notify NETDEV_RELEASE event, as this will
+ * stop netconsole on the bridge.
+ */
del_nbp(p);
spin_lock_bh(&br->lock);
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 8/8] bridge: use list_for_each_entry() in netpoll functions
2012-08-06 14:23 [PATCH v2 0/8] some netpoll and netconsole fixes Cong Wang
` (6 preceding siblings ...)
2012-08-06 14:23 ` [PATCH 7/8] bridge: add some comments for NETDEV_RELEASE Cong Wang
@ 2012-08-06 14:23 ` Cong Wang
2012-08-11 18:07 ` Stephen Hemminger
2012-08-07 6:58 ` [PATCH v2 0/8] some netpoll and netconsole fixes Cong Wang
8 siblings, 1 reply; 21+ messages in thread
From: Cong Wang @ 2012-08-06 14:23 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Stephen Hemminger
We don't delete 'p' from the list in the loop,
so we can just use list_for_each_entry().
Cc: Stephen Hemminger <shemminger@vyatta.com>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
net/bridge/br_device.c | 9 ++++-----
1 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index f41ba40..32211fa 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -206,21 +206,20 @@ static void br_poll_controller(struct net_device *br_dev)
static void br_netpoll_cleanup(struct net_device *dev)
{
struct net_bridge *br = netdev_priv(dev);
- struct net_bridge_port *p, *n;
+ struct net_bridge_port *p;
- list_for_each_entry_safe(p, n, &br->port_list, list) {
+ list_for_each_entry(p, &br->port_list, list)
br_netpoll_disable(p);
- }
}
static int br_netpoll_setup(struct net_device *dev, struct netpoll_info *ni,
gfp_t gfp)
{
struct net_bridge *br = netdev_priv(dev);
- struct net_bridge_port *p, *n;
+ struct net_bridge_port *p;
int err = 0;
- list_for_each_entry_safe(p, n, &br->port_list, list) {
+ list_for_each_entry(p, &br->port_list, list) {
if (!p->dev)
continue;
err = br_netpoll_enable(p, gfp);
--
1.7.7.6
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/8] some netpoll and netconsole fixes
2012-08-06 14:23 [PATCH v2 0/8] some netpoll and netconsole fixes Cong Wang
` (7 preceding siblings ...)
2012-08-06 14:23 ` [PATCH 8/8] bridge: use list_for_each_entry() in netpoll functions Cong Wang
@ 2012-08-07 6:58 ` Cong Wang
8 siblings, 0 replies; 21+ messages in thread
From: Cong Wang @ 2012-08-07 6:58 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller
On Mon, 2012-08-06 at 22:23 +0800, Cong Wang wrote:
> V2:
> * update patch 1/8 as Eric suggested
> * drop the bridge patch, add comments instead
> * add patch 8/8
>
Hi, David,
Please do _not_ apply this patchset, I plan to add 2 more patches,
because I found another issue of netpoll.
I will send V3 soon.
Thanks!
^ permalink raw reply [flat|nested] 21+ messages in thread