From: Or Gerlitz <ogerlitz@mellanox•com>
To: Jay Vosburgh <fubar@us•ibm.com>
Cc: <davem@davemloft•net>, <roland@kernel•org>,
<netdev@vger•kernel.org>, Shlomo Pongratz <shlomop@mellanox•com>
Subject: Re: [PATCH net 2/2] net/bonding: correctly proxy slave neigh param setup ndo function
Date: Wed, 4 Apr 2012 11:03:41 +0300 [thread overview]
Message-ID: <4F7C005D.60709@mellanox.com> (raw)
In-Reply-To: <10209.1333493626@death.nxdomain>
On 4/4/2012 1:53 AM, Jay Vosburgh wrote:
> Or Gerlitz<ogerlitz@mellanox•com> wrote:
>
>> From: Shlomo Pongratz<shlomop@mellanox•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@mellanox•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.
next prev parent reply other threads:[~2012-04-04 8:04 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-02 16:17 [PATCH net 0/2] net/bonding: fixes related to neigh Or Gerlitz
2012-04-02 16:17 ` [PATCH net 1/2] net/bonding: emit address change event in more places Or Gerlitz
2012-04-03 22:28 ` Jay Vosburgh
2012-04-04 7:57 ` Or Gerlitz
2012-04-02 16:17 ` [PATCH net 2/2] net/bonding: correctly proxy slave neigh param setup ndo function Or Gerlitz
2012-04-03 22:53 ` Jay Vosburgh
2012-04-04 8:03 ` Or Gerlitz [this message]
2012-04-04 16:56 ` Jay Vosburgh
2012-04-03 18:58 ` [PATCH net 0/2] net/bonding: fixes related to neigh Or Gerlitz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F7C005D.60709@mellanox.com \
--to=ogerlitz@mellanox$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=fubar@us$(echo .)ibm.com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=roland@kernel$(echo .)org \
--cc=shlomop@mellanox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox