[PATCH net-next 0/9] bonding: remove bond_next_slave()

[PATCH net-next 0/9] bonding: remove bond_next_slave()

[Veaceslav Falico]

(on top of "[PATCH net-next 0/2] bonding: fix 3ad slave (de)init" - the
patchset is essential)

Hi,

As Ben Hutchings and Nikolay Aleksandrov correctly noted -
bond_next_slave() already is not O(1), but rather O(n), so it's only useful
for one-off operations and shouldn't be used widely - for example, for list
traversal, which will take O(n^2) time, which will be disastrous for any
hot path with a large number of slaves.

Currently, bond_next_slave() is used several times in 802.3ad and for
seq_read - bond_info_seq_next().

The 802.3ad part uses it only in constructs like:

for (X = __get_first_X(); X; X = __get_next_X()) {

where __get_next_X() uses bond_next_slave().

This for can (and should) actually be replaced by the standard

bond_for_each_slave(bond, slave, iter) {
	X = __get_X_by_slave(slave);

it's faster, easier to read, debug and maintain. Also, removes a lot of
code lines.

After replacing it, the only user of bond_next_slave() is
bond_info_seq_next() - which can, actually, implement it by itself, and not
call another function.

So, that way, we can completely remove the bond_next_slave(), cleanup and
optimize a bit.

p.s. the 802.3ad code is horrible, both style-wise and from the logical
point of view - so I've decided to *not* change anything except that what
this patch is intended to provide. The cleanup and some refactoring should
be done in another patchset, which I've began to work on already.

Thank you!

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

---
 drivers/net/bonding/bond_3ad.c    | 137 ++++++++++++++------------------------
 drivers/net/bonding/bond_main.c   |   2 +-
 drivers/net/bonding/bond_procfs.c |  16 +++--
 drivers/net/bonding/bonding.h     |  31 ---------
 4 files changed, 62 insertions(+), 124 deletions(-)

[PATCH net-next 1/9] bonding: remove __get_next_port()

[Veaceslav Falico]

Currently this function is only used in constructs like

for (port = __get_first_port(bond); port; port = __get_next_port(port))

which is basicly the same as

bond_for_each_slave(bond, slave, iter) {
	port = &(SLAVE_AD_INFO(slave).port);

but a more time consuming.

Remove the function and convert the users to bond_for_each_slave().

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_3ad.c | 34 +++++++++-------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 2715ea8..c1535f8 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -149,28 +149,6 @@ static inline struct port *__get_first_port(struct bonding *bond)
 }
 
 /**
- * __get_next_port - get the next port in the bond
- * @port: the port we're looking at
- *
- * Return the port of the slave that is next in line of @port's slave in the
- * bond, or %NULL if it can't be found.
- */
-static inline struct port *__get_next_port(struct port *port)
-{
-	struct bonding *bond = __get_bond_by_port(port);
-	struct slave *slave = port->slave, *slave_next;
-
-	// If there's no bond for this port, or this is the last slave
-	if (bond == NULL)
-		return NULL;
-	slave_next = bond_next_slave(bond, slave);
-	if (!slave_next || bond_is_first_slave(bond, slave_next))
-		return NULL;
-
-	return &(SLAVE_AD_INFO(slave_next).port);
-}
-
-/**
  * __get_first_agg - get the first aggregator in the bond
  * @bond: the bond we're looking at
  *
@@ -2113,8 +2091,10 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
 {
 	struct bonding *bond = container_of(work, struct bonding,
 					    ad_work.work);
-	struct port *port;
 	struct aggregator *aggregator;
+	struct list_head *iter;
+	struct slave *slave;
+	struct port *port;
 
 	read_lock(&bond->lock);
 
@@ -2139,7 +2119,8 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
 	}
 
 	// for each port run the state machines
-	for (port = __get_first_port(bond); port; port = __get_next_port(port)) {
+	bond_for_each_slave(bond, slave, iter) {
+		port = &(SLAVE_AD_INFO(slave).port);
 		if (!port->slave) {
 			pr_warning("%s: Warning: Found an uninitialized port\n",
 				   bond->dev->name);
@@ -2384,9 +2365,12 @@ int __bond_3ad_get_active_agg_info(struct bonding *bond,
 				   struct ad_info *ad_info)
 {
 	struct aggregator *aggregator = NULL;
+	struct list_head *iter;
+	struct slave *slave;
 	struct port *port;
 
-	for (port = __get_first_port(bond); port; port = __get_next_port(port)) {
+	bond_for_each_slave(bond, slave, iter) {
+		port = &(SLAVE_AD_INFO(slave).port);
 		if (port->aggregator && port->aggregator->is_active) {
 			aggregator = port->aggregator;
 			break;
-- 
1.8.4

[PATCH net-next 2/9] bonding: remove __get_first_port()

[Veaceslav Falico]

Currently we have only one user of it, so it's kind of useless and just
obfusicates things.

Remove it and move the logic to the only user -
bond_3ad_state_machine_handler().

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_3ad.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index c1535f8..0f86d2b 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -136,19 +136,6 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
 }
 
 /**
- * __get_first_port - get the first port in the bond
- * @bond: the bond we're looking at
- *
- * Return the port of the first slave in @bond, or %NULL if it can't be found.
- */
-static inline struct port *__get_first_port(struct bonding *bond)
-{
-	struct slave *first_slave = bond_first_slave(bond);
-
-	return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
-}
-
-/**
  * __get_first_agg - get the first aggregator in the bond
  * @bond: the bond we're looking at
  *
@@ -2104,8 +2091,11 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
 
 	// check if agg_select_timer timer after initialize is timed out
 	if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) {
+		slave = bond_first_slave(bond);
+		port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL;
+
 		// select the active aggregator for the bond
-		if ((port = __get_first_port(bond))) {
+		if (port) {
 			if (!port->slave) {
 				pr_warning("%s: Warning: bond's first port is uninitialized\n",
 					   bond->dev->name);
-- 
1.8.4

[PATCH net-next 3/9] bonding: make ad_port_selection_logic() use bond_for_each_slave()

[Veaceslav Falico]

Currently, ad_port_selection_logic() uses

for (aggregator = __get_first_agg(port); aggregator;
     aggregator = __get_next_agg(aggregator)) {

construct, however it's suboptimal, difficult to read and understand.

Change it to a standard bond_for_each_slave(), so that we won't need
__get_first/next_agg() and have it more readable.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_3ad.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 0f86d2b..8b64ed4 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1239,12 +1239,17 @@ static void ad_port_selection_logic(struct port *port)
 {
 	struct aggregator *aggregator, *free_aggregator = NULL, *temp_aggregator;
 	struct port *last_port = NULL, *curr_port;
+	struct list_head *iter;
+	struct bonding *bond;
+	struct slave *slave;
 	int found = 0;
 
 	// if the port is already Selected, do nothing
 	if (port->sm_vars & AD_PORT_SELECTED)
 		return;
 
+	bond = __get_bond_by_port(port);
+
 	// if the port is connected to other aggregator, detach it
 	if (port->aggregator) {
 		// detach the port from its former aggregator
@@ -1285,8 +1290,8 @@ static void ad_port_selection_logic(struct port *port)
 		}
 	}
 	// search on all aggregators for a suitable aggregator for this port
-	for (aggregator = __get_first_agg(port); aggregator;
-	     aggregator = __get_next_agg(aggregator)) {
+	bond_for_each_slave(bond, slave, iter) {
+		aggregator = &(SLAVE_AD_INFO(slave).aggregator);
 
 		// keep a free aggregator for later use(if needed)
 		if (!aggregator->lag_ports) {
-- 
1.8.4

[PATCH net-next 4/9] bonding: make __get_active_agg() use bond_for_each_slave()

[Veaceslav Falico]

Currently we're relying on suboptimal construct

for (; aggregator; aggregator = __get_next_agg(aggregator)) {

where aggregator is an argument of __get_active_agg() which is _always_ the
first slave's aggregator - judging by all the callers, comments in the
ad_agg_selection_logic() and by logic.

Convert it to use the standard bond_for_each_slave().

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_3ad.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 8b64ed4..1109d82 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -720,16 +720,15 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
  */
 static struct aggregator *__get_active_agg(struct aggregator *aggregator)
 {
-	struct aggregator *retval = NULL;
+	struct bonding *bond = aggregator->slave->bond;
+	struct list_head *iter;
+	struct slave *slave;
 
-	for (; aggregator; aggregator = __get_next_agg(aggregator)) {
-		if (aggregator->is_active) {
-			retval = aggregator;
-			break;
-		}
-	}
+	bond_for_each_slave(bond, slave, iter)
+		if (SLAVE_AD_INFO(slave).aggregator.is_active)
+			return &(SLAVE_AD_INFO(slave).aggregator);
 
-	return retval;
+	return NULL;
 }
 
 /**
-- 
1.8.4

[PATCH net-next 5/9] bonding: make ad_agg_selection_logic() use bond_for_each_slave()

[Veaceslav Falico]

Convert all instances of

for (agg = __get_first_agg(); agg; agg = __get_next_port)

to the standard bond_for_each_slave(). Also, remove the useless checks
before calling bond_3ad_set_carrier() - if we have something NULL - it
would fire long ago, in __get_first/next_port(), per example.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_3ad.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 1109d82..6cd1444 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1484,19 +1484,23 @@ static int agg_device_up(const struct aggregator *agg)
 static void ad_agg_selection_logic(struct aggregator *agg)
 {
 	struct aggregator *best, *active, *origin;
+	struct bonding *bond = agg->slave->bond;
+	struct list_head *iter;
+	struct slave *slave;
 	struct port *port;
 
 	origin = agg;
 	active = __get_active_agg(agg);
 	best = (active && agg_device_up(active)) ? active : NULL;
 
-	do {
+	bond_for_each_slave(bond, slave, iter) {
+		agg = &(SLAVE_AD_INFO(slave).aggregator);
+
 		agg->is_active = 0;
 
 		if (agg->num_of_ports && agg_device_up(agg))
 			best = ad_agg_selection_test(best, agg);
-
-	} while ((agg = __get_next_agg(agg)));
+	}
 
 	if (best &&
 	    __get_agg_selection_mode(best->lag_ports) == BOND_AD_STABLE) {
@@ -1534,8 +1538,8 @@ static void ad_agg_selection_logic(struct aggregator *agg)
 			 best->lag_ports, best->slave,
 			 best->slave ? best->slave->dev->name : "NULL");
 
-		for (agg = __get_first_agg(best->lag_ports); agg;
-		     agg = __get_next_agg(agg)) {
+		bond_for_each_slave(bond, slave, iter) {
+			agg = &(SLAVE_AD_INFO(slave).aggregator);
 
 			pr_debug("Agg=%d; P=%d; a k=%d; p k=%d; Ind=%d; Act=%d\n",
 				 agg->aggregator_identifier, agg->num_of_ports,
@@ -1583,13 +1587,7 @@ static void ad_agg_selection_logic(struct aggregator *agg)
 		}
 	}
 
-	if (origin->slave) {
-		struct bonding *bond;
-
-		bond = bond_get_bond_by_slave(origin->slave);
-		if (bond)
-			bond_3ad_set_carrier(bond);
-	}
+	bond_3ad_set_carrier(bond);
 }
 
 /**
-- 
1.8.4

[PATCH net-next 6/9] bonding: make bond_3ad_unbind_slave() use bond_for_each_slave()

[Veaceslav Falico]

Convert all instances of

for (agg = __get_first_agg(); agg; agg = __get_next_port)

to the standard bond_for_each_slave().

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_3ad.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 6cd1444..6dbb84d 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1936,6 +1936,9 @@ void bond_3ad_unbind_slave(struct slave *slave)
 	struct port *port, *prev_port, *temp_port;
 	struct aggregator *aggregator, *new_aggregator, *temp_aggregator;
 	int select_new_active_agg = 0;
+	struct bonding *bond = slave->bond;
+	struct slave *slave_iter;
+	struct list_head *iter;
 
 	// find the aggregator related to this slave
 	aggregator = &(SLAVE_AD_INFO(slave).aggregator);
@@ -1965,14 +1968,16 @@ void bond_3ad_unbind_slave(struct slave *slave)
 		// reason to search for new aggregator, and that we will find one
 		if ((aggregator->lag_ports != port) || (aggregator->lag_ports->next_port_in_aggregator)) {
 			// find new aggregator for the related port(s)
-			new_aggregator = __get_first_agg(port);
-			for (; new_aggregator; new_aggregator = __get_next_agg(new_aggregator)) {
+			bond_for_each_slave(bond, slave_iter, iter) {
+				new_aggregator = &(SLAVE_AD_INFO(slave_iter).aggregator);
 				// if the new aggregator is empty, or it is connected to our port only
 				if (!new_aggregator->lag_ports
 				    || ((new_aggregator->lag_ports == port)
 					&& !new_aggregator->lag_ports->next_port_in_aggregator))
 					break;
 			}
+			if (!slave_iter)
+				new_aggregator = NULL;
 			// if new aggregator found, copy the aggregator's parameters
 			// and connect the related lag_ports to the new aggregator
 			if ((new_aggregator) && ((!new_aggregator->lag_ports) || ((new_aggregator->lag_ports == port) && !new_aggregator->lag_ports->next_port_in_aggregator))) {
@@ -2032,8 +2037,8 @@ void bond_3ad_unbind_slave(struct slave *slave)
 
 	pr_debug("Unbinding port %d\n", port->actor_port_number);
 	// find the aggregator that this port is connected to
-	temp_aggregator = __get_first_agg(port);
-	for (; temp_aggregator; temp_aggregator = __get_next_agg(temp_aggregator)) {
+	bond_for_each_slave(bond, slave_iter, iter) {
+		temp_aggregator = &(SLAVE_AD_INFO(slave_iter).aggregator);
 		prev_port = NULL;
 		// search the port in the aggregator's related ports
 		for (temp_port = temp_aggregator->lag_ports; temp_port;
-- 
1.8.4

[PATCH net-next 7/9] bonding: remove unused __get_next_agg()

[Veaceslav Falico]

It has no users, so it's safe to remove it completely.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_3ad.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 6dbb84d..c62606a 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -155,28 +155,6 @@ static inline struct aggregator *__get_first_agg(struct port *port)
 	return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
 }
 
-/**
- * __get_next_agg - get the next aggregator in the bond
- * @aggregator: the aggregator we're looking at
- *
- * Return the aggregator of the slave that is next in line of @aggregator's
- * slave in the bond, or %NULL if it can't be found.
- */
-static inline struct aggregator *__get_next_agg(struct aggregator *aggregator)
-{
-	struct slave *slave = aggregator->slave, *slave_next;
-	struct bonding *bond = bond_get_bond_by_slave(slave);
-
-	// If there's no bond for this aggregator, or this is the last slave
-	if (bond == NULL)
-		return NULL;
-	slave_next = bond_next_slave(bond, slave);
-	if (!slave_next || bond_is_first_slave(bond, slave_next))
-		return NULL;
-
-	return &(SLAVE_AD_INFO(slave_next).aggregator);
-}
-
 /*
  * __agg_has_partner
  *
-- 
1.8.4

[PATCH net-next 8/9] bonding: don't use bond_next_slave() in bond_info_seq_next()

[Veaceslav Falico]

We don't need the circular loop there and it's the only current user of
bond_next_slave() - so just use the standard bond_for_each_slave().

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_procfs.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
index 7af5646..fb868d6 100644
--- a/drivers/net/bonding/bond_procfs.c
+++ b/drivers/net/bonding/bond_procfs.c
@@ -31,17 +31,25 @@ static void *bond_info_seq_start(struct seq_file *seq, loff_t *pos)
 static void *bond_info_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 {
 	struct bonding *bond = seq->private;
-	struct slave *slave = v;
+	struct list_head *iter;
+	struct slave *slave;
+	bool found = false;
 
 	++*pos;
 	if (v == SEQ_START_TOKEN)
 		return bond_first_slave(bond);
 
-	if (bond_is_last_slave(bond, slave))
+	if (bond_is_last_slave(bond, v))
 		return NULL;
-	slave = bond_next_slave(bond, slave);
 
-	return slave;
+	bond_for_each_slave(bond, slave, iter) {
+		if (found)
+			return slave;
+		if (slave == v)
+			found = true;
+	}
+
+	return NULL;
 }
 
 static void bond_info_seq_stop(struct seq_file *seq, void *v)
-- 
1.8.4

[PATCH net-next 9/9] bonding: remove bond_next_slave()

[Veaceslav Falico]

There are no users left, so it's safe to remove.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bonding.h | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 5b71601..713b6af 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -89,9 +89,6 @@
 #define bond_is_first_slave(bond, pos) (pos == bond_first_slave(bond))
 #define bond_is_last_slave(bond, pos) (pos == bond_last_slave(bond))
 
-/* Since bond_first/last_slave can return NULL, these can return NULL too */
-#define bond_next_slave(bond, pos) __bond_next_slave(bond, pos)
-
 /**
  * bond_for_each_slave - iterate over all slaves
  * @bond:	the bond holding this list
@@ -244,34 +241,6 @@ struct bonding {
 	((struct slave *) rtnl_dereference(dev->rx_handler_data))
 
 /**
- * __bond_next_slave - get the next slave after the one provided
- * @bond - bonding struct
- * @slave - the slave provided
- *
- * Returns the next slave after the slave provided, first slave if the
- * slave provided is the last slave and NULL if slave is not found
- */
-static inline struct slave *__bond_next_slave(struct bonding *bond,
-					      struct slave *slave)
-{
-	struct slave *slave_iter;
-	struct list_head *iter;
-	bool found = false;
-
-	netdev_for_each_lower_private(bond->dev, slave_iter, iter) {
-		if (found)
-			return slave_iter;
-		if (slave_iter == slave)
-			found = true;
-	}
-
-	if (found)
-		return bond_first_slave(bond);
-
-	return NULL;
-}
-
-/**
  * Returns NULL if the net_device does not belong to any of the bond's slaves
  *
  * Caller must hold bond lock for read
-- 
1.8.4

RE: [PATCH net-next 2/9] bonding: remove __get_first_port()

[David Laight]

> @@ -2104,8 +2091,11 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
> 
>  	// check if agg_select_timer timer after initialize is timed out
>  	if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) {
> +		slave = bond_first_slave(bond);
> +		port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL;
> +
>  		// select the active aggregator for the bond
> -		if ((port = __get_first_port(bond))) {
> +		if (port) {
>  			if (!port->slave) {
>  				pr_warning("%s: Warning: bond's first port is uninitialized\n",
>  					   bond->dev->name);
> --

Looks like that could be:
		slave = bond_first_slave(bond);
		if (slave) {
			port = SLAVE_AD_INFO(slave).port;
and I assume 'slave == port->slave' so there is no need for the latter check?

	David

Re: [PATCH net-next 2/9] bonding: remove __get_first_port()

[Veaceslav Falico]

On Fri, Sep 27, 2013 at 03:50:12PM +0100, David Laight wrote:
>> @@ -2104,8 +2091,11 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
>>
>>  	// check if agg_select_timer timer after initialize is timed out
>>  	if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) {
>> +		slave = bond_first_slave(bond);
>> +		port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL;
>> +
>>  		// select the active aggregator for the bond
>> -		if ((port = __get_first_port(bond))) {
>> +		if (port) {
>>  			if (!port->slave) {
>>  				pr_warning("%s: Warning: bond's first port is uninitialized\n",
>>  					   bond->dev->name);
>> --
>
>Looks like that could be:
>		slave = bond_first_slave(bond);
>		if (slave) {
>			port = SLAVE_AD_INFO(slave).port;
>and I assume 'slave == port->slave' so there is no need for the latter check?

I've also fallen to this trap at first - slave->port can (virtually) be
NULL, and this way we'll panic on "if (!port->slave)".

>
>	David
>
>
>

Re: [PATCH net-next 2/9] bonding: remove __get_first_port()

[Veaceslav Falico]

On Fri, Sep 27, 2013 at 04:58:25PM +0200, Veaceslav Falico wrote:
>On Fri, Sep 27, 2013 at 03:50:12PM +0100, David Laight wrote:
>>>@@ -2104,8 +2091,11 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
>>>
>>> 	// check if agg_select_timer timer after initialize is timed out
>>> 	if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) {
>>>+		slave = bond_first_slave(bond);
>>>+		port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL;
>>>+
>>> 		// select the active aggregator for the bond
>>>-		if ((port = __get_first_port(bond))) {
>>>+		if (port) {
>>> 			if (!port->slave) {
>>> 				pr_warning("%s: Warning: bond's first port is uninitialized\n",
>>> 					   bond->dev->name);
>>>--
>>
>>Looks like that could be:
>>		slave = bond_first_slave(bond);
>>		if (slave) {
>>			port = SLAVE_AD_INFO(slave).port;
>>and I assume 'slave == port->slave' so there is no need for the latter check?
>
>I've also fallen to this trap at first - slave->port can (virtually) be
>NULL, and this way we'll panic on "if (!port->slave)".

Err, forgot to address the 'slave == port->slave' - yes, virtually it
should be - if it's initialized (and, it should be) - however, as I've
stated in the cover letter - there are *tons* of cleanups/optimizations of
these kind that might be done here - and not to mix cleanups/optimizations
with the thing that these patches should do (remove bond_next_slave) - I've
decided to leave it as close to the original as possible.

>
>>
>>	David
>>
>>
>>

Re: [PATCH net-next 0/9] bonding: remove bond_next_slave()

[David Miller]

From: Veaceslav Falico <vfalico@redhat.com>
Date: Fri, 27 Sep 2013 16:11:56 +0200

> (on top of "[PATCH net-next 0/2] bonding: fix 3ad slave (de)init" - the
> patchset is essential)
> 
> Hi,
> 
> As Ben Hutchings and Nikolay Aleksandrov correctly noted -
> bond_next_slave() already is not O(1), but rather O(n), so it's only useful
> for one-off operations and shouldn't be used widely - for example, for list
> traversal, which will take O(n^2) time, which will be disastrous for any
> hot path with a large number of slaves.
> 
> Currently, bond_next_slave() is used several times in 802.3ad and for
> seq_read - bond_info_seq_next().
> 
> The 802.3ad part uses it only in constructs like:
> 
> for (X = __get_first_X(); X; X = __get_next_X()) {
> 
> where __get_next_X() uses bond_next_slave().
> 
> This for can (and should) actually be replaced by the standard
> 
> bond_for_each_slave(bond, slave, iter) {
> 	X = __get_X_by_slave(slave);
> 
> it's faster, easier to read, debug and maintain. Also, removes a lot of
> code lines.
> 
> After replacing it, the only user of bond_next_slave() is
> bond_info_seq_next() - which can, actually, implement it by itself, and not
> call another function.
> 
> So, that way, we can completely remove the bond_next_slave(), cleanup and
> optimize a bit.
> 
> p.s. the 802.3ad code is horrible, both style-wise and from the logical
> point of view - so I've decided to *not* change anything except that what
> this patch is intended to provide. The cleanup and some refactoring should
> be done in another patchset, which I've began to work on already.
> 
> Thank you!
> 
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

Also applied, thanks.