| Message ID | 1359748930-31475-6-git-send-email-vyasevic@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
2013/2/1 Vlad Yasevich <vyasevic@redhat.com>: > A user may designate a certain vlan as PVID. This means that > any ingress frame that does not contain a vlan tag is assigned to > this vlan and any forwarding decisions are made with this vlan in mind. [...] > struct net_port_vlans { > u16 port_idx; > + u16 pvid; I'm confused about the implementation. I would expect pvid field in net_bridge_port and adding a tag if it isn't there on ingress path. The rest would be just like without PVIDs. But here you pvid field to net_port_vlans, and don't do anything with it in receive nor transmit path. Does it work? What am I missing? Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013/2/2 Michał Mirosław <mirqus@gmail.com>: > 2013/2/1 Vlad Yasevich <vyasevic@redhat.com>: >> A user may designate a certain vlan as PVID. This means that >> any ingress frame that does not contain a vlan tag is assigned to >> this vlan and any forwarding decisions are made with this vlan in mind. > [...] >> struct net_port_vlans { >> u16 port_idx; >> + u16 pvid; > > I'm confused about the implementation. I would expect pvid field in > net_bridge_port and adding a tag if it isn't there on ingress path. > The rest would be just like without PVIDs. But here you pvid field to > net_port_vlans, and don't do anything with it in receive nor transmit > path. Does it work? What am I missing? Found the answer in next patch (you should merge #5 and #6). Still, the implementation looks overly complicated. If you force the packet to canonical form on ingress (keeping outer tag in skb->vlan_tci, and setting skb->vlan_tci = pvid if there is no tag) the code should get simpler. Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/01/2013 08:15 PM, Michał Mirosław wrote: > 2013/2/2 Michał Mirosław <mirqus@gmail.com>: >> 2013/2/1 Vlad Yasevich <vyasevic@redhat.com>: >>> A user may designate a certain vlan as PVID. This means that >>> any ingress frame that does not contain a vlan tag is assigned to >>> this vlan and any forwarding decisions are made with this vlan in mind. >> [...] >>> struct net_port_vlans { >>> u16 port_idx; >>> + u16 pvid; >> >> I'm confused about the implementation. I would expect pvid field in >> net_bridge_port and adding a tag if it isn't there on ingress path. >> The rest would be just like without PVIDs. But here you pvid field to >> net_port_vlans, and don't do anything with it in receive nor transmit >> path. Does it work? What am I missing? > > Found the answer in next patch (you should merge #5 and #6). It was split for incremental testing. #5 added the ability to set and delete it without impacting anything. #6 added the actual work that pvid does. > Still, > the implementation looks overly complicated. If you force the packet > to canonical form on ingress (keeping outer tag in skb->vlan_tci, and > setting skb->vlan_tci = pvid if there is no tag) the code should get > simpler. What if there is no outer tag? That's what the ingress code is doing. If there is no outer tag, pvid is written to vlan_tci. If there was outer tag in vlan_tci, it's left alone. This way at the end of ingress vlan_tci is always set. At egress, we grab that tag and compare it against pvid if any. If it matches, it's stripped. If it doesn't, we output with the tag thus adding the header. The only thing I can simplify is grab the tci directly at egress, but that's what the code will do anyway. -vlad > > Best Regards, > Michał Mirosław > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013/2/2 Vlad Yasevich <vyasevic@redhat.com>: > On 02/01/2013 08:15 PM, Michał Mirosław wrote: >> 2013/2/2 Michał Mirosław <mirqus@gmail.com>: >>> 2013/2/1 Vlad Yasevich <vyasevic@redhat.com>: >>>> A user may designate a certain vlan as PVID. This means that >>>> any ingress frame that does not contain a vlan tag is assigned to >>>> this vlan and any forwarding decisions are made with this vlan in mind. >>> >>> [...] >>>> >>>> struct net_port_vlans { >>>> u16 port_idx; >>>> + u16 pvid; >>> >>> >>> I'm confused about the implementation. I would expect pvid field in >>> net_bridge_port and adding a tag if it isn't there on ingress path. >>> The rest would be just like without PVIDs. But here you pvid field to >>> net_port_vlans, and don't do anything with it in receive nor transmit >>> path. Does it work? What am I missing? >> Found the answer in next patch (you should merge #5 and #6). > It was split for incremental testing. #5 added the ability to set and > delete it without impacting anything. #6 added the actual work that pvid > does. > >> Still, >> the implementation looks overly complicated. If you force the packet >> to canonical form on ingress (keeping outer tag in skb->vlan_tci, and >> setting skb->vlan_tci = pvid if there is no tag) the code should get >> simpler. > > > What if there is no outer tag? That's what the ingress code is doing. > If there is no outer tag, pvid is written to vlan_tci. If there was > outer tag in vlan_tci, it's left alone. This way at the end of ingress > vlan_tci is always set. > At egress, we grab that tag and compare it against pvid if any. If it > matches, it's stripped. If it doesn't, we output with the tag thus > adding the header. > > The only thing I can simplify is grab the tci directly at egress, but > that's what the code will do anyway. ... br_allowed_input(..., struct sk_buff **pskb) { *pskb = vlan_untag(*pskb); skb = *pskb; if (!skb) return 0; if (!skb->vlan_tci && (pvid & VLAN_TAG_PRESENT)) skb->vlan_tci = pvid; return check_vlan_allowed(skb->vlan_tci); } struct sk_buff *br_handle_vlan(..., struct sk_buff *skb) { /* we guaranteed in br_allowed_input() that all packets processed in bridge code * will be like received with NETIF_F_HW_VLAN_RX feature enabled. */ if (skb->vlan_tci & (VLAN_VID_MASK|VLAN_TAG_PRESENT) == pvid) skb->vlan_tci = 0; // what about 802.1p? return skb; } BTW, you implement three features: VLAN filtering, PVID handling, per-VLAN FDB. Yet there are 10 patches that mix and match parts of the implementation. It's really hard to review this when you have to jump between patches to understand whats going on. I don't know what the best split would be, but I got the feeling that this is not the presented one. Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013/2/3 Michał Mirosław <mirqus@gmail.com>: > 2013/2/2 Vlad Yasevich <vyasevic@redhat.com>: >> On 02/01/2013 08:15 PM, Michał Mirosław wrote: >>> 2013/2/2 Michał Mirosław <mirqus@gmail.com>: >>>> 2013/2/1 Vlad Yasevich <vyasevic@redhat.com>: >>>>> A user may designate a certain vlan as PVID. This means that >>>>> any ingress frame that does not contain a vlan tag is assigned to >>>>> this vlan and any forwarding decisions are made with this vlan in mind. >>>> >>>> [...] >>>>> >>>>> struct net_port_vlans { >>>>> u16 port_idx; >>>>> + u16 pvid; >>>> >>>> >>>> I'm confused about the implementation. I would expect pvid field in >>>> net_bridge_port and adding a tag if it isn't there on ingress path. >>>> The rest would be just like without PVIDs. But here you pvid field to >>>> net_port_vlans, and don't do anything with it in receive nor transmit >>>> path. Does it work? What am I missing? >>> Found the answer in next patch (you should merge #5 and #6). >> It was split for incremental testing. #5 added the ability to set and >> delete it without impacting anything. #6 added the actual work that pvid >> does. >> >>> Still, >>> the implementation looks overly complicated. If you force the packet >>> to canonical form on ingress (keeping outer tag in skb->vlan_tci, and >>> setting skb->vlan_tci = pvid if there is no tag) the code should get >>> simpler. >> >> >> What if there is no outer tag? That's what the ingress code is doing. >> If there is no outer tag, pvid is written to vlan_tci. If there was >> outer tag in vlan_tci, it's left alone. This way at the end of ingress >> vlan_tci is always set. >> At egress, we grab that tag and compare it against pvid if any. If it >> matches, it's stripped. If it doesn't, we output with the tag thus >> adding the header. >> >> The only thing I can simplify is grab the tci directly at egress, but >> that's what the code will do anyway. > > ... br_allowed_input(..., struct sk_buff **pskb) > { > *pskb = vlan_untag(*pskb); > skb = *pskb; > if (!skb) > return 0; > > if (!skb->vlan_tci && (pvid & VLAN_TAG_PRESENT)) > skb->vlan_tci = pvid; > return check_vlan_allowed(skb->vlan_tci); > } Or maybe some cheating to save unconditional vlan_untag()ging: since vlan_tci is used only when vlan_tx_tag_present() returns true, we can cache VID there for the bridge code when VLANs are not HW-accelerated. I'm not sure of this one as I don't know if the netfilter part keeps vlan_tci intact (quick grep suggests it does). ... br_allowed_input(..., struct sk_buff **pskb) { skb = *pskb; if (!vlan_tx_tag_present(skb)) { if (!__vlan_get_tag(skb, &skb->vlan_tci)) skb->vlan_tci &= VLAN_VID_MASK; else skb->vlan_tci = 0; } if (!skb->vlan_tci && pvid & VLAN_TAG_PRESENT) // FIXME: 802.1p-tagged is put in VLAN 0 skb->vlan_tci = pvid; return check_vlan_allowed(skb, skb->vlan_tci & VLAN_VID_MASK); } struct sk_buff *br_handle_vlan(..., struct sk_buff *skb) { /* we guaranteed in br_allowed_input() that all packets processed /* in bridge code will be like received with NETIF_F_HW_VLAN_RX * feature enabled or have VID cached in vlan_tci without * VLAN_TAG_PRESENT bit set. */ if ((skb->vlan_tci & VLAN_VID_MASK) | VLAN_TAG_PRESENT != pvid) return skb; if (!vlan_tx_tag_present(skb)) { skb = vlan_untag(skb); if (!skb) return skb; } skb->vlan_tci = 0; // FIXME: what about 802.1p? return skb; } Best Regards, Michał Mirosław -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/03/2013 09:49 AM, Michał Mirosław wrote: > 2013/2/2 Vlad Yasevich <vyasevic@redhat.com>: >> On 02/01/2013 08:15 PM, Michał Mirosław wrote: >>> 2013/2/2 Michał Mirosław <mirqus@gmail.com>: >>>> 2013/2/1 Vlad Yasevich <vyasevic@redhat.com>: >>>>> A user may designate a certain vlan as PVID. This means that >>>>> any ingress frame that does not contain a vlan tag is assigned to >>>>> this vlan and any forwarding decisions are made with this vlan in mind. >>>> >>>> [...] >>>>> >>>>> struct net_port_vlans { >>>>> u16 port_idx; >>>>> + u16 pvid; >>>> >>>> >>>> I'm confused about the implementation. I would expect pvid field in >>>> net_bridge_port and adding a tag if it isn't there on ingress path. >>>> The rest would be just like without PVIDs. But here you pvid field to >>>> net_port_vlans, and don't do anything with it in receive nor transmit >>>> path. Does it work? What am I missing? >>> Found the answer in next patch (you should merge #5 and #6). >> It was split for incremental testing. #5 added the ability to set and >> delete it without impacting anything. #6 added the actual work that pvid >> does. >> >>> Still, >>> the implementation looks overly complicated. If you force the packet >>> to canonical form on ingress (keeping outer tag in skb->vlan_tci, and >>> setting skb->vlan_tci = pvid if there is no tag) the code should get >>> simpler. >> >> >> What if there is no outer tag? That's what the ingress code is doing. >> If there is no outer tag, pvid is written to vlan_tci. If there was >> outer tag in vlan_tci, it's left alone. This way at the end of ingress >> vlan_tci is always set. >> At egress, we grab that tag and compare it against pvid if any. If it >> matches, it's stripped. If it doesn't, we output with the tag thus >> adding the header. >> >> The only thing I can simplify is grab the tci directly at egress, but >> that's what the code will do anyway. > > ... br_allowed_input(..., struct sk_buff **pskb) > { > *pskb = vlan_untag(*pskb); > skb = *pskb; > if (!skb) > return 0; > > if (!skb->vlan_tci && (pvid & VLAN_TAG_PRESENT)) > skb->vlan_tci = pvid; > return check_vlan_allowed(skb->vlan_tci); > } vlan_untag typically already happens for non-accelerated packets, so no need to call it again. We shouldn't really be touching accelerated packets at ingress, because we may have to undo what we've done on egress. That would be very expensive in case of flooding. > > struct sk_buff *br_handle_vlan(..., struct sk_buff *skb) > { > /* we guaranteed in br_allowed_input() that all packets processed > in bridge code > * will be like received with NETIF_F_HW_VLAN_RX feature enabled. */ > if (skb->vlan_tci & (VLAN_VID_MASK|VLAN_TAG_PRESENT) == pvid) > skb->vlan_tci = 0; // what about 802.1p? > return skb; > } This doesn't work for when you should be sending a tagged frame to the bridge device when receive was non-accelerated. It would mean that the 802.1q header was already stripped, and you have to put it back correctly so that it can go through the vlan interface (think vlan on top of bridge). > > BTW, you implement three features: VLAN filtering, PVID handling, > per-VLAN FDB. Yet there are 10 patches that mix and match parts of the > implementation. I've tried to separate them as well as I can. I guess the only ones out of order are 11 and 12. 12 is last on purpose since its contentious and can be easily dropped from the end. I can and should move 11 in with main vlan filtering code. Essentially 1-5 would be VLAN filtering, 5 & 6 would be PVID, and the rest would FDB. If you can see a better breakdown, I would appreciate it. Thanks -vlad > It's really hard to review this when you have to jump > between patches to understand whats going on. I don't know what the > best split would be, but I got the feeling that this is not the > presented one. > > Best Regards, > Michał Mirosław > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h index 3ca9817..c6c30e2 100644 --- a/include/uapi/linux/if_bridge.h +++ b/include/uapi/linux/if_bridge.h @@ -120,6 +120,7 @@ enum { #define IFLA_BRIDGE_MAX (__IFLA_BRIDGE_MAX - 1) #define BRIDGE_VLAN_INFO_MASTER (1<<0) /* Operate on Bridge device as well */ +#define BRIDGE_VLAN_INFO_PVID (1<<1) /* VLAN is PVID, ingress untagged */ struct bridge_vlan_info { u16 flags; diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 534a9f4..0089a3f 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -198,14 +198,15 @@ static int br_afspec(struct net_bridge *br, switch (cmd) { case RTM_SETLINK: if (p) { - err = nbp_vlan_add(p, vinfo->vid); + err = nbp_vlan_add(p, vinfo->vid, vinfo->flags); if (err) break; if (vinfo->flags & BRIDGE_VLAN_INFO_MASTER) - err = br_vlan_add(p->br, vinfo->vid); + err = br_vlan_add(p->br, vinfo->vid, + vinfo->flags); } else - err = br_vlan_add(br, vinfo->vid); + err = br_vlan_add(br, vinfo->vid, vinfo->flags); if (err) break; diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 801a707..7ad26f5 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -68,6 +68,7 @@ struct br_ip struct net_port_vlans { u16 port_idx; + u16 pvid; void *parent; struct rcu_head rcu; unsigned long vlan_bitmap[BR_VLAN_BITMAP_LEN]; @@ -556,11 +557,11 @@ extern bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v, extern bool br_allowed_egress(struct net_bridge *br, const struct net_port_vlans *v, const struct sk_buff *skb); -extern int br_vlan_add(struct net_bridge *br, u16 vid); +extern int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags); extern int br_vlan_delete(struct net_bridge *br, u16 vid); extern void br_vlan_flush(struct net_bridge *br); extern int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val); -extern int nbp_vlan_add(struct net_bridge_port *port, u16 vid); +extern int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags); extern int nbp_vlan_delete(struct net_bridge_port *port, u16 vid); extern void nbp_vlan_flush(struct net_bridge_port *port); @@ -602,7 +603,7 @@ static inline bool br_allowed_egress(struct net_bridge *br, return true; } -static inline int br_vlan_add(struct net_bridge *br, u16 vid) +static inline int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags) { return -EOPNOTSUPP; } @@ -616,7 +617,7 @@ static inline void br_vlan_flush(struct net_bridge *br) { } -static inline int nbp_vlan_add(struct net_bridge_port *port, u16 vid) +static inline int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags) { return -EOPNOTSUPP; } diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index a291bca..b5b6265 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -7,12 +7,33 @@ #define vlans_to_parent(type, pv) ((type *)(pv)->parent) -static int __vlan_add(struct net_port_vlans *v, u16 vid) +static void __vlan_add_pvid(struct net_port_vlans *v, u16 vid) +{ + if (v->pvid == vid) + return; + + smp_wmb(); + v->pvid = vid; +} + +static void __vlan_delete_pvid(struct net_port_vlans *v, u16 vid) +{ + if (v->pvid != vid) + return; + + smp_wmb(); + v->pvid = BR_INVALID_VID; +} + +static int __vlan_add(struct net_port_vlans *v, u16 vid, u16 flags) { int err; - if (test_bit(vid, v->vlan_bitmap)) - return -EEXIST; + if (test_bit(vid, v->vlan_bitmap)) { + if (flags & BRIDGE_VLAN_INFO_PVID) + __vlan_add_pvid(v, vid); + return 0; + } if (v->port_idx && vid) { struct net_device *dev = @@ -31,6 +52,9 @@ static int __vlan_add(struct net_port_vlans *v, u16 vid) } set_bit(vid, v->vlan_bitmap); + if (flags & BRIDGE_VLAN_INFO_PVID) + __vlan_add_pvid(v, vid); + return 0; } @@ -42,6 +66,8 @@ static int __vlan_del(struct net_port_vlans *v, u16 vid) if (!test_bit(vid, v->vlan_bitmap)) return -EINVAL; + __vlan_delete_pvid(v, vid); + /* Check to see if any other vlans are in this table. If this * is the last vlan, delete the whole structure. If this is not the * last vlan, just clear the bit. @@ -75,6 +101,8 @@ static int __vlan_del(struct net_port_vlans *v, u16 vid) static void __vlan_flush(struct net_port_vlans *v) { + smp_wmb(); + v->pvid = BR_INVALID_VID; bitmap_zero(v->vlan_bitmap, BR_VLAN_BITMAP_LEN); if (v->port_idx) { struct net_bridge_port *p = @@ -134,7 +162,7 @@ bool br_allowed_egress(struct net_bridge *br, } /* Must be protected by RTNL */ -int br_vlan_add(struct net_bridge *br, u16 vid) +int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags) { struct net_port_vlans *pv = NULL; int err; @@ -143,7 +171,7 @@ int br_vlan_add(struct net_bridge *br, u16 vid) pv = rtnl_dereference(br->vlan_info); if (pv) - return __vlan_add(pv, vid); + return __vlan_add(pv, vid, flags); /* Create port vlan infomration */ @@ -152,7 +180,8 @@ int br_vlan_add(struct net_bridge *br, u16 vid) return -ENOMEM; pv->parent = br; - err = __vlan_add(pv, vid); + pv->pvid = BR_INVALID_VID; + err = __vlan_add(pv, vid, flags); if (err) goto out; @@ -183,7 +212,6 @@ void br_vlan_flush(struct net_bridge *br) struct net_port_vlans *pv; ASSERT_RTNL(); - pv = rtnl_dereference(br->vlan_info); if (!pv) return; @@ -207,7 +235,7 @@ unlock: } /* Must be protected by RTNL */ -int nbp_vlan_add(struct net_bridge_port *port, u16 vid) +int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags) { struct net_port_vlans *pv = NULL; int err; @@ -216,7 +244,7 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid) pv = rtnl_dereference(port->vlan_info); if (pv) - return __vlan_add(pv, vid); + return __vlan_add(pv, vid, flags); /* Create port vlan infomration */ @@ -228,7 +256,8 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid) pv->port_idx = port->port_no; pv->parent = port; - err = __vlan_add(pv, vid); + pv->pvid = BR_INVALID_VID; + err = __vlan_add(pv, vid, flags); if (err) goto clean_up;
A user may designate a certain vlan as PVID. This means that any ingress frame that does not contain a vlan tag is assigned to this vlan and any forwarding decisions are made with this vlan in mind. Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> --- include/uapi/linux/if_bridge.h | 1 + net/bridge/br_netlink.c | 7 +++-- net/bridge/br_private.h | 9 ++++--- net/bridge/br_vlan.c | 49 +++++++++++++++++++++++++++++++-------- 4 files changed, 49 insertions(+), 17 deletions(-)