I just sent a v4, no need to review this one.
Jarno
> On Jul 29, 2016, at 6:37 PM, Jarno Rajahalme <ja...@ovn.org> 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. These can be uninitialized right after the start phase
> during the commit, which may also reduce the total memory needed
> during the bundle commit.
>
> Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
> ---
> v3: Reduced code duplication + use the template rule for the first
> modified instead of creating another rule for it.
>
> ofproto/bundles.c | 2 +-
> ofproto/bundles.h | 4 +-
> ofproto/ofproto-provider.h | 43 +++-
> ofproto/ofproto.c | 577 ++++++++++++++++++++++++++++-----------------
> tests/ofproto.at | 4 -
> 5 files changed, 400 insertions(+), 230 deletions(-)
>
> diff --git a/ofproto/bundles.c b/ofproto/bundles.c
> index aa8e58c..e8fb7ba 100644
> --- a/ofproto/bundles.c
> +++ b/ofproto/bundles.c
> @@ -66,7 +66,7 @@ ofp_bundle_remove__(struct ofconn *ofconn, struct
> ofp_bundle *bundle,
> LIST_FOR_EACH_POP (msg, node, &bundle->msg_list) {
> if (success && msg->type == OFPTYPE_FLOW_MOD) {
> /* Tell connmgr about successful flow mods. */
> - ofconn_report_flow_mod(ofconn, msg->ofm.fm.command);
> + ofconn_report_flow_mod(ofconn, msg->ofm.command);
> }
> ofp_bundle_entry_free(msg);
> }
> diff --git a/ofproto/bundles.h b/ofproto/bundles.h
> index 2054303..802de77 100644
> --- a/ofproto/bundles.h
> +++ b/ofproto/bundles.h
> @@ -36,7 +36,7 @@ struct ofp_bundle_entry {
> enum ofptype type; /* OFPTYPE_FLOW_MOD, OFPTYPE_PORT_MOD, or
> * OFPTYPE_GROUP_MOD. */
> union {
> - struct ofproto_flow_mod ofm; /* ofm.fm.ofpacts must be malloced. */
> + struct ofproto_flow_mod ofm;
> struct ofproto_port_mod opm;
> struct ofproto_group_mod ogm;
> };
> @@ -90,7 +90,7 @@ ofp_bundle_entry_free(struct ofp_bundle_entry *entry)
> {
> if (entry) {
> if (entry->type == OFPTYPE_FLOW_MOD) {
> - free(entry->ofm.fm.ofpacts);
> + ofproto_flow_mod_uninit(&entry->ofm);
> } else if (entry->type == OFPTYPE_GROUP_MOD) {
> ofputil_uninit_group_mod(&entry->ogm.gm);
> }
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 67a85e0..401d1b5 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1838,25 +1838,64 @@ extern const struct ofproto_class ofproto_dpif_class;
> int ofproto_class_register(const struct ofproto_class *);
> int ofproto_class_unregister(const struct ofproto_class *);
>
> +/* Criteria that flow_mod and other operations use for selecting rules on
> + * which to operate. */
> +struct rule_criteria {
> + /* An OpenFlow table or 255 for all tables. */
> + uint8_t table_id;
> +
> + /* OpenFlow matching criteria. Interpreted different in "loose" way by
> + * collect_rules_loose() and "strict" way by collect_rules_strict(), as
> + * defined in the OpenFlow spec. */
> + struct cls_rule cr;
> + ovs_version_t version;
> +
> + /* Matching criteria for the OpenFlow cookie. Consider a bit B in a
> rule's
> + * cookie and the corresponding bits C in 'cookie' and M in
> 'cookie_mask'.
> + * The rule will not be selected if M is 1 and B != C. */
> + ovs_be64 cookie;
> + ovs_be64 cookie_mask;
> +
> + /* Selection based on actions within a rule:
> + *
> + * If out_port != OFPP_ANY, selects only rules that output to out_port.
> + * If out_group != OFPG_ALL, select only rules that output to out_group.
> */
> + ofp_port_t out_port;
> + uint32_t out_group;
> +
> + /* If true, collects only rules that are modifiable. */
> + bool include_hidden;
> + bool include_readonly;
> +};
> +
> /* flow_mod with execution context. */
> struct ofproto_flow_mod {
> - struct ofputil_flow_mod fm;
> + /* Allocated by 'init' phase, may be freed after 'start' phase, as these
> + * are not needed for 'revert' nor 'finish'. */
> + struct rule *temp_rule;
> + struct rule_criteria criteria;
> + struct cls_conjunction *conjs;
> + size_t n_conjs;
>
> /* Replicate needed fields from ofputil_flow_mod to not need it after the
> * flow has been created. */
> uint16_t command;
> uint32_t buffer_id;
> bool modify_cookie;
> -
> + /* Fields derived from ofputil_flow_mod. */
> bool modify_may_add_flow;
> enum nx_flow_update_event event;
>
> + /* These are only used during commit execution.
> + * ofproto_flow_mod_uninit() does NOT clean these up. */
> ovs_version_t version; /* Version in which changes take
> * effect. */
> struct rule_collection old_rules; /* Affected rules. */
> struct rule_collection new_rules; /* Replacement rules. */
> };
>
> +void ofproto_flow_mod_uninit(struct ofproto_flow_mod *);
> +
> /* port_mod with execution context. */
> struct ofproto_port_mod {
> struct ofputil_port_mod pm;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 8e59c69..0a5383b 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -129,36 +129,6 @@ static void eviction_group_add_rule(struct rule *)
> static void eviction_group_remove_rule(struct rule *)
> OVS_REQUIRES(ofproto_mutex);
>
> -/* Criteria that flow_mod and other operations use for selecting rules on
> - * which to operate. */
> -struct rule_criteria {
> - /* An OpenFlow table or 255 for all tables. */
> - uint8_t table_id;
> -
> - /* OpenFlow matching criteria. Interpreted different in "loose" way by
> - * collect_rules_loose() and "strict" way by collect_rules_strict(), as
> - * defined in the OpenFlow spec. */
> - struct cls_rule cr;
> - ovs_version_t version;
> -
> - /* Matching criteria for the OpenFlow cookie. Consider a bit B in a
> rule's
> - * cookie and the corresponding bits C in 'cookie' and M in
> 'cookie_mask'.
> - * The rule will not be selected if M is 1 and B != C. */
> - ovs_be64 cookie;
> - ovs_be64 cookie_mask;
> -
> - /* Selection based on actions within a rule:
> - *
> - * If out_port != OFPP_ANY, selects only rules that output to out_port.
> - * If out_group != OFPG_ALL, select only rules that output to out_group.
> */
> - ofp_port_t out_port;
> - uint32_t out_group;
> -
> - /* If true, collects only rules that are modifiable. */
> - bool include_hidden;
> - bool include_readonly;
> -};
> -
> static void rule_criteria_init(struct rule_criteria *, uint8_t table_id,
> const struct match *match, int priority,
> ovs_version_t version,
> @@ -264,17 +234,19 @@ struct openflow_mod_requester {
> };
>
> /* OpenFlow. */
> -static enum ofperr replace_rule_create(struct ofproto *,
> - struct ofproto_flow_mod *ofm,
> - const struct ofputil_flow_mod *,
> - struct cls_rule *cr, uint8_t table_id,
> - struct rule *old_rule,
> +static enum ofperr ofproto_rule_create(struct ofproto *, struct cls_rule *,
> + uint8_t table_id, ovs_be64 new_cookie,
> + uint16_t idle_timeout,
> + uint16_t hard_timeout,
> + enum ofputil_flow_mod_flags flags,
> + uint16_t importance,
> + const struct ofpact *ofpacts,
> + size_t ofpacts_len,
> struct rule **new_rule)
> - OVS_REQUIRES(ofproto_mutex);
> + OVS_NO_THREAD_SAFETY_ANALYSIS;
>
> -static void replace_rule_start(struct ofproto *, ovs_version_t version,
> - struct rule *old_rule, struct rule *new_rule,
> - struct cls_conjunction *, size_t n_conjs)
> +static void replace_rule_start(struct ofproto *, struct ofproto_flow_mod *,
> + struct rule *old_rule, struct rule *new_rule)
> OVS_REQUIRES(ofproto_mutex);
>
> static void replace_rule_revert(struct ofproto *, struct rule *old_rule,
> @@ -297,6 +269,10 @@ static void send_buffered_packet(const struct
> openflow_mod_requester *,
>
> static bool ofproto_group_exists(const struct ofproto *, uint32_t group_id);
> static void handle_openflow(struct ofconn *, const struct ofpbuf *);
> +static enum ofperr ofproto_flow_mod_init(struct ofproto *,
> + struct ofproto_flow_mod *,
> + const struct ofputil_flow_mod *fm)
> + OVS_EXCLUDED(ofproto_mutex);
> static enum ofperr ofproto_flow_mod_start(struct ofproto *,
> struct ofproto_flow_mod *)
> OVS_REQUIRES(ofproto_mutex);
> @@ -2196,8 +2172,7 @@ ofproto_flow_mod(struct ofproto *ofproto, const struct
> ofputil_flow_mod *fm)
> OVS_VERSION_MAX));
> if (rule) {
> /* Reading many of the rule fields and writing on 'modified'
> - * requires the rule->mutex. Also, rule->actions may change
> - * if rule->mutex is not held. */
> + * requires the rule->mutex. */
> const struct rule_actions *actions;
>
> ovs_mutex_lock(&rule->mutex);
> @@ -4073,6 +4048,7 @@ static void
> rule_criteria_destroy(struct rule_criteria *criteria)
> {
> cls_rule_destroy(&criteria->cr);
> + criteria->version = OVS_VERSION_NOT_REMOVED; /* Mark as destroyed. */
> }
>
> /* Schedules postponed removal of rules, destroys 'rules'. */
> @@ -4627,7 +4603,6 @@ evict_rules_from_table(struct oftable *table)
> static void
> get_conjunctions(const struct ofputil_flow_mod *fm,
> struct cls_conjunction **conjsp, size_t *n_conjsp)
> - OVS_REQUIRES(ofproto_mutex)
> {
> struct cls_conjunction *conjs = NULL;
> int n_conjs = 0;
> @@ -4673,24 +4648,28 @@ get_conjunctions(const struct ofputil_flow_mod *fm,
> * 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_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
> - OVS_REQUIRES(ofproto_mutex)
> +add_flow_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
> + const struct ofputil_flow_mod *fm)
> + OVS_EXCLUDED(ofproto_mutex)
> {
> - struct ofputil_flow_mod *fm = &ofm->fm;
> - struct rule **old_rule = rule_collection_stub(&ofm->old_rules);
> - struct rule **new_rule = rule_collection_stub(&ofm->new_rules);
> struct oftable *table;
> struct cls_rule cr;
> - struct rule *rule;
> uint8_t table_id;
> - struct cls_conjunction *conjs;
> - size_t n_conjs;
> enum ofperr error;
>
> if (!check_table_id(ofproto, fm->table_id)) {
> - error = OFPERR_OFPBRC_BAD_TABLE_ID;
> - return error;
> + return OFPERR_OFPBRC_BAD_TABLE_ID;
> }
>
> /* Pick table. */
> @@ -4727,47 +4706,71 @@ add_flow_start(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm)
>
> cls_rule_init(&cr, &fm->match, fm->priority);
>
> + /* Allocate new rule. Destroys 'cr'. */
> + error = ofproto_rule_create(ofproto, &cr, table - ofproto->tables,
> + fm->new_cookie, fm->idle_timeout,
> + fm->hard_timeout, fm->flags, fm->importance,
> + fm->ofpacts, fm->ofpacts_len,
> &ofm->temp_rule);
> + if (error) {
> + return error;
> + }
> +
> + get_conjunctions(fm, &ofm->conjs, &ofm->n_conjs);
> + return 0;
> +}
> +
> +static enum ofperr
> +add_flow_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
> + OVS_REQUIRES(ofproto_mutex)
> +{
> + struct rule *old_rule = NULL;
> + struct rule *new_rule = ofm->temp_rule;
> + const struct rule_actions *actions = rule_get_actions(new_rule);
> + struct oftable *table = &ofproto->tables[new_rule->table_id];
> + enum ofperr error;
> +
> + /* Must check actions while holding ofproto_mutex to avoid a race. */
> + error = ofproto_check_ofpacts(ofproto, actions->ofpacts,
> + actions->ofpacts_len);
> + if (error) {
> + return error;
> + }
> +
> /* Check for the existence of an identical rule.
> * This will not return rules earlier marked for removal. */
> - rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls, &cr,
> - ofm->version));
> - *old_rule = rule;
> - if (!rule) {
> + old_rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls,
> + &new_rule->cr,
> +
> ofm->version));
> + if (!old_rule) {
> /* Check for overlap, if requested. */
> - if (fm->flags & OFPUTIL_FF_CHECK_OVERLAP
> - && classifier_rule_overlaps(&table->cls, &cr, ofm->version)) {
> - cls_rule_destroy(&cr);
> + if (new_rule->flags & OFPUTIL_FF_CHECK_OVERLAP
> + && classifier_rule_overlaps(&table->cls, &new_rule->cr,
> + ofm->version)) {
> return OFPERR_OFPFMFC_OVERLAP;
> }
>
> /* If necessary, evict an existing rule to clear out space. */
> if (table->n_flows >= table->max_flows) {
> - if (!choose_rule_to_evict(table, &rule)) {
> - error = OFPERR_OFPFMFC_TABLE_FULL;
> - cls_rule_destroy(&cr);
> - return error;
> + if (!choose_rule_to_evict(table, &old_rule)) {
> + return OFPERR_OFPFMFC_TABLE_FULL;
> }
> - eviction_group_remove_rule(rule);
> - /* Marks '*old_rule' as an evicted rule rather than replaced
> rule.
> + eviction_group_remove_rule(old_rule);
> + /* Marks 'old_rule' as an evicted rule rather than replaced rule.
> */
> - rule->removed_reason = OFPRR_EVICTION;
> - *old_rule = rule;
> + old_rule->removed_reason = OFPRR_EVICTION;
> }
> } else {
> ofm->modify_cookie = true;
> }
>
> - /* Allocate new rule. */
> - error = replace_rule_create(ofproto, ofm, fm, &cr, table -
> ofproto->tables,
> - rule, new_rule);
> - if (error) {
> - return error;
> + if (old_rule) {
> + rule_collection_add(&ofm->old_rules, old_rule);
> }
> + /* Take ownership of the temp_rule. */
> + rule_collection_stub(&ofm->new_rules)[0] = new_rule;
> + ofm->temp_rule = NULL;
>
> - get_conjunctions(fm, &conjs, &n_conjs);
> - replace_rule_start(ofproto, ofm->version, rule, *new_rule, conjs,
> n_conjs);
> - free(conjs);
> -
> + replace_rule_start(ofproto, ofm, old_rule, new_rule);
> return 0;
> }
>
> @@ -4776,14 +4779,10 @@ static void
> add_flow_revert(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
> OVS_REQUIRES(ofproto_mutex)
> {
> - struct rule *old_rule = rule_collection_stub(&ofm->old_rules)[0];
> + struct rule *old_rule = rule_collection_n(&ofm->old_rules)
> + ? rule_collection_stub(&ofm->old_rules)[0] : NULL;
> struct rule *new_rule = rule_collection_stub(&ofm->new_rules)[0];
>
> - if (old_rule && old_rule->removed_reason == OFPRR_EVICTION) {
> - /* Revert the eviction. */
> - eviction_group_add_rule(old_rule);
> - }
> -
> replace_rule_revert(ofproto, old_rule, new_rule);
> }
>
> @@ -4793,7 +4792,8 @@ add_flow_finish(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
> const struct openflow_mod_requester *req)
> OVS_REQUIRES(ofproto_mutex)
> {
> - struct rule *old_rule = rule_collection_stub(&ofm->old_rules)[0];
> + struct rule *old_rule = rule_collection_n(&ofm->old_rules)
> + ? rule_collection_stub(&ofm->old_rules)[0] : NULL;
> struct rule *new_rule = rule_collection_stub(&ofm->new_rules)[0];
> struct ovs_list dead_cookies = OVS_LIST_INITIALIZER(&dead_cookies);
>
> @@ -4816,14 +4816,16 @@ add_flow_finish(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
>
> /* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */
>
> -/* Create a new rule based on attributes in 'fm', match in 'cr', 'table_id',
> - * and 'old_rule'. Note that the rule is NOT inserted into a any data
> +/* Create a new rule. Note that the rule is NOT inserted into a any data
> * structures yet. Takes ownership of 'cr'. */
> static enum ofperr
> -replace_rule_create(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
> - const struct ofputil_flow_mod *fm,
> - struct cls_rule *cr, uint8_t table_id,
> - struct rule *old_rule, struct rule **new_rule)
> +ofproto_rule_create(struct ofproto *ofproto, struct cls_rule *cr,
> + uint8_t table_id, ovs_be64 new_cookie,
> + uint16_t idle_timeout, uint16_t hard_timeout,
> + enum ofputil_flow_mod_flags flags, uint16_t importance,
> + const struct ofpact *ofpacts, size_t ofpacts_len,
> + struct rule **new_rule)
> + OVS_NO_THREAD_SAFETY_ANALYSIS
> {
> struct rule *rule;
> enum ofperr error;
> @@ -4840,46 +4842,28 @@ replace_rule_create(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
> *CONST_CAST(struct ofproto **, &rule->ofproto) = ofproto;
> cls_rule_move(CONST_CAST(struct cls_rule *, &rule->cr), cr);
> ovs_refcount_init(&rule->ref_count);
> - rule->flow_cookie = fm->new_cookie;
> - rule->created = rule->modified = time_msec();
>
> ovs_mutex_init(&rule->mutex);
> ovs_mutex_lock(&rule->mutex);
> - rule->idle_timeout = fm->idle_timeout;
> - rule->hard_timeout = fm->hard_timeout;
> - *CONST_CAST(uint16_t *, &rule->importance) = fm->importance;
> + rule->flow_cookie = new_cookie;
> + rule->created = rule->modified = time_msec();
> + rule->idle_timeout = idle_timeout;
> + rule->hard_timeout = hard_timeout;
> + *CONST_CAST(uint16_t *, &rule->importance) = importance;
> rule->removed_reason = OVS_OFPRR_NONE;
>
> *CONST_CAST(uint8_t *, &rule->table_id) = table_id;
> - rule->flags = fm->flags & OFPUTIL_FF_STATE;
> + rule->flags = flags & OFPUTIL_FF_STATE;
> +
> *CONST_CAST(const struct rule_actions **, &rule->actions)
> - = rule_actions_create(fm->ofpacts, fm->ofpacts_len);
> + = rule_actions_create(ofpacts, ofpacts_len);
> +
> ovs_list_init(&rule->meter_list_node);
> rule->eviction_group = NULL;
> - ovs_list_init(&rule->expirable);
> rule->monitor_flags = 0;
> rule->add_seqno = 0;
> rule->modify_seqno = 0;
> -
> - /* Copy values from old rule for modify semantics. */
> - if (old_rule && old_rule->removed_reason != OFPRR_EVICTION) {
> - bool change_cookie = (ofm->modify_cookie
> - && fm->new_cookie != OVS_BE64_MAX
> - && fm->new_cookie != old_rule->flow_cookie);
> -
> - ovs_mutex_lock(&old_rule->mutex);
> - if (fm->command != OFPFC_ADD) {
> - rule->idle_timeout = old_rule->idle_timeout;
> - rule->hard_timeout = old_rule->hard_timeout;
> - *CONST_CAST(uint16_t *, &rule->importance) =
> old_rule->importance;
> - rule->flags = old_rule->flags;
> - rule->created = old_rule->created;
> - }
> - if (!change_cookie) {
> - rule->flow_cookie = old_rule->flow_cookie;
> - }
> - ovs_mutex_unlock(&old_rule->mutex);
> - }
> + ovs_list_init(&rule->expirable);
> ovs_mutex_unlock(&rule->mutex);
>
> /* Construct rule, initializing derived state. */
> @@ -4896,16 +4880,37 @@ replace_rule_create(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
> }
>
> static void
> -replace_rule_start(struct ofproto *ofproto, ovs_version_t version,
> - struct rule *old_rule, struct rule *new_rule,
> - struct cls_conjunction *conjs, size_t n_conjs)
> +replace_rule_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
> + struct rule *old_rule, struct rule *new_rule)
> {
> struct oftable *table = &ofproto->tables[new_rule->table_id];
>
> /* 'old_rule' may be either an evicted rule or replaced rule. */
> if (old_rule) {
> + /* Copy values from old rule for modify semantics. */
> + if (old_rule->removed_reason != OFPRR_EVICTION) {
> + bool change_cookie = (ofm->modify_cookie
> + && new_rule->flow_cookie != OVS_BE64_MAX
> + && new_rule->flow_cookie !=
> old_rule->flow_cookie);
> +
> + ovs_mutex_lock(&new_rule->mutex);
> + ovs_mutex_lock(&old_rule->mutex);
> + if (ofm->command != OFPFC_ADD) {
> + new_rule->idle_timeout = old_rule->idle_timeout;
> + new_rule->hard_timeout = old_rule->hard_timeout;
> + *CONST_CAST(uint16_t *, &new_rule->importance) =
> old_rule->importance;
> + new_rule->flags = old_rule->flags;
> + new_rule->created = old_rule->created;
> + }
> + if (!change_cookie) {
> + new_rule->flow_cookie = old_rule->flow_cookie;
> + }
> + ovs_mutex_unlock(&old_rule->mutex);
> + ovs_mutex_unlock(&new_rule->mutex);
> + }
> +
> /* Mark the old rule for removal in the next version. */
> - cls_rule_make_invisible_in_version(&old_rule->cr, version);
> + cls_rule_make_invisible_in_version(&old_rule->cr, ofm->version);
>
> /* Remove the old rule from data structures. */
> ofproto_rule_remove__(ofproto, old_rule);
> @@ -4918,7 +4923,8 @@ replace_rule_start(struct ofproto *ofproto,
> ovs_version_t version,
> ofproto_rule_insert__(ofproto, new_rule);
> /* Make the new rule visible for classifier lookups only from the next
> * version. */
> - classifier_insert(&table->cls, &new_rule->cr, version, conjs, n_conjs);
> + classifier_insert(&table->cls, &new_rule->cr, ofm->version, ofm->conjs,
> + ofm->n_conjs);
> }
>
> static void
> @@ -4928,6 +4934,11 @@ replace_rule_revert(struct ofproto *ofproto,
> struct oftable *table = &ofproto->tables[new_rule->table_id];
>
> if (old_rule) {
> + if (old_rule->removed_reason == OFPRR_EVICTION) {
> + /* Revert the eviction. */
> + eviction_group_add_rule(old_rule);
> + }
> +
> /* Restore the old rule to data structures. */
> ofproto_rule_insert__(ofproto, old_rule);
>
> @@ -4978,6 +4989,9 @@ replace_rule_finish(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
> learned_cookies_dec(ofproto, old_actions, dead_cookies);
>
> if (replaced_rule) {
> + enum nx_flow_update_event event = ofm->command == OFPFC_ADD
> + ? NXFME_ADDED : NXFME_MODIFIED;
> +
> bool changed_cookie = (new_rule->flow_cookie
> != old_rule->flow_cookie);
>
> @@ -4986,9 +5000,9 @@ replace_rule_finish(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
> old_actions->ofpacts,
> old_actions->ofpacts_len);
>
> - if (ofm->event != NXFME_MODIFIED || changed_actions
> + if (event != NXFME_MODIFIED || changed_actions
> || changed_cookie) {
> - ofmonitor_report(ofproto->connmgr, new_rule, ofm->event, 0,
> + ofmonitor_report(ofproto->connmgr, new_rule, event, 0,
> req ? req->ofconn : NULL,
> req ? req->request->xid : 0,
> changed_actions ? old_actions : NULL);
> @@ -5003,51 +5017,72 @@ replace_rule_finish(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
> }
> }
>
> +/* 'ofm->new_rules' has the template rule, which this function will consume.
> */
> static enum ofperr
> modify_flows_start__(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
> OVS_REQUIRES(ofproto_mutex)
> {
> - struct ofputil_flow_mod *fm = &ofm->fm;
> struct rule_collection *old_rules = &ofm->old_rules;
> struct rule_collection *new_rules = &ofm->new_rules;
> enum ofperr error;
>
> - rule_collection_init(new_rules);
> -
> if (rule_collection_n(old_rules) > 0) {
> - struct cls_conjunction *conjs;
> - size_t n_conjs;
> -
> /* Create a new 'modified' rule for each old rule. */
> - struct rule *old_rule;
> - RULE_COLLECTION_FOR_EACH (old_rule, old_rules) {
> - struct rule *new_rule;
> - struct cls_rule cr;
> + struct rule *old_rule, *new_rule;
> + const struct rule_actions *actions =
> rule_get_actions(ofm->temp_rule);
>
> - cls_rule_clone(&cr, &old_rule->cr);
> - error = replace_rule_create(ofproto, ofm, fm, &cr,
> - old_rule->table_id, old_rule,
> - &new_rule);
> - if (!error) {
> - rule_collection_add(new_rules, new_rule);
> + /* Must check actions while holding ofproto_mutex to avoid a race. */
> + error = ofproto_check_ofpacts(ofproto, actions->ofpacts,
> + actions->ofpacts_len);
> + if (error) {
> + return error;
> + }
> +
> + /* Use the temp rule as the first new rule, and as the template for
> + * the rest. */
> + struct rule *temp = ofm->temp_rule;
> + ofm->temp_rule = NULL; /* We consume the template. */
> +
> + bool first = true;
> + RULE_COLLECTION_FOR_EACH (old_rule, old_rules) {
> + if (first) {
> + /* The template rule's match is possibly a loose one, so it
> + * must be replaced with the old rule's match so that the new
> + * rule actually replaces the old one. */
> + cls_rule_destroy(CONST_CAST(struct cls_rule *, &temp->cr));
> + cls_rule_clone(CONST_CAST(struct cls_rule *, &temp->cr),
> + &old_rule->cr);
> + *CONST_CAST(uint8_t *, &temp->table_id) = old_rule->table_id;
> + rule_collection_add(new_rules, temp);
> + first = false;
> } else {
> - rule_collection_unref(new_rules);
> - rule_collection_destroy(new_rules);
> - return error;
> + struct cls_rule cr;
> + cls_rule_clone(&cr, &old_rule->cr);
> + error = ofproto_rule_create(ofproto, &cr, old_rule->table_id,
> + temp->flow_cookie,
> + temp->idle_timeout,
> + temp->hard_timeout, temp->flags,
> + temp->importance,
> + temp->actions->ofpacts,
> + temp->actions->ofpacts_len,
> + &new_rule);
> + if (!error) {
> + rule_collection_add(new_rules, new_rule);
> + } else {
> + rule_collection_unref(new_rules);
> + rule_collection_destroy(new_rules);
> + return error;
> + }
> }
> }
> ovs_assert(rule_collection_n(new_rules)
> == rule_collection_n(old_rules));
>
> - get_conjunctions(fm, &conjs, &n_conjs);
> - struct rule *new_rule;
> RULE_COLLECTIONS_FOR_EACH (old_rule, new_rule, old_rules, new_rules) {
> - replace_rule_start(ofproto, ofm->version, old_rule, new_rule,
> - conjs, n_conjs);
> + replace_rule_start(ofproto, ofm, old_rule, new_rule);
> }
> - free(conjs);
> } else if (ofm->modify_may_add_flow) {
> - /* No match, add a new flow. */
> + /* No match, add a new flow, consumes 'temp'. */
> error = add_flow_start(ofproto, ofm);
> new_rules->collection.n = 1;
> } else {
> @@ -5057,6 +5092,24 @@ modify_flows_start__(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm)
> return error;
> }
>
> +static enum ofperr
> +modify_flows_init_loose(struct ofproto *ofproto,
> + struct ofproto_flow_mod *ofm,
> + const struct ofputil_flow_mod *fm)
> + OVS_EXCLUDED(ofproto_mutex)
> +{
> + rule_criteria_init(&ofm->criteria, fm->table_id, &fm->match, 0,
> + OVS_VERSION_MAX, fm->cookie, fm->cookie_mask,
> OFPP_ANY,
> + OFPG_ANY);
> + rule_criteria_require_rw(&ofm->criteria,
> + (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
> + /* Must create a new flow in advance for the case that no matches are
> + * found. Also used for template for multiple modified flows. */
> + add_flow_init(ofproto, ofm, fm);
> +
> + return 0;
> +}
> +
> /* Implements OFPFC_MODIFY. Returns 0 on success or an OpenFlow error code on
> * failure.
> *
> @@ -5066,17 +5119,10 @@ static enum ofperr
> modify_flows_start_loose(struct ofproto *ofproto, struct ofproto_flow_mod
> *ofm)
> OVS_REQUIRES(ofproto_mutex)
> {
> - struct ofputil_flow_mod *fm = &ofm->fm;
> struct rule_collection *old_rules = &ofm->old_rules;
> - struct rule_criteria criteria;
> enum ofperr error;
>
> - rule_criteria_init(&criteria, fm->table_id, &fm->match, 0,
> OVS_VERSION_MAX,
> - fm->cookie, fm->cookie_mask, OFPP_ANY, OFPG_ANY);
> - rule_criteria_require_rw(&criteria,
> - (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
> - error = collect_rules_loose(ofproto, &criteria, old_rules);
> - rule_criteria_destroy(&criteria);
> + error = collect_rules_loose(ofproto, &ofm->criteria, old_rules);
>
> if (!error) {
> error = modify_flows_start__(ofproto, ofm);
> @@ -5085,6 +5131,7 @@ modify_flows_start_loose(struct ofproto *ofproto,
> struct ofproto_flow_mod *ofm)
> if (error) {
> rule_collection_destroy(old_rules);
> }
> +
> return error;
> }
>
> @@ -5096,10 +5143,7 @@ modify_flows_revert(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm)
> struct rule_collection *new_rules = &ofm->new_rules;
>
> /* Old rules were not changed yet, only need to revert new rules. */
> - if (rule_collection_n(old_rules) == 0
> - && rule_collection_n(new_rules) == 1) {
> - add_flow_revert(ofproto, ofm);
> - } else if (rule_collection_n(old_rules) > 0) {
> + if (rule_collection_n(old_rules) > 0) {
> struct rule *old_rule, *new_rule;
> RULE_COLLECTIONS_FOR_EACH (old_rule, new_rule, old_rules, new_rules) {
> replace_rule_revert(ofproto, old_rule, new_rule);
> @@ -5140,33 +5184,40 @@ modify_flows_finish(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm,
> }
> }
>
> +static enum ofperr
> +modify_flow_init_strict(struct ofproto *ofproto OVS_UNUSED,
> + struct ofproto_flow_mod *ofm,
> + const struct ofputil_flow_mod *fm)
> + OVS_EXCLUDED(ofproto_mutex)
> +{
> + rule_criteria_init(&ofm->criteria, fm->table_id, &fm->match,
> fm->priority,
> + OVS_VERSION_MAX, fm->cookie, fm->cookie_mask,
> OFPP_ANY,
> + OFPG_ANY);
> + rule_criteria_require_rw(&ofm->criteria,
> + (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
> + /* Must create a new flow in advance for the case that no matches are
> + * found. Also used for template for multiple modified flows. */
> + add_flow_init(ofproto, ofm, fm);
> +
> + return 0;
> +}
> +
> /* Implements OFPFC_MODIFY_STRICT. Returns 0 on success or an OpenFlow error
> * code on failure. */
> static enum ofperr
> modify_flow_start_strict(struct ofproto *ofproto, struct ofproto_flow_mod
> *ofm)
> OVS_REQUIRES(ofproto_mutex)
> {
> - struct ofputil_flow_mod *fm = &ofm->fm;
> struct rule_collection *old_rules = &ofm->old_rules;
> - struct rule_criteria criteria;
> enum ofperr error;
>
> - rule_criteria_init(&criteria, fm->table_id, &fm->match, fm->priority,
> - OVS_VERSION_MAX, fm->cookie, fm->cookie_mask,
> OFPP_ANY,
> - OFPG_ANY);
> - rule_criteria_require_rw(&criteria,
> - (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
> - error = collect_rules_strict(ofproto, &criteria, old_rules);
> - rule_criteria_destroy(&criteria);
> + error = collect_rules_strict(ofproto, &ofm->criteria, old_rules);
>
> if (!error) {
> /* collect_rules_strict() can return max 1 rule. */
> error = modify_flows_start__(ofproto, ofm);
> }
>
> - if (error) {
> - rule_collection_destroy(old_rules);
> - }
> return error;
> }
>
> @@ -5262,23 +5313,29 @@ delete_flows__(struct rule_collection *rules,
> }
> }
>
> +static enum ofperr
> +delete_flows_init_loose(struct ofproto *ofproto OVS_UNUSED,
> + struct ofproto_flow_mod *ofm,
> + const struct ofputil_flow_mod *fm)
> + OVS_EXCLUDED(ofproto_mutex)
> +{
> + rule_criteria_init(&ofm->criteria, fm->table_id, &fm->match, 0,
> + OVS_VERSION_MAX, fm->cookie, fm->cookie_mask,
> + fm->out_port, fm->out_group);
> + rule_criteria_require_rw(&ofm->criteria,
> + (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
> + return 0;
> +}
> +
> /* Implements OFPFC_DELETE. */
> static enum ofperr
> delete_flows_start_loose(struct ofproto *ofproto, struct ofproto_flow_mod
> *ofm)
> OVS_REQUIRES(ofproto_mutex)
> {
> - const struct ofputil_flow_mod *fm = &ofm->fm;
> struct rule_collection *rules = &ofm->old_rules;
> - struct rule_criteria criteria;
> enum ofperr error;
>
> - rule_criteria_init(&criteria, fm->table_id, &fm->match, 0,
> OVS_VERSION_MAX,
> - fm->cookie, fm->cookie_mask, fm->out_port,
> - fm->out_group);
> - rule_criteria_require_rw(&criteria,
> - (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
> - error = collect_rules_loose(ofproto, &criteria, rules);
> - rule_criteria_destroy(&criteria);
> + error = collect_rules_loose(ofproto, &ofm->criteria, rules);
>
> if (!error) {
> delete_flows_start__(ofproto, ofm->version, rules);
> @@ -5292,7 +5349,6 @@ delete_flows_revert(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm)
> OVS_REQUIRES(ofproto_mutex)
> {
> delete_flows_revert__(ofproto, &ofm->old_rules);
> - rule_collection_destroy(&ofm->old_rules);
> }
>
> static void
> @@ -5304,24 +5360,30 @@ delete_flows_finish(struct ofproto *ofproto,
> delete_flows_finish__(ofproto, &ofm->old_rules, OFPRR_DELETE, req);
> }
>
> +static enum ofperr
> +delete_flows_init_strict(struct ofproto *ofproto OVS_UNUSED,
> + struct ofproto_flow_mod *ofm,
> + const struct ofputil_flow_mod *fm)
> + OVS_EXCLUDED(ofproto_mutex)
> +{
> + rule_criteria_init(&ofm->criteria, fm->table_id, &fm->match,
> fm->priority,
> + OVS_VERSION_MAX, fm->cookie, fm->cookie_mask,
> + fm->out_port, fm->out_group);
> + rule_criteria_require_rw(&ofm->criteria,
> + (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
> + return 0;
> +}
> +
> /* Implements OFPFC_DELETE_STRICT. */
> static enum ofperr
> delete_flow_start_strict(struct ofproto *ofproto,
> struct ofproto_flow_mod *ofm)
> OVS_REQUIRES(ofproto_mutex)
> {
> - const struct ofputil_flow_mod *fm = &ofm->fm;
> struct rule_collection *rules = &ofm->old_rules;
> - struct rule_criteria criteria;
> enum ofperr error;
>
> - rule_criteria_init(&criteria, fm->table_id, &fm->match, fm->priority,
> - OVS_VERSION_MAX, fm->cookie, fm->cookie_mask,
> - fm->out_port, fm->out_group);
> - rule_criteria_require_rw(&criteria,
> - (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
> - error = collect_rules_strict(ofproto, &criteria, rules);
> - rule_criteria_destroy(&criteria);
> + error = collect_rules_strict(ofproto, &ofm->criteria, rules);
>
> if (!error) {
> delete_flows_start__(ofproto, ofm->version, rules);
> @@ -5447,7 +5509,10 @@ handle_flow_mod__(struct ofproto *ofproto, const
> struct ofputil_flow_mod *fm,
> struct ofproto_flow_mod ofm;
> enum ofperr error;
>
> - ofm.fm = *fm;
> + error = ofproto_flow_mod_init(ofproto, &ofm, fm);
> + if (error) {
> + return error;
> + }
>
> ovs_mutex_lock(&ofproto_mutex);
> ofm.version = ofproto->tables_version + 1;
> @@ -7022,46 +7087,107 @@ handle_table_mod(struct ofconn *ofconn, const struct
> ofp_header *oh)
> return table_mod(ofproto, &tm);
> }
>
> +/* Free resources that may be allocated by ofproto_flow_mod_init(). */
> +void
> +ofproto_flow_mod_uninit(struct ofproto_flow_mod *ofm)
> +{
> + if (ofm->temp_rule) {
> + ofproto_rule_unref(ofm->temp_rule);
> + ofm->temp_rule = NULL;
> + }
> + if (ofm->criteria.version != OVS_VERSION_NOT_REMOVED) {
> + rule_criteria_destroy(&ofm->criteria);
> + }
> + if (ofm->conjs) {
> + free(ofm->conjs);
> + ofm->conjs = NULL;
> + ofm->n_conjs = 0;
> + }
> +}
> +
> static enum ofperr
> -ofproto_flow_mod_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
> - OVS_REQUIRES(ofproto_mutex)
> +ofproto_flow_mod_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
> + const struct ofputil_flow_mod *fm)
> + OVS_EXCLUDED(ofproto_mutex)
> {
> - /* Must check actions while holding ofproto_mutex to avoid a race. */
> - enum ofperr error = ofproto_check_ofpacts(ofproto, ofm->fm.ofpacts,
> - ofm->fm.ofpacts_len);
> + enum ofperr error;
> +
> + /* Forward flow mod fields we need later. */
> + ofm->command = fm->command;
> + ofm->buffer_id = fm->buffer_id;
> + ofm->modify_cookie = fm->modify_cookie;
> +
> + ofm->modify_may_add_flow = (fm->new_cookie != OVS_BE64_MAX
> + && fm->cookie_mask == htonll(0));
> +
> + /* Initialize state needed by ofproto_flow_mod_uninit(). */
> + ofm->temp_rule = NULL;
> + ofm->criteria.version = OVS_VERSION_NOT_REMOVED;
> + ofm->conjs = NULL;
> + ofm->n_conjs = 0;
> +
> + switch (ofm->command) {
> + case OFPFC_ADD:
> + error = add_flow_init(ofproto, ofm, fm);
> + break;
> + case OFPFC_MODIFY:
> + error = modify_flows_init_loose(ofproto, ofm, fm);
> + break;
> + case OFPFC_MODIFY_STRICT:
> + error = modify_flow_init_strict(ofproto, ofm, fm);
> + break;
> + case OFPFC_DELETE:
> + error = delete_flows_init_loose(ofproto, ofm, fm);
> + break;
> + case OFPFC_DELETE_STRICT:
> + error = delete_flows_init_strict(ofproto, ofm, fm);
> + break;
> + default:
> + error = OFPERR_OFPFMFC_BAD_COMMAND;
> + break;
> + }
> if (error) {
> - return error;
> + ofproto_flow_mod_uninit(ofm);
> }
> + return error;
> +}
>
> - /* Forward flow mod fields we need later. */
> - ofm->command = ofm->fm.command;
> - ofm->buffer_id = ofm->fm.buffer_id;
> - ofm->modify_cookie = ofm->fm.modify_cookie;
> +static enum ofperr
> +ofproto_flow_mod_start(struct ofproto *ofproto, struct ofproto_flow_mod *ofm)
> + OVS_REQUIRES(ofproto_mutex)
> +{
> + enum ofperr error;
>
> - ofm->modify_may_add_flow = (ofm->fm.new_cookie != OVS_BE64_MAX
> - && ofm->fm.cookie_mask == htonll(0));
> + rule_collection_init(&ofm->old_rules);
> + rule_collection_init(&ofm->new_rules);
>
> switch (ofm->command) {
> case OFPFC_ADD:
> - ofm->event = NXFME_ADDED;
> - return add_flow_start(ofproto, ofm);
> - /* , &be->old_rules.stub[0],
> - &be->new_rules.stub[0]); */
> + error = add_flow_start(ofproto, ofm);
> + break;
> case OFPFC_MODIFY:
> - ofm->event = NXFME_MODIFIED;
> - return modify_flows_start_loose(ofproto, ofm);
> + error = modify_flows_start_loose(ofproto, ofm);
> + break;
> case OFPFC_MODIFY_STRICT:
> - ofm->event = NXFME_MODIFIED;
> - return modify_flow_start_strict(ofproto, ofm);
> + error = modify_flow_start_strict(ofproto, ofm);
> + break;
> case OFPFC_DELETE:
> - ofm->event = NXFME_DELETED;
> - return delete_flows_start_loose(ofproto, ofm);
> + error = delete_flows_start_loose(ofproto, ofm);
> + break;
> case OFPFC_DELETE_STRICT:
> - ofm->event = NXFME_DELETED;
> - return delete_flow_start_strict(ofproto, ofm);
> + error = delete_flow_start_strict(ofproto, ofm);
> + break;
> + default:
> + OVS_NOT_REACHED();
> }
> + /* Release resources not needed after start. */
> + ofproto_flow_mod_uninit(ofm);
>
> - return OFPERR_OFPFMFC_BAD_COMMAND;
> + if (error) {
> + rule_collection_destroy(&ofm->old_rules);
> + rule_collection_destroy(&ofm->new_rules);
> + }
> + return error;
> }
>
> static void
> @@ -7086,6 +7212,8 @@ ofproto_flow_mod_revert(struct ofproto *ofproto, struct
> ofproto_flow_mod *ofm)
> default:
> break;
> }
> + rule_collection_destroy(&ofm->old_rules);
> + rule_collection_destroy(&ofm->new_rules);
> }
>
> static void
> @@ -7113,6 +7241,9 @@ ofproto_flow_mod_finish(struct ofproto *ofproto,
> break;
> }
>
> + rule_collection_destroy(&ofm->old_rules);
> + rule_collection_destroy(&ofm->new_rules);
> +
> if (req) {
> ofconn_report_flow_mod(req->ofconn, ofm->command);
> }
> @@ -7318,6 +7449,7 @@ handle_bundle_control(struct ofconn *ofconn, const
> struct ofp_header *oh)
>
> static enum ofperr
> handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh)
> + OVS_EXCLUDED(ofproto_mutex)
> {
> struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
> enum ofperr error;
> @@ -7340,17 +7472,20 @@ handle_bundle_add(struct ofconn *ofconn, const struct
> ofp_header *oh)
> if (type == OFPTYPE_PORT_MOD) {
> error = ofputil_decode_port_mod(badd.msg, &bmsg->opm.pm, false);
> } else if (type == OFPTYPE_FLOW_MOD) {
> + struct ofputil_flow_mod fm;
> struct ofpbuf ofpacts;
> uint64_t ofpacts_stub[1024 / 8];
>
> ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
> - error = ofputil_decode_flow_mod(&bmsg->ofm.fm, badd.msg,
> + error = ofputil_decode_flow_mod(&fm, badd.msg,
> ofconn_get_protocol(ofconn),
> &ofpacts,
> u16_to_ofp(ofproto->max_ports),
> ofproto->n_tables);
> - /* Move actions to heap. */
> - bmsg->ofm.fm.ofpacts = ofpbuf_steal_data(&ofpacts);
> + if (!error) {
> + error = ofproto_flow_mod_init(ofproto, &bmsg->ofm, &fm);
> + ofpbuf_uninit(&ofpacts);
> + }
> } else if (type == OFPTYPE_GROUP_MOD) {
> error = ofputil_decode_group_mod(badd.msg, &bmsg->ogm.gm);
> } else {
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 9a7ee89..147ac23 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -4921,8 +4921,6 @@ add table=254 actions=drop
> AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt 2>&1 | sed '/talking
> to/,$d'],
> [0], [dnl
> Error OFPBRC_EPERM for: OFPT_FLOW_MOD (OF1.4) (xid=0xb): ADD table:254
> actions=drop
> -Error OFPBFC_MSG_FAILED for: OFPT_BUNDLE_CONTROL (OF1.4) (xid=0xe):
> - bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
> ])
>
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> @@ -5408,8 +5406,6 @@ add table=254 actions=drop
> AT_CHECK([ovs-ofctl -O OpenFlow13 --bundle add-flows br0 flows.txt 2>&1 | sed
> '/talking to/,$d'],
> [0], [dnl
> Error OFPBRC_EPERM for: OFPT_FLOW_MOD (OF1.3) (xid=0xb): ADD table:254
> actions=drop
> -Error OFPBFC_MSG_FAILED for: ONFT_BUNDLE_CONTROL (OF1.3) (xid=0xe):
> - bundle_id=0 type=COMMIT_REQUEST flags=atomic ordered
> ])
>
> AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> --
> 2.1.4
>
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
