On 9/1/23 07:06, 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 last_used parameter to ofproto_flow_mod_learn
> allowing the caller to provide a timestamp that will be fed into the
> learned rule's modified time. The provided timestamp should be the time
> of the last packet activity. If last_used is not set then the current
> time is used, as is the current behaviour.
>
> This change also adds a check when replacing a learned rule to favour the
> newest rule.
>
> 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
> v5: Fixed typos, updated commit message
> Changed timestamps to use datapath timestamp more consistently
> v6: Added a unit test, changed some variable names and comments,
> zero is no longer an acceptable value
> v7: Simplified unit test, updated comments.
>
> ---
> ofproto/ofproto-dpif-xlate-cache.c | 2 +-
> ofproto/ofproto-dpif-xlate.c | 10 ++-
> ofproto/ofproto-dpif.c | 2 +-
> ofproto/ofproto-provider.h | 6 +-
> ofproto/ofproto.c | 60 +++++++++++--
> tests/learn.at | 132 +++++++++++++++++++++++++++++
> 6 files changed, 198 insertions(+), 14 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..d608a5f25 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -5700,8 +5700,16 @@ xlate_learn_action(struct xlate_ctx *ctx, const struct
> ofpact_learn *learn)
> if (!error) {
> bool success = true;
> if (ctx->xin->allow_side_effects) {
> + long long int last_used;
> +
> + if (ctx->xin->resubmit_stats) {
> + last_used = ctx->xin->resubmit_stats->used;
> + } else {
> + last_used = time_msec();
> + }
> error = ofproto_flow_mod_learn(ofm, ctx->xin->xcache != NULL,
> - learn->limit, &success);
> + learn->limit, &success,
> + last_used);
> } 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..ba5706f6a 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, time_msec());
> if (error) {
> goto error_out;
> }
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 143ded690..9f7b8b6e8 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 last_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 last_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..7d3b3b6d1 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 last_used)
> {
> enum ofperr error = 0;
>
> @@ -5493,9 +5494,37 @@ ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod
> *ofm)
> * this function is executed the rule will be reinstated. */
> if (rule->state == RULE_REMOVED) {
> struct cls_rule cr;
> + 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;
> +
> + /* Only check for matching classifier rules and their modified
> + * time, instead of also checking all rule metadata, with the
> goal
> + * of suprressing a learn action update that would replace a more
s/ suprressing / suppressing /
> + * recent rule in the classifier. */
> + 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);
> + long long int curr_last_used;
> +
> + ovs_mutex_lock(&curr_rule->mutex);
> + curr_last_used = curr_rule->modified;
> + ovs_mutex_unlock(&curr_rule->mutex);
> +
> + if (curr_last_used > last_used) {
> + /* In the case of a newer visible rule, don't recreate
> the
> + * current rule. */
> + return 0;
> + }
> + }
> + }
>
> - cls_rule_clone(&cr, &rule->cr);
> ovs_mutex_lock(&rule->mutex);
> + cls_rule_clone(&cr, &rule->cr);
> error = ofproto_rule_create(rule->ofproto, &cr, rule->table_id,
> rule->flow_cookie,
> rule->idle_timeout,
> @@ -5506,6 +5535,7 @@ ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod
> *ofm)
> rule->match_tlv_bitmap,
> rule->ofpacts_tlv_bitmap,
> &ofm->temp_rule);
> + ofm->temp_rule->modified = last_used;
> ovs_mutex_unlock(&rule->mutex);
> if (!error) {
> ofproto_rule_unref(rule); /* Release old reference. */
> @@ -5513,7 +5543,7 @@ ofproto_flow_mod_learn_refresh(struct ofproto_flow_mod
> *ofm)
> } else {
> /* Refresh the existing rule. */
> ovs_mutex_lock(&rule->mutex);
> - rule->modified = time_msec();
> + rule->modified = last_used;
> ovs_mutex_unlock(&rule->mutex);
> }
> return error;
> @@ -5565,10 +5595,16 @@ 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 'last_used'
> + * parameter is used to determine whether the newer rule is replaced or kept.
> + * If 'last_used' is greater than the last modified time of an identical rule
> + * in the classifier, 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 the
> + * rule has been removed but another identical rule doesn't exist in the
> + * classifier, then it will be recreated. If the rule hasn't been removed
> + * from the classifier, then 'last_used' is used to update the rules modified
> + * time. 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 +5615,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 last_used)
> OVS_EXCLUDED(ofproto_mutex)
> {
> - enum ofperr error = ofproto_flow_mod_learn_refresh(ofm);
> + enum ofperr error = ofproto_flow_mod_learn_refresh(ofm, last_used);
> struct rule *rule = ofm->temp_rule;
> bool below_limit = true;
>
> @@ -5615,6 +5652,11 @@ ofproto_flow_mod_learn(struct ofproto_flow_mod *ofm,
> bool keep_ref,
>
> error = ofproto_flow_mod_learn_start(ofm);
> if (!error) {
> + /* ofproto_flow_mod_learn_start may have overwritten
> + * modified with current time. */
> + ovs_mutex_lock(&ofm->temp_rule->mutex);
> + ofm->temp_rule->modified = last_used;
> + ovs_mutex_unlock(&ofm->temp_rule->mutex);
> error = ofproto_flow_mod_learn_finish(ofm, NULL);
> }
> } else {
> diff --git a/tests/learn.at b/tests/learn.at
> index d127fed34..8e5054c08 100644
> --- a/tests/learn.at
> +++ b/tests/learn.at
> @@ -836,3 +836,135 @@ AT_CHECK([ovs-vsctl add-br br1 -- set b br1
> datapath_type=dummy])
>
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> +
> +AT_SETUP([learning action - flapping learn rule])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2 3
> +
> +ovs-appctl time/stop
> +AT_CHECK([[ovs-ofctl add-flow br0
> 'table=0,priority=2,in_port=1,actions=resubmit(,2)']])
> +AT_CHECK([[ovs-ofctl add-flow br0
> 'table=0,priority=2,in_port=3,actions=resubmit(,2)']])
> +AT_CHECK([[ovs-ofctl add-flow br0 'table=0,in_port=2,actions=normal']])
> +AT_CHECK([[ovs-ofctl add-flow br0 'table=0,priority=4,actions=drop']])
> +AT_CHECK([[ovs-ofctl add-flow br0
> 'table=2,actions=learn(table=0,hard_timeout=3,priority=1,cookie=0x0001020304050607,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],output:OXM_OF_IN_PORT[]),output:2']])
> +
> +packet1='in_port(1),eth(src=50:54:00:00:00:06,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9)'
> +packet2='in_port(3),eth(src=50:54:00:00:00:06,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9)'
> +
> +ovs-appctl revalidator/pause
> +ovs-appctl netdev-dummy/receive p3 $packet2
> +ovs-appctl time/warp 75
> +ovs-appctl netdev-dummy/receive p1 $packet1
> +ovs-appctl time/warp 75
> +ovs-appctl netdev-dummy/receive p3 $packet2
> +ovs-appctl time/warp 75
> +ovs-appctl netdev-dummy/receive p1 $packet1
> +ovs-appctl time/warp 75
> +ovs-appctl revalidator/resume
> +ovs-appctl revalidator/wait
> +
> +AT_CHECK([ovs-ofctl --no-stats dump-flows br0 | ofctl_strip | sed -e
> 's/n_packets=[[0-9]]*, n_bytes=[[0-9]]*, //' | sort], [0], [dnl
> + cookie=0x1020304050607, hard_timeout=3,
> priority=1,vlan_tci=0x0000/0x0fff,dl_dst=50:54:00:00:00:06 actions=output:1
> + in_port=2 actions=NORMAL
> + priority=2 actions=drop
> + priority=2,in_port=1 actions=resubmit(,2)
> + priority=2,in_port=3 actions=resubmit(,2)
> + table=2,
> actions=learn(table=0,hard_timeout=3,priority=1,cookie=0x1020304050607,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],output:OXM_OF_IN_PORT[[]]),output:2
> +NXST_FLOW reply:
> +])
> +
> +ovs-appctl revalidator/pause
> +ovs-appctl netdev-dummy/receive p1 $packet1
> +ovs-appctl time/warp 75
> +ovs-appctl netdev-dummy/receive p3 $packet2
> +ovs-appctl time/warp 75
> +ovs-appctl netdev-dummy/receive p1 $packet1
> +ovs-appctl time/warp 75
> +ovs-appctl netdev-dummy/receive p3 $packet2
> +ovs-appctl time/warp 75
> +
> +ovs-appctl revalidator/resume
> +ovs-appctl revalidator/wait
> +
> +AT_CHECK([ovs-ofctl --no-stats dump-flows br0 | ofctl_strip | sort], [0],
> [dnl
> + cookie=0x1020304050607, hard_timeout=3,
> priority=1,vlan_tci=0x0000/0x0fff,dl_dst=50:54:00:00:00:06 actions=output:3
> + in_port=2 actions=NORMAL
> + priority=2 actions=drop
> + priority=2,in_port=1 actions=resubmit(,2)
> + priority=2,in_port=3 actions=resubmit(,2)
> + table=2,
> actions=learn(table=0,hard_timeout=3,priority=1,cookie=0x1020304050607,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],output:OXM_OF_IN_PORT[[]]),output:2
> +NXST_FLOW reply:
> +])
> +
> +ovs-appctl time/warp 1000 3001
> +
> +AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
> + in_port=2 actions=NORMAL
> + priority=2 actions=drop
> + priority=2,in_port=1 actions=resubmit(,2)
> + priority=2,in_port=3 actions=resubmit(,2)
> + table=2,
> actions=learn(table=0,hard_timeout=3,priority=1,cookie=0x1020304050607,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],output:OXM_OF_IN_PORT[[]]),output:2
> +NXST_FLOW reply:
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
The test above looks like a debug version of a test below and always fails.
Please, remove.
> +
> +AT_SETUP([learning action - flapping learn rule])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2 3
> +
> +ovs-appctl time/stop
> +AT_CHECK([[ovs-ofctl add-flow br0
> 'table=0,priority=2,in_port=1,actions=resubmit(,2)']])
> +AT_CHECK([[ovs-ofctl add-flow br0
> 'table=0,priority=2,in_port=2,actions=resubmit(,2)']])
> +AT_CHECK([[ovs-ofctl add-flow br0
> 'table=2,actions=learn(table=0,hard_timeout=3,priority=1,cookie=0x123,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],output:OXM_OF_IN_PORT[]),output:3']])
> +
> +packet1='eth(src=50:54:00:00:00:06,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9)'
> +packet2='eth(src=50:54:00:00:00:06,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9)'
> +
Why two variables for the same packet?
Might make sense to also use m4_define for it, so the value is visible
in the test log.
Are we protected from hashing and flow dump order differences without
a loop that we had in previous version?
> +ovs-appctl revalidator/pause
> +ovs-appctl netdev-dummy/receive p2 $packet2
> +ovs-appctl time/warp 75
> +ovs-appctl netdev-dummy/receive p1 $packet1
> +ovs-appctl time/warp 75
> +ovs-appctl netdev-dummy/receive p2 $packet2
> +ovs-appctl time/warp 75
> +ovs-appctl netdev-dummy/receive p1 $packet1
> +ovs-appctl time/warp 75
> +ovs-appctl revalidator/resume
> +ovs-appctl revalidator/wait
Please, wrap all the ovs-appctl commands into AT_CHECK, so they are visible
in the test log. To save some LOCs, time warps can be written as:
AT_CHECK([ovs-appctl time/warp 75], [0], [ignore])
> +
> +ovs-ofctl --no-stats dump-flows br0
Seems unnecessary.
> +
> +AT_CHECK([ovs-ofctl --no-stats dump-flows br0 | ofctl_strip | sort | grep
> 0x123], [0], [dnl
> + cookie=0x123, hard_timeout=3, priority=1,dl_dst=50:54:00:00:00:06
> actions=output:1
> + table=2,
> actions=learn(table=0,hard_timeout=3,priority=1,cookie=0x123,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],output:OXM_OF_IN_PORT[[]]),output:3
> +])
> +
> +ovs-appctl revalidator/pause
> +ovs-appctl netdev-dummy/receive p1 $packet1
> +ovs-appctl time/warp 75
> +ovs-appctl netdev-dummy/receive p2 $packet2
> +ovs-appctl time/warp 75
> +ovs-appctl netdev-dummy/receive p1 $packet1
> +ovs-appctl time/warp 75
> +ovs-appctl netdev-dummy/receive p2 $packet2
> +ovs-appctl time/warp 75
> +
> +ovs-appctl revalidator/resume
> +ovs-appctl revalidator/wait
Ditto.
> +
> +AT_CHECK([ovs-ofctl --no-stats dump-flows br0 | ofctl_strip | sort | grep
> 0x123], [0], [dnl
> + cookie=0x123, hard_timeout=3, priority=1,dl_dst=50:54:00:00:00:06
> actions=output:2
> + table=2,
> actions=learn(table=0,hard_timeout=3,priority=1,cookie=0x123,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],output:OXM_OF_IN_PORT[[]]),output:3
> +])
> +
> +dnl Wait and check for learned rule eviction
Period at the end of a sentence. Also, might make sense to point out
that eviction is happening due to hard timeout.
> +ovs-appctl time/warp 3200
> +
> +AT_CHECK([ovs-ofctl --no-stats dump-flows br0 | ofctl_strip | grep 0x123],
> [0], [dnl
> + table=2,
> actions=learn(table=0,hard_timeout=3,priority=1,cookie=0x123,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],output:OXM_OF_IN_PORT[[]]),output:3
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> --
> 2.39.3
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
