bonding:use latest lacp_rate and ad_select and delete unused ad_timer and arp_mon_pt

PATCH In review Weiping Pan 3 participants 10 messages Jun 9, 2011

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

Weiping Pan

author

2011-06-09 07:18 UTC #0 lore raw

v4 cover bonding:use latest lacp_rate and ad_select and delete unused ad_timer and arp_mon_pt

There is bug that when you modify lacp_rate via sysfs, 802.3ad won't use the new value of lacp_rate to transmit packets. This is because port->actor_oper_port_state isn't changed.

As for ad_select, it can work,
but both struct bond_params and ad_bond_info have lacp_fast and ad_select, they are duplicate and need extra synchronization.
802.3ad can get them from bond_params directly every time.

And ad_timer and arp_mon_pt aren't used any more, just delete them.

changelog:
v2:
add bond_3ad_update_lacp_rate() as a helper function, and hold bond->lock when iterates slave list.

v3:
delete duplicate lacp_fast and agg_select_mode from struct ad_bond_info.

v4:
delete unused ad_timer and arp_mon_pt.

Weiping Pan (5):
bonding: make 802.3ad use latest lacp_rate bonding:delete lacp_fast from ad_bond_info bonding:delete agg_select_mode from ad_bond_info
bonding: delete unused ad_timer
bonding: delete unused arp_mon_pt

 drivers/net/bonding/bond_3ad.c   |   41 ++++++++++++++++++++++++++++++++-----
 drivers/net/bonding/bond_3ad.h   |    8 +-----
 drivers/net/bonding/bond_main.c  |    3 +-
 drivers/net/bonding/bond_sysfs.c |    1 +
 drivers/net/bonding/bonding.h    |    1 -
 5 files changed, 39 insertions(+), 15 deletions(-)

Signature · 1 line

1.7.4.4

Weiping Pan

author

2011-06-09 07:18 UTC #1 lore raw

PATCH v4 1/5 bonding: make 802.3ad use latest lacp_rate

There is bug that when you modify lacp_rate via sysfs, 802.3ad won't use the new value of lacp_rate to transmit packets. This is because port->actor_oper_port_state isn't changed.

Signed-off-by Weiping Pan <[email protected]>

---
 drivers/net/bonding/bond_3ad.c   |   31 +++++++++++++++++++++++++++++++
 drivers/net/bonding/bond_3ad.h   |    1 +
 drivers/net/bonding/bond_sysfs.c |    1 +
 3 files changed, 33 insertions(+), 0 deletions(-)

drivers/net/bonding/bond_3ad.c +31 -0

index c7537abc..4512bc4 100644
@@ -2473,3 +2473,34 @@ void bond_3ad_lacpdu_recv(struct sk_buff *skb, struct bonding *bond,
 	bond_3ad_rx_indication((struct lacpdu *) skb->data, slave, skb->len);
 	read_unlock(&bond->lock);
 }
+
+/*
+ * When modify lacp_rate parameter via sysfs,
+ * update actor_oper_port_state of each port.
+ *
+ * Hold slave->state_machine_lock,
+ * so we can modify port->actor_oper_port_state,
+ * no matter bond is up or down.
+ */
+void bond_3ad_update_lacp_rate(struct bonding *bond)
+{
+	int i;
+	struct slave *slave;
+	struct port *port = NULL;
+	int lacp_fast;
+
+	read_lock(&bond->lock);
+	lacp_fast = bond->params.lacp_fast;
+
+	bond_for_each_slave(bond, slave, i) {
+		port = &(SLAVE_AD_INFO(slave).port);
+		__get_state_machine_lock(port);
+		if (lacp_fast)
+			port->actor_oper_port_state |= AD_STATE_LACP_TIMEOUT;
+		else
+			port->actor_oper_port_state &= ~AD_STATE_LACP_TIMEOUT;
+		__release_state_machine_lock(port);
+	}
+
+	read_unlock(&bond->lock);
+}

drivers/net/bonding/bond_3ad.h +1 -0

