On 7/15/16 9:09 AM, Xiao Liang wrote:
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).
Yes, both vlans should be encoded in the data path.
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
