[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