index 0ee3f16..e466faf 100644
@@ -282,5 +282,6 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
 void bond_3ad_lacpdu_recv(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);
 #endif //__BOND_3AD_H__

drivers/net/bonding/bond_sysfs.c +1 -0

index 88fcb25..03d1196 100644
@@ -804,6 +804,7 @@ static ssize_t bonding_store_lacp(struct device *d,
 
 	if ((new_value == 1) || (new_value == 0)) {
 		bond->params.lacp_fast = new_value;
+		bond_3ad_update_lacp_rate(bond);
 		pr_info("%s: Setting LACP rate to %s (%d).\n",
 			bond->dev->name, bond_lacp_tbl[new_value].modename,
 			new_value);

Signature · 1 line

1.7.4.4

Weiping Pan

author

2011-06-09 07:18 UTC #2 lore raw

PATCH v4 2/5 bonding:delete lacp_fast from ad_bond_info

In reply to Weiping Pan

These is also a bug, that if you modify lacp_rate via sysfs, and add new slaves in bonding, new slaves won't use the latest lacp_rate, since ad_bond_info->lacp_fast is initialized only once,
in bond_3ad_initialize().

Since both struct bond_params and ad_bond_info have lacp_fast, they are duplicate and need extra synchronization.

bond_3ad_bind_slave() can use bond_params->lacp_fast to initialize port. So we can just remove lacp_fast from struct ad_bond_info.

Signed-off-by Weiping Pan <[email protected]>

---
 drivers/net/bonding/bond_3ad.c  |    7 +++----
 drivers/net/bonding/bond_3ad.h  |    5 +----
 drivers/net/bonding/bond_main.c |    3 +--
 3 files changed, 5 insertions(+), 10 deletions(-)

drivers/net/bonding/bond_3ad.c +3 -4

index 4512bc4..013a801 100644
@@ -1868,11 +1868,10 @@ static u16 aggregator_identifier;
  * bond_3ad_initialize - initialize a bond's 802.3ad parameters and structures
  * @bond: bonding struct to work on
  * @tick_resolution: tick duration (millisecond resolution)
- * @lacp_fast: boolean. whether fast periodic should be used
  *
  * Can be called only after the mac address of the bond is set.
  */
-void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution, int lacp_fast)
+void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution)
 {
 	// check that the bond is not initialized yet
 	if (MAC_ADDRESS_COMPARE(&(BOND_AD_INFO(bond).system.sys_mac_addr),

@@ -1880,7 +1879,6 @@ void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution, int lacp_fas
 
 		aggregator_identifier = 0;
 
-		BOND_AD_INFO(bond).lacp_fast = lacp_fast;
 		BOND_AD_INFO(bond).system.sys_priority = 0xFFFF;
 		BOND_AD_INFO(bond).system.sys_mac_addr = *((struct mac_addr *)bond->dev->dev_addr);

@@ -1903,6 +1901,7 @@ void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution, int lacp_fas
 int bond_3ad_bind_slave(struct slave *slave)
 {
 	struct bonding *bond = bond_get_bond_by_slave(slave);
+	int lacp_fast = bond->params.lacp_fast;
 	struct port *port;
 	struct aggregator *aggregator;

@@ -1918,7 +1917,7 @@ int bond_3ad_bind_slave(struct slave *slave)
 		// port initialization
 		port = &(SLAVE_AD_INFO(slave).port);
 
-		ad_initialize_port(port, BOND_AD_INFO(bond).lacp_fast);
+		ad_initialize_port(port, lacp_fast);
 
 		port->slave = slave;
 		port->actor_port_number = SLAVE_AD_INFO(slave).id;

drivers/net/bonding/bond_3ad.h +1 -4

index e466faf..9782785 100644
@@ -254,9 +254,6 @@ struct ad_bond_info {
 	struct ad_system system;	    /* 802.3ad system structure */
 	u32 agg_select_timer;	    // Timer to select aggregator after all adapter's hand shakes
 	u32 agg_select_mode;	    // Mode of selection of active aggregator(bandwidth/count)
-	int lacp_fast;		/* whether fast periodic tx should be
-				 * requested
-				 */
 	struct timer_list ad_timer;
 };

@@ -269,7 +266,7 @@ struct ad_slave_info {
 };
 
 // ================= AD Exported functions to the main bonding code ==================
-void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution, int lacp_fast);
+void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution);
 int  bond_3ad_bind_slave(struct slave *slave);
 void bond_3ad_unbind_slave(struct slave *slave);
 void bond_3ad_state_machine_handler(struct work_struct *);

drivers/net/bonding/bond_main.c +1 -2

index 716c852..bb1af9c 100644
@@ -1843,8 +1843,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 			/* Initialize AD with the number of times that the AD timer is called in 1 second
 			 * can be called only after the mac address of the bond is set
 			 */
-			bond_3ad_initialize(bond, 1000/AD_TIMER_INTERVAL,
-					    bond->params.lacp_fast);
+			bond_3ad_initialize(bond, 1000/AD_TIMER_INTERVAL);
 		} else {
 			SLAVE_AD_INFO(new_slave).id =
 				SLAVE_AD_INFO(new_slave->prev).id + 1;

Signature · 1 line

1.7.4.4

Weiping Pan

author

2011-06-09 07:18 UTC #3 lore raw

PATCH v4 3/5 bonding:delete agg_select_mode from ad_bond_info

In reply to Weiping Pan

bond_params->ad_select and ad_bond_info->agg_select_mode have the same meaning, they are duplicate and need extra synchronization.

__get_agg_selection_mode() get ad_select from bond_params directly.

Signed-off-by Weiping Pan <[email protected]>

---
 drivers/net/bonding/bond_3ad.c |    3 +--
 drivers/net/bonding/bond_3ad.h |    1 -
 2 files changed, 1 insertions(+), 3 deletions(-)

drivers/net/bonding/bond_3ad.c +1 -2

index 013a801..6122725 100644
@@ -262,7 +262,7 @@ static inline u32 __get_agg_selection_mode(struct port *port)
 	if (bond == NULL)
 		return BOND_AD_STABLE;
 
-	return BOND_AD_INFO(bond).agg_select_mode;
+	return bond->params.ad_select;
 }
 
 /**

@@ -1859,7 +1859,6 @@ static void ad_marker_response_received(struct bond_marker *marker,
 void bond_3ad_initiate_agg_selection(struct bonding *bond, int timeout)
 {
 	BOND_AD_INFO(bond).agg_select_timer = timeout;
-	BOND_AD_INFO(bond).agg_select_mode = bond->params.ad_select;
 }
 
 static u16 aggregator_identifier;

drivers/net/bonding/bond_3ad.h +0 -1

index 9782785..1682e69 100644
@@ -253,7 +253,6 @@ struct ad_system {
 struct ad_bond_info {
 	struct ad_system system;	    /* 802.3ad system structure */
 	u32 agg_select_timer;	    // Timer to select aggregator after all adapter's hand shakes
-	u32 agg_select_mode;	    // Mode of selection of active aggregator(bandwidth/count)
 	struct timer_list ad_timer;
 };

Signature · 1 line

1.7.4.4

Weiping Pan

author

2011-06-09 07:19 UTC #4 lore raw

PATCH v4 4/5 bonding: delete unused ad_timer

In reply to Weiping Pan

Now we use agg_select_timer and ad_work.

Reviewed-by WANG Cong <[email protected]>

Signed-off-by Weiping Pan <[email protected]>

---
 drivers/net/bonding/bond_3ad.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

drivers/net/bonding/bond_3ad.h +0 -1

index 1682e69..235b2cc 100644
@@ -253,7 +253,6 @@ struct ad_system {
 struct ad_bond_info {
 	struct ad_system system;	    /* 802.3ad system structure */
 	u32 agg_select_timer;	    // Timer to select aggregator after all adapter's hand shakes
-	struct timer_list ad_timer;
 };
 
 struct ad_slave_info {

Signature · 1 line

1.7.4.4

Weiping Pan

author

2011-06-09 07:18 UTC #5 lore raw

PATCH v4 5/5 bonding: delete unused arp_mon_pt

In reply to Weiping Pan

Now all received packets are handled by bond_handle_frame, and arp_mon_pt isn't used any more.

Reviewed-by WANG Cong <[email protected]>

Signed-off-by Weiping Pan <[email protected]>

---
 drivers/net/bonding/bonding.h |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

drivers/net/bonding/bonding.h +0 -1

index ea1d005..382903f 100644
@@ -240,7 +240,6 @@ struct bonding {
 	struct   bond_params params;
 	struct   list_head vlan_list;
 	struct   vlan_group *vlgrp;
-	struct   packet_type arp_mon_pt;
 	struct   workqueue_struct *wq;
 	struct   delayed_work mii_work;
 	struct   delayed_work arp_work;

Signature · 1 line

1.7.4.4

David Miller

maintainer

2011-06-09 07:21 UTC #6 lore raw

In reply to Weiping Pan

My head will spin if I see yet another submission of these patches :-(

WeipingPan

author

2011-06-09 07:29 UTC #7 lore raw

Quoted 2 lines · David Miller

My head will spin if I see yet another submission of these patches :-(

sorry for the inconvenience

Weiping Pan

Jay Vosburgh

2011-06-09 21:58 UTC #8 lore raw

In reply to Weiping Pan

Quoted 16 lines · Weiping Pan

There is bug that when you modify lacp_rate via sysfs, 802.3ad won't use the new value of lacp_rate to transmit packets. This is because port->actor_oper_port_state isn't changed.
As for ad_select, it can work,
but both struct bond_params and ad_bond_info have lacp_fast and ad_select, they are duplicate and need extra synchronization.
802.3ad can get them from bond_params directly every time.
And ad_timer and arp_mon_pt aren't used any more, just delete them.
changelog:
v2:
add bond_3ad_update_lacp_rate() as a helper function, and hold bond->lock when iterates slave list.

Quoted 5 lines

v3:
delete duplicate lacp_fast and agg_select_mode from struct ad_bond_info.
v4:
delete unused ad_timer and arp_mon_pt.

All patches in the series look reasonable.

Signed-off-by Jay Vosburgh <[email protected]>

-J

Quoted 17 lines

Weiping Pan (5):
bonding: make 802.3ad use latest lacp_rate
bonding:delete lacp_fast from ad_bond_info
bonding:delete agg_select_mode from ad_bond_info
bonding: delete unused ad_timer
bonding: delete unused arp_mon_pt
drivers/net/bonding/bond_3ad.c   |   41 ++++++++++++++++++++++++++++++++-----
drivers/net/bonding/bond_3ad.h   |    8 +-----
drivers/net/bonding/bond_main.c  |    3 +-
drivers/net/bonding/bond_sysfs.c |    1 +
drivers/net/bonding/bonding.h    |    1 -
5 files changed, 39 insertions(+), 15 deletions(-)
Signature · 1 line
1.7.4.4

David Miller

maintainer

2011-06-09 22:03 UTC #9 lore raw

From: Jay Vosburgh <[email protected]>
Date: Thu, 09 Jun 2011 14:58:11 -0700

Quoted 21 lines

Quoted 19 lines · Weiping Pan
There is bug that when you modify lacp_rate via sysfs, 802.3ad won't use the new value of lacp_rate to transmit packets. This is because port->actor_oper_port_state isn't changed.
As for ad_select, it can work,
but both struct bond_params and ad_bond_info have lacp_fast and ad_select, they are duplicate and need extra synchronization.
802.3ad can get them from bond_params directly every time.
And ad_timer and arp_mon_pt aren't used any more, just delete them.
changelog:
v2:
add bond_3ad_update_lacp_rate() as a helper function, and hold bond->lock when iterates slave list.
v3:
delete duplicate lacp_fast and agg_select_mode from struct ad_bond_info.

Quoted 6 lines

Quoted 2 lines
v4:
delete unused ad_timer and arp_mon_pt.
All patches in the series look reasonable.
Signed-off-by Jay Vosburgh <[email protected]>

All applied, thanks everyone.