> On Aug 14, 2016, at 5:34 PM, Ben Pfaff <b...@ovn.org> wrote: > > On Mon, Aug 08, 2016 at 11:26:30AM -0700, Jarno Rajahalme wrote: >> Instead of storing the (big) struct ofputil_flow_mod, create the new >> rule and/or create the rule criteria for matching at bundle message >> insert time. This change reduces the size of a bundle flow mod from >> 3.5kb to 272 bytes, not counting the created rule, which was anyway >> created during bundle commit. >> >> In successful bundles this shifts work out of the ofproto_mutex >> critical section and should thus reduce the time the mutex is held >> during bundle commit. >> >> Signed-off-by: Jarno Rajahalme <ja...@ovn.org> >> --- >> v4: Fixed comments & memory leak. > > There's something really funny going on above add_flow_init(). It has > two function comments: >
I forgot to consolidate the comments and fixed my copy to read: /* add_flow_init(), add_flow_start(), add_flow_revert(), and add_flow_finish() * implement OFPFC_ADD and the cases for OFPFC_MODIFY and OFPFC_MODIFY_STRICT * in which no matching flow already exists in the flow table. * * add_flow_init() creates a new flow according to 'fm' and stores it to 'ofm' * for later reference. If the flow replaces other flow, it will be updated to * match modify semantics later by add_flow_start() (by calling * replace_rule_start()). * * Returns 0 on success, or an OpenFlow error code on failure. * * On successful return the caller must complete the operation by calling * add_flow_start(), and if that succeeds, then either add_flow_finish(), or * add_flow_revert() if the operation needs to be reverted due to a later * failure. */ With this the intent should be clear, or do you want me to issue a new version before you proceed with the review? Jarno > /* Implements OFPFC_ADD and the cases for OFPFC_MODIFY and OFPFC_MODIFY_STRICT > * in which no matching flow already exists in the flow table. > * > * Adds the flow specified by 'fm', to the ofproto's flow table. Returns 0 on > * success, or an OpenFlow error code on failure. > * > * On successful return the caller must complete the operation either by > * calling add_flow_finish(), or add_flow_revert() if the operation needs to > * be reverted. > * > * The caller retains ownership of 'fm->ofpacts'. */ > > /* Creates a new flow according to 'fm' and stores it to 'ofm' for later > * reference. If the flow replaces other flow, it will be updated to match > * modify semantics later. > * > * On successful return the caller must complete the operation by calling > * add_flow_start(), and if that succeeds, then either add_flow_finish(), or > * add_flow_revert() if the operation needs to be reverted due to a later > * failure. > */ > static enum ofperr > add_flow_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, > const struct ofputil_flow_mod *fm) > OVS_EXCLUDED(ofproto_mutex) > > Probably, this is an easy fix, but this code is complicated enough that > I'm reluctant to fully review it before the intent is clear from the > comments. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
