* [PATCH v2] bonding: "primary_reselect" with "failure" is not working properly @ 2015-07-02 12:07 Mazhar Rana 2015-07-02 20:42 ` Jay Vosburgh 0 siblings, 1 reply; 7+ messages in thread From: Mazhar Rana @ 2015-07-02 12:07 UTC (permalink / raw) To: netdev, j.vosburgh, vfalico, gospo Cc: Mazhar Rana, davem, sanket.shah, mazhar.rana, linux-kernel When "primary_reselect" is set to "failure", primary interface should not become active until current active slave is up. But if we set first member of bond device as a "primary" interface and "primary_reselect" is set to "failure" then whenever primary interface's link get back(up) it become active slave even if current active slave is still up. With this patch, "bond_find_best_slave" will not traverse members if primary interface is not candidate for failover/reselection and current active slave is still up. Signed-off-by: Mazhar Rana <mazhar.rana@cyberoam.com> Reviewed-by: Sanket Shah <sanket.shah@cyberoam.com> --- v2: return "curr" instead of "bond->curr_active_slave". drivers/net/bonding/bond_main.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 19eb990..ac71261 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -715,7 +715,7 @@ static bool bond_should_change_active(struct bonding *bond) */ static struct slave *bond_find_best_slave(struct bonding *bond) { - struct slave *slave, *bestslave = NULL, *primary; + struct slave *slave, *bestslave = NULL, *primary, *curr; struct list_head *iter; int mintime = bond->params.updelay; @@ -724,6 +724,14 @@ static struct slave *bond_find_best_slave(struct bonding *bond) bond_should_change_active(bond)) return primary; + /* We are here means primary interface is not candidate for + * reslection/failover. If currenet active slave is still up + * then there is no meaning to traverse members. + */ + curr = rtnl_dereference(bond->curr_active_slave); + if (curr && curr->link == BOND_LINK_UP) + return curr; + bond_for_each_slave(bond, slave, iter) { if (slave->link == BOND_LINK_UP) return slave; -- 1.9.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] bonding: "primary_reselect" with "failure" is not working properly 2015-07-02 12:07 [PATCH v2] bonding: "primary_reselect" with "failure" is not working properly Mazhar Rana @ 2015-07-02 20:42 ` Jay Vosburgh 2015-07-03 9:53 ` GMAIL 0 siblings, 1 reply; 7+ messages in thread From: Jay Vosburgh @ 2015-07-02 20:42 UTC (permalink / raw) To: Mazhar Rana Cc: netdev, vfalico, gospo, davem, sanket.shah, mazhar.rana, linux-kernel [ added netdev to cc ] Mazhar Rana <ranamazharp@gmail.com> wrote: >When "primary_reselect" is set to "failure", primary interface should >not become active until current active slave is up. But if we set first I think you mean "until current active slave is down" here, not "up." >member of bond device as a "primary" interface and "primary_reselect" >is set to "failure" then whenever primary interface's link get back(up) >it become active slave even if current active slave is still up. > >With this patch, "bond_find_best_slave" will not traverse members if >primary interface is not candidate for failover/reselection and current >active slave is still up. > >Signed-off-by: Mazhar Rana <mazhar.rana@cyberoam.com> >Reviewed-by: Sanket Shah <sanket.shah@cyberoam.com> >--- >v2: return "curr" instead of "bond->curr_active_slave". > > drivers/net/bonding/bond_main.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 19eb990..ac71261 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -715,7 +715,7 @@ static bool bond_should_change_active(struct bonding *bond) > */ > static struct slave *bond_find_best_slave(struct bonding *bond) > { >- struct slave *slave, *bestslave = NULL, *primary; >+ struct slave *slave, *bestslave = NULL, *primary, *curr; > struct list_head *iter; > int mintime = bond->params.updelay; > >@@ -724,6 +724,14 @@ static struct slave *bond_find_best_slave(struct bonding *bond) > bond_should_change_active(bond)) > return primary; > >+ /* We are here means primary interface is not candidate for >+ * reslection/failover. If currenet active slave is still up >+ * then there is no meaning to traverse members. >+ */ >+ curr = rtnl_dereference(bond->curr_active_slave); >+ if (curr && curr->link == BOND_LINK_UP) >+ return curr; >+ > bond_for_each_slave(bond, slave, iter) { > if (slave->link == BOND_LINK_UP) > return slave; >-- I believe the above patch will work, but I also think these functions are kind of hacky, as bond_should_change_active() doesn't really give the answer its name implies, so we have to second guess here. I think the following, while a bigger change, ends up with clearer code. Compile tested only. Comments? -J diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 19eb990..8c30f6b 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -689,40 +689,54 @@ out: } -static bool bond_should_change_active(struct bonding *bond) +static struct slave *bond_choose_primary_or_current(struct bonding *bond) { struct slave *prim = rtnl_dereference(bond->primary_slave); struct slave *curr = rtnl_dereference(bond->curr_active_slave); - if (!prim || !curr || curr->link != BOND_LINK_UP) - return true; + if (!prim || !prim->link == BOND_LINK_UP) + return curr; + if (bond->force_primary) { bond->force_primary = false; - return true; + return prim; + } + + if (!curr || curr->link != BOND_LINK_UP) + return prim; + + /* At this point, prim and curr are both up */ + switch (bond->params.primary_reselect) { + case BOND_PRI_RESELECT_ALWAYS: + return prim; + case BOND_PRI_RESELECT_BETTER: + if (prim->speed < curr->speed) + return curr; + if (prim->speed == curr->speed && prim->duplex <= curr->duplex) + return curr; + return prim; + case BOND_PRI_RESELECT_FAILURE: + return curr; + default: + netdev_err(bond->dev, "impossible primary_reselect %d\n", + bond->params.primary_reselect); + return curr; } - if (bond->params.primary_reselect == BOND_PRI_RESELECT_BETTER && - (prim->speed < curr->speed || - (prim->speed == curr->speed && prim->duplex <= curr->duplex))) - return false; - if (bond->params.primary_reselect == BOND_PRI_RESELECT_FAILURE) - return false; - return true; } /** - * find_best_interface - select the best available slave to be the active one + * bond_find_best_slave - select the best available slave to be the active one * @bond: our bonding struct */ static struct slave *bond_find_best_slave(struct bonding *bond) { - struct slave *slave, *bestslave = NULL, *primary; + struct slave *slave, *bestslave = NULL; struct list_head *iter; int mintime = bond->params.updelay; - primary = rtnl_dereference(bond->primary_slave); - if (primary && primary->link == BOND_LINK_UP && - bond_should_change_active(bond)) - return primary; + slave = bond_choose_primary_or_current(bond); + if (slave) + return slave; bond_for_each_slave(bond, slave, iter) { if (slave->link == BOND_LINK_UP) --- -Jay Vosburgh, jay.vosburgh@canonical.com ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] bonding: "primary_reselect" with "failure" is not working properly 2015-07-02 20:42 ` Jay Vosburgh @ 2015-07-03 9:53 ` GMAIL 2015-07-03 18:16 ` Jay Vosburgh 0 siblings, 1 reply; 7+ messages in thread From: GMAIL @ 2015-07-03 9:53 UTC (permalink / raw) To: Jay Vosburgh Cc: netdev, vfalico, gospo, davem, sanket.shah, mazhar.rana, linux-kernel Hi Jay, On Friday 03 July 2015 02:12 AM, Jay Vosburgh wrote: > [ added netdev to cc ] > > Mazhar Rana <ranamazharp@gmail.com> wrote: > >> When "primary_reselect" is set to "failure", primary interface should >> not become active until current active slave is up. But if we set first > I think you mean "until current active slave is down" here, not > "up." Yes, It should be "up", grammatical mistake > >> member of bond device as a "primary" interface and "primary_reselect" >> is set to "failure" then whenever primary interface's link get back(up) >> it become active slave even if current active slave is still up. >> >> With this patch, "bond_find_best_slave" will not traverse members if >> primary interface is not candidate for failover/reselection and current >> active slave is still up. >> >> Signed-off-by: Mazhar Rana <mazhar.rana@cyberoam.com> >> Reviewed-by: Sanket Shah <sanket.shah@cyberoam.com> >> --- >> v2: return "curr" instead of "bond->curr_active_slave". >> >> drivers/net/bonding/bond_main.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 19eb990..ac71261 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -715,7 +715,7 @@ static bool bond_should_change_active(struct bonding *bond) >> */ >> static struct slave *bond_find_best_slave(struct bonding *bond) >> { >> - struct slave *slave, *bestslave = NULL, *primary; >> + struct slave *slave, *bestslave = NULL, *primary, *curr; >> struct list_head *iter; >> int mintime = bond->params.updelay; >> >> @@ -724,6 +724,14 @@ static struct slave *bond_find_best_slave(struct bonding *bond) >> bond_should_change_active(bond)) >> return primary; >> >> + /* We are here means primary interface is not candidate for >> + * reslection/failover. If currenet active slave is still up >> + * then there is no meaning to traverse members. >> + */ >> + curr = rtnl_dereference(bond->curr_active_slave); >> + if (curr && curr->link == BOND_LINK_UP) >> + return curr; >> + >> bond_for_each_slave(bond, slave, iter) { >> if (slave->link == BOND_LINK_UP) >> return slave; >> -- > I believe the above patch will work, but I also think these > functions are kind of hacky, as bond_should_change_active() doesn't > really give the answer its name implies, so we have to second guess > here. > > I think the following, while a bigger change, ends up with > clearer code. Compile tested only. Comments? > > -J > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 19eb990..8c30f6b 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -689,40 +689,54 @@ out: > > } > > -static bool bond_should_change_active(struct bonding *bond) > +static struct slave *bond_choose_primary_or_current(struct bonding *bond) > { > struct slave *prim = rtnl_dereference(bond->primary_slave); > struct slave *curr = rtnl_dereference(bond->curr_active_slave); > > - if (!prim || !curr || curr->link != BOND_LINK_UP) > - return true; > + if (!prim || !prim->link == BOND_LINK_UP) > + return curr; This will not work when prim and curr both are pointing to same interface, e.g. bond0={eth0, eth1, eth2}, prim=eth0, curr=eth0, now if prim goes down then it will return curr which is again pointing to primary interface, so failover never happen. > + > if (bond->force_primary) { > bond->force_primary = false; > - return true; > + return prim; > + } > + > + if (!curr || curr->link != BOND_LINK_UP) > + return prim; > + > + /* At this point, prim and curr are both up */ > + switch (bond->params.primary_reselect) { > + case BOND_PRI_RESELECT_ALWAYS: > + return prim; > + case BOND_PRI_RESELECT_BETTER: > + if (prim->speed < curr->speed) > + return curr; > + if (prim->speed == curr->speed && prim->duplex <= curr->duplex) > + return curr; > + return prim; > + case BOND_PRI_RESELECT_FAILURE: > + return curr; > + default: > + netdev_err(bond->dev, "impossible primary_reselect %d\n", > + bond->params.primary_reselect); > + return curr; > } > - if (bond->params.primary_reselect == BOND_PRI_RESELECT_BETTER && > - (prim->speed < curr->speed || > - (prim->speed == curr->speed && prim->duplex <= curr->duplex))) > - return false; > - if (bond->params.primary_reselect == BOND_PRI_RESELECT_FAILURE) > - return false; > - return true; > } > > /** > - * find_best_interface - select the best available slave to be the active one > + * bond_find_best_slave - select the best available slave to be the active one > * @bond: our bonding struct > */ > static struct slave *bond_find_best_slave(struct bonding *bond) > { > - struct slave *slave, *bestslave = NULL, *primary; > + struct slave *slave, *bestslave = NULL; > struct list_head *iter; > int mintime = bond->params.updelay; > > - primary = rtnl_dereference(bond->primary_slave); > - if (primary && primary->link == BOND_LINK_UP && > - bond_should_change_active(bond)) > - return primary; > + slave = bond_choose_primary_or_current(bond); primary_reselect policy only make sense when 'primary' interface is specified. And we should go for reselection only when primary link is up. So this function call should be conditional. If we make this call conditional and remove first condition/check from "bond_choose_primary_or_current" then it will work fine. > + if (slave) > + return slave; > > bond_for_each_slave(bond, slave, iter) { > if (slave->link == BOND_LINK_UP) > > --- > -Jay Vosburgh, jay.vosburgh@canonical.com Below is the updated version of your patch. Any Comments or suggestions ? diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 19eb990..3520a1b 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -689,40 +689,54 @@ out: } -static bool bond_should_change_active(struct bonding *bond) +static struct slave *bond_choose_primary_or_current(struct bonding *bond) { struct slave *prim = rtnl_dereference(bond->primary_slave); struct slave *curr = rtnl_dereference(bond->curr_active_slave); - if (!prim || !curr || curr->link != BOND_LINK_UP) - return true; if (bond->force_primary) { bond->force_primary = false; - return true; + return prim; + } + + if (!curr || curr->link != BOND_LINK_UP) + return prim; + + /* At this point, prim and curr are both up */ + switch (bond->params.primary_reselect) { + case BOND_PRI_RESELECT_ALWAYS: + return prim; + case BOND_PRI_RESELECT_BETTER: + if (prim->speed < curr->speed) + return curr; + if (prim->speed == curr->speed && prim->duplex <= curr->duplex) + return curr; + return prim; + case BOND_PRI_RESELECT_FAILURE: + return curr; + default: + netdev_err(bond->dev, "impossible primary_reselect %d\n", + bond->params.primary_reselect); + return curr; } - if (bond->params.primary_reselect == BOND_PRI_RESELECT_BETTER && - (prim->speed < curr->speed || - (prim->speed == curr->speed && prim->duplex <= curr->duplex))) - return false; - if (bond->params.primary_reselect == BOND_PRI_RESELECT_FAILURE) - return false; - return true; } /** - * find_best_interface - select the best available slave to be the active one + * bond_find_best_slave - select the best available slave to be the active one * @bond: our bonding struct */ static struct slave *bond_find_best_slave(struct bonding *bond) { - struct slave *slave, *bestslave = NULL, *primary; + struct slave *slave = NULL, *bestslave = NULL, *primary; struct list_head *iter; int mintime = bond->params.updelay; primary = rtnl_dereference(bond->primary_slave); - if (primary && primary->link == BOND_LINK_UP && - bond_should_change_active(bond)) - return primary; + if (primary && primary->link == BOND_LINK_UP) + slave = bond_choose_primary_or_current(bond); + + if (slave) + return slave; bond_for_each_slave(bond, slave, iter) { if (slave->link == BOND_LINK_UP) --- Regards, Mazhar Rana ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] bonding: "primary_reselect" with "failure" is not working properly 2015-07-03 9:53 ` GMAIL @ 2015-07-03 18:16 ` Jay Vosburgh 2015-07-06 12:04 ` GMAIL 0 siblings, 1 reply; 7+ messages in thread From: Jay Vosburgh @ 2015-07-03 18:16 UTC (permalink / raw) To: GMAIL; +Cc: netdev, vfalico, gospo, davem, sanket.shah, mazhar.rana, linux-kernel GMAIL <ranamazharp@gmail.com> wrote: >Hi Jay, > >On Friday 03 July 2015 02:12 AM, Jay Vosburgh wrote: > >> [ added netdev to cc ] >> >> Mazhar Rana <ranamazharp@gmail.com> wrote: >> >>> When "primary_reselect" is set to "failure", primary interface should >>> not become active until current active slave is up. But if we set first >> I think you mean "until current active slave is down" here, not >> "up." > >Yes, It should be "up", grammatical mistake "down," right? [...] >Below is the updated version of your patch. Any Comments or suggestions ? [...] > static struct slave *bond_find_best_slave(struct bonding *bond) > { >- struct slave *slave, *bestslave = NULL, *primary; >+ struct slave *slave = NULL, *bestslave = NULL, *primary; > struct list_head *iter; > int mintime = bond->params.updelay; > primary = rtnl_dereference(bond->primary_slave); >- if (primary && primary->link == BOND_LINK_UP && >- bond_should_change_active(bond)) >- return primary; >+ if (primary && primary->link == BOND_LINK_UP) >+ slave = bond_choose_primary_or_current(bond); >+ >+ if (slave) >+ return slave; > bond_for_each_slave(bond, slave, iter) { > if (slave->link == BOND_LINK_UP) I think this will misbehave in the case that curr is up and available, but primary is NULL (this can happen when the primary option is cleared). In this case, the above code will not call bond_choose_primary_or_current, and will then run the loop to find a new curr, which may select a different slave unnecessarily. How does the following look? I prefer to make the call to choose_primary_or_current unconditional, and have it decide if the search loop should be run. In this version, _choose_ tests curr if prim is not suitable. Compile tested only. Thoughts? -J diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 19eb990..1e35e25 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -689,40 +689,57 @@ out: } -static bool bond_should_change_active(struct bonding *bond) +static struct slave *bond_choose_primary_or_current(struct bonding *bond) { struct slave *prim = rtnl_dereference(bond->primary_slave); struct slave *curr = rtnl_dereference(bond->curr_active_slave); - if (!prim || !curr || curr->link != BOND_LINK_UP) - return true; + if (!prim || !prim->link == BOND_LINK_UP) { + if (!curr || !curr->link == BOND_LINK_UP) + return NULL; + return curr; + } + if (bond->force_primary) { bond->force_primary = false; - return true; + return prim; + } + + if (!curr || curr->link != BOND_LINK_UP) + return prim; + + /* At this point, prim and curr are both up */ + switch (bond->params.primary_reselect) { + case BOND_PRI_RESELECT_ALWAYS: + return prim; + case BOND_PRI_RESELECT_BETTER: + if (prim->speed < curr->speed) + return curr; + if (prim->speed == curr->speed && prim->duplex <= curr->duplex) + return curr; + return prim; + case BOND_PRI_RESELECT_FAILURE: + return curr; + default: + netdev_err(bond->dev, "impossible primary_reselect %d\n", + bond->params.primary_reselect); + return curr; } - if (bond->params.primary_reselect == BOND_PRI_RESELECT_BETTER && - (prim->speed < curr->speed || - (prim->speed == curr->speed && prim->duplex <= curr->duplex))) - return false; - if (bond->params.primary_reselect == BOND_PRI_RESELECT_FAILURE) - return false; - return true; } /** - * find_best_interface - select the best available slave to be the active one + * bond_find_best_slave - select the best available slave to be the active one * @bond: our bonding struct */ static struct slave *bond_find_best_slave(struct bonding *bond) { - struct slave *slave, *bestslave = NULL, *primary; + struct slave *slave, *bestslave = NULL; struct list_head *iter; int mintime = bond->params.updelay; - primary = rtnl_dereference(bond->primary_slave); - if (primary && primary->link == BOND_LINK_UP && - bond_should_change_active(bond)) - return primary; + slave = bond_choose_primary_or_current(bond); + if (slave) + return slave; bond_for_each_slave(bond, slave, iter) { if (slave->link == BOND_LINK_UP) --- -Jay Vosburgh, jay.vosburgh@canonical.com ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] bonding: "primary_reselect" with "failure" is not working properly 2015-07-03 18:16 ` Jay Vosburgh @ 2015-07-06 12:04 ` GMAIL 2015-07-06 15:32 ` Andy Gospodarek 0 siblings, 1 reply; 7+ messages in thread From: GMAIL @ 2015-07-06 12:04 UTC (permalink / raw) To: Jay Vosburgh Cc: netdev, vfalico, gospo, davem, sanket.shah, mazhar.rana, linux-kernel On Friday 03 July 2015 11:46 PM, Jay Vosburgh wrote: > GMAIL <ranamazharp@gmail.com> wrote: > >> Hi Jay, >> >> On Friday 03 July 2015 02:12 AM, Jay Vosburgh wrote: >> >>> [ added netdev to cc ] >>> >>> Mazhar Rana <ranamazharp@gmail.com> wrote: >>> >>>> When "primary_reselect" is set to "failure", primary interface should >>>> not become active until current active slave is up. But if we set first >>> I think you mean "until current active slave is down" here, not >>> "up." >> Yes, It should be "up", grammatical mistake > "down," right? Yes, "Down". > > [...] >> Below is the updated version of your patch. Any Comments or suggestions ? > [...] >> static struct slave *bond_find_best_slave(struct bonding *bond) >> { >> - struct slave *slave, *bestslave = NULL, *primary; >> + struct slave *slave = NULL, *bestslave = NULL, *primary; >> struct list_head *iter; >> int mintime = bond->params.updelay; >> primary = rtnl_dereference(bond->primary_slave); >> - if (primary && primary->link == BOND_LINK_UP && >> - bond_should_change_active(bond)) >> - return primary; >> + if (primary && primary->link == BOND_LINK_UP) >> + slave = bond_choose_primary_or_current(bond); >> + >> + if (slave) >> + return slave; >> bond_for_each_slave(bond, slave, iter) { >> if (slave->link == BOND_LINK_UP) > I think this will misbehave in the case that curr is up and > available, but primary is NULL (this can happen when the primary option > is cleared). In this case, the above code will not call > bond_choose_primary_or_current, and will then run the loop to find a new > curr, which may select a different slave unnecessarily. > > How does the following look? I prefer to make the call to > choose_primary_or_current unconditional, and have it decide if the > search loop should be run. In this version, _choose_ tests curr if prim > is not suitable. Compile tested only. > > Thoughts? > > -J > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 19eb990..1e35e25 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -689,40 +689,57 @@ out: > > } > > -static bool bond_should_change_active(struct bonding *bond) > +static struct slave *bond_choose_primary_or_current(struct bonding *bond) > { > struct slave *prim = rtnl_dereference(bond->primary_slave); > struct slave *curr = rtnl_dereference(bond->curr_active_slave); > > - if (!prim || !curr || curr->link != BOND_LINK_UP) > - return true; > + if (!prim || !prim->link == BOND_LINK_UP) { > + if (!curr || !curr->link == BOND_LINK_UP) > + return NULL; > + return curr; > + } > + > if (bond->force_primary) { > bond->force_primary = false; > - return true; > + return prim; > + } > + > + if (!curr || curr->link != BOND_LINK_UP) > + return prim; > + > + /* At this point, prim and curr are both up */ > + switch (bond->params.primary_reselect) { > + case BOND_PRI_RESELECT_ALWAYS: > + return prim; > + case BOND_PRI_RESELECT_BETTER: > + if (prim->speed < curr->speed) > + return curr; > + if (prim->speed == curr->speed && prim->duplex <= curr->duplex) > + return curr; > + return prim; > + case BOND_PRI_RESELECT_FAILURE: > + return curr; > + default: > + netdev_err(bond->dev, "impossible primary_reselect %d\n", > + bond->params.primary_reselect); > + return curr; > } > - if (bond->params.primary_reselect == BOND_PRI_RESELECT_BETTER && > - (prim->speed < curr->speed || > - (prim->speed == curr->speed && prim->duplex <= curr->duplex))) > - return false; > - if (bond->params.primary_reselect == BOND_PRI_RESELECT_FAILURE) > - return false; > - return true; > } > > /** > - * find_best_interface - select the best available slave to be the active one > + * bond_find_best_slave - select the best available slave to be the active one > * @bond: our bonding struct > */ > static struct slave *bond_find_best_slave(struct bonding *bond) > { > - struct slave *slave, *bestslave = NULL, *primary; > + struct slave *slave, *bestslave = NULL; > struct list_head *iter; > int mintime = bond->params.updelay; > > - primary = rtnl_dereference(bond->primary_slave); > - if (primary && primary->link == BOND_LINK_UP && > - bond_should_change_active(bond)) > - return primary; > + slave = bond_choose_primary_or_current(bond); > + if (slave) > + return slave; > > bond_for_each_slave(bond, slave, iter) { > if (slave->link == BOND_LINK_UP) > > --- > -Jay Vosburgh, jay.vosburgh@canonical.com Looks good, added cosmetic changes for more readability, it might save some instructions :) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 19eb990..317a494 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -689,40 +689,57 @@ out: } -static bool bond_should_change_active(struct bonding *bond) +static struct slave *bond_choose_primary_or_current(struct bonding *bond) { struct slave *prim = rtnl_dereference(bond->primary_slave); struct slave *curr = rtnl_dereference(bond->curr_active_slave); - if (!prim || !curr || curr->link != BOND_LINK_UP) - return true; + if (!prim || prim->link != BOND_LINK_UP) { + if (!curr || curr->link != BOND_LINK_UP) + return NULL; + return curr; + } + if (bond->force_primary) { bond->force_primary = false; - return true; + return prim; + } + + if (!curr || curr->link != BOND_LINK_UP) + return prim; + + /* At this point, prim and curr are both up */ + switch (bond->params.primary_reselect) { + case BOND_PRI_RESELECT_ALWAYS: + return prim; + case BOND_PRI_RESELECT_BETTER: + if (prim->speed < curr->speed) + return curr; + if (prim->speed == curr->speed && prim->duplex <= curr->duplex) + return curr; + return prim; + case BOND_PRI_RESELECT_FAILURE: + return curr; + default: + netdev_err(bond->dev, "impossible primary_reselect %d\n", + bond->params.primary_reselect); + return curr; } - if (bond->params.primary_reselect == BOND_PRI_RESELECT_BETTER && - (prim->speed < curr->speed || - (prim->speed == curr->speed && prim->duplex <= curr->duplex))) - return false; - if (bond->params.primary_reselect == BOND_PRI_RESELECT_FAILURE) - return false; - return true; } /** - * find_best_interface - select the best available slave to be the active one + * bond_find_best_slave - select the best available slave to be the active one * @bond: our bonding struct */ static struct slave *bond_find_best_slave(struct bonding *bond) { - struct slave *slave, *bestslave = NULL, *primary; + struct slave *slave, *bestslave = NULL; struct list_head *iter; int mintime = bond->params.updelay; - primary = rtnl_dereference(bond->primary_slave); - if (primary && primary->link == BOND_LINK_UP && - bond_should_change_active(bond)) - return primary; + slave = bond_choose_primary_or_current(bond); + if (slave) + return slave; bond_for_each_slave(bond, slave, iter) { if (slave->link == BOND_LINK_UP) --- Regards, Mazhar Rana ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] bonding: "primary_reselect" with "failure" is not working properly 2015-07-06 12:04 ` GMAIL @ 2015-07-06 15:32 ` Andy Gospodarek 2015-07-07 9:07 ` GMAIL 0 siblings, 1 reply; 7+ messages in thread From: Andy Gospodarek @ 2015-07-06 15:32 UTC (permalink / raw) To: GMAIL Cc: Jay Vosburgh, netdev, vfalico, davem, sanket.shah, mazhar.rana, linux-kernel On Mon, Jul 06, 2015 at 05:34:01PM +0530, GMAIL wrote: > On Friday 03 July 2015 11:46 PM, Jay Vosburgh wrote: > >GMAIL <ranamazharp@gmail.com> wrote: [...] > > Looks good, added cosmetic changes for more readability, > it might save some instructions :) > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 19eb990..317a494 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -689,40 +689,57 @@ out: > } > -static bool bond_should_change_active(struct bonding *bond) > +static struct slave *bond_choose_primary_or_current(struct bonding *bond) > { > struct slave *prim = rtnl_dereference(bond->primary_slave); > struct slave *curr = rtnl_dereference(bond->curr_active_slave); Probably a good idea to add back a blank line here. Otherwise this logic appears to be proper to resolve your issue and Jay's additions appear to handle the case where primary_slave is NULL. > - if (!prim || !curr || curr->link != BOND_LINK_UP) > - return true; > + if (!prim || prim->link != BOND_LINK_UP) { > + if (!curr || curr->link != BOND_LINK_UP) > + return NULL; > + return curr; > + } > + > if (bond->force_primary) { > bond->force_primary = false; > - return true; > + return prim; > + } > + > + if (!curr || curr->link != BOND_LINK_UP) > + return prim; > + > + /* At this point, prim and curr are both up */ > + switch (bond->params.primary_reselect) { > + case BOND_PRI_RESELECT_ALWAYS: > + return prim; > + case BOND_PRI_RESELECT_BETTER: > + if (prim->speed < curr->speed) > + return curr; > + if (prim->speed == curr->speed && prim->duplex <= curr->duplex) > + return curr; > + return prim; > + case BOND_PRI_RESELECT_FAILURE: > + return curr; > + default: > + netdev_err(bond->dev, "impossible primary_reselect %d\n", > + bond->params.primary_reselect); > + return curr; > } > - if (bond->params.primary_reselect == BOND_PRI_RESELECT_BETTER && > - (prim->speed < curr->speed || > - (prim->speed == curr->speed && prim->duplex <= curr->duplex))) > - return false; > - if (bond->params.primary_reselect == BOND_PRI_RESELECT_FAILURE) > - return false; > - return true; > } > > /** > - * find_best_interface - select the best available slave to be the active one > + * bond_find_best_slave - select the best available slave to be the active one > * @bond: our bonding struct > */ > static struct slave *bond_find_best_slave(struct bonding *bond) > { > - struct slave *slave, *bestslave = NULL, *primary; > + struct slave *slave, *bestslave = NULL; > struct list_head *iter; > int mintime = bond->params.updelay; > > - primary = rtnl_dereference(bond->primary_slave); > - if (primary && primary->link == BOND_LINK_UP && > - bond_should_change_active(bond)) > - return primary; > + slave = bond_choose_primary_or_current(bond); > + if (slave) > + return slave; > > bond_for_each_slave(bond, slave, iter) { > if (slave->link == BOND_LINK_UP) > --- > > Regards, > Mazhar Rana > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] bonding: "primary_reselect" with "failure" is not working properly 2015-07-06 15:32 ` Andy Gospodarek @ 2015-07-07 9:07 ` GMAIL 0 siblings, 0 replies; 7+ messages in thread From: GMAIL @ 2015-07-07 9:07 UTC (permalink / raw) To: Andy Gospodarek Cc: Jay Vosburgh, netdev, vfalico, davem, sanket.shah, mazhar.rana, linux-kernel On Monday 06 July 2015 09:02 PM, Andy Gospodarek wrote: > On Mon, Jul 06, 2015 at 05:34:01PM +0530, GMAIL wrote: >> On Friday 03 July 2015 11:46 PM, Jay Vosburgh wrote: >>> GMAIL<ranamazharp@gmail.com> wrote: > [...] >> Looks good, added cosmetic changes for more readability, >> it might save some instructions :) >> >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 19eb990..317a494 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -689,40 +689,57 @@ out: >> } >> -static bool bond_should_change_active(struct bonding *bond) >> +static struct slave *bond_choose_primary_or_current(struct bonding *bond) >> { >> struct slave *prim = rtnl_dereference(bond->primary_slave); >> struct slave *curr = rtnl_dereference(bond->curr_active_slave); > Probably a good idea to add back a blank line here. > > Otherwise this logic appears to be proper to resolve your issue and > Jay's additions appear to handle the case where primary_slave is NULL. It was there, I don't know, may be mail client issue. >> - if (!prim || !curr || curr->link != BOND_LINK_UP) >> - return true; >> + if (!prim || prim->link != BOND_LINK_UP) { >> + if (!curr || curr->link != BOND_LINK_UP) >> + return NULL; >> + return curr; >> + } >> + >> if (bond->force_primary) { >> bond->force_primary = false; >> - return true; >> + return prim; >> + } >> + >> + if (!curr || curr->link != BOND_LINK_UP) >> + return prim; >> + >> + /* At this point, prim and curr are both up */ >> + switch (bond->params.primary_reselect) { >> + case BOND_PRI_RESELECT_ALWAYS: >> + return prim; >> + case BOND_PRI_RESELECT_BETTER: >> + if (prim->speed < curr->speed) >> + return curr; >> + if (prim->speed == curr->speed && prim->duplex <= curr->duplex) >> + return curr; >> + return prim; >> + case BOND_PRI_RESELECT_FAILURE: >> + return curr; >> + default: >> + netdev_err(bond->dev, "impossible primary_reselect %d\n", >> + bond->params.primary_reselect); >> + return curr; >> } >> - if (bond->params.primary_reselect == BOND_PRI_RESELECT_BETTER && >> - (prim->speed < curr->speed || >> - (prim->speed == curr->speed && prim->duplex <= curr->duplex))) >> - return false; >> - if (bond->params.primary_reselect == BOND_PRI_RESELECT_FAILURE) >> - return false; >> - return true; >> } >> >> /** >> - * find_best_interface - select the best available slave to be the active one >> + * bond_find_best_slave - select the best available slave to be the active one >> * @bond: our bonding struct >> */ >> static struct slave *bond_find_best_slave(struct bonding *bond) >> { >> - struct slave *slave, *bestslave = NULL, *primary; >> + struct slave *slave, *bestslave = NULL; >> struct list_head *iter; >> int mintime = bond->params.updelay; >> >> - primary = rtnl_dereference(bond->primary_slave); >> - if (primary && primary->link == BOND_LINK_UP && >> - bond_should_change_active(bond)) >> - return primary; >> + slave = bond_choose_primary_or_current(bond); >> + if (slave) >> + return slave; >> >> bond_for_each_slave(bond, slave, iter) { >> if (slave->link == BOND_LINK_UP) >> --- >> >> Regards, >> Mazhar Rana >> Sending updated version of patch(v3) separately which will accommodate my and Jay's changes. Regards, Mazhar Rana ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-07-07 9:07 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-07-02 12:07 [PATCH v2] bonding: "primary_reselect" with "failure" is not working properly Mazhar Rana 2015-07-02 20:42 ` Jay Vosburgh 2015-07-03 9:53 ` GMAIL 2015-07-03 18:16 ` Jay Vosburgh 2015-07-06 12:04 ` GMAIL 2015-07-06 15:32 ` Andy Gospodarek 2015-07-07 9:07 ` GMAIL
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).