Re: [PATCH net-next] bonding: make 802.3ad use update lacp_rate

From: Jay Vosburgh
Date: Fri Jun 03 2011 - 15:43:26 EST

Next message: Alejandro Cabrera: "[PATCH] Add Xilinx watchdog timer driver"
Previous message: Alexander Holler: "Re: bridge/netfilter: regression in 2.6.39.1"
In reply to: Jiri Pirko: "Re: [PATCH net-next] bonding: make 802.3ad use update lacp_rate"
Next in thread: Weiping Pan: "[PATCH net-next] bonding: make 802.3ad use update lacp_rate (v2)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Weiping Pan <panweiping3@xxxxxxxxx> wrote:

>There is a bug that when you modify lacp_rate via sysfs,
>802.3ad won't use the new value of lacp_rate to transmit packets.
>That is because port->actor_oper_port_state isn't changed.
>
>And I want to use AD_STATE_LACP_TIMEOUT,
>so I move related macros from bond_3ad.c into bond_3ad.h.
>
>Signed-off-by: Weiping Pan <panweiping3@xxxxxxxxx>
>---
> drivers/net/bonding/bond_3ad.c | 48 --------------------------------------
> drivers/net/bonding/bond_3ad.h | 48 ++++++++++++++++++++++++++++++++++++++
> drivers/net/bonding/bond_sysfs.c | 15 +++++++++++-
> 3 files changed, 62 insertions(+), 49 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index c7537abc..9083258 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -34,54 +34,6 @@
> #include "bonding.h"
> #include "bond_3ad.h"
>
>-// General definitions
>-#define AD_SHORT_TIMEOUT 1
>-#define AD_LONG_TIMEOUT 0
>-#define AD_STANDBY 0x2
>-#define AD_MAX_TX_IN_SECOND 3
>-#define AD_COLLECTOR_MAX_DELAY 0
>-
>-// Timer definitions(43.4.4 in the 802.3ad standard)
>-#define AD_FAST_PERIODIC_TIME 1
>-#define AD_SLOW_PERIODIC_TIME 30
>-#define AD_SHORT_TIMEOUT_TIME (3*AD_FAST_PERIODIC_TIME)
>-#define AD_LONG_TIMEOUT_TIME (3*AD_SLOW_PERIODIC_TIME)
>-#define AD_CHURN_DETECTION_TIME 60
>-#define AD_AGGREGATE_WAIT_TIME 2
>-
>-// Port state definitions(43.4.2.2 in the 802.3ad standard)
>-#define AD_STATE_LACP_ACTIVITY 0x1
>-#define AD_STATE_LACP_TIMEOUT 0x2
>-#define AD_STATE_AGGREGATION 0x4
>-#define AD_STATE_SYNCHRONIZATION 0x8
>-#define AD_STATE_COLLECTING 0x10
>-#define AD_STATE_DISTRIBUTING 0x20
>-#define AD_STATE_DEFAULTED 0x40
>-#define AD_STATE_EXPIRED 0x80
>-
>-// Port Variables definitions used by the State Machines(43.4.7 in the 802.3ad standard)
>-#define AD_PORT_BEGIN 0x1
>-#define AD_PORT_LACP_ENABLED 0x2
>-#define AD_PORT_ACTOR_CHURN 0x4
>-#define AD_PORT_PARTNER_CHURN 0x8
>-#define AD_PORT_READY 0x10
>-#define AD_PORT_READY_N 0x20
>-#define AD_PORT_MATCHED 0x40
>-#define AD_PORT_STANDBY 0x80
>-#define AD_PORT_SELECTED 0x100
>-#define AD_PORT_MOVED 0x200
>-
>-// Port Key definitions
>-// key is determined according to the link speed, duplex and
>-// user key(which is yet not supported)
>-// ------------------------------------------------------------
>-// Port key : | User key | Speed |Duplex|
>-// ------------------------------------------------------------
>-// 16 6 1 0
>-#define AD_DUPLEX_KEY_BITS 0x1
>-#define AD_SPEED_KEY_BITS 0x3E
>-#define AD_USER_KEY_BITS 0xFFC0
>-
> //dalloun
> #define AD_LINK_SPEED_BITMASK_1MBPS 0x1
> #define AD_LINK_SPEED_BITMASK_10MBPS 0x2
>diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
>index 0ee3f16..6ce1f6b 100644
>--- a/drivers/net/bonding/bond_3ad.h
>+++ b/drivers/net/bonding/bond_3ad.h
>@@ -37,6 +37,54 @@
> #define AD_LACP_SLOW 0
> #define AD_LACP_FAST 1
>
>+#define AD_SHORT_TIMEOUT 1
>+#define AD_LONG_TIMEOUT 0
>+#define AD_STANDBY 0x2
>+#define AD_MAX_TX_IN_SECOND 3
>+#define AD_COLLECTOR_MAX_DELAY 0
>+
>+// Timer definitions(43.4.4 in the 802.3ad standard)
>+#define AD_FAST_PERIODIC_TIME 1
>+#define AD_SLOW_PERIODIC_TIME 30
>+#define AD_SHORT_TIMEOUT_TIME (3*AD_FAST_PERIODIC_TIME)
>+#define AD_LONG_TIMEOUT_TIME (3*AD_SLOW_PERIODIC_TIME)
>+#define AD_CHURN_DETECTION_TIME 60
>+#define AD_AGGREGATE_WAIT_TIME 2
>+
>+// Port state definitions(43.4.2.2 in the 802.3ad standard)
>+#define AD_STATE_LACP_ACTIVITY 0x1
>+#define AD_STATE_LACP_TIMEOUT 0x2
>+#define AD_STATE_AGGREGATION 0x4
>+#define AD_STATE_SYNCHRONIZATION 0x8
>+#define AD_STATE_COLLECTING 0x10
>+#define AD_STATE_DISTRIBUTING 0x20
>+#define AD_STATE_DEFAULTED 0x40
>+#define AD_STATE_EXPIRED 0x80
>+
>+// Port Variables definitions used by the State Machines(43.4.7 in the 802.3ad standard)
>+#define AD_PORT_BEGIN 0x1
>+#define AD_PORT_LACP_ENABLED 0x2
>+#define AD_PORT_ACTOR_CHURN 0x4
>+#define AD_PORT_PARTNER_CHURN 0x8
>+#define AD_PORT_READY 0x10
>+#define AD_PORT_READY_N 0x20
>+#define AD_PORT_MATCHED 0x40
>+#define AD_PORT_STANDBY 0x80
>+#define AD_PORT_SELECTED 0x100
>+#define AD_PORT_MOVED 0x200
>+
>+// Port Key definitions
>+// key is determined according to the link speed, duplex and
>+// user key(which is yet not supported)
>+// ------------------------------------------------------------
>+// Port key : | User key | Speed |Duplex|
>+// ------------------------------------------------------------
>+// 16 6 1 0
>+#define AD_DUPLEX_KEY_BITS 0x1
>+#define AD_SPEED_KEY_BITS 0x3E
>+#define AD_USER_KEY_BITS 0xFFC0
>+
>+
> typedef struct mac_addr {
> u8 mac_addr_value[ETH_ALEN];
> } __packed mac_addr_t;
>diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
>index 88fcb25..2cad514 100644
>--- a/drivers/net/bonding/bond_sysfs.c
>+++ b/drivers/net/bonding/bond_sysfs.c
>@@ -783,8 +783,13 @@ static ssize_t bonding_store_lacp(struct device *d,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
>- int new_value, ret = count;
>+ int new_value, i, ret = count;
> struct bonding *bond = to_bond(d);
>+ struct slave *slave;
>+ struct port *port = NULL;
>+
>+ if (!rtnl_trylock())
>+ return restart_syscall();
>
> if (bond->dev->flags & IFF_UP) {
> pr_err("%s: Unable to update LACP rate because interface is up.\n",
>@@ -804,6 +809,13 @@ static ssize_t bonding_store_lacp(struct device *d,
>
> if ((new_value == 1) || (new_value == 0)) {
> bond->params.lacp_fast = new_value;
>+ bond_for_each_slave(bond, slave, i) {
>+ port = &(slave->ad_info.port);
>+ if (new_value)
>+ port->actor_oper_port_state |= AD_STATE_LACP_TIMEOUT;
>+ else
>+ port->actor_oper_port_state &= ~AD_STATE_LACP_TIMEOUT;
>+ }

I think this would be cleaner if done via a helper function in
bond_3ad.c rather than inline.

Also, this should either have a comment about not needing
locking beyond RTNL, or just do the "correct" locking.

Since this requires the bond to be down, there is no real
contention for the port state (because the state machines and LACPDU
processing does not run), and holding RTNL is enough to mutex. If the
!IFF_UP restriction is ever removed, though, this would require holding
bond->lock for read and locking each port's state machine lock before
altering actor_oper_port_state.

-J

> pr_info("%s: Setting LACP rate to %s (%d).\n",
> bond->dev->name, bond_lacp_tbl[new_value].modename,
> new_value);
>@@ -813,6 +825,7 @@ static ssize_t bonding_store_lacp(struct device *d,
> ret = -EINVAL;
> }
> out:
>+ rtnl_unlock();
> return ret;
> }
> static DEVICE_ATTR(lacp_rate, S_IRUGO | S_IWUSR,
>--
>1.7.4.4
>

---
-Jay Vosburgh, IBM Linux Technology Center, fubar@xxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Alejandro Cabrera: "[PATCH] Add Xilinx watchdog timer driver"
Previous message: Alexander Holler: "Re: bridge/netfilter: regression in 2.6.39.1"
In reply to: Jiri Pirko: "Re: [PATCH net-next] bonding: make 802.3ad use update lacp_rate"
Next in thread: Weiping Pan: "[PATCH net-next] bonding: make 802.3ad use update lacp_rate (v2)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]