Thanks for the review! Sorry about duplicating your work--I think that we both prepared the same change at the same time.
I applied this to master. On Tue, Jan 30, 2018 at 01:16:26PM -0800, Yifeng Sun wrote: > 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
