[ovs-dev] [RFC PATCH 4/4] lib/ofpbuf: Compact
Lori Jakab
lojakab at cisco.com
Wed Apr 2 18:07:37 UTC 2014
On 4/2/14, 9:09 PM, Jarno Rajahalme wrote:
>
> On Apr 1, 2014, at 1:28 PM, Lori Jakab <lojakab at cisco.com
> <mailto:lojakab at cisco.com>> wrote:
>
>> On 4/1/14, 5:43 PM, Jarno Rajahalme wrote:
>>>
>>> Sent from my iPhone
>>>
>>>> On Apr 1, 2014, at 7:27 AM, Ben Pfaff <blp at nicira.com
>>>> <mailto:blp at nicira.com>> wrote:
>>>>
>>>>> On Tue, Apr 01, 2014 at 03:14:16PM +0300, Lori Jakab wrote:
>>>> Can we do the following?
>>>> 3) Replace 'l2' by 'l2_ofs' also?
>>> I'll look into this today. However, my earlier analysis was that
>>> this would complicate things when the offset basis ('data') is
>>> changing a lot (e.g., when pulling data from ofpbuf). Maybe the
>>> cleanest solution would be to have a pointer to the start of packet
>>> and an additional l2 offset.
>>
>> Yes, that's the most elegant solution IMHO.
>
> I just sent out a patch that renames ‘l2’ as ‘frame’, but does not add
> an l2 offset, as it is not needed. We already have a condition, when
> the ‘frame’ is set, but there is no Ethernet header, which happens if
> the frame is too short to hold and ethernet frame. We have not checked
> for this in past as it has not happened in practice, but I added some
> checks for this. The new ofpbuf_get_l2() now returns NULL if the l3
> offset is not set.
>
> When you add support to non-Ethernet packets, you should add a
> condition “l3_ofs != 0” to the ofpbuf_get_l2(), so that it returns
> NULL if l3_ofs is zero.
>
> Do you have plans to support MPLS packets without Ethernet header? If
> yes, you will also need to take l2_5 offset into consideration.
>
>>
>>> However, as 0 is a valid offset value, having a separate l2 offset
>>> seems redundant as l3_ofs 0 would be an unambiguous way of encoding
>>> the absence of the l2 header.
>>
>> I think having 'l2' set to a value other then NULL for layer 3
>> packets can be confusing, even if l3_ofs is set to 0, but I don't
>> mind implementing L3 support that way.
>>
>
> Maybe this is less conferring after renaming ‘l2’ as ‘frame’. Also,
> the new ofpbuf_get_l2() is now used only when we explicitly want the
> address of the Ethernet frame. Other uses of ofpbuf (see the new
> comment in the patch) use ‘frame’ directly.
Thanks Jarno, I took a quick look at the patch and this is much better
for enabling layer 3 support.
>
>> I like Ben's proposal 3) better than 2), but I understand the
>> concerns about the basis changing a lot.
>>
>
> I actually tried to use ‘data’ as the basis, but is turned out to be
> impossible with out current practice of setting the ‘l2’ and ‘l3’ to
> the OpenFlow header and then pulling ‘data’ past it; the offsets would
> have become negative and each _pull and _push operation would have
> needed to update all the offsets.
Makes sense, thanks for the explanation.
-Lori
>
> Jarno
>
More information about the dev
mailing list