Message ID | 20110610083539.GB17568@minipsycho.redhat.com |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
On Fri, Jun 10, 2011 at 4:35 PM, Jiri Pirko <jpirko@redhat.com> wrote: > + > +/* Should be used only to revert vlan_reorder_header action */ > +static struct sk_buff *vlan_unreorder_header(struct sk_buff *skb) > +{ > + unsigned char *mac_header; > + struct vlan_ethhdr *veth; > + > + if (skb_cow(skb, skb_headroom(skb)) < 0) > + return NULL; I think we need to make sure if there is enough headroom for this header expansion. > + skb->mac_header -= VLAN_HLEN; skb->mac_len isn't adjusted. You forgot to move the headers resetting after vlan_untag().
Fri, Jun 10, 2011 at 11:26:06AM CEST, xiaosuo@gmail.com wrote: >On Fri, Jun 10, 2011 at 4:35 PM, Jiri Pirko <jpirko@redhat.com> wrote: >> + >> +/* Should be used only to revert vlan_reorder_header action */ >> +static struct sk_buff *vlan_unreorder_header(struct sk_buff *skb) >> +{ >> + unsigned char *mac_header; >> + struct vlan_ethhdr *veth; >> + >> + if (skb_cow(skb, skb_headroom(skb)) < 0) >> + return NULL; > >I think we need to make sure if there is enough headroom for this >header expansion. Well the header expansion was previously there so there should be place there in all cases, or am I wrong? > >> + skb->mac_header -= VLAN_HLEN; > >skb->mac_len isn't adjusted. You forgot to move the headers resetting >after vlan_untag(). that is correct, I'll move that. I'll also add reset after unreorder. Thanks Changli. Jirka > > > >-- >Regards, >Changli Gao(xiaosuo@gmail.com) -- 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 Fri, Jun 10, 2011 at 5:34 PM, Jiri Pirko <jpirko@redhat.com> wrote: > Fri, Jun 10, 2011 at 11:26:06AM CEST, xiaosuo@gmail.com wrote: >>On Fri, Jun 10, 2011 at 4:35 PM, Jiri Pirko <jpirko@redhat.com> wrote: >>> + >>> +/* Should be used only to revert vlan_reorder_header action */ >>> +static struct sk_buff *vlan_unreorder_header(struct sk_buff *skb) >>> +{ >>> + unsigned char *mac_header; >>> + struct vlan_ethhdr *veth; >>> + >>> + if (skb_cow(skb, skb_headroom(skb)) < 0) >>> + return NULL; >> >>I think we need to make sure if there is enough headroom for this >>header expansion. > > Well the header expansion was previously there so there should be place > there in all cases, or am I wrong? For hw-accel-vlan-rx, is the headroom for this header expansion reserved? I don't think so. Thanks.
Fri, Jun 10, 2011 at 11:49:27AM CEST, xiaosuo@gmail.com wrote: >On Fri, Jun 10, 2011 at 5:34 PM, Jiri Pirko <jpirko@redhat.com> wrote: >> Fri, Jun 10, 2011 at 11:26:06AM CEST, xiaosuo@gmail.com wrote: >>>On Fri, Jun 10, 2011 at 4:35 PM, Jiri Pirko <jpirko@redhat.com> wrote: >>>> + >>>> +/* Should be used only to revert vlan_reorder_header action */ >>>> +static struct sk_buff *vlan_unreorder_header(struct sk_buff *skb) >>>> +{ >>>> + unsigned char *mac_header; >>>> + struct vlan_ethhdr *veth; >>>> + >>>> + if (skb_cow(skb, skb_headroom(skb)) < 0) >>>> + return NULL; >>> >>>I think we need to make sure if there is enough headroom for this >>>header expansion. >> >> Well the header expansion was previously there so there should be place >> there in all cases, or am I wrong? > >For hw-accel-vlan-rx, is the headroom for this header expansion >reserved? I don't think so. Thanks. But this wasn't done for hw-accel-vlan-rx previously right? So why don't check for hw-accel-vlan-rx and don do unreorder in that case? > >-- >Regards, >Changli Gao(xiaosuo@gmail.com) -- 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 Fri, Jun 10, 2011 at 6:35 PM, Jiri Pirko <jpirko@redhat.com> wrote: > Fri, Jun 10, 2011 at 11:49:27AM CEST, xiaosuo@gmail.com wrote: >> >>For hw-accel-vlan-rx, is the headroom for this header expansion >>reserved? I don't think so. Thanks. > > But this wasn't done for hw-accel-vlan-rx previously right? So why don't > check for hw-accel-vlan-rx and don do unreorder in that case? > Yes, it wasn't done before. We want to make the behaviors of hw-accel-vlan-rx and non-hw-accel-vlan-rx are the same. Is it your original goal of your vlan-untag patch?
Fri, Jun 10, 2011 at 01:20:29PM CEST, xiaosuo@gmail.com wrote: >On Fri, Jun 10, 2011 at 6:35 PM, Jiri Pirko <jpirko@redhat.com> wrote: >> Fri, Jun 10, 2011 at 11:49:27AM CEST, xiaosuo@gmail.com wrote: >>> >>>For hw-accel-vlan-rx, is the headroom for this header expansion >>>reserved? I don't think so. Thanks. >> >> But this wasn't done for hw-accel-vlan-rx previously right? So why don't >> check for hw-accel-vlan-rx and don do unreorder in that case? >> > >Yes, it wasn't done before. We want to make the behaviors of >hw-accel-vlan-rx and non-hw-accel-vlan-rx are the same. Is it your >original goal of your vlan-untag patch? You are correct. I just thought that since disabling reordering will be most likely removed in near future, non-hw-accel-vlan-rx reorder disable would not be needed. > >-- >Regards, >Changli Gao(xiaosuo@gmail.com) -- 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/net/8021q/vlan_core.c b/net/8021q/vlan_core.c index 41495dc..d71cc81 100644 --- a/net/8021q/vlan_core.c +++ b/net/8021q/vlan_core.c @@ -4,6 +4,33 @@ #include <linux/netpoll.h> #include "vlan.h" +static struct sk_buff *vlan_reorder_header(struct sk_buff *skb) +{ + if (skb_cow(skb, skb_headroom(skb)) < 0) + return NULL; + /* Lifted from Gleb's VLAN code... */ + memmove(skb->data - ETH_HLEN, skb->data - VLAN_ETH_HLEN, 12); + skb->mac_header += VLAN_HLEN; + return skb; +} + +/* Should be used only to revert vlan_reorder_header action */ +static struct sk_buff *vlan_unreorder_header(struct sk_buff *skb) +{ + unsigned char *mac_header; + struct vlan_ethhdr *veth; + + if (skb_cow(skb, skb_headroom(skb)) < 0) + return NULL; + skb->mac_header -= VLAN_HLEN; + mac_header = skb_mac_header(skb); + memmove(mac_header, mac_header + VLAN_HLEN, 12); + veth = (struct vlan_ethhdr *) mac_header; + veth->h_vlan_proto = htons(ETH_P_8021Q); + veth->h_vlan_TCI = htons(skb->vlan_tci); + return skb; +} + bool vlan_do_receive(struct sk_buff **skbp) { struct sk_buff *skb = *skbp; @@ -23,6 +50,21 @@ bool vlan_do_receive(struct sk_buff **skbp) return false; skb->dev = vlan_dev; + if (skb->pkt_type == PACKET_OTHERHOST) { + /* Our lower layer thinks this is not local, let's make sure. + * This allows the VLAN to have a different MAC than the + * underlying device, and still route correctly. */ + if (!compare_ether_addr(eth_hdr(skb)->h_dest, + vlan_dev->dev_addr)) + skb->pkt_type = PACKET_HOST; + } + + if (!(vlan_dev_info(vlan_dev)->flags & VLAN_FLAG_REORDER_HDR)) { + skb = *skbp = vlan_unreorder_header(skb); + if (!skb) + return false; + } + skb->priority = vlan_get_ingress_priority(vlan_dev, skb->vlan_tci); skb->vlan_tci = 0; @@ -31,22 +73,8 @@ bool vlan_do_receive(struct sk_buff **skbp) u64_stats_update_begin(&rx_stats->syncp); rx_stats->rx_packets++; rx_stats->rx_bytes += skb->len; - - switch (skb->pkt_type) { - case PACKET_BROADCAST: - break; - case PACKET_MULTICAST: + if (skb->pkt_type == PACKET_MULTICAST) rx_stats->rx_multicast++; - break; - case PACKET_OTHERHOST: - /* Our lower layer thinks this is not local, let's make sure. - * This allows the VLAN to have a different MAC than the - * underlying device, and still route correctly. */ - if (!compare_ether_addr(eth_hdr(skb)->h_dest, - vlan_dev->dev_addr)) - skb->pkt_type = PACKET_HOST; - break; - } u64_stats_update_end(&rx_stats->syncp); return true; @@ -89,21 +117,6 @@ gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp, } EXPORT_SYMBOL(vlan_gro_frags); -static struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb) -{ - if (vlan_dev_info(skb->dev)->flags & VLAN_FLAG_REORDER_HDR) { - if (skb_cow(skb, skb_headroom(skb)) < 0) - skb = NULL; - if (skb) { - /* Lifted from Gleb's VLAN code... */ - memmove(skb->data - ETH_HLEN, - skb->data - VLAN_ETH_HLEN, 12); - skb->mac_header += VLAN_HLEN; - } - } - return skb; -} - static void vlan_set_encap_proto(struct sk_buff *skb, struct vlan_hdr *vhdr) { __be16 proto; @@ -161,7 +174,7 @@ struct sk_buff *vlan_untag(struct sk_buff *skb) skb_pull_rcsum(skb, VLAN_HLEN); vlan_set_encap_proto(skb, vhdr); - skb = vlan_check_reorder_header(skb); + skb = vlan_reorder_header(skb); if (unlikely(!skb)) goto err_free;