[ovs-dev] [PATCH] datapath: geneve: Handle vlan tag

[Joe Stringer]

On 1 November 2016 at 10:48, Pravin Shelar <pshelar at ovn.org> wrote:
> On Tue, Nov 1, 2016 at 10:30 AM, Joe Stringer <joe at ovn.org> wrote:
>> On 31 October 2016 at 22:00, Pravin B Shelar <pshelar at 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 at 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?