[ovs-dev] [PATCH v3 0/4] Add support for 802.1ad (QinQ tunneling)

[Xiao Liang]

On Fri, Jul 15, 2016 at 8:08 PM, Thomas F Herbert
<thomasfherbert at gmail.com> wrote:
> Eric and Xiao, I am cc'ing this to the ovs-dev list because other reviewers
> who reviewed the kernel module may want to weigh in.
>
> --TFH
>
> On 7/12/16 2:17 PM, Eric Garver wrote:
>>
>> This looks okay to me. This function was definitely missing a call to
>> parse_flow_mask_nlattrs() for the masked case. My only question is why
>> the original code has "if (unlikely(inner))".
>>
>> Tom,
>> Can you speak to why this was checking for the inner tag?
>
> Eric, please see my comments below.
>>
>>
>> On Tue, Jul 12, 2016 at 11:37:32PM +0800, Xiao Liang wrote:
>>>
>>> Hi Eric,
>>>
>>> Nice to hear that we could work together on this feature, and many
>>> thanks for you effort. Hope we could get it work in the upstream soon.
>>>
>>> By the way, for the kernel patch, I changed the last lines in
>>> __parse_vlan_from_nlattrs:
>>>          if (unlikely(inner)) {
>>>                  err = parse_flow_nlattrs(encap, a, en_attrs, log);
>>>                  if (err)
>>>                          return err;
>>>          }
>>>          return 0;
>>> to:
>>>          if (is_mask)
>>>                  err = parse_flow_mask_nlattrs(encap, a, en_attrs, log);
>>>          else
>>>                  err = parse_flow_nlattrs(encap, a, en_attrs, log);
>>>
>>>          return err;
>>>
>>> Is it reasonable?
>
> Yes, I think you are correct. The mask case seems to be missing from my V20
> kernel module patch. Maybe when I refactored the code to use a common
> function for inner and outer vlans, I dropped the mask case. However,
> unlikely(inner) may be OK though because the single tagged vlan is the more
> common case.
>
> A complication is that the OF spec says that matching should be on the outer
> tag only. (See section 7.2.3.8.) I don't recall but the intent of this code
> may have been to check for a mask on the outer tag only. However, it makes
> sense to code it in such a way that both tags are handled symmetrically.

It's true with a single OF table, but inner tag could be matched by
using multiple tables:
table=0,vlan_tci=0x1000/0x1000,actions=pop_vlan,goto_table:1
table=1,vlan_tci=0x1234/0x1fff,actions=output:1
As dp flow, I think it should always contain both VLAN headers (if present).

I'm still wondering how could the second call to
__parse_vlan_from_nlattrs in parse_vlan_from_nlattrs be reached?
Did you mean "if (likely(!inner))"?

Thanks,
Xiao