netdev - Re: [PATCH next v3 2/6] bonding: implement bond_poll

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]

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