[PATCH net-next 0/7] bridge: don't age out externally added FDB entries

[PATCH net-next 0/7] bridge: don't age out externally added FDB entries

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

Siva originally proposed skipping externally added FDB entries in the bridge's
FDB garbage collection func, and moving the ageing of externally added entries
to the port driver/device.  This broke rocker, since rocker didn't have a
hardware (or software) mechanism for ageing out its learned FDB entries.

This patchset reintroduces Siva's bridge driver patch to skip externally added
entries and adds support in rocker so rocker can age out its own entries.
Rocker does this using a software timer similar to the bridge's FDB garbage
collection timer.  Other switchdev devices/drivers can use this software timer
method or program the device to nofity aged-out entries to the driver.

Updated switchdev.txt documentation to reflect current state-of-the-art.  This
removes one more XXX todo comment in switchdev.txt.

Scott Feldman (6):
  rocker: track when FDB entry is touched.
  rocker: store rocker_port in fdb key rather than pport
  rocker: adding port ageing_time for ageing out FDB entries
  bridge: define some min/max ageing time constants we'll use next
  rocker: add FDB cleanup timer
  switchdev: update documentation on FDB ageing_time

Siva Mannem (1):
  bridge: don't age externally added FDB entries

 Documentation/networking/switchdev.txt |   24 +++++------
 drivers/net/ethernet/rocker/rocker.c   |   69 +++++++++++++++++++++++++++-----
 include/linux/if_bridge.h              |    4 ++
 net/bridge/br_fdb.c                    |    2 +
 4 files changed, 77 insertions(+), 22 deletions(-)

-- 
1.7.10.4

[PATCH net-next 1/7] rocker: track when FDB entry is touched.

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

The entry is touched once when created, and touched again for each update.
The touched time is used to calculate FDB entry age.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 drivers/net/ethernet/rocker/rocker.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 34ac41a..e517e9c 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -152,6 +152,7 @@ struct rocker_fdb_tbl_entry {
 	struct hlist_node entry;
 	u32 key_crc32; /* key */
 	bool learned;
+	unsigned long touched;
 	struct rocker_fdb_tbl_key {
 		u32 pport;
 		u8 addr[ETH_ALEN];
@@ -3629,6 +3630,7 @@ static int rocker_port_fdb(struct rocker_port *rocker_port,
 		return -ENOMEM;
 
 	fdb->learned = (flags & ROCKER_OP_FLAG_LEARNED);
+	fdb->touched = jiffies;
 	fdb->key.pport = rocker_port->pport;
 	ether_addr_copy(fdb->key.addr, addr);
 	fdb->key.vlan_id = vlan_id;
@@ -3638,13 +3640,17 @@ static int rocker_port_fdb(struct rocker_port *rocker_port,
 
 	found = rocker_fdb_tbl_find(rocker, fdb);
 
-	if (removing && found) {
-		rocker_port_kfree(trans, fdb);
-		if (trans != SWITCHDEV_TRANS_PREPARE)
-			hash_del(&found->entry);
-	} else if (!removing && !found) {
+	if (found) {
+		found->touched = jiffies;
+		if (removing) {
+			rocker_port_kfree(trans, fdb);
+			if (trans != SWITCHDEV_TRANS_PREPARE)
+				hash_del(&found->entry);
+		}
+	} else if (!removing) {
 		if (trans != SWITCHDEV_TRANS_PREPARE)
-			hash_add(rocker->fdb_tbl, &fdb->entry, fdb->key_crc32);
+			hash_add(rocker->fdb_tbl, &fdb->entry,
+				 fdb->key_crc32);
 	}
 
 	spin_unlock_irqrestore(&rocker->fdb_tbl_lock, lock_flags);
-- 
1.7.10.4

Re: [PATCH net-next 1/7] rocker: track when FDB entry is touched.

[Jiri Pirko]

Fri, Sep 18, 2015 at 09:55:45PM CEST, sfeldma@gmail.com wrote:
>From: Scott Feldman <sfeldma@gmail.com>
>
>The entry is touched once when created, and touched again for each update.
>The touched time is used to calculate FDB entry age.
>
>Signed-off-by: Scott Feldman <sfeldma@gmail.com>

Acked-by: Jiri Pirko <jiri@resnulli.us>

[PATCH net-next 2/7] rocker: store rocker_port in fdb key rather than pport

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

We'll need more info from rocker_port than just pport when we age out fdb
entries, so store rocker_port rather than pport in each fdb entry.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 drivers/net/ethernet/rocker/rocker.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index e517e9c..f55ed2c 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -154,7 +154,7 @@ struct rocker_fdb_tbl_entry {
 	bool learned;
 	unsigned long touched;
 	struct rocker_fdb_tbl_key {
-		u32 pport;
+		struct rocker_port *rocker_port;
 		u8 addr[ETH_ALEN];
 		__be16 vlan_id;
 	} key;
@@ -3631,7 +3631,7 @@ static int rocker_port_fdb(struct rocker_port *rocker_port,
 
 	fdb->learned = (flags & ROCKER_OP_FLAG_LEARNED);
 	fdb->touched = jiffies;
-	fdb->key.pport = rocker_port->pport;
+	fdb->key.rocker_port = rocker_port;
 	ether_addr_copy(fdb->key.addr, addr);
 	fdb->key.vlan_id = vlan_id;
 	fdb->key_crc32 = crc32(~0, &fdb->key, sizeof(fdb->key));
@@ -3686,7 +3686,7 @@ static int rocker_port_fdb_flush(struct rocker_port *rocker_port,
 	spin_lock_irqsave(&rocker->fdb_tbl_lock, lock_flags);
 
 	hash_for_each_safe(rocker->fdb_tbl, bkt, tmp, found, entry) {
-		if (found->key.pport != rocker_port->pport)
+		if (found->key.rocker_port != rocker_port)
 			continue;
 		if (!found->learned)
 			continue;
@@ -4553,7 +4553,7 @@ static int rocker_port_fdb_dump(const struct rocker_port *rocker_port,
 
 	spin_lock_irqsave(&rocker->fdb_tbl_lock, lock_flags);
 	hash_for_each_safe(rocker->fdb_tbl, bkt, tmp, found, entry) {
-		if (found->key.pport != rocker_port->pport)
+		if (found->key.rocker_port != rocker_port)
 			continue;
 		fdb->addr = found->key.addr;
 		fdb->ndm_state = NUD_REACHABLE;
-- 
1.7.10.4

Re: [PATCH net-next 2/7] rocker: store rocker_port in fdb key rather than pport

[Jiri Pirko]

Fri, Sep 18, 2015 at 09:55:46PM CEST, sfeldma@gmail.com wrote:
>From: Scott Feldman <sfeldma@gmail.com>
>
>We'll need more info from rocker_port than just pport when we age out fdb
>entries, so store rocker_port rather than pport in each fdb entry.
>
>Signed-off-by: Scott Feldman <sfeldma@gmail.com>

Acked-by: Jiri Pirko <jiri@resnulli.us>

[PATCH net-next 3/7] rocker: adding port ageing_time for ageing out FDB entries

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

Follow-up patcheset will allow user to change ageing_time, but for now
just hard-code it to a fixed value (the same value used as the default
for the bridge driver).

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 drivers/net/ethernet/rocker/rocker.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index f55ed2c..eba22f5 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -221,6 +221,7 @@ struct rocker_port {
 	__be16 internal_vlan_id;
 	int stp_state;
 	u32 brport_flags;
+	unsigned long ageing_time;
 	bool ctrls[ROCKER_CTRL_MAX];
 	unsigned long vlan_bitmap[ROCKER_VLAN_BITMAP_LEN];
 	struct napi_struct napi_tx;
@@ -4975,6 +4976,7 @@ static int rocker_probe_port(struct rocker *rocker, unsigned int port_number)
 	rocker_port->port_number = port_number;
 	rocker_port->pport = port_number + 1;
 	rocker_port->brport_flags = BR_LEARNING | BR_LEARNING_SYNC;
+	rocker_port->ageing_time = 300 * HZ;
 	INIT_LIST_HEAD(&rocker_port->trans_mem);
 
 	rocker_port_dev_addr_init(rocker_port);
-- 
1.7.10.4

Re: [PATCH net-next 3/7] rocker: adding port ageing_time for ageing out FDB entries

[Jiri Pirko]

Fri, Sep 18, 2015 at 09:55:47PM CEST, sfeldma@gmail.com wrote:
>From: Scott Feldman <sfeldma@gmail.com>
>
>Follow-up patcheset will allow user to change ageing_time, but for now
>just hard-code it to a fixed value (the same value used as the default
>for the bridge driver).
>
>Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>---
> drivers/net/ethernet/rocker/rocker.c |    2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
>index f55ed2c..eba22f5 100644
>--- a/drivers/net/ethernet/rocker/rocker.c
>+++ b/drivers/net/ethernet/rocker/rocker.c
>@@ -221,6 +221,7 @@ struct rocker_port {
> 	__be16 internal_vlan_id;
> 	int stp_state;
> 	u32 brport_flags;
>+	unsigned long ageing_time;
> 	bool ctrls[ROCKER_CTRL_MAX];
> 	unsigned long vlan_bitmap[ROCKER_VLAN_BITMAP_LEN];
> 	struct napi_struct napi_tx;
>@@ -4975,6 +4976,7 @@ static int rocker_probe_port(struct rocker *rocker, unsigned int port_number)
> 	rocker_port->port_number = port_number;
> 	rocker_port->pport = port_number + 1;
> 	rocker_port->brport_flags = BR_LEARNING | BR_LEARNING_SYNC;
>+	rocker_port->ageing_time = 300 * HZ;

How about to add also "BR_DEFAULT_AGEING_TIME" and use it here?
>

Re: [PATCH net-next 3/7] rocker: adding port ageing_time for ageing out FDB entries

[Scott Feldman]

On Fri, Sep 18, 2015 at 11:30 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Sep 18, 2015 at 09:55:47PM CEST, sfeldma@gmail.com wrote:
>>From: Scott Feldman <sfeldma@gmail.com>
>>
>>Follow-up patcheset will allow user to change ageing_time, but for now
>>just hard-code it to a fixed value (the same value used as the default
>>for the bridge driver).
>>
>>Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>>---
>> drivers/net/ethernet/rocker/rocker.c |    2 ++
>> 1 file changed, 2 insertions(+)
>>
>>diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
>>index f55ed2c..eba22f5 100644
>>--- a/drivers/net/ethernet/rocker/rocker.c
>>+++ b/drivers/net/ethernet/rocker/rocker.c
>>@@ -221,6 +221,7 @@ struct rocker_port {
>>       __be16 internal_vlan_id;
>>       int stp_state;
>>       u32 brport_flags;
>>+      unsigned long ageing_time;
>>       bool ctrls[ROCKER_CTRL_MAX];
>>       unsigned long vlan_bitmap[ROCKER_VLAN_BITMAP_LEN];
>>       struct napi_struct napi_tx;
>>@@ -4975,6 +4976,7 @@ static int rocker_probe_port(struct rocker *rocker, unsigned int port_number)
>>       rocker_port->port_number = port_number;
>>       rocker_port->pport = port_number + 1;
>>       rocker_port->brport_flags = BR_LEARNING | BR_LEARNING_SYNC;
>>+      rocker_port->ageing_time = 300 * HZ;
>
> How about to add also "BR_DEFAULT_AGEING_TIME" and use it here?

Yes, added for v2

[PATCH net-next 4/7] bridge: define some min/max ageing time constants we'll use next

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 include/linux/if_bridge.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index dad8b00..6cc6dbc 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -46,6 +46,10 @@ 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)
+
 extern void brioctl_set(int (*ioctl_hook)(struct net *, unsigned int, void __user *));
 
 typedef int br_should_route_hook_t(struct sk_buff *skb);
-- 
1.7.10.4

Re: [PATCH net-next 4/7] bridge: define some min/max ageing time constants we'll use next

[Jiri Pirko]

Fri, Sep 18, 2015 at 09:55:48PM CEST, sfeldma@gmail.com wrote:
>From: Scott Feldman <sfeldma@gmail.com>
>
>Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>---
> include/linux/if_bridge.h |    4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>index dad8b00..6cc6dbc 100644
>--- a/include/linux/if_bridge.h
>+++ b/include/linux/if_bridge.h
>@@ -46,6 +46,10 @@ 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)

I think that a bridge patch checking against these values should be
introduced along with these values, in the same patchset

Re: [PATCH net-next 4/7] bridge: define some min/max ageing time constants we'll use next

[Scott Feldman]

On Fri, Sep 18, 2015 at 11:45 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Sep 18, 2015 at 09:55:48PM CEST, sfeldma@gmail.com wrote:
>>From: Scott Feldman <sfeldma@gmail.com>
>>
>>Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>>---
>> include/linux/if_bridge.h |    4 ++++
>> 1 file changed, 4 insertions(+)
>>
>>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>>index dad8b00..6cc6dbc 100644
>>--- a/include/linux/if_bridge.h
>>+++ b/include/linux/if_bridge.h
>>@@ -46,6 +46,10 @@ 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)
>
> I think that a bridge patch checking against these values should be
> introduced along with these values, in the same patchset

I need the MIN value for this patchset in rocker's ageing timer, so
it's introduced here.  MIN/MAX will be used again in follow-on patch
Prem is going to send to range check user input.

RE: [PATCH net-next 4/7] bridge: define some min/max ageing time constants we'll use next

[Premkumar Jonnala]

> -----Original Message-----
> From: Scott Feldman [mailto:sfeldma@gmail.com]
> Sent: Saturday, September 19, 2015 10:51 PM
> To: Jiri Pirko
> Cc: Netdev; Siva Mannem; Premkumar Jonnala; stephen@networkplumber.org;
> Roopa Prabhu; andrew@lunn.ch; Florian Fainelli; Vivien Didelot
> Subject: Re: [PATCH net-next 4/7] bridge: define some min/max ageing time
> constants we'll use next
> 
> On Fri, Sep 18, 2015 at 11:45 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> > Fri, Sep 18, 2015 at 09:55:48PM CEST, sfeldma@gmail.com wrote:
> >>From: Scott Feldman <sfeldma@gmail.com>
> >>
> >>Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> >>---
> >> include/linux/if_bridge.h |    4 ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >>diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> >>index dad8b00..6cc6dbc 100644
> >>--- a/include/linux/if_bridge.h
> >>+++ b/include/linux/if_bridge.h
> >>@@ -46,6 +46,10 @@ 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)
> >
> > I think that a bridge patch checking against these values should be
> > introduced along with these values, in the same patchset
> 
> I need the MIN value for this patchset in rocker's ageing timer, so
> it's introduced here.  MIN/MAX will be used again in follow-on patch
> Prem is going to send to range check user input.

Ack that.  I will add those checks as part of the patch for iproute2.

-Prem

[PATCH net-next 5/7] rocker: add FDB cleanup timer

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

Add a timer to each rocker switch to do FDB entry cleanup by ageing out
expired entries.  The timer scheduling algo is copied from the bridge
driver, for the most part, to keep the firing of the timer to a minimum.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 drivers/net/ethernet/rocker/rocker.c |   41 ++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index eba22f5..232df69 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -248,6 +248,7 @@ struct rocker {
 	u64 flow_tbl_next_cookie;
 	DECLARE_HASHTABLE(group_tbl, 16);
 	spinlock_t group_tbl_lock;		/* for group tbl accesses */
+	struct timer_list fdb_cleanup_timer;
 	DECLARE_HASHTABLE(fdb_tbl, 16);
 	spinlock_t fdb_tbl_lock;		/* for fdb tbl accesses */
 	unsigned long internal_vlan_bitmap[ROCKER_INTERNAL_VLAN_BITMAP_LEN];
@@ -3706,6 +3707,41 @@ err_out:
 	return err;
 }
 
+static void rocker_fdb_cleanup(unsigned long data)
+{
+	struct rocker *rocker = (struct rocker *)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 expires;
+	unsigned long lock_flags;
+	int flags = ROCKER_OP_FLAG_NOWAIT | ROCKER_OP_FLAG_REMOVE |
+		    ROCKER_OP_FLAG_LEARNED;
+	int bkt;
+
+	spin_lock_irqsave(&rocker->fdb_tbl_lock, lock_flags);
+
+	hash_for_each_safe(rocker->fdb_tbl, bkt, tmp, entry, entry) {
+		if (!entry->learned)
+			continue;
+		rocker_port = entry->key.rocker_port;
+		expires = entry->touched + rocker_port->ageing_time;
+		if (time_before_eq(expires, jiffies)) {
+			rocker_port_fdb_learn(rocker_port, SWITCHDEV_TRANS_NONE,
+					      flags, entry->key.addr,
+					      entry->key.vlan_id);
+			hash_del(&entry->entry);
+		} else if (time_before(expires, next_timer)) {
+			next_timer = expires;
+		}
+	}
+
+	spin_unlock_irqrestore(&rocker->fdb_tbl_lock, lock_flags);
+
+	mod_timer(&rocker->fdb_cleanup_timer, round_jiffies_up(next_timer));
+}
+
 static int rocker_port_router_mac(struct rocker_port *rocker_port,
 				  enum switchdev_trans trans, int flags,
 				  __be16 vlan_id)
@@ -5191,6 +5227,10 @@ static int rocker_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err_init_tbls;
 	}
 
+	setup_timer(&rocker->fdb_cleanup_timer, rocker_fdb_cleanup,
+		    (unsigned long) rocker);
+	mod_timer(&rocker->fdb_cleanup_timer, jiffies);
+
 	err = rocker_probe_ports(rocker);
 	if (err) {
 		dev_err(&pdev->dev, "failed to probe ports\n");
@@ -5203,6 +5243,7 @@ static int rocker_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	return 0;
 
 err_probe_ports:
+	del_timer(&rocker->fdb_cleanup_timer);
 	rocker_free_tbls(rocker);
 err_init_tbls:
 	free_irq(rocker_msix_vector(rocker, ROCKER_MSIX_VEC_EVENT), rocker);
-- 
1.7.10.4

Re: [PATCH net-next 5/7] rocker: add FDB cleanup timer

[Jiri Pirko]

Fri, Sep 18, 2015 at 09:55:49PM CEST, sfeldma@gmail.com wrote:
>From: Scott Feldman <sfeldma@gmail.com>
>
>Add a timer to each rocker switch to do FDB entry cleanup by ageing out
>expired entries.  The timer scheduling algo is copied from the bridge
>driver, for the most part, to keep the firing of the timer to a minimum.
>
>Signed-off-by: Scott Feldman <sfeldma@gmail.com>

Acked-by: Jiri Pirko <jiri@resnulli.us>

[PATCH net-next 6/7] bridge: don't age externally added FDB entries

[sfeldma]

From: Siva Mannem <siva.mannem.lnx@gmail.com>

Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 net/bridge/br_fdb.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 9e9875d..6663cc0 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -299,6 +299,8 @@ void br_fdb_cleanup(unsigned long _data)
 			unsigned long this_timer;
 			if (f->is_static)
 				continue;
+			if (f->added_by_external_learn)
+				continue;
 			this_timer = f->updated + delay;
 			if (time_before_eq(this_timer, jiffies))
 				fdb_delete(br, f);
-- 
1.7.10.4

Re: [PATCH net-next 6/7] bridge: don't age externally added FDB entries

[Vivien Didelot]

On Sep. Friday 18 (38) 12:55 PM, sfeldma@gmail.com wrote:
> From: Siva Mannem <siva.mannem.lnx@gmail.com>
> 
> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>

Acked-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Re: [PATCH net-next 6/7] bridge: don't age externally added FDB entries

[Jiri Pirko]

Fri, Sep 18, 2015 at 09:55:50PM CEST, sfeldma@gmail.com wrote:
>From: Siva Mannem <siva.mannem.lnx@gmail.com>
>
>Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
>Signed-off-by: Scott Feldman <sfeldma@gmail.com>

Acked-by: Jiri Pirko <jiri@resnulli.us>

RE: [PATCH net-next 6/7] bridge: don't age externally added FDB entries

[Premkumar Jonnala]

> From: Siva Mannem <siva.mannem.lnx@gmail.com>
> 
> Signed-off-by: Siva Mannem <siva.mannem.lnx@gmail.com>
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>

Acked-by: Premkumar Jonnala <pjonnala@broadcom.com>

> ---
>  net/bridge/br_fdb.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 9e9875d..6663cc0 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -299,6 +299,8 @@ void br_fdb_cleanup(unsigned long _data)
>  			unsigned long this_timer;
>  			if (f->is_static)
>  				continue;
> +			if (f->added_by_external_learn)
> +				continue;
>  			this_timer = f->updated + delay;
>  			if (time_before_eq(this_timer, jiffies))
>  				fdb_delete(br, f);
> --
> 1.7.10.4

[PATCH net-next 7/7] switchdev: update documentation on FDB ageing_time

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 Documentation/networking/switchdev.txt |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/Documentation/networking/switchdev.txt b/Documentation/networking/switchdev.txt
index 476df04..67e43ee 100644
--- a/Documentation/networking/switchdev.txt
+++ b/Documentation/networking/switchdev.txt
@@ -239,20 +239,20 @@ The driver should initialize the attributes to the hardware defaults.
 FDB Ageing
 ^^^^^^^^^^
 
-There are two FDB ageing models supported: 1) ageing by the device, and 2)
-ageing by the kernel.  Ageing by the device is preferred if many FDB entries
-are supported.  The driver calls call_switchdev_notifiers(SWITCHDEV_FDB_DEL,
-...) to age out the FDB entry.  In this model, ageing by the kernel should be
-turned off.  XXX: how to turn off ageing in kernel on a per-port basis or
-otherwise prevent the kernel from ageing out the FDB entry?
-
-In the kernel ageing model, the standard bridge ageing mechanism is used to age
-out stale FDB entries.  To keep an FDB entry "alive", the driver should refresh
-the FDB entry by calling call_switchdev_notifiers(SWITCHDEV_FDB_ADD, ...).  The
+The bridge will skip ageing FDB entries marked with NTF_EXT_LEARNED and it is
+the responsibility of the port driver/device to age out these entries.  If the
+port device supports ageing, when the FDB entry expires, it will notify the
+driver which in turn will notify the bridge with SWITCHDEV_FDB_DEL.  If the
+device does not support ageing, the driver can simulate ageing using a
+garbage collection timer to monitor FBD entries.  Expired entries will be
+notified to the bridge using SWITCHDEV_FDB_DEL.  See rocker driver for
+example of driver running ageing timer.
+
+To keep an NTF_EXT_LEARNED entry "alive", the driver should refresh the FDB
+entry by calling call_switchdev_notifiers(SWITCHDEV_FDB_ADD, ...).  The
 notification will reset the FDB entry's last-used time to now.  The driver
 should rate limit refresh notifications, for example, no more than once a
-second.  If the FDB entry expires, fdb_delete is called to remove entry from
-the device.
+second.  (The last-used time is visible using the bridge -s fdb option).
 
 STP State Change on Port
 ^^^^^^^^^^^^^^^^^^^^^^^^
-- 
1.7.10.4

Re: [PATCH net-next 7/7] switchdev: update documentation on FDB ageing_time

[Vivien Didelot]

On Sep. Friday 18 (38) 12:55 PM, sfeldma@gmail.com wrote:
> From: Scott Feldman <sfeldma@gmail.com>
> 
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Re: [PATCH net-next 7/7] switchdev: update documentation on FDB ageing_time

[Jiri Pirko]

Fri, Sep 18, 2015 at 09:55:51PM CEST, sfeldma@gmail.com wrote:
>From: Scott Feldman <sfeldma@gmail.com>
>
>Signed-off-by: Scott Feldman <sfeldma@gmail.com>

Acked-by: Jiri Pirko <jiri@resnulli.us>

Re: [PATCH net-next 7/7] switchdev: update documentation on FDB ageing_time

[roopa]

On 9/18/15, 12:55 PM, sfeldma@gmail.com wrote:
> From: Scott Feldman <sfeldma@gmail.com>
>
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> ---
>   Documentation/networking/switchdev.txt |   24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/networking/switchdev.txt b/Documentation/networking/switchdev.txt
> index 476df04..67e43ee 100644
> --- a/Documentation/networking/switchdev.txt
> +++ b/Documentation/networking/switchdev.txt
> @@ -239,20 +239,20 @@ The driver should initialize the attributes to the hardware defaults.
>   FDB Ageing
>   ^^^^^^^^^^
>   
> -There are two FDB ageing models supported: 1) ageing by the device, and 2)
> -ageing by the kernel.  Ageing by the device is preferred if many FDB entries
> -are supported.  The driver calls call_switchdev_notifiers(SWITCHDEV_FDB_DEL,
> -...) to age out the FDB entry.  In this model, ageing by the kernel should be
> -turned off.  XXX: how to turn off ageing in kernel on a per-port basis or
> -otherwise prevent the kernel from ageing out the FDB entry?
> -
> -In the kernel ageing model, the standard bridge ageing mechanism is used to age
> -out stale FDB entries.  To keep an FDB entry "alive", the driver should refresh
> -the FDB entry by calling call_switchdev_notifiers(SWITCHDEV_FDB_ADD, ...).  The
> +The bridge will skip ageing FDB entries marked with NTF_EXT_LEARNED and it is
> +the responsibility of the port driver/device to age out these entries.  If the
> +port device supports ageing, when the FDB entry expires, it will notify the
> +driver which in turn will notify the bridge with SWITCHDEV_FDB_DEL.  If the
> +device does not support ageing, the driver can simulate ageing using a
> +garbage collection timer to monitor FBD entries.  Expired entries will be
> +notified to the bridge using SWITCHDEV_FDB_DEL.  See rocker driver for
> +example of driver running ageing timer.
We do rely on the bridge driver ageing out entries. We have gone from 
hardware ageing to ageing in the switch driver to ultimately ageing in 
the bridge driver.  :). And we keep the fdb entries in the bridge driver 
"alive" by using 'NTF_USE' from the user-space driver.

> +
> +To keep an NTF_EXT_LEARNED entry "alive", the driver should refresh the FDB
> +entry by calling call_switchdev_notifiers(SWITCHDEV_FDB_ADD, ...).  The
>
Even with your current patches, looks like the switch driver will need 
to refresh the entries anyways to keep the "last-used" time to now.
In which case is there much value in switch driver doing the ageing ?.

I am thinking keeping the default behavior of the bridge driver to age 
and anything else configurable might be a better option.

Thanks,
Roopa

Re: [PATCH net-next 7/7] switchdev: update documentation on FDB ageing_time

[Scott Feldman]

On Sat, Sep 19, 2015 at 6:21 PM, roopa <roopa@cumulusnetworks.com> wrote:
> On 9/18/15, 12:55 PM, sfeldma@gmail.com wrote:
>>
>> From: Scott Feldman <sfeldma@gmail.com>
>>
>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> ---
>>   Documentation/networking/switchdev.txt |   24 ++++++++++++------------
>>   1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/networking/switchdev.txt
>> b/Documentation/networking/switchdev.txt
>> index 476df04..67e43ee 100644
>> --- a/Documentation/networking/switchdev.txt
>> +++ b/Documentation/networking/switchdev.txt
>> @@ -239,20 +239,20 @@ The driver should initialize the attributes to the
>> hardware defaults.
>>   FDB Ageing
>>   ^^^^^^^^^^
>>   -There are two FDB ageing models supported: 1) ageing by the device, and
>> 2)
>> -ageing by the kernel.  Ageing by the device is preferred if many FDB
>> entries
>> -are supported.  The driver calls
>> call_switchdev_notifiers(SWITCHDEV_FDB_DEL,
>> -...) to age out the FDB entry.  In this model, ageing by the kernel
>> should be
>> -turned off.  XXX: how to turn off ageing in kernel on a per-port basis or
>> -otherwise prevent the kernel from ageing out the FDB entry?
>> -
>> -In the kernel ageing model, the standard bridge ageing mechanism is used
>> to age
>> -out stale FDB entries.  To keep an FDB entry "alive", the driver should
>> refresh
>> -the FDB entry by calling call_switchdev_notifiers(SWITCHDEV_FDB_ADD,
>> ...).  The
>> +The bridge will skip ageing FDB entries marked with NTF_EXT_LEARNED and
>> it is
>> +the responsibility of the port driver/device to age out these entries.
>> If the
>> +port device supports ageing, when the FDB entry expires, it will notify
>> the
>> +driver which in turn will notify the bridge with SWITCHDEV_FDB_DEL.  If
>> the
>> +device does not support ageing, the driver can simulate ageing using a
>> +garbage collection timer to monitor FBD entries.  Expired entries will be
>> +notified to the bridge using SWITCHDEV_FDB_DEL.  See rocker driver for
>> +example of driver running ageing timer.
>
> We do rely on the bridge driver ageing out entries. We have gone from
> hardware ageing to ageing in the switch driver to ultimately ageing in the
> bridge driver.  :). And we keep the fdb entries in the bridge driver "alive"
> by using 'NTF_USE' from the user-space driver.

Yes, your switch driver is in user-space so you have to use NTF_USE to
refresh the entry since you cannot use the kernel driver model to
call_switchdev_notifiers(SWITCHDEV_FDB_ADD, ...).  Consequently, your
entries are not marked with NTF_EXT_LEARNED, so this patch is a no-op
for you.  You can continue to use the bridge driver to age out your
entries.

>> +To keep an NTF_EXT_LEARNED entry "alive", the driver should refresh the
>> FDB
>> +entry by calling call_switchdev_notifiers(SWITCHDEV_FDB_ADD, ...).  The
>>
> Even with your current patches, looks like the switch driver will need to
> refresh the entries anyways to keep the "last-used" time to now.
> In which case is there much value in switch driver doing the ageing ?.

"should" not "must".

Value is for the many learned FDB entries case, to move the ageing
function to hardware.

> I am thinking keeping the default behavior of the bridge driver to age and
> anything else configurable might be a better option.

I'd rather someone add that knob when it's actually needed.  When the
first in-kernel switchdev driver that wants to use the bridge driver's
ageing function, then we can make that adjustment.

Re: [PATCH net-next 7/7] switchdev: update documentation on FDB ageing_time

[roopa]

On 9/19/15, 7:21 PM, Scott Feldman wrote:
> Yes, your switch driver is in user-space so you have to use NTF_USE to
> refresh the entry since you cannot use the kernel driver model to
> call_switchdev_notifiers(SWITCHDEV_FDB_ADD, ...).  Consequently, your
> entries are not marked with NTF_EXT_LEARNED, so this patch is a no-op
> for you.  You can continue to use the bridge driver to age out your
> entries.
yes, correct.  I was not really saying this because it will cause us any 
problems.
I was trying to say this for switchdev in general.

> I'd rather someone add that knob when it's actually needed. When the 
> first in-kernel switchdev driver that wants to use the bridge driver's 
> ageing function, then we can make that adjustment. 
I was suggesting the other way around. Keep the default to what is in 
the kernel today and the first in-kernel switchdev driver that wants to age,
should introduce the ability to not age in the bridge driver (Rocker 
will continue to work as it does today). Because, I am only concerned 
that rocker may end up being the only device that uses the default 
behavior introduced by this patch. And every real hardware uses the 
bridge driver to age (because there are no in kernel examples today).  I 
am curious to know who else is using hardware ageing today.

Re: [PATCH net-next 7/7] switchdev: update documentation on FDB ageing_time

[Scott Feldman]

On Sun, Sep 20, 2015 at 7:24 AM, roopa <roopa@cumulusnetworks.com> wrote:
> On 9/19/15, 7:21 PM, Scott Feldman wrote:
>>
>> Yes, your switch driver is in user-space so you have to use NTF_USE to
>> refresh the entry since you cannot use the kernel driver model to
>> call_switchdev_notifiers(SWITCHDEV_FDB_ADD, ...).  Consequently, your
>> entries are not marked with NTF_EXT_LEARNED, so this patch is a no-op
>> for you.  You can continue to use the bridge driver to age out your
>> entries.
>
> yes, correct.  I was not really saying this because it will cause us any
> problems.
> I was trying to say this for switchdev in general.
>
>> I'd rather someone add that knob when it's actually needed. When the first
>> in-kernel switchdev driver that wants to use the bridge driver's ageing
>> function, then we can make that adjustment.
>
> I was suggesting the other way around. Keep the default to what is in the
> kernel today and the first in-kernel switchdev driver that wants to age,
> should introduce the ability to not age in the bridge driver (Rocker will
> continue to work as it does today). Because, I am only concerned that rocker
> may end up being the only device that uses the default behavior introduced
> by this patch. And every real hardware uses the bridge driver to age
> (because there are no in kernel examples today).  I am curious to know who
> else is using hardware ageing today.

A driver patch for a (real) hardware device which does the ageing in
hw is around the corner.