V3:
Update to latest Linus' tree.
Fix deadlocks when releasing slaves of bonding devices.
Thanks to Andy.
V2:
Fix some bugs of previous version.
Remove ->netpoll_setup and ->netpoll_xmit, they are not necessary.
Don't poll all underlying devices, poll ->real_dev in struct netpoll.
Thanks to David for suggesting above.
--------->
This whole patchset is for adding netpoll support to bridge and bonding
devices. I already tested it for bridge, bonding, bridge over bonding,
and bonding over bridge. It looks fine now.
Please comment.
To make bridge and bonding support netpoll, we need to adjust
some netpoll generic code. This patch does the following things:
1) introduce two new priv_flags for struct net_device:
IFF_IN_NETPOLL which identifies we are processing a netpoll;
IFF_DISABLE_NETPOLL is used to disable netpoll support for a device
at run-time;
2) introduce one new method for netdev_ops:
->ndo_netpoll_cleanup() is used to clean up netpoll when a device is
removed.
3) introduce netpoll_poll_dev() which takes a struct net_device * parameter;
export netpoll_send_skb() and netpoll_poll_dev() which will be used later;
4) hide a pointer to struct netpoll in struct netpoll_info, ditto.
5) introduce ->real_dev for struct netpoll.
6) introduce a new status NETDEV_BONDING_DESLAE, which is used to disable
netconsole before releasing a slave, to avoid deadlocks.
Cc: David Miller <[email protected]>
Cc: Neil Horman <[email protected]>
Signed-off-by: WANG Cong <[email protected]>
---
Index: linux-2.6/include/linux/if.h
===================================================================
--- linux-2.6.orig/include/linux/if.h
+++ linux-2.6/include/linux/if.h
@@ -71,6 +71,8 @@
* release skb->dst
*/
#define IFF_DONT_BRIDGE 0x800 /* disallow bridging this ether dev */
+#define IFF_IN_NETPOLL 0x1000 /* whether we are processing netpoll */
+#define IFF_DISABLE_NETPOLL 0x2000 /* disable netpoll at run-time */
#define IF_GET_IFACE 0x0001 /* for querying only */
#define IF_GET_PROTO 0x0002
Index: linux-2.6/include/linux/netdevice.h
===================================================================
--- linux-2.6.orig/include/linux/netdevice.h
+++ linux-2.6/include/linux/netdevice.h
@@ -667,6 +667,7 @@ struct net_device_ops {
unsigned short vid);
#ifdef CONFIG_NET_POLL_CONTROLLER
void (*ndo_poll_controller)(struct net_device *dev);
+ void (*ndo_netpoll_cleanup)(struct net_device *dev);
#endif
int (*ndo_set_vf_mac)(struct net_device *dev,
int queue, u8 *mac);
Index: linux-2.6/include/linux/netpoll.h
===================================================================
--- linux-2.6.orig/include/linux/netpoll.h
+++ linux-2.6/include/linux/netpoll.h
@@ -14,6 +14,7 @@
struct netpoll {
struct net_device *dev;
+ struct net_device *real_dev;
char dev_name[IFNAMSIZ];
const char *name;
void (*rx_hook)(struct netpoll *, int, char *, int);
@@ -36,8 +37,11 @@ struct netpoll_info {
struct sk_buff_head txq;
struct delayed_work tx_work;
+
+ struct netpoll *netpoll;
};
+void netpoll_poll_dev(struct net_device *dev);
void netpoll_poll(struct netpoll *np);
void netpoll_send_udp(struct netpoll *np, const char *msg, int len);
void netpoll_print_options(struct netpoll *np);
@@ -47,6 +51,7 @@ int netpoll_trap(void);
void netpoll_set_trap(int trap);
void netpoll_cleanup(struct netpoll *np);
int __netpoll_rx(struct sk_buff *skb);
+void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb);
#ifdef CONFIG_NETPOLL
Index: linux-2.6/net/core/netpoll.c
===================================================================
--- linux-2.6.orig/net/core/netpoll.c
+++ linux-2.6/net/core/netpoll.c
@@ -179,9 +179,8 @@ static void service_arp_queue(struct net
}
}
-void netpoll_poll(struct netpoll *np)
+void netpoll_poll_dev(struct net_device *dev)
{
- struct net_device *dev = np->dev;
const struct net_device_ops *ops;
if (!dev || !netif_running(dev))
@@ -201,6 +200,11 @@ void netpoll_poll(struct netpoll *np)
zap_completion_queue();
}
+void netpoll_poll(struct netpoll *np)
+{
+ netpoll_poll_dev(np->dev);
+}
+
static void refill_skbs(void)
{
struct sk_buff *skb;
@@ -282,7 +286,7 @@ static int netpoll_owner_active(struct n
return 0;
}
-static void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
+void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
{
int status = NETDEV_TX_BUSY;
unsigned long tries;
@@ -308,7 +312,9 @@ static void netpoll_send_skb(struct netp
tries > 0; --tries) {
if (__netif_tx_trylock(txq)) {
if (!netif_tx_queue_stopped(txq)) {
+ dev->priv_flags |= IFF_IN_NETPOLL;
status = ops->ndo_start_xmit(skb, dev);
+ dev->priv_flags &= ~IFF_IN_NETPOLL;
if (status == NETDEV_TX_OK)
txq_trans_update(txq);
}
@@ -756,7 +762,10 @@ int netpoll_setup(struct netpoll *np)
atomic_inc(&npinfo->refcnt);
}
- if (!ndev->netdev_ops->ndo_poll_controller) {
+ npinfo->netpoll = np;
+
+ if (ndev->priv_flags & IFF_DISABLE_NETPOLL
+ || !ndev->netdev_ops->ndo_poll_controller) {
printk(KERN_ERR "%s: %s doesn't support polling, aborting.\n",
np->name, np->dev_name);
err = -ENOTSUPP;
@@ -878,6 +887,7 @@ void netpoll_cleanup(struct netpoll *np)
}
if (atomic_dec_and_test(&npinfo->refcnt)) {
+ const struct net_device_ops *ops;
skb_queue_purge(&npinfo->arp_tx);
skb_queue_purge(&npinfo->txq);
cancel_rearming_delayed_work(&npinfo->tx_work);
@@ -885,7 +895,11 @@ void netpoll_cleanup(struct netpoll *np)
/* clean after last, unfinished work */
__skb_queue_purge(&npinfo->txq);
kfree(npinfo);
- np->dev->npinfo = NULL;
+ ops = np->dev->netdev_ops;
+ if (ops->ndo_netpoll_cleanup)
+ ops->ndo_netpoll_cleanup(np->dev);
+ else
+ np->dev->npinfo = NULL;
}
}
@@ -908,6 +922,7 @@ void netpoll_set_trap(int trap)
atomic_dec(&trapped);
}
+EXPORT_SYMBOL(netpoll_send_skb);
EXPORT_SYMBOL(netpoll_set_trap);
EXPORT_SYMBOL(netpoll_trap);
EXPORT_SYMBOL(netpoll_print_options);
@@ -915,4 +930,5 @@ EXPORT_SYMBOL(netpoll_parse_options);
EXPORT_SYMBOL(netpoll_setup);
EXPORT_SYMBOL(netpoll_cleanup);
EXPORT_SYMBOL(netpoll_send_udp);
+EXPORT_SYMBOL(netpoll_poll_dev);
EXPORT_SYMBOL(netpoll_poll);
Index: linux-2.6/drivers/net/netconsole.c
===================================================================
--- linux-2.6.orig/drivers/net/netconsole.c
+++ linux-2.6/drivers/net/netconsole.c
@@ -665,7 +665,8 @@ static int netconsole_netdev_event(struc
struct netconsole_target *nt;
struct net_device *dev = ptr;
- if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER))
+ if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
+ event == NETDEV_BONDING_DESLAVE))
goto done;
spin_lock_irqsave(&target_list_lock, flags);
@@ -677,19 +678,20 @@ static int netconsole_netdev_event(struc
strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ);
break;
case NETDEV_UNREGISTER:
- if (!nt->enabled)
- break;
netpoll_cleanup(&nt->np);
+ /* Fall through */
+ case NETDEV_BONDING_DESLAVE:
nt->enabled = 0;
- printk(KERN_INFO "netconsole: network logging stopped"
- ", interface %s unregistered\n",
- dev->name);
break;
}
}
netconsole_target_put(nt);
}
spin_unlock_irqrestore(&target_list_lock, flags);
+ if (event == NETDEV_UNREGISTER || event == NETDEV_BONDING_DESLAVE)
+ printk(KERN_INFO "netconsole: network logging stopped, "
+ "interface %s %s\n", dev->name,
+ event == NETDEV_UNREGISTER ? "unregistered" : "released slaves");
done:
return NOTIFY_DONE;
Index: linux-2.6/include/linux/notifier.h
===================================================================
--- linux-2.6.orig/include/linux/notifier.h
+++ linux-2.6/include/linux/notifier.h
@@ -203,6 +203,7 @@ static inline int notifier_to_errno(int
#define NETDEV_BONDING_NEWTYPE 0x000F
#define NETDEV_POST_INIT 0x0010
#define NETDEV_UNREGISTER_BATCH 0x0011
+#define NETDEV_BONDING_DESLAVE 0x0012
#define SYS_DOWN 0x0001 /* Notify of system down */
#define SYS_RESTART SYS_DOWN
Based on Andy's work, but I modified a lot.
Similar to the patch for bridge, this patch does:
1) implement the 2 methods to support netpoll for bonding;
2) modify netpoll during forwarding packets via bonding;
3) disable netpoll support of bonding when a netpoll-unabled device
is added to bonding;
4) enable netpoll support when all underlying devices support netpoll.
Cc: Andy Gospodarek <[email protected]>
Cc: Jeff Moyer <[email protected]>
Cc: Matt Mackall <[email protected]>
Cc: Neil Horman <[email protected]>
Cc: Jay Vosburgh <[email protected]>
Cc: David Miller <[email protected]>
Signed-off-by: WANG Cong <[email protected]>
---
Index: linux-2.6/drivers/net/bonding/bond_main.c
===================================================================
--- linux-2.6.orig/drivers/net/bonding/bond_main.c
+++ linux-2.6/drivers/net/bonding/bond_main.c
@@ -59,6 +59,7 @@
#include <linux/uaccess.h>
#include <linux/errno.h>
#include <linux/netdevice.h>
+#include <linux/netpoll.h>
#include <linux/inetdevice.h>
#include <linux/igmp.h>
#include <linux/etherdevice.h>
@@ -430,7 +431,18 @@ int bond_dev_queue_xmit(struct bonding *
}
skb->priority = 1;
- dev_queue_xmit(skb);
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ if (bond->dev->priv_flags & IFF_IN_NETPOLL) {
+ struct netpoll *np = bond->dev->npinfo->netpoll;
+ slave_dev->npinfo = bond->dev->npinfo;
+ np->real_dev = np->dev = skb->dev;
+ slave_dev->priv_flags |= IFF_IN_NETPOLL;
+ netpoll_send_skb(np, skb);
+ slave_dev->priv_flags &= ~IFF_IN_NETPOLL;
+ np->dev = bond->dev;
+ } else
+#endif
+ dev_queue_xmit(skb);
return 0;
}
@@ -1329,6 +1341,61 @@ static void bond_detach_slave(struct bon
bond->slave_cnt--;
}
+#ifdef CONFIG_NET_POLL_CONTROLLER
+/*
+ * You must hold read lock on bond->lock before calling this.
+ */
+static bool slaves_support_netpoll(struct net_device *bond_dev)
+{
+ struct bonding *bond = netdev_priv(bond_dev);
+ struct slave *slave;
+ int i = 0;
+ bool ret = true;
+
+ bond_for_each_slave(bond, slave, i) {
+ if ((slave->dev->priv_flags & IFF_DISABLE_NETPOLL)
+ || !slave->dev->netdev_ops->ndo_poll_controller)
+ ret = false;
+ }
+ return i != 0 && ret;
+}
+
+static void bond_poll_controller(struct net_device *bond_dev)
+{
+ struct net_device *dev = bond_dev->npinfo->netpoll->real_dev;
+ if (dev != bond_dev)
+ netpoll_poll_dev(dev);
+}
+
+static void bond_netpoll_cleanup(struct net_device *bond_dev)
+{
+ struct bonding *bond = netdev_priv(bond_dev);
+ struct slave *slave;
+ const struct net_device_ops *ops;
+ int i;
+
+ read_lock(&bond->lock);
+ bond_dev->npinfo = NULL;
+ bond_for_each_slave(bond, slave, i) {
+ if (slave->dev) {
+ ops = slave->dev->netdev_ops;
+ if (ops->ndo_netpoll_cleanup)
+ ops->ndo_netpoll_cleanup(slave->dev);
+ else
+ slave->dev->npinfo = NULL;
+ }
+ }
+ read_unlock(&bond->lock);
+}
+
+#else
+
+static void bond_netpoll_cleanup(struct net_device *bond_dev)
+{
+}
+
+#endif
+
/*---------------------------------- IOCTL ----------------------------------*/
static int bond_sethwaddr(struct net_device *bond_dev,
@@ -1735,6 +1802,18 @@ int bond_enslave(struct net_device *bond
bond_set_carrier(bond);
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ if (slaves_support_netpoll(bond_dev)) {
+ bond_dev->priv_flags &= ~IFF_DISABLE_NETPOLL;
+ if (bond_dev->npinfo)
+ slave_dev->npinfo = bond_dev->npinfo;
+ } else if (!(bond_dev->priv_flags & IFF_DISABLE_NETPOLL)) {
+ bond_dev->priv_flags |= IFF_DISABLE_NETPOLL;
+ pr_info("New slave device %s does not support netpoll\n",
+ slave_dev->name);
+ pr_info("Disabling netpoll support for %s\n", bond_dev->name);
+ }
+#endif
read_unlock(&bond->lock);
res = bond_create_slave_symlinks(bond_dev, slave_dev);
@@ -1801,6 +1880,7 @@ int bond_release(struct net_device *bond
return -EINVAL;
}
+ netdev_bonding_change(bond_dev, NETDEV_BONDING_DESLAVE);
write_lock_bh(&bond->lock);
slave = bond_get_slave_by_dev(bond, slave_dev);
@@ -1929,6 +2009,17 @@ int bond_release(struct net_device *bond
netdev_set_master(slave_dev, NULL);
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ read_lock_bh(&bond->lock);
+ if (slaves_support_netpoll(bond_dev))
+ bond_dev->priv_flags &= ~IFF_DISABLE_NETPOLL;
+ read_unlock_bh(&bond->lock);
+ if (slave_dev->netdev_ops->ndo_netpoll_cleanup)
+ slave_dev->netdev_ops->ndo_netpoll_cleanup(slave_dev);
+ else
+ slave_dev->npinfo = NULL;
+#endif
+
/* close slave before restoring its mac address */
dev_close(slave_dev);
@@ -4448,6 +4539,10 @@ static const struct net_device_ops bond_
.ndo_vlan_rx_register = bond_vlan_rx_register,
.ndo_vlan_rx_add_vid = bond_vlan_rx_add_vid,
.ndo_vlan_rx_kill_vid = bond_vlan_rx_kill_vid,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ .ndo_netpoll_cleanup = bond_netpoll_cleanup,
+ .ndo_poll_controller = bond_poll_controller,
+#endif
};
static void bond_destructor(struct net_device *bond_dev)
@@ -4541,6 +4636,8 @@ static void bond_uninit(struct net_devic
{
struct bonding *bond = netdev_priv(bond_dev);
+ bond_netpoll_cleanup(bond_dev);
+
/* Release the bonded slaves */
bond_release_all(bond_dev);
Based on the previous patch, make bridge support netpoll by:
1) implement the 2 methods to support netpoll for bridge;
2) modify netpoll during forwarding packets via bridge;
3) disable netpoll support of bridge when a netpoll-unabled device
is added to bridge;
4) enable netpoll support when all underlying devices support netpoll.
Cc: David Miller <[email protected]>
Cc: Neil Horman <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Cc: Matt Mackall <[email protected]>
Signed-off-by: WANG Cong <[email protected]>
---
Index: linux-2.6/net/bridge/br_device.c
===================================================================
--- linux-2.6.orig/net/bridge/br_device.c
+++ linux-2.6/net/bridge/br_device.c
@@ -13,8 +13,10 @@
#include <linux/kernel.h>
#include <linux/netdevice.h>
+#include <linux/netpoll.h>
#include <linux/etherdevice.h>
#include <linux/ethtool.h>
+#include <linux/list.h>
#include <asm/uaccess.h>
#include "br_private.h"
@@ -162,6 +164,59 @@ static int br_set_tx_csum(struct net_dev
return 0;
}
+#ifdef CONFIG_NET_POLL_CONTROLLER
+bool br_devices_support_netpoll(struct net_bridge *br)
+{
+ struct net_bridge_port *p;
+ bool ret = true;
+ int count = 0;
+ unsigned long flags;
+
+ spin_lock_irqsave(&br->lock, flags);
+ list_for_each_entry(p, &br->port_list, list) {
+ count++;
+ if (p->dev->priv_flags & IFF_DISABLE_NETPOLL
+ || !p->dev->netdev_ops->ndo_poll_controller)
+ ret = false;
+ }
+ spin_unlock_irqrestore(&br->lock, flags);
+ return count != 0 && ret;
+}
+
+static void br_poll_controller(struct net_device *br_dev)
+{
+ struct netpoll *np = br_dev->npinfo->netpoll;
+
+ if (np->real_dev != br_dev)
+ netpoll_poll_dev(np->real_dev);
+}
+
+void br_netpoll_cleanup(struct net_device *br_dev)
+{
+ struct net_bridge *br = netdev_priv(br_dev);
+ struct net_bridge_port *p, *n;
+ const struct net_device_ops *ops;
+
+ br->dev->npinfo = NULL;
+ list_for_each_entry_safe(p, n, &br->port_list, list) {
+ if (p->dev) {
+ ops = p->dev->netdev_ops;
+ if (ops->ndo_netpoll_cleanup)
+ ops->ndo_netpoll_cleanup(p->dev);
+ else
+ p->dev->npinfo = NULL;
+ }
+ }
+}
+
+#else
+
+void br_netpoll_cleanup(struct net_device *br_dev)
+{
+}
+
+#endif
+
static const struct ethtool_ops br_ethtool_ops = {
.get_drvinfo = br_getinfo,
.get_link = ethtool_op_get_link,
@@ -184,6 +239,10 @@ static const struct net_device_ops br_ne
.ndo_set_multicast_list = br_dev_set_multicast_list,
.ndo_change_mtu = br_change_mtu,
.ndo_do_ioctl = br_dev_ioctl,
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ .ndo_netpoll_cleanup = br_netpoll_cleanup,
+ .ndo_poll_controller = br_poll_controller,
+#endif
};
void br_dev_setup(struct net_device *dev)
Index: linux-2.6/net/bridge/br_forward.c
===================================================================
--- linux-2.6.orig/net/bridge/br_forward.c
+++ linux-2.6/net/bridge/br_forward.c
@@ -15,6 +15,7 @@
#include <linux/slab.h>
#include <linux/kernel.h>
#include <linux/netdevice.h>
+#include <linux/netpoll.h>
#include <linux/skbuff.h>
#include <linux/if_vlan.h>
#include <linux/netfilter_bridge.h>
@@ -50,7 +51,13 @@ int br_dev_queue_push_xmit(struct sk_buf
else {
skb_push(skb, ETH_HLEN);
- dev_queue_xmit(skb);
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ if (skb->dev->priv_flags & IFF_IN_NETPOLL) {
+ netpoll_send_skb(skb->dev->npinfo->netpoll, skb);
+ skb->dev->priv_flags &= ~IFF_IN_NETPOLL;
+ } else
+#endif
+ dev_queue_xmit(skb);
}
}
@@ -66,9 +73,23 @@ int br_forward_finish(struct sk_buff *sk
static void __br_deliver(const struct net_bridge_port *to, struct sk_buff *skb)
{
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ struct net_bridge *br = to->br;
+ if (br->dev->priv_flags & IFF_IN_NETPOLL) {
+ struct netpoll *np;
+ to->dev->npinfo = skb->dev->npinfo;
+ np = skb->dev->npinfo->netpoll;
+ np->real_dev = np->dev = to->dev;
+ to->dev->priv_flags |= IFF_IN_NETPOLL;
+ }
+#endif
skb->dev = to->dev;
NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_OUT, skb, NULL, skb->dev,
br_forward_finish);
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ if (skb->dev->npinfo)
+ skb->dev->npinfo->netpoll->dev = br->dev;
+#endif
}
static void __br_forward(const struct net_bridge_port *to, struct sk_buff *skb)
Index: linux-2.6/net/bridge/br_if.c
===================================================================
--- linux-2.6.orig/net/bridge/br_if.c
+++ linux-2.6/net/bridge/br_if.c
@@ -13,6 +13,7 @@
#include <linux/kernel.h>
#include <linux/netdevice.h>
+#include <linux/netpoll.h>
#include <linux/ethtool.h>
#include <linux/if_arp.h>
#include <linux/module.h>
@@ -153,6 +154,14 @@ static void del_nbp(struct net_bridge_po
kobject_uevent(&p->kobj, KOBJ_REMOVE);
kobject_del(&p->kobj);
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ if (br_devices_support_netpoll(br))
+ br->dev->priv_flags &= ~IFF_DISABLE_NETPOLL;
+ if (dev->netdev_ops->ndo_netpoll_cleanup)
+ dev->netdev_ops->ndo_netpoll_cleanup(dev);
+ else
+ dev->npinfo = NULL;
+#endif
call_rcu(&p->rcu, destroy_nbp_rcu);
}
@@ -165,6 +174,8 @@ static void del_br(struct net_bridge *br
del_nbp(p);
}
+ br_netpoll_cleanup(br->dev);
+
del_timer_sync(&br->gc_timer);
br_sysfs_delbr(br->dev);
@@ -438,6 +449,20 @@ int br_add_if(struct net_bridge *br, str
kobject_uevent(&p->kobj, KOBJ_ADD);
+#ifdef CONFIG_NET_POLL_CONTROLLER
+ if (br_devices_support_netpoll(br)) {
+ br->dev->priv_flags &= ~IFF_DISABLE_NETPOLL;
+ if (br->dev->npinfo)
+ dev->npinfo = br->dev->npinfo;
+ } else if (!(br->dev->priv_flags & IFF_DISABLE_NETPOLL)) {
+ br->dev->priv_flags |= IFF_DISABLE_NETPOLL;
+ printk(KERN_INFO "New device %s does not support netpoll\n",
+ dev->name);
+ printk(KERN_INFO "Disabling netpoll for %s\n",
+ br->dev->name);
+ }
+#endif
+
return 0;
err2:
br_fdb_delete_by_port(br, p, 1);
Index: linux-2.6/net/bridge/br_private.h
===================================================================
--- linux-2.6.orig/net/bridge/br_private.h
+++ linux-2.6/net/bridge/br_private.h
@@ -233,6 +233,8 @@ static inline int br_is_root_bridge(cons
extern void br_dev_setup(struct net_device *dev);
extern netdev_tx_t br_dev_xmit(struct sk_buff *skb,
struct net_device *dev);
+extern bool br_devices_support_netpoll(struct net_bridge *br);
+extern void br_netpoll_cleanup(struct net_device *br_dev);
/* br_fdb.c */
extern int br_fdb_init(void);
On Thu, 8 Apr 2010 02:18:58 -0400
Amerigo Wang <[email protected]> wrote:
>
> Based on the previous patch, make bridge support netpoll by:
>
> 1) implement the 2 methods to support netpoll for bridge;
>
> 2) modify netpoll during forwarding packets via bridge;
>
> 3) disable netpoll support of bridge when a netpoll-unabled device
> is added to bridge;
>
> 4) enable netpoll support when all underlying devices support netpoll.
>
> Cc: David Miller <[email protected]>
> Cc: Neil Horman <[email protected]>
> Cc: Stephen Hemminger <[email protected]>
> Cc: Matt Mackall <[email protected]>
> Signed-off-by: WANG Cong <[email protected]>
>
> ---
>
> Index: linux-2.6/net/bridge/br_device.c
> ===================================================================
> --- linux-2.6.orig/net/bridge/br_device.c
> +++ linux-2.6/net/bridge/br_device.c
> @@ -13,8 +13,10 @@
>
> #include <linux/kernel.h>
> #include <linux/netdevice.h>
> +#include <linux/netpoll.h>
> #include <linux/etherdevice.h>
> #include <linux/ethtool.h>
> +#include <linux/list.h>
>
> #include <asm/uaccess.h>
> #include "br_private.h"
> @@ -162,6 +164,59 @@ static int br_set_tx_csum(struct net_dev
> return 0;
> }
>
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> +bool br_devices_support_netpoll(struct net_bridge *br)
> +{
> + struct net_bridge_port *p;
> + bool ret = true;
> + int count = 0;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&br->lock, flags);
> + list_for_each_entry(p, &br->port_list, list) {
> + count++;
> + if (p->dev->priv_flags & IFF_DISABLE_NETPOLL
> + || !p->dev->netdev_ops->ndo_poll_controller)
> + ret = false;
> + }
> + spin_unlock_irqrestore(&br->lock, flags);
> + return count != 0 && ret;
> +}
> +
> +static void br_poll_controller(struct net_device *br_dev)
> +{
> + struct netpoll *np = br_dev->npinfo->netpoll;
> +
> + if (np->real_dev != br_dev)
> + netpoll_poll_dev(np->real_dev);
> +}
> +
> +void br_netpoll_cleanup(struct net_device *br_dev)
> +{
> + struct net_bridge *br = netdev_priv(br_dev);
> + struct net_bridge_port *p, *n;
> + const struct net_device_ops *ops;
> +
> + br->dev->npinfo = NULL;
> + list_for_each_entry_safe(p, n, &br->port_list, list) {
> + if (p->dev) {
> + ops = p->dev->netdev_ops;
> + if (ops->ndo_netpoll_cleanup)
> + ops->ndo_netpoll_cleanup(p->dev);
> + else
> + p->dev->npinfo = NULL;
> + }
> + }
> +}
> +
> +#else
> +
> +void br_netpoll_cleanup(struct net_device *br_dev)
> +{
> +}
> +
> +#endif
Could you use more stub functions to eliminate #ifdef's in code.
> static const struct ethtool_ops br_ethtool_ops = {
> .get_drvinfo = br_getinfo,
> .get_link = ethtool_op_get_link,
> @@ -184,6 +239,10 @@ static const struct net_device_ops br_ne
> .ndo_set_multicast_list = br_dev_set_multicast_list,
> .ndo_change_mtu = br_change_mtu,
> .ndo_do_ioctl = br_dev_ioctl,
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> + .ndo_netpoll_cleanup = br_netpoll_cleanup,
> + .ndo_poll_controller = br_poll_controller,
> +#endif
> };
>
> void br_dev_setup(struct net_device *dev)
> Index: linux-2.6/net/bridge/br_forward.c
> ===================================================================
> --- linux-2.6.orig/net/bridge/br_forward.c
> +++ linux-2.6/net/bridge/br_forward.c
> @@ -15,6 +15,7 @@
> #include <linux/slab.h>
> #include <linux/kernel.h>
> #include <linux/netdevice.h>
> +#include <linux/netpoll.h>
> #include <linux/skbuff.h>
> #include <linux/if_vlan.h>
> #include <linux/netfilter_bridge.h>
> @@ -50,7 +51,13 @@ int br_dev_queue_push_xmit(struct sk_buf
> else {
> skb_push(skb, ETH_HLEN);
>
> - dev_queue_xmit(skb);
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> + if (skb->dev->priv_flags & IFF_IN_NETPOLL) {
> + netpoll_send_skb(skb->dev->npinfo->netpoll, skb);
> + skb->dev->priv_flags &= ~IFF_IN_NETPOLL;
> + } else
> +#endif
There is no protection on dev->priv_flags for SMP access.
It would better bit value in dev->state if you are using it as control flag.
Then you could use
if (unlikely(test_and_clear_bit(__IN_NETPOLL, &skb->dev->state)))
netpoll_send_skb(...)
> + dev_queue_xmit(skb);
> }
> }
>
> @@ -66,9 +73,23 @@ int br_forward_finish(struct sk_buff *sk
>
> static void __br_deliver(const struct net_bridge_port *to, struct sk_buff *skb)
> {
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> + struct net_bridge *br = to->br;
> + if (br->dev->priv_flags & IFF_IN_NETPOLL) {
> + struct netpoll *np;
> + to->dev->npinfo = skb->dev->npinfo;
> + np = skb->dev->npinfo->netpoll;
> + np->real_dev = np->dev = to->dev;
> + to->dev->priv_flags |= IFF_IN_NETPOLL;
> + }
> +#endif
This is n hot path, so use unlikely()
> skb->dev = to->dev;
> NF_HOOK(PF_BRIDGE, NF_BR_LOCAL_OUT, skb, NULL, skb->dev,
> br_forward_finish);
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> + if (skb->dev->npinfo)
> + skb->dev->npinfo->netpoll->dev = br->dev;
> +#endif
> }
>
> static void __br_forward(const struct net_bridge_port *to, struct sk_buff *skb)
> Index: linux-2.6/net/bridge/br_if.c
> ===================================================================
> --- linux-2.6.orig/net/bridge/br_if.c
> +++ linux-2.6/net/bridge/br_if.c
> @@ -13,6 +13,7 @@
>
> #include <linux/kernel.h>
> #include <linux/netdevice.h>
> +#include <linux/netpoll.h>
> #include <linux/ethtool.h>
> #include <linux/if_arp.h>
> #include <linux/module.h>
> @@ -153,6 +154,14 @@ static void del_nbp(struct net_bridge_po
> kobject_uevent(&p->kobj, KOBJ_REMOVE);
> kobject_del(&p->kobj);
>
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> + if (br_devices_support_netpoll(br))
> + br->dev->priv_flags &= ~IFF_DISABLE_NETPOLL;
> + if (dev->netdev_ops->ndo_netpoll_cleanup)
> + dev->netdev_ops->ndo_netpoll_cleanup(dev);
> + else
> + dev->npinfo = NULL;
> +#endif
> call_rcu(&p->rcu, destroy_nbp_rcu);
> }
>
> @@ -165,6 +174,8 @@ static void del_br(struct net_bridge *br
> del_nbp(p);
> }
>
> + br_netpoll_cleanup(br->dev);
> +
> del_timer_sync(&br->gc_timer);
>
> br_sysfs_delbr(br->dev);
> @@ -438,6 +449,20 @@ int br_add_if(struct net_bridge *br, str
>
> kobject_uevent(&p->kobj, KOBJ_ADD);
>
> +#ifdef CONFIG_NET_POLL_CONTROLLER
> + if (br_devices_support_netpoll(br)) {
> + br->dev->priv_flags &= ~IFF_DISABLE_NETPOLL;
> + if (br->dev->npinfo)
> + dev->npinfo = br->dev->npinfo;
> + } else if (!(br->dev->priv_flags & IFF_DISABLE_NETPOLL)) {
> + br->dev->priv_flags |= IFF_DISABLE_NETPOLL;
> + printk(KERN_INFO "New device %s does not support netpoll\n",
> + dev->name);
> + printk(KERN_INFO "Disabling netpoll for %s\n",
> + br->dev->name);
One message is sufficient.
--
Stephen Hemminger wrote:
> On Thu, 8 Apr 2010 02:18:58 -0400
> Amerigo Wang <[email protected]> wrote:
>
>> Based on the previous patch, make bridge support netpoll by:
>>
>> 1) implement the 2 methods to support netpoll for bridge;
>>
>> 2) modify netpoll during forwarding packets via bridge;
>>
>> 3) disable netpoll support of bridge when a netpoll-unabled device
>> is added to bridge;
>>
>> 4) enable netpoll support when all underlying devices support netpoll.
>>
>> Cc: David Miller <[email protected]>
>> Cc: Neil Horman <[email protected]>
>> Cc: Stephen Hemminger <[email protected]>
>> Cc: Matt Mackall <[email protected]>
>> Signed-off-by: WANG Cong <[email protected]>
>>
>> ---
>>
>> Index: linux-2.6/net/bridge/br_device.c
>> ===================================================================
>> --- linux-2.6.orig/net/bridge/br_device.c
>> +++ linux-2.6/net/bridge/br_device.c
>> @@ -13,8 +13,10 @@
>>
>> #include <linux/kernel.h>
>> #include <linux/netdevice.h>
>> +#include <linux/netpoll.h>
>> #include <linux/etherdevice.h>
>> #include <linux/ethtool.h>
>> +#include <linux/list.h>
>>
>> #include <asm/uaccess.h>
>> #include "br_private.h"
>> @@ -162,6 +164,59 @@ static int br_set_tx_csum(struct net_dev
>> return 0;
>> }
>>
>> +#ifdef CONFIG_NET_POLL_CONTROLLER
>> +bool br_devices_support_netpoll(struct net_bridge *br)
>> +{
>> + struct net_bridge_port *p;
>> + bool ret = true;
>> + int count = 0;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&br->lock, flags);
>> + list_for_each_entry(p, &br->port_list, list) {
>> + count++;
>> + if (p->dev->priv_flags & IFF_DISABLE_NETPOLL
>> + || !p->dev->netdev_ops->ndo_poll_controller)
>> + ret = false;
>> + }
>> + spin_unlock_irqrestore(&br->lock, flags);
>> + return count != 0 && ret;
>> +}
>> +
>> +static void br_poll_controller(struct net_device *br_dev)
>> +{
>> + struct netpoll *np = br_dev->npinfo->netpoll;
>> +
>> + if (np->real_dev != br_dev)
>> + netpoll_poll_dev(np->real_dev);
>> +}
>> +
>> +void br_netpoll_cleanup(struct net_device *br_dev)
>> +{
>> + struct net_bridge *br = netdev_priv(br_dev);
>> + struct net_bridge_port *p, *n;
>> + const struct net_device_ops *ops;
>> +
>> + br->dev->npinfo = NULL;
>> + list_for_each_entry_safe(p, n, &br->port_list, list) {
>> + if (p->dev) {
>> + ops = p->dev->netdev_ops;
>> + if (ops->ndo_netpoll_cleanup)
>> + ops->ndo_netpoll_cleanup(p->dev);
>> + else
>> + p->dev->npinfo = NULL;
>> + }
>> + }
>> +}
>> +
>> +#else
>> +
>> +void br_netpoll_cleanup(struct net_device *br_dev)
>> +{
>> +}
>> +
>> +#endif
>
> Could you use more stub functions to eliminate #ifdef's in code.
Probably no, because only br_netpoll_cleanup() will be called
no matter if CONFIG_NET_POLL_CONTROLLER is defined.
>> @@ -50,7 +51,13 @@ int br_dev_queue_push_xmit(struct sk_buf
>> else {
>> skb_push(skb, ETH_HLEN);
>>
>> - dev_queue_xmit(skb);
>> +#ifdef CONFIG_NET_POLL_CONTROLLER
>> + if (skb->dev->priv_flags & IFF_IN_NETPOLL) {
>> + netpoll_send_skb(skb->dev->npinfo->netpoll, skb);
>> + skb->dev->priv_flags &= ~IFF_IN_NETPOLL;
>> + } else
>> +#endif
>
> There is no protection on dev->priv_flags for SMP access.
> It would better bit value in dev->state if you are using it as control flag.
>
> Then you could use
> if (unlikely(test_and_clear_bit(__IN_NETPOLL, &skb->dev->state)))
> netpoll_send_skb(...)
>
Yes? netpoll_send_skb() needs to see IFF_IN_NETPOLL is set, so
we can't clear this bit before calling it.
But we do need a find a safe way to check/set this flag.
>> static void __br_deliver(const struct net_bridge_port *to, struct sk_buff *skb)
>> {
>> +#ifdef CONFIG_NET_POLL_CONTROLLER
>> + struct net_bridge *br = to->br;
>> + if (br->dev->priv_flags & IFF_IN_NETPOLL) {
>> + struct netpoll *np;
>> + to->dev->npinfo = skb->dev->npinfo;
>> + np = skb->dev->npinfo->netpoll;
>> + np->real_dev = np->dev = to->dev;
>> + to->dev->priv_flags |= IFF_IN_NETPOLL;
>> + }
>> +#endif
>
> This is n hot path, so use unlikely()
Ok, good point.
>> +#ifdef CONFIG_NET_POLL_CONTROLLER
>> + if (br_devices_support_netpoll(br)) {
>> + br->dev->priv_flags &= ~IFF_DISABLE_NETPOLL;
>> + if (br->dev->npinfo)
>> + dev->npinfo = br->dev->npinfo;
>> + } else if (!(br->dev->priv_flags & IFF_DISABLE_NETPOLL)) {
>> + br->dev->priv_flags |= IFF_DISABLE_NETPOLL;
>> + printk(KERN_INFO "New device %s does not support netpoll\n",
>> + dev->name);
>> + printk(KERN_INFO "Disabling netpoll for %s\n",
>> + br->dev->name);
>
> One message is sufficient.
>
Yes? The first messages explains the reason for the second message.
Thanks.
Stephen Hemminger wrote:
>> Index: linux-2.6/net/bridge/br_forward.c
>> ===================================================================
>> --- linux-2.6.orig/net/bridge/br_forward.c
>> +++ linux-2.6/net/bridge/br_forward.c
>> @@ -15,6 +15,7 @@
>> #include <linux/slab.h>
>> #include <linux/kernel.h>
>> #include <linux/netdevice.h>
>> +#include <linux/netpoll.h>
>> #include <linux/skbuff.h>
>> #include <linux/if_vlan.h>
>> #include <linux/netfilter_bridge.h>
>> @@ -50,7 +51,13 @@ int br_dev_queue_push_xmit(struct sk_buf
>> else {
>> skb_push(skb, ETH_HLEN);
>>
>> - dev_queue_xmit(skb);
>> +#ifdef CONFIG_NET_POLL_CONTROLLER
>> + if (skb->dev->priv_flags & IFF_IN_NETPOLL) {
>> + netpoll_send_skb(skb->dev->npinfo->netpoll, skb);
>> + skb->dev->priv_flags &= ~IFF_IN_NETPOLL;
>> + } else
>> +#endif
>
> There is no protection on dev->priv_flags for SMP access.
> It would better bit value in dev->state if you are using it as control flag.
>
> Then you could use
> if (unlikely(test_and_clear_bit(__IN_NETPOLL, &skb->dev->state)))
> netpoll_send_skb(...)
>
>
Hmm, I think we can't use ->state here, it is not for this kind of purpose,
according to its comments.
Also, I find other usages of IFF_XXX flags of ->priv_flags are also using
&, | to set or clear the flags. So there must be some other things preventing
the race...
Thanks.
Le lundi 12 avril 2010 à 18:37 +0800, Cong Wang a écrit :
> Stephen Hemminger wrote:
> > There is no protection on dev->priv_flags for SMP access.
> > It would better bit value in dev->state if you are using it as control flag.
> >
> > Then you could use
> > if (unlikely(test_and_clear_bit(__IN_NETPOLL, &skb->dev->state)))
> > netpoll_send_skb(...)
> >
> >
>
> Hmm, I think we can't use ->state here, it is not for this kind of purpose,
> according to its comments.
>
> Also, I find other usages of IFF_XXX flags of ->priv_flags are also using
> &, | to set or clear the flags. So there must be some other things preventing
> the race...
Yes, its RTNL that protects priv_flags changes, hopefully...
On Mon, 12 Apr 2010 12:38:57 +0200
Eric Dumazet <[email protected]> wrote:
> Le lundi 12 avril 2010 à 18:37 +0800, Cong Wang a écrit :
> > Stephen Hemminger wrote:
> > > There is no protection on dev->priv_flags for SMP access.
> > > It would better bit value in dev->state if you are using it as control flag.
> > >
> > > Then you could use
> > > if (unlikely(test_and_clear_bit(__IN_NETPOLL, &skb->dev->state)))
> > > netpoll_send_skb(...)
> > >
> > >
> >
> > Hmm, I think we can't use ->state here, it is not for this kind of purpose,
> > according to its comments.
> >
> > Also, I find other usages of IFF_XXX flags of ->priv_flags are also using
> > &, | to set or clear the flags. So there must be some other things preventing
> > the race...
>
> Yes, its RTNL that protects priv_flags changes, hopefully...
>
The patch was not protecting priv_flags with RTNL.
For example..
@@ -308,7 +312,9 @@ static void netpoll_send_skb(struct netp
tries > 0; --tries) {
if (__netif_tx_trylock(txq)) {
if (!netif_tx_queue_stopped(txq)) {
+ dev->priv_flags |= IFF_IN_NETPOLL;
status = ops->ndo_start_xmit(skb, dev);
+ dev->priv_flags &= ~IFF_IN_NETPOLL;
if (status == NETDEV_TX_OK)
txq_trans_update(txq);
Stephen Hemminger wrote:
> On Mon, 12 Apr 2010 12:38:57 +0200
> Eric Dumazet <[email protected]> wrote:
>
>> Le lundi 12 avril 2010 à 18:37 +0800, Cong Wang a écrit :
>>> Stephen Hemminger wrote:
>>>> There is no protection on dev->priv_flags for SMP access.
>>>> It would better bit value in dev->state if you are using it as control flag.
>>>>
>>>> Then you could use
>>>> if (unlikely(test_and_clear_bit(__IN_NETPOLL, &skb->dev->state)))
>>>> netpoll_send_skb(...)
>>>>
>>>>
>>> Hmm, I think we can't use ->state here, it is not for this kind of purpose,
>>> according to its comments.
>>>
>>> Also, I find other usages of IFF_XXX flags of ->priv_flags are also using
>>> &, | to set or clear the flags. So there must be some other things preventing
>>> the race...
>> Yes, its RTNL that protects priv_flags changes, hopefully...
>>
>
> The patch was not protecting priv_flags with RTNL.
> For example..
>
>
> @@ -308,7 +312,9 @@ static void netpoll_send_skb(struct netp
> tries > 0; --tries) {
> if (__netif_tx_trylock(txq)) {
> if (!netif_tx_queue_stopped(txq)) {
> + dev->priv_flags |= IFF_IN_NETPOLL;
> status = ops->ndo_start_xmit(skb, dev);
> + dev->priv_flags &= ~IFF_IN_NETPOLL;
> if (status == NETDEV_TX_OK)
> txq_trans_update(txq);
Hmm, but I checked the bonding case (IFF_BONDING), it doesn't
hold rtnl_lock. Strange.
Cong Wang <[email protected]> wrote:
>Stephen Hemminger wrote:
>> On Mon, 12 Apr 2010 12:38:57 +0200
>> Eric Dumazet <[email protected]> wrote:
>>
>>> Le lundi 12 avril 2010 à 18:37 +0800, Cong Wang a écrit :
>>>> Stephen Hemminger wrote:
>>>>> There is no protection on dev->priv_flags for SMP access.
>>>>> It would better bit value in dev->state if you are using it as control flag.
>>>>>
>>>>> Then you could use
>>>>> if (unlikely(test_and_clear_bit(__IN_NETPOLL, &skb->dev->state)))
>>>>> netpoll_send_skb(...)
>>>>>
>>>>>
>>>> Hmm, I think we can't use ->state here, it is not for this kind of purpose,
>>>> according to its comments.
>>>>
>>>> Also, I find other usages of IFF_XXX flags of ->priv_flags are also using
>>>> &, | to set or clear the flags. So there must be some other things preventing
>>>> the race...
>>> Yes, its RTNL that protects priv_flags changes, hopefully...
>>>
>>
>> The patch was not protecting priv_flags with RTNL.
>> For example..
>>
>>
>> @@ -308,7 +312,9 @@ static void netpoll_send_skb(struct netp
>> tries > 0; --tries) {
>> if (__netif_tx_trylock(txq)) {
>> if (!netif_tx_queue_stopped(txq)) {
>> + dev->priv_flags |= IFF_IN_NETPOLL;
>> status = ops->ndo_start_xmit(skb, dev);
>> + dev->priv_flags &= ~IFF_IN_NETPOLL;
>> if (status == NETDEV_TX_OK)
>> txq_trans_update(txq);
>
>Hmm, but I checked the bonding case (IFF_BONDING), it doesn't
>hold rtnl_lock. Strange.
I looked, and there are a couple of cases in bonding that don't
have RTNL for adjusting priv_flags (in bond_ab_arp_probe when no slaves
are up, and a couple of cases in 802.3ad). I think the solution there
is to move bonding away from priv_flags for some of this (e.g., convert
bonding to use a frame hook like bridge and macvlan, and greatly
simplify skb_bond_should_drop), but that's a separate topic.
The majority of the cases, however, do hold RTNL. Bonding
generally doesn't have to acquire RTNL itself, since whatever called
into bonding is holding it already. For example, the slave add and
remove paths (bond_enslave, bond_release) are called either via sysfs or
ioctl, both of which acquire RTNL. All of the set and clear operations
for IFF_BONDING fall into this category; look at bonding_store_slaves
for an example.
Bonding does acquire RTNL itself when performing failovers,
e.g., bond_mii_monitor holds RTNL prior to calling bond_miimon_commit,
which will change priv_flags.
-J
---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]
On Tue, 13 Apr 2010 09:52:47 -0700
Jay Vosburgh <[email protected]> wrote:
> Cong Wang <[email protected]> wrote:
>
> >Stephen Hemminger wrote:
> >> On Mon, 12 Apr 2010 12:38:57 +0200
> >> Eric Dumazet <[email protected]> wrote:
> >>
> >>> Le lundi 12 avril 2010 à 18:37 +0800, Cong Wang a écrit :
> >>>> Stephen Hemminger wrote:
> >>>>> There is no protection on dev->priv_flags for SMP access.
> >>>>> It would better bit value in dev->state if you are using it as control flag.
> >>>>>
> >>>>> Then you could use
> >>>>> if (unlikely(test_and_clear_bit(__IN_NETPOLL, &skb->dev->state)))
> >>>>> netpoll_send_skb(...)
> >>>>>
> >>>>>
> >>>> Hmm, I think we can't use ->state here, it is not for this kind of purpose,
> >>>> according to its comments.
> >>>>
> >>>> Also, I find other usages of IFF_XXX flags of ->priv_flags are also using
> >>>> &, | to set or clear the flags. So there must be some other things preventing
> >>>> the race...
> >>> Yes, its RTNL that protects priv_flags changes, hopefully...
> >>>
> >>
> >> The patch was not protecting priv_flags with RTNL.
> >> For example..
> >>
> >>
> >> @@ -308,7 +312,9 @@ static void netpoll_send_skb(struct netp
> >> tries > 0; --tries) {
> >> if (__netif_tx_trylock(txq)) {
> >> if (!netif_tx_queue_stopped(txq)) {
> >> + dev->priv_flags |= IFF_IN_NETPOLL;
> >> status = ops->ndo_start_xmit(skb, dev);
> >> + dev->priv_flags &= ~IFF_IN_NETPOLL;
> >> if (status == NETDEV_TX_OK)
> >> txq_trans_update(txq);
> >
> >Hmm, but I checked the bonding case (IFF_BONDING), it doesn't
> >hold rtnl_lock. Strange.
>
> I looked, and there are a couple of cases in bonding that don't
> have RTNL for adjusting priv_flags (in bond_ab_arp_probe when no slaves
> are up, and a couple of cases in 802.3ad). I think the solution there
> is to move bonding away from priv_flags for some of this (e.g., convert
> bonding to use a frame hook like bridge and macvlan, and greatly
> simplify skb_bond_should_drop), but that's a separate topic.
>
> The majority of the cases, however, do hold RTNL. Bonding
> generally doesn't have to acquire RTNL itself, since whatever called
> into bonding is holding it already. For example, the slave add and
> remove paths (bond_enslave, bond_release) are called either via sysfs or
> ioctl, both of which acquire RTNL. All of the set and clear operations
> for IFF_BONDING fall into this category; look at bonding_store_slaves
> for an example.
>
> Bonding does acquire RTNL itself when performing failovers,
> e.g., bond_mii_monitor holds RTNL prior to calling bond_miimon_commit,
> which will change priv_flags.
>
All this was related to netpoll. And netpoll processing often needs to occur
in hard IRQ context. Therefor netpoll stuff and RTNL (which is a mutex),
really don't mix well. Keep RTNL for what it was meant for network
reconfiguration. Don't turn it into a network special BKL.
--
Jay Vosburgh wrote:
> Cong Wang <[email protected]> wrote:
>
>> Stephen Hemminger wrote:
>>> On Mon, 12 Apr 2010 12:38:57 +0200
>>> Eric Dumazet <[email protected]> wrote:
>>>
>>>> Le lundi 12 avril 2010 à 18:37 +0800, Cong Wang a écrit :
>>>>> Stephen Hemminger wrote:
>>>>>> There is no protection on dev->priv_flags for SMP access.
>>>>>> It would better bit value in dev->state if you are using it as control flag.
>>>>>>
>>>>>> Then you could use
>>>>>> if (unlikely(test_and_clear_bit(__IN_NETPOLL, &skb->dev->state)))
>>>>>> netpoll_send_skb(...)
>>>>>>
>>>>>>
>>>>> Hmm, I think we can't use ->state here, it is not for this kind of purpose,
>>>>> according to its comments.
>>>>>
>>>>> Also, I find other usages of IFF_XXX flags of ->priv_flags are also using
>>>>> &, | to set or clear the flags. So there must be some other things preventing
>>>>> the race...
>>>> Yes, its RTNL that protects priv_flags changes, hopefully...
>>>>
>>> The patch was not protecting priv_flags with RTNL.
>>> For example..
>>>
>>>
>>> @@ -308,7 +312,9 @@ static void netpoll_send_skb(struct netp
>>> tries > 0; --tries) {
>>> if (__netif_tx_trylock(txq)) {
>>> if (!netif_tx_queue_stopped(txq)) {
>>> + dev->priv_flags |= IFF_IN_NETPOLL;
>>> status = ops->ndo_start_xmit(skb, dev);
>>> + dev->priv_flags &= ~IFF_IN_NETPOLL;
>>> if (status == NETDEV_TX_OK)
>>> txq_trans_update(txq);
>> Hmm, but I checked the bonding case (IFF_BONDING), it doesn't
>> hold rtnl_lock. Strange.
>
> I looked, and there are a couple of cases in bonding that don't
> have RTNL for adjusting priv_flags (in bond_ab_arp_probe when no slaves
> are up, and a couple of cases in 802.3ad). I think the solution there
> is to move bonding away from priv_flags for some of this (e.g., convert
> bonding to use a frame hook like bridge and macvlan, and greatly
> simplify skb_bond_should_drop), but that's a separate topic.
>
> The majority of the cases, however, do hold RTNL. Bonding
> generally doesn't have to acquire RTNL itself, since whatever called
> into bonding is holding it already. For example, the slave add and
> remove paths (bond_enslave, bond_release) are called either via sysfs or
> ioctl, both of which acquire RTNL. All of the set and clear operations
> for IFF_BONDING fall into this category; look at bonding_store_slaves
> for an example.
>
> Bonding does acquire RTNL itself when performing failovers,
> e.g., bond_mii_monitor holds RTNL prior to calling bond_miimon_commit,
> which will change priv_flags.
>
Thanks a lot for your reply!
You are right, I missed something.
Hmm, for bonding, RTNL lock is necessary because there are sysfs
interface and ioctl interface to change its configuration.
Stephen Hemminger wrote:
> On Tue, 13 Apr 2010 09:52:47 -0700
> Jay Vosburgh <[email protected]> wrote:
>
>> Cong Wang <[email protected]> wrote:
>>
>>> Stephen Hemminger wrote:
>>>> On Mon, 12 Apr 2010 12:38:57 +0200
>>>> Eric Dumazet <[email protected]> wrote:
>>>>
>>>>> Le lundi 12 avril 2010 à 18:37 +0800, Cong Wang a écrit :
>>>>>> Stephen Hemminger wrote:
>>>>>>> There is no protection on dev->priv_flags for SMP access.
>>>>>>> It would better bit value in dev->state if you are using it as control flag.
>>>>>>>
>>>>>>> Then you could use
>>>>>>> if (unlikely(test_and_clear_bit(__IN_NETPOLL, &skb->dev->state)))
>>>>>>> netpoll_send_skb(...)
>>>>>>>
>>>>>>>
>>>>>> Hmm, I think we can't use ->state here, it is not for this kind of purpose,
>>>>>> according to its comments.
>>>>>>
>>>>>> Also, I find other usages of IFF_XXX flags of ->priv_flags are also using
>>>>>> &, | to set or clear the flags. So there must be some other things preventing
>>>>>> the race...
>>>>> Yes, its RTNL that protects priv_flags changes, hopefully...
>>>>>
>>>> The patch was not protecting priv_flags with RTNL.
>>>> For example..
>>>>
>>>>
>>>> @@ -308,7 +312,9 @@ static void netpoll_send_skb(struct netp
>>>> tries > 0; --tries) {
>>>> if (__netif_tx_trylock(txq)) {
>>>> if (!netif_tx_queue_stopped(txq)) {
>>>> + dev->priv_flags |= IFF_IN_NETPOLL;
>>>> status = ops->ndo_start_xmit(skb, dev);
>>>> + dev->priv_flags &= ~IFF_IN_NETPOLL;
>>>> if (status == NETDEV_TX_OK)
>>>> txq_trans_update(txq);
>>> Hmm, but I checked the bonding case (IFF_BONDING), it doesn't
>>> hold rtnl_lock. Strange.
>> I looked, and there are a couple of cases in bonding that don't
>> have RTNL for adjusting priv_flags (in bond_ab_arp_probe when no slaves
>> are up, and a couple of cases in 802.3ad). I think the solution there
>> is to move bonding away from priv_flags for some of this (e.g., convert
>> bonding to use a frame hook like bridge and macvlan, and greatly
>> simplify skb_bond_should_drop), but that's a separate topic.
>>
>> The majority of the cases, however, do hold RTNL. Bonding
>> generally doesn't have to acquire RTNL itself, since whatever called
>> into bonding is holding it already. For example, the slave add and
>> remove paths (bond_enslave, bond_release) are called either via sysfs or
>> ioctl, both of which acquire RTNL. All of the set and clear operations
>> for IFF_BONDING fall into this category; look at bonding_store_slaves
>> for an example.
>>
>> Bonding does acquire RTNL itself when performing failovers,
>> e.g., bond_mii_monitor holds RTNL prior to calling bond_miimon_commit,
>> which will change priv_flags.
>>
>
> All this was related to netpoll. And netpoll processing often needs to occur
> in hard IRQ context. Therefor netpoll stuff and RTNL (which is a mutex),
> really don't mix well. Keep RTNL for what it was meant for network
> reconfiguration. Don't turn it into a network special BKL.
>
Hmm, I think for my patch, holding RTNL lock is not necessary,
because there're no other call pathes to change IFF_IN_NETPOLL bit,
which is unlike bonding or bridge cases where sysfs/ioctl is provided
to change it.
The only chance to change IFF_IN_NETPOLL is in netpoll_send_skb()
which can't be called simultaneously because there are other locks
protecting it.
Or am I still missing something?
Thanks.