From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jay Vosburgh Subject: Re: [PATCH net 2/2] net/bonding: correctly proxy slave neigh param setup ndo function Date: Wed, 04 Apr 2012 09:56:22 -0700 Message-ID: <17441.1333558582@death.nxdomain> References: <1333383430-17456-1-git-send-email-ogerlitz@mellanox.com> <1333383430-17456-3-git-send-email-ogerlitz@mellanox.com> <10209.1333493626@death.nxdomain> <4F7C005D.60709@mellanox.com> Cc: davem@davemloft.net, roland@kernel.org, netdev@vger.kernel.org, Shlomo Pongratz To: Or Gerlitz Return-path: Received: from e9.ny.us.ibm.com ([32.97.182.139]:55463 "EHLO e9.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756993Ab2DDRG6 (ORCPT ); Wed, 4 Apr 2012 13:06:58 -0400 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 4 Apr 2012 13:06:57 -0400 Received: from d01relay05.pok.ibm.com (d01relay05.pok.ibm.com [9.56.227.237]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id BA44A38C8059 for ; Wed, 4 Apr 2012 13:06:53 -0400 (EDT) Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q34H6ol3256986 for ; Wed, 4 Apr 2012 13:06:50 -0400 Received: from d03av05.boulder.ibm.com (loopback [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q34H6kEZ006320 for ; Wed, 4 Apr 2012 11:06:48 -0600 In-reply-to: <4F7C005D.60709@mellanox.com> Sender: netdev-owner@vger.kernel.org List-ID: Or Gerlitz wrote: >On 4/4/2012 1:53 AM, Jay Vosburgh wrote: >> Or Gerlitz wrote: >> >>> From: Shlomo Pongratz >>> >>> The current implemenation was buggy for slaves who use ndo_neigh_setup, >>> since the networking stack invokes the bonding device ndo entry (from >>> neigh_params_alloc) before any devices are enslaved, and the bonding >>> driver can't further delegate the call at that point in time. As a >>> result when bonding IPoIB devices, the neigh_cleanup hasn't been called. >>> >>> Fix that by deferring the actual call into the slave ndo_neigh_setup >> >from the time the bonding neigh_setup is called. >>> >>> Signed-off-by: Shlomo Pongratz >>> --- >>> drivers/net/bonding/bond_main.c | 51 ++++++++++++++++++++++++++++++++------ >>> 1 files changed, 43 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >>> index b0a278d..2eed155 100644 >>> --- a/drivers/net/bonding/bond_main.c >>> +++ b/drivers/net/bonding/bond_main.c >>> @@ -3707,17 +3707,52 @@ static void bond_set_multicast_list(struct net_device *bond_dev) >>> read_unlock(&bond->lock); >>> } >>> >>> -static int bond_neigh_setup(struct net_device *dev, struct neigh_parms *parms) >>> +static int bond_neigh_init(struct neighbour *n) >>> { >>> - struct bonding *bond = netdev_priv(dev); >>> + struct bonding *bond = netdev_priv(n->dev); >>> struct slave *slave = bond->first_slave; >>> + const struct net_device_ops *slave_ops; >>> + struct neigh_parms parms; >>> + int ret; >>> + >>> + if (!slave) >>> + return 0; >>> + >>> + slave_ops = slave->dev->netdev_ops; >>> + >>> + if (!slave_ops->ndo_neigh_setup) >>> + return 0; >>> + >>> + parms.neigh_setup = NULL; >>> + parms.neigh_cleanup = NULL; >>> + ret = slave_ops->ndo_neigh_setup(slave->dev,&parms); >>> + if (ret) >>> + return ret; >>> + >>> + /* >>> + * must bind here to the slave clenaup. Since when last slave is removed >>> + * there will be no slave device to dereference in a bonding >>> + * neigh_cleanup function that we have could add. >>> + */ >>> + n->parms->neigh_cleanup = parms.neigh_cleanup; >> >> I'd write this comment as: >> >> /* Assign slave's neigh_cleanup to neighbour in case cleanup is >> * called after bond has been destroyed. Assumes that all slaves >> * utilize the same neigh_cleanup (true at this writing as only user >> * is ipoib). >> */ >> >> I.e., this logic works only because there cannot currently be a >> situation wherein two slaves have different neigh_cleanup functions >> (including one slave with a neigh_cleanup, and another without). > >Jay, we do need that proxy-ing for the specific case of deleting the last >slave, since in bond_release >the address change and the event emission happen --after-- calling >bond_detach_slave. Still, will pick >your phrasing for the comment and replace "after bond has been destroyed" >with "after last slave has been detached" Yes, I understand that the proxying is needed; the point of the comment is that if there's ever a situation in the future that two slaves have different neigh_cleanup functions, this methodology will not work. There is no guarantee that the slave on which ndo_neigh_setup is called on will also be the last slave to be removed. The change to the comment is ok; I was thinking about ipoib always destroying the bond itself immediately after releasing the final slave, so for ipoib, the two events always happen together. -J >> >> + /* Does slave implement neigh_setup ? */ >> + if (!parms.neigh_setup) >> + return 0; >> >> I don't think this comment is necessary. > >okay, will remove > >Or. > --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com