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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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