Re: [v3 Patch 2/3] bridge: make bridge support netpoll

[Cong Wang]

Stephen Hemminger wrote:
> On Thu, 8 Apr 2010 02:18:58 -0400
> Amerigo Wang <amwang@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > Cc: Neil Horman <nhorman@xxxxxxxxxxxxx>
> > Cc: Stephen Hemminger <shemminger@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Matt Mackall <mpm@xxxxxxxxxxx>
> > Signed-off-by: WANG Cong <amwang@xxxxxxxxxx>
> >
> > ---
> >
> > 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/