[ovs-dev] [PATCH] odp-util: Update ODPUTIL_FLOW_KEY_BYTES for current kernel flow format.

[Justin Pettit]

On May 15, 2012, at 9:58 AM, Ben Pfaff wrote:

> On Tue, May 15, 2012 at 09:40:46AM -0700, Justin Pettit wrote:
>> That's quite a packet.  The change looks reasonable to me--and 200
>> bytes seems adequate to cover that packet.  However, it might be worth
>> having Jesse double-check that that's the correct encapsulation in the
>> example
> 
> I am always in favor of more checking, especially by careful developers
> like Jesse, but perhaps I gave an incorrect impression that this value
> depends on kernel code?  It doesn't.  ODPUTIL_FLOW_KEY_BYTES is the
> maximum number of bytes that the userspace function
> odp_flow_key_from_flow() can write to a buffer.

Okay.  However, I thought that function depended on the number and type of attributes that the kernel used to represent a flow.  You and Jesse have the best understanding of what that interface is, and since this hypothetical flow is complicated, I suggested Jesse may want to look at it.  If you're satisfied that it's correct (or I'm misunderstanding something), I obviously trust your judgment.

> Actually that's inadequately documented.  I'm appending a revised patch
> that updates some comments too.

Great.  That's an improvement.

>> Do you think it's worth having the total be a #define, and then do a
>> build-time assertion that the total value is less than
>> ODPUTIL_FLOW_KEY_BYTES?
> 
> Can you explain how that would be different?

Right now, we have a chart that maps out the total number of bytes needed to represent the largest flow.  When updating that chart it's pretty obvious that the "total" field at the bottom needs to be updated.  I feel like we've had issues in the past where "total" was updated, but ODPUTIL_FLOW_KEY_BYTES was not.  This would serve as a reminder.  It's pretty unlikely to occur, but I was thinking it would be a good sanity-check.  We could also mandate a certain amount a slack.  However, if you think it's unnecessary, I'm fine not adding one, too.

--Justin