[PATCH net-next] bridge: multicast to unicast

[PATCH net-next] bridge: multicast to unicast

[Linus Lüssing]

Implements an optional, per bridge port flag and feature to deliver
multicast packets to any host on the according port via unicast
individually. This is done by copying the packet per host and
changing the multicast destination MAC to a unicast one accordingly.

multicast-to-unicast works on top of the multicast snooping feature of
the bridge. Which means unicast copies are only delivered to hosts which
are interested in it and signalized this via IGMP/MLD reports
previously.

This feature is intended for interface types which have a more reliable
and/or efficient way to deliver unicast packets than broadcast ones
(e.g. wifi).

However, it should only be enabled on interfaces where no IGMPv2/MLDv1
report suppression takes place. This feature is disabled by default.

The initial patch and idea is from Felix Fietkau.

Cc: Felix Fietkau <nbd@nbd.name>
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>

---

This feature is used and enabled by default in OpenWRT and LEDE for AP
interfaces for more than a year now to allow both a more robust multicast
delivery and multicast at higher rates (e.g. multicast streaming).

In OpenWRT/LEDE the IGMP/MLD report suppression issue is overcome by
the network daemon enabling AP isolation and by that separating all STAs.
Delivery of STA-to-STA IP mulitcast is made possible again by
enabling and utilizing the bridge hairpin mode, which considers the
incoming port as a potential outgoing port, too.

Hairpin-mode is performed after multicast snooping, therefore leading to
only deliver reports to STAs running a multicast router.
---
 include/linux/if_bridge.h |  1 +
 net/bridge/br_forward.c   | 44 +++++++++++++++++++++--
 net/bridge/br_mdb.c       |  2 +-
 net/bridge/br_multicast.c | 92 ++++++++++++++++++++++++++++++++++-------------
 net/bridge/br_private.h   |  4 ++-
 net/bridge/br_sysfs_if.c  |  2 ++
 6 files changed, 115 insertions(+), 30 deletions(-)

diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
index c6587c0..f1b0d78 100644
--- a/include/linux/if_bridge.h
+++ b/include/linux/if_bridge.h
@@ -46,6 +46,7 @@ struct br_ip_list {
 #define BR_LEARNING_SYNC	BIT(9)
 #define BR_PROXYARP_WIFI	BIT(10)
 #define BR_MCAST_FLOOD		BIT(11)
+#define BR_MULTICAST_TO_UCAST	BIT(12)
 
 #define BR_DEFAULT_AGEING_TIME	(300 * HZ)
 
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index 7cb41ae..49d742d 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -174,6 +174,33 @@ static struct net_bridge_port *maybe_deliver(
 	return p;
 }
 
+static struct net_bridge_port *maybe_deliver_addr(
+	struct net_bridge_port *prev, struct net_bridge_port *p,
+	struct sk_buff *skb, const unsigned char *addr,
+	bool local_orig)
+{
+	struct net_device *dev = BR_INPUT_SKB_CB(skb)->brdev;
+	const unsigned char *src = eth_hdr(skb)->h_source;
+
+	if (!should_deliver(p, skb))
+		return prev;
+
+	/* Even with hairpin, no soliloquies - prevent breaking IPv6 DAD */
+	if (skb->dev == p->dev && ether_addr_equal(src, addr))
+		return prev;
+
+	skb = skb_copy(skb, GFP_ATOMIC);
+	if (!skb) {
+		dev->stats.tx_dropped++;
+		return prev;
+	}
+
+	memcpy(eth_hdr(skb)->h_dest, addr, ETH_ALEN);
+	__br_forward(p, skb, local_orig);
+
+	return prev;
+}
+
 /* called under rcu_read_lock */
 void br_flood(struct net_bridge *br, struct sk_buff *skb,
 	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
@@ -231,6 +258,7 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
 	struct net_bridge_port *prev = NULL;
 	struct net_bridge_port_group *p;
 	struct hlist_node *rp;
+	const unsigned char *addr;
 
 	rp = rcu_dereference(hlist_first_rcu(&br->router_list));
 	p = mdst ? rcu_dereference(mdst->ports) : NULL;
@@ -241,10 +269,20 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
 		rport = rp ? hlist_entry(rp, struct net_bridge_port, rlist) :
 			     NULL;
 
-		port = (unsigned long)lport > (unsigned long)rport ?
-		       lport : rport;
+		if ((unsigned long)lport > (unsigned long)rport) {
+			port = lport;
+			addr = p->unicast ? p->eth_addr : NULL;
+		} else {
+			port = rport;
+			addr = NULL;
+		}
+
+		if (addr)
+			prev = maybe_deliver_addr(prev, port, skb, addr,
+						  local_orig);
+		else
+			prev = maybe_deliver(prev, port, skb, local_orig);
 
-		prev = maybe_deliver(prev, port, skb, local_orig);
 		if (IS_ERR(prev))
 			goto out;
 		if (prev == port)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 7dbc80d..056e6ac 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -531,7 +531,7 @@ static int br_mdb_add_group(struct net_bridge *br, struct net_bridge_port *port,
 			break;
 	}
 
-	p = br_multicast_new_port_group(port, group, *pp, state);
+	p = br_multicast_new_port_group(port, group, *pp, state, NULL);
 	if (unlikely(!p))
 		return -ENOMEM;
 	rcu_assign_pointer(*pp, p);
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index b30e77e..470a2409 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -43,12 +43,14 @@ static void br_multicast_add_router(struct net_bridge *br,
 static void br_ip4_multicast_leave_group(struct net_bridge *br,
 					 struct net_bridge_port *port,
 					 __be32 group,
-					 __u16 vid);
+					 __u16 vid,
+					 const unsigned char *src);
+
 #if IS_ENABLED(CONFIG_IPV6)
 static void br_ip6_multicast_leave_group(struct net_bridge *br,
 					 struct net_bridge_port *port,
 					 const struct in6_addr *group,
-					 __u16 vid);
+					 __u16 vid, const unsigned char *src);
 #endif
 unsigned int br_mdb_rehash_seq;
 
@@ -711,7 +713,8 @@ struct net_bridge_port_group *br_multicast_new_port_group(
 			struct net_bridge_port *port,
 			struct br_ip *group,
 			struct net_bridge_port_group __rcu *next,
-			unsigned char flags)
+			unsigned char flags,
+			const unsigned char *src)
 {
 	struct net_bridge_port_group *p;
 
@@ -726,12 +729,35 @@ struct net_bridge_port_group *br_multicast_new_port_group(
 	hlist_add_head(&p->mglist, &port->mglist);
 	setup_timer(&p->timer, br_multicast_port_group_expired,
 		    (unsigned long)p);
+
+	if ((port->flags & BR_MULTICAST_TO_UCAST) && src) {
+		memcpy(p->eth_addr, src, ETH_ALEN);
+		p->unicast = true;
+	}
+
 	return p;
 }
 
+static bool br_port_group_equal(struct net_bridge_port_group *p,
+				struct net_bridge_port *port,
+				const unsigned char *src)
+{
+	if (p->port != port)
+		return false;
+
+	if (!p->unicast)
+		return true;
+
+	if (!src)
+		return false;
+
+	return ether_addr_equal(src, p->eth_addr);
+}
+
 static int br_multicast_add_group(struct net_bridge *br,
 				  struct net_bridge_port *port,
-				  struct br_ip *group)
+				  struct br_ip *group,
+				  const unsigned char *src)
 {
 	struct net_bridge_port_group __rcu **pp;
 	struct net_bridge_port_group *p;
@@ -758,13 +784,13 @@ static int br_multicast_add_group(struct net_bridge *br,
 	for (pp = &mp->ports;
 	     (p = mlock_dereference(*pp, br)) != NULL;
 	     pp = &p->next) {
-		if (p->port == port)
+		if (br_port_group_equal(p, port, src))
 			goto found;
 		if ((unsigned long)p->port < (unsigned long)port)
 			break;
 	}
 
-	p = br_multicast_new_port_group(port, group, *pp, 0);
+	p = br_multicast_new_port_group(port, group, *pp, 0, src);
 	if (unlikely(!p))
 		goto err;
 	rcu_assign_pointer(*pp, p);
@@ -783,7 +809,8 @@ static int br_multicast_add_group(struct net_bridge *br,
 static int br_ip4_multicast_add_group(struct net_bridge *br,
 				      struct net_bridge_port *port,
 				      __be32 group,
-				      __u16 vid)
+				      __u16 vid,
+				      const unsigned char *src)
 {
 	struct br_ip br_group;
 
@@ -794,14 +821,15 @@ static int br_ip4_multicast_add_group(struct net_bridge *br,
 	br_group.proto = htons(ETH_P_IP);
 	br_group.vid = vid;
 
-	return br_multicast_add_group(br, port, &br_group);
+	return br_multicast_add_group(br, port, &br_group, src);
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
 static int br_ip6_multicast_add_group(struct net_bridge *br,
 				      struct net_bridge_port *port,
 				      const struct in6_addr *group,
-				      __u16 vid)
+				      __u16 vid,
+				      const unsigned char *src)
 {
 	struct br_ip br_group;
 
@@ -812,7 +840,7 @@ static int br_ip6_multicast_add_group(struct net_bridge *br,
 	br_group.proto = htons(ETH_P_IPV6);
 	br_group.vid = vid;
 
-	return br_multicast_add_group(br, port, &br_group);
+	return br_multicast_add_group(br, port, &br_group, src);
 }
 #endif
 
@@ -1081,6 +1109,7 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
 					 struct sk_buff *skb,
 					 u16 vid)
 {
+	const unsigned char *src;
 	struct igmpv3_report *ih;
 	struct igmpv3_grec *grec;
 	int i;
@@ -1121,12 +1150,14 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
 			continue;
 		}
 
+		src = eth_hdr(skb)->h_source;
 		if ((type == IGMPV3_CHANGE_TO_INCLUDE ||
 		     type == IGMPV3_MODE_IS_INCLUDE) &&
 		    ntohs(grec->grec_nsrcs) == 0) {
-			br_ip4_multicast_leave_group(br, port, group, vid);
+			br_ip4_multicast_leave_group(br, port, group, vid, src);
 		} else {
-			err = br_ip4_multicast_add_group(br, port, group, vid);
+			err = br_ip4_multicast_add_group(br, port, group, vid,
+							 src);
 			if (err)
 				break;
 		}
@@ -1141,6 +1172,7 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
 					struct sk_buff *skb,
 					u16 vid)
 {
+	const unsigned char *src = eth_hdr(skb)->h_source;
 	struct icmp6hdr *icmp6h;
 	struct mld2_grec *grec;
 	int i;
@@ -1192,10 +1224,11 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
 		     grec->grec_type == MLD2_MODE_IS_INCLUDE) &&
 		    ntohs(*nsrcs) == 0) {
 			br_ip6_multicast_leave_group(br, port, &grec->grec_mca,
-						     vid);
+						     vid, src);
 		} else {
 			err = br_ip6_multicast_add_group(br, port,
-							 &grec->grec_mca, vid);
+							 &grec->grec_mca, vid,
+							 src);
 			if (err)
 				break;
 		}
@@ -1511,7 +1544,8 @@ br_multicast_leave_group(struct net_bridge *br,
 			 struct net_bridge_port *port,
 			 struct br_ip *group,
 			 struct bridge_mcast_other_query *other_query,
-			 struct bridge_mcast_own_query *own_query)
+			 struct bridge_mcast_own_query *own_query,
+			 const unsigned char *src)
 {
 	struct net_bridge_mdb_htable *mdb;
 	struct net_bridge_mdb_entry *mp;
@@ -1535,7 +1569,7 @@ br_multicast_leave_group(struct net_bridge *br,
 		for (pp = &mp->ports;
 		     (p = mlock_dereference(*pp, br)) != NULL;
 		     pp = &p->next) {
-			if (p->port != port)
+			if (!br_port_group_equal(p, port, src))
 				continue;
 
 			rcu_assign_pointer(*pp, p->next);
@@ -1566,7 +1600,7 @@ br_multicast_leave_group(struct net_bridge *br,
 		for (p = mlock_dereference(mp->ports, br);
 		     p != NULL;
 		     p = mlock_dereference(p->next, br)) {
-			if (p->port != port)
+			if (!br_port_group_equal(p, port, src))
 				continue;
 
 			if (!hlist_unhashed(&p->mglist) &&
@@ -1617,7 +1651,8 @@ br_multicast_leave_group(struct net_bridge *br,
 static void br_ip4_multicast_leave_group(struct net_bridge *br,
 					 struct net_bridge_port *port,
 					 __be32 group,
-					 __u16 vid)
+					 __u16 vid,
+					 const unsigned char *src)
 {
 	struct br_ip br_group;
 	struct bridge_mcast_own_query *own_query;
@@ -1632,14 +1667,15 @@ static void br_ip4_multicast_leave_group(struct net_bridge *br,
 	br_group.vid = vid;
 
 	br_multicast_leave_group(br, port, &br_group, &br->ip4_other_query,
-				 own_query);
+				 own_query, src);
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
 static void br_ip6_multicast_leave_group(struct net_bridge *br,
 					 struct net_bridge_port *port,
 					 const struct in6_addr *group,
-					 __u16 vid)
+					 __u16 vid,
+					 const unsigned char *src)
 {
 	struct br_ip br_group;
 	struct bridge_mcast_own_query *own_query;
@@ -1654,7 +1690,7 @@ static void br_ip6_multicast_leave_group(struct net_bridge *br,
 	br_group.vid = vid;
 
 	br_multicast_leave_group(br, port, &br_group, &br->ip6_other_query,
-				 own_query);
+				 own_query, src);
 }
 #endif
 
@@ -1711,6 +1747,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
 				 struct sk_buff *skb,
 				 u16 vid)
 {
+	const unsigned char *src;
 	struct sk_buff *skb_trimmed = NULL;
 	struct igmphdr *ih;
 	int err;
@@ -1731,13 +1768,14 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
 	}
 
 	ih = igmp_hdr(skb);
+	src = eth_hdr(skb)->h_source;
 	BR_INPUT_SKB_CB(skb)->igmp = ih->type;
 
 	switch (ih->type) {
 	case IGMP_HOST_MEMBERSHIP_REPORT:
 	case IGMPV2_HOST_MEMBERSHIP_REPORT:
 		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
-		err = br_ip4_multicast_add_group(br, port, ih->group, vid);
+		err = br_ip4_multicast_add_group(br, port, ih->group, vid, src);
 		break;
 	case IGMPV3_HOST_MEMBERSHIP_REPORT:
 		err = br_ip4_multicast_igmp3_report(br, port, skb_trimmed, vid);
@@ -1746,7 +1784,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
 		err = br_ip4_multicast_query(br, port, skb_trimmed, vid);
 		break;
 	case IGMP_HOST_LEAVE_MESSAGE:
-		br_ip4_multicast_leave_group(br, port, ih->group, vid);
+		br_ip4_multicast_leave_group(br, port, ih->group, vid, src);
 		break;
 	}
 
@@ -1765,6 +1803,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 				 struct sk_buff *skb,
 				 u16 vid)
 {
+	const unsigned char *src;
 	struct sk_buff *skb_trimmed = NULL;
 	struct mld_msg *mld;
 	int err;
@@ -1785,8 +1824,10 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 
 	switch (mld->mld_type) {
 	case ICMPV6_MGM_REPORT:
+		src = eth_hdr(skb)->h_source;
 		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
-		err = br_ip6_multicast_add_group(br, port, &mld->mld_mca, vid);
+		err = br_ip6_multicast_add_group(br, port, &mld->mld_mca, vid,
+						 src);
 		break;
 	case ICMPV6_MLD2_REPORT:
 		err = br_ip6_multicast_mld2_report(br, port, skb_trimmed, vid);
@@ -1795,7 +1836,8 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
 		err = br_ip6_multicast_query(br, port, skb_trimmed, vid);
 		break;
 	case ICMPV6_MGM_REDUCTION:
-		br_ip6_multicast_leave_group(br, port, &mld->mld_mca, vid);
+		src = eth_hdr(skb)->h_source;
+		br_ip6_multicast_leave_group(br, port, &mld->mld_mca, vid, src);
 		break;
 	}
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 8ce621e..cc55100 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -177,6 +177,8 @@ struct net_bridge_port_group {
 	struct timer_list		timer;
 	struct br_ip			addr;
 	unsigned char			flags;
+	unsigned char			eth_addr[ETH_ALEN];
+	bool				unicast;
 };
 
 struct net_bridge_mdb_entry
@@ -599,7 +601,7 @@ void br_multicast_free_pg(struct rcu_head *head);
 struct net_bridge_port_group *
 br_multicast_new_port_group(struct net_bridge_port *port, struct br_ip *group,
 			    struct net_bridge_port_group __rcu *next,
-			    unsigned char flags);
+			    unsigned char flags, const unsigned char *src);
 void br_mdb_init(void);
 void br_mdb_uninit(void);
 void br_mdb_notify(struct net_device *dev, struct net_bridge_port *port,
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 8bd5696..1730278 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -188,6 +188,7 @@ static BRPORT_ATTR(multicast_router, S_IRUGO | S_IWUSR, show_multicast_router,
 		   store_multicast_router);
 
 BRPORT_ATTR_FLAG(multicast_fast_leave, BR_MULTICAST_FAST_LEAVE);
+BRPORT_ATTR_FLAG(multicast_to_unicast, BR_MULTICAST_TO_UCAST);
 #endif
 
 static const struct brport_attribute *brport_attrs[] = {
@@ -214,6 +215,7 @@ static const struct brport_attribute *brport_attrs[] = {
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	&brport_attr_multicast_router,
 	&brport_attr_multicast_fast_leave,
+	&brport_attr_multicast_to_unicast,
 #endif
 	&brport_attr_proxyarp,
 	&brport_attr_proxyarp_wifi,
-- 
2.10.2

Re: [PATCH net-next] bridge: multicast to unicast

[Nikolay Aleksandrov]

On 02/01/17 20:32, Linus Lüssing wrote:
> Implements an optional, per bridge port flag and feature to deliver
> multicast packets to any host on the according port via unicast
> individually. This is done by copying the packet per host and
> changing the multicast destination MAC to a unicast one accordingly.
>
> multicast-to-unicast works on top of the multicast snooping feature of
> the bridge. Which means unicast copies are only delivered to hosts which
> are interested in it and signalized this via IGMP/MLD reports
> previously.
>
> This feature is intended for interface types which have a more reliable
> and/or efficient way to deliver unicast packets than broadcast ones
> (e.g. wifi).
>
> However, it should only be enabled on interfaces where no IGMPv2/MLDv1
> report suppression takes place. This feature is disabled by default.
>
> The initial patch and idea is from Felix Fietkau.
>
> Cc: Felix Fietkau <nbd@nbd.name>
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
>
> ---
>

Hi Linus,
A few comments below, in general I have 2 concerns: the new mcast fast-path
tests and cache line ref, and adding netlink support for the new flag.

> This feature is used and enabled by default in OpenWRT and LEDE for AP
> interfaces for more than a year now to allow both a more robust multicast
> delivery and multicast at higher rates (e.g. multicast streaming).
>
> In OpenWRT/LEDE the IGMP/MLD report suppression issue is overcome by
> the network daemon enabling AP isolation and by that separating all STAs.
> Delivery of STA-to-STA IP mulitcast is made possible again by
> enabling and utilizing the bridge hairpin mode, which considers the
> incoming port as a potential outgoing port, too.
>
> Hairpin-mode is performed after multicast snooping, therefore leading to
> only deliver reports to STAs running a multicast router.
> ---
>  include/linux/if_bridge.h |  1 +
>  net/bridge/br_forward.c   | 44 +++++++++++++++++++++--
>  net/bridge/br_mdb.c       |  2 +-
>  net/bridge/br_multicast.c | 92 ++++++++++++++++++++++++++++++++++-------------
>  net/bridge/br_private.h   |  4 ++-
>  net/bridge/br_sysfs_if.c  |  2 ++
>  6 files changed, 115 insertions(+), 30 deletions(-)
>
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index c6587c0..f1b0d78 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -46,6 +46,7 @@ struct br_ip_list {
>  #define BR_LEARNING_SYNC	BIT(9)
>  #define BR_PROXYARP_WIFI	BIT(10)
>  #define BR_MCAST_FLOOD		BIT(11)
> +#define BR_MULTICAST_TO_UCAST	BIT(12)
>
>  #define BR_DEFAULT_AGEING_TIME	(300 * HZ)
>
> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> index 7cb41ae..49d742d 100644
> --- a/net/bridge/br_forward.c
> +++ b/net/bridge/br_forward.c
> @@ -174,6 +174,33 @@ static struct net_bridge_port *maybe_deliver(
>  	return p;
>  }
>
> +static struct net_bridge_port *maybe_deliver_addr(
> +	struct net_bridge_port *prev, struct net_bridge_port *p,
> +	struct sk_buff *skb, const unsigned char *addr,
> +	bool local_orig)
> +{
> +	struct net_device *dev = BR_INPUT_SKB_CB(skb)->brdev;
> +	const unsigned char *src = eth_hdr(skb)->h_source;
> +
> +	if (!should_deliver(p, skb))
> +		return prev;
> +
> +	/* Even with hairpin, no soliloquies - prevent breaking IPv6 DAD */
> +	if (skb->dev == p->dev && ether_addr_equal(src, addr))
> +		return prev;
> +
> +	skb = skb_copy(skb, GFP_ATOMIC);
> +	if (!skb) {
> +		dev->stats.tx_dropped++;
> +		return prev;
> +	}
> +
> +	memcpy(eth_hdr(skb)->h_dest, addr, ETH_ALEN);
> +	__br_forward(p, skb, local_orig);
> +
> +	return prev;
> +}
> +
>  /* called under rcu_read_lock */
>  void br_flood(struct net_bridge *br, struct sk_buff *skb,
>  	      enum br_pkt_type pkt_type, bool local_rcv, bool local_orig)
> @@ -231,6 +258,7 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
>  	struct net_bridge_port *prev = NULL;
>  	struct net_bridge_port_group *p;
>  	struct hlist_node *rp;
> +	const unsigned char *addr;

nit: please arrange these into reverse christmas tree

>
>  	rp = rcu_dereference(hlist_first_rcu(&br->router_list));
>  	p = mdst ? rcu_dereference(mdst->ports) : NULL;
> @@ -241,10 +269,20 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
>  		rport = rp ? hlist_entry(rp, struct net_bridge_port, rlist) :
>  			     NULL;
>
> -		port = (unsigned long)lport > (unsigned long)rport ?
> -		       lport : rport;
> +		if ((unsigned long)lport > (unsigned long)rport) {
> +			port = lport;
> +			addr = p->unicast ? p->eth_addr : NULL;
> +		} else {
> +			port = rport;
> +			addr = NULL;
> +		}
> +
> +		if (addr)
> +			prev = maybe_deliver_addr(prev, port, skb, addr,
> +						  local_orig);
> +		else
> +			prev = maybe_deliver(prev, port, skb, local_orig);

This hunk adds 2 new tests and an additional cache line ref to all mcast forwarding,
regardless if the new (special case) flag is set or not.

Also are you intentionally sending the original skb through the last port ?

>
> -		prev = maybe_deliver(prev, port, skb, local_orig);
>  		if (IS_ERR(prev))
>  			goto out;
>  		if (prev == port)
> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index 7dbc80d..056e6ac 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -531,7 +531,7 @@ static int br_mdb_add_group(struct net_bridge *br, struct net_bridge_port *port,
>  			break;
>  	}
>
> -	p = br_multicast_new_port_group(port, group, *pp, state);
> +	p = br_multicast_new_port_group(port, group, *pp, state, NULL);
>  	if (unlikely(!p))
>  		return -ENOMEM;
>  	rcu_assign_pointer(*pp, p);
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index b30e77e..470a2409 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -43,12 +43,14 @@ static void br_multicast_add_router(struct net_bridge *br,
>  static void br_ip4_multicast_leave_group(struct net_bridge *br,
>  					 struct net_bridge_port *port,
>  					 __be32 group,
> -					 __u16 vid);
> +					 __u16 vid,
> +					 const unsigned char *src);
> +
>  #if IS_ENABLED(CONFIG_IPV6)
>  static void br_ip6_multicast_leave_group(struct net_bridge *br,
>  					 struct net_bridge_port *port,
>  					 const struct in6_addr *group,
> -					 __u16 vid);
> +					 __u16 vid, const unsigned char *src);
>  #endif
>  unsigned int br_mdb_rehash_seq;
>
> @@ -711,7 +713,8 @@ struct net_bridge_port_group *br_multicast_new_port_group(
>  			struct net_bridge_port *port,
>  			struct br_ip *group,
>  			struct net_bridge_port_group __rcu *next,
> -			unsigned char flags)
> +			unsigned char flags,
> +			const unsigned char *src)
>  {
>  	struct net_bridge_port_group *p;
>
> @@ -726,12 +729,35 @@ struct net_bridge_port_group *br_multicast_new_port_group(
>  	hlist_add_head(&p->mglist, &port->mglist);
>  	setup_timer(&p->timer, br_multicast_port_group_expired,
>  		    (unsigned long)p);
> +
> +	if ((port->flags & BR_MULTICAST_TO_UCAST) && src) {
> +		memcpy(p->eth_addr, src, ETH_ALEN);
> +		p->unicast = true;
> +	}
> +
>  	return p;
>  }
>
> +static bool br_port_group_equal(struct net_bridge_port_group *p,
> +				struct net_bridge_port *port,
> +				const unsigned char *src)
> +{
> +	if (p->port != port)
> +		return false;
> +
> +	if (!p->unicast)
> +		return true;
> +
> +	if (!src)
> +		return false;
> +
> +	return ether_addr_equal(src, p->eth_addr);
> +}
> +
>  static int br_multicast_add_group(struct net_bridge *br,
>  				  struct net_bridge_port *port,
> -				  struct br_ip *group)
> +				  struct br_ip *group,
> +				  const unsigned char *src)
>  {
>  	struct net_bridge_port_group __rcu **pp;
>  	struct net_bridge_port_group *p;
> @@ -758,13 +784,13 @@ static int br_multicast_add_group(struct net_bridge *br,
>  	for (pp = &mp->ports;
>  	     (p = mlock_dereference(*pp, br)) != NULL;
>  	     pp = &p->next) {
> -		if (p->port == port)
> +		if (br_port_group_equal(p, port, src))
>  			goto found;
>  		if ((unsigned long)p->port < (unsigned long)port)
>  			break;
>  	}
>
> -	p = br_multicast_new_port_group(port, group, *pp, 0);
> +	p = br_multicast_new_port_group(port, group, *pp, 0, src);
>  	if (unlikely(!p))
>  		goto err;
>  	rcu_assign_pointer(*pp, p);
> @@ -783,7 +809,8 @@ static int br_multicast_add_group(struct net_bridge *br,
>  static int br_ip4_multicast_add_group(struct net_bridge *br,
>  				      struct net_bridge_port *port,
>  				      __be32 group,
> -				      __u16 vid)
> +				      __u16 vid,
> +				      const unsigned char *src)
>  {
>  	struct br_ip br_group;
>
> @@ -794,14 +821,15 @@ static int br_ip4_multicast_add_group(struct net_bridge *br,
>  	br_group.proto = htons(ETH_P_IP);
>  	br_group.vid = vid;
>
> -	return br_multicast_add_group(br, port, &br_group);
> +	return br_multicast_add_group(br, port, &br_group, src);
>  }
>
>  #if IS_ENABLED(CONFIG_IPV6)
>  static int br_ip6_multicast_add_group(struct net_bridge *br,
>  				      struct net_bridge_port *port,
>  				      const struct in6_addr *group,
> -				      __u16 vid)
> +				      __u16 vid,
> +				      const unsigned char *src)
>  {
>  	struct br_ip br_group;
>
> @@ -812,7 +840,7 @@ static int br_ip6_multicast_add_group(struct net_bridge *br,
>  	br_group.proto = htons(ETH_P_IPV6);
>  	br_group.vid = vid;
>
> -	return br_multicast_add_group(br, port, &br_group);
> +	return br_multicast_add_group(br, port, &br_group, src);
>  }
>  #endif
>
> @@ -1081,6 +1109,7 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
>  					 struct sk_buff *skb,
>  					 u16 vid)
>  {
> +	const unsigned char *src;
>  	struct igmpv3_report *ih;
>  	struct igmpv3_grec *grec;
>  	int i;
> @@ -1121,12 +1150,14 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge *br,
>  			continue;
>  		}
>
> +		src = eth_hdr(skb)->h_source;
>  		if ((type == IGMPV3_CHANGE_TO_INCLUDE ||
>  		     type == IGMPV3_MODE_IS_INCLUDE) &&
>  		    ntohs(grec->grec_nsrcs) == 0) {
> -			br_ip4_multicast_leave_group(br, port, group, vid);
> +			br_ip4_multicast_leave_group(br, port, group, vid, src);
>  		} else {
> -			err = br_ip4_multicast_add_group(br, port, group, vid);
> +			err = br_ip4_multicast_add_group(br, port, group, vid,
> +							 src);
>  			if (err)
>  				break;
>  		}
> @@ -1141,6 +1172,7 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
>  					struct sk_buff *skb,
>  					u16 vid)
>  {
> +	const unsigned char *src = eth_hdr(skb)->h_source;
>  	struct icmp6hdr *icmp6h;
>  	struct mld2_grec *grec;
>  	int i;
> @@ -1192,10 +1224,11 @@ static int br_ip6_multicast_mld2_report(struct net_bridge *br,
>  		     grec->grec_type == MLD2_MODE_IS_INCLUDE) &&
>  		    ntohs(*nsrcs) == 0) {
>  			br_ip6_multicast_leave_group(br, port, &grec->grec_mca,
> -						     vid);
> +						     vid, src);
>  		} else {
>  			err = br_ip6_multicast_add_group(br, port,
> -							 &grec->grec_mca, vid);
> +							 &grec->grec_mca, vid,
> +							 src);
>  			if (err)
>  				break;
>  		}
> @@ -1511,7 +1544,8 @@ br_multicast_leave_group(struct net_bridge *br,
>  			 struct net_bridge_port *port,
>  			 struct br_ip *group,
>  			 struct bridge_mcast_other_query *other_query,
> -			 struct bridge_mcast_own_query *own_query)
> +			 struct bridge_mcast_own_query *own_query,
> +			 const unsigned char *src)
>  {
>  	struct net_bridge_mdb_htable *mdb;
>  	struct net_bridge_mdb_entry *mp;
> @@ -1535,7 +1569,7 @@ br_multicast_leave_group(struct net_bridge *br,
>  		for (pp = &mp->ports;
>  		     (p = mlock_dereference(*pp, br)) != NULL;
>  		     pp = &p->next) {
> -			if (p->port != port)
> +			if (!br_port_group_equal(p, port, src))
>  				continue;
>
>  			rcu_assign_pointer(*pp, p->next);
> @@ -1566,7 +1600,7 @@ br_multicast_leave_group(struct net_bridge *br,
>  		for (p = mlock_dereference(mp->ports, br);
>  		     p != NULL;
>  		     p = mlock_dereference(p->next, br)) {
> -			if (p->port != port)
> +			if (!br_port_group_equal(p, port, src))
>  				continue;
>
>  			if (!hlist_unhashed(&p->mglist) &&
> @@ -1617,7 +1651,8 @@ br_multicast_leave_group(struct net_bridge *br,
>  static void br_ip4_multicast_leave_group(struct net_bridge *br,
>  					 struct net_bridge_port *port,
>  					 __be32 group,
> -					 __u16 vid)
> +					 __u16 vid,
> +					 const unsigned char *src)
>  {
>  	struct br_ip br_group;
>  	struct bridge_mcast_own_query *own_query;
> @@ -1632,14 +1667,15 @@ static void br_ip4_multicast_leave_group(struct net_bridge *br,
>  	br_group.vid = vid;
>
>  	br_multicast_leave_group(br, port, &br_group, &br->ip4_other_query,
> -				 own_query);
> +				 own_query, src);
>  }
>
>  #if IS_ENABLED(CONFIG_IPV6)
>  static void br_ip6_multicast_leave_group(struct net_bridge *br,
>  					 struct net_bridge_port *port,
>  					 const struct in6_addr *group,
> -					 __u16 vid)
> +					 __u16 vid,
> +					 const unsigned char *src)
>  {
>  	struct br_ip br_group;
>  	struct bridge_mcast_own_query *own_query;
> @@ -1654,7 +1690,7 @@ static void br_ip6_multicast_leave_group(struct net_bridge *br,
>  	br_group.vid = vid;
>
>  	br_multicast_leave_group(br, port, &br_group, &br->ip6_other_query,
> -				 own_query);
> +				 own_query, src);
>  }
>  #endif
>
> @@ -1711,6 +1747,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
>  				 struct sk_buff *skb,
>  				 u16 vid)
>  {
> +	const unsigned char *src;

nit: please arrange these in reverse christmas tree

>  	struct sk_buff *skb_trimmed = NULL;
>  	struct igmphdr *ih;
>  	int err;
> @@ -1731,13 +1768,14 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
>  	}
>
>  	ih = igmp_hdr(skb);
> +	src = eth_hdr(skb)->h_source;
>  	BR_INPUT_SKB_CB(skb)->igmp = ih->type;
>
>  	switch (ih->type) {
>  	case IGMP_HOST_MEMBERSHIP_REPORT:
>  	case IGMPV2_HOST_MEMBERSHIP_REPORT:
>  		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
> -		err = br_ip4_multicast_add_group(br, port, ih->group, vid);
> +		err = br_ip4_multicast_add_group(br, port, ih->group, vid, src);
>  		break;
>  	case IGMPV3_HOST_MEMBERSHIP_REPORT:
>  		err = br_ip4_multicast_igmp3_report(br, port, skb_trimmed, vid);
> @@ -1746,7 +1784,7 @@ static int br_multicast_ipv4_rcv(struct net_bridge *br,
>  		err = br_ip4_multicast_query(br, port, skb_trimmed, vid);
>  		break;
>  	case IGMP_HOST_LEAVE_MESSAGE:
> -		br_ip4_multicast_leave_group(br, port, ih->group, vid);
> +		br_ip4_multicast_leave_group(br, port, ih->group, vid, src);
>  		break;
>  	}
>
> @@ -1765,6 +1803,7 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
>  				 struct sk_buff *skb,
>  				 u16 vid)
>  {
> +	const unsigned char *src;

nit: same about arrangement

>  	struct sk_buff *skb_trimmed = NULL;
>  	struct mld_msg *mld;
>  	int err;
> @@ -1785,8 +1824,10 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
>
>  	switch (mld->mld_type) {
>  	case ICMPV6_MGM_REPORT:
> +		src = eth_hdr(skb)->h_source;
>  		BR_INPUT_SKB_CB(skb)->mrouters_only = 1;
> -		err = br_ip6_multicast_add_group(br, port, &mld->mld_mca, vid);
> +		err = br_ip6_multicast_add_group(br, port, &mld->mld_mca, vid,
> +						 src);
>  		break;
>  	case ICMPV6_MLD2_REPORT:
>  		err = br_ip6_multicast_mld2_report(br, port, skb_trimmed, vid);
> @@ -1795,7 +1836,8 @@ static int br_multicast_ipv6_rcv(struct net_bridge *br,
>  		err = br_ip6_multicast_query(br, port, skb_trimmed, vid);
>  		break;
>  	case ICMPV6_MGM_REDUCTION:
> -		br_ip6_multicast_leave_group(br, port, &mld->mld_mca, vid);
> +		src = eth_hdr(skb)->h_source;
> +		br_ip6_multicast_leave_group(br, port, &mld->mld_mca, vid, src);
>  		break;
>  	}
>
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 8ce621e..cc55100 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -177,6 +177,8 @@ struct net_bridge_port_group {
>  	struct timer_list		timer;
>  	struct br_ip			addr;
>  	unsigned char			flags;
> +	unsigned char			eth_addr[ETH_ALEN];
> +	bool				unicast;

I think you can remove the boolean unicast here and either use the "flags" or
the eth_addr itself.
This structure needs a serious re-arrangement.

>  };
>
>  struct net_bridge_mdb_entry
> @@ -599,7 +601,7 @@ void br_multicast_free_pg(struct rcu_head *head);
>  struct net_bridge_port_group *
>  br_multicast_new_port_group(struct net_bridge_port *port, struct br_ip *group,
>  			    struct net_bridge_port_group __rcu *next,
> -			    unsigned char flags);
> +			    unsigned char flags, const unsigned char *src);
>  void br_mdb_init(void);
>  void br_mdb_uninit(void);
>  void br_mdb_notify(struct net_device *dev, struct net_bridge_port *port,
> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> index 8bd5696..1730278 100644
> --- a/net/bridge/br_sysfs_if.c
> +++ b/net/bridge/br_sysfs_if.c
> @@ -188,6 +188,7 @@ static BRPORT_ATTR(multicast_router, S_IRUGO | S_IWUSR, show_multicast_router,
>  		   store_multicast_router);
>
>  BRPORT_ATTR_FLAG(multicast_fast_leave, BR_MULTICAST_FAST_LEAVE);
> +BRPORT_ATTR_FLAG(multicast_to_unicast, BR_MULTICAST_TO_UCAST);
>  #endif
>
>  static const struct brport_attribute *brport_attrs[] = {
> @@ -214,6 +215,7 @@ static const struct brport_attribute *brport_attrs[] = {
>  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
>  	&brport_attr_multicast_router,
>  	&brport_attr_multicast_fast_leave,
> +	&brport_attr_multicast_to_unicast,
>  #endif
>  	&brport_attr_proxyarp,
>  	&brport_attr_proxyarp_wifi,
>

Please also add netlink support, we've been working hard at adding support for all bridge
options via netlink.

Thanks,
  Nik

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-02 20:32, Linus Lüssing wrote:
> Implements an optional, per bridge port flag and feature to deliver
> multicast packets to any host on the according port via unicast
> individually. This is done by copying the packet per host and
> changing the multicast destination MAC to a unicast one accordingly.
> 
> multicast-to-unicast works on top of the multicast snooping feature of
> the bridge. Which means unicast copies are only delivered to hosts which
> are interested in it and signalized this via IGMP/MLD reports
> previously.
> 
> This feature is intended for interface types which have a more reliable
> and/or efficient way to deliver unicast packets than broadcast ones
> (e.g. wifi).
> 
> However, it should only be enabled on interfaces where no IGMPv2/MLDv1
> report suppression takes place. This feature is disabled by default.
> 
> The initial patch and idea is from Felix Fietkau.
> 
> Cc: Felix Fietkau <nbd@nbd.name>
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
Please add Signed-off-by: Felix Fietkau <nbd@nbd.name>
in the next version, and maybe also From:

Thanks,

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Johannes Berg]

On Mon, 2017-01-02 at 20:32 +0100, Linus Lüssing wrote:
> Implements an optional, per bridge port flag and feature to deliver
> multicast packets to any host on the according port via unicast
> individually. This is done by copying the packet per host and
> changing the multicast destination MAC to a unicast one accordingly.

How does this compare and/or relate to the multicast-to-unicast feature
we were going to add to the wifi stack, particularly mac80211? Do we
perhaps not need that feature at all, if bridging will have it?

I suppose that the feature there could apply also to locally generated
traffic when the AP interface isn't in a bridge, but I think I could
live with requiring the AP to be put into a bridge to achieve a similar
configuration?

Additionally, on an unrelated note, this seems to apply generically to
all kinds of frames, losing information by replacing the address.
Shouldn't it have similar limitations as the wifi stack feature has
then, like only applying to ARP, IPv4, IPv6 and not general protocols?

Also, it should probably come with the same caveat as we documented for
the wifi feature:

    Note that this may break certain expectations of the receiver,
    such as the ability to drop unicast IP packets received within
    multicast L2 frames, or the ability to not send ICMP destination
    unreachable messages for packets received in L2 multicast (which
    is required, but the receiver can't tell the difference if this
    new option is enabled.)


I'll hold off sending my tree in until we see that we really need both
features, or decide that we want the wifi feature *instead* of the
bridge feature.

johannes

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-06 13:47, Johannes Berg wrote:
> On Mon, 2017-01-02 at 20:32 +0100, Linus Lüssing wrote:
>> Implements an optional, per bridge port flag and feature to deliver
>> multicast packets to any host on the according port via unicast
>> individually. This is done by copying the packet per host and
>> changing the multicast destination MAC to a unicast one accordingly.
> 
> How does this compare and/or relate to the multicast-to-unicast feature
> we were going to add to the wifi stack, particularly mac80211? Do we
> perhaps not need that feature at all, if bridging will have it?
> 
> I suppose that the feature there could apply also to locally generated
> traffic when the AP interface isn't in a bridge, but I think I could
> live with requiring the AP to be put into a bridge to achieve a similar
> configuration?
> 
> Additionally, on an unrelated note, this seems to apply generically to
> all kinds of frames, losing information by replacing the address.
> Shouldn't it have similar limitations as the wifi stack feature has
> then, like only applying to ARP, IPv4, IPv6 and not general protocols?
> 
> Also, it should probably come with the same caveat as we documented for
> the wifi feature:
> 
>     Note that this may break certain expectations of the receiver,
>     such as the ability to drop unicast IP packets received within
>     multicast L2 frames, or the ability to not send ICMP destination
>     unreachable messages for packets received in L2 multicast (which
>     is required, but the receiver can't tell the difference if this
>     new option is enabled.)
> 
> 
> I'll hold off sending my tree in until we see that we really need both
> features, or decide that we want the wifi feature *instead* of the
> bridge feature.
The bridge layer can use IGMP snooping to ensure that the multicast
stream is only transmitted to clients that are actually a member of the
group. Can the mac80211 feature do the same?

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Johannes Berg]

> The bridge layer can use IGMP snooping to ensure that the multicast
> stream is only transmitted to clients that are actually a member of
> the group. Can the mac80211 feature do the same?

No, it'll convert the packet for all clients that are behind that
netdev. But that's an argument for dropping the mac80211 feature, which
hasn't been merged upstream yet, no?

johannes

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-06 14:54, Johannes Berg wrote:
> 
>> The bridge layer can use IGMP snooping to ensure that the multicast
>> stream is only transmitted to clients that are actually a member of
>> the group. Can the mac80211 feature do the same?
> 
> No, it'll convert the packet for all clients that are behind that
> netdev. But that's an argument for dropping the mac80211 feature, which
> hasn't been merged upstream yet, no?
Right.

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Stephen Hemminger]

On Mon,  2 Jan 2017 20:32:14 +0100
Linus Lüssing <linus.luessing@c0d3.blue> wrote:

> This feature is intended for interface types which have a more reliable
> and/or efficient way to deliver unicast packets than broadcast ones
> (e.g. wifi).


Why is this not done in MAC80211 rather than  bridge?

Re: [PATCH net-next] bridge: multicast to unicast

[M. Braun]

Am 06.01.2017 um 14:54 schrieb Johannes Berg:
> 
>> The bridge layer can use IGMP snooping to ensure that the multicast
>> stream is only transmitted to clients that are actually a member of
>> the group. Can the mac80211 feature do the same?
> 
> No, it'll convert the packet for all clients that are behind that
> netdev. But that's an argument for dropping the mac80211 feature, which
> hasn't been merged upstream yet, no?

But there is multicast/broadcast traffic like e.g. ARP and some IP
multicast groups that are not covered by IGMP snooping. The mac80211
patch converts this to unicast as well, which the bridge cannot do.

That way, these features both complement and overlap each other.

Regards,
Michael

Re: [PATCH net-next] bridge: multicast to unicast

[Linus Lüssing]

On Sat, Jan 07, 2017 at 11:32:57AM +0100, M. Braun wrote:
> Am 06.01.2017 um 14:54 schrieb Johannes Berg:
> > 
> >> The bridge layer can use IGMP snooping to ensure that the multicast
> >> stream is only transmitted to clients that are actually a member of
> >> the group. Can the mac80211 feature do the same?
> > 
> > No, it'll convert the packet for all clients that are behind that
> > netdev. But that's an argument for dropping the mac80211 feature, which
> > hasn't been merged upstream yet, no?
> 
> But there is multicast/broadcast traffic like e.g. ARP and some IP
> multicast groups that are not covered by IGMP snooping. The mac80211
> patch converts this to unicast as well, which the bridge cannot do.
> 
> That way, these features both complement and overlap each other.

Right, I'd agree with that.

I didn't write it explicitly in the commit message, but yes, the
like anything concerning bridge multicast snooping, bridge
multicast-to-unicast can only affect packets as noted in
RFC4541 ("Considerations for Internet Group Management Protocol (IGMP)
and Multicast Listener Discovery (MLD) Snooping Switches"), too.

So it is only working for IPv4 multicast, excluding link-local
(224.0.0.0/24), and IPv6 multicast, excluding all-host-multicast
(ff02::1).

And does not concern ARP in any way.


The nice complementary effect is, that the bridge can first sieve
out those IP packets thanks to IGMP/MLD snooping knowledge and for
anything else, like ARP, 224.0.0.x or ff02::1, the mac80211
multicast-to-unicast could do its job.


For APs with a small number of STAs (like your private home AP),
you might want to enable both bridge multicast-to-unicast and
mac80211 multicast-to-unicast for this complementary effect. While
on public APs with 30 to 50 STAs with varying distances and bitrates,
you might only one to enable the bridge one, because sending an ARP
packet 50x might actually reduce performance and airtime
significantly.

Re: [PATCH net-next] bridge: multicast to unicast

[Linus Lüssing]

On Fri, Jan 06, 2017 at 07:13:56PM -0800, Stephen Hemminger wrote:
> On Mon,  2 Jan 2017 20:32:14 +0100
> Linus Lüssing <linus.luessing@c0d3.blue> wrote:
> 
> > This feature is intended for interface types which have a more reliable
> > and/or efficient way to deliver unicast packets than broadcast ones
> > (e.g. wifi).
> 
> 
> Why is this not done in MAC80211 rather than  bridge?

Because mac80211 does not have the IGMP/MLD snooping code as
the bridge has?

Reimplementing the snooping in mac80211 does not make sense
because of duplicating code. Moving the snooping code from the
bridge to mac80211 does not make sense either, because we want
multicast snooping, software based, (virtually) wired switches,
too.

The "best way" (TM) would probably be to migrate the IGMP/MLD
snooping from the bridge code to the net device code on the long
run to make such a database usable for any kind of device, without
needing this bridge hack.

But such a migration would also need a way more invasive patchset.

While Felix's idea might look a little "ugly" due it's hacky
nature, I think it is also quite beautiful thanks to it's
simplicity.

Re: [PATCH net-next] bridge: multicast to unicast

[Linus Lüssing]

On Fri, Jan 06, 2017 at 01:47:52PM +0100, Johannes Berg wrote:
> How does this compare and/or relate to the multicast-to-unicast feature
> we were going to add to the wifi stack, particularly mac80211? Do we
> perhaps not need that feature at all, if bridging will have it?
> 
> I suppose that the feature there could apply also to locally generated
> traffic when the AP interface isn't in a bridge, but I think I could
> live with requiring the AP to be put into a bridge to achieve a similar
> configuration?
> 
> Additionally, on an unrelated note, this seems to apply generically to
> all kinds of frames, losing information by replacing the address.
> Shouldn't it have similar limitations as the wifi stack feature has
> then, like only applying to ARP, IPv4, IPv6 and not general protocols?

(should all three be answered with Michael's and my reply to
Michael's mail, I think)

> 
> Also, it should probably come with the same caveat as we documented for
> the wifi feature:
> 
>     Note that this may break certain expectations of the receiver,
>     such as the ability to drop unicast IP packets received within
>     multicast L2 frames, or the ability to not send ICMP destination
>     unreachable messages for packets received in L2 multicast (which
>     is required, but the receiver can't tell the difference if this
>     new option is enabled.)

Actually, I do not quite understand that remark in the mac80211
multicast-to-unicast patch. IP should not care about the ethernet
header?

Re: [PATCH net-next] bridge: multicast to unicast

[Johannes Berg]

On Sat, 2017-01-07 at 16:15 +0100, Linus Lüssing wrote:

> Actually, I do not quite understand that remark in the mac80211
> multicast-to-unicast patch. IP should not care about the ethernet
> header?

But it does, for example RFC 1122 states:

         When a host sends a datagram to a link-layer broadcast address,
         the IP destination address MUST be a legal IP broadcast or IP
         multicast address.

         A host SHOULD silently discard a datagram that is received via
         a link-layer broadcast (see Section 2.4) but does not specify
         an IP multicast or broadcast destination address.

You can probably find other examples too.

johannes

Re: [PATCH net-next] bridge: multicast to unicast

[Johannes Berg]

On Sat, 2017-01-07 at 15:55 +0100, Linus Lüssing wrote:
> On Sat, Jan 07, 2017 at 11:32:57AM +0100, M. Braun wrote:
> > Am 06.01.2017 um 14:54 schrieb Johannes Berg:
> > > 
> > > > The bridge layer can use IGMP snooping to ensure that the
> > > > multicast
> > > > stream is only transmitted to clients that are actually a
> > > > member of
> > > > the group. Can the mac80211 feature do the same?
> > > 
> > > No, it'll convert the packet for all clients that are behind that
> > > netdev. But that's an argument for dropping the mac80211 feature,
> > > which
> > > hasn't been merged upstream yet, no?
> > 
> > But there is multicast/broadcast traffic like e.g. ARP and some IP
> > multicast groups that are not covered by IGMP snooping. The
> > mac80211
> > patch converts this to unicast as well, which the bridge cannot do.
> > 
> > That way, these features both complement and overlap each other.
> 
> Right, I'd agree with that.

Ok.

> I didn't write it explicitly in the commit message, but yes, the
> like anything concerning bridge multicast snooping, bridge
> multicast-to-unicast can only affect packets as noted in
> RFC4541 ("Considerations for Internet Group Management Protocol (IGMP)
> and Multicast Listener Discovery (MLD) Snooping Switches"), too.
> 
> So it is only working for IPv4 multicast, excluding link-local
> (224.0.0.0/24), and IPv6 multicast, excluding all-host-multicast
> (ff02::1).
> 
> And does not concern ARP in any way.
> 
> 
> The nice complementary effect is, that the bridge can first sieve
> out those IP packets thanks to IGMP/MLD snooping knowledge and for
> anything else, like ARP, 224.0.0.x or ff02::1, the mac80211
> multicast-to-unicast could do its job.
> 
> 
> For APs with a small number of STAs (like your private home AP),
> you might want to enable both bridge multicast-to-unicast and
> mac80211 multicast-to-unicast for this complementary effect. While
> on public APs with 30 to 50 STAs with varying distances and bitrates,
> you might only one to enable the bridge one, because sending an ARP
> packet 50x might actually reduce performance and airtime
> significantly.

Does it make sense to implement the two in separate layers though?

Clearly, this part needs to be implemented in the bridge layer due to
the snooping knowledge, but the code is very similar to what mac80211
has now.

It would probably not make sense to combine the two options into one,
but it seems relatively simple for bridge to also implement the one
mac80211 tentatively has now, with multiple benefits:

 * single place for configuration, leading to less possible confusion

 * single implementation for all wireless devices, including ones with
   Full-MAC firmware that don't use mac80211

 * code sharing for the duplication, although admittedly not so much

Thoughts?

johannes

RE: [PATCH net-next] bridge: multicast to unicast

[Jean-Pierre Tosoni]

> -----Message d'origine-----
> De : linux-wireless-owner@vger.kernel.org [mailto:linux-wireless-
> owner@vger.kernel.org] De la part de Stephen Hemminger
> Envoyé : samedi 7 janvier 2017 04:14
> À : Linus Lüssing
> Cc : netdev@vger.kernel.org; David S . Miller; bridge@lists.linux-
> foundation.org; linux-kernel@vger.kernel.org; linux-
> wireless@vger.kernel.org; Felix Fietkau
> Objet : Re: [PATCH net-next] bridge: multicast to unicast
> 
> On Mon,  2 Jan 2017 20:32:14 +0100
> Linus Lüssing <linus.luessing@c0d3.blue> wrote:
> 
> > This feature is intended for interface types which have a more
> > reliable and/or efficient way to deliver unicast packets than
> > broadcast ones (e.g. wifi).
> 
> 
> Why is this not done in MAC80211 rather than  bridge?

OTOH mac80211 has more information to decide whether it is more economic to send one multicast or several unicast.
It depends on the bitrate of each station, number of stations and the (not necessarily slower) bitrate of multicasts.

Re: [PATCH net-next] bridge: multicast to unicast

[M. Braun]

Am 09.01.2017 um 09:08 schrieb Johannes Berg:
> Does it make sense to implement the two in separate layers though?
> 
> Clearly, this part needs to be implemented in the bridge layer due to
> the snooping knowledge, but the code is very similar to what mac80211
> has now.

Does the bridge always know about all stations connected?

That is bridge fdb entries (need to) expire so the bridge might "forget"
a still-connected station not sending but only consuming broadcast traffic.

E.g. there is a television broadcast station here that receives a video
stream (via wifi, udp packets) and then airs it (dvb-t) but (on its own)
would not send any data packet on wifi (static ip, etc.).

An other reason to implement this in mac80211 initially was that
mac80211 could encapsulate broacast/multicast ethernet packtes in
unicast A-MSDU packets in a way, so that the receiver would still see
process ethernet packets (after conversion) but have unicast wifi
frames. This cannot be done in bridge easily but one might want to add
this later to mac80211.

Michael

Re: [PATCH net-next] bridge: multicast to unicast

[Johannes Berg]

On Mon, 2017-01-09 at 12:44 +0100, M. Braun wrote:
> Am 09.01.2017 um 09:08 schrieb Johannes Berg:
> > Does it make sense to implement the two in separate layers though?
> > 
> > Clearly, this part needs to be implemented in the bridge layer due
> > to
> > the snooping knowledge, but the code is very similar to what
> > mac80211
> > has now.
> 
> Does the bridge always know about all stations connected?
> 
> That is bridge fdb entries (need to) expire so the bridge might
> "forget" a still-connected station not sending but only consuming
> broadcast traffic.
> 
> E.g. there is a television broadcast station here that receives a
> video stream (via wifi, udp packets) and then airs it (dvb-t) but (on
> its own) would not send any data packet on wifi (static ip, etc.).

Ok, that I don't know. Somehow if you address a unicast packet there
the bridge has to make a decision - so it really should know? Would it
query the port somehow to see if the device is behind it, if getting a
packet for a station it forgot about?

> An other reason to implement this in mac80211 initially was that
> mac80211 could encapsulate broacast/multicast ethernet packtes in
> unicast A-MSDU packets in a way, so that the receiver would still see
> process ethernet packets (after conversion) but have unicast wifi
> frames. This cannot be done in bridge easily but one might want to
> add this later to mac80211.

Yes, DMG would have to be done in mac80211, but that's a lot clearer
case too since it requires negotiation functionality etc.

johannes

Re: [PATCH net-next] bridge: multicast to unicast

[Linus Lüssing]

On Mon, Jan 09, 2017 at 09:05:49AM +0100, Johannes Berg wrote:
> On Sat, 2017-01-07 at 16:15 +0100, Linus Lüssing wrote:
> 
> > Actually, I do not quite understand that remark in the mac80211
> > multicast-to-unicast patch. IP should not care about the ethernet
> > header?
> 
> But it does, for example RFC 1122 states:
> 
>          When a host sends a datagram to a link-layer broadcast address,
>          the IP destination address MUST be a legal IP broadcast or IP
>          multicast address.
> 
>          A host SHOULD silently discard a datagram that is received via
>          a link-layer broadcast (see Section 2.4) but does not specify
>          an IP multicast or broadcast destination address.

This example is the other way round. It specifies how the IP
destination should look like in case of link-layer broadcast. Not
how the link-layer destination should look like in case of a
multicast/broadcast IP destination.

Any other examples?

Re: [PATCH net-next] bridge: multicast to unicast

[Johannes Berg]

> >          A host SHOULD silently discard a datagram that is received via
> >          a link-layer broadcast (see Section 2.4) but does not specify
> >          an IP multicast or broadcast destination address.
> 
> This example is the other way round. It specifies how the IP
> destination should look like in case of link-layer broadcast. Not
> how the link-layer destination should look like in case of a
> multicast/broadcast IP destination.

You stopped reading too early - snipped the context part for you :)

johannes

Re: [PATCH net-next] bridge: multicast to unicast

[michael-dev]

Am 09.01.2017 13:15, schrieb Johannes Berg:
>> That is bridge fdb entries (need to) expire so the bridge might
>> "forget" a still-connected station not sending but only consuming
>> broadcast traffic.
> 
> Ok, that I don't know. Somehow if you address a unicast packet there
> the bridge has to make a decision - so it really should know?

If the bridge has not learned the unicast destination mac address on any 
port, it will flood the packet on all ports except the port it received 
the packet on.

Regards,
M. Braun

Re: [PATCH net-next] bridge: multicast to unicast

[Johannes Berg]

On Mon, 2017-01-09 at 16:25 +0100, michael-dev wrote:
> Am 09.01.2017 13:15, schrieb Johannes Berg:
> > > That is bridge fdb entries (need to) expire so the bridge might
> > > "forget" a still-connected station not sending but only consuming
> > > broadcast traffic.
> > 
> > Ok, that I don't know. Somehow if you address a unicast packet
> > there
> > the bridge has to make a decision - so it really should know?
> 
> If the bridge has not learned the unicast destination mac address on
> any port, it will flood the packet on all ports except the port it
> received the packet on.

Ok, so this really needs to be done in mac80211.

I'll send out the pull request soon then.

johannes

Re: [PATCH net-next] bridge: multicast to unicast

[Linus Lüssing]

On Mon, Jan 09, 2017 at 12:44:19PM +0100, M. Braun wrote:
> Am 09.01.2017 um 09:08 schrieb Johannes Berg:
> > Does it make sense to implement the two in separate layers though?
> > 
> > Clearly, this part needs to be implemented in the bridge layer due to
> > the snooping knowledge, but the code is very similar to what mac80211
> > has now.
> 
> Does the bridge always know about all stations connected?

The bridge does not always know about all stations, especially the
silent ones like in your DVB-T example.

However, concerning IP multicast, there is IGMP/MLD. So the bridge
does know about all stations which are interested in a specific IP
multicast stream.

(As long as there is a querier on the link, which periodically
queriers for IGMP/MLD reports from any listener. If there is no
querier then the bridge multicast snooping, including the bridge
multicast-to-unicast will fall back to flooding)


So if your television example uses IP multicast properly, it is
completely doable with the bridge multicast-to-unicast, thanks to
IGMP/MLD.

Re: [PATCH net-next] bridge: multicast to unicast

[Stephen Hemminger]

On Mon, 9 Jan 2017 22:23:45 +0100
Linus Lüssing <linus.luessing@c0d3.blue> wrote:

> On Mon, Jan 09, 2017 at 12:44:19PM +0100, M. Braun wrote:
> > Am 09.01.2017 um 09:08 schrieb Johannes Berg:  
> > > Does it make sense to implement the two in separate layers though?
> > > 
> > > Clearly, this part needs to be implemented in the bridge layer due to
> > > the snooping knowledge, but the code is very similar to what mac80211
> > > has now.  
> > 
> > Does the bridge always know about all stations connected?  
> 
> The bridge does not always know about all stations, especially the
> silent ones like in your DVB-T example.
> 
> However, concerning IP multicast, there is IGMP/MLD. So the bridge
> does know about all stations which are interested in a specific IP
> multicast stream.
> 
> (As long as there is a querier on the link, which periodically
> queriers for IGMP/MLD reports from any listener. If there is no
> querier then the bridge multicast snooping, including the bridge
> multicast-to-unicast will fall back to flooding)
> 
> 
> So if your television example uses IP multicast properly, it is
> completely doable with the bridge multicast-to-unicast, thanks to
> IGMP/MLD.

I wonder if MAC80211 should be doing IGMP snooping and not bridge
in this environment.

Re: [PATCH net-next] bridge: multicast to unicast

[Linus Lüssing]

On Mon, Jan 09, 2017 at 10:42:46PM +0100, Johannes Berg wrote:
> On Mon, 2017-01-09 at 22:33 +0100, Linus Lüssing wrote:
> > On Mon, Jan 09, 2017 at 01:44:03PM +0100, Johannes Berg wrote:
> > > 
> > > > >          A host SHOULD silently discard a datagram that is
> > > > > received via
> > > > >          a link-layer broadcast (see Section 2.4) but does not
> > > > > specify
> > > > >          an IP multicast or broadcast destination address.
> > > > 
> > > > This example is the other way round. It specifies how the IP
> > > > destination should look like in case of link-layer broadcast. Not
> > > > how the link-layer destination should look like in case of a
> > > > multicast/broadcast IP destination.
> > > 
> > > You stopped reading too early - snipped the context part for you :)
> > 
> > Sorry for writing to you directly, but I still have some
> > difficulties. In pseudo-code that line says:
> > 
> > -----
> > if ll_dst(pkt) == bcast AND ip_dst(pkt) != mcast/bcast:
> > -> drop(pkt)
> > -----
> > 
> > But after multicast-to-unicast conversion, we have:
> > 
> > -----
> > ll_dst(pkt) == ucast AND ip_dst(pkt) == mcast
> > -----
> > 
> > So none of the two requirements for dropping are matched?
> > 
> 
> Exactly. My point is that this is breaking the expectation that hosts
> are actually able to drop such packets.

[readding CCs I removed earlier]

Ah! Thanks. I was worried about creating packetloss :D.

Hm, for this other other way round, I think it does not apply for
the bridge multicast-to-unicast patch if I'm not misreading the bridge code:

For a packet with a link-layer multicast address but a unicast IP
destination, the bridge MDB lookup will fail.
(http://lxr.free-electrons.com/source/net/bridge/br_multicast.c?v=4.8#L178
 returns NULL)

Case A): No multicast router on port:
-> bridge, br_multicast_flood(), will drop the packet already
   (no matter if multicast-to-unicast is enabled or not)

Case B): Multicast router present on port:
-> The new patch does not apply multicast-to-unicast but just floods
   packet unaltered
   ("else { port = rport; addr = NULL; }" branch)

Regards, Linus

Re: [PATCH net-next] bridge: multicast to unicast

[Linus Lüssing]

On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
> I wonder if MAC80211 should be doing IGMP snooping and not bridge
> in this environment.

In the long term, yes. For now, not quite sure.

I personally like to go for simple solutions first :).

Re: [PATCH net-next] bridge: multicast to unicast

[Johannes Berg]

On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote:
> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
> > I wonder if MAC80211 should be doing IGMP snooping and not bridge
> > in this environment.
> 
> In the long term, yes. For now, not quite sure.

There's no "for now" in the kernel. Code added now will have to be
maintained essentially forever.

johannes

Re: [PATCH net-next] bridge: multicast to unicast

[Dave Taht]

In the case of wifi I have 3 issues with this line of thought.

multicast in wifi has generally supposed to be unreliable. This makes
it reliable. reliability comes at a cost -

multicast is typically set at a fixed low rate today. unicast is
retried at different rates until it succeeds - for every station
listening. If one station is already at the lowest rate, the total
cost of the transmit increases, rather than decreases.

unicast gets block acks until it succeeds. Again, more delay.

I think there is something like 31 soft-retries in the ath9k driver....

what happens to diffserv markings here? for unicast CS1 goes into the
BE queue, CS6, the VO queue. Do we go from one flat queue for all of
multicast to punching it through one of the hardware queues based on
the diffserv mark now with this patch?

I would like it if there was a way to preserve the unreliability
(which multiple mesh protocols depend on), send stuff with QoSNoack,
etc - or dynamically choose (based on the rates of the stations)
between conventional multicast and unicast.

Or - better, IMHO, keep sending multicast as is but pick the best of
the rates available to all the listening stations for it.

Has anyone actually looked at the effects of this with, say, 5-10
stations at middlin to poor quality (longer distance)? using something
to measure the real effect of the multicast conversion? (uftp, mdns?)

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-10 18:17, Dave Taht wrote:
> In the case of wifi I have 3 issues with this line of thought.
> 
> multicast in wifi has generally supposed to be unreliable. This makes
> it reliable. reliability comes at a cost -
> 
> multicast is typically set at a fixed low rate today. unicast is
> retried at different rates until it succeeds - for every station
> listening. If one station is already at the lowest rate, the total
> cost of the transmit increases, rather than decreases.
> 
> unicast gets block acks until it succeeds. Again, more delay.
> 
> I think there is something like 31 soft-retries in the ath9k driver....
If I remember correctly, hardware retries are counted here as well.

> what happens to diffserv markings here? for unicast CS1 goes into the
> BE queue, CS6, the VO queue. Do we go from one flat queue for all of
> multicast to punching it through one of the hardware queues based on
> the diffserv mark now with this patch?
> 
> I would like it if there was a way to preserve the unreliability
> (which multiple mesh protocols depend on), send stuff with QoSNoack,
> etc - or dynamically choose (based on the rates of the stations)
> between conventional multicast and unicast.
> 
> Or - better, IMHO, keep sending multicast as is but pick the best of
> the rates available to all the listening stations for it.
The advantage of the multicast-to-unicast conversion goes beyond simply
selecting a better rate - aggregation matters a lot as well, and that is
simply incompatible with normal multicast.

Some multicast streams use lots of small-ish packets, the airtime impact
of those is vastly reduced, even if the transmission has to be
duplicated for a few stations.

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Dave Taht]

On Tue, Jan 10, 2017 at 9:23 AM, Felix Fietkau <nbd@nbd.name> wrote:
> On 2017-01-10 18:17, Dave Taht wrote:
>> In the case of wifi I have 3 issues with this line of thought.
>>
>> multicast in wifi has generally supposed to be unreliable. This makes
>> it reliable. reliability comes at a cost -
>>
>> multicast is typically set at a fixed low rate today. unicast is
>> retried at different rates until it succeeds - for every station
>> listening. If one station is already at the lowest rate, the total
>> cost of the transmit increases, rather than decreases.
>>
>> unicast gets block acks until it succeeds. Again, more delay.
>>
>> I think there is something like 31 soft-retries in the ath9k driver....
> If I remember correctly, hardware retries are counted here as well.

I chopped this to something more reasonable but never got around to
quantifying it, so never pushed the patch. I figured I'd measure ATF
in a noisy environment (which I'd be doing now if it weren't for
https://bugs.lede-project.org/index.php?do=details&task_id=368 )
first.

>> what happens to diffserv markings here? for unicast CS1 goes into the
>> BE queue, CS6, the VO queue. Do we go from one flat queue for all of
>> multicast to punching it through one of the hardware queues based on
>> the diffserv mark now with this patch?

I meant CS1=BK here. Tracing the path through the bridge code made my
head hurt, I can go look at some aircaps to see if the mcast->unicast
conversion respects those markings or not (my vote is *not*).

>> I would like it if there was a way to preserve the unreliability
>> (which multiple mesh protocols depend on), send stuff with QoSNoack,
>> etc - or dynamically choose (based on the rates of the stations)
>> between conventional multicast and unicast.
>>
>> Or - better, IMHO, keep sending multicast as is but pick the best of
>> the rates available to all the listening stations for it.

> The advantage of the multicast-to-unicast conversion goes beyond simply
> selecting a better rate - aggregation matters a lot as well, and that is
> simply incompatible with normal multicast.

Except for the VO queue which cannot aggregate. And for that matter,
using any other hardware queue than BE tends to eat a txop that would
otherwise possibly be combined with an aggregate.

(and the VI queue has always misbehaved, long on my todo list)

> Some multicast streams use lots of small-ish packets, the airtime impact
> of those is vastly reduced, even if the transmission has to be
> duplicated for a few stations.

The question was basically how far up does it scale. Arguably, for a
very few, well connected stations, this patch would help. For a
network with more - and more badly connected stations, I think it
would hurt.

What sorts of multicast traffic are being observed that flood the
network sufficiently to be worth optimizing out? arp? nd? upnp? mdns?
uftp? tv?

(my questions above are related to basically trying to setup a sane
a/b test, I've been building up a new testbed in noisy environment to
match the one I have in a quiet one, and don't have any "good" mcast
tests defined. Has anyone done an a/b test of this code with some
repeatable test already?)

(In my observations... The only truly heavy creator of a multicast
"burp" has tended to be upnp and mdns on smaller networks. Things like
nd and arp get more problematic as the number of stations go up also.
I can try things like abusing vlc or uftp to see what happens?)

I certainly agree multicast is a "problem" (I've seen 20-80% or more
of a given wifi network eaten by multicast) but I'm not convinced that
making it reliable, aggregatable unicast scales much past
basement-level testing of a few "good" stations, and don't know which
protocols are making it worse, the worst, in typical environments.
Certainly apple gear puts out a lot of multicast.

...

As best as I recall a recommendation in the 802.11-2012 standard was
that multicast packets be rate-limited so that you'd have a fixed
amount of crap after each beacon sufficient to keep the rest of the
unicast traffic flowing rapidly, instead of dumping everything into a
given beacon transmit.

That, combined with (maybe) picking the "best" union of known rates
per station, was essentially the strategy I'd intended[1] to pursue
for tackling the currently infinite wifi multicast queue - fq the
entries, have a fairly short queue (codel is not the best choice here)
drop from head, and limit the number of packets transmitted per beacon
to spread them out. That would solve the issue for sparse multicast
(dhcp etc), and smooth out the burps from bigger chunks while
impacting conventional unicast minimally.

There's also the pursuit of less multicast overall at least in some protocols

https://tools.ietf.org/html/draft-ietf-dnssd-hybrid-05


>
> - Felix


[1] but make-wifi-fast has been out of funding since august

-- 
Dave Täht
Let's go make home routers and wifi faster! With better software!
http://blog.cerowrt.org

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-10 11:56, Johannes Berg wrote:
> On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote:
>> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
>> > I wonder if MAC80211 should be doing IGMP snooping and not bridge
>> > in this environment.
>> 
>> In the long term, yes. For now, not quite sure.
> 
> There's no "for now" in the kernel. Code added now will have to be
> maintained essentially forever.
I'm not sure that putting the IGMP snooping code in mac80211 is a good
idea, that would be quite a bit of code duplication.
This implementation works, it's very simple, and it's quite flexible for
a number of use cases.

Is there any remaining objection to merging this in principle (aside
from potential issues with the code)?

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Johannes Berg]

> > Exactly. My point is that this is breaking the expectation that
> > hosts are actually able to drop such packets.
> 
> [readding CCs I removed earlier]
> 
> Ah! Thanks. I was worried about creating packetloss :D.

Ah, well, no - at least not in this case.

> Hm, for this other other way round, I think it does not apply for
> the bridge multicast-to-unicast patch if I'm not misreading the
> bridge code:
> 
> For a packet with a link-layer multicast address but a unicast IP
> destination, the bridge MDB lookup will fail.
> (http://lxr.free-electrons.com/source/net/bridge/br_multicast.c?v=4.8
> #L178
>  returns NULL)
> 
> Case A): No multicast router on port:
> -> bridge, br_multicast_flood(), will drop the packet already
>    (no matter if multicast-to-unicast is enabled or not)
> 
> Case B): Multicast router present on port:
> -> The new patch does not apply multicast-to-unicast but just floods
>    packet unaltered
>    ("else { port = rport; addr = NULL; }" branch)

Ah, interesting. This is different then - the mac80211 code is not L3
aware at all.

johannes

Re: [PATCH net-next] bridge: multicast to unicast

[IgorMitsyanko]

On 01/11/2017 12:27 AM, Felix Fietkau wrote:
> On 2017-01-10 11:56, Johannes Berg wrote:
>> On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote:
>>> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
>>>> I wonder if MAC80211 should be doing IGMP snooping and not bridge
>>>> in this environment.
>>>
>>> In the long term, yes. For now, not quite sure.
>>
>> There's no "for now" in the kernel. Code added now will have to be
>> maintained essentially forever.
> I'm not sure that putting the IGMP snooping code in mac80211 is a good
> idea, that would be quite a bit of code duplication.
> This implementation works, it's very simple, and it's quite flexible for
> a number of use cases.
>
> Is there any remaining objection to merging this in principle (aside
> from potential issues with the code)?
>
> - Felix
>


Hi Felix, can we consider two examples configurations with multicast 
traffic:

1. AP is a source of multicast traffic itself, no bridge on AP. For 
example, wireless video server streaming to several clients.
In this situation, we can not make use of possible advantages given by 
mc-to-uc conversion?

2. A configuration with AP + STA + 3 client devices behind STA.
                             ----|client 1|
                             |
|  mc  |----|AP|----|STA|---|---|client 2|
|server|                    |
                             ----|client 3|

Multicast server behind AP streams MC video traffic. All 3 clients 
behind the STA have joined the multicast group.
I'm not sure if this case will be handled correctly with mc-to-uc 
conversion in bridge on AP?

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-11 12:26, IgorMitsyanko wrote:
> On 01/11/2017 12:27 AM, Felix Fietkau wrote:
>> On 2017-01-10 11:56, Johannes Berg wrote:
>>> On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote:
>>>> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
>>>>> I wonder if MAC80211 should be doing IGMP snooping and not bridge
>>>>> in this environment.
>>>>
>>>> In the long term, yes. For now, not quite sure.
>>>
>>> There's no "for now" in the kernel. Code added now will have to be
>>> maintained essentially forever.
>> I'm not sure that putting the IGMP snooping code in mac80211 is a good
>> idea, that would be quite a bit of code duplication.
>> This implementation works, it's very simple, and it's quite flexible for
>> a number of use cases.
>>
>> Is there any remaining objection to merging this in principle (aside
>> from potential issues with the code)?
>>
>> - Felix
>>
> 
> 
> Hi Felix, can we consider two examples configurations with multicast 
> traffic:
> 
> 1. AP is a source of multicast traffic itself, no bridge on AP. For 
> example, wireless video server streaming to several clients.
> In this situation, we can not make use of possible advantages given by 
> mc-to-uc conversion?
You could simply put the AP interface in a bridge, no need to have any
other bridge members present.

> 2. A configuration with AP + STA + 3 client devices behind STA.
>                              ----|client 1|
>                              |
> |  mc  |----|AP|----|STA|---|---|client 2|
> |server|                    |
>                              ----|client 3|
> 
> Multicast server behind AP streams MC video traffic. All 3 clients 
> behind the STA have joined the multicast group.
> I'm not sure if this case will be handled correctly with mc-to-uc 
> conversion in bridge on AP?
What do you mean by "3 client devices behind STA"? Are you using a
4-addr STA, multicast routing, or some kind of vendor specific "client
bridge" hackery?

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[IgorMitsyanko]

On 01/11/2017 02:30 PM, Felix Fietkau wrote:
> On 2017-01-11 12:26, IgorMitsyanko wrote:
>> On 01/11/2017 12:27 AM, Felix Fietkau wrote:
>>> On 2017-01-10 11:56, Johannes Berg wrote:
>>>> On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote:
>>>>> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
>>>>>> I wonder if MAC80211 should be doing IGMP snooping and not bridge
>>>>>> in this environment.
>>>>> In the long term, yes. For now, not quite sure.
>>>> There's no "for now" in the kernel. Code added now will have to be
>>>> maintained essentially forever.
>>> I'm not sure that putting the IGMP snooping code in mac80211 is a good
>>> idea, that would be quite a bit of code duplication.
>>> This implementation works, it's very simple, and it's quite flexible for
>>> a number of use cases.
>>>
>>> Is there any remaining objection to merging this in principle (aside
>>> from potential issues with the code)?
>>>
>>> - Felix
>>>
>>
>> Hi Felix, can we consider two examples configurations with multicast
>> traffic:
>>
>> 1. AP is a source of multicast traffic itself, no bridge on AP. For
>> example, wireless video server streaming to several clients.
>> In this situation, we can not make use of possible advantages given by
>> mc-to-uc conversion?
> You could simply put the AP interface in a bridge, no need to have any
> other bridge members present.
>
>> 2. A configuration with AP + STA + 3 client devices behind STA.
>>                               ----|client 1|
>>                              |
>> |  mc  |----|AP|----|STA|---|---|client 2|
>> |server|                    |
>>                               ----|client 3|
>>
>> Multicast server behind AP streams MC video traffic. All 3 clients
>> behind the STA have joined the multicast group.
>> I'm not sure if this case will be handled correctly with mc-to-uc
>> conversion in bridge on AP?
> What do you mean by "3 client devices behind STA"? Are you using a
> 4-addr STA, multicast routing, or some kind of vendor specific "client
> bridge" hackery?

3 client devices connected by backbone Ethernet network. Generic
case is probably STA/AP operating in 4-addr mode (more or less standard
solution as far as I know).

"Client bridge" approach should not concern us here I think, it will
seem to AP and AP's bridge as a single client.

>
> - Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-11 13:15, IgorMitsyanko wrote:
> On 01/11/2017 02:30 PM, Felix Fietkau wrote:
>> On 2017-01-11 12:26, IgorMitsyanko wrote:
>>> On 01/11/2017 12:27 AM, Felix Fietkau wrote:
>>>> On 2017-01-10 11:56, Johannes Berg wrote:
>>>>> On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote:
>>>>>> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
>>>>>>> I wonder if MAC80211 should be doing IGMP snooping and not bridge
>>>>>>> in this environment.
>>>>>> In the long term, yes. For now, not quite sure.
>>>>> There's no "for now" in the kernel. Code added now will have to be
>>>>> maintained essentially forever.
>>>> I'm not sure that putting the IGMP snooping code in mac80211 is a good
>>>> idea, that would be quite a bit of code duplication.
>>>> This implementation works, it's very simple, and it's quite flexible for
>>>> a number of use cases.
>>>>
>>>> Is there any remaining objection to merging this in principle (aside
>>>> from potential issues with the code)?
>>>>
>>>> - Felix
>>>>
>>>
>>> Hi Felix, can we consider two examples configurations with multicast
>>> traffic:
>>>
>>> 1. AP is a source of multicast traffic itself, no bridge on AP. For
>>> example, wireless video server streaming to several clients.
>>> In this situation, we can not make use of possible advantages given by
>>> mc-to-uc conversion?
>> You could simply put the AP interface in a bridge, no need to have any
>> other bridge members present.
>>
>>> 2. A configuration with AP + STA + 3 client devices behind STA.
>>>                               ----|client 1|
>>>                              |
>>> |  mc  |----|AP|----|STA|---|---|client 2|
>>> |server|                    |
>>>                               ----|client 3|
>>>
>>> Multicast server behind AP streams MC video traffic. All 3 clients
>>> behind the STA have joined the multicast group.
>>> I'm not sure if this case will be handled correctly with mc-to-uc
>>> conversion in bridge on AP?
>> What do you mean by "3 client devices behind STA"? Are you using a
>> 4-addr STA, multicast routing, or some kind of vendor specific "client
>> bridge" hackery?
> 
> 3 client devices connected by backbone Ethernet network. Generic
> case is probably STA/AP operating in 4-addr mode (more or less standard
> solution as far as I know).
If the AP is running in 4-addr mode, it will need to have a bridge
interface anyway, because the link to the STA will be split out into a
separate virtual interface (AP_VLAN iftype).

In this case you don't actually need any multicast-to-unicast
conversion, because the multicast traffic will be unicast on 802.11
already (due to use of 4-addr mode).

- Felix