[ovs-dev] [#8024v2 2/5] Don't overload IP TOS with the frag matching bits.

[Jesse Gross]

On Tue, Nov 8, 2011 at 11:56 PM, Justin Pettit <jpettit at nicira.com> wrote:
> On Nov 8, 2011, at 8:12 PM, Jesse Gross wrote:
>
>> On Tue, Nov 8, 2011 at 3:57 PM, Justin Pettit <jpettit at nicira.com> wrote:
>>> diff --git a/datapath/flow.h b/datapath/flow.h
>>> index ba8c66a..f12b11a 100644
>>> --- a/datapath/flow.h
>>> +++ b/datapath/flow.h
>>> @@ -147,7 +143,7 @@ u64 flow_used_time(unsigned long flow_jiffies);
>>>  *  OVS_KEY_ATTR_ETHERNET     12    --     4     16
>>>  *  OVS_KEY_ATTR_8021Q         4    --     4      8
>>>  *  OVS_KEY_ATTR_ETHERTYPE     2     2     4      8
>>> - *  OVS_KEY_ATTR_IPV6         38     2     4     44
>>> + *  OVS_KEY_ATTR_IPV6         39     1     4     44
>>
>> I think this is just a mistake since this patch has nothing IPv6
>> specific in it but this list is for the userspace interface, which
>> this doesn't change.
>
> I don't understand this or your follow-up about it being incorrect before this patch.  Both the IPv4 and IPv6 "ipvX_tos" fields that expand the struct size by one.  Are you stating that it's inconsistent with "lib/odp-util.h"?  If so, then I've updated them to be in sync.

The IPv6 part was misleading but here's what I was trying to say:
This figure is about size of the buffer needed to store the
Netlink-formatted version of the flow key, not how it is actually
stored in userspace or kernel.  The frag type was never stored
together with ToS in Netlink so this unstuffing patch shouldn't change
anything.  However, since it wasn't increased when frag was added in
the first place, it was wrong beforehand.

>> In userspace there's a fair amount of masking with IP_DSCP_MASK even
>> though it is unnecessary because the userspace flow shouldn't contain
>> the ECN bits.  I'm guessing that it all gets resolved (or becomes
>> necessary) in the patch where ECN support is added, so it doesn't
>> really matter.
>
> Correct.  Or, at least, that's the hope.  :-)

Yeah, it looked right in the ECN patch.

>> I don't have any further comments beyond Ben's
>
>
> At the end of the message,  I've included an incremental that includes Ben's suggestions.

Looks good, thanks.