[ovs-dev] [PATCH 10/11] tunnel: Geneve TLV handling support for OpenFlow.

[Ben Pfaff]

On Wed, Jun 24, 2015 at 06:34:05PM -0700, Jesse Gross wrote:
> On Wed, Jun 24, 2015 at 3:57 PM, Ben Pfaff <blp at nicira.com> wrote:
> > On Fri, Jun 19, 2015 at 04:13:24PM -0700, Jesse Gross wrote:
> > I like the implementation approach used in ULONG_FOR_EACH_1 better than
> > the one in PRESENT_OPT_FOR_EACH, since the user doesn't have to provide
> > a scratch variable.  Also it protects the macro arguments better.
> > Actually ULONG_FOR_EACH_1 could be made to work with 32- or 64-bit maps
> > just by changing "unsigned long" to "uint64_t" in its definition; maybe
> > we should.
> 
> I looked at converting ULONG_FOR_EACH_1 but decided against it since
> it would introduce an additional branch (to decide whether to use
> __builtin_ctz or __builtin_ctz_ll) 

Really?  It shouldn't, "__builtin_constant_p(n <= UINT32_MAX) && n <=
UINT32_MAX" should be a compile-time constant.

> and it is used in places that are very careful about that kind thing,
> like the DPDK datapath. I just adopted the logic instead in
> PRESENT_OPT_FOR_EACH.

That's fine too.

> > Acked-by: Ben Pfaff <blp at nicira.com>
> 
> Thanks a lot for all of the reviews (it turns out userspace has become
> a lot more complicated since the last time that I did any serious work
> on it...). 

True, and it's unfortunate, and if you have any ideas for making it
simpler again I'm all ears!  I like simple.

> I'll wait a bit before pushing to look it over and give you a chance
> to object to anything I said. I have the patch for Geneve option
> support in the userspace datapath ready to go out once this in to
> complete the initial needs for OVN.

No objections here, apply it when you're satisfied.

I'll be really happy to be able to use this in OVN.