[PATCH net 0/3] bridge: ageing timer regression fix

[PATCH net 0/3] bridge: ageing timer regression fix

[Stephen Hemminger]

This fixes regression in how ageing timer is managed.
Backing out the change required fixing switch drivers as well.

Ido Schimmel (2):
  mlxsw: spectrum: Check requested ageing time is valid
  rocker: Set FDB cleanup timer according to lowest ageing time

Stephen Hemminger (1):
  bridge: fix regression in ageing time

 drivers/net/ethernet/mellanox/mlxsw/spectrum.h           |  2 ++
 drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c |  9 +++++++--
 drivers/net/ethernet/rocker/rocker.c                     |  8 +++++++-
 include/linux/if_bridge.h                                |  4 ----
 net/bridge/br_stp.c                                      | 11 ++++++++---
 5 files changed, 24 insertions(+), 10 deletions(-)

-- 
2.1.4

[PATCH 1/3] mlxsw: spectrum: Check requested ageing time is valid

[Stephen Hemminger]

From: Ido Schimmel <idosch@mellanox.com>

Commit c62987bbd8a1 ("bridge: push bridge setting ageing_time down to
switchdev") added a check for minimum and maximum ageing time, but this
breaks existing behaviour where one can set ageing time to 0 for a
non-learning bridge.

Push this check down to the driver and allow the check in the bridge
layer to be removed. Currently ageing time 0 is refused by the driver,
but we can later add support for this functionality.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h           | 2 ++
 drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 9 +++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index 3b89ed2..65a115f 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -118,6 +118,8 @@ struct mlxsw_sp {
 #define MLXSW_SP_DEFAULT_LEARNING_INTERVAL 100
 		unsigned int interval; /* ms */
 	} fdb_notify;
+#define MLXSW_SP_MIN_AGEING_TIME 10
+#define MLXSW_SP_MAX_AGEING_TIME 1000000
 #define MLXSW_SP_DEFAULT_AGEING_TIME 300
 	u32 ageing_time;
 	struct mlxsw_sp_upper master_bridge;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index 7b56098..e1c74ef 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -311,8 +311,13 @@ static int mlxsw_sp_port_attr_br_ageing_set(struct mlxsw_sp_port *mlxsw_sp_port,
 	unsigned long ageing_jiffies = clock_t_to_jiffies(ageing_clock_t);
 	u32 ageing_time = jiffies_to_msecs(ageing_jiffies) / 1000;
 
-	if (switchdev_trans_ph_prepare(trans))
-		return 0;
+	if (switchdev_trans_ph_prepare(trans)) {
+		if (ageing_time < MLXSW_SP_MIN_AGEING_TIME ||
+		    ageing_time > MLXSW_SP_MAX_AGEING_TIME)
+			return -ERANGE;
+		else
+			return 0;
+	}
 
 	return mlxsw_sp_ageing_set(mlxsw_sp, ageing_time);
 }
-- 
2.1.4

Re: [PATCH 1/3] mlxsw: spectrum: Check requested ageing time is valid

[Jiri Pirko]

Tue, Mar 08, 2016 at 09:59:33PM CET, stephen@networkplumber.org wrote:
>From: Ido Schimmel <idosch@mellanox.com>
>
>Commit c62987bbd8a1 ("bridge: push bridge setting ageing_time down to
>switchdev") added a check for minimum and maximum ageing time, but this
>breaks existing behaviour where one can set ageing time to 0 for a
>non-learning bridge.
>
>Push this check down to the driver and allow the check in the bridge
>layer to be removed. Currently ageing time 0 is refused by the driver,
>but we can later add support for this functionality.
>
>Signed-off-by: Ido Schimmel <idosch@mellanox.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

[PATCH 2/3] rocker: set FDB cleanup timer according to lowest ageing time

[Stephen Hemminger]

From: Ido Schimmel <idosch@mellanox.com>

In rocker, ageing time is a per-port attribute, so the next time the FDB
cleanup timer fires should be set according to the lowest ageing time.

This will later allow us to delete the BR_MIN_AGEING_TIME macro, which was
added to guarantee minimum ageing time in the bridge layer, thereby breaking
existing behavior.

Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/rocker/rocker.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 166a7fc..19c65503 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -239,6 +239,7 @@ struct rocker {
 	struct {
 		u64 id;
 	} hw;
+	unsigned long ageing_time;
 	spinlock_t cmd_ring_lock;		/* for cmd ring accesses */
 	struct rocker_dma_ring_info cmd_ring;
 	struct rocker_dma_ring_info event_ring;
@@ -3704,7 +3705,7 @@ static void rocker_fdb_cleanup(unsigned long data)
 	struct rocker_port *rocker_port;
 	struct rocker_fdb_tbl_entry *entry;
 	struct hlist_node *tmp;
-	unsigned long next_timer = jiffies + BR_MIN_AGEING_TIME;
+	unsigned long next_timer = jiffies + rocker->ageing_time;
 	unsigned long expires;
 	unsigned long lock_flags;
 	int flags = ROCKER_OP_FLAG_NOWAIT | ROCKER_OP_FLAG_REMOVE |
@@ -4367,8 +4368,12 @@ static int rocker_port_bridge_ageing_time(struct rocker_port *rocker_port,
 					  struct switchdev_trans *trans,
 					  u32 ageing_time)
 {
+	struct rocker *rocker = rocker_port->rocker;
+
 	if (!switchdev_trans_ph_prepare(trans)) {
 		rocker_port->ageing_time = clock_t_to_jiffies(ageing_time);
+		if (rocker_port->ageing_time < rocker->ageing_time)
+			rocker->ageing_time = rocker_port->ageing_time;
 		mod_timer(&rocker_port->rocker->fdb_cleanup_timer, jiffies);
 	}
 
@@ -5206,6 +5211,7 @@ static int rocker_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err_init_tbls;
 	}
 
+	rocker->ageing_time = BR_DEFAULT_AGEING_TIME;
 	setup_timer(&rocker->fdb_cleanup_timer, rocker_fdb_cleanup,
 		    (unsigned long) rocker);
 	mod_timer(&rocker->fdb_cleanup_timer, jiffies);
-- 
2.1.4

Re: [PATCH 2/3] rocker: set FDB cleanup timer according to lowest ageing time

[Jiri Pirko]

Tue, Mar 08, 2016 at 09:59:34PM CET, stephen@networkplumber.org wrote:
>From: Ido Schimmel <idosch@mellanox.com>
>
>In rocker, ageing time is a per-port attribute, so the next time the FDB
>cleanup timer fires should be set according to the lowest ageing time.
>
>This will later allow us to delete the BR_MIN_AGEING_TIME macro, which was
>added to guarantee minimum ageing time in the bridge layer, thereby breaking
>existing behavior.
>
>Signed-off-by: Ido Schimmel <idosch@mellanox.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

[PATCH 3/3] bridge: allow zero ageing time

[Stephen Hemminger]

From: Stephen Hemminger <shemming@brocade.com>

This fixes a regression in the bridge ageing time caused by:
commit c62987bbd8a1 ("bridge: push bridge setting ageing_time down to switchdev")

There are users of Linux bridge which use the feature that if ageing time
is set to 0 it causes entries to never expire. See:
  https://www.linuxfoundation.org/collaborate/workgroups/networking/bridge

For a pure software bridge, it is unnecessary for the code to have
arbitrary restrictions on what values are allowable.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

---
Please apply to 4.4 for stable as well.
---
 include/linux/if_bridge.h |  4 ----
 net/bridge/br_stp.c       | 11 ++++++++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index a338a68..dcb89e3 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -46,10 +46,6 @@ struct br_ip_list {
 #define BR_LEARNING_SYNC	BIT(9)
 #define BR_PROXYARP_WIFI	BIT(10)
 
-/* values as per ieee8021QBridgeFdbAgingTime */
-#define BR_MIN_AGEING_TIME	(10 * HZ)
-#define BR_MAX_AGEING_TIME	(1000000 * HZ)
-
 #define BR_DEFAULT_AGEING_TIME	(300 * HZ)
 
 extern void brioctl_set(int (*ioctl_hook)(struct net *, unsigned int, void __user *));
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index b3cca12..8b5898a 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -568,6 +568,14 @@ int br_set_max_age(struct net_bridge *br, unsigned long val)
 
 }
 
+/* Set time interval that dynamic forwarding entries live
+ * For pure software bridge, allow values outside the 802.1
+ * standard specification for special cases:
+ *  0 - entry never ages (all permanant)
+ *  1 - entry disappears (no persistance)
+ *
+ * Offloaded switch entries maybe more restrictive
+ */
 int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
 {
 	struct switchdev_attr attr = {
@@ -579,9 +587,6 @@ int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
 	unsigned long t = clock_t_to_jiffies(ageing_time);
 	int err;
 
-	if (t < BR_MIN_AGEING_TIME || t > BR_MAX_AGEING_TIME)
-		return -ERANGE;
-
 	err = switchdev_port_attr_set(br->dev, &attr);
 	if (err)
 		return err;
-- 
2.1.4

Re: [PATCH 3/3] bridge: allow zero ageing time

[Jiri Pirko]

Tue, Mar 08, 2016 at 09:59:35PM CET, stephen@networkplumber.org wrote:
>From: Stephen Hemminger <shemming@brocade.com>
>
>This fixes a regression in the bridge ageing time caused by:
>commit c62987bbd8a1 ("bridge: push bridge setting ageing_time down to switchdev")
>
>There are users of Linux bridge which use the feature that if ageing time
>is set to 0 it causes entries to never expire. See:
>  https://www.linuxfoundation.org/collaborate/workgroups/networking/bridge
>
>For a pure software bridge, it is unnecessary for the code to have
>arbitrary restrictions on what values are allowable.
>
>Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCH net 0/3] bridge: ageing timer regression fix

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Tue,  8 Mar 2016 12:59:32 -0800

> This fixes regression in how ageing timer is managed.
> Backing out the change required fixing switch drivers as well.

Series applied, thanks.

Jiri, since I applied this to net-next, I had to deal with the conflicts
created by your world splitup of the rocker driver.  I wasn't sure, while
doing so, whether you'd like this new ageing_time member to remain in the
rocker struct, as in Stephen's patch, or to be placed in the ofdpa struct.
I choose the former, send me a patch if you want the latter.

I'll queue this up for -stable, as well.

Re: [PATCH net 0/3] bridge: ageing timer regression fix

[Jiri Pirko]

Fri, Mar 11, 2016 at 09:04:48PM CET, davem@davemloft.net wrote:
>From: Stephen Hemminger <stephen@networkplumber.org>
>Date: Tue,  8 Mar 2016 12:59:32 -0800
>
>> This fixes regression in how ageing timer is managed.
>> Backing out the change required fixing switch drivers as well.
>
>Series applied, thanks.
>
>Jiri, since I applied this to net-next, I had to deal with the conflicts
>created by your world splitup of the rocker driver.  I wasn't sure, while
>doing so, whether you'd like this new ageing_time member to remain in the
>rocker struct, as in Stephen's patch, or to be placed in the ofdpa struct.
>I choose the former, send me a patch if you want the latter.

Yeah, it belongs into ofdpa. I'll send you patch in a jiff. Thanks!