[ovs-dev] [PATCH 5/9] Native OpenFlow 1.2 set_field action support.

[Jarno Rajahalme]

On Oct 17, 2013, at 12:37 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Wed, Oct 16, 2013 at 04:16:07PM -0700, Jarno Rajahalme wrote:
>> This is a little bit simpler and marginally more efficient, but also makes
>> following changes easier.
>> 
>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> 
> I had to read the patch before I understood what it did.  Could you
> elaborate the change log slightly?
> 
> Any particular reason to inline mf_from_id()?  I don't really object, I
> think, but rationale would be nice.  Also I don't see yet how this is
> related to the rest of the patch.  Could it be separated?

When I decided to use the enum instead of a pointer I decided to inline the mf_from_id to avoid any extra cost in action translation. It seems to me that the compiler should be able to completely optimize mf_from_id away in most cases.

I can easily separate this, no problem.

On the same note, we have a lot of heavily used single line/trivial utility functions that I think would benefit from inlining. Alternatively, LTO might be used for the same effect (?), but I have no experience with that so far. 

> 
> Because ofpacts are padded to a multiple of 8 bytes, and union mf_value
> is 16 bytes long, it looks to me like the trickery around struct
> ofpact_set_field could save at most 8 bytes, so that a struct
> ofpact_set_field is either 16 bytes or 24 bytes long when included in a
> series of struct ofpacts.  I doubt that's worth it.
> 

The next patch makes this a difference between 8 and 24 bytes (vs. 40 bytes (on 64-bit) for a set_field using the ofpact_reg_load).

> I didn't really review the whole patch, that's just from a skim.  I'll
> fully review it on the next go through the series.

OK.