linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC net-next] net: bridge: igmp: Extend IGMP query with vlan support
@ 2020-12-11  9:26 Horatiu Vultur
  2020-12-11  9:46 ` Nikolay Aleksandrov
  0 siblings, 1 reply; 3+ messages in thread
From: Horatiu Vultur @ 2020-12-11  9:26 UTC (permalink / raw)
  To: nikolay, roopa, davem, kuba, bridge, netdev, linux-kernel, allan.nielsen
  Cc: Horatiu Vultur

This patch tries to add vlan support to IGMP queries.
It extends the function 'br_ip4_multicast_alloc_query' to add
also a vlan tag if vlan is enabled. Therefore the bridge will send
queries for each vlan the ports are in.

There are few other places that needs to be updated to be fully
functional. But I am curious if this is the way to go forward or is
there a different way of implementing this?

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_multicast.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 484820c223a3..4c2db8a9efe0 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -688,7 +688,8 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
 						    __be32 ip_dst, __be32 group,
 						    bool with_srcs, bool over_lmqt,
 						    u8 sflag, u8 *igmp_type,
-						    bool *need_rexmit)
+						    bool *need_rexmit,
+						    __u16 vid)
 {
 	struct net_bridge_port *p = pg ? pg->key.port : NULL;
 	struct net_bridge_group_src *ent;
@@ -724,6 +725,9 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
 	}
 
 	pkt_size = sizeof(*eth) + sizeof(*iph) + 4 + igmp_hdr_size;
+	if (br_vlan_enabled(br->dev) && vid != 0)
+		pkt_size += 4;
+
 	if ((p && pkt_size > p->dev->mtu) ||
 	    pkt_size > br->dev->mtu)
 		return NULL;
@@ -732,6 +736,9 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
 	if (!skb)
 		goto out;
 
+	if (br_vlan_enabled(br->dev) && vid != 0)
+		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
+
 	skb->protocol = htons(ETH_P_IP);
 
 	skb_reset_mac_header(skb);
@@ -1008,7 +1015,8 @@ static struct sk_buff *br_multicast_alloc_query(struct net_bridge *br,
 						    ip4_dst, group->dst.ip4,
 						    with_srcs, over_lmqt,
 						    sflag, igmp_type,
-						    need_rexmit);
+						    need_rexmit,
+						    group->vid);
 #if IS_ENABLED(CONFIG_IPV6)
 	case htons(ETH_P_IPV6): {
 		struct in6_addr ip6_dst;
@@ -1477,6 +1485,8 @@ static void br_multicast_send_query(struct net_bridge *br,
 				    struct bridge_mcast_own_query *own_query)
 {
 	struct bridge_mcast_other_query *other_query = NULL;
+	struct net_bridge_vlan_group *vg;
+	struct net_bridge_vlan *v;
 	struct br_ip br_group;
 	unsigned long time;
 
@@ -1485,7 +1495,7 @@ static void br_multicast_send_query(struct net_bridge *br,
 	    !br_opt_get(br, BROPT_MULTICAST_QUERIER))
 		return;
 
-	memset(&br_group.dst, 0, sizeof(br_group.dst));
+	memset(&br_group, 0, sizeof(br_group));
 
 	if (port ? (own_query == &port->ip4_own_query) :
 		   (own_query == &br->ip4_own_query)) {
@@ -1501,8 +1511,19 @@ static void br_multicast_send_query(struct net_bridge *br,
 	if (!other_query || timer_pending(&other_query->timer))
 		return;
 
-	__br_multicast_send_query(br, port, NULL, NULL, &br_group, false, 0,
-				  NULL);
+	if (br_vlan_enabled(br->dev) && port) {
+		vg = nbp_vlan_group(port);
+
+		list_for_each_entry(v, &vg->vlan_list, vlist) {
+			br_group.vid = v->vid == vg->pvid ? 0 : v->vid;
+
+			__br_multicast_send_query(br, port, NULL, NULL,
+						  &br_group, false, 0, NULL);
+		}
+	} else {
+		__br_multicast_send_query(br, port, NULL, NULL, &br_group,
+					  false, 0, NULL);
+	}
 
 	time = jiffies;
 	time += own_query->startup_sent < br->multicast_startup_query_count ?
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [RFC net-next] net: bridge: igmp: Extend IGMP query with vlan support
  2020-12-11  9:26 [RFC net-next] net: bridge: igmp: Extend IGMP query with vlan support Horatiu Vultur
@ 2020-12-11  9:46 ` Nikolay Aleksandrov
  2020-12-11 10:10   ` Horatiu Vultur
  0 siblings, 1 reply; 3+ messages in thread
From: Nikolay Aleksandrov @ 2020-12-11  9:46 UTC (permalink / raw)
  To: Horatiu Vultur, roopa, davem, kuba, bridge, netdev, linux-kernel,
	allan.nielsen

On 11/12/2020 11:26, Horatiu Vultur wrote:
> This patch tries to add vlan support to IGMP queries.
> It extends the function 'br_ip4_multicast_alloc_query' to add
> also a vlan tag if vlan is enabled. Therefore the bridge will send
> queries for each vlan the ports are in.
> 
> There are few other places that needs to be updated to be fully
> functional. But I am curious if this is the way to go forward or is
> there a different way of implementing this?
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  net/bridge/br_multicast.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 

Hi Horatiu,
We've discussed this with other people on netdev before, the way forward is to
implement it as a per-vlan option and then have a per-vlan querier. Which would also
make the change much bigger and more complex. In general some of the multicast options
need to be replicated for vlans to get proper per-vlan multicast control and operation, but
that would require to change a lot of logic around the whole bridge (fast-path included,
where it'd be most sensitive). The good news is that these days we have per-vlan options
support and so only the actual per-vlan multicast implementation is left to be done.
I have this on my TODO list, unfortunately that list gets longer and longer,
so I'd be happy to review patches if someone decides to do it sooner. :)

Sorry, I couldn't find the previous discussion, it was a few years back.

Cheers,
 Nik

> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 484820c223a3..4c2db8a9efe0 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -688,7 +688,8 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
>  						    __be32 ip_dst, __be32 group,
>  						    bool with_srcs, bool over_lmqt,
>  						    u8 sflag, u8 *igmp_type,
> -						    bool *need_rexmit)
> +						    bool *need_rexmit,
> +						    __u16 vid)
>  {
>  	struct net_bridge_port *p = pg ? pg->key.port : NULL;
>  	struct net_bridge_group_src *ent;
> @@ -724,6 +725,9 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
>  	}
>  
>  	pkt_size = sizeof(*eth) + sizeof(*iph) + 4 + igmp_hdr_size;
> +	if (br_vlan_enabled(br->dev) && vid != 0)
> +		pkt_size += 4;
> +
>  	if ((p && pkt_size > p->dev->mtu) ||
>  	    pkt_size > br->dev->mtu)
>  		return NULL;
> @@ -732,6 +736,9 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
>  	if (!skb)
>  		goto out;
>  
> +	if (br_vlan_enabled(br->dev) && vid != 0)
> +		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
> +
>  	skb->protocol = htons(ETH_P_IP);
>  
>  	skb_reset_mac_header(skb);
> @@ -1008,7 +1015,8 @@ static struct sk_buff *br_multicast_alloc_query(struct net_bridge *br,
>  						    ip4_dst, group->dst.ip4,
>  						    with_srcs, over_lmqt,
>  						    sflag, igmp_type,
> -						    need_rexmit);
> +						    need_rexmit,
> +						    group->vid);
>  #if IS_ENABLED(CONFIG_IPV6)
>  	case htons(ETH_P_IPV6): {
>  		struct in6_addr ip6_dst;
> @@ -1477,6 +1485,8 @@ static void br_multicast_send_query(struct net_bridge *br,
>  				    struct bridge_mcast_own_query *own_query)
>  {
>  	struct bridge_mcast_other_query *other_query = NULL;
> +	struct net_bridge_vlan_group *vg;
> +	struct net_bridge_vlan *v;
>  	struct br_ip br_group;
>  	unsigned long time;
>  
> @@ -1485,7 +1495,7 @@ static void br_multicast_send_query(struct net_bridge *br,
>  	    !br_opt_get(br, BROPT_MULTICAST_QUERIER))
>  		return;
>  
> -	memset(&br_group.dst, 0, sizeof(br_group.dst));
> +	memset(&br_group, 0, sizeof(br_group));
>  
>  	if (port ? (own_query == &port->ip4_own_query) :
>  		   (own_query == &br->ip4_own_query)) {
> @@ -1501,8 +1511,19 @@ static void br_multicast_send_query(struct net_bridge *br,
>  	if (!other_query || timer_pending(&other_query->timer))
>  		return;
>  
> -	__br_multicast_send_query(br, port, NULL, NULL, &br_group, false, 0,
> -				  NULL);
> +	if (br_vlan_enabled(br->dev) && port) {
> +		vg = nbp_vlan_group(port);
> +
> +		list_for_each_entry(v, &vg->vlan_list, vlist) {
> +			br_group.vid = v->vid == vg->pvid ? 0 : v->vid;
> +
> +			__br_multicast_send_query(br, port, NULL, NULL,
> +						  &br_group, false, 0, NULL);
> +		}
> +	} else {
> +		__br_multicast_send_query(br, port, NULL, NULL, &br_group,
> +					  false, 0, NULL);
> +	}
>  
>  	time = jiffies;
>  	time += own_query->startup_sent < br->multicast_startup_query_count ?
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [RFC net-next] net: bridge: igmp: Extend IGMP query with vlan support
  2020-12-11  9:46 ` Nikolay Aleksandrov
@ 2020-12-11 10:10   ` Horatiu Vultur
  0 siblings, 0 replies; 3+ messages in thread
From: Horatiu Vultur @ 2020-12-11 10:10 UTC (permalink / raw)
  To: Nikolay Aleksandrov
  Cc: roopa, davem, kuba, bridge, netdev, linux-kernel, allan.nielsen

The 12/11/2020 11:46, Nikolay Aleksandrov wrote:
> 
> On 11/12/2020 11:26, Horatiu Vultur wrote:
> > This patch tries to add vlan support to IGMP queries.
> > It extends the function 'br_ip4_multicast_alloc_query' to add
> > also a vlan tag if vlan is enabled. Therefore the bridge will send
> > queries for each vlan the ports are in.
> >
> > There are few other places that needs to be updated to be fully
> > functional. But I am curious if this is the way to go forward or is
> > there a different way of implementing this?
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  net/bridge/br_multicast.c | 31 ++++++++++++++++++++++++++-----
> >  1 file changed, 26 insertions(+), 5 deletions(-)
> >

Hi Nik,

> 
> Hi Horatiu,
> We've discussed this with other people on netdev before, the way forward is to
> implement it as a per-vlan option and then have a per-vlan querier. Which would also
> make the change much bigger and more complex. In general some of the multicast options
> need to be replicated for vlans to get proper per-vlan multicast control and operation, but
> that would require to change a lot of logic around the whole bridge (fast-path included,
> where it'd be most sensitive).

Thanks for the suggestion and for the heads up. I will have a look and
see how to do it like you mention.


> The good news is that these days we have per-vlan options
> support and so only the actual per-vlan multicast implementation is left to be done.
> I have this on my TODO list, unfortunately that list gets longer and longer,
> so I'd be happy to review patches if someone decides to do it sooner. :)

That would be much appreciated :).

> 
> Sorry, I couldn't find the previous discussion, it was a few years back.
> 
> Cheers,
>  Nik
> 
> > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> > index 484820c223a3..4c2db8a9efe0 100644
> > --- a/net/bridge/br_multicast.c
> > +++ b/net/bridge/br_multicast.c
> > @@ -688,7 +688,8 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
> >                                                   __be32 ip_dst, __be32 group,
> >                                                   bool with_srcs, bool over_lmqt,
> >                                                   u8 sflag, u8 *igmp_type,
> > -                                                 bool *need_rexmit)
> > +                                                 bool *need_rexmit,
> > +                                                 __u16 vid)
> >  {
> >       struct net_bridge_port *p = pg ? pg->key.port : NULL;
> >       struct net_bridge_group_src *ent;
> > @@ -724,6 +725,9 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
> >       }
> >
> >       pkt_size = sizeof(*eth) + sizeof(*iph) + 4 + igmp_hdr_size;
> > +     if (br_vlan_enabled(br->dev) && vid != 0)
> > +             pkt_size += 4;
> > +
> >       if ((p && pkt_size > p->dev->mtu) ||
> >           pkt_size > br->dev->mtu)
> >               return NULL;
> > @@ -732,6 +736,9 @@ static struct sk_buff *br_ip4_multicast_alloc_query(struct net_bridge *br,
> >       if (!skb)
> >               goto out;
> >
> > +     if (br_vlan_enabled(br->dev) && vid != 0)
> > +             __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
> > +
> >       skb->protocol = htons(ETH_P_IP);
> >
> >       skb_reset_mac_header(skb);
> > @@ -1008,7 +1015,8 @@ static struct sk_buff *br_multicast_alloc_query(struct net_bridge *br,
> >                                                   ip4_dst, group->dst.ip4,
> >                                                   with_srcs, over_lmqt,
> >                                                   sflag, igmp_type,
> > -                                                 need_rexmit);
> > +                                                 need_rexmit,
> > +                                                 group->vid);
> >  #if IS_ENABLED(CONFIG_IPV6)
> >       case htons(ETH_P_IPV6): {
> >               struct in6_addr ip6_dst;
> > @@ -1477,6 +1485,8 @@ static void br_multicast_send_query(struct net_bridge *br,
> >                                   struct bridge_mcast_own_query *own_query)
> >  {
> >       struct bridge_mcast_other_query *other_query = NULL;
> > +     struct net_bridge_vlan_group *vg;
> > +     struct net_bridge_vlan *v;
> >       struct br_ip br_group;
> >       unsigned long time;
> >
> > @@ -1485,7 +1495,7 @@ static void br_multicast_send_query(struct net_bridge *br,
> >           !br_opt_get(br, BROPT_MULTICAST_QUERIER))
> >               return;
> >
> > -     memset(&br_group.dst, 0, sizeof(br_group.dst));
> > +     memset(&br_group, 0, sizeof(br_group));
> >
> >       if (port ? (own_query == &port->ip4_own_query) :
> >                  (own_query == &br->ip4_own_query)) {
> > @@ -1501,8 +1511,19 @@ static void br_multicast_send_query(struct net_bridge *br,
> >       if (!other_query || timer_pending(&other_query->timer))
> >               return;
> >
> > -     __br_multicast_send_query(br, port, NULL, NULL, &br_group, false, 0,
> > -                               NULL);
> > +     if (br_vlan_enabled(br->dev) && port) {
> > +             vg = nbp_vlan_group(port);
> > +
> > +             list_for_each_entry(v, &vg->vlan_list, vlist) {
> > +                     br_group.vid = v->vid == vg->pvid ? 0 : v->vid;
> > +
> > +                     __br_multicast_send_query(br, port, NULL, NULL,
> > +                                               &br_group, false, 0, NULL);
> > +             }
> > +     } else {
> > +             __br_multicast_send_query(br, port, NULL, NULL, &br_group,
> > +                                       false, 0, NULL);
> > +     }
> >
> >       time = jiffies;
> >       time += own_query->startup_sent < br->multicast_startup_query_count ?
> >
> 

-- 
/Horatiu

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-12-11 10:13 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-11  9:26 [RFC net-next] net: bridge: igmp: Extend IGMP query with vlan support Horatiu Vultur
2020-12-11  9:46 ` Nikolay Aleksandrov
2020-12-11 10:10   ` Horatiu Vultur

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).