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

[Ben Pfaff]

I see the preprocessor expression 
        #if !defined(HAVE_HH_SEQ) || !defined(HAVE_RT_GENID)
appears a few times.  Should we make
a simpler macro out of it?

The clean_port_table() function doesn't do what I would expect, based on
the name.  I would have thought that "cleaning" was what happened every
5*HZ jiffies based on a timer.

The opposite of clean_port_table() is written out inline, twice, as:
        schedule_delayed_work(&cache_cleaner_wq, CACHE_CLEANER_INTERVAL);
Although it's only a one-liner, it might be better to have the two
behaviors paired, each as a function.

I notice that move_port() can fail after the vport is removed but before
it is re-added elsewhere.  Do we leak the vport in that case?

I think that the check_mtu() function will always store exactly 0 or
exactly htons(IP_DF) into *frag_offp.  Is that correct?  I think so, but
it seems possible that maybe it's wrong and that there should be a ~ in
this line:
> +		frag_off |= old_iph->frag_off & htons(IP_DF);

Should check_cache_valid() use time_before() instead of comparing
jiffies with <?

There's an odd blank line in the middle of cache_cleaner_cb().

It seems a little odd to have a spinlock that is only ever trylocked.
Makes me wonder whether some other synchronization primitive is more
appropriate.

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?

I had a hard time convincing myself that locking was OK in
cache_cleaner_cb().

Does check_headroom() cause double-accounting?  I think that the
destructor will be called for both new and old packets.

Can we really skip linearizing if the page refcounts are 1?  Hmm, I
guess so, since then we know that, say, the page cache doesn't have the
page.  (Does this optimization pay off in practice?)

There's a line
        };
in send_frag().  (Should just be "}".)

In tnl_send(), update_csum could be initialized to update_iph instead of
to false.  Also: isn't it quite likely that the checksum needs to be
updated, because the default tot_len fills the MTU, but there are plenty
of sub-MTU size packets?  Do you really think it is worthwhile to avoid
re-checksumming the IP header?  A checksum of the 20-byte IP header is
pretty cheap, and if you always did it you could skip a considerable
amount of bookkeeping.  Alternatively, it might be possible to compute a
partial checksum as part of the cache, and then always finish it off in
the loop after filling in the fields that can vary.

tnl_send() and send_frags() both have loops that free an skb and all of
the skbs chained to it.  I know I've seen this code elsewhere too.
Should we have a helper function?

In the comment in tunnel.h, to me the "first" bits of a number are
ambiguous.  I think you mean the "low" or "least-significant" bits, and
if so I'd prefer if you used one of those terms instead.

I'm surprised that there's no easy way to just ensure that struct
tnl_cache is padded out to a suitable alignment, instead of needing to
deal with that when we allocate it.