| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20090915150848.GC6624@psychotron.lab.eng.brq.redhat.com>
Date: Tue, 15 Sep 2009 17:08:49 +0200
From: Jiri Pirko <jpirko@...hat.com>
To: Jay Vosburgh <fubar@...ibm.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net,
bonding-devel@...ts.sourceforge.net, nicolas.2p.debian@...e.fr
Subject: Re: [PATCH net-next-2.6] bonding: make ab_arp select active slaves
as other modes
Fri, Sep 11, 2009 at 02:32:18AM CEST, fubar@...ibm.com wrote:
>Jiri Pirko <jpirko@...hat.com> wrote:
>
>>When I was implementing primary_passive option (formely named primary_lazy) I've
>>run into troubles with ab_arp. This is the only mode which is not using
>>bond_select_active_slave() function to select active slave and instead it
>>selects it itself. This seems to be not the right behaviour and it would be
>>better to do it in bond_select_active_slave() for all cases. This patch makes
>>this happen. Please review.
>
> Sorry for the delay in response; was out of the office.
>
> My first question is whether this affect the "current_arp_slave"
>behavior, i.e., the round-robining of the ARP probes when no slaves are
>active. Is that something you checked?
Yes, according to my tests this behaves the same way as original code.
How about your tests?
Jirka
>
> I'll give this a test tomorrow as well.
>
> -J
>
>---
> -Jay Vosburgh, IBM Linux Technology Center, fubar@...ibm.com
>
>>Signed-off-by: Jiri Pirko <jpirko@...hat.com>
>>
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index 7c0e0bd..6ebd88d 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1093,15 +1093,8 @@ static struct slave *bond_find_best_slave(struct bonding *bond)
>> return NULL; /* still no slave, return NULL */
>> }
>>
>>- /*
>>- * first try the primary link; if arping, a link must tx/rx
>>- * traffic before it can be considered the curr_active_slave.
>>- * also, we would skip slaves between the curr_active_slave
>>- * and primary_slave that may be up and able to arp
>>- */
>> if ((bond->primary_slave) &&
>>- (!bond->params.arp_interval) &&
>>- (IS_UP(bond->primary_slave->dev))) {
>>+ bond->primary_slave->link == BOND_LINK_UP) {
>> new_active = bond->primary_slave;
>> }
>>
>>@@ -1109,15 +1102,14 @@ static struct slave *bond_find_best_slave(struct bonding *bond)
>> old_active = new_active;
>>
>> bond_for_each_slave_from(bond, new_active, i, old_active) {
>>- if (IS_UP(new_active->dev)) {
>>- if (new_active->link == BOND_LINK_UP) {
>>- return new_active;
>>- } else if (new_active->link == BOND_LINK_BACK) {
>>- /* link up, but waiting for stabilization */
>>- if (new_active->delay < mintime) {
>>- mintime = new_active->delay;
>>- bestslave = new_active;
>>- }
>>+ if (new_active->link == BOND_LINK_UP) {
>>+ return new_active;
>>+ } else if (new_active->link == BOND_LINK_BACK &&
>>+ IS_UP(new_active->dev)) {
>>+ /* link up, but waiting for stabilization */
>>+ if (new_active->delay < mintime) {
>>+ mintime = new_active->delay;
>>+ bestslave = new_active;
>
> Is there a functional reason for rearranging this (i.e., did the
>use of select_active_slave need this for some reason)?
>
>
>> }
>> }
>> }
>>@@ -2929,18 +2921,6 @@ static int bond_ab_arp_inspect(struct bonding *bond, int delta_in_ticks)
>> }
>> }
>>
>>- read_lock(&bond->curr_slave_lock);
>>-
>>- /*
>>- * Trigger a commit if the primary option setting has changed.
>>- */
>>- if (bond->primary_slave &&
>>- (bond->primary_slave != bond->curr_active_slave) &&
>>- (bond->primary_slave->link == BOND_LINK_UP))
>>- commit++;
>>-
>>- read_unlock(&bond->curr_slave_lock);
>>-
>> return commit;
>> }
>>
>>@@ -2961,90 +2941,58 @@ static void bond_ab_arp_commit(struct bonding *bond, int delta_in_ticks)
>> continue;
>>
>> case BOND_LINK_UP:
>>- write_lock_bh(&bond->curr_slave_lock);
>>-
>>- if (!bond->curr_active_slave &&
>>- time_before_eq(jiffies, dev_trans_start(slave->dev) +
>>- delta_in_ticks)) {
>>+ if ((!bond->curr_active_slave &&
>>+ time_before_eq(jiffies,
>>+ dev_trans_start(slave->dev) +
>>+ delta_in_ticks)) ||
>>+ bond->curr_active_slave != slave) {
>> slave->link = BOND_LINK_UP;
>>- bond_change_active_slave(bond, slave);
>> bond->current_arp_slave = NULL;
>>
>> pr_info(DRV_NAME
>>- ": %s: %s is up and now the "
>>- "active interface\n",
>>- bond->dev->name, slave->dev->name);
>>-
>>- } else if (bond->curr_active_slave != slave) {
>>- /* this slave has just come up but we
>>- * already have a current slave; this can
>>- * also happen if bond_enslave adds a new
>>- * slave that is up while we are searching
>>- * for a new slave
>>- */
>>- slave->link = BOND_LINK_UP;
>>- bond_set_slave_inactive_flags(slave);
>>- bond->current_arp_slave = NULL;
>>+ ": %s: link status definitely "
>>+ "up for interface %s.\n",
>>+ bond->dev->name, slave->dev->name);
>>
>>- pr_info(DRV_NAME
>>- ": %s: backup interface %s is now up\n",
>>- bond->dev->name, slave->dev->name);
>>- }
>>+ if (!bond->curr_active_slave ||
>>+ (slave == bond->primary_slave))
>>+ goto do_failover;
>>
>>- write_unlock_bh(&bond->curr_slave_lock);
>>+ }
>>
>>- break;
>>+ continue;
>>
>> case BOND_LINK_DOWN:
>> if (slave->link_failure_count < UINT_MAX)
>> slave->link_failure_count++;
>>
>> slave->link = BOND_LINK_DOWN;
>>+ bond_set_slave_inactive_flags(slave);
>>
>>- if (slave == bond->curr_active_slave) {
>>- pr_info(DRV_NAME
>>- ": %s: link status down for active "
>>- "interface %s, disabling it\n",
>>- bond->dev->name, slave->dev->name);
>>-
>>- bond_set_slave_inactive_flags(slave);
>>-
>>- write_lock_bh(&bond->curr_slave_lock);
>>-
>>- bond_select_active_slave(bond);
>>- if (bond->curr_active_slave)
>>- bond->curr_active_slave->jiffies =
>>- jiffies;
>>-
>>- write_unlock_bh(&bond->curr_slave_lock);
>>+ pr_info(DRV_NAME
>>+ ": %s: link status definitely down for "
>>+ "interface %s, disabling it\n",
>>+ bond->dev->name, slave->dev->name);
>>
>>+ if (slave == bond->curr_active_slave) {
>> bond->current_arp_slave = NULL;
>>-
>>- } else if (slave->state == BOND_STATE_BACKUP) {
>>- pr_info(DRV_NAME
>>- ": %s: backup interface %s is now down\n",
>>- bond->dev->name, slave->dev->name);
>>-
>>- bond_set_slave_inactive_flags(slave);
>>+ goto do_failover;
>> }
>>- break;
>>+
>>+ continue;
>>
>> default:
>> pr_err(DRV_NAME
>> ": %s: impossible: new_link %d on slave %s\n",
>> bond->dev->name, slave->new_link,
>> slave->dev->name);
>>+ continue;
>> }
>>- }
>>
>>- /*
>>- * No race with changes to primary via sysfs, as we hold rtnl.
>>- */
>>- if (bond->primary_slave &&
>>- (bond->primary_slave != bond->curr_active_slave) &&
>>- (bond->primary_slave->link == BOND_LINK_UP)) {
>>+do_failover:
>>+ ASSERT_RTNL();
>> write_lock_bh(&bond->curr_slave_lock);
>>- bond_change_active_slave(bond, bond->primary_slave);
>>+ bond_select_active_slave(bond);
>> write_unlock_bh(&bond->curr_slave_lock);
>> }
>>
>>--
>>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
>
>
>
--
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