On 8/18/23 00:46, Mike Pattrick wrote: > 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.
The commit message is outdated. Also, it should be possible to add a test for this issue using time warping. Please, add one. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2213892 > Signed-off-by: Mike Pattrick <[email protected]> > --- > v2: Added additional checks if rule is removed > v3: v2 patch was corrupted in transit > v4: Added check against dpif flow stats > --- > 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 | 51 +++++++++++++++++++++++++----- > 5 files changed, 50 insertions(+), 13 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate-cache.c > b/ofproto/ofproto-dpif-xlate-cache.c > index 9224ee2e6..2e1fcb3a6 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, > stats->used); > 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..3a68cef4a 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, 0); > } 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..1efd9fc91 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, 0); > if (error) { > goto error_out; > } > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 143ded690..dc5c1a60d 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, > + long long int stats_used) > 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, > + long long int stats_used); > 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..a86d6a58e 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -5472,7 +5472,8 @@ 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, > + long long int stats_used) > { > enum ofperr error = 0; > > @@ -5494,8 +5495,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 (stats_used) { > + /* If the caller hasn't explicitly requested rule recreation and Seems outdated. > + * 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; No space between the star and the variable. > + > + 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 > stats_used) { We're comparing apples to oranges here. 'modified' is a time of the last rule refresh during revalidation, and 'stats_used' is a last time packet hit the learning flow in the datapath. The 'modified' value has to be set to 'stats_used' somewhere. > + 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 +5595,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 replaced by another rule, the stats_used > + * parameter can be used to deptermine whether the newer rule is replaced or Typo: deptermine > + * kept. If stats_used is set and greater than the current rule's last > modified > + * time then 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. > * > * 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 +5612,11 @@ 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, > + long long int stats_used) > OVS_EXCLUDED(ofproto_mutex) > { > - enum ofperr error = ofproto_flow_mod_learn_refresh(ofm); > + enum ofperr error = ofproto_flow_mod_learn_refresh(ofm, stats_used); > struct rule *rule = ofm->temp_rule; > bool below_limit = true; > > @@ -5659,6 +5693,7 @@ replace_rule_start(struct ofproto *ofproto, struct > ofproto_flow_mod *ofm, > *CONST_CAST(uint16_t *, &new_rule->importance) = > old_rule->importance; > new_rule->flags = old_rule->flags; > new_rule->created = old_rule->created; > + new_rule->modified = time_msec(); > } > if (!change_cookie) { > *CONST_CAST(ovs_be64 *, &new_rule->flow_cookie) > -- > 2.39.3 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
