> On Aug 30, 2016, at 9:06 AM, Ben Pfaff <[email protected]> wrote: > > On Tue, Aug 23, 2016 at 05:51:37PM -0700, Jarno Rajahalme wrote: >> Change the value and mask to be added to the end of the set field >> action without any extra bytes, exept for the usual ofp-actions >> padding to 8 bytes. Together with some structure member packing this >> saves on average about to 256 bytes for each set field and load action >> (set field internal representation is also used for load actions). >> >> On a specific production data set each flow entry uses on average >> about 4.1 load and set field actions. This means that with this patch >> an average of more than 1kb can be saved for each flow with such a >> flow table. >> >> Signed-off-by: Jarno Rajahalme <[email protected]> > > Thanks for working on this! > > Building on 32-bit I get assertion failures without: > > diff --git a/include/openvswitch/ofp-actions.h > b/include/openvswitch/ofp-actions.h > index 9eaa78c..198eedd 100644 > --- a/include/openvswitch/ofp-actions.h > +++ b/include/openvswitch/ofp-actions.h > @@ -479,9 +479,11 @@ struct ofpact_stack { > * > * Used for NXAST_REG_LOAD and OFPAT12_SET_FIELD. */ > struct ofpact_set_field { > - struct ofpact ofpact; > - bool flow_has_vlan; /* VLAN present at action validation time. */ > - const struct mf_field *field; > + OFPACT_PADDED_MEMBERS( > + struct ofpact ofpact; > + bool flow_has_vlan; /* VLAN present at action validation time. */ > + const struct mf_field *field; > + ); > union mf_value value[]; /* Significant value bytes followed by > * significant mask bytes. */ > }; >
Thanks for checking the 32-bit build! Incremental applied. > I'd suggest ALIGNED_CAST in ofpact_set_field_mask() to make the > cast-to-void-then-to-real-type trick more greppable. > Done. > There's a number of instances of code similar to: > > + struct ofpact_set_field *sf = ofpact_put_set_field(ofpacts, mf); > + memcpy(sf->value, &sf_value, mf->n_bytes); > + memcpy(ofpact_set_field_mask(sf), &sf_mask, mf->n_bytes); > > so that it might be worthwhile to make a function that combines the > three steps (or just the memcpy()s?). > I added void * value and mask parameters to ofpact_put_set_field() to this effect. > Acked-by: Ben Pfaff <[email protected]> Thanks again for the review! Pushed to master. How about branch-2.6? Jarno _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
