* [v3 PATCH 1/2] bonding: sync netpoll code with bridge @ 2010-12-08 7:52 Amerigo Wang 2010-12-08 7:52 ` [v3 PATCH 2/2] netpoll: remove IFF_IN_NETPOLL flag Amerigo Wang 2010-12-08 13:57 ` [v3 PATCH 1/2] bonding: sync netpoll code with bridge Neil Horman 0 siblings, 2 replies; 11+ messages in thread From: Amerigo Wang @ 2010-12-08 7:52 UTC (permalink / raw) To: linux-kernel Cc: Jiri Pirko, Neil Horman, netdev, David S. Miller, Eric W. Biederman, Amerigo Wang, Herbert Xu, bonding-devel, Jay Vosburgh, Stephen Hemminger From: Amerigo Wang <amwang@redhat.com> Date: Thu, 2 Dec 2010 21:31:19 +0800 Subject: [v3 PATCH 1/2] bonding: sync netpoll code with bridge V3: remove an useless #ifdef. This patch unifies the netpoll code in bonding with netpoll code in bridge, thanks to Herbert that code is much cleaner now. Signed-off-by: WANG Cong <amwang@redhat.com> Cc: Neil Horman <nhorman@redhat.com> Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: Jay Vosburgh <fubar@us.ibm.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Stephen Hemminger <shemminger@vyatta.com> Cc: Jiri Pirko <jpirko@redhat.com> Cc: "Eric W. Biederman" <ebiederm@xmission.com> --- drivers/net/bonding/bond_main.c | 155 ++++++++++++++++++++++++-------------- drivers/net/bonding/bonding.h | 20 +++++ 2 files changed, 118 insertions(+), 57 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 0273ad0..7fafe06 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -59,7 +59,6 @@ #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> @@ -449,15 +448,11 @@ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, } skb->priority = 1; -#ifdef CONFIG_NET_POLL_CONTROLLER - if (unlikely(bond->dev->priv_flags & IFF_IN_NETPOLL)) { - struct netpoll *np = bond->dev->npinfo->netpoll; - slave_dev->npinfo = bond->dev->npinfo; + if (unlikely(netpoll_tx_running(slave_dev))) { slave_dev->priv_flags |= IFF_IN_NETPOLL; - netpoll_send_skb_on_dev(np, skb, slave_dev); + bond_netpoll_send_skb(bond_get_slave_by_dev(bond, slave_dev), skb); slave_dev->priv_flags &= ~IFF_IN_NETPOLL; } else -#endif dev_queue_xmit(skb); return 0; @@ -1310,63 +1305,113 @@ static void bond_detach_slave(struct bonding *bond, struct slave *slave) } #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) +static inline int slave_enable_netpoll(struct slave *slave) { - struct bonding *bond = netdev_priv(bond_dev); - struct slave *slave; - int i = 0; - bool ret = true; + struct netpoll *np; + int err = 0; - bond_for_each_slave(bond, slave, i) { - if ((slave->dev->priv_flags & IFF_DISABLE_NETPOLL) || - !slave->dev->netdev_ops->ndo_poll_controller) - ret = false; + np = kmalloc(sizeof(*np), GFP_KERNEL); + err = -ENOMEM; + if (!np) + goto out; + + np->dev = slave->dev; + err = __netpoll_setup(np); + if (err) { + kfree(np); + goto out; } - return i != 0 && ret; + slave->np = np; +out: + return err; +} +static inline void slave_disable_netpoll(struct slave *slave) +{ + struct netpoll *np = slave->np; + + if (!np) + return; + + slave->np = NULL; + synchronize_rcu_bh(); + __netpoll_cleanup(np); + kfree(np); +} +static inline bool slave_dev_support_netpoll(struct net_device *slave_dev) +{ + if (slave_dev->priv_flags & IFF_DISABLE_NETPOLL) + return false; + if (!slave_dev->netdev_ops->ndo_poll_controller) + return false; + return true; } static void bond_poll_controller(struct net_device *bond_dev) { - struct bonding *bond = netdev_priv(bond_dev); +} + +static void __bond_netpoll_cleanup(struct bonding *bond) +{ struct slave *slave; int i; - bond_for_each_slave(bond, slave, i) { - if (slave->dev && IS_UP(slave->dev)) - netpoll_poll_dev(slave->dev); - } + bond_for_each_slave(bond, slave, i) + if (slave->dev) + slave_disable_netpoll(slave); } - static void bond_netpoll_cleanup(struct net_device *bond_dev) { struct bonding *bond = netdev_priv(bond_dev); + + read_lock(&bond->lock); + __bond_netpoll_cleanup(bond); + read_unlock(&bond->lock); +} + +static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni) +{ + struct bonding *bond = netdev_priv(dev); struct slave *slave; - const struct net_device_ops *ops; - int i; + int i, err = 0; 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; + if (!slave->dev) + continue; + err = slave_enable_netpoll(slave); + if (err) { + __bond_netpoll_cleanup(bond); + break; } } read_unlock(&bond->lock); + return err; } -#else +static struct netpoll_info *bond_netpoll_info(struct bonding *bond) +{ + return bond->dev->npinfo; +} +#else +static inline int slave_enable_netpoll(struct slave *slave) +{ + return 0; +} +static inline void slave_disable_netpoll(struct slave *slave) +{ +} static void bond_netpoll_cleanup(struct net_device *bond_dev) { } - +static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni) +{ + return 0; +} +static struct netpoll_info *bond_netpoll_info(struct bonding *bond) +{ + return NULL; +} #endif /*---------------------------------- IOCTL ----------------------------------*/ @@ -1804,17 +1849,19 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) 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); + slave_dev->npinfo = bond_netpoll_info(bond); + if (slave_dev->npinfo) { + if (slave_enable_netpoll(new_slave)) { + read_unlock(&bond->lock); + pr_info("Error, %s: master_dev is using netpoll, " + "but new slave device does not support netpoll.\n", + bond_dev->name); + res = -EBUSY; + goto err_close; + } } #endif + read_unlock(&bond->lock); res = bond_create_slave_symlinks(bond_dev, slave_dev); @@ -2016,17 +2063,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev) 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 + slave_disable_netpoll(slave); /* close slave before restoring its mac address */ dev_close(slave_dev); @@ -2061,6 +2098,7 @@ static int bond_release_and_destroy(struct net_device *bond_dev, ret = bond_release(bond_dev, slave_dev); if ((ret == 0) && (bond->slave_cnt == 0)) { + bond_dev->priv_flags |= IFF_DISABLE_NETPOLL; pr_info("%s: destroying bond %s.\n", bond_dev->name, bond_dev->name); unregister_netdevice(bond_dev); @@ -2138,6 +2176,8 @@ static int bond_release_all(struct net_device *bond_dev) netdev_set_master(slave_dev, NULL); + slave_disable_netpoll(slave); + /* close slave before restoring its mac address */ dev_close(slave_dev); @@ -4670,6 +4710,7 @@ static const struct net_device_ops bond_netdev_ops = { .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_setup = bond_netpoll_setup, .ndo_netpoll_cleanup = bond_netpoll_cleanup, .ndo_poll_controller = bond_poll_controller, #endif diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index ad3ae46..c4f6a94 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -21,6 +21,7 @@ #include <linux/kobject.h> #include <linux/cpumask.h> #include <linux/in6.h> +#include <linux/netpoll.h> #include "bond_3ad.h" #include "bond_alb.h" @@ -203,6 +204,9 @@ struct slave { u16 queue_id; struct ad_slave_info ad_info; /* HUGE - better to dynamically alloc */ struct tlb_slave_info tlb_info; +#ifdef CONFIG_NET_POLL_CONTROLLER + struct netpoll *np; +#endif }; /* @@ -324,6 +328,22 @@ static inline unsigned long slave_last_rx(struct bonding *bond, return slave->dev->last_rx; } +#ifdef CONFIG_NET_POLL_CONTROLLER +static inline void bond_netpoll_send_skb(const struct slave *slave, + struct sk_buff *skb) +{ + struct netpoll *np = slave->np; + + if (np) + netpoll_send_skb(np, skb); +} +#else +static inline void bond_netpoll_send_skb(const struct slave *slave, + struct sk_buff *skb) +{ +} +#endif + static inline void bond_set_slave_inactive_flags(struct slave *slave) { struct bonding *bond = netdev_priv(slave->dev->master); -- 1.7.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [v3 PATCH 2/2] netpoll: remove IFF_IN_NETPOLL flag 2010-12-08 7:52 [v3 PATCH 1/2] bonding: sync netpoll code with bridge Amerigo Wang @ 2010-12-08 7:52 ` Amerigo Wang 2010-12-08 8:16 ` Changli Gao 2010-12-08 13:57 ` [v3 PATCH 1/2] bonding: sync netpoll code with bridge Neil Horman 1 sibling, 1 reply; 11+ messages in thread From: Amerigo Wang @ 2010-12-08 7:52 UTC (permalink / raw) To: linux-kernel Cc: Jiri Pirko, Neil Horman, netdev, David S. Miller, Eric W. Biederman, Amerigo Wang, Herbert Xu, bonding-devel, Jay Vosburgh, Stephen Hemminger From: Amerigo Wang <amwang@redhat.com> Date: Thu, 2 Dec 2010 21:34:44 +0800 Subject: [v3 PATCH 2/2] netpoll: remove IFF_IN_NETPOLL flag This patch removes the flag IFF_IN_NETPOLL, we don't need it any more since we have netpoll_tx_running() now. Signed-off-by: WANG Cong <amwang@redhat.com> Cc: Neil Horman <nhorman@redhat.com> Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: Jay Vosburgh <fubar@us.ibm.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Stephen Hemminger <shemminger@vyatta.com> Cc: Jiri Pirko <jpirko@redhat.com> Cc: "Eric W. Biederman" <ebiederm@xmission.com> --- drivers/net/bonding/bond_main.c | 6 ++---- drivers/net/bonding/bonding.h | 2 +- include/linux/if.h | 9 ++++----- net/core/netpoll.c | 2 -- 4 files changed, 7 insertions(+), 12 deletions(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 7fafe06..21ac08b 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -448,11 +448,9 @@ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, } skb->priority = 1; - if (unlikely(netpoll_tx_running(slave_dev))) { - slave_dev->priv_flags |= IFF_IN_NETPOLL; + if (unlikely(netpoll_tx_running(slave_dev))) bond_netpoll_send_skb(bond_get_slave_by_dev(bond, slave_dev), skb); - slave_dev->priv_flags &= ~IFF_IN_NETPOLL; - } else + else dev_queue_xmit(skb); return 0; diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h index c4f6a94..493e645 100644 --- a/drivers/net/bonding/bonding.h +++ b/drivers/net/bonding/bonding.h @@ -138,7 +138,7 @@ static inline void unblock_netpoll_tx(void) static inline int is_netpoll_tx_blocked(struct net_device *dev) { - if (unlikely(dev->priv_flags & IFF_IN_NETPOLL)) + if (unlikely(netpoll_tx_running(dev))) return cpumask_test_cpu(smp_processor_id(), netpoll_block_tx); return 0; } diff --git a/include/linux/if.h b/include/linux/if.h index 1239599..3bc63e6 100644 --- a/include/linux/if.h +++ b/include/linux/if.h @@ -71,11 +71,10 @@ * 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 IFF_MACVLAN_PORT 0x4000 /* device used as macvlan port */ -#define IFF_BRIDGE_PORT 0x8000 /* device used as bridge port */ -#define IFF_OVS_DATAPATH 0x10000 /* device used as Open vSwitch +#define IFF_DISABLE_NETPOLL 0x1000 /* disable netpoll at run-time */ +#define IFF_MACVLAN_PORT 0x2000 /* device used as macvlan port */ +#define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */ +#define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch * datapath port */ #define IF_GET_IFACE 0x0001 /* for querying only */ diff --git a/net/core/netpoll.c b/net/core/netpoll.c index ee38acb..8a5c1c9 100644 --- a/net/core/netpoll.c +++ b/net/core/netpoll.c @@ -314,9 +314,7 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb, 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); } -- 1.7.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [v3 PATCH 2/2] netpoll: remove IFF_IN_NETPOLL flag 2010-12-08 7:52 ` [v3 PATCH 2/2] netpoll: remove IFF_IN_NETPOLL flag Amerigo Wang @ 2010-12-08 8:16 ` Changli Gao 2010-12-08 8:36 ` Cong Wang 0 siblings, 1 reply; 11+ messages in thread From: Changli Gao @ 2010-12-08 8:16 UTC (permalink / raw) To: Amerigo Wang Cc: linux-kernel, Jiri Pirko, Neil Horman, netdev, David S. Miller, Eric W. Biederman, Herbert Xu, bonding-devel, Jay Vosburgh, Stephen Hemminger On Wed, Dec 8, 2010 at 3:52 PM, Amerigo Wang <amwang@redhat.com> wrote: > From: Amerigo Wang <amwang@redhat.com> > Date: Thu, 2 Dec 2010 21:34:44 +0800 > Subject: [v3 PATCH 2/2] netpoll: remove IFF_IN_NETPOLL flag > > This patch removes the flag IFF_IN_NETPOLL, we don't need it any more since > we have netpoll_tx_running() now. > > Signed-off-by: WANG Cong <amwang@redhat.com> > Cc: Neil Horman <nhorman@redhat.com> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: Jay Vosburgh <fubar@us.ibm.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Stephen Hemminger <shemminger@vyatta.com> > Cc: Jiri Pirko <jpirko@redhat.com> > Cc: "Eric W. Biederman" <ebiederm@xmission.com> > > --- > > drivers/net/bonding/bond_main.c | 6 ++---- > drivers/net/bonding/bonding.h | 2 +- > include/linux/if.h | 9 ++++----- > net/core/netpoll.c | 2 -- > 4 files changed, 7 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 7fafe06..21ac08b 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -448,11 +448,9 @@ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, > } > > skb->priority = 1; > - if (unlikely(netpoll_tx_running(slave_dev))) { > - slave_dev->priv_flags |= IFF_IN_NETPOLL; > + if (unlikely(netpoll_tx_running(slave_dev))) > bond_netpoll_send_skb(bond_get_slave_by_dev(bond, slave_dev), skb); > - slave_dev->priv_flags &= ~IFF_IN_NETPOLL; > - } else > + else > dev_queue_xmit(skb); > > return 0; > diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h > index c4f6a94..493e645 100644 > --- a/drivers/net/bonding/bonding.h > +++ b/drivers/net/bonding/bonding.h > @@ -138,7 +138,7 @@ static inline void unblock_netpoll_tx(void) > > static inline int is_netpoll_tx_blocked(struct net_device *dev) > { > - if (unlikely(dev->priv_flags & IFF_IN_NETPOLL)) > + if (unlikely(netpoll_tx_running(dev))) > return cpumask_test_cpu(smp_processor_id(), netpoll_block_tx); > return 0; > } > diff --git a/include/linux/if.h b/include/linux/if.h > index 1239599..3bc63e6 100644 > --- a/include/linux/if.h > +++ b/include/linux/if.h > @@ -71,11 +71,10 @@ > * 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 IFF_MACVLAN_PORT 0x4000 /* device used as macvlan port */ > -#define IFF_BRIDGE_PORT 0x8000 /* device used as bridge port */ > -#define IFF_OVS_DATAPATH 0x10000 /* device used as Open vSwitch > +#define IFF_DISABLE_NETPOLL 0x1000 /* disable netpoll at run-time */ > +#define IFF_MACVLAN_PORT 0x2000 /* device used as macvlan port */ > +#define IFF_BRIDGE_PORT 0x4000 /* device used as bridge port */ > +#define IFF_OVS_DATAPATH 0x8000 /* device used as Open vSwitch > * datapath port */ > You can't change the values of these macros or delete some of them, because they are exported to user space and are parts of ABI. -- Regards, Changli Gao(xiaosuo@gmail.com) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v3 PATCH 2/2] netpoll: remove IFF_IN_NETPOLL flag 2010-12-08 8:16 ` Changli Gao @ 2010-12-08 8:36 ` Cong Wang 2010-12-08 8:49 ` Changli Gao 0 siblings, 1 reply; 11+ messages in thread From: Cong Wang @ 2010-12-08 8:36 UTC (permalink / raw) To: Changli Gao Cc: linux-kernel, Jiri Pirko, Neil Horman, netdev, David S. Miller, Eric W. Biederman, Herbert Xu, bonding-devel, Jay Vosburgh, Stephen Hemminger On 12/08/10 16:16, Changli Gao wrote: > > You can't change the values of these macros or delete some of them, > because they are exported to user space and are parts of ABI. > A few lines above that code said: /* Private (from user) interface flags (netdevice->priv_flags). */ unless this comment is wrong, we don't need to worry. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v3 PATCH 2/2] netpoll: remove IFF_IN_NETPOLL flag 2010-12-08 8:36 ` Cong Wang @ 2010-12-08 8:49 ` Changli Gao 2010-12-08 8:52 ` Cong Wang 0 siblings, 1 reply; 11+ messages in thread From: Changli Gao @ 2010-12-08 8:49 UTC (permalink / raw) To: Cong Wang Cc: linux-kernel, Jiri Pirko, Neil Horman, netdev, David S. Miller, Eric W. Biederman, Herbert Xu, bonding-devel, Jay Vosburgh, Stephen Hemminger On Wed, Dec 8, 2010 at 4:36 PM, Cong Wang <amwang@redhat.com> wrote: > > A few lines above that code said: > > /* Private (from user) interface flags (netdevice->priv_flags). */ > > unless this comment is wrong, we don't need to worry. > It seems these macros are exported to user space wrongly. -- Regards, Changli Gao(xiaosuo@gmail.com) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v3 PATCH 2/2] netpoll: remove IFF_IN_NETPOLL flag 2010-12-08 8:49 ` Changli Gao @ 2010-12-08 8:52 ` Cong Wang 0 siblings, 0 replies; 11+ messages in thread From: Cong Wang @ 2010-12-08 8:52 UTC (permalink / raw) To: Changli Gao Cc: linux-kernel, Jiri Pirko, Neil Horman, netdev, David S. Miller, Eric W. Biederman, Herbert Xu, bonding-devel, Jay Vosburgh, Stephen Hemminger On 12/08/10 16:49, Changli Gao wrote: > On Wed, Dec 8, 2010 at 4:36 PM, Cong Wang<amwang@redhat.com> wrote: >> >> A few lines above that code said: >> >> /* Private (from user) interface flags (netdevice->priv_flags). */ >> >> unless this comment is wrong, we don't need to worry. >> > > It seems these macros are exported to user space wrongly. > Maybe we should add #ifdef __KERNEL__ to protect them, I think. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v3 PATCH 1/2] bonding: sync netpoll code with bridge 2010-12-08 7:52 [v3 PATCH 1/2] bonding: sync netpoll code with bridge Amerigo Wang 2010-12-08 7:52 ` [v3 PATCH 2/2] netpoll: remove IFF_IN_NETPOLL flag Amerigo Wang @ 2010-12-08 13:57 ` Neil Horman 2010-12-09 7:33 ` Cong Wang 1 sibling, 1 reply; 11+ messages in thread From: Neil Horman @ 2010-12-08 13:57 UTC (permalink / raw) To: Amerigo Wang Cc: linux-kernel, Jiri Pirko, netdev, David S. Miller, Eric W. Biederman, Herbert Xu, bonding-devel, Jay Vosburgh, Stephen Hemminger On Wed, Dec 08, 2010 at 02:52:08AM -0500, Amerigo Wang wrote: > From: Amerigo Wang <amwang@redhat.com> > Date: Thu, 2 Dec 2010 21:31:19 +0800 > Subject: [v3 PATCH 1/2] bonding: sync netpoll code with bridge > > V3: remove an useless #ifdef. > > This patch unifies the netpoll code in bonding with netpoll code in bridge, > thanks to Herbert that code is much cleaner now. > > Signed-off-by: WANG Cong <amwang@redhat.com> > Cc: Neil Horman <nhorman@redhat.com> > Cc: Herbert Xu <herbert@gondor.apana.org.au> > Cc: Jay Vosburgh <fubar@us.ibm.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Stephen Hemminger <shemminger@vyatta.com> > Cc: Jiri Pirko <jpirko@redhat.com> > Cc: "Eric W. Biederman" <ebiederm@xmission.com> > > > --- > > drivers/net/bonding/bond_main.c | 155 ++++++++++++++++++++++++-------------- > drivers/net/bonding/bonding.h | 20 +++++ > 2 files changed, 118 insertions(+), 57 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 0273ad0..7fafe06 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -59,7 +59,6 @@ > #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> > @@ -449,15 +448,11 @@ int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, > } > > skb->priority = 1; > -#ifdef CONFIG_NET_POLL_CONTROLLER > - if (unlikely(bond->dev->priv_flags & IFF_IN_NETPOLL)) { > - struct netpoll *np = bond->dev->npinfo->netpoll; > - slave_dev->npinfo = bond->dev->npinfo; > + if (unlikely(netpoll_tx_running(slave_dev))) { > slave_dev->priv_flags |= IFF_IN_NETPOLL; > - netpoll_send_skb_on_dev(np, skb, slave_dev); > + bond_netpoll_send_skb(bond_get_slave_by_dev(bond, slave_dev), skb); > slave_dev->priv_flags &= ~IFF_IN_NETPOLL; > } else > -#endif > dev_queue_xmit(skb); > > return 0; > @@ -1310,63 +1305,113 @@ static void bond_detach_slave(struct bonding *bond, struct slave *slave) > } > > #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) > +static inline int slave_enable_netpoll(struct slave *slave) > { > - struct bonding *bond = netdev_priv(bond_dev); > - struct slave *slave; > - int i = 0; > - bool ret = true; > + struct netpoll *np; > + int err = 0; > > - bond_for_each_slave(bond, slave, i) { > - if ((slave->dev->priv_flags & IFF_DISABLE_NETPOLL) || > - !slave->dev->netdev_ops->ndo_poll_controller) > - ret = false; > + np = kmalloc(sizeof(*np), GFP_KERNEL); > + err = -ENOMEM; > + if (!np) > + goto out; > + > + np->dev = slave->dev; > + err = __netpoll_setup(np); Setting up our own netpoll instance on each slave worries me a bit. The implication here is that, by doing so, some frames will get entirely processed by the slave. Most notably arp frames. That means anything that gets queued up to the arp_tx queue in __netpoll_rx will get processed during that poll event, and responded to with the mac of the slave device, rather than with the mac of the bond device, which isn't always what you want. I think if you go with this route, you'll need to add code to netpoll_poll_dev, right before the call to service_arp_queue, to check if IFF_SLAVE is set in priv_flags, and move the list to the master device, or some such. It also seems like you'll want to zero out the other fields in the netpoll structure. Leaving garbage in them will be bad. Most notably here I'm looking at the rx_hook field. If its non-null we're going to add a bogus pointer to the rx_np list and call off into space at some point. > + if (err) { > + kfree(np); > + goto out; > } > - return i != 0 && ret; > + slave->np = np; > +out: > + return err; > +} > +static inline void slave_disable_netpoll(struct slave *slave) > +{ > + struct netpoll *np = slave->np; > + > + if (!np) > + return; > + > + slave->np = NULL; > + synchronize_rcu_bh(); > + __netpoll_cleanup(np); > + kfree(np); > +} > +static inline bool slave_dev_support_netpoll(struct net_device *slave_dev) > +{ > + if (slave_dev->priv_flags & IFF_DISABLE_NETPOLL) > + return false; > + if (!slave_dev->netdev_ops->ndo_poll_controller) > + return false; > + return true; > } > > static void bond_poll_controller(struct net_device *bond_dev) > { > - struct bonding *bond = netdev_priv(bond_dev); > +} > + > +static void __bond_netpoll_cleanup(struct bonding *bond) > +{ > struct slave *slave; > int i; > > - bond_for_each_slave(bond, slave, i) { > - if (slave->dev && IS_UP(slave->dev)) > - netpoll_poll_dev(slave->dev); > - } > + bond_for_each_slave(bond, slave, i) > + if (slave->dev) Why are you checking slave->dev here? If the dev pointer has been set to NULL here it would seem we're not holding on to dev long enough. If we enabled netpoll with a dev pointer and lost it somewhere along the way, we're going to leak that struct netpoll memory that we allocated. > + slave_disable_netpoll(slave); > } > - > static void bond_netpoll_cleanup(struct net_device *bond_dev) > { > struct bonding *bond = netdev_priv(bond_dev); > + > + read_lock(&bond->lock); > + __bond_netpoll_cleanup(bond); > + read_unlock(&bond->lock); > +} > + > +static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni) > +{ > + struct bonding *bond = netdev_priv(dev); > struct slave *slave; > - const struct net_device_ops *ops; > - int i; > + int i, err = 0; > > 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; > + if (!slave->dev) > + continue; > + err = slave_enable_netpoll(slave); > + if (err) { > + __bond_netpoll_cleanup(bond); > + break; > } > } > read_unlock(&bond->lock); > + return err; > } > > -#else > +static struct netpoll_info *bond_netpoll_info(struct bonding *bond) > +{ > + return bond->dev->npinfo; > +} > > +#else > +static inline int slave_enable_netpoll(struct slave *slave) > +{ > + return 0; > +} > +static inline void slave_disable_netpoll(struct slave *slave) > +{ > +} > static void bond_netpoll_cleanup(struct net_device *bond_dev) > { > } > - > +static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni) > +{ > + return 0; > +} > +static struct netpoll_info *bond_netpoll_info(struct bonding *bond) > +{ > + return NULL; > +} > #endif > > /*---------------------------------- IOCTL ----------------------------------*/ > @@ -1804,17 +1849,19 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) > 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); > + slave_dev->npinfo = bond_netpoll_info(bond); > + if (slave_dev->npinfo) { > + if (slave_enable_netpoll(new_slave)) { > + read_unlock(&bond->lock); > + pr_info("Error, %s: master_dev is using netpoll, " > + "but new slave device does not support netpoll.\n", > + bond_dev->name); > + res = -EBUSY; > + goto err_close; > + } > } > #endif > + > read_unlock(&bond->lock); > > res = bond_create_slave_symlinks(bond_dev, slave_dev); > @@ -2016,17 +2063,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev) > > 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 > + slave_disable_netpoll(slave); > > /* close slave before restoring its mac address */ > dev_close(slave_dev); > @@ -2061,6 +2098,7 @@ static int bond_release_and_destroy(struct net_device *bond_dev, > > ret = bond_release(bond_dev, slave_dev); > if ((ret == 0) && (bond->slave_cnt == 0)) { > + bond_dev->priv_flags |= IFF_DISABLE_NETPOLL; Why are you setting IFF_DISABLE_NETPOLL here? That seems unnecessecary > pr_info("%s: destroying bond %s.\n", > bond_dev->name, bond_dev->name); > unregister_netdevice(bond_dev); > @@ -2138,6 +2176,8 @@ static int bond_release_all(struct net_device *bond_dev) > > netdev_set_master(slave_dev, NULL); > > + slave_disable_netpoll(slave); > + > /* close slave before restoring its mac address */ > dev_close(slave_dev); > > @@ -4670,6 +4710,7 @@ static const struct net_device_ops bond_netdev_ops = { > .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_setup = bond_netpoll_setup, > .ndo_netpoll_cleanup = bond_netpoll_cleanup, > .ndo_poll_controller = bond_poll_controller, > #endif > diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h > index ad3ae46..c4f6a94 100644 > --- a/drivers/net/bonding/bonding.h > +++ b/drivers/net/bonding/bonding.h > @@ -21,6 +21,7 @@ > #include <linux/kobject.h> > #include <linux/cpumask.h> > #include <linux/in6.h> > +#include <linux/netpoll.h> > #include "bond_3ad.h" > #include "bond_alb.h" > > @@ -203,6 +204,9 @@ struct slave { > u16 queue_id; > struct ad_slave_info ad_info; /* HUGE - better to dynamically alloc */ > struct tlb_slave_info tlb_info; > +#ifdef CONFIG_NET_POLL_CONTROLLER > + struct netpoll *np; > +#endif > }; > > /* > @@ -324,6 +328,22 @@ static inline unsigned long slave_last_rx(struct bonding *bond, > return slave->dev->last_rx; > } > > +#ifdef CONFIG_NET_POLL_CONTROLLER > +static inline void bond_netpoll_send_skb(const struct slave *slave, > + struct sk_buff *skb) > +{ > + struct netpoll *np = slave->np; > + > + if (np) > + netpoll_send_skb(np, skb); > +} > +#else > +static inline void bond_netpoll_send_skb(const struct slave *slave, > + struct sk_buff *skb) > +{ > +} > +#endif > + > static inline void bond_set_slave_inactive_flags(struct slave *slave) > { > struct bonding *bond = netdev_priv(slave->dev->master); > -- > 1.7.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v3 PATCH 1/2] bonding: sync netpoll code with bridge 2010-12-08 13:57 ` [v3 PATCH 1/2] bonding: sync netpoll code with bridge Neil Horman @ 2010-12-09 7:33 ` Cong Wang 2010-12-09 7:40 ` Cong Wang 2010-12-15 10:52 ` Cong Wang 0 siblings, 2 replies; 11+ messages in thread From: Cong Wang @ 2010-12-09 7:33 UTC (permalink / raw) To: Neil Horman Cc: linux-kernel, Jiri Pirko, netdev, David S. Miller, Eric W. Biederman, Herbert Xu, bonding-devel, Jay Vosburgh, Stephen Hemminger On 12/08/10 21:57, Neil Horman wrote: > On Wed, Dec 08, 2010 at 02:52:08AM -0500, Amerigo Wang wrote: >> - bond_for_each_slave(bond, slave, i) { >> - if ((slave->dev->priv_flags& IFF_DISABLE_NETPOLL) || >> - !slave->dev->netdev_ops->ndo_poll_controller) >> - ret = false; >> + np = kmalloc(sizeof(*np), GFP_KERNEL); >> + err = -ENOMEM; >> + if (!np) >> + goto out; >> + >> + np->dev = slave->dev; >> + err = __netpoll_setup(np); > Setting up our own netpoll instance on each slave worries me a bit. The > implication here is that, by doing so, some frames will get entirely processed > by the slave. Most notably arp frames. That means anything that gets queued up > to the arp_tx queue in __netpoll_rx will get processed during that poll event, > and responded to with the mac of the slave device, rather than with the mac of > the bond device, which isn't always what you want. I think if you go with this > route, you'll need to add code to netpoll_poll_dev, right before the call to > service_arp_queue, to check if IFF_SLAVE is set in priv_flags, and move the list > to the master device, or some such. Good point! Will fix it. > > It also seems like you'll want to zero out the other fields in the netpoll > structure. Leaving garbage in them will be bad. Most notably here I'm looking > at the rx_hook field. If its non-null we're going to add a bogus pointer to the > rx_np list and call off into space at some point. > Ouch! I remember I really used kzalloc() here, don't know why kmalloc() gets into the final patch. Odd, I need to double check the patch. :-/ <...> >> +static void __bond_netpoll_cleanup(struct bonding *bond) >> +{ >> struct slave *slave; >> int i; >> >> - bond_for_each_slave(bond, slave, i) { >> - if (slave->dev&& IS_UP(slave->dev)) >> - netpoll_poll_dev(slave->dev); >> - } >> + bond_for_each_slave(bond, slave, i) >> + if (slave->dev) > Why are you checking slave->dev here? If the dev pointer has been set to NULL > here it would seem we're not holding on to dev long enough. If we enabled > netpoll with a dev pointer and lost it somewhere along the way, we're going to > leak that struct netpoll memory that we allocated. > Hmm, seems you are right, read_lock should guarantee every slave on the list has the right ->dev... But I think I should keep that IS_UP() checking... <...> >> >> /* close slave before restoring its mac address */ >> dev_close(slave_dev); >> @@ -2061,6 +2098,7 @@ static int bond_release_and_destroy(struct net_device *bond_dev, >> >> ret = bond_release(bond_dev, slave_dev); >> if ((ret == 0)&& (bond->slave_cnt == 0)) { >> + bond_dev->priv_flags |= IFF_DISABLE_NETPOLL; > Why are you setting IFF_DISABLE_NETPOLL here? That seems unnecessecary > It gets removed in patch 2/2. :) Thanks for review! ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v3 PATCH 1/2] bonding: sync netpoll code with bridge 2010-12-09 7:33 ` Cong Wang @ 2010-12-09 7:40 ` Cong Wang 2010-12-15 10:52 ` Cong Wang 1 sibling, 0 replies; 11+ messages in thread From: Cong Wang @ 2010-12-09 7:40 UTC (permalink / raw) To: Neil Horman Cc: linux-kernel, Jiri Pirko, netdev, David S. Miller, Eric W. Biederman, Herbert Xu, bonding-devel, Jay Vosburgh, Stephen Hemminger On 12/09/10 15:33, Cong Wang wrote: >>> >>> /* close slave before restoring its mac address */ >>> dev_close(slave_dev); >>> @@ -2061,6 +2098,7 @@ static int bond_release_and_destroy(struct >>> net_device *bond_dev, >>> >>> ret = bond_release(bond_dev, slave_dev); >>> if ((ret == 0)&& (bond->slave_cnt == 0)) { >>> + bond_dev->priv_flags |= IFF_DISABLE_NETPOLL; >> Why are you setting IFF_DISABLE_NETPOLL here? That seems unnecessecary >> > > It gets removed in patch 2/2. :) Oops! I misread IFF_DISABLE_NETPOLL as IFF_IN_NETPOLL... I think there is a small window between bond_release() and unregister_netdevice(), setting this could prevent netpoll is setup again on this bond? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v3 PATCH 1/2] bonding: sync netpoll code with bridge 2010-12-09 7:33 ` Cong Wang 2010-12-09 7:40 ` Cong Wang @ 2010-12-15 10:52 ` Cong Wang 2010-12-15 11:59 ` Neil Horman 1 sibling, 1 reply; 11+ messages in thread From: Cong Wang @ 2010-12-15 10:52 UTC (permalink / raw) To: Neil Horman Cc: linux-kernel, Jiri Pirko, netdev, David S. Miller, Eric W. Biederman, Herbert Xu, bonding-devel, Jay Vosburgh, Stephen Hemminger 于 2010年12月09日 15:33, Cong Wang 写道: > On 12/08/10 21:57, Neil Horman wrote: >> On Wed, Dec 08, 2010 at 02:52:08AM -0500, Amerigo Wang wrote: >>> - bond_for_each_slave(bond, slave, i) { >>> - if ((slave->dev->priv_flags& IFF_DISABLE_NETPOLL) || >>> - !slave->dev->netdev_ops->ndo_poll_controller) >>> - ret = false; >>> + np = kmalloc(sizeof(*np), GFP_KERNEL); >>> + err = -ENOMEM; >>> + if (!np) >>> + goto out; >>> + >>> + np->dev = slave->dev; >>> + err = __netpoll_setup(np); >> Setting up our own netpoll instance on each slave worries me a bit. The >> implication here is that, by doing so, some frames will get entirely processed >> by the slave. Most notably arp frames. That means anything that gets queued up >> to the arp_tx queue in __netpoll_rx will get processed during that poll event, >> and responded to with the mac of the slave device, rather than with the mac of >> the bond device, which isn't always what you want. I think if you go with this >> route, you'll need to add code to netpoll_poll_dev, right before the call to >> service_arp_queue, to check if IFF_SLAVE is set in priv_flags, and move the list >> to the master device, or some such. > > > Good point! Will fix i Hi, Neil, I think we should do that in bond_poll_controller() rather than netpoll_poll_dev(), right? Since this is bond-specific. Does moving all arp_tx of slaves to their bond address your concern? Thanks! ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [v3 PATCH 1/2] bonding: sync netpoll code with bridge 2010-12-15 10:52 ` Cong Wang @ 2010-12-15 11:59 ` Neil Horman 0 siblings, 0 replies; 11+ messages in thread From: Neil Horman @ 2010-12-15 11:59 UTC (permalink / raw) To: Cong Wang Cc: Neil Horman, linux-kernel, Jiri Pirko, netdev, David S. Miller, Eric W. Biederman, Herbert Xu, bonding-devel, Jay Vosburgh, Stephen Hemminger On Wed, Dec 15, 2010 at 06:52:26PM +0800, Cong Wang wrote: > 于 2010年12月09日 15:33, Cong Wang 写道: > >On 12/08/10 21:57, Neil Horman wrote: > >>On Wed, Dec 08, 2010 at 02:52:08AM -0500, Amerigo Wang wrote: > >>>- bond_for_each_slave(bond, slave, i) { > >>>- if ((slave->dev->priv_flags& IFF_DISABLE_NETPOLL) || > >>>- !slave->dev->netdev_ops->ndo_poll_controller) > >>>- ret = false; > >>>+ np = kmalloc(sizeof(*np), GFP_KERNEL); > >>>+ err = -ENOMEM; > >>>+ if (!np) > >>>+ goto out; > >>>+ > >>>+ np->dev = slave->dev; > >>>+ err = __netpoll_setup(np); > >>Setting up our own netpoll instance on each slave worries me a bit. The > >>implication here is that, by doing so, some frames will get entirely processed > >>by the slave. Most notably arp frames. That means anything that gets queued up > >>to the arp_tx queue in __netpoll_rx will get processed during that poll event, > >>and responded to with the mac of the slave device, rather than with the mac of > >>the bond device, which isn't always what you want. I think if you go with this > >>route, you'll need to add code to netpoll_poll_dev, right before the call to > >>service_arp_queue, to check if IFF_SLAVE is set in priv_flags, and move the list > >>to the master device, or some such. > > > > > >Good point! Will fix i > > Hi, Neil, > > I think we should do that in bond_poll_controller() rather than netpoll_poll_dev(), > right? Since this is bond-specific. Does moving all arp_tx of slaves to their bond > address your concern? > Don't think that will work, because, since your using multiple netpoll instances here, the slaves arp_tx queue will get serviced on the return from the slaves netpoll_poll_dev call, prior to returning to the bond_poll_controller call. In other words, bond_poll_controller won't ever see any frames on the slaves arp_tx queue to migrate, since they will have already been responded to by the slave directly. Neil > Thanks! > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-12-15 11:59 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-12-08 7:52 [v3 PATCH 1/2] bonding: sync netpoll code with bridge Amerigo Wang 2010-12-08 7:52 ` [v3 PATCH 2/2] netpoll: remove IFF_IN_NETPOLL flag Amerigo Wang 2010-12-08 8:16 ` Changli Gao 2010-12-08 8:36 ` Cong Wang 2010-12-08 8:49 ` Changli Gao 2010-12-08 8:52 ` Cong Wang 2010-12-08 13:57 ` [v3 PATCH 1/2] bonding: sync netpoll code with bridge Neil Horman 2010-12-09 7:33 ` Cong Wang 2010-12-09 7:40 ` Cong Wang 2010-12-15 10:52 ` Cong Wang 2010-12-15 11:59 ` Neil Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).