When the a revalidator thread is updating statistics for an XC_LEARN xcache entry in xlate_push_stats_entry it uses ofproto_flow_mod_learn. The revalidator will update stats for rules even if they are in a removed state or marked as invisible. However, ofproto_flow_mod_learn will detect if a flow has been removed and re-add it in that case. This can result in an old learn action replacing the new learn action that had replaced it in the first place.
This change adds a new force parameter to ofproto_flow_mod_learn allowing the caller to specify an action to take if temp_rule is removed. If force is set to false and the rule has been replaced in the classifier with a more recent rule, then ofproto_flow_mod_learn will just return. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2213892 Signed-off-by: Mike Pattrick <m...@redhat.com> --- v2: Added additional checks if rule is removed v3: v2 patch was corrupted in transit --- ofproto/ofproto-dpif-xlate-cache.c | 2 +- ofproto/ofproto-dpif-xlate.c | 2 +- ofproto/ofproto-dpif.c | 2 +- ofproto/ofproto-provider.h | 6 ++-- ofproto/ofproto.c | 48 +++++++++++++++++++++++++----- 5 files changed, 47 insertions(+), 13 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate-cache.c b/ofproto/ofproto-dpif-xlate-cache.c index 9224ee2e6..843903614 100644 --- a/ofproto/ofproto-dpif-xlate-cache.c +++ b/ofproto/ofproto-dpif-xlate-cache.c @@ -125,7 +125,7 @@ xlate_push_stats_entry(struct xc_entry *entry, case XC_LEARN: { enum ofperr error; error = ofproto_flow_mod_learn(entry->learn.ofm, true, - entry->learn.limit, NULL); + entry->learn.limit, NULL, false); if (error) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); VLOG_WARN_RL(&rl, "xcache LEARN action execution failed."); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 47ea0f47e..144938855 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -5701,7 +5701,7 @@ xlate_learn_action(struct xlate_ctx *ctx, const struct ofpact_learn *learn) bool success = true; if (ctx->xin->allow_side_effects) { error = ofproto_flow_mod_learn(ofm, ctx->xin->xcache != NULL, - learn->limit, &success); + learn->limit, &success, true); } else if (learn->limit) { if (!ofm->temp_rule || ofm->temp_rule->state != RULE_INSERTED) { diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index e22ca757a..9eb276108 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4880,7 +4880,7 @@ packet_xlate(struct ofproto *ofproto_, struct ofproto_packet_out *opo) if (entry->type == XC_LEARN) { struct ofproto_flow_mod *ofm = entry->learn.ofm; - error = ofproto_flow_mod_learn_refresh(ofm); + error = ofproto_flow_mod_learn_refresh(ofm, true); if (error) { goto error_out; } diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 143ded690..8ac85ea71 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -2027,9 +2027,11 @@ enum ofperr ofproto_flow_mod_init_for_learn(struct ofproto *, struct ofproto_flow_mod *) OVS_EXCLUDED(ofproto_mutex); enum ofperr ofproto_flow_mod_learn(struct ofproto_flow_mod *, bool keep_ref, - unsigned limit, bool *below_limit) + unsigned limit, bool *below_limit, + bool force) OVS_EXCLUDED(ofproto_mutex); -enum ofperr ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm); +enum ofperr ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm, + bool force); enum ofperr ofproto_flow_mod_learn_start(struct ofproto_flow_mod *ofm) OVS_REQUIRES(ofproto_mutex); void ofproto_flow_mod_learn_revert(struct ofproto_flow_mod *ofm) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index dbf4958bc..611823119 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -5472,7 +5472,7 @@ ofproto_flow_mod_init_for_learn(struct ofproto *ofproto, } enum ofperr -ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm) +ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm, bool force) { enum ofperr error = 0; @@ -5494,8 +5494,37 @@ ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod *ofm) if (rule->state == RULE_REMOVED) { struct cls_rule cr; - cls_rule_clone(&cr, &rule->cr); ovs_mutex_lock(&rule->mutex); + if (!force) { + /* If the caller hasn't explicitly requested rule recreation and + * the rule is removed and not visible, and the classifier has a + * newer version of this rule, then don't reinsert it. */ + struct oftable *table = &rule->ofproto->tables[rule->table_id]; + ovs_version_t tables_version = rule->ofproto->tables_version; + + if (!cls_rule_visible_in_version(&rule->cr, tables_version)) { + const struct cls_rule * curr_cls_rule; + + curr_cls_rule = classifier_find_rule_exactly(&table->cls, + &rule->cr, + tables_version); + if (curr_cls_rule) { + struct rule *curr_rule = rule_from_cls_rule(curr_cls_rule); + + ovs_mutex_lock(&curr_rule->mutex); + if (curr_rule->state != RULE_REMOVED && + curr_rule->modified > rule->modified) { + ovs_mutex_unlock(&curr_rule->mutex); + ovs_mutex_unlock(&rule->mutex); + + return OFPERR_OFPFMFC_UNKNOWN; + } + ovs_mutex_unlock(&curr_rule->mutex); + } + } + } + + cls_rule_clone(&cr, &rule->cr); error = ofproto_rule_create(rule->ofproto, &cr, rule->table_id, rule->flow_cookie, rule->idle_timeout, @@ -5565,10 +5594,13 @@ ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm, /* Refresh 'ofm->temp_rule', for which the caller holds a reference, if already * in the classifier, insert it otherwise. If the rule has already been - * removed from the classifier, a new rule is created using 'ofm->temp_rule' as - * a template and the reference to the old 'ofm->temp_rule' is freed. If - * 'keep_ref' is true, then a reference to the current rule is held, otherwise - * it is released and 'ofm->temp_rule' is set to NULL. + * removed from the classifier and 'force' is true, a new rule is created + * using 'ofm->temp_rule' as a template and the reference to the old + * 'ofm->temp_rule' is freed. If force is false, and the rule has been + * removed, and there is a newer copy of that rule in the classifier, do not + * reinsert a new copy of that rule. If 'keep_ref' is true, then a reference + * to the current rule is held, otherwise it is released and + * 'ofm->temp_rule' is set to NULL. * * If 'limit' != 0, insertion will fail if there are more than 'limit' rules * in the same table with the same cookie. If insertion succeeds, @@ -5579,10 +5611,10 @@ ofproto_flow_mod_learn_finish(struct ofproto_flow_mod *ofm, * during the call. */ enum ofperr ofproto_flow_mod_learn(struct ofproto_flow_mod *ofm, bool keep_ref, - unsigned limit, bool *below_limitp) + unsigned limit, bool *below_limitp, bool force) OVS_EXCLUDED(ofproto_mutex) { - enum ofperr error = ofproto_flow_mod_learn_refresh(ofm); + enum ofperr error = ofproto_flow_mod_learn_refresh(ofm, force); struct rule *rule = ofm->temp_rule; bool below_limit = true; -- 2.39.3 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
