| Message ID | 1477976454-83179-1-git-send-email-pshelar@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On 31 October 2016 at 22:00, Pravin B Shelar <pshelar@ovn.org> wrote: > The compat vlan code ignores vlan tag for inner packet > on egress path. Following patch fixes this by inserting the > tag for inner packet before tunnel encapsulation. > > Signed-off-by: Pravin B Shelar <pshelar@ovn.org> Is this a problem upstream and for other tunnels too? > --- > datapath/linux/compat/geneve.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c > index 7f2b192..6cce5ca 100644 > --- a/datapath/linux/compat/geneve.c > +++ b/datapath/linux/compat/geneve.c > @@ -750,11 +750,22 @@ static int geneve_build_skb(struct rtable *rt, struct sk_buff *skb, > skb_scrub_packet(skb, xnet); > > min_headroom = LL_RESERVED_SPACE(rt->dst.dev) + rt->dst.header_len > - + GENEVE_BASE_HLEN + opt_len + sizeof(struct iphdr); > + + GENEVE_BASE_HLEN + opt_len + sizeof(struct iphdr) > + + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0); > + > err = skb_cow_head(skb, min_headroom); > if (unlikely(err)) > goto free_rt; > > + if (skb_vlan_tag_present(skb)) { > + err = __vlan_insert_tag(skb, skb->vlan_proto, > + skb_vlan_tag_get(skb)); Does the proto need to be set? I see that the equivalent vxlan code upstream uses vlan_hwaccel_push_inside() instead. > + > + if (unlikely(err)) > + goto free_rt; > + skb->vlan_tci = 0; > + } > + > err = udp_tunnel_handle_offloads(skb, udp_sum); > if (err) > goto free_rt; > @@ -783,11 +794,22 @@ static int geneve6_build_skb(struct dst_entry *dst, struct sk_buff *skb, > skb_scrub_packet(skb, xnet); > > min_headroom = LL_RESERVED_SPACE(dst->dev) + dst->header_len > - + GENEVE_BASE_HLEN + opt_len + sizeof(struct ipv6hdr); > + + GENEVE_BASE_HLEN + opt_len + sizeof(struct ipv6hdr) > + + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0); > + > err = skb_cow_head(skb, min_headroom); > if (unlikely(err)) > goto free_dst; > > + if (skb_vlan_tag_present(skb)) { > + err = __vlan_insert_tag(skb, skb->vlan_proto, > + skb_vlan_tag_get(skb)); > + > + if (unlikely(err)) > + goto free_dst; > + skb->vlan_tci = 0; > + } > + > err = udp_tunnel_handle_offloads(skb, udp_sum); > if (err) > goto free_dst; > -- > 2.5.5 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Tue, Nov 1, 2016 at 10:30 AM, Joe Stringer <joe@ovn.org> wrote: > On 31 October 2016 at 22:00, Pravin B Shelar <pshelar@ovn.org> wrote: >> The compat vlan code ignores vlan tag for inner packet >> on egress path. Following patch fixes this by inserting the >> tag for inner packet before tunnel encapsulation. >> >> Signed-off-by: Pravin B Shelar <pshelar@ovn.org> > > Is this a problem upstream and for other tunnels too? > upstream does not has this issue since networking stack would handle vlan tag for geneve device. >> --- >> datapath/linux/compat/geneve.c | 26 ++++++++++++++++++++++++-- >> 1 file changed, 24 insertions(+), 2 deletions(-) >> >> diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c >> index 7f2b192..6cce5ca 100644 >> --- a/datapath/linux/compat/geneve.c >> +++ b/datapath/linux/compat/geneve.c >> @@ -750,11 +750,22 @@ static int geneve_build_skb(struct rtable *rt, struct sk_buff *skb, >> skb_scrub_packet(skb, xnet); >> >> min_headroom = LL_RESERVED_SPACE(rt->dst.dev) + rt->dst.header_len >> - + GENEVE_BASE_HLEN + opt_len + sizeof(struct iphdr); >> + + GENEVE_BASE_HLEN + opt_len + sizeof(struct iphdr) >> + + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0); >> + >> err = skb_cow_head(skb, min_headroom); >> if (unlikely(err)) >> goto free_rt; >> >> + if (skb_vlan_tag_present(skb)) { >> + err = __vlan_insert_tag(skb, skb->vlan_proto, >> + skb_vlan_tag_get(skb)); > > Does the proto need to be set? I see that the equivalent vxlan code > upstream uses vlan_hwaccel_push_inside() instead. > We can use vlan_hwaccel_push_inside(), but it frees the skb, thats why I prefer __vlan_insert_tag(). It allows us to write simple error handling code.
On 1 November 2016 at 10:48, Pravin Shelar <pshelar@ovn.org> wrote: > On Tue, Nov 1, 2016 at 10:30 AM, Joe Stringer <joe@ovn.org> wrote: >> On 31 October 2016 at 22:00, Pravin B Shelar <pshelar@ovn.org> wrote: >>> The compat vlan code ignores vlan tag for inner packet >>> on egress path. Following patch fixes this by inserting the >>> tag for inner packet before tunnel encapsulation. >>> >>> Signed-off-by: Pravin B Shelar <pshelar@ovn.org> >> >> Is this a problem upstream and for other tunnels too? >> > upstream does not has this issue since networking stack would handle > vlan tag for geneve device. Bear with me, but why does upstream vxlan do something like this but upstream geneve doesn't? >>> --- >>> datapath/linux/compat/geneve.c | 26 ++++++++++++++++++++++++-- >>> 1 file changed, 24 insertions(+), 2 deletions(-) >>> >>> diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c >>> index 7f2b192..6cce5ca 100644 >>> --- a/datapath/linux/compat/geneve.c >>> +++ b/datapath/linux/compat/geneve.c >>> @@ -750,11 +750,22 @@ static int geneve_build_skb(struct rtable *rt, struct sk_buff *skb, >>> skb_scrub_packet(skb, xnet); >>> >>> min_headroom = LL_RESERVED_SPACE(rt->dst.dev) + rt->dst.header_len >>> - + GENEVE_BASE_HLEN + opt_len + sizeof(struct iphdr); >>> + + GENEVE_BASE_HLEN + opt_len + sizeof(struct iphdr) >>> + + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0); >>> + >>> err = skb_cow_head(skb, min_headroom); >>> if (unlikely(err)) >>> goto free_rt; >>> >>> + if (skb_vlan_tag_present(skb)) { >>> + err = __vlan_insert_tag(skb, skb->vlan_proto, >>> + skb_vlan_tag_get(skb)); >> >> Does the proto need to be set? I see that the equivalent vxlan code >> upstream uses vlan_hwaccel_push_inside() instead. >> > We can use vlan_hwaccel_push_inside(), but it frees the skb, thats why > I prefer __vlan_insert_tag(). It allows us to write simple error > handling code. OK, so skb->proto doesn't need to be set to the vlan proto?
On Tue, Nov 1, 2016 at 11:06 AM, Joe Stringer <joe@ovn.org> wrote: > On 1 November 2016 at 10:48, Pravin Shelar <pshelar@ovn.org> wrote: >> On Tue, Nov 1, 2016 at 10:30 AM, Joe Stringer <joe@ovn.org> wrote: >>> On 31 October 2016 at 22:00, Pravin B Shelar <pshelar@ovn.org> wrote: >>>> The compat vlan code ignores vlan tag for inner packet >>>> on egress path. Following patch fixes this by inserting the >>>> tag for inner packet before tunnel encapsulation. >>>> >>>> Signed-off-by: Pravin B Shelar <pshelar@ovn.org> >>> >>> Is this a problem upstream and for other tunnels too? >>> >> upstream does not has this issue since networking stack would handle >> vlan tag for geneve device. > > Bear with me, but why does upstream vxlan do something like this but > upstream geneve doesn't? > Because Geneve device does not expose VLAN offload features. Btw I have patch to handle this discrepancy between geneve and vxlan. >>>> --- >>>> datapath/linux/compat/geneve.c | 26 ++++++++++++++++++++++++-- >>>> 1 file changed, 24 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c >>>> index 7f2b192..6cce5ca 100644 >>>> --- a/datapath/linux/compat/geneve.c >>>> +++ b/datapath/linux/compat/geneve.c >>>> @@ -750,11 +750,22 @@ static int geneve_build_skb(struct rtable *rt, struct sk_buff *skb, >>>> skb_scrub_packet(skb, xnet); >>>> >>>> min_headroom = LL_RESERVED_SPACE(rt->dst.dev) + rt->dst.header_len >>>> - + GENEVE_BASE_HLEN + opt_len + sizeof(struct iphdr); >>>> + + GENEVE_BASE_HLEN + opt_len + sizeof(struct iphdr) >>>> + + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0); >>>> + >>>> err = skb_cow_head(skb, min_headroom); >>>> if (unlikely(err)) >>>> goto free_rt; >>>> >>>> + if (skb_vlan_tag_present(skb)) { >>>> + err = __vlan_insert_tag(skb, skb->vlan_proto, >>>> + skb_vlan_tag_get(skb)); >>> >>> Does the proto need to be set? I see that the equivalent vxlan code >>> upstream uses vlan_hwaccel_push_inside() instead. >>> >> We can use vlan_hwaccel_push_inside(), but it frees the skb, thats why >> I prefer __vlan_insert_tag(). It allows us to write simple error >> handling code. > > OK, so skb->proto doesn't need to be set to the vlan proto? OIC, you meant skb->protocol, Yes I need to set it. __vlan_insert_tag() does not do it for us.
diff --git a/datapath/linux/compat/geneve.c b/datapath/linux/compat/geneve.c index 7f2b192..6cce5ca 100644 --- a/datapath/linux/compat/geneve.c +++ b/datapath/linux/compat/geneve.c @@ -750,11 +750,22 @@ static int geneve_build_skb(struct rtable *rt, struct sk_buff *skb, skb_scrub_packet(skb, xnet); min_headroom = LL_RESERVED_SPACE(rt->dst.dev) + rt->dst.header_len - + GENEVE_BASE_HLEN + opt_len + sizeof(struct iphdr); + + GENEVE_BASE_HLEN + opt_len + sizeof(struct iphdr) + + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0); + err = skb_cow_head(skb, min_headroom); if (unlikely(err)) goto free_rt; + if (skb_vlan_tag_present(skb)) { + err = __vlan_insert_tag(skb, skb->vlan_proto, + skb_vlan_tag_get(skb)); + + if (unlikely(err)) + goto free_rt; + skb->vlan_tci = 0; + } + err = udp_tunnel_handle_offloads(skb, udp_sum); if (err) goto free_rt; @@ -783,11 +794,22 @@ static int geneve6_build_skb(struct dst_entry *dst, struct sk_buff *skb, skb_scrub_packet(skb, xnet); min_headroom = LL_RESERVED_SPACE(dst->dev) + dst->header_len - + GENEVE_BASE_HLEN + opt_len + sizeof(struct ipv6hdr); + + GENEVE_BASE_HLEN + opt_len + sizeof(struct ipv6hdr) + + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0); + err = skb_cow_head(skb, min_headroom); if (unlikely(err)) goto free_dst; + if (skb_vlan_tag_present(skb)) { + err = __vlan_insert_tag(skb, skb->vlan_proto, + skb_vlan_tag_get(skb)); + + if (unlikely(err)) + goto free_dst; + skb->vlan_tci = 0; + } + err = udp_tunnel_handle_offloads(skb, udp_sum); if (err) goto free_dst;
The compat vlan code ignores vlan tag for inner packet on egress path. Following patch fixes this by inserting the tag for inner packet before tunnel encapsulation. Signed-off-by: Pravin B Shelar <pshelar@ovn.org> --- datapath/linux/compat/geneve.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-)