[ovs-dev] [RFC PATCH v2 3/7] flow: Always inline miniflows.

[Ben Pfaff]

On Wed, Jul 15, 2015 at 01:11:54PM -0700, Jarno Rajahalme wrote:
> 
> > On Jul 14, 2015, at 4:50 PM, Ben Pfaff <blp at 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 <jrajahalme at 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.