Skip to content

Commit

Permalink
classifier: Refactor interface for classifier_remove().
Browse files Browse the repository at this point in the history 
Until now, classifier_remove() returned either null or the classifier rule
passed to it, which is an unusual interface.  This commit changes it to
return true if it succeeds or false on failure.

In addition, most of classifier_remove()'s callers know ahead of time that
it must succeed, even though most of them didn't bother with an assertion,
so this commit adds a classifier_remove_assert() function as a helper.

Signed-off-by: Ben Pfaff <blp@ovn.org>
Tested-by: Yifeng Sun <pkusunyifeng@gmail.com>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
  • Loading branch information
@blp
blp committed Jan 31, 2018
1 parent 186667a commit 46ab60b
Show file tree
Hide file tree
Showing 8 changed files with 43 additions and 46 deletions.
24 changes: 16 additions & 8 deletions lib/classifier.c
View file
Expand Up @@ -695,15 +695,16 @@ classifier_insert(struct classifier *cls, const struct cls_rule *rule,
ovs_assert(!displaced_rule);
}

/* Removes 'rule' from 'cls'. It is the caller's responsibility to destroy
* 'rule' with cls_rule_destroy(), freeing the memory block in which 'rule'
* resides, etc., as necessary.
/* If 'rule' is in 'cls', removes 'rule' from 'cls' and returns true. It is
* the caller's responsibility to destroy 'rule' with cls_rule_destroy(),
* freeing the memory block in which 'rule' resides, etc., as necessary.
*
* Does nothing if 'rule' has been already removed, or was never inserted.
* If 'rule' is not in any classifier, returns false without making any
* changes.
*
* Returns the removed rule, or NULL, if it was already removed.
* 'rule' must not be in some classifier other than 'cls'.
*/
const struct cls_rule *
bool
classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)
{
struct cls_match *rule, *prev, *next, *head;
Expand All @@ -716,7 +717,7 @@ classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)

rule = get_cls_match_protected(cls_rule);
if (!rule) {
return NULL;
return false;
}
/* Mark as removed. */
ovsrcu_set(&CONST_CAST(struct cls_rule *, cls_rule)->cls_match, NULL);
Expand Down Expand Up @@ -820,7 +821,14 @@ classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)
ovsrcu_postpone(cls_match_free_cb, rule);
cls->n_rules--;

return cls_rule;
return true;
}

void
classifier_remove_assert(struct classifier *cls,
const struct cls_rule *cls_rule)
{
ovs_assert(classifier_remove(cls, cls_rule));
}

/* Prefix tree context. Valid when 'lookup_done' is true. Can skip all
Expand Down
4 changes: 2 additions & 2 deletions lib/classifier.h
View file
Expand Up @@ -387,8 +387,8 @@ const struct cls_rule *classifier_replace(struct classifier *,
ovs_version_t,
const struct cls_conjunction *,
size_t n_conjunctions);
const struct cls_rule *classifier_remove(struct classifier *,
const struct cls_rule *);
bool classifier_remove(struct classifier *, const struct cls_rule *);
void classifier_remove_assert(struct classifier *, const struct cls_rule *);
static inline void classifier_defer(struct classifier *);
static inline void classifier_publish(struct classifier *);

Expand Down
19 changes: 8 additions & 11 deletions lib/ovs-router.c
View file
Expand Up @@ -245,19 +245,14 @@ ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t plen,
ovs_router_insert__(mark, plen, ip_dst, plen, output_bridge, gw);
}

static bool
__rt_entry_delete(const struct cls_rule *cr)
static void
rt_entry_delete__(const struct cls_rule *cr)
{
struct ovs_router_entry *p = ovs_router_entry_cast(cr);

tnl_port_map_delete_ipdev(p->output_bridge);
/* Remove it. */
cr = classifier_remove(&cls, cr);
if (cr) {
ovsrcu_postpone(rt_entry_free, ovs_router_entry_cast(cr));
return true;
}
return false;
classifier_remove_assert(&cls, cr);
ovsrcu_postpone(rt_entry_free, ovs_router_entry_cast(cr));
}

static bool
Expand All @@ -277,8 +272,10 @@ rt_entry_delete(uint32_t mark, uint8_t priority,
cr = classifier_find_rule_exactly(&cls, &rule, OVS_VERSION_MAX);
if (cr) {
ovs_mutex_lock(&mutex);
res = __rt_entry_delete(cr);
rt_entry_delete__(cr);
ovs_mutex_unlock(&mutex);

res = true;
}

cls_rule_destroy(&rule);
Expand Down Expand Up @@ -476,7 +473,7 @@ ovs_router_flush(void)
classifier_defer(&cls);
CLS_FOR_EACH(rt, cr, &cls) {
if (rt->priority == rt->plen) {
__rt_entry_delete(&rt->cr);
rt_entry_delete__(&rt->cr);
}
}
classifier_publish(&cls);
Expand Down
5 changes: 2 additions & 3 deletions lib/tnl-ports.c
View file
Expand Up @@ -223,9 +223,8 @@ tnl_port_unref(const struct cls_rule *cr)
struct tnl_port_in *p = tnl_port_cast(cr);

if (cr && ovs_refcount_unref_relaxed(&p->ref_cnt) == 1) {
if (classifier_remove(&cls, cr)) {
ovsrcu_postpone(tnl_port_free, p);
}
classifier_remove_assert(&cls, cr);
ovsrcu_postpone(tnl_port_free, p);
}
}

Expand Down
14 changes: 4 additions & 10 deletions ofproto/ofproto.c
View file
Expand Up @@ -1520,10 +1520,8 @@ ofproto_rule_delete(struct ofproto *ofproto, struct rule *rule)
/* Make sure there is no postponed removal of the rule. */
ovs_assert(cls_rule_visible_in_version(&rule->cr, OVS_VERSION_MAX));

if (!classifier_remove(&rule->ofproto->tables[rule->table_id].cls,
&rule->cr)) {
OVS_NOT_REACHED();
}
classifier_remove_assert(&rule->ofproto->tables[rule->table_id].cls,
&rule->cr);
ofproto_rule_remove__(rule->ofproto, rule);
if (ofproto->ofproto_class->rule_delete) {
ofproto->ofproto_class->rule_delete(rule);
Expand Down Expand Up @@ -2885,9 +2883,7 @@ remove_rule_rcu__(struct rule *rule)
struct oftable *table = &ofproto->tables[rule->table_id];

ovs_assert(!cls_rule_visible_in_version(&rule->cr, OVS_VERSION_MAX));
if (!classifier_remove(&table->cls, &rule->cr)) {
OVS_NOT_REACHED();
}
classifier_remove_assert(&table->cls, &rule->cr);
if (ofproto->ofproto_class->rule_delete) {
ofproto->ofproto_class->rule_delete(rule);
}
Expand Down Expand Up @@ -5231,9 +5227,7 @@ replace_rule_revert(struct ofproto *ofproto,
}

/* Remove the new rule immediately. It was never visible to lookups. */
if (!classifier_remove(&table->cls, &new_rule->cr)) {
OVS_NOT_REACHED();
}
classifier_remove_assert(&table->cls, &new_rule->cr);
ofproto_rule_remove__(ofproto, new_rule);
ofproto_rule_unref(new_rule);
}
Expand Down
19 changes: 9 additions & 10 deletions tests/test-classifier.c
View file
Expand Up @@ -466,9 +466,8 @@ destroy_classifier(struct classifier *cls)

classifier_defer(cls);
CLS_FOR_EACH (rule, cls_rule, cls) {
if (classifier_remove(cls, &rule->cls_rule)) {
ovsrcu_postpone(free_rule, rule);
}
classifier_remove_assert(cls, &rule->cls_rule);
ovsrcu_postpone(free_rule, rule);
}
classifier_destroy(cls);
}
Expand Down Expand Up @@ -816,7 +815,7 @@ test_single_rule(struct ovs_cmdl_context *ctx OVS_UNUSED)
compare_classifiers(&cls, 0, OVS_VERSION_MIN, &tcls);
check_tables(&cls, 1, 1, 0, 0, OVS_VERSION_MIN);

classifier_remove(&cls, &rule->cls_rule);
classifier_remove_assert(&cls, &rule->cls_rule);
tcls_remove(&tcls, tcls_rule);
assert(classifier_is_empty(&cls));
assert(tcls_is_empty(&tcls));
Expand Down Expand Up @@ -864,7 +863,7 @@ test_rule_replacement(struct ovs_cmdl_context *ctx OVS_UNUSED)
compare_classifiers(&cls, 0, OVS_VERSION_MIN, &tcls);
check_tables(&cls, 1, 1, 0, 0, OVS_VERSION_MIN);
classifier_defer(&cls);
classifier_remove(&cls, &rule2->cls_rule);
classifier_remove_assert(&cls, &rule2->cls_rule);

tcls_destroy(&tcls);
destroy_classifier(&cls);
Expand Down Expand Up @@ -1018,7 +1017,7 @@ test_many_rules_in_one_list (struct ovs_cmdl_context *ctx OVS_UNUSED)
n_invisible_rules++;
removable_rule = &rules[j]->cls_rule;
} else {
classifier_remove(&cls, &rules[j]->cls_rule);
classifier_remove_assert(&cls, &rules[j]->cls_rule);
}
tcls_remove(&tcls, tcls_rules[j]);
tcls_rules[j] = NULL;
Expand All @@ -1039,7 +1038,7 @@ test_many_rules_in_one_list (struct ovs_cmdl_context *ctx OVS_UNUSED)
/* Removable rule is no longer visible. */
assert(cls_match);
assert(!cls_match_visible_in_version(cls_match, version));
classifier_remove(&cls, removable_rule);
classifier_remove_assert(&cls, removable_rule);
n_invisible_rules--;
}
}
Expand Down Expand Up @@ -1139,7 +1138,7 @@ test_many_rules_in_one_table(struct ovs_cmdl_context *ctx OVS_UNUSED)
version);
n_invisible_rules++;
} else {
classifier_remove(&cls, &rules[i]->cls_rule);
classifier_remove_assert(&cls, &rules[i]->cls_rule);
}
compare_classifiers(&cls, n_invisible_rules, version, &tcls);
check_tables(&cls, i < N_RULES - 1, N_RULES - (i + 1), 0,
Expand All @@ -1151,7 +1150,7 @@ test_many_rules_in_one_table(struct ovs_cmdl_context *ctx OVS_UNUSED)

if (versioned) {
for (i = 0; i < N_RULES; i++) {
classifier_remove(&cls, &rules[i]->cls_rule);
classifier_remove_assert(&cls, &rules[i]->cls_rule);
n_invisible_rules--;

compare_classifiers(&cls, n_invisible_rules, version, &tcls);
Expand Down Expand Up @@ -1250,7 +1249,7 @@ test_many_rules_in_n_tables(int n_tables)

/* Remove rules that are no longer visible. */
LIST_FOR_EACH_POP (rule, list_node, &list) {
classifier_remove(&cls, &rule->cls_rule);
classifier_remove_assert(&cls, &rule->cls_rule);
n_invisible_rules--;

compare_classifiers(&cls, n_invisible_rules, version,
Expand Down
2 changes: 1 addition & 1 deletion tests/test-ovn.c
View file
Expand Up @@ -972,7 +972,7 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
struct test_rule *test_rule;

CLS_FOR_EACH (test_rule, cr, &cls) {
classifier_remove(&cls, &test_rule->cr);
classifier_remove_assert(&cls, &test_rule->cr);
ovsrcu_postpone(free_rule, test_rule);
}
classifier_destroy(&cls);
Expand Down
2 changes: 1 addition & 1 deletion utilities/ovs-ofctl.c
View file
Expand Up @@ -3220,7 +3220,7 @@ fte_free_all(struct flow_tables *tables)

classifier_defer(cls);
CLS_FOR_EACH (fte, rule, cls) {
classifier_remove(cls, &fte->rule);
classifier_remove_assert(cls, &fte->rule);
ovsrcu_postpone(fte_free, fte);
}
classifier_destroy(cls);
Expand Down

0 comments on commit 46ab60b

Please sign in to comment.