[PATCH v3 net-next 0/5] IGMP snooping for local traffic

[PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Andrew Lunn]

The linux bridge supports IGMP snooping. It will listen to IGMP
reports on bridge ports and keep track of which groups have been
joined on an interface. It will then forward multicast based on this
group membership.

When the bridge adds or removed groups from an interface, it uses
switchdev to request the hardware add an mdb to a port, so the
hardware can perform the selective forwarding between ports.

What is not covered by the current bridge code, is IGMP joins/leaves
from the host on the brX interface. These are not reported via
switchdev so that hardware knows the local host is interested in the
multicast frames.

Luckily, the bridge does track joins/leaves on the brX interface. The
code is obfusticated, which is why i missed it with my first attempt.
So the first patch tries to remove this obfustication. Currently,
there is no notifications sent when the bridge interface joins a
group. The second patch adds them. bridge monitor then shows
joins/leaves in the same way as for other ports of the bridge.

Then starts the work passing down to the hardware that the host has
joined/left a group. The existing switchdev mdb object cannot be used,
since the semantics are different. The existing
SWITCHDEV_OBJ_ID_PORT_MDB is used to indicate a specific multicast
group should be forwarded out that port of the switch. However here we
require the exact opposite. We want multicast frames for the group
received on the port to the forwarded to the host. Hence add a new
object SWITCHDEV_OBJ_ID_HOST_MDB, a multicast database entry to
forward to the host. This new object is then propagated through the
DSA layers. No DSA driver changes should be needed, this should just
work...

Andrew Lunn (5):
  net: bridge: Rename mglist to host_joined
  net: bridge: Send notification when host join/leaves a group
  net: bridge: Add/del switchdev object on host join/leave
  net: dsa: slave: Handle switchdev host mdb add/del
  net: dsa: switch: Don't add CPU port to an mdb by default

 include/net/switchdev.h   |  1 +
 net/bridge/br_input.c     |  2 +-
 net/bridge/br_mdb.c       | 50 +++++++++++++++++++++++++++++++++++++++++++----
 net/bridge/br_multicast.c | 18 ++++++++++-------
 net/bridge/br_private.h   |  2 +-
 net/dsa/slave.c           | 13 ++++++++++++
 net/dsa/switch.c          |  3 ++-
 net/switchdev/switchdev.c |  2 ++
 8 files changed, 77 insertions(+), 14 deletions(-)

-- 
2.15.0

[PATCH v3 net-next 1/5] net: bridge: Rename mglist to host_joined

[Andrew Lunn]

The boolean mglist indicates the host has joined a particular
multicast group on the bridge interface. It is badly named, obscuring
what is means. Rename it.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/bridge/br_input.c     |  2 +-
 net/bridge/br_mdb.c       |  2 +-
 net/bridge/br_multicast.c | 14 +++++++-------
 net/bridge/br_private.h   |  2 +-
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index a096d3e189da..7f98a7d25866 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -137,7 +137,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 		mdst = br_mdb_get(br, skb, vid);
 		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
 		    br_multicast_querier_exists(br, eth_hdr(skb))) {
-			if ((mdst && mdst->mglist) ||
+			if ((mdst && mdst->host_joined) ||
 			    br_multicast_is_router(br)) {
 				local_rcv = true;
 				br->dev->stats.multicast++;
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 31ddff22563e..aa716a33cb71 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -655,7 +655,7 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
 		call_rcu_bh(&p->rcu, br_multicast_free_pg);
 		err = 0;
 
-		if (!mp->ports && !mp->mglist &&
+		if (!mp->ports && !mp->host_joined &&
 		    netif_running(br->dev))
 			mod_timer(&mp->timer, jiffies);
 		break;
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 5f7f0e9d446c..bfe5adb1f51c 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -249,7 +249,7 @@ static void br_multicast_group_expired(struct timer_list *t)
 	if (!netif_running(br->dev) || timer_pending(&mp->timer))
 		goto out;
 
-	mp->mglist = false;
+	mp->host_joined = false;
 
 	if (mp->ports)
 		goto out;
@@ -292,7 +292,7 @@ static void br_multicast_del_pg(struct net_bridge *br,
 			      p->flags);
 		call_rcu_bh(&p->rcu, br_multicast_free_pg);
 
-		if (!mp->ports && !mp->mglist &&
+		if (!mp->ports && !mp->host_joined &&
 		    netif_running(br->dev))
 			mod_timer(&mp->timer, jiffies);
 
@@ -773,7 +773,7 @@ static int br_multicast_add_group(struct net_bridge *br,
 		goto err;
 
 	if (!port) {
-		mp->mglist = true;
+		mp->host_joined = true;
 		mod_timer(&mp->timer, now + br->multicast_membership_interval);
 		goto out;
 	}
@@ -1477,7 +1477,7 @@ static int br_ip4_multicast_query(struct net_bridge *br,
 
 	max_delay *= br->multicast_last_member_count;
 
-	if (mp->mglist &&
+	if (mp->host_joined &&
 	    (timer_pending(&mp->timer) ?
 	     time_after(mp->timer.expires, now + max_delay) :
 	     try_to_del_timer_sync(&mp->timer) >= 0))
@@ -1561,7 +1561,7 @@ static int br_ip6_multicast_query(struct net_bridge *br,
 		goto out;
 
 	max_delay *= br->multicast_last_member_count;
-	if (mp->mglist &&
+	if (mp->host_joined &&
 	    (timer_pending(&mp->timer) ?
 	     time_after(mp->timer.expires, now + max_delay) :
 	     try_to_del_timer_sync(&mp->timer) >= 0))
@@ -1622,7 +1622,7 @@ br_multicast_leave_group(struct net_bridge *br,
 			br_mdb_notify(br->dev, port, group, RTM_DELMDB,
 				      p->flags);
 
-			if (!mp->ports && !mp->mglist &&
+			if (!mp->ports && !mp->host_joined &&
 			    netif_running(br->dev))
 				mod_timer(&mp->timer, jiffies);
 		}
@@ -1662,7 +1662,7 @@ br_multicast_leave_group(struct net_bridge *br,
 		     br->multicast_last_member_interval;
 
 	if (!port) {
-		if (mp->mglist &&
+		if (mp->host_joined &&
 		    (timer_pending(&mp->timer) ?
 		     time_after(mp->timer.expires, time) :
 		     try_to_del_timer_sync(&mp->timer) >= 0)) {
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 40553d832b6e..1312b8d20ec3 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -209,7 +209,7 @@ struct net_bridge_mdb_entry
 	struct rcu_head			rcu;
 	struct timer_list		timer;
 	struct br_ip			addr;
-	bool				mglist;
+	bool				host_joined;
 };
 
 struct net_bridge_mdb_htable
-- 
2.15.0

[PATCH v3 net-next 2/5] net: bridge: Send notification when host join/leaves a group

[Andrew Lunn]

The host can join or leave a multicast group on the brX interface, as
indicated by IGMP snooping.  This is tracked within the bridge
multicast code. Send a notification when this happens, in the same way
a notification is sent when a port of the bridge joins/leaves a group
because of IGMP snooping.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/bridge/br_mdb.c       | 9 ++++++---
 net/bridge/br_multicast.c | 6 +++++-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index aa716a33cb71..702408d2a93c 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -317,7 +317,7 @@ static void __br_mdb_notify(struct net_device *dev, struct net_bridge_port *p,
 #endif
 
 	mdb.obj.orig_dev = port_dev;
-	if (port_dev && type == RTM_NEWMDB) {
+	if (p && port_dev && type == RTM_NEWMDB) {
 		complete_info = kmalloc(sizeof(*complete_info), GFP_ATOMIC);
 		if (complete_info) {
 			complete_info->port = p;
@@ -327,7 +327,7 @@ static void __br_mdb_notify(struct net_device *dev, struct net_bridge_port *p,
 			if (switchdev_port_obj_add(port_dev, &mdb.obj))
 				kfree(complete_info);
 		}
-	} else if (port_dev && type == RTM_DELMDB) {
+	} else if (p && port_dev && type == RTM_DELMDB) {
 		switchdev_port_obj_del(port_dev, &mdb.obj);
 	}
 
@@ -353,7 +353,10 @@ void br_mdb_notify(struct net_device *dev, struct net_bridge_port *port,
 	struct br_mdb_entry entry;
 
 	memset(&entry, 0, sizeof(entry));
-	entry.ifindex = port->dev->ifindex;
+	if (port)
+		entry.ifindex = port->dev->ifindex;
+	else
+		entry.ifindex = dev->ifindex;
 	entry.addr.proto = group->proto;
 	entry.addr.u.ip4 = group->u.ip4;
 #if IS_ENABLED(CONFIG_IPV6)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index bfe5adb1f51c..cb4729539b82 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -250,6 +250,7 @@ static void br_multicast_group_expired(struct timer_list *t)
 		goto out;
 
 	mp->host_joined = false;
+	br_mdb_notify(br->dev, NULL, &mp->addr, RTM_DELMDB, 0);
 
 	if (mp->ports)
 		goto out;
@@ -773,7 +774,10 @@ static int br_multicast_add_group(struct net_bridge *br,
 		goto err;
 
 	if (!port) {
-		mp->host_joined = true;
+		if (!mp->host_joined) {
+			mp->host_joined = true;
+			br_mdb_notify(br->dev, NULL, &mp->addr, RTM_NEWMDB, 0);
+		}
 		mod_timer(&mp->timer, now + br->multicast_membership_interval);
 		goto out;
 	}
-- 
2.15.0

[PATCH v3 net-next 3/5] net: bridge: Add/del switchdev object on host join/leave

[Andrew Lunn]

When the host joins or leaves a multicast group, use switchdev to add
an object to the hardware to forward traffic for the group to the
host.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/net/switchdev.h   |  1 +
 net/bridge/br_mdb.c       | 39 +++++++++++++++++++++++++++++++++++++++
 net/switchdev/switchdev.c |  2 ++
 3 files changed, 42 insertions(+)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index d756fbe46625..39bc855d7fee 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -76,6 +76,7 @@ enum switchdev_obj_id {
 	SWITCHDEV_OBJ_ID_UNDEFINED,
 	SWITCHDEV_OBJ_ID_PORT_VLAN,
 	SWITCHDEV_OBJ_ID_PORT_MDB,
+	SWITCHDEV_OBJ_ID_HOST_MDB,
 };
 
 struct switchdev_obj {
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 702408d2a93c..2a3daa11f60b 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -292,6 +292,42 @@ static void br_mdb_complete(struct net_device *dev, int err, void *priv)
 	kfree(priv);
 }
 
+static void br_mdb_switchdev_host_port(struct net_device *dev,
+				       struct net_device *lower_dev,
+				       struct br_mdb_entry *entry, int type)
+{
+	struct switchdev_obj_port_mdb mdb = {
+		.obj = {
+			.id = SWITCHDEV_OBJ_ID_HOST_MDB,
+			.flags = SWITCHDEV_F_DEFER,
+		},
+		.vid = entry->vid,
+	};
+
+	if (entry->addr.proto == htons(ETH_P_IP))
+		ip_eth_mc_map(entry->addr.u.ip4, mdb.addr);
+#if IS_ENABLED(CONFIG_IPV6)
+	else
+		ipv6_eth_mc_map(&entry->addr.u.ip6, mdb.addr);
+#endif
+
+	mdb.obj.orig_dev = dev;
+	if (type == RTM_NEWMDB)
+		switchdev_port_obj_add(lower_dev, &mdb.obj);
+	if (type == RTM_DELMDB)
+		switchdev_port_obj_del(lower_dev, &mdb.obj);
+}
+
+static void br_mdb_switchdev_host(struct net_device *dev,
+				  struct br_mdb_entry *entry, int type)
+{
+	struct net_device *lower_dev;
+	struct list_head *iter;
+
+	netdev_for_each_lower_dev(dev, lower_dev, iter)
+		br_mdb_switchdev_host_port(dev, lower_dev, entry, type);
+}
+
 static void __br_mdb_notify(struct net_device *dev, struct net_bridge_port *p,
 			    struct br_mdb_entry *entry, int type)
 {
@@ -331,6 +367,9 @@ static void __br_mdb_notify(struct net_device *dev, struct net_bridge_port *p,
 		switchdev_port_obj_del(port_dev, &mdb.obj);
 	}
 
+	if (!p)
+		br_mdb_switchdev_host(dev, entry, type);
+
 	skb = nlmsg_new(rtnl_mdb_nlmsg_size(), GFP_ATOMIC);
 	if (!skb)
 		goto errout;
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 0531b41d1f2d..74b9d916a58b 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -345,6 +345,8 @@ static size_t switchdev_obj_size(const struct switchdev_obj *obj)
 		return sizeof(struct switchdev_obj_port_vlan);
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		return sizeof(struct switchdev_obj_port_mdb);
+	case SWITCHDEV_OBJ_ID_HOST_MDB:
+		return sizeof(struct switchdev_obj_port_mdb);
 	default:
 		BUG();
 	}
-- 
2.15.0

[PATCH v3 net-next 4/5] net: dsa: slave: Handle switchdev host mdb add/del

[Andrew Lunn]

Add code to handle switchdev host mdb add/del. Since DSA uses one of
the switch ports as a transport to the host, we just need to add an
MDB on this port.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/slave.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 1179e4cd6701..4b80d2ac1d69 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -304,6 +304,13 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		err = dsa_port_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj), trans);
 		break;
+	case SWITCHDEV_OBJ_ID_HOST_MDB:
+		/* DSA can directly translate this to a normal MDB add,
+		 * but on the CPU port.
+		 */
+		err = dsa_port_mdb_add(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj),
+				       trans);
+		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
 		err = dsa_port_vlan_add(dp, SWITCHDEV_OBJ_PORT_VLAN(obj),
 					trans);
@@ -326,6 +333,12 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		err = dsa_port_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
+	case SWITCHDEV_OBJ_ID_HOST_MDB:
+		/* DSA can directly translate this to a normal MDB add,
+		 * but on the CPU port.
+		 */
+		err = dsa_port_mdb_del(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj));
+		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
 		err = dsa_port_vlan_del(dp, SWITCHDEV_OBJ_PORT_VLAN(obj));
 		break;
-- 
2.15.0

[PATCH v3 net-next 5/5] net: dsa: switch: Don't add CPU port to an mdb by default

[Andrew Lunn]

Now that the host indicates when a multicast group should be forwarded
from the switch to the host, don't do it by default.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/switch.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index e6c06aa349a6..e95722036a45 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -121,7 +121,7 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
 	if (ds->index == info->sw_index)
 		set_bit(info->port, group);
 	for (port = 0; port < ds->num_ports; port++)
-		if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
+		if (dsa_is_dsa_port(ds, port))
 			set_bit(port, group);
 
 	if (switchdev_trans_ph_prepare(trans)) {
@@ -141,6 +141,7 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
 	return 0;
 }
 
+
 static int dsa_switch_mdb_del(struct dsa_switch *ds,
 			      struct dsa_notifier_mdb_info *info)
 {
-- 
2.15.0

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Stephen Hemminger]

On Tue,  7 Nov 2017 00:26:53 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> The linux bridge supports IGMP snooping. It will listen to IGMP
> reports on bridge ports and keep track of which groups have been
> joined on an interface. It will then forward multicast based on this
> group membership.
> 
> When the bridge adds or removed groups from an interface, it uses
> switchdev to request the hardware add an mdb to a port, so the
> hardware can perform the selective forwarding between ports.
> 
> What is not covered by the current bridge code, is IGMP joins/leaves
> from the host on the brX interface. These are not reported via
> switchdev so that hardware knows the local host is interested in the
> multicast frames.
> 
> Luckily, the bridge does track joins/leaves on the brX interface. The
> code is obfusticated, which is why i missed it with my first attempt.
> So the first patch tries to remove this obfustication. Currently,
> there is no notifications sent when the bridge interface joins a
> group. The second patch adds them. bridge monitor then shows
> joins/leaves in the same way as for other ports of the bridge.
> 
> Then starts the work passing down to the hardware that the host has
> joined/left a group. The existing switchdev mdb object cannot be used,
> since the semantics are different. The existing
> SWITCHDEV_OBJ_ID_PORT_MDB is used to indicate a specific multicast
> group should be forwarded out that port of the switch. However here we
> require the exact opposite. We want multicast frames for the group
> received on the port to the forwarded to the host. Hence add a new
> object SWITCHDEV_OBJ_ID_HOST_MDB, a multicast database entry to
> forward to the host. This new object is then propagated through the
> DSA layers. No DSA driver changes should be needed, this should just
> work...
> 
> Andrew Lunn (5):
>   net: bridge: Rename mglist to host_joined
>   net: bridge: Send notification when host join/leaves a group
>   net: bridge: Add/del switchdev object on host join/leave
>   net: dsa: slave: Handle switchdev host mdb add/del
>   net: dsa: switch: Don't add CPU port to an mdb by default
> 
>  include/net/switchdev.h   |  1 +
>  net/bridge/br_input.c     |  2 +-
>  net/bridge/br_mdb.c       | 50 +++++++++++++++++++++++++++++++++++++++++++----
>  net/bridge/br_multicast.c | 18 ++++++++++-------
>  net/bridge/br_private.h   |  2 +-
>  net/dsa/slave.c           | 13 ++++++++++++
>  net/dsa/switch.c          |  3 ++-
>  net/switchdev/switchdev.c |  2 ++
>  8 files changed, 77 insertions(+), 14 deletions(-)

The bridge part looks good.
Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH v3 net-next 5/5] net: dsa: switch: Don't add CPU port to an mdb by default

[Sergei Shtylyov]

Hello!

On 11/7/2017 2:26 AM, Andrew Lunn wrote:

> Now that the host indicates when a multicast group should be forwarded
> from the switch to the host, don't do it by default.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>   net/dsa/switch.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index e6c06aa349a6..e95722036a45 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -121,7 +121,7 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
>   	if (ds->index == info->sw_index)
>   		set_bit(info->port, group);
>   	for (port = 0; port < ds->num_ports; port++)
> -		if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> +		if (dsa_is_dsa_port(ds, port))
>   			set_bit(port, group);
>   
>   	if (switchdev_trans_ph_prepare(trans)) {
> @@ -141,6 +141,7 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
>   	return 0;
>   }
>   
> +
>   static int dsa_switch_mdb_del(struct dsa_switch *ds,
>   			      struct dsa_notifier_mdb_info *info)
>   {

    Unrelated whitespace change?

MBR, Sergei

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> Then starts the work passing down to the hardware that the host has
> joined/left a group. The existing switchdev mdb object cannot be used,
> since the semantics are different. The existing
> SWITCHDEV_OBJ_ID_PORT_MDB is used to indicate a specific multicast
> group should be forwarded out that port of the switch. However here we
> require the exact opposite. We want multicast frames for the group
> received on the port to the forwarded to the host. Hence add a new
> object SWITCHDEV_OBJ_ID_HOST_MDB, a multicast database entry to
> forward to the host. This new object is then propagated through the
> DSA layers. No DSA driver changes should be needed, this should just
> work...

The patchset looks good. I would like to ask you some details though,
because I don't understand why the semantics are different.

Technically, what happens is that an MDB entry is programmed on the
bridge interface. From the _bridge point of view_, there is no technical
difference as in programming an MDB entry on a bridged port. Correct?

So we can simply use an mdb switchdev_obj_port_mdb structure with:

    mdb.orig_dev = mdb.dev = br->dev;

The structure will still be propagated to all bridge members as usual
and switchdev users can check the above fields.

>From the _DSA point of view_, mdb->orig_dev == dp->bridge_dev would mean
that we must program dp->cpu_dp, the bridge port's dedicated CPU port.

To me it looks like the only confusion here is in the name of the
switchdev object, where SWITCHDEV_OBJ_ID_PORT_MDB and
switchdev_obj_port_mdb must be in fact SWITCHDEV_OBJ_ID_MDB and
switchdev_obj_mdb. Or I am missing something?

I just want to make sure I understand the whole picture correctly here.
About the patchset itself, introducing SWITCHDEV_OBJ_ID_HOST_MDB seems
to be only meant to avoid a noisy global renaming, in which case I'm
fine with that.

My only concern is that it looks like we'll have the same issue with
programming e.g. VLAN or static FDB entries on the bridge interface, in
which case the correct way to go would be to use and check orig_dev.


Thanks,

        Vivien

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Egil Hjelmeland]

On 07. nov. 2017 00:26, Andrew Lunn wrote:
> The linux bridge supports IGMP snooping. It will listen to IGMP
> reports on bridge ports and keep track of which groups have been
> joined on an interface. It will then forward multicast based on this
> group membership.
> 
> When the bridge adds or removed groups from an interface, it uses
> switchdev to request the hardware add an mdb to a port, so the
> hardware can perform the selective forwarding between ports.
> 
> What is not covered by the current bridge code, is IGMP joins/leaves
> from the host on the brX interface. These are not reported via
> switchdev so that hardware knows the local host is interested in the
> multicast frames.
> 
> Luckily, the bridge does track joins/leaves on the brX interface. The
> code is obfusticated, which is why i missed it with my first attempt.
> So the first patch tries to remove this obfustication. Currently,
> there is no notifications sent when the bridge interface joins a
> group. The second patch adds them. bridge monitor then shows
> joins/leaves in the same way as for other ports of the bridge.
> 
> Then starts the work passing down to the hardware that the host has
> joined/left a group. The existing switchdev mdb object cannot be used,
> since the semantics are different. The existing
> SWITCHDEV_OBJ_ID_PORT_MDB is used to indicate a specific multicast
> group should be forwarded out that port of the switch. However here we
> require the exact opposite. We want multicast frames for the group
> received on the port to the forwarded to the host. Hence add a new
> object SWITCHDEV_OBJ_ID_HOST_MDB, a multicast database entry to
> forward to the host. This new object is then propagated through the
> DSA layers. No DSA driver changes should be needed, this should just
> work...
> 
> Andrew Lunn (5):
>    net: bridge: Rename mglist to host_joined
>    net: bridge: Send notification when host join/leaves a group
>    net: bridge: Add/del switchdev object on host join/leave
>    net: dsa: slave: Handle switchdev host mdb add/del
>    net: dsa: switch: Don't add CPU port to an mdb by default
> 
>   include/net/switchdev.h   |  1 +
>   net/bridge/br_input.c     |  2 +-
>   net/bridge/br_mdb.c       | 50 +++++++++++++++++++++++++++++++++++++++++++----
>   net/bridge/br_multicast.c | 18 ++++++++++-------
>   net/bridge/br_private.h   |  2 +-
>   net/dsa/slave.c           | 13 ++++++++++++
>   net/dsa/switch.c          |  3 ++-
>   net/switchdev/switchdev.c |  2 ++
>   8 files changed, 77 insertions(+), 14 deletions(-)
> 
Hi Andrew!

I tested this series today on my board with lan9303, and it does seem to 
work. But no extensive testing though.

I have not looked at the contents of the patches.

Some observations, not meant to delay the series:

When local application join multicast; the driver get 2 X 
.port_mdb_prepare + 4 x .port_mdb_add for the address.

"bridge mdb" does not list local multicast memberships, is that a "feature"?

While I had an application joined on a multicast address; I restarted 
the networking: meaning deleting the bridge and setting it up again. No 
.port_mdb_del on the multicast address. Stopped the application. Still 
no .port_mdb_del on the multicast address. So the multicast address 
stays in the HW fdb, until I started and stopped the application again. 
Not really a problem, just an observation.


Then finally, there is a more serious issue related to IGMPv2. As I 
suspected, with "IGMP snooping for local traffic", I have to activate 
"IGMP trapping" in the lan9303 HW in order to get it to work with a 
IGMPv2 querier. (Remember, unlike IGMPv3, IGMPv2 reports are "inband" in 
the group). The HW allows me to select which ports receive IGMP packets. 
The best is to only send to CPU, so the SW bridge can do the right thing 
in all situations. However, then we must make sure skb->offload_fwd_mark 
is zero for IGMP packets. Which was easy to do in my proof-of-concept 
test, but I suspect a proper test on ip->protocol is more complicated?

Acked-by: Egil Hjelmeland <privat@egil-hjelmeland.no>


Cheers
Egil

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Andrew Lunn]

On Tue, Nov 07, 2017 at 12:03:54PM -0500, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > Then starts the work passing down to the hardware that the host has
> > joined/left a group. The existing switchdev mdb object cannot be used,
> > since the semantics are different. The existing
> > SWITCHDEV_OBJ_ID_PORT_MDB is used to indicate a specific multicast
> > group should be forwarded out that port of the switch. However here we
> > require the exact opposite. We want multicast frames for the group
> > received on the port to the forwarded to the host. Hence add a new
> > object SWITCHDEV_OBJ_ID_HOST_MDB, a multicast database entry to
> > forward to the host. This new object is then propagated through the
> > DSA layers. No DSA driver changes should be needed, this should just
> > work...
> 
> The patchset looks good. I would like to ask you some details though,
> because I don't understand why the semantics are different.
> 
> Technically, what happens is that an MDB entry is programmed on the
> bridge interface. From the _bridge point of view_, there is no technical
> difference as in programming an MDB entry on a bridged port. Correct?

Hi Vivien

It is not quite as simple as that. Image:

brctl addbr br0
brctl addif br0 eth2
brctl addif br0 lan0

where eth2 is just a regular ethernet interface. Say there is a join
received on eth2 for group 224.42.42.42. The IGMP snooping code in the
software bridge then needs to tell lan0 to forward all multicast
traffic for 224.42.42.42 to the software bridge, so it can forward it
to eth2. The br0 interface is not involved.

Now, my patchset is not implementing this use case. But at some point,
we probably will want to implement it. We want a generic switchdev API
which says forward all the traffic for a group to the host. The host
will then decide what to do with it.

Now, your suggestion would be to pass br0 for the use case i'm
implementing here. And we could pass eth2 for the above use cases. But
for the hardware offload, it does not matter what interface the frames
are heading towards. All the offload needs to know is that the host
software bridge wants the frames.

Multicast is often special in that you need to specify where the
frames are coming from, not where they are going to. This makes the
semantics different. And you need to keep the differences clear,
otherwise you quickly get confused. Using a different API should be a
clear warning. Be careful, the semantics are different. Re-using an
existing API, and needing to carefully look at the parameters to know
you need to do something completed different is just asking for
trouble.

      Andrew

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Andrew Lunn]

> Hi Andrew!
> 
> I tested this series today on my board with lan9303, and it does seem to
> work. But no extensive testing though.

Cool, thanks. I've only been able to test on Marvell hardware.

> Some observations, not meant to delay the series:
> 
> When local application join multicast; the driver get 2 X .port_mdb_prepare
> + 4 x .port_mdb_add for the address.

Humm, i would expect equal numbers of those.

> "bridge mdb" does not list local multicast memberships, is that a "feature"?

Yes. The br0 interface is not really a port of the bridge. It is
special. And hence many of the bridge commands don't list it.

> While I had an application joined on a multicast address; I restarted the
> networking: meaning deleting the bridge and setting it up again. No
> .port_mdb_del on the multicast address. Stopped the application. Still no
> .port_mdb_del on the multicast address. So the multicast address stays in
> the HW fdb, until I started and stopped the application again. Not really a
> problem, just an observation.

The bridge is working by snooping joins/leaves. When the application
joined the group, the kernel would of sent an IGMP report. This
triggers the snooping code to request the hardware to start forwarding
frames to the hardware. When the application quits, it might send a
leave. But it is not required. The bridge however has a timer. The
IGMP querier in your network should cause all active groups to send a
report every so often. This keeps the timer alive. If the timer
expires, the bridge should then delete the group from the hardware.

So try stopping your application, and waiting a while.

> Then finally, there is a more serious issue related to IGMPv2. As I
> suspected, with "IGMP snooping for local traffic", I have to activate "IGMP
> trapping" in the lan9303 HW in order to get it to work with a IGMPv2
> querier.

For Marvell hardware, it detects all IGMP frames and forwards them to
the CPU. It does not matter if they are IGMPv2 or v3.

> (Remember, unlike IGMPv3, IGMPv2 reports are "inband" in the
> group). The HW allows me to select which ports receive IGMP packets. The
> best is to only send to CPU, so the SW bridge can do the right thing in all
> situations. However, then we must make sure skb->offload_fwd_mark is zero
> for IGMP packets. Which was easy to do in my proof-of-concept test, but I
> suspect a proper test on ip->protocol is more complicated?

Humm, interesting. Not thought about that. And my testing has been
limited to a single switch. I will try to find a setup with multiple
switches and see what happens with the Marvell hardware.

Thanks
	Andrew

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Florian Fainelli]

Hi Andrew,

On 11/07/2017 09:42 AM, Andrew Lunn wrote:
> On Tue, Nov 07, 2017 at 12:03:54PM -0500, Vivien Didelot wrote:
>> Hi Andrew,
>>
>> Andrew Lunn <andrew@lunn.ch> writes:
>>
>>> Then starts the work passing down to the hardware that the host has
>>> joined/left a group. The existing switchdev mdb object cannot be used,
>>> since the semantics are different. The existing
>>> SWITCHDEV_OBJ_ID_PORT_MDB is used to indicate a specific multicast
>>> group should be forwarded out that port of the switch. However here we
>>> require the exact opposite. We want multicast frames for the group
>>> received on the port to the forwarded to the host. Hence add a new
>>> object SWITCHDEV_OBJ_ID_HOST_MDB, a multicast database entry to
>>> forward to the host. This new object is then propagated through the
>>> DSA layers. No DSA driver changes should be needed, this should just
>>> work...
>>
>> The patchset looks good. I would like to ask you some details though,
>> because I don't understand why the semantics are different.
>>
>> Technically, what happens is that an MDB entry is programmed on the
>> bridge interface. From the _bridge point of view_, there is no technical
>> difference as in programming an MDB entry on a bridged port. Correct?
> 
> Hi Vivien
> 
> It is not quite as simple as that. Image:
> 
> brctl addbr br0
> brctl addif br0 eth2
> brctl addif br0 lan0
> 
> where eth2 is just a regular ethernet interface. Say there is a join
> received on eth2 for group 224.42.42.42. The IGMP snooping code in the
> software bridge then needs to tell lan0 to forward all multicast
> traffic for 224.42.42.42 to the software bridge, so it can forward it
> to eth2. The br0 interface is not involved.
> 
> Now, my patchset is not implementing this use case. But at some point,
> we probably will want to implement it. We want a generic switchdev API
> which says forward all the traffic for a group to the host. The host
> will then decide what to do with it.

What this seems to indicate though is that if any of the member of the
bridge (say lan0 here) is part of a switchdev/offload capable device, we
also need to have an event targeting the management interface of that
offload device (DSA master netdev in our case) to let the specific
traffic go all the way to the software bridge and then have the software
bridge send back to the other member, right?

> 
> Now, your suggestion would be to pass br0 for the use case i'm
> implementing here. And we could pass eth2 for the above use cases. But
> for the hardware offload, it does not matter what interface the frames
> are heading towards. All the offload needs to know is that the host
> software bridge wants the frames.

Correct, but the software bridge needs to be "backed" by some kind of
management network device, here that would be lan0 + the DSA master
netdev, so we should have 3 MDB notifications to let multicast flow through:

- one targeting lan0, which would be treated as a normal switch port
- one targeting lan0's DSA master netdev, to make multicast go all the
way to the host, via lan0
- one targeting eth2, which would be a normal NIC

Does that make sense?

> 
> Multicast is often special in that you need to specify where the
> frames are coming from, not where they are going to. This makes the
> semantics different. And you need to keep the differences clear,
> otherwise you quickly get confused. Using a different API should be a
> clear warning. Be careful, the semantics are different. Re-using an
> existing API, and needing to carefully look at the parameters to know
> you need to do something completed different is just asking for
> trouble.
> 
>       Andrew
> 


-- 
Florian

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> > Then starts the work passing down to the hardware that the host has
>> > joined/left a group. The existing switchdev mdb object cannot be used,
>> > since the semantics are different. The existing
>> > SWITCHDEV_OBJ_ID_PORT_MDB is used to indicate a specific multicast
>> > group should be forwarded out that port of the switch. However here we
>> > require the exact opposite. We want multicast frames for the group
>> > received on the port to the forwarded to the host. Hence add a new
>> > object SWITCHDEV_OBJ_ID_HOST_MDB, a multicast database entry to
>> > forward to the host. This new object is then propagated through the
>> > DSA layers. No DSA driver changes should be needed, this should just
>> > work...
>> 
>> The patchset looks good. I would like to ask you some details though,
>> because I don't understand why the semantics are different.
>> 
>> Technically, what happens is that an MDB entry is programmed on the
>> bridge interface. From the _bridge point of view_, there is no technical
>> difference as in programming an MDB entry on a bridged port. Correct?
>
> It is not quite as simple as that. Image:
>
> brctl addbr br0
> brctl addif br0 eth2
> brctl addif br0 lan0
>
> where eth2 is just a regular ethernet interface. Say there is a join
> received on eth2 for group 224.42.42.42. The IGMP snooping code in the
> software bridge then needs to tell lan0 to forward all multicast
> traffic for 224.42.42.42 to the software bridge, so it can forward it
> to eth2. The br0 interface is not involved.
>
> Now, my patchset is not implementing this use case. But at some point,
> we probably will want to implement it. We want a generic switchdev API
> which says forward all the traffic for a group to the host. The host
> will then decide what to do with it.
>
> Now, your suggestion would be to pass br0 for the use case i'm
> implementing here. And we could pass eth2 for the above use cases. But
> for the hardware offload, it does not matter what interface the frames
> are heading towards. All the offload needs to know is that the host
> software bridge wants the frames.

I do understand a bit more. So when an MDB entry is programmed on a
bridge port, all we need to do is program the dedicated CPU port of a
switch port if the target bridge port isn't the switch port.

Something like:

    // the bridge code sends this mdb object:
    struct switchdev_obj_port_mdb mdb = { .orig_dev = eth2, .dev = lan0 }

    // the DSA code receives it:
    /* if the switch port isn't the target port, program its CPU port */
    dp = dsa_slave_to_port(mdb->dev)
    if (mdb->dev != mdb->orig_dev)
        dp = dp->cpu_dp;
    dsa_port_mdb_add(dp, mdb);

Doesn't this cover your use cases?


Thanks,

        Vivien

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Andrew Lunn]

> > It is not quite as simple as that. Image:
> >
> > brctl addbr br0
> > brctl addif br0 eth2
> > brctl addif br0 lan0

Hi Vivien

I will reply to your other points later. But another scenario to think
about.

Take the above configuration.

A join is received on eth2. The bridge makes switchdev calls. Your
idea would be it passes add MDB(eth2).

Now an application performs a join on br0. The bridge does nothing
with switchdev. It already gets the frames it needs.

eth2 leaves the group. The switch does nothing. It still needs the frames.

The application leaves the group on br0. The bridge makes switchdev
calls to tell the hardware to stop sending it frames. With your
scheme, it would pass del MDB(br0).

So we start it with eth2, stop it with br0. I think this makes it
clear, the interface name is not important. All we need is a bit,
which says is this a normal MDB request, or a host MDB request.  I
encode this bit by having a different requests.

       Andrew

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Florian Fainelli]

On 11/07/2017 01:01 PM, Andrew Lunn wrote:
>>> It is not quite as simple as that. Image:
>>>
>>> brctl addbr br0
>>> brctl addif br0 eth2
>>> brctl addif br0 lan0
> 
> Hi Vivien
> 
> I will reply to your other points later. But another scenario to think
> about.
> 
> Take the above configuration.
> 
> A join is received on eth2. The bridge makes switchdev calls. Your
> idea would be it passes add MDB(eth2).
> 
> Now an application performs a join on br0. The bridge does nothing
> with switchdev. It already gets the frames it needs.
> 
> eth2 leaves the group. The switch does nothing. It still needs the frames.
> 
> The application leaves the group on br0. The bridge makes switchdev
> calls to tell the hardware to stop sending it frames. With your
> scheme, it would pass del MDB(br0).
> 
> So we start it with eth2, stop it with br0. I think this makes it
> clear, the interface name is not important. All we need is a bit,
> which says is this a normal MDB request, or a host MDB request.  I
> encode this bit by having a different requests.

Andrew, I am afraid you lost me here, I don't even understand the
problem you are trying to describe :) What would be the rationale for
differentiation "normal MDB requests" from "host MDB requests", how and
why should we treat them differently? In a switch case, they all
translate to programming a MDB entry for a given switch port, right?
-- 
Florian

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Andrew Lunn]

> In a switch case, they all translate to programming a MDB entry for
> a given switch port, right?

No, in fact it is the exact opposite.

A normal switchdev MDB says send traffic for a group OUT this port.

A host switchdev MDB says send traffic coming IN from a port to the
CPU.

This is why i think there should be two different switchdev calls, but
Vivien wants one call, and to look carefully at the parameters to
determine which of the two above to do.

	  Andrew

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> In a switch case, they all translate to programming a MDB entry for
>> a given switch port, right?
>
> No, in fact it is the exact opposite.

Yes, they do. The proof is you call dsa_port_mdb_add.

> A normal switchdev MDB says send traffic for a group OUT this port.
>
> A host switchdev MDB says send traffic coming IN from a port to the
> CPU.

Still, what I see here _from a switch driver point of view_ is either
program an MDB entry on a user port, or on its CPU port.

There's no direction. There's only an association between a given switch
port and a MAC address (in a given VLAN ID for sure.)

> This is why i think there should be two different switchdev calls, but
> Vivien wants one call, and to look carefully at the parameters to
> determine which of the two above to do.

I do not "want one call". I'm not fighting you. I'm just trying to
understand and avoid adding new object when it isn't necessary. Maybe
your proposal is the correct way to go, maybe translating orig_dev to
the CPU port is just enough. The thing is it is still unclear to me.
Maybe I'm stupid.


    Vivien

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Andrew Lunn]

On Tue, Nov 07, 2017 at 05:37:32PM -0500, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> >> In a switch case, they all translate to programming a MDB entry for
> >> a given switch port, right?
> >
> > No, in fact it is the exact opposite.
> 
> Yes, they do. The proof is you call dsa_port_mdb_add.

Note that i always say switchdev.

switchdev has no concept of the CPU port. All switchdev has is the
concept of the external ports.

So when there is a join on the br0 interface, the bridge code will
iterative over each port in the bridge, and make a switchdev call to
each of the external ports in the bridge asking it to forward
multicast traffic for a group to the host.

Now, deep down in DSA, we can translate this to a dsa_port_mdb_add, on
the CPU port. And we do that for every call the bridge makes for each
of the external ports in the bridge.

However, a pure switchdev device won't do that. It does not have a CPU
port. It probably needs to add a match/action rule to its tables for
the actual external port saying to forward the frame out the slow
path.

> Still, what I see here _from a switch driver point of view_ is either
> program an MDB entry on a user port, or on its CPU port.

I agree with this, if you make one change:

_from a DSA switch driver point of view_

However, in the general case, this is not true. We need an API which
works for Mellonex and Netranome as well, systems without a CPU port.

      Andrew

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Florian Fainelli]

On 11/07/2017 03:17 PM, Andrew Lunn wrote:
> On Tue, Nov 07, 2017 at 05:37:32PM -0500, Vivien Didelot wrote:
>> Hi Andrew,
>>
>> Andrew Lunn <andrew@lunn.ch> writes:
>>
>>>> In a switch case, they all translate to programming a MDB entry for
>>>> a given switch port, right?
>>>
>>> No, in fact it is the exact opposite.
>>
>> Yes, they do. The proof is you call dsa_port_mdb_add.
> 
> Note that i always say switchdev.
> 
> switchdev has no concept of the CPU port. All switchdev has is the
> concept of the external ports.

Correct.

> 
> So when there is a join on the br0 interface, the bridge code will
> iterative over each port in the bridge, and make a switchdev call to
> each of the external ports in the bridge asking it to forward
> multicast traffic for a group to the host.

Right, and that makes sense thus far.

> 
> Now, deep down in DSA, we can translate this to a dsa_port_mdb_add, on
> the CPU port. And we do that for every call the bridge makes for each
> of the external ports in the bridge.

Right, and we should actually do that, because this is a DSA specific
detail: that there is a separate management port that needs specific
treatment here.

> 
> However, a pure switchdev device won't do that. It does not have a CPU
> port. It probably needs to add a match/action rule to its tables for
> the actual external port saying to forward the frame out the slow
> path.

Right, that's why DSA builds on top of switchdev for most notifications
and also generate its own for things that are inherently DSA specific.

> 
>> Still, what I see here _from a switch driver point of view_ is either
>> program an MDB entry on a user port, or on its CPU port.
> 
> I agree with this, if you make one change:
> 
> _from a DSA switch driver point of view_
> 
> However, in the general case, this is not true. We need an API which
> works for Mellonex and Netranome as well, systems without a CPU port.

We can have the exact same type of notification being sent today and
with your changes target the bridge network devices, any other driver
other than DSA which implements switchdev should just ignore that
notification when they cannot resolve this network device to something
that makes sense for their HW. In fact, Jiri and Ido (which should
probably be copied on this patch series, along with Nikolay) was adamant
to the idea:

https://lists.linux-foundation.org/pipermail/bridge/2016-November/010124.html

The usual model for notifications is that if you can interpret and act
on them, great, if you can't do nothing.
-- 
Florian

Re: [PATCH v3 net-next 1/5] net: bridge: Rename mglist to host_joined

[Nikolay Aleksandrov]

On  7.11.2017 01:26, Andrew Lunn wrote:
> The boolean mglist indicates the host has joined a particular
> multicast group on the bridge interface. It is badly named, obscuring
> what is means. Rename it.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  net/bridge/br_input.c     |  2 +-
>  net/bridge/br_mdb.c       |  2 +-
>  net/bridge/br_multicast.c | 14 +++++++-------
>  net/bridge/br_private.h   |  2 +-
>  4 files changed, 10 insertions(+), 10 deletions(-)
> 

Much better, thanks!

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Re: [PATCH v3 net-next 2/5] net: bridge: Send notification when host join/leaves a group

[Nikolay Aleksandrov]

On  7.11.2017 01:26, Andrew Lunn wrote:
> The host can join or leave a multicast group on the brX interface, as
> indicated by IGMP snooping.  This is tracked within the bridge
> multicast code. Send a notification when this happens, in the same way
> a notification is sent when a port of the bridge joins/leaves a group
> because of IGMP snooping.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  net/bridge/br_mdb.c       | 9 ++++++---
>  net/bridge/br_multicast.c | 6 +++++-
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 

We really should rewrite __br_mdb_notify to be more readable, but that
is beyond the scope of this set and can be done later.

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Re: [PATCH v3 net-next 1/5] net: bridge: Rename mglist to host_joined

[Florian Fainelli]

On 11/06/2017 03:26 PM, Andrew Lunn wrote:
> The boolean mglist indicates the host has joined a particular
> multicast group on the bridge interface. It is badly named, obscuring
> what is means. Rename it.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v3 net-next 2/5] net: bridge: Send notification when host join/leaves a group

[Florian Fainelli]

On 11/06/2017 03:26 PM, Andrew Lunn wrote:
> The host can join or leave a multicast group on the brX interface, as
> indicated by IGMP snooping.  This is tracked within the bridge
> multicast code. Send a notification when this happens, in the same way
> a notification is sent when a port of the bridge joins/leaves a group
> because of IGMP snooping.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v3 net-next 3/5] net: bridge: Add/del switchdev object on host join/leave

[Nikolay Aleksandrov]

On  7.11.2017 01:26, Andrew Lunn wrote:
> When the host joins or leaves a multicast group, use switchdev to add
> an object to the hardware to forward traffic for the group to the
> host.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  include/net/switchdev.h   |  1 +
>  net/bridge/br_mdb.c       | 39 +++++++++++++++++++++++++++++++++++++++
>  net/switchdev/switchdev.c |  2 ++
>  3 files changed, 42 insertions(+)
> 

One minor nit below, functionally looks good.

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index d756fbe46625..39bc855d7fee 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -76,6 +76,7 @@ enum switchdev_obj_id {
>  	SWITCHDEV_OBJ_ID_UNDEFINED,
>  	SWITCHDEV_OBJ_ID_PORT_VLAN,
>  	SWITCHDEV_OBJ_ID_PORT_MDB,
> +	SWITCHDEV_OBJ_ID_HOST_MDB,
>  };
>  
>  struct switchdev_obj {
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index 702408d2a93c..2a3daa11f60b 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -292,6 +292,42 @@ static void br_mdb_complete(struct net_device *dev, int err, void *priv)
>  	kfree(priv);
>  }
>  
> +static void br_mdb_switchdev_host_port(struct net_device *dev,
> +				       struct net_device *lower_dev,
> +				       struct br_mdb_entry *entry, int type)
> +{
> +	struct switchdev_obj_port_mdb mdb = {
> +		.obj = {
> +			.id = SWITCHDEV_OBJ_ID_HOST_MDB,
> +			.flags = SWITCHDEV_F_DEFER,
> +		},
> +		.vid = entry->vid,
> +	};
> +
> +	if (entry->addr.proto == htons(ETH_P_IP))
> +		ip_eth_mc_map(entry->addr.u.ip4, mdb.addr);
> +#if IS_ENABLED(CONFIG_IPV6)
> +	else
> +		ipv6_eth_mc_map(&entry->addr.u.ip6, mdb.addr);
> +#endif
> +
> +	mdb.obj.orig_dev = dev;
> +	if (type == RTM_NEWMDB)
> +		switchdev_port_obj_add(lower_dev, &mdb.obj);
> +	if (type == RTM_DELMDB)
> +		switchdev_port_obj_del(lower_dev, &mdb.obj);

nit: type can be only one of these, could you make that explicit (e.g.
switch) ?


> +}
> +
> +static void br_mdb_switchdev_host(struct net_device *dev,
> +				  struct br_mdb_entry *entry, int type)
> +{
> +	struct net_device *lower_dev;
> +	struct list_head *iter;
> +
> +	netdev_for_each_lower_dev(dev, lower_dev, iter)
> +		br_mdb_switchdev_host_port(dev, lower_dev, entry, type);
> +}
> +
>  static void __br_mdb_notify(struct net_device *dev, struct net_bridge_port *p,
>  			    struct br_mdb_entry *entry, int type)
>  {
> @@ -331,6 +367,9 @@ static void __br_mdb_notify(struct net_device *dev, struct net_bridge_port *p,
>  		switchdev_port_obj_del(port_dev, &mdb.obj);
>  	}
>  
> +	if (!p)
> +		br_mdb_switchdev_host(dev, entry, type);
> +
>  	skb = nlmsg_new(rtnl_mdb_nlmsg_size(), GFP_ATOMIC);
>  	if (!skb)
>  		goto errout;
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 0531b41d1f2d..74b9d916a58b 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -345,6 +345,8 @@ static size_t switchdev_obj_size(const struct switchdev_obj *obj)
>  		return sizeof(struct switchdev_obj_port_vlan);
>  	case SWITCHDEV_OBJ_ID_PORT_MDB:
>  		return sizeof(struct switchdev_obj_port_mdb);
> +	case SWITCHDEV_OBJ_ID_HOST_MDB:
> +		return sizeof(struct switchdev_obj_port_mdb);
>  	default:
>  		BUG();
>  	}
> 

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Egil Hjelmeland]

On 07. nov. 2017 18:58, Andrew Lunn wrote:
>> Hi Andrew!
>>
>> When local application join multicast; the driver get 2 X .port_mdb_prepare
>> + 4 x .port_mdb_add for the address.
> 
> Humm, i would expect equal numbers of those.
> 

To be precise: it is (1 .port_mdb_prepare + 2 x .port_mdb_add), two times.

I see that the outer repeat is due to netdev_for_each_lower_dev() in 
br_mdb_switchdev_host(). So that means we get one notification for each 
dsa-port which is joined to the bridge.


But _all_ .port_mdb_add are repeated twice as well. This is more 
interesting. I suspect that there is a missing "return 0;" in 
dsa_switch_mdb_add(), at the end of the "if 
(switchdev_trans_ph_prepare(trans)) {". Dating back to
a1a6b7ea7f2de270a51360cc48e7c49f7a283dda.



>> While I had an application joined on a multicast address; I restarted the
>> networking: meaning deleting the bridge and setting it up again. No
>> .port_mdb_del on the multicast address. Stopped the application. Still no
>> .port_mdb_del on the multicast address. So the multicast address stays in
>> the HW fdb, until I started and stopped the application again. Not really a
>> problem, just an observation.
> 
> The bridge is working by snooping joins/leaves. When the application
> joined the group, the kernel would of sent an IGMP report. This
> triggers the snooping code to request the hardware to start forwarding
> frames to the hardware. When the application quits, it might send a
> leave. But it is not required. The bridge however has a timer. The
> IGMP querier in your network should cause all active groups to send a
> report every so often. This keeps the timer alive. If the timer
> expires, the bridge should then delete the group from the hardware.
> 
> So try stopping your application, and waiting a while.
> 

I waited 10 minutes after stopping the application, still no cleanup. 
That was the case when restarted the networking while the application 
was running.

But when stopping the application having not restarted the networking, 
the .port_mdb_del happen within few seconds.

Cheers
Egil

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Andrew Lunn]

On Wed, Nov 08, 2017 at 04:11:12PM +0100, Egil Hjelmeland wrote:
> On 07. nov. 2017 18:58, Andrew Lunn wrote:
> >>Hi Andrew!
> >>
> >>When local application join multicast; the driver get 2 X .port_mdb_prepare
> >>+ 4 x .port_mdb_add for the address.
> >
> >Humm, i would expect equal numbers of those.
> >
> 
> To be precise: it is (1 .port_mdb_prepare + 2 x .port_mdb_add), two times.
> 
> I see that the outer repeat is due to netdev_for_each_lower_dev() in
> br_mdb_switchdev_host(). So that means we get one notification for each
> dsa-port which is joined to the bridge.

Correct. That is intentional.

> But _all_ .port_mdb_add are repeated twice as well. This is more
> interesting. I suspect that there is a missing "return 0;" in
> dsa_switch_mdb_add(), at the end of the "if
> (switchdev_trans_ph_prepare(trans)) {". Dating back to
> a1a6b7ea7f2de270a51360cc48e7c49f7a283dda.

Yes. Good catch.

     Andrew

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Vivien Didelot]

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

> But _all_ .port_mdb_add are repeated twice as well. This is more 
> interesting. I suspect that there is a missing "return 0;" in 
> dsa_switch_mdb_add(), at the end of the "if 
> (switchdev_trans_ph_prepare(trans)) {". Dating back to
> a1a6b7ea7f2de270a51360cc48e7c49f7a283dda.

Thank for the report! I've sent a fix to -net for MDB and VLAN as well
which has the same issue.

      Vivien

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Tue,  7 Nov 2017 00:26:53 +0100

> The linux bridge supports IGMP snooping. It will listen to IGMP
> reports on bridge ports and keep track of which groups have been
> joined on an interface. It will then forward multicast based on this
> group membership.
> 
> When the bridge adds or removed groups from an interface, it uses
> switchdev to request the hardware add an mdb to a port, so the
> hardware can perform the selective forwarding between ports.
> 
> What is not covered by the current bridge code, is IGMP joins/leaves
> from the host on the brX interface. These are not reported via
> switchdev so that hardware knows the local host is interested in the
> multicast frames.
> 
> Luckily, the bridge does track joins/leaves on the brX interface. The
> code is obfusticated, which is why i missed it with my first attempt.
> So the first patch tries to remove this obfustication. Currently,
> there is no notifications sent when the bridge interface joins a
> group. The second patch adds them. bridge monitor then shows
> joins/leaves in the same way as for other ports of the bridge.
> 
> Then starts the work passing down to the hardware that the host has
> joined/left a group. The existing switchdev mdb object cannot be used,
> since the semantics are different. The existing
> SWITCHDEV_OBJ_ID_PORT_MDB is used to indicate a specific multicast
> group should be forwarded out that port of the switch. However here we
> require the exact opposite. We want multicast frames for the group
> received on the port to the forwarded to the host. Hence add a new
> object SWITCHDEV_OBJ_ID_HOST_MDB, a multicast database entry to
> forward to the host. This new object is then propagated through the
> DSA layers. No DSA driver changes should be needed, this should just
> work...

Series applied, with the spurious whitespace change removed from patch
#5.

Thanks!

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Thu, 09 Nov 2017 11:30:32 +0900 (KST)

> Series applied, with the spurious whitespace change removed from patch
> #5.

Actually I had to revert, this doesn't build:

net/dsa/slave.c: In function ‘dsa_slave_port_obj_add’:
net/dsa/slave.c:311:26: warning: passing argument 1 of ‘dsa_port_mdb_add’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   err = dsa_port_mdb_add(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj),
                          ^~
In file included from net/dsa/slave.c:26:0:
net/dsa/dsa_priv.h:150:5: note: expected ‘struct dsa_port *’ but argument is of type ‘const struct dsa_port *’
 int dsa_port_mdb_add(struct dsa_port *dp,
     ^~~~~~~~~~~~~~~~
net/dsa/slave.c: In function ‘dsa_slave_port_obj_del’:
net/dsa/slave.c:340:26: warning: passing argument 1 of ‘dsa_port_mdb_del’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   err = dsa_port_mdb_del(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj));
                          ^~
In file included from net/dsa/slave.c:26:0:
net/dsa/dsa_priv.h:153:5: note: expected ‘struct dsa_port *’ but argument is of type ‘const struct dsa_port *’
 int dsa_port_mdb_del(struct dsa_port *dp,
     ^~~~~~~~~~~~~~~~
  C-c C-cmake[3]: *** [scripts/Makefile.build:320: drivers/net/team/team.o] Interrupt

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Vivien Didelot]

Hi David,

David Miller <davem@davemloft.net> writes:

> From: David Miller <davem@davemloft.net>
> Date: Thu, 09 Nov 2017 11:30:32 +0900 (KST)
>
>> Series applied, with the spurious whitespace change removed from patch
>> #5.
>
> Actually I had to revert, this doesn't build:
>
> net/dsa/slave.c: In function ‘dsa_slave_port_obj_add’:
> net/dsa/slave.c:311:26: warning: passing argument 1 of ‘dsa_port_mdb_add’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>    err = dsa_port_mdb_add(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj),
>                           ^~
> In file included from net/dsa/slave.c:26:0:
> net/dsa/dsa_priv.h:150:5: note: expected ‘struct dsa_port *’ but argument is of type ‘const struct dsa_port *’
>  int dsa_port_mdb_add(struct dsa_port *dp,
>      ^~~~~~~~~~~~~~~~
> net/dsa/slave.c: In function ‘dsa_slave_port_obj_del’:
> net/dsa/slave.c:340:26: warning: passing argument 1 of ‘dsa_port_mdb_del’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>    err = dsa_port_mdb_del(dp->cpu_dp, SWITCHDEV_OBJ_PORT_MDB(obj));
>                           ^~
> In file included from net/dsa/slave.c:26:0:
> net/dsa/dsa_priv.h:153:5: note: expected ‘struct dsa_port *’ but argument is of type ‘const struct dsa_port *’
>  int dsa_port_mdb_del(struct dsa_port *dp,
>      ^~~~~~~~~~~~~~~~
>   C-c C-cmake[3]: *** [scripts/Makefile.build:320: drivers/net/team/team.o] Interrupt

This patch series is passing cpu_dp to dsa_port_mdb_* which is const.
I have sent a revert patch to net-next to make it non-const again, so
patch series like this can be applied without compile error.


Thanks,

        Vivien

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Florian Fainelli]

On 11/07/2017 04:41 PM, Florian Fainelli wrote:
> On 11/07/2017 03:17 PM, Andrew Lunn wrote:
>> On Tue, Nov 07, 2017 at 05:37:32PM -0500, Vivien Didelot wrote:
>>> Hi Andrew,
>>>
>>> Andrew Lunn <andrew@lunn.ch> writes:
>>>
>>>>> In a switch case, they all translate to programming a MDB entry for
>>>>> a given switch port, right?
>>>>
>>>> No, in fact it is the exact opposite.
>>>
>>> Yes, they do. The proof is you call dsa_port_mdb_add.
>>
>> Note that i always say switchdev.
>>
>> switchdev has no concept of the CPU port. All switchdev has is the
>> concept of the external ports.
> 
> Correct.
> 
>>
>> So when there is a join on the br0 interface, the bridge code will
>> iterative over each port in the bridge, and make a switchdev call to
>> each of the external ports in the bridge asking it to forward
>> multicast traffic for a group to the host.
> 
> Right, and that makes sense thus far.
> 
>>
>> Now, deep down in DSA, we can translate this to a dsa_port_mdb_add, on
>> the CPU port. And we do that for every call the bridge makes for each
>> of the external ports in the bridge.
> 
> Right, and we should actually do that, because this is a DSA specific
> detail: that there is a separate management port that needs specific
> treatment here.
> 
>>
>> However, a pure switchdev device won't do that. It does not have a CPU
>> port. It probably needs to add a match/action rule to its tables for
>> the actual external port saying to forward the frame out the slow
>> path.
> 
> Right, that's why DSA builds on top of switchdev for most notifications
> and also generate its own for things that are inherently DSA specific.
> 
>>
>>> Still, what I see here _from a switch driver point of view_ is either
>>> program an MDB entry on a user port, or on its CPU port.
>>
>> I agree with this, if you make one change:
>>
>> _from a DSA switch driver point of view_
>>
>> However, in the general case, this is not true. We need an API which
>> works for Mellonex and Netranome as well, systems without a CPU port.
> 
> We can have the exact same type of notification being sent today and
> with your changes target the bridge network devices, any other driver
> other than DSA which implements switchdev should just ignore that
> notification when they cannot resolve this network device to something
> that makes sense for their HW. In fact, Jiri and Ido (which should
> probably be copied on this patch series, along with Nikolay) was adamant
> to the idea:
> 
> https://lists.linux-foundation.org/pipermail/bridge/2016-November/010124.html
> 
> The usual model for notifications is that if you can interpret and act
> on them, great, if you can't do nothing.

BTW, to continue on your argument that we want this to work for both DSA
and switchdev, keep in mind that switchdev drivers, although
conceptually similar to DSA devices do not have a management interface
that consists of a separate CPU port, the management interface is
intertwined with DMA/PIO and specific descriptors within the switch
hardware. This is what differentiates switchdev drivers from DSA
drivers, switchdev drivers do not really have a concept of specific
host/CPU management interface, the over arching hardware/driver
represent the management interface and every other port is an
user-facing port.

This means that switchdev drivers won't ever have to treat a HOST_MDB
notification any differently than a PORT_MDB notification, because in
their case, they would decide, on a per-port basis whether to trap to
management. In DSA, we want the same, except that the management port is
also a semi-normal CPU port of the switch, so we need to target it
specifically.

My 2 cents.
-- 
Florian

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Andrew Lunn]

> This means that switchdev drivers won't ever have to treat a HOST_MDB
> notification any differently than a PORT_MDB notification

No, they need to treat it very differently. 

A PORT_MDB says that frames for a group should be sent out that port.
So it probably needs to iterate all the ports in the bridge and add a
match/action to each port saying frames coming in for that group
should be sent out the port listed in the PORT_MDB.

A HOST_MDB say that frames for a group coming in from the port listed
in the HOST_MDB must be sent to the host. The match/action applies
directly to the port, other ports are not involved.

	 Andrew

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Florian Fainelli]

On 11/09/2017 11:30 AM, Andrew Lunn wrote:
>> This means that switchdev drivers won't ever have to treat a HOST_MDB
>> notification any differently than a PORT_MDB notification
> 
> No, they need to treat it very differently. 

Allow me to rephrase, switchdev drivers will ignore HOST_MDB
notifications because that does not resolve to something they can do
something about.

> 
> A PORT_MDB says that frames for a group should be sent out that port.
> So it probably needs to iterate all the ports in the bridge and add a
> match/action to each port saying frames coming in for that group
> should be sent out the port listed in the PORT_MDB.
> 
> A HOST_MDB say that frames for a group coming in from the port listed
> in the HOST_MDB must be sent to the host. The match/action applies
> directly to the port, other ports are not involved.

Fine, then add a boolean to the PORT_MDB notification that says ingress
or egress and voila, or am I missing something?
-- 
Florian

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Andrew Lunn]

On Thu, Nov 09, 2017 at 11:38:26AM -0800, Florian Fainelli wrote:
> On 11/09/2017 11:30 AM, Andrew Lunn wrote:
> >> This means that switchdev drivers won't ever have to treat a HOST_MDB
> >> notification any differently than a PORT_MDB notification
> > 
> > No, they need to treat it very differently. 
> 
> Allow me to rephrase, switchdev drivers will ignore HOST_MDB
> notifications because that does not resolve to something they can do
> something about.

Hi Florian

Yes, they can. In fact, if they want to support IGMP snooping on the
bridge interface, they have to. How else do they know to forward
traffic to the host?

> Fine, then add a boolean to the PORT_MDB notification that says ingress
> or egress and voila, or am I missing something?

But since the semantics are so different, why not just have a
different messages?

	  Andrew

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Florian Fainelli]

+Ido, Jiri,

On 11/09/2017 12:21 PM, Andrew Lunn wrote:
> On Thu, Nov 09, 2017 at 11:38:26AM -0800, Florian Fainelli wrote:
>> On 11/09/2017 11:30 AM, Andrew Lunn wrote:
>>>> This means that switchdev drivers won't ever have to treat a HOST_MDB
>>>> notification any differently than a PORT_MDB notification
>>>
>>> No, they need to treat it very differently. 
>>
>> Allow me to rephrase, switchdev drivers will ignore HOST_MDB
>> notifications because that does not resolve to something they can do
>> something about.
> 
> Hi Florian
> 
> Yes, they can. In fact, if they want to support IGMP snooping on the
> bridge interface, they have to. How else do they know to forward
> traffic to the host?

On a switchdev fabric, you need to have at least one user-facing port be
a member of the bridge, and when the switchdev driver configures that,
it should just make the IGMP packets trap to the management interface
such that they can be delivered from the port member to the bridge
network device (br0). In that case, I don't really see why you would
need to send a HOST_MDB message to a switchdev fabric, since that should
be part of enslaving the port to the bridge in the first place and
appropriately configure the management interface to get IGMP snooping,
BDPU etc.

> 
>> Fine, then add a boolean to the PORT_MDB notification that says ingress
>> or egress and voila, or am I missing something?
> 
> But since the semantics are so different, why not just have a
> different messages?
> 
> 	  Andrew
> 


-- 
Florian

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Andrew Lunn]

On Thu, Nov 09, 2017 at 12:35:32PM -0800, Florian Fainelli wrote:
> +Ido, Jiri,
> 
> On 11/09/2017 12:21 PM, Andrew Lunn wrote:
> > On Thu, Nov 09, 2017 at 11:38:26AM -0800, Florian Fainelli wrote:
> >> On 11/09/2017 11:30 AM, Andrew Lunn wrote:
> >>>> This means that switchdev drivers won't ever have to treat a HOST_MDB
> >>>> notification any differently than a PORT_MDB notification
> >>>
> >>> No, they need to treat it very differently. 
> >>
> >> Allow me to rephrase, switchdev drivers will ignore HOST_MDB
> >> notifications because that does not resolve to something they can do
> >> something about.
> > 
> > Hi Florian
> > 
> > Yes, they can. In fact, if they want to support IGMP snooping on the
> > bridge interface, they have to. How else do they know to forward
> > traffic to the host?
> 
> On a switchdev fabric, you need to have at least one user-facing port be
> a member of the bridge, and when the switchdev driver configures that,
> it should just make the IGMP packets trap to the management interface
> such that they can be delivered from the port member to the bridge
> network device (br0). In that case, I don't really see why you would
> need to send a HOST_MDB message to a switchdev fabric, since that should
> be part of enslaving the port to the bridge in the first place and
> appropriately configure the management interface to get IGMP snooping,
> BDPU etc.

So your network is carrying gigabits of multicast traffic. Are you
saying it should all hit the host, so the bridge can throw it away?
No, it is much more efficient that the bridge tells the switch when it
is interested in a specific multicast group. I.e. it sends a HOST_MDB
request for the group. Only then will the switch start to send the
data for that group to the host.

     Andrew

Re: [PATCH v3 net-next 0/5] IGMP snooping for local traffic

[Ido Schimmel]

On Thu, Nov 09, 2017 at 10:13:15PM +0100, Andrew Lunn wrote:
> On Thu, Nov 09, 2017 at 12:35:32PM -0800, Florian Fainelli wrote:
> > +Ido, Jiri,
> > 
> > On 11/09/2017 12:21 PM, Andrew Lunn wrote:
> > > On Thu, Nov 09, 2017 at 11:38:26AM -0800, Florian Fainelli wrote:
> > >> On 11/09/2017 11:30 AM, Andrew Lunn wrote:
> > >>>> This means that switchdev drivers won't ever have to treat a HOST_MDB
> > >>>> notification any differently than a PORT_MDB notification
> > >>>
> > >>> No, they need to treat it very differently. 
> > >>
> > >> Allow me to rephrase, switchdev drivers will ignore HOST_MDB
> > >> notifications because that does not resolve to something they can do
> > >> something about.
> > > 
> > > Hi Florian
> > > 
> > > Yes, they can. In fact, if they want to support IGMP snooping on the
> > > bridge interface, they have to. How else do they know to forward
> > > traffic to the host?
> > 
> > On a switchdev fabric, you need to have at least one user-facing port be
> > a member of the bridge, and when the switchdev driver configures that,
> > it should just make the IGMP packets trap to the management interface
> > such that they can be delivered from the port member to the bridge
> > network device (br0). In that case, I don't really see why you would
> > need to send a HOST_MDB message to a switchdev fabric, since that should
> > be part of enslaving the port to the bridge in the first place and
> > appropriately configure the management interface to get IGMP snooping,
> > BDPU etc.
> 
> So your network is carrying gigabits of multicast traffic. Are you
> saying it should all hit the host, so the bridge can throw it away?
> No, it is much more efficient that the bridge tells the switch when it
> is interested in a specific multicast group. I.e. it sends a HOST_MDB
> request for the group. Only then will the switch start to send the
> data for that group to the host.

Yep. We already have support for marking br0 as router port which means
the bridge driver will get both unregistered multicast packets and
packets hitting mdb entries. With your patch, we'll need to add the
ability to get only packets hitting a specific mdb entry which should be
trivial enough.

Florian, you're right that by default IGMP (control) packets are trapped
to the bridge driver, but Andrew's set adds the ability to trap the
_data_ packets hitting a specific mdb entry.