Thanks for the patch, which is better than the one I submitted. Tested-by: Yifeng Sun <[email protected]>
Reviewed-by: Yifeng Sun <[email protected]> On Tue, Jan 30, 2018 at 1:00 PM, Ben Pfaff <[email protected]> wrote: > 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 <[email protected]> > --- > lib/classifier.c | 25 +++++++++++++++++-------- > lib/classifier.h | 4 ++-- > lib/ovs-router.c | 19 ++++++++----------- > lib/tnl-ports.c | 5 ++--- > ofproto/ofproto.c | 14 ++++---------- > tests/test-classifier.c | 19 +++++++++---------- > tests/test-ovn.c | 2 +- > utilities/ovs-ofctl.c | 2 +- > 8 files changed, 44 insertions(+), 46 deletions(-) > > diff --git a/lib/classifier.c b/lib/classifier.c > index 16c451da1b30..9ad3710d61a1 100644 > --- a/lib/classifier.c > +++ b/lib/classifier.c > @@ -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; > @@ -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); > @@ -820,7 +821,15 @@ check_priority: > 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) > +{ > + bool OVS_UNUSED removed = classifier_remove(cls, cls_rule); > + ovs_assert(removed); > } > > /* Prefix tree context. Valid when 'lookup_done' is true. Can skip all > diff --git a/lib/classifier.h b/lib/classifier.h > index 71c2e507d7c3..31d4a1b08bd2 100644 > --- a/lib/classifier.h > +++ b/lib/classifier.h > @@ -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 *); > > diff --git a/lib/ovs-router.c b/lib/ovs-router.c > index cd2ab15fb003..e6cc81fd0827 100644 > --- a/lib/ovs-router.c > +++ b/lib/ovs-router.c > @@ -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 > @@ -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); > @@ -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); > diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c > index 04d2b3f7c6cf..b814f7a0a50a 100644 > --- a/lib/tnl-ports.c > +++ b/lib/tnl-ports.c > @@ -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); > } > } > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 4f17f79d286f..536636393850 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -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); > @@ -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); > } > @@ -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); > } > diff --git a/tests/test-classifier.c b/tests/test-classifier.c > index edaf4a393382..ef6693e61bcb 100644 > --- a/tests/test-classifier.c > +++ b/tests/test-classifier.c > @@ -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); > } > @@ -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)); > @@ -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); > @@ -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; > @@ -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--; > } > } > @@ -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, > @@ -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); > @@ -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, > diff --git a/tests/test-ovn.c b/tests/test-ovn.c > index 539e778e1c30..85763718d401 100644 > --- a/tests/test-ovn.c > +++ b/tests/test-ovn.c > @@ -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); > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c > index 3239d6fcb018..e1e4d37ed3af 100644 > --- a/utilities/ovs-ofctl.c > +++ b/utilities/ovs-ofctl.c > @@ -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); > -- > 2.15.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
