On Oct 17, 2013, at 12:37 PM, Ben Pfaff <b...@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 <jrajaha...@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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
