> On Aug 30, 2016, at 9:06 AM, Ben Pfaff <b...@ovn.org> 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 <ja...@ovn.org>
>
> 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 <b...@ovn.org>
Thanks again for the review!
Pushed to master. How about branch-2.6?
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
