From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Nicolas_de_Peslo=FCan?= Subject: Re: [patch net-next-2.6 2/8] bonding: register slave pointer for rx_handler Date: Sat, 05 Mar 2011 15:06:58 +0100 Message-ID: <4D724382.7010705@gmail.com> References: <1299320969-7951-1-git-send-email-jpirko@redhat.com> <1299320969-7951-3-git-send-email-jpirko@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: netdev@vger.kernel.org, davem@davemloft.net, shemminger@linux-foundation.org, kaber@trash.net, fubar@us.ibm.com, eric.dumazet@gmail.com, andy@greyhouse.net To: Jiri Pirko Return-path: Received: from mail-wy0-f174.google.com ([74.125.82.174]:64430 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751115Ab1CEOHC (ORCPT ); Sat, 5 Mar 2011 09:07:02 -0500 Received: by wyg36 with SMTP id 36so2996830wyg.19 for ; Sat, 05 Mar 2011 06:07:00 -0800 (PST) In-Reply-To: <1299320969-7951-3-git-send-email-jpirko@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: Le 05/03/2011 11:29, Jiri Pirko a =E9crit : > Register slave pointer as rx_handler data. That would eventually prev= ent > need to loop over slave devices to find the right slave. > > Use synchronize_net to ensure that bond_handle_frame does not get sla= ve > structure freed when working with that. > > Signed-off-by: Jiri Pirko > --- > drivers/net/bonding/bond_main.c | 17 +++++++++++------ > drivers/net/bonding/bonding.h | 3 +++ > 2 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bo= nd_main.c > index 0592e6d..1c19368 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -1495,21 +1495,22 @@ static bool bond_should_deliver_exact_match(s= truct sk_buff *skb, > > static struct sk_buff *bond_handle_frame(struct sk_buff *skb) > { > - struct net_device *slave_dev; > + struct slave *slave; > struct net_device *bond_dev; > > skb =3D skb_share_check(skb, GFP_ATOMIC); > if (unlikely(!skb)) > return NULL; > - slave_dev =3D skb->dev; > - bond_dev =3D ACCESS_ONCE(slave_dev->master); > + > + slave =3D bond_slave_get_rcu(skb->dev); > + bond_dev =3D ACCESS_ONCE(slave->dev->master); > if (unlikely(!bond_dev)) > return skb; > > if (bond_dev->priv_flags& IFF_MASTER_ARPMON) > - slave_dev->last_rx =3D jiffies; > + slave->dev->last_rx =3D jiffies; > > - if (bond_should_deliver_exact_match(skb, slave_dev, bond_dev)) { > + if (bond_should_deliver_exact_match(skb, slave->dev, bond_dev)) { > skb->deliver_no_wcard =3D 1; > return skb; > } Up to this point, it looks like cleanup, and unrelated to the title and= description of the patch. Anyway, the cleanup looks good to me. Should just be in a separate patc= h. > @@ -1703,7 +1704,8 @@ int bond_enslave(struct net_device *bond_dev, s= truct net_device *slave_dev) > pr_debug("Error %d calling netdev_set_bond_master\n", res); > goto err_restore_mac; > } > - res =3D netdev_rx_handler_register(slave_dev, bond_handle_frame, NU= LL); > + res =3D netdev_rx_handler_register(slave_dev, bond_handle_frame, > + new_slave); And using rx_handler data for this purpose sounds good to me. Reviewed-by: Nicolas de Peslo=FCan > if (res) { > pr_debug("Error %d calling netdev_rx_handler_register\n", res); > goto err_unset_master; > @@ -1925,6 +1927,7 @@ err_close: > > err_unreg_rxhandler: > netdev_rx_handler_unregister(slave_dev); > + synchronize_net(); > > err_unset_master: > netdev_set_bond_master(slave_dev, NULL); > @@ -2108,6 +2111,7 @@ int bond_release(struct net_device *bond_dev, s= truct net_device *slave_dev) > } > > netdev_rx_handler_unregister(slave_dev); > + synchronize_net(); > netdev_set_bond_master(slave_dev, NULL); > > slave_disable_netpoll(slave); > @@ -2222,6 +2226,7 @@ static int bond_release_all(struct net_device *= bond_dev) > } > > netdev_rx_handler_unregister(slave_dev); > + synchronize_net(); > netdev_set_bond_master(slave_dev, NULL); > > slave_disable_netpoll(slave); > diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bond= ing.h > index ff4e269..1aac5cd 100644 > --- a/drivers/net/bonding/bonding.h > +++ b/drivers/net/bonding/bonding.h > @@ -264,6 +264,9 @@ struct bonding { > #endif /* CONFIG_DEBUG_FS */ > }; > > +#define bond_slave_get_rcu(dev) \ > + ((struct slave *) rcu_dereference(dev->rx_handler_data)) > + > /** > * Returns NULL if the net_device does not belong to any of the bon= d's slaves > *