| Message ID | 1333383430-17456-3-git-send-email-ogerlitz@mellanox.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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). >+ /* Does slave implement neigh_setup ? */ >+ if (!parms.neigh_setup) >+ return 0; I don't think this comment is necessary. -J >+ >+ return parms.neigh_setup(n); >+} >+ >+/* >+ * The bonding ndo_neigh_setup is called at init time beofre any >+ * slave exists. So we must declare proxy setup function which will >+ * be used at run time to resolve the actual slave neigh param setup. >+ */ >+static int bond_neigh_setup(struct net_device *dev, >+ struct neigh_parms *parms) >+{ >+ parms->neigh_setup = bond_neigh_init; > >- if (slave) { >- const struct net_device_ops *slave_ops >- = slave->dev->netdev_ops; >- if (slave_ops->ndo_neigh_setup) >- return slave_ops->ndo_neigh_setup(slave->dev, parms); >- } > return 0; > } --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Or Gerlitz <ogerlitz@mellanox.com> wrote: >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" 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + + /* Does slave implement neigh_setup ? */ + if (!parms.neigh_setup) + return 0; + + return parms.neigh_setup(n); +} + +/* + * The bonding ndo_neigh_setup is called at init time beofre any + * slave exists. So we must declare proxy setup function which will + * be used at run time to resolve the actual slave neigh param setup. + */ +static int bond_neigh_setup(struct net_device *dev, + struct neigh_parms *parms) +{ + parms->neigh_setup = bond_neigh_init; - if (slave) { - const struct net_device_ops *slave_ops - = slave->dev->netdev_ops; - if (slave_ops->ndo_neigh_setup) - return slave_ops->ndo_neigh_setup(slave->dev, parms); - } return 0; }