On Fri, Oct 25, 2013 at 10:25 AM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
>
> On Oct 25, 2013, at 9:27 AM, Jesse Gross <je...@nicira.com> wrote:
>
>> On Fri, Oct 25, 2013 at 9:13 AM, Jarno Rajahalme <jrajaha...@nicira.com>
>> wrote:
>>>
>>>> On Oct 25, 2013, at 9:06 AM, Jesse Gross <je...@nicira.com> wrote:
>>>>
>>>>> On Thu, Oct 24, 2013 at 9:01 AM, Jarno Rajahalme <jrajaha...@nicira.com>
>>>>> wrote:
>>>>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>>>>> index 515a9f6..fc6f42e 100644
>>>>> --- a/datapath/flow_netlink.c
>>>>> +++ b/datapath/flow_netlink.c
>>>>> @@ -154,8 +155,10 @@ static bool match_validate(const struct
>>>>> sw_flow_match *match,
>>>>>
>>>>> if (match->key->ip.proto == IPPROTO_TCP) {
>>>>> key_expected |= 1ULL << OVS_KEY_ATTR_TCP;
>>>>> - if (match->mask &&
>>>>> (match->mask->key.ip.proto == 0xff))
>>>>> + if (match->mask &&
>>>>> (match->mask->key.ip.proto == 0xff)) {
>>>>> mask_allowed |= 1ULL <<
>>>>> OVS_KEY_ATTR_TCP;
>>>>> + mask_allowed |= 1ULL <<
>>>>> OVS_KEY_ATTR_TCP_FLAGS;
>>>>
>>>> One thing that I should mention about this is that it doesn't require
>>>> the TCP_FLAGS key to be present if the protocol indicates that the
>>>> protocol is TCP. Requiring it would be the most consistent with the
>>>> rest of our netlink interface. However, I'm not sure that it's really
>>>> necessary any more now that we don't require exact match on all fields
>>>> with megaflows.
>>>
>>> I made it that way on purpose for compatibility with older userspaces.
>>> Maybe this is worth a comment, though.
>>
>> I don't think that it's necessary from a compatibility point of view
>> because older userspace will just echo back the fields that they
>> received. The protocol is supposed to be able to handle the addition
>> of new fields like this.
>>
>
> I see quite a many uses of odp_flow_key_from_flow() that would indicate that
> this not always a case. This is used, for example, when userspace wants to
> send a packet not originally received from the kernel.
I'm pretty sure that it works because this mismatch generally occurs
whenever someone uses OVS from the upstream kernel since the versions
are not tightly coupled.
If a packet is being sent to the kernel, then userspace only needs to
supply "metadata" - the fields not present in the packet itself, all
of which have reasonable defaults. The kernel will extract all fields
it can from the packet and use those as the source of truth.
Therefore, the addition of new fields only matters on flow setups
where userspace has historically needed to install an exact flow. In
this case, the compatibility mechanism is that userspace will extract
its notion of the flow from the packet itself and use that to compute
an action list. If the kernel's version contains fields that userspace
does not understand then it will just store and echo that back to the
kernel on the assumption that it's simply a more fined-grained version
of what it extracted.
>> This will kill performance on those versions though. I thought that I
>> had gone through it before and decided that this wouldn't be a problem
>> but I can't remember what I was thinking now.
>
> Can you be more specific? You mean that the allowance of omitting the TCP
> flags key causes performance problems, or the fact that kernel is reporting a
> key attribute that the userspace does not understand?
The problem is that old userspaces will not know how to mask out
certain TCP flag values and so it will treat every field as exact
match. Things will still be functionally correct due to the above
compatibility mechanism but it means that an old userspace running on
a new kernel will see a flow setup for every different combination of
TCP flags.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
