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: <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