| 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: <20150207090400.GA11343@vps.falico.eu>
Date: Sat, 7 Feb 2015 10:04:00 +0100
From: Veaceslav Falico <vfalico@...il.com>
To: Mahesh Bandewar <maheshb@...gle.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 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?
>+ 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.
>+ 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