On Fri, Jul 15, 2016 at 8:08 PM, Thomas F Herbert
<thomasfherb...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
