[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 4 Apr 2012 11:03:41 +0300
From: Or Gerlitz <ogerlitz@...lanox.com>
To: Jay Vosburgh <fubar@...ibm.com>
CC: <davem@...emloft.net>, <roland@...nel.org>,
<netdev@...r.kernel.org>, Shlomo Pongratz <shlomop@...lanox.com>
Subject: Re: [PATCH net 2/2] net/bonding: correctly proxy slave neigh param
setup ndo function
On 4/4/2012 1:53 AM, Jay Vosburgh wrote:
> Or Gerlitz<ogerlitz@...lanox.com> wrote:
>
>> From: Shlomo Pongratz<shlomop@...lanox.com>
>>
>> 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<shlomop@...lanox.com>
>> ---
>> 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"
>
> + /* Does slave implement neigh_setup ? */
> + if (!parms.neigh_setup)
> + return 0;
>
> I don't think this comment is necessary.
okay, will remove
Or.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists