| 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
| ||
|
Date: Sat, 7 Feb 2015 21:38:43 -0800
From: Mahesh Bandewar <maheshb@...gle.com>
To: Veaceslav Falico <vfalico@...il.com>
Cc: Jay Vosburgh <j.vosburgh@...il.com>,
Andy Gospodarek <andy@...yhouse.net>,
Nikolay Aleksandrov <nikolay@...hat.com>,
David Miller <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>,
Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH next 2/6] bonding: implement bond_poll_controller()
On Sat, Feb 7, 2015 at 1:04 AM, Veaceslav Falico <vfalico@...il.com> wrote:
> On Fri, Feb 06, 2015 at 04:51:51PM -0800, Mahesh Bandewar wrote:
>>
>> This patches implements the poll_controller support for all
>> bonding driver. If the slaves have poll_controller net_op defined,
>> this implementation calls them. This is mode agnostic implementation
>> and iterates through all slaves (based on mode) and calls respective
>> handler.
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@...gle.com>
>
>
> Really good patchset, thank you. Two nitpicks below, nothing serious. Also,
> please, add Doc/bonding.txt changes for every user-visible
> change/enhancement, this doc is a go-to for a lot of users.
>
> Otherwise, with the doc and other issues, pointed by Jay and Andy, fixed, I
> guess it's good to go. The 0/ patch would be welcome too, btw.
>
>
>> ---
>> drivers/net/bonding/bond_3ad.c | 24 +++++++++++++++++++++
>> drivers/net/bonding/bond_main.c | 47
>> +++++++++++++++++++++++++++++++++++++++++
>> include/net/bond_3ad.h | 1 +
>> 3 files changed, 72 insertions(+)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c
>> b/drivers/net/bonding/bond_3ad.c
>> index 9b436696b95e..14f2ebe786c5 100644
>> --- a/drivers/net/bonding/bond_3ad.c
>> +++ b/drivers/net/bonding/bond_3ad.c
>> @@ -2477,6 +2477,30 @@ int bond_3ad_get_active_agg_info(struct bonding
>> *bond, struct ad_info *ad_info)
>> return ret;
>> }
>>
>> +#define BOND_3AD_PORT_OPERATIONAL \
>> + (AD_STATE_DISTRIBUTING | AD_STATE_COLLECTING | \
>> + AD_STATE_SYNCHRONIZATION | AD_STATE_AGGREGATION)
>> +
>> +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;
>> +}
>> +
>> +/* bond_3ad_port_is_active - check if a slave port is active or not. A
>> port
>> + * is active when it can forward traffic.
>> + *
>> + * @slave: slave port to check state for.
>> + * Returns: 0 if not active else is active.
>> + */
>> +int bond_3ad_port_is_active(struct slave *slave)
>> +{
>> + return bond_3ad_port_operational(slave);
>> +}
>> +
>> int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding *bond,
>> struct slave *slave)
>> {
>> diff --git a/drivers/net/bonding/bond_main.c
>> b/drivers/net/bonding/bond_main.c
>> index c9e519cb9214..a50ec87486f3 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -928,6 +928,53 @@ static inline void slave_disable_netpoll(struct slave
>> *slave)
>>
>> static void bond_poll_controller(struct net_device *bond_dev)
>> {
>> + struct bonding *bond = netdev_priv(bond_dev);
>> + struct slave *slave = NULL;
>> + struct list_head *iter;
>> + struct ad_info ad_info;
>> + struct aggregator *agg;
>> + const struct net_device_ops *ops;
>> + bool call_slave_netpoll;
>> +
>> + 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) {
>> + call_slave_netpoll = false;
>> + switch (BOND_MODE(bond)) {
>> + case BOND_MODE_8023AD:
>> + agg = SLAVE_AD_INFO(slave)->port.aggregator;
>> + if (!bond_slave_is_up(slave))
>
>
> bond_3ad_port_is_active() already contains the check for being up.
>
>> + break;
>> + if (agg && agg->aggregator_identifier !=
>
>
> Hm, should we poll it without an aggregator?
>
I think I missed this comment earlier, but it would be wrong. I mean
we have to perform this check otherwise it would be wrong.
>> + ad_info.aggregator_id)
>> + break;
>> + if (!bond_3ad_port_is_active(slave) &&
>> + ad_info.ports != 1)
>> + break;
>> +
>> + call_slave_netpoll = true;
>> + break;
>> + default:
>> + if (bond_slave_is_up(slave))
>> + call_slave_netpoll = true;
>> + break;
>> + }
>> +
>> + if (call_slave_netpoll) {
>> + ops = slave->dev->netdev_ops;
>> + if (ops->ndo_poll_controller) {
>
>
> It's weird to see this check so down the road. Maybe reorg the logic to
> something like:
>
> bond_for_each_slave() {
> if (!bond_slave_is_up() || !bond_slave_has_ndo_poll())
> continue;
>
> if (BOND_MODE(bond) == 3AD && !bond_3ad_port_is_active())
> continue;
>
> slave-do_ndo_poll_stuff();
> }
>
> Just as a thought, might be easier to read/shorter.
>
I tried this and it does save one bool variable but not much
different. Here it is -
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;
}
ni = rcu_dereference_bh(slave->dev->npinfo);
if (down_trylock(&ni->dev_lock))
continue;
ops->ndo_poll_controller(slave->dev);
up(&ni->dev_lock);
}
I'm not sure if it's any different! BTW I did keep the aggregator
check since I'm not convinced that it's OK to eliminate the check.
>
>> + struct netpoll_info *ni =
>> +
>> rcu_dereference_bh(slave->dev->npinfo);
>> +
>> + if (down_trylock(&ni->dev_lock))
>> + continue;
>> + ops->ndo_poll_controller(slave->dev);
>> + up(&ni->dev_lock);
>> + }
>> + }
>> + }
>> }
>>
>> static void bond_netpoll_cleanup(struct net_device *bond_dev)
>> diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h
>> index f04cdbb7848e..6c455c646d61 100644
>> --- a/include/net/bond_3ad.h
>> +++ b/include/net/bond_3ad.h
>> @@ -278,5 +278,6 @@ int bond_3ad_lacpdu_recv(const struct sk_buff *skb,
>> struct bonding *bond,
>> struct slave *slave);
>> int bond_3ad_set_carrier(struct bonding *bond);
>> void bond_3ad_update_lacp_rate(struct bonding *bond);
>> +int bond_3ad_port_is_active(struct slave *slave);
>> #endif /* _NET_BOND_3AD_H */
>>
>> --
>> 2.2.0.rc0.207.ga3a616c
>>
>
--
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