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
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <15770.1424011874@nyx>
Date:	Sun, 15 Feb 2015 06:51:14 -0800
From:	Jay Vosburgh <jay.vosburgh@...onical.com>
To:	Mahesh Bandewar <maheshb@...gle.com>
cc:	Andy Gospodarek <andy@...yhouse.net>,
	Veaceslav Falico <vfalico@...il.com>,
	Nikolay Aleksandrov <nikolay@...hat.com>,
	David Miller <davem@...emloft.net>,
	Maciej Zenczykowski <maze@...gle.com>,
	netdev <netdev@...r.kernel.org>,
	Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH next v3 2/6] bonding: implement bond_poll_controller()

Mahesh Bandewar <maheshb@...gle.com> wrote:

>On Thu, Feb 12, 2015 at 1:45 PM, Jay Vosburgh <jay.vosburgh@...onical.com>
>wrote:
[...]
>    >+ if (BOND_MODE(bond) == BOND_MODE_8023AD)
>    >+ if (bond_3ad_get_active_agg_info(bond, &ad_info))
>    >+ return;
>    >+
>    >+ bond_for_each_slave(bond, slave, iter) {
>    >+ ops = slave->dev->netdev_ops;
>    >+ if (!bond_slave_is_up(slave) || !ops->ndo_poll_controller)
>    >+ continue;
>    >+
>    >+ if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>    >+ struct aggregator *agg =
>    >+ SLAVE_AD_INFO(slave)->port.aggregator;
>    >+
>    >+ if (agg && agg->aggregator_identifier !=
>    >+ ad_info.aggregator_id)
>    >+ continue;
>    >+ if (!bond_3ad_port_is_active(slave) || ad_info.ports != 1)
>    >+ continue;
>    
>    
>    The above will exclude slaves that are in an aggregator with
>    more than one member, which is likely to be the usual case. Is that
>    intentional?
>    
>    
>
>The idea is to use all the ports in the aggregator. In a situation where
>there is only one port but is not active, then *only* use it. So from that
>perspective this logic needs '&&' instead of '||'.

	If you want to use any port from the active aggregator, then I
think logic to first find the active agg, then cycle through its ports
would be better.  This would also eliminate the concern from your other
reply regarding the following:

> >+static int bond_3ad_port_operational(struct slave *slave)
> >+{
> >+      port_t *port = &SLAVE_AD_INFO(slave)->port;
> >+
> >+      return bond_slave_can_tx(slave) &&
> >+             (port->actor_oper_port_state & port->partner_oper.port_state &
> >+              BOND_3AD_PORT_OPERATIONAL) == BOND_3AD_PORT_OPERATIONAL;
> >+}

	needing to test the port_state; if the logic cycles through the
ports of the active aggregator, then it shouldn't need to check the
state.  Any port in the active aggregator should be able to transmit,
even if it is in the "no LACP peer" fallback situation (with one
exception, noted below).

	I think something like:

	first_slave = bond_first_slave_rcu(bond);
	agg = __get_active_agg(&(SLAVE_AD_INFO(first_slave)->aggregator));
	for (port = agg->lag_ports; port;
	     port = port->next_port_in_aggregator) {
		if (bond_slave_can_tx(port->slave))
			/* use this one */
	}

	would do roughly what you're describing, although it will always
choose the first available port of the active aggregator, even if there
are more than one.

	Generally, slaves that are not up will not remain in the active
aggregator, but the slave_can_tx test will cover the window between when
a slave goes carrier down and the 3ad logic removes it from the active
agg.

	-J

---
	-Jay Vosburgh, jay.vosburgh@...onical.com
--
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