From 97f63e57a8f31952107e23d164b9347eda2ddc98 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 9 Sep 2013 11:19:41 -0700 Subject: [PATCH] ofproto: Remove soon-to-be-invalid optimizations. Until now, ofproto_add_flow() and ofproto_delete_flow() assumed that the flow table could not change between its flow table check and its later modification to the flow table. This assumption will soon be untrue, so remove it. Signed-off-by: Ben Pfaff Acked-by: Ethan Jackson --- ofproto/ofproto.c | 64 +++++++++++++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 21 deletions(-) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 21faa975d..f84510fab 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -1732,6 +1732,33 @@ ofproto_port_del(struct ofproto *ofproto, ofp_port_t ofp_port) return error; } +static int +simple_flow_mod(struct ofproto *ofproto, + const struct match *match, unsigned int priority, + const struct ofpact *ofpacts, size_t ofpacts_len, + enum ofp_flow_mod_command command) +{ + struct ofputil_flow_mod fm; + + memset(&fm, 0, sizeof fm); + fm.match = *match; + fm.priority = priority; + fm.cookie = 0; + fm.new_cookie = 0; + fm.modify_cookie = false; + fm.table_id = 0; + fm.command = command; + fm.idle_timeout = 0; + fm.hard_timeout = 0; + fm.buffer_id = UINT32_MAX; + fm.out_port = OFPP_ANY; + fm.out_group = OFPG_ANY; + fm.flags = 0; + fm.ofpacts = CONST_CAST(struct ofpact *, ofpacts); + fm.ofpacts_len = ofpacts_len; + return handle_flow_mod__(ofproto, NULL, &fm, NULL); +} + /* Adds a flow to OpenFlow flow table 0 in 'p' that matches 'cls_rule' and * performs the 'n_actions' actions in 'actions'. The new flow will not * timeout. @@ -1750,21 +1777,21 @@ ofproto_add_flow(struct ofproto *ofproto, const struct match *match, { const struct rule *rule; + /* First do a cheap check whether the rule we're looking for already exists + * with the actions that we want. If it does, then we're done. */ ovs_rwlock_rdlock(&ofproto->tables[0].cls.rwlock); rule = rule_from_cls_rule(classifier_find_match_exactly( &ofproto->tables[0].cls, match, priority)); ovs_rwlock_unlock(&ofproto->tables[0].cls.rwlock); + + /* If there's no such rule or the rule doesn't have the actions we want, + * fall back to a executing a full flow mod. We can't optimize this at + * all because we didn't take enough locks above to ensure that the flow + * table didn't already change beneath us. */ if (!rule || !ofpacts_equal(rule->ofpacts, rule->ofpacts_len, ofpacts, ofpacts_len)) { - struct ofputil_flow_mod fm; - - memset(&fm, 0, sizeof fm); - fm.match = *match; - fm.priority = priority; - fm.buffer_id = UINT32_MAX; - fm.ofpacts = CONST_CAST(struct ofpact *, ofpacts); - fm.ofpacts_len = ofpacts_len; - add_flow(ofproto, NULL, &fm, NULL); + simple_flow_mod(ofproto, match, priority, ofpacts, ofpacts_len, + OFPFC_MODIFY_STRICT); } } @@ -1790,26 +1817,21 @@ ofproto_delete_flow(struct ofproto *ofproto, struct classifier *cls = &ofproto->tables[0].cls; struct rule *rule; + /* First do a cheap check whether the rule we're looking for has already + * been deleted. If so, then we're done. */ ovs_rwlock_rdlock(&cls->rwlock); rule = rule_from_cls_rule(classifier_find_match_exactly(cls, target, priority)); ovs_rwlock_unlock(&cls->rwlock); if (!rule) { - /* No such rule -> success. */ - return true; - } else if (rule->pending) { - /* An operation on the rule is already pending -> failure. - * Caller must retry later if it's important. */ - return false; - } else { - /* Initiate deletion -> success. */ - ovs_rwlock_wrlock(&cls->rwlock); - ofproto_rule_delete(ofproto, cls, rule); - ovs_rwlock_unlock(&cls->rwlock); - return true; } + /* Fall back to a executing a full flow mod. We can't optimize this at all + * because we didn't take enough locks above to ensure that the flow table + * didn't already change beneath us. */ + return simple_flow_mod(ofproto, target, priority, NULL, 0, + OFPFC_DELETE_STRICT) != OFPROTO_POSTPONE; } /* Starts the process of deleting all of the flows from all of ofproto's flow -- 2.43.0