On 9/1/23 18:06, Mike Pattrick wrote: > On Fri, Sep 1, 2023 at 9:14 AM Ilya Maximets <[email protected]> wrote: >> >> 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. > > Hello Ilya, > > Thanks for the feedback, I'll resubmit with many changes incorporated. > However, I don't think using m4_define like that would be consistent > with any other tests.
It's not unheard of. We do use m4 definitions in ovsdb tests and in the system-traffic tests for different reasons. So, can be used, especially since they are shown in testsuite logs unlike shell variables. But it's not critical here, so, OK. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
