Thanks for the review, pushed to master.
Jarno
> On Jul 15, 2015, at 3:05 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> On Wed, Jul 15, 2015 at 01:11:54PM -0700, Jarno Rajahalme wrote:
>>
>>> On Jul 14, 2015, at 4:50 PM, Ben Pfaff <b...@nicira.com> wrote:
>>>
>>> On Fri, Jul 10, 2015 at 10:35:21AM -0700, Jarno Rajahalme wrote:
>>>> Now that performance critical code already inlines miniflows and
>>>> minimasks, we can simplify struct miniflow by always dynamically
>>>> allocating miniflows and minimasks to the correct size. This changes
>>>> the struct minimatch to always contain pointers to its miniflow and
>>>> minimask.
>>>>
>>>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>>>
>>> I like simplifying things so that two possibilities become one. It's
>>> nice.
>>>
>>> I see two possible ways to simplify this, each with its own tradeoffs:
>>>
>>> 1. The way that you did it, which means that one must always
>>> either dynamically allocate struct miniflow to a proper size
>>> or ensure that there's enough space for the maximum size flow
>>> immediately following struct miniflow.
>>>
>>> 2. Change struct miniflow to:
>>>
>>> struct miniflow {
>>> uint64_t map;
>>> uint64_t *values;
>>> };
>>>
>>> With this strategy, there's no need to dynamically allocate
>>> struct miniflow itself, so it can be directly embedded in
>>> larger structures instead of via pointer. The usual time cost
>>> of accessing miniflow data is again one pointer dereference
>>> (of 'values').
>>>
>>> This can be kind of nice because you're not fighting with the
>>> compiler to put data in the right place.
>>>
>>> Either way, the usual time cost of accessing miniflow data is one
>>> pointer dereference (in case #1, from whatever contains the miniflow; in
>>> case #2, dereference of 'values'), and the usual space cost of embedding
>>> a miniflow is one pointer (although in case #1 the pointer is in what
>>> contains the miniflow and in case #2 the pointer is in the miniflow
>>> itself).
>>>
>>> Anyway, I wanted to make sure that you considered both of these
>>> solutions and decided that #1 was better. If so:
>>>
>>
>> I agree that case #2 is syntactically nicer in the generic case, where
>> one pointer dereference is always needed. However, we already embed
>> miniflows to other data structures (flow keys and masks in classifier
>> and userspace datapath), where avoid the dereference and hence make
>> accessing the miniflow faster. With case #2 we’d need to define
>> another “struct inline_miniflow” (basically the case #1) to avoid the
>> dereference. IMO it is better to use the case #1 everywhere than
>> defining two different miniflow structures.
>
> OK, I'm glad that you considered the possibility then.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
