bridge: make bridge support netpoll
in [Kernel]
|
Prev: bonding: make bonding support netpoll
Next: [PATCH] kbuild: Include gen_initramfs_list.sh and the file list in the .d file
From: David Miller on 23 Mar 2010 01:00 From: Cong Wang <amwang(a)redhat.com> Date: Tue, 23 Mar 2010 12:39:35 +0800 > How could you let the bridge know netpoll is not sent to > the one that doesn't support netpoll during setup? This will > be complex, I am afraid. Why does this matter at all? I told you in another mail that we should do away with these callbacks and all the crazy 'npinfo' assignments and just do it in the generic code. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Cong Wang on 23 Mar 2010 01:00 Matt Mackall wrote: > On Tue, 2010-03-23 at 12:39 +0800, Cong Wang wrote: >> Matt Mackall wrote: >>> On Tue, 2010-03-23 at 10:03 +0800, Cong Wang wrote: >>>> Matt Mackall wrote: >>>>> On Mon, 2010-03-22 at 04:17 -0400, Amerigo Wang wrote: >>>>>> Based on the previous patch, make bridge support netpoll by: >>>>>> >>>>>> 1) implement the 4 methods to support netpoll for bridge; >>>>>> >>>>>> 2) modify netpoll during forwarding packets in bridge; >>>>>> >>>>>> 3) disable netpoll support of bridge when a netpoll-unabled device >>>>>> is added to bridge; >>>>> Not sure if this is the right thing to do. Shouldn't we simply enable >>>>> polling on all devices that support it and warn about the others (aka >>>>> best effort)? >>>>> >>>> I don't think it's a good idea, because we check if a device >>>> supports netpoll by checking if it has ndo_poll_controller method. >>> Uh, what? If we have 5 devices on a bridge and 4 support netpoll, then >>> shouldn't we just send netconsole messages to those 4 devices? Isn't >>> this much better than simply refusing to work? >>> >> How could you let the bridge know netpoll is not sent to >> the one that doesn't support netpoll during setup? This will >> be complex, I am afraid. > > I thought I saw a simple loop over bridge devices at poll time in your > patch. So it should be a simple matter of skipping unsupported devices > in that loop. Nope, we need to check if the target address is owned by a device that doesn't support netpoll or not, simple skipping will not work. > > But Dave thinks there a bigger problems here, so I recommend first > figuring out the architecture issues, then we can get back to the policy > issues. > Ok. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Cong Wang on 23 Mar 2010 01:10 David Miller wrote: > From: Cong Wang <amwang(a)redhat.com> > Date: Tue, 23 Mar 2010 12:39:35 +0800 > >> How could you let the bridge know netpoll is not sent to >> the one that doesn't support netpoll during setup? This will >> be complex, I am afraid. > > Why does this matter at all? Because currently we check netpoll support by ->ndo_poll_controller, for example, tap driver doesn't have ->ndo_poll_controller now, if I choose the target "@192.168.0.2/br0" where "192.168.0.2" is owned by "tap0" which is managed by "br0", netconsole may not work. > > I told you in another mail that we should do away with > these callbacks and all the crazy 'npinfo' assignments > and just do it in the generic code. I think ->ndo_poll_controller is not in the case that you talked about. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Stephen Hemminger on 8 Apr 2010 11:40 On Thu, 8 Apr 2010 02:18:58 -0400 Amerigo Wang <amwang(a)redhat.com> 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 <davem(a)davemloft.net> > Cc: Neil Horman <nhorman(a)tuxdriver.com> > Cc: Stephen Hemminger <shemminger(a)linux-foundation.org> > Cc: Matt Mackall <mpm(a)selenic.com> > Signed-off-by: WANG Cong <amwang(a)redhat.com> > > --- > > 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. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Cong Wang on 9 Apr 2010 01:50
Stephen Hemminger wrote: > On Thu, 8 Apr 2010 02:18:58 -0400 > Amerigo Wang <amwang(a)redhat.com> 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 <davem(a)davemloft.net> >> Cc: Neil Horman <nhorman(a)tuxdriver.com> >> Cc: Stephen Hemminger <shemminger(a)linux-foundation.org> >> Cc: Matt Mackall <mpm(a)selenic.com> >> Signed-off-by: WANG Cong <amwang(a)redhat.com> >> >> --- >> >> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |