[ovs-dev] [PATCH 6/7] datapath: Add tunnel header caching.

[Ben Pfaff]

On Wed, Sep 22, 2010 at 11:44:33AM -0700, Jesse Gross wrote:
> On Wed, Sep 22, 2010 at 10:44 AM, Ben Pfaff <blp at nicira.com> wrote:
> > My own plan, that I've had in the back of my mind for a while, is to
> > support deleting when memory allocation fails simply by writing NULL to
> > the pointer in the array.  The search code would then have to tolerate
> > NULLs, but that's probably a low cost.  (Another way would be to
> > duplicate one of the adjacent entries.  It would get hit twice on
> > searches, but it would be cache-hot the second time and we'd avoid a
> > test-and-branch on the fast path.)
> 
> This is interesting but I'm unsure of the benefits.  

A linked list-based design is probably better.  I hadn't considered
going back to one (before you mentioned it), so I was describing the
band-aid fix I had in mind for the array-based design.

> >> > In build_cache() it is too bad that we have to build up an skb just to
> >> > pass it to flow_extract() and free it.  Do you think it is expensive and
> >> > frequent enough to optimize?
> >>
> >> It is unfortunate but I'm not sure whether the benefit is high enough
> >> to justify the cost.  Building the cache is somewhat expensive and
> >> hopefully rare anyways.  I see three options:
> >> * Make the normal flow_extract() capable of handling this case.  I
> >> don't want to do that since it will make the common case slower to
> >> accommodate the rare case.
> >> * Have a second specialized function.  I'm afraid that it might
> >> diverge from the primary one over time, subtly breaking things.
> >> * Keep it the way it is.
> >
> > I don't think it's that important, so of those options "keep it the way
> > it is" is probably the best choice.
> >
> > Isn't the header basically fixed in form?  I would think that you could
> > easily write simple code in-line there to initialize odp_flow_key
> > properly.  Maybe you didn't want to maintain it as odp_flow_key changes.
> 
> This is actually the first thing that I did.  I realized later that
> since I was thinking about GRE that I had left out the transport port
> numbers and so it didn't work for CAPWAP.  This is easy to fix, of
> course, but the problem was that it would work correctly but silently
> lose the performance benefit.  I'm worried that something similar
> would happen if additional fields are added to the flow key,
> introducing non-obvious performance regressions.  It's easy to do
> either way though if this proves to be a problem.

Let's leave it the way you have it for now, then.