On Mon, Aug 21, 2023 at 7:15 AM Ilya Maximets <[email protected]> wrote:
>
> 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.
We update the modified timestamp in replace_rule_start(), so we only
enter this new code path if the new rule was created after stats.used.
Thanks,
M
>
> > + 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
