| Message ID | 20201211092626.809206-1-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] net: bridge: igmp: Extend IGMP query with vlan support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 73 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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 ? >
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 ? > > >
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 ?
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(-)