Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754457Ab0DIFkl (ORCPT ); Fri, 9 Apr 2010 01:40:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:64858 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754147Ab0DIFki (ORCPT ); Fri, 9 Apr 2010 01:40:38 -0400 Message-ID: <4BBEBE93.2030507@redhat.com> Date: Fri, 09 Apr 2010 13:43:47 +0800 From: Cong Wang User-Agent: Thunderbird 2.0.0.23 (X11/20091001) MIME-Version: 1.0 To: Stephen Hemminger CC: linux-kernel@vger.kernel.org, netdev@vger.kernel.org, bridge@lists.linux-foundation.org, Andy Gospodarek , Neil Horman , Jeff Moyer , Matt Mackall , bonding-devel@lists.sourceforge.net, Jay Vosburgh , David Miller Subject: Re: [v3 Patch 2/3] bridge: make bridge support netpoll References: <20100408062234.4499.17042.sendpatchset@localhost.localdomain> <20100408062246.4499.5670.sendpatchset@localhost.localdomain> <20100408083710.2b61ee44@nehalam> In-Reply-To: <20100408083710.2b61ee44@nehalam> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4913 Lines: 172 Stephen Hemminger wrote: > On Thu, 8 Apr 2010 02:18:58 -0400 > Amerigo Wang 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 >> Cc: Neil Horman >> Cc: Stephen Hemminger >> Cc: Matt Mackall >> Signed-off-by: WANG Cong >> >> --- >> >> 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 >> #include >> +#include >> #include >> #include >> +#include >> >> #include >> #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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/