[patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Jiri Pirko]

From: Scott Feldman <sfeldma@gmail.com>

When the swdev device learns a new mac/vlan on a port, it sends some async
notification to the driver and the driver installs an FDB in the device.
To give a holistic system view, the learned mac/vlan should be reflected
in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
what is currently learned by the device.  This API on the bridge driver gives
a way for the swdev driver to install an FBD entry in the bridge FBD table.
(And remove one).

This is equivalent to the device running these cmds:

  bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master

This patch needs some extra eyeballs for review, in paricular around the
locking and contexts.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
v2->v3:
-added "external" word into function names to emphasize fdbs are learned
 externally
-added "added_by_external_learn" to fbd entry struct indicate the entry
 was learned externaly and build some logic around that
-expose the fact that fdb entry was learned externally to userspace
v1->v2:
-no change
---
 include/linux/if_bridge.h      | 18 +++++++++
 include/uapi/linux/neighbour.h |  1 +
 net/bridge/br_fdb.c            | 91 +++++++++++++++++++++++++++++++++++++++++-
 net/bridge/br_private.h        |  1 +
 4 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index 808dcb8..fa2eca6 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -37,6 +37,24 @@ extern void brioctl_set(int (*ioctl_hook)(struct net *, unsigned int, void __use
 typedef int br_should_route_hook_t(struct sk_buff *skb);
 extern br_should_route_hook_t __rcu *br_should_route_hook;
 
+#if IS_ENABLED(CONFIG_BRIDGE)
+int br_fdb_external_learn_add(struct net_device *dev,
+			      const unsigned char *addr, u16 vid);
+int br_fdb_external_learn_del(struct net_device *dev,
+			      const unsigned char *addr, u16 vid);
+#else
+static inline int br_fdb_external_learn_add(struct net_device *dev,
+					    const unsigned char *addr, u16 vid)
+{
+	return 0;
+}
+static inline int br_fdb_external_learn_del(struct net_device *dev,
+					    const unsigned char *addr, u16 vid)
+{
+	return 0;
+}
+#endif
+
 #if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_BRIDGE_IGMP_SNOOPING)
 int br_multicast_list_adjacent(struct net_device *dev,
 			       struct list_head *br_ip_list);
diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index 4a1d7e9..3a9b0df 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -40,6 +40,7 @@ enum {
 
 #define NTF_SELF	0x02
 #define NTF_MASTER	0x04
+#define NTF_EXT_LEARNED	0x10
 
 /*
  *	Neighbor Cache Entry States.
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index b1be971..b42e71d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -481,6 +481,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
 		fdb->is_local = 0;
 		fdb->is_static = 0;
 		fdb->added_by_user = 0;
+		fdb->added_by_external_learn = 0;
 		fdb->updated = fdb->used = jiffies;
 		hlist_add_head_rcu(&fdb->hlist, head);
 	}
@@ -613,7 +614,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
 	ndm->ndm_family	 = AF_BRIDGE;
 	ndm->ndm_pad1    = 0;
 	ndm->ndm_pad2    = 0;
-	ndm->ndm_flags	 = 0;
+	ndm->ndm_flags	 = fdb->added_by_external_learn ? NTF_EXT_LEARNED : 0;
 	ndm->ndm_type	 = 0;
 	ndm->ndm_ifindex = fdb->dst ? fdb->dst->dev->ifindex : br->dev->ifindex;
 	ndm->ndm_state   = fdb_to_nud(fdb);
@@ -983,3 +984,91 @@ void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
 		}
 	}
 }
+
+int br_fdb_external_learn_add(struct net_device *dev,
+			      const unsigned char *addr, u16 vid)
+{
+	struct net_bridge_port *p;
+	struct net_bridge *br;
+	struct hlist_head *head;
+	struct net_bridge_fdb_entry *fdb;
+	int err = 0;
+
+	rtnl_lock();
+
+	p = br_port_get_rtnl(dev);
+	if (!p) {
+		pr_info("bridge: %s not a bridge port\n", dev->name);
+		err = -EINVAL;
+		goto err_rtnl_unlock;
+	}
+
+	br = p->br;
+
+	spin_lock(&br->hash_lock);
+
+	head = &br->hash[br_mac_hash(addr, vid)];
+	fdb = fdb_find(head, addr, vid);
+	if (!fdb) {
+		fdb = fdb_create(head, p, addr, vid);
+		if (!fdb) {
+			err = -ENOMEM;
+			goto err_unlock;
+		}
+		fdb->added_by_external_learn = 1;
+		fdb_notify(br, fdb, RTM_NEWNEIGH);
+	} else if (fdb->added_by_external_learn) {
+		/* Refresh entry */
+		fdb->updated = fdb->used = jiffies;
+	} else if (!fdb->added_by_user) {
+		/* Take over SW learned entry */
+		fdb->added_by_external_learn = 1;
+		fdb->updated = jiffies;
+		fdb_notify(br, fdb, RTM_NEWNEIGH);
+	}
+
+err_unlock:
+	spin_unlock(&br->hash_lock);
+err_rtnl_unlock:
+	rtnl_unlock();
+
+	return err;
+}
+EXPORT_SYMBOL(br_fdb_external_learn_add);
+
+int br_fdb_external_learn_del(struct net_device *dev,
+			      const unsigned char *addr, u16 vid)
+{
+	struct net_bridge_port *p;
+	struct net_bridge *br;
+	struct hlist_head *head;
+	struct net_bridge_fdb_entry *fdb;
+	int err = 0;
+
+	rtnl_lock();
+
+	p = br_port_get_rtnl(dev);
+	if (!p) {
+		pr_info("bridge: %s not a bridge port\n", dev->name);
+		err = -EINVAL;
+		goto err_rtnl_unlock;
+	}
+
+	br = p->br;
+
+	spin_lock(&br->hash_lock);
+
+	head = &br->hash[br_mac_hash(addr, vid)];
+	fdb = fdb_find(head, addr, vid);
+	if (fdb && fdb->added_by_external_learn)
+		fdb_delete(br, fdb);
+	else
+		err = -ENOENT;
+
+	spin_unlock(&br->hash_lock);
+err_rtnl_unlock:
+	rtnl_unlock();
+
+	return err;
+}
+EXPORT_SYMBOL(br_fdb_external_learn_del);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4f577c4..02cd63b 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -101,6 +101,7 @@ struct net_bridge_fdb_entry
 	unsigned char			is_local;
 	unsigned char			is_static;
 	unsigned char			added_by_user;
+	unsigned char			added_by_external_learn;
 	__u16				vlan_id;
 };
 
-- 
1.9.3

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Jamal Hadi Salim]

On 11/25/14 05:28, Jiri Pirko wrote:
> From: Scott Feldman <sfeldma@gmail.com>
>
> When the swdev device learns a new mac/vlan on a port, it sends some async
> notification to the driver and the driver installs an FDB in the device.
> To give a holistic system view, the learned mac/vlan should be reflected
> in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
> what is currently learned by the device.  This API on the bridge driver gives
> a way for the swdev driver to install an FBD entry in the bridge FBD table.
> (And remove one).
>
> This is equivalent to the device running these cmds:
>
>    bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
>
> This patch needs some extra eyeballs for review, in paricular around the
> locking and contexts.
>
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Like it (thanks for taking the earlier feedback) but I do not see
other issue we discussed on policy attribute check that
says "I need you to sync this from offload/chip to kernel" so for now
only conditional Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Andy Gospodarek]

On Tue, Nov 25, 2014 at 11:28:40AM +0100, Jiri Pirko wrote:
> From: Scott Feldman <sfeldma@gmail.com>
> 
> When the swdev device learns a new mac/vlan on a port, it sends some async
> notification to the driver and the driver installs an FDB in the device.
> To give a holistic system view, the learned mac/vlan should be reflected
> in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
> what is currently learned by the device.  This API on the bridge driver gives
> a way for the swdev driver to install an FBD entry in the bridge FBD table.
> (And remove one).
> 
> This is equivalent to the device running these cmds:
> 
>   bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
> 
> This patch needs some extra eyeballs for review, in paricular around the
> locking and contexts.
> 
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
> v2->v3:
> -added "external" word into function names to emphasize fdbs are learned
>  externally
> -added "added_by_external_learn" to fbd entry struct indicate the entry
>  was learned externaly and build some logic around that
> -expose the fact that fdb entry was learned externally to userspace
> v1->v2:
> -no change
> ---
>  include/linux/if_bridge.h      | 18 +++++++++
>  include/uapi/linux/neighbour.h |  1 +
>  net/bridge/br_fdb.c            | 91 +++++++++++++++++++++++++++++++++++++++++-
>  net/bridge/br_private.h        |  1 +
>  4 files changed, 110 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index 808dcb8..fa2eca6 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -37,6 +37,24 @@ extern void brioctl_set(int (*ioctl_hook)(struct net *, unsigned int, void __use
>  typedef int br_should_route_hook_t(struct sk_buff *skb);
>  extern br_should_route_hook_t __rcu *br_should_route_hook;
>  
> +#if IS_ENABLED(CONFIG_BRIDGE)
> +int br_fdb_external_learn_add(struct net_device *dev,
> +			      const unsigned char *addr, u16 vid);
> +int br_fdb_external_learn_del(struct net_device *dev,
> +			      const unsigned char *addr, u16 vid);
> +#else
> +static inline int br_fdb_external_learn_add(struct net_device *dev,
> +					    const unsigned char *addr, u16 vid)
> +{
> +	return 0;
> +}
> +static inline int br_fdb_external_learn_del(struct net_device *dev,
> +					    const unsigned char *addr, u16 vid)
> +{
> +	return 0;
> +}
> +#endif
> +
>  #if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_BRIDGE_IGMP_SNOOPING)
>  int br_multicast_list_adjacent(struct net_device *dev,
>  			       struct list_head *br_ip_list);
> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
> index 4a1d7e9..3a9b0df 100644
> --- a/include/uapi/linux/neighbour.h
> +++ b/include/uapi/linux/neighbour.h
> @@ -40,6 +40,7 @@ enum {
>  
>  #define NTF_SELF	0x02
>  #define NTF_MASTER	0x04
> +#define NTF_EXT_LEARNED	0x10
>  
>  /*
>   *	Neighbor Cache Entry States.
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index b1be971..b42e71d 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -481,6 +481,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct hlist_head *head,
>  		fdb->is_local = 0;
>  		fdb->is_static = 0;
>  		fdb->added_by_user = 0;
> +		fdb->added_by_external_learn = 0;
>  		fdb->updated = fdb->used = jiffies;
>  		hlist_add_head_rcu(&fdb->hlist, head);
>  	}
> @@ -613,7 +614,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
>  	ndm->ndm_family	 = AF_BRIDGE;
>  	ndm->ndm_pad1    = 0;
>  	ndm->ndm_pad2    = 0;
> -	ndm->ndm_flags	 = 0;
> +	ndm->ndm_flags	 = fdb->added_by_external_learn ? NTF_EXT_LEARNED : 0;
>  	ndm->ndm_type	 = 0;
>  	ndm->ndm_ifindex = fdb->dst ? fdb->dst->dev->ifindex : br->dev->ifindex;
>  	ndm->ndm_state   = fdb_to_nud(fdb);
> @@ -983,3 +984,91 @@ void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
>  		}
>  	}
>  }
> +
> +int br_fdb_external_learn_add(struct net_device *dev,
> +			      const unsigned char *addr, u16 vid)
> +{
> +	struct net_bridge_port *p;
> +	struct net_bridge *br;
> +	struct hlist_head *head;
> +	struct net_bridge_fdb_entry *fdb;
> +	int err = 0;
> +
> +	rtnl_lock();
> +
> +	p = br_port_get_rtnl(dev);
> +	if (!p) {
> +		pr_info("bridge: %s not a bridge port\n", dev->name);
> +		err = -EINVAL;
> +		goto err_rtnl_unlock;
> +	}
> +
> +	br = p->br;
> +
> +	spin_lock(&br->hash_lock);
(Since you asked to check locking...)

Most of the other fdb_add/delete/insert/update calls take this with
spin_lock_bh.  Did you try this with lockdep enabled just to see if that
is needed here?  I suspect that anytime br->hash_lock is taken it will
need to be with softirqs disabled from this point forward.

> +
> +	head = &br->hash[br_mac_hash(addr, vid)];
> +	fdb = fdb_find(head, addr, vid);
> +	if (!fdb) {
> +		fdb = fdb_create(head, p, addr, vid);
> +		if (!fdb) {
> +			err = -ENOMEM;
> +			goto err_unlock;
> +		}
> +		fdb->added_by_external_learn = 1;
> +		fdb_notify(br, fdb, RTM_NEWNEIGH);
> +	} else if (fdb->added_by_external_learn) {
> +		/* Refresh entry */
> +		fdb->updated = fdb->used = jiffies;
> +	} else if (!fdb->added_by_user) {
> +		/* Take over SW learned entry */
> +		fdb->added_by_external_learn = 1;
> +		fdb->updated = jiffies;
> +		fdb_notify(br, fdb, RTM_NEWNEIGH);
> +	}
> +
> +err_unlock:
> +	spin_unlock(&br->hash_lock);
> +err_rtnl_unlock:
> +	rtnl_unlock();
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(br_fdb_external_learn_add);
> +
> +int br_fdb_external_learn_del(struct net_device *dev,
> +			      const unsigned char *addr, u16 vid)
> +{
> +	struct net_bridge_port *p;
> +	struct net_bridge *br;
> +	struct hlist_head *head;
> +	struct net_bridge_fdb_entry *fdb;
> +	int err = 0;
> +
> +	rtnl_lock();
> +
> +	p = br_port_get_rtnl(dev);
> +	if (!p) {
> +		pr_info("bridge: %s not a bridge port\n", dev->name);
> +		err = -EINVAL;
> +		goto err_rtnl_unlock;
> +	}
> +
> +	br = p->br;
> +
> +	spin_lock(&br->hash_lock);
Same comment as above here.

> +
> +	head = &br->hash[br_mac_hash(addr, vid)];
> +	fdb = fdb_find(head, addr, vid);
> +	if (fdb && fdb->added_by_external_learn)
> +		fdb_delete(br, fdb);
> +	else
> +		err = -ENOENT;
> +
> +	spin_unlock(&br->hash_lock);
> +err_rtnl_unlock:
> +	rtnl_unlock();
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(br_fdb_external_learn_del);
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 4f577c4..02cd63b 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -101,6 +101,7 @@ struct net_bridge_fdb_entry
>  	unsigned char			is_local;
>  	unsigned char			is_static;
>  	unsigned char			added_by_user;
> +	unsigned char			added_by_external_learn;
>  	__u16				vlan_id;
>  };
>  
> -- 
> 1.9.3
> 

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Thomas Graf]

On 11/25/14 at 11:38am, Andy Gospodarek wrote:
> On Tue, Nov 25, 2014 at 11:28:40AM +0100, Jiri Pirko wrote:
> > From: Scott Feldman <sfeldma@gmail.com>
> > 
> > When the swdev device learns a new mac/vlan on a port, it sends some async
> > notification to the driver and the driver installs an FDB in the device.
> > To give a holistic system view, the learned mac/vlan should be reflected
> > in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
> > what is currently learned by the device.  This API on the bridge driver gives
> > a way for the swdev driver to install an FBD entry in the bridge FBD table.
> > (And remove one).
> > 
> > This is equivalent to the device running these cmds:
> > 
> >   bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
> > 
> > This patch needs some extra eyeballs for review, in paricular around the
> > locking and contexts.
> > 
> > Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> > Signed-off-by: Jiri Pirko <jiri@resnulli.us>

I like the simplicity of this. That said, given we'll have multiple
users of swdev including OVS, shouldn't this be a notifier or a
callback that depends on who is controlling the device?

> > +	spin_lock(&br->hash_lock);
> (Since you asked to check locking...)
> 
> Most of the other fdb_add/delete/insert/update calls take this with
> spin_lock_bh.  Did you try this with lockdep enabled just to see if that
> is needed here?  I suspect that anytime br->hash_lock is taken it will
> need to be with softirqs disabled from this point forward.

At least br_fdb_update() seems to be called from BH context so I would
agree and argue the lock in br_fdb_cleanup() and br_fdb_update() need a
fix too. I'll send a patch.

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Florian Fainelli]

On 25/11/14 02:28, Jiri Pirko wrote:
> From: Scott Feldman <sfeldma@gmail.com>
> 
> When the swdev device learns a new mac/vlan on a port, it sends some async
> notification to the driver and the driver installs an FDB in the device.
> To give a holistic system view, the learned mac/vlan should be reflected
> in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
> what is currently learned by the device.  This API on the bridge driver gives
> a way for the swdev driver to install an FBD entry in the bridge FBD table.
> (And remove one).
> 
> This is equivalent to the device running these cmds:
> 
>   bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
> 
> This patch needs some extra eyeballs for review, in paricular around the
> locking and contexts.
> 
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---

[snip]

> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
> index 4a1d7e9..3a9b0df 100644
> --- a/include/uapi/linux/neighbour.h
> +++ b/include/uapi/linux/neighbour.h
> @@ -40,6 +40,7 @@ enum {
>  
>  #define NTF_SELF	0x02
>  #define NTF_MASTER	0x04
> +#define NTF_EXT_LEARNED	0x10

This file could use some re-ordering to have the constants in ascending
order.

[snip]

> +	head = &br->hash[br_mac_hash(addr, vid)];
> +	fdb = fdb_find(head, addr, vid);
> +	if (!fdb) {
> +		fdb = fdb_create(head, p, addr, vid);
> +		if (!fdb) {
> +			err = -ENOMEM;
> +			goto err_unlock;
> +		}
> +		fdb->added_by_external_learn = 1;
> +		fdb_notify(br, fdb, RTM_NEWNEIGH);
> +	} else if (fdb->added_by_external_learn) {
> +		/* Refresh entry */
> +		fdb->updated = fdb->used = jiffies;
> +	} else if (!fdb->added_by_user) {
> +		/* Take over SW learned entry */
> +		fdb->added_by_external_learn = 1;
> +		fdb->updated = jiffies;
> +		fdb_notify(br, fdb, RTM_NEWNEIGH);
> +	}

Is there any case where this fdb entry gets re-used and is no longer
added by an external learning? Should we clear this flag somewhere?

[snip]

> +EXPORT_SYMBOL(br_fdb_external_learn_del);
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 4f577c4..02cd63b 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -101,6 +101,7 @@ struct net_bridge_fdb_entry
>  	unsigned char			is_local;
>  	unsigned char			is_static;
>  	unsigned char			added_by_user;
> +	unsigned char			added_by_external_learn;

Pheww, we could be saving some memory footprint here by using different
types here ;)
--
Florian

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Scott Feldman]

On Tue, Nov 25, 2014 at 12:36 PM, Thomas Graf <tgraf@suug.ch> wrote:
> On 11/25/14 at 11:38am, Andy Gospodarek wrote:
>> On Tue, Nov 25, 2014 at 11:28:40AM +0100, Jiri Pirko wrote:
>> > From: Scott Feldman <sfeldma@gmail.com>
>> >
>> > When the swdev device learns a new mac/vlan on a port, it sends some async
>> > notification to the driver and the driver installs an FDB in the device.
>> > To give a holistic system view, the learned mac/vlan should be reflected
>> > in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
>> > what is currently learned by the device.  This API on the bridge driver gives
>> > a way for the swdev driver to install an FBD entry in the bridge FBD table.
>> > (And remove one).
>> >
>> > This is equivalent to the device running these cmds:
>> >
>> >   bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
>> >
>> > This patch needs some extra eyeballs for review, in paricular around the
>> > locking and contexts.
>> >
>> > Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> > Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>
> I like the simplicity of this. That said, given we'll have multiple
> users of swdev including OVS, shouldn't this be a notifier or a
> callback that depends on who is controlling the device?

I like the idea.  When the switch port joins Linux bridge or OVS
datapath, a callback is registered with the driver.  That way the
driver doesn't really care if the port is a bridge member or an OVS
vport in a datapath.  It's just passing up the FDB entry
(port/mac/vlan) details to the container device.  Can we hold this
idea until this patchset sticks?  I think once OVS support comes back
into the swdev model would be the time to address this.

>
>> > +   spin_lock(&br->hash_lock);
>> (Since you asked to check locking...)
>>
>> Most of the other fdb_add/delete/insert/update calls take this with
>> spin_lock_bh.  Did you try this with lockdep enabled just to see if that
>> is needed here?  I suspect that anytime br->hash_lock is taken it will
>> need to be with softirqs disabled from this point forward.
>
> At least br_fdb_update() seems to be called from BH context so I would
> agree and argue the lock in br_fdb_cleanup() and br_fdb_update() need a
> fix too. I'll send a patch.

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Scott Feldman]

On Tue, Nov 25, 2014 at 12:44 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 25/11/14 02:28, Jiri Pirko wrote:
>> From: Scott Feldman <sfeldma@gmail.com>
>>
>> When the swdev device learns a new mac/vlan on a port, it sends some async
>> notification to the driver and the driver installs an FDB in the device.
>> To give a holistic system view, the learned mac/vlan should be reflected
>> in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
>> what is currently learned by the device.  This API on the bridge driver gives
>> a way for the swdev driver to install an FBD entry in the bridge FBD table.
>> (And remove one).
>>
>> This is equivalent to the device running these cmds:
>>
>>   bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
>>
>> This patch needs some extra eyeballs for review, in paricular around the
>> locking and contexts.
>>
>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>
> [snip]
>
>> +     head = &br->hash[br_mac_hash(addr, vid)];
>> +     fdb = fdb_find(head, addr, vid);
>> +     if (!fdb) {
>> +             fdb = fdb_create(head, p, addr, vid);
>> +             if (!fdb) {
>> +                     err = -ENOMEM;
>> +                     goto err_unlock;
>> +             }
>> +             fdb->added_by_external_learn = 1;
>> +             fdb_notify(br, fdb, RTM_NEWNEIGH);
>> +     } else if (fdb->added_by_external_learn) {
>> +             /* Refresh entry */
>> +             fdb->updated = fdb->used = jiffies;
>> +     } else if (!fdb->added_by_user) {
>> +             /* Take over SW learned entry */
>> +             fdb->added_by_external_learn = 1;
>> +             fdb->updated = jiffies;
>> +             fdb_notify(br, fdb, RTM_NEWNEIGH);
>> +     }
>
> Is there any case where this fdb entry gets re-used and is no longer
> added by an external learning? Should we clear this flag somewhere?

Once the FDB entry is marked "added_by_external_learn" it stays marked
as such until removed by aging cleanup process (or flushed due to
interface down, etc).  If aged out (and now deleted), the FDB entry
may come back either by SW learn or by HW learn.  If SW learn comes
first, and then HW learn, HW learn will override and mark the existing
FDB entry "added_by_external_learn".  So there is take-over by HW but
no give-back to SW.  And there is no explicit clearing of the mark
short of deleting the FDB entry.  The mark is mostly for letting
user's know which FDB entries where learned by HW and synced to the
bridge's FDB.

> [snip]
>
>> +EXPORT_SYMBOL(br_fdb_external_learn_del);
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 4f577c4..02cd63b 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -101,6 +101,7 @@ struct net_bridge_fdb_entry
>>       unsigned char                   is_local;
>>       unsigned char                   is_static;
>>       unsigned char                   added_by_user;
>> +     unsigned char                   added_by_external_learn;
>
> Pheww, we could be saving some memory footprint here by using different
> types here ;)
> --
> Florian

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Florian Fainelli]

On 25/11/14 18:03, Scott Feldman wrote:
> On Tue, Nov 25, 2014 at 12:44 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 25/11/14 02:28, Jiri Pirko wrote:
>>> From: Scott Feldman <sfeldma@gmail.com>
>>>
>>> When the swdev device learns a new mac/vlan on a port, it sends some async
>>> notification to the driver and the driver installs an FDB in the device.
>>> To give a holistic system view, the learned mac/vlan should be reflected
>>> in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
>>> what is currently learned by the device.  This API on the bridge driver gives
>>> a way for the swdev driver to install an FBD entry in the bridge FBD table.
>>> (And remove one).
>>>
>>> This is equivalent to the device running these cmds:
>>>
>>>   bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
>>>
>>> This patch needs some extra eyeballs for review, in paricular around the
>>> locking and contexts.
>>>
>>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>> ---
>>
>> [snip]
>>
>>> +     head = &br->hash[br_mac_hash(addr, vid)];
>>> +     fdb = fdb_find(head, addr, vid);
>>> +     if (!fdb) {
>>> +             fdb = fdb_create(head, p, addr, vid);
>>> +             if (!fdb) {
>>> +                     err = -ENOMEM;
>>> +                     goto err_unlock;
>>> +             }
>>> +             fdb->added_by_external_learn = 1;
>>> +             fdb_notify(br, fdb, RTM_NEWNEIGH);
>>> +     } else if (fdb->added_by_external_learn) {
>>> +             /* Refresh entry */
>>> +             fdb->updated = fdb->used = jiffies;
>>> +     } else if (!fdb->added_by_user) {
>>> +             /* Take over SW learned entry */
>>> +             fdb->added_by_external_learn = 1;
>>> +             fdb->updated = jiffies;
>>> +             fdb_notify(br, fdb, RTM_NEWNEIGH);
>>> +     }
>>
>> Is there any case where this fdb entry gets re-used and is no longer
>> added by an external learning? Should we clear this flag somewhere?
> 
> Once the FDB entry is marked "added_by_external_learn" it stays marked
> as such until removed by aging cleanup process (or flushed due to
> interface down, etc).  If aged out (and now deleted), the FDB entry
> may come back either by SW learn or by HW learn.  If SW learn comes
> first, and then HW learn, HW learn will override and mark the existing
> FDB entry "added_by_external_learn".  So there is take-over by HW but
> no give-back to SW.  And there is no explicit clearing of the mark
> short of deleting the FDB entry.  The mark is mostly for letting
> user's know which FDB entries where learned by HW and synced to the
> bridge's FDB.

Thanks, makes sense now. This is probably obvious in this context, but
maybe it would not hurt to come up with a documentation that describe
the offload API, FDB entry lifetime and HW/SW ownership etc...

> 
>> [snip]
>>
>>> +EXPORT_SYMBOL(br_fdb_external_learn_del);
>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>> index 4f577c4..02cd63b 100644
>>> --- a/net/bridge/br_private.h
>>> +++ b/net/bridge/br_private.h
>>> @@ -101,6 +101,7 @@ struct net_bridge_fdb_entry
>>>       unsigned char                   is_local;
>>>       unsigned char                   is_static;
>>>       unsigned char                   added_by_user;
>>> +     unsigned char                   added_by_external_learn;
>>
>> Pheww, we could be saving some memory footprint here by using different
>> types here ;)
>> --
>> Florian

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Scott Feldman]

On Tue, Nov 25, 2014 at 4:34 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 25/11/14 18:03, Scott Feldman wrote:
>> On Tue, Nov 25, 2014 at 12:44 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> On 25/11/14 02:28, Jiri Pirko wrote:
>>>> From: Scott Feldman <sfeldma@gmail.com>
>>>>
>>>> When the swdev device learns a new mac/vlan on a port, it sends some async
>>>> notification to the driver and the driver installs an FDB in the device.
>>>> To give a holistic system view, the learned mac/vlan should be reflected
>>>> in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
>>>> what is currently learned by the device.  This API on the bridge driver gives
>>>> a way for the swdev driver to install an FBD entry in the bridge FBD table.
>>>> (And remove one).
>>>>
>>>> This is equivalent to the device running these cmds:
>>>>
>>>>   bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
>>>>
>>>> This patch needs some extra eyeballs for review, in paricular around the
>>>> locking and contexts.
>>>>
>>>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>>> ---
>>>
>>> [snip]
>>>
>>>> +     head = &br->hash[br_mac_hash(addr, vid)];
>>>> +     fdb = fdb_find(head, addr, vid);
>>>> +     if (!fdb) {
>>>> +             fdb = fdb_create(head, p, addr, vid);
>>>> +             if (!fdb) {
>>>> +                     err = -ENOMEM;
>>>> +                     goto err_unlock;
>>>> +             }
>>>> +             fdb->added_by_external_learn = 1;
>>>> +             fdb_notify(br, fdb, RTM_NEWNEIGH);
>>>> +     } else if (fdb->added_by_external_learn) {
>>>> +             /* Refresh entry */
>>>> +             fdb->updated = fdb->used = jiffies;
>>>> +     } else if (!fdb->added_by_user) {
>>>> +             /* Take over SW learned entry */
>>>> +             fdb->added_by_external_learn = 1;
>>>> +             fdb->updated = jiffies;
>>>> +             fdb_notify(br, fdb, RTM_NEWNEIGH);
>>>> +     }
>>>
>>> Is there any case where this fdb entry gets re-used and is no longer
>>> added by an external learning? Should we clear this flag somewhere?
>>
>> Once the FDB entry is marked "added_by_external_learn" it stays marked
>> as such until removed by aging cleanup process (or flushed due to
>> interface down, etc).  If aged out (and now deleted), the FDB entry
>> may come back either by SW learn or by HW learn.  If SW learn comes
>> first, and then HW learn, HW learn will override and mark the existing
>> FDB entry "added_by_external_learn".  So there is take-over by HW but
>> no give-back to SW.  And there is no explicit clearing of the mark
>> short of deleting the FDB entry.  The mark is mostly for letting
>> user's know which FDB entries where learned by HW and synced to the
>> bridge's FDB.
>
> Thanks, makes sense now. This is probably obvious in this context, but
> maybe it would not hurt to come up with a documentation that describe
> the offload API, FDB entry lifetime and HW/SW ownership etc...

I have an updated Documentation/networking/switchdev.txt that covers
the swdev APIs and usage and notes, but Jiri is being stingy with it.
Will get this out, either in v4 or follow-on patches.  There is enough
going on just with L2 offload that we're going to need some good
documentation to guide implementers.

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Jamal Hadi Salim]

On 11/25/14 17:44, Florian Fainelli wrote:
> On 25/11/14 02:28, Jiri Pirko wrote:

>> @@ -101,6 +101,7 @@ struct net_bridge_fdb_entry
>>   	unsigned char			is_local;
>>   	unsigned char			is_static;
>>   	unsigned char			added_by_user;
>> +	unsigned char			added_by_external_learn;
>
> Pheww, we could be saving some memory footprint here by using different
> types here ;)

I tried to bring up this issue earlier.
Unfortuately about 15 different things being transfered over bridge
netlink use 8 bits for representing a bit of information.
A bitmap selector would be a lot more efficient. Not unusual to have a
few hundred thousand entries.

cheers,
jamal

> --
> Florian
>

RE: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Arad, Ronen]

> >>>
> >>> Is there any case where this fdb entry gets re-used and is no 
> >>> longer added by an external learning? Should we clear this flag somewhere?
> >>
> >> Once the FDB entry is marked "added_by_external_learn" it stays 
> >> marked as such until removed by aging cleanup process (or flushed 
> >> due to interface down, etc).  If aged out (and now deleted), the 
> >> FDB entry may come back either by SW learn or by HW learn.  If SW 
> >> learn comes first, and then HW learn, HW learn will override and 
> >> mark the existing FDB entry "added_by_external_learn".  So there is 
> >> take-over by HW but no give-back to SW.  And there is no explicit 
> >> clearing of the mark short of deleting the FDB entry.  The mark is 
> >> mostly for letting user's know which FDB entries where learned by 
> >> HW and synced to the bridge's FDB.
> >
> > Thanks, makes sense now. This is probably obvious in this context, 
> > but maybe it would not hurt to come up with a documentation that 
> > describe the offload API, FDB entry lifetime and HW/SW ownership etc...
> 
> I have an updated Documentation/networking/switchdev.txt that covers 
> the swdev APIs and usage and notes, but Jiri is being stingy with it.
> Will get this out, either in v4 or follow-on patches.  There is enough 
> going on just with L2 offload that we're going to need some good 
> documentation to guide implementers.
> --

To control the lifetime of an externally learned FDB entry, the bridge shall provide an API for the switch driver to update the freshness of externally learned entries. Otherwise, the bridge aging will age entries which are currently or frequently used by the HW.
Is this covered in the updated document?
Is this functionality planned for v4?
 

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Jiri Pirko]

Wed, Nov 26, 2014 at 03:40:59AM CET, sfeldma@gmail.com wrote:
>On Tue, Nov 25, 2014 at 4:34 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 25/11/14 18:03, Scott Feldman wrote:
>>> On Tue, Nov 25, 2014 at 12:44 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> On 25/11/14 02:28, Jiri Pirko wrote:
>>>>> From: Scott Feldman <sfeldma@gmail.com>
>>>>>
>>>>> When the swdev device learns a new mac/vlan on a port, it sends some async
>>>>> notification to the driver and the driver installs an FDB in the device.
>>>>> To give a holistic system view, the learned mac/vlan should be reflected
>>>>> in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
>>>>> what is currently learned by the device.  This API on the bridge driver gives
>>>>> a way for the swdev driver to install an FBD entry in the bridge FBD table.
>>>>> (And remove one).
>>>>>
>>>>> This is equivalent to the device running these cmds:
>>>>>
>>>>>   bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
>>>>>
>>>>> This patch needs some extra eyeballs for review, in paricular around the
>>>>> locking and contexts.
>>>>>
>>>>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>>>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>>>> ---
>>>>
>>>> [snip]
>>>>
>>>>> +     head = &br->hash[br_mac_hash(addr, vid)];
>>>>> +     fdb = fdb_find(head, addr, vid);
>>>>> +     if (!fdb) {
>>>>> +             fdb = fdb_create(head, p, addr, vid);
>>>>> +             if (!fdb) {
>>>>> +                     err = -ENOMEM;
>>>>> +                     goto err_unlock;
>>>>> +             }
>>>>> +             fdb->added_by_external_learn = 1;
>>>>> +             fdb_notify(br, fdb, RTM_NEWNEIGH);
>>>>> +     } else if (fdb->added_by_external_learn) {
>>>>> +             /* Refresh entry */
>>>>> +             fdb->updated = fdb->used = jiffies;
>>>>> +     } else if (!fdb->added_by_user) {
>>>>> +             /* Take over SW learned entry */
>>>>> +             fdb->added_by_external_learn = 1;
>>>>> +             fdb->updated = jiffies;
>>>>> +             fdb_notify(br, fdb, RTM_NEWNEIGH);
>>>>> +     }
>>>>
>>>> Is there any case where this fdb entry gets re-used and is no longer
>>>> added by an external learning? Should we clear this flag somewhere?
>>>
>>> Once the FDB entry is marked "added_by_external_learn" it stays marked
>>> as such until removed by aging cleanup process (or flushed due to
>>> interface down, etc).  If aged out (and now deleted), the FDB entry
>>> may come back either by SW learn or by HW learn.  If SW learn comes
>>> first, and then HW learn, HW learn will override and mark the existing
>>> FDB entry "added_by_external_learn".  So there is take-over by HW but
>>> no give-back to SW.  And there is no explicit clearing of the mark
>>> short of deleting the FDB entry.  The mark is mostly for letting
>>> user's know which FDB entries where learned by HW and synced to the
>>> bridge's FDB.
>>
>> Thanks, makes sense now. This is probably obvious in this context, but
>> maybe it would not hurt to come up with a documentation that describe
>> the offload API, FDB entry lifetime and HW/SW ownership etc...
>
>I have an updated Documentation/networking/switchdev.txt that covers
>the swdev APIs and usage and notes, but Jiri is being stingy with it.

The doc update you mention includes also fib offload which we are not
pushing now. I have that patches in queue.

>Will get this out, either in v4 or follow-on patches.  There is enough
>going on just with L2 offload that we're going to need some good
>documentation to guide implementers.

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Jiri Pirko]

Wed, Nov 26, 2014 at 02:48:04AM CET, sfeldma@gmail.com wrote:
>On Tue, Nov 25, 2014 at 12:36 PM, Thomas Graf <tgraf@suug.ch> wrote:
>> On 11/25/14 at 11:38am, Andy Gospodarek wrote:
>>> On Tue, Nov 25, 2014 at 11:28:40AM +0100, Jiri Pirko wrote:
>>> > From: Scott Feldman <sfeldma@gmail.com>
>>> >
>>> > When the swdev device learns a new mac/vlan on a port, it sends some async
>>> > notification to the driver and the driver installs an FDB in the device.
>>> > To give a holistic system view, the learned mac/vlan should be reflected
>>> > in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
>>> > what is currently learned by the device.  This API on the bridge driver gives
>>> > a way for the swdev driver to install an FBD entry in the bridge FBD table.
>>> > (And remove one).
>>> >
>>> > This is equivalent to the device running these cmds:
>>> >
>>> >   bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
>>> >
>>> > This patch needs some extra eyeballs for review, in paricular around the
>>> > locking and contexts.
>>> >
>>> > Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>>> > Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>
>> I like the simplicity of this. That said, given we'll have multiple
>> users of swdev including OVS, shouldn't this be a notifier or a
>> callback that depends on who is controlling the device?
>
>I like the idea.  When the switch port joins Linux bridge or OVS
>datapath, a callback is registered with the driver.  That way the
>driver doesn't really care if the port is a bridge member or an OVS
>vport in a datapath.  It's just passing up the FDB entry
>(port/mac/vlan) details to the container device.  Can we hold this
>idea until this patchset sticks?  I think once OVS support comes back
>into the swdev model would be the time to address this.

Yep, I agree this is a good idea and I also vote for implemeting this as
a follow-up. Thanks.

>
>>
>>> > +   spin_lock(&br->hash_lock);
>>> (Since you asked to check locking...)
>>>
>>> Most of the other fdb_add/delete/insert/update calls take this with
>>> spin_lock_bh.  Did you try this with lockdep enabled just to see if that
>>> is needed here?  I suspect that anytime br->hash_lock is taken it will
>>> need to be with softirqs disabled from this point forward.
>>
>> At least br_fdb_update() seems to be called from BH context so I would
>> agree and argue the lock in br_fdb_cleanup() and br_fdb_update() need a
>> fix too. I'll send a patch.

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Scott Feldman]

On Tue, Nov 25, 2014 at 9:37 PM, Arad, Ronen <ronen.arad@intel.com> wrote:
>> >>>
>> >>> Is there any case where this fdb entry gets re-used and is no
>> >>> longer added by an external learning? Should we clear this flag somewhere?
>> >>
>> >> Once the FDB entry is marked "added_by_external_learn" it stays
>> >> marked as such until removed by aging cleanup process (or flushed
>> >> due to interface down, etc).  If aged out (and now deleted), the
>> >> FDB entry may come back either by SW learn or by HW learn.  If SW
>> >> learn comes first, and then HW learn, HW learn will override and
>> >> mark the existing FDB entry "added_by_external_learn".  So there is
>> >> take-over by HW but no give-back to SW.  And there is no explicit
>> >> clearing of the mark short of deleting the FDB entry.  The mark is
>> >> mostly for letting user's know which FDB entries where learned by
>> >> HW and synced to the bridge's FDB.
>> >
>> > Thanks, makes sense now. This is probably obvious in this context,
>> > but maybe it would not hurt to come up with a documentation that
>> > describe the offload API, FDB entry lifetime and HW/SW ownership etc...
>>
>> I have an updated Documentation/networking/switchdev.txt that covers
>> the swdev APIs and usage and notes, but Jiri is being stingy with it.
>> Will get this out, either in v4 or follow-on patches.  There is enough
>> going on just with L2 offload that we're going to need some good
>> documentation to guide implementers.
>> --
>
> To control the lifetime of an externally learned FDB entry, the bridge shall provide an API for the switch driver to update the freshness of externally learned entries. Otherwise, the bridge aging will age entries which are currently or frequently used by the HW.
> Is this covered in the updated document?
> Is this functionality planned for v4?

Hi Ronen,

It's already there: driver calls br_fdb_external_learn_add() to
refresh FBD entry, which updates the fdb->updated and fdb->used
timestamps, preventing bridge from prematurely aging out the entry.
We'll make sure that detail gets in the doc.  It's up to the driver on
how frequently it calls br_fdb_external_learn_add().  Maybe it just
blindly makes the call every 1s.  That's what rocker driver does (as
long as the FDB entry continues to get hits).  From the user's
perspective, 1s update is nice when looking at the stats dump for
fdbs, since the timestamps are in secs.

-scott

RE: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[Arad, Ronen]

> -----Original Message-----
> From: Scott Feldman [mailto:sfeldma@gmail.com]
> Sent: Wednesday, November 26, 2014 11:04 PM
> To: Arad, Ronen
> Cc: netdev@vger.kernel.org
> Subject: Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver
> of learned FBD on offloaded device
> 
> On Tue, Nov 25, 2014 at 9:37 PM, Arad, Ronen <ronen.arad@intel.com>
> wrote:
> >> >>>
> >> >>> Is there any case where this fdb entry gets re-used and is no
> >> >>> longer added by an external learning? Should we clear this flag
> somewhere?
> >> >>
> >> >> Once the FDB entry is marked "added_by_external_learn" it stays
> >> >> marked as such until removed by aging cleanup process (or flushed
> >> >> due to interface down, etc).  If aged out (and now deleted), the
> >> >> FDB entry may come back either by SW learn or by HW learn.  If SW
> >> >> learn comes first, and then HW learn, HW learn will override and
> >> >> mark the existing FDB entry "added_by_external_learn".  So there
> >> >> is take-over by HW but no give-back to SW.  And there is no
> >> >> explicit clearing of the mark short of deleting the FDB entry.
> >> >> The mark is mostly for letting user's know which FDB entries where
> >> >> learned by HW and synced to the bridge's FDB.
> >> >
> >> > Thanks, makes sense now. This is probably obvious in this context,
> >> > but maybe it would not hurt to come up with a documentation that
> >> > describe the offload API, FDB entry lifetime and HW/SW ownership etc...
> >>
> >> I have an updated Documentation/networking/switchdev.txt that covers
> >> the swdev APIs and usage and notes, but Jiri is being stingy with it.
> >> Will get this out, either in v4 or follow-on patches.  There is
> >> enough going on just with L2 offload that we're going to need some
> >> good documentation to guide implementers.
> >> --
> >
> > To control the lifetime of an externally learned FDB entry, the bridge shall
> provide an API for the switch driver to update the freshness of externally
> learned entries. Otherwise, the bridge aging will age entries which are
> currently or frequently used by the HW.
> > Is this covered in the updated document?
> > Is this functionality planned for v4?
> 
> Hi Ronen,
> 
> It's already there: driver calls br_fdb_external_learn_add() to refresh FBD
> entry, which updates the fdb->updated and fdb->used timestamps,
> preventing bridge from prematurely aging out the entry.
> We'll make sure that detail gets in the doc.  It's up to the driver on how
> frequently it calls br_fdb_external_learn_add().  Maybe it just blindly makes
> the call every 1s.  That's what rocker driver does (as long as the FDB entry
> continues to get hits).  From the user's perspective, 1s update is nice when
> looking at the stats dump for fdbs, since the timestamps are in secs.
> 
> -scott

Hi Scott,

Thanks. This should work. I overlooked that but now I see it in the code with a clear comment.

-ronen

Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device

[John Fastabend]

On 11/26/2014 11:03 PM, Scott Feldman wrote:
> On Tue, Nov 25, 2014 at 9:37 PM, Arad, Ronen <ronen.arad@intel.com> wrote:
>>>>>>
>>>>>> Is there any case where this fdb entry gets re-used and is no
>>>>>> longer added by an external learning? Should we clear this flag somewhere?
>>>>>
>>>>> Once the FDB entry is marked "added_by_external_learn" it stays
>>>>> marked as such until removed by aging cleanup process (or flushed
>>>>> due to interface down, etc).  If aged out (and now deleted), the
>>>>> FDB entry may come back either by SW learn or by HW learn.  If SW
>>>>> learn comes first, and then HW learn, HW learn will override and
>>>>> mark the existing FDB entry "added_by_external_learn".  So there is
>>>>> take-over by HW but no give-back to SW.  And there is no explicit
>>>>> clearing of the mark short of deleting the FDB entry.  The mark is
>>>>> mostly for letting user's know which FDB entries where learned by
>>>>> HW and synced to the bridge's FDB.
>>>>
>>>> Thanks, makes sense now. This is probably obvious in this context,
>>>> but maybe it would not hurt to come up with a documentation that
>>>> describe the offload API, FDB entry lifetime and HW/SW ownership etc...
>>>
>>> I have an updated Documentation/networking/switchdev.txt that covers
>>> the swdev APIs and usage and notes, but Jiri is being stingy with it.
>>> Will get this out, either in v4 or follow-on patches.  There is enough
>>> going on just with L2 offload that we're going to need some good
>>> documentation to guide implementers.
>>> --
>>
>> To control the lifetime of an externally learned FDB entry, the bridge shall provide an API for the switch driver to update the freshness of externally learned entries. Otherwise, the bridge aging will age entries which are currently or frequently used by the HW.
>> Is this covered in the updated document?
>> Is this functionality planned for v4?
>
> Hi Ronen,
>
> It's already there: driver calls br_fdb_external_learn_add() to
> refresh FBD entry, which updates the fdb->updated and fdb->used
> timestamps, preventing bridge from prematurely aging out the entry.
> We'll make sure that detail gets in the doc.  It's up to the driver on
> how frequently it calls br_fdb_external_learn_add().  Maybe it just
> blindly makes the call every 1s.  That's what rocker driver does (as
> long as the FDB entry continues to get hits).  From the user's
> perspective, 1s update is nice when looking at the stats dump for
> fdbs, since the timestamps are in secs.
>
> -scott

We could at some point add a driver knob to set the timing of the
updates as well, but I don't see the need for it yet. Rocker switch
doesn't need it but if vendors devices want to do this it wouldn't
be difficult.

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
John Fastabend         Intel Corporation