Skip to content

Commit

Permalink
ofproto-dpif-xlate: Don't reinstall removed XC_LEARN rule.
Browse files Browse the repository at this point in the history 
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]>
Signed-off-by: Ilya Maximets <[email protected]>
  • Loading branch information
@mkp-rh @igsilya
mkp-rh authored and igsilya committed Sep 6, 2023
1 parent 6753b10 commit 04b8fe4
Show file tree
Hide file tree
Showing 6 changed files with 126 additions and 14 deletions.
2 changes: 1 addition & 1 deletion ofproto/ofproto-dpif-xlate-cache.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Expand Down
10 changes: 9 additions & 1 deletion ofproto/ofproto-dpif-xlate.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5696,8 +5696,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) {
Expand Down
2 changes: 1 addition & 1 deletion ofproto/ofproto-dpif.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
6 changes: 4 additions & 2 deletions ofproto/ofproto-provider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
60 changes: 51 additions & 9 deletions ofproto/ofproto.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5466,7 +5466,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;

Expand All @@ -5487,9 +5488,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 suppressing a learn action update that would replace a more
* 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,
Expand All @@ -5500,14 +5529,15 @@ 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. */
}
} 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;
Expand Down Expand Up @@ -5559,10 +5589,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,
Expand All @@ -5573,10 +5609,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;

Expand Down Expand Up @@ -5609,6 +5646,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 {
Expand Down
60 changes: 60 additions & 0 deletions tests/learn.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -836,3 +836,63 @@ 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

AT_CHECK([ovs-appctl time/stop], [0], [ignore])
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']])

packet="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)"

dnl Run this test a few times in a loop to reduce the likelyhood that it passes by chance.
for i in 1 2 3; do
AT_CHECK([ovs-appctl revalidator/pause], [0])
AT_CHECK([ovs-appctl netdev-dummy/receive p2 $packet], [0])
AT_CHECK([ovs-appctl time/warp 75], [0], [ignore])
AT_CHECK([ovs-appctl netdev-dummy/receive p1 $packet], [0])
AT_CHECK([ovs-appctl time/warp 75], [0], [ignore])
AT_CHECK([ovs-appctl netdev-dummy/receive p2 $packet], [0])
AT_CHECK([ovs-appctl time/warp 75], [0], [ignore])
AT_CHECK([ovs-appctl netdev-dummy/receive p1 $packet], [0])
AT_CHECK([ovs-appctl time/warp 75], [0], [ignore])

AT_CHECK([ovs-appctl revalidator/resume], [0])
AT_CHECK([ovs-appctl revalidator/wait], [0])

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
])

AT_CHECK([ovs-appctl revalidator/pause], [0])
AT_CHECK([ovs-appctl netdev-dummy/receive p1 $packet], [0])
AT_CHECK([ovs-appctl time/warp 75], [0], [ignore])
AT_CHECK([ovs-appctl netdev-dummy/receive p2 $packet], [0])
AT_CHECK([ovs-appctl time/warp 75], [0], [ignore])
AT_CHECK([ovs-appctl netdev-dummy/receive p1 $packet], [0])
AT_CHECK([ovs-appctl time/warp 75], [0], [ignore])
AT_CHECK([ovs-appctl netdev-dummy/receive p2 $packet], [0])
AT_CHECK([ovs-appctl time/warp 75], [0], [ignore])

AT_CHECK([ovs-appctl revalidator/resume], [0])
AT_CHECK([ovs-appctl revalidator/wait], [0])

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
])
done

dnl Wait and check for learned rule eviction due to hard timeout.
AT_CHECK([ovs-appctl time/warp 3200], [0], [ignore])

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

0 comments on commit 04b8fe4

Please sign in to comment.