[ovs-dev] [PATCH v2] lib/classifier: Lockless lookups.
Ben Pfaff
blp at nicira.com
Mon Jul 7 16:31:23 UTC 2014
It looks like you posted a v2 of just the beginning of the series also?
I'll review those, then we can look at the rest of the series.
On Mon, Jun 30, 2014 at 04:40:06PM -0700, Jarno Rajahalme wrote:
> Just to be clear, this is a v2 on the patch 12/12 of the series, due to the batching changes Ethan just pushed. The 1-11 seem to still apply.
>
> Jarno
>
> On Jun 30, 2014, at 4:37 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>
> > Now that all the relevant classifier structures use RCU and internal
> > mutual exclusion for modifications, we can remove the fat-rwlock and
> > thus make the classifier lookups lockless.
> >
> > As the readers are operating concurrently with the writers, a
> > concurrent reader may or may not see a new rule being added by a
> > writer, depending on how the concurrent events overlap with each
> > other. Overall, this is no different from the former locked behavior,
> > but there the visibility of the new rule only depended on the timing
> > of the locking functions.
> >
> > A new rule is first added to the segment indices, so the readers may
> > find the rule in the indices before the rule is visible in the
> > subtables 'rules' map. This may result in us losing the opportunity
> > to quit lookups earlier, resulting in sub-optimal wildcarding. This
> > will be fixed by forthcoming revalidation always scheduled after flow
> > table changes.
> >
> > Similar behavior may happen due to us removing the overlapping rule
> > (if any) from the indices only after the corresponding new rule has
> > been added.
> >
> > The subtable's max priority is updated only after a rule is inserted
> > to the maps, so the concurrent readers may not see the rule, as the
> > updated priority ordered subtable list will only be visible after the
> > subtable's max priority is updated.
> >
> > Similarly, the classifier's partitions are updated by the caller after
> > the rule is inserted to the maps, so the readers may keep skipping the
> > subtable until they see the updated partitions.
> >
> > Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> > ---
> > v2: Rebase to master (classifier lookup batching).
> >
> > lib/classifier.c | 35 ++++++++++++++++++++++++++++++++---
> > lib/classifier.h | 45 ++++++++++++++++-----------------------------
> > lib/dpif-netdev.c | 30 ++++--------------------------
> > ofproto/ofproto-dpif.c | 2 --
> > ofproto/ofproto.c | 33 ++-------------------------------
> > ofproto/ofproto.h | 3 +++
> > tests/test-classifier.c | 36 +-----------------------------------
> > utilities/ovs-ofctl.c | 4 ----
> > 8 files changed, 58 insertions(+), 130 deletions(-)
> >
> > diff --git a/lib/classifier.c b/lib/classifier.c
> > index 8ba6867..fc05efe 100644
> > --- a/lib/classifier.c
> > +++ b/lib/classifier.c
> > @@ -481,7 +481,6 @@ classifier_init(struct classifier *cls_, const uint8_t *flow_segments)
> > {
> > struct cls_classifier *cls = xmalloc(sizeof *cls);
> >
> > - fat_rwlock_init(&cls_->rwlock);
> > ovs_mutex_init(&cls->mutex);
> >
> > ovs_mutex_lock(&cls->mutex);
> > @@ -506,7 +505,8 @@ classifier_init(struct classifier *cls_, const uint8_t *flow_segments)
> > }
> >
> > /* Destroys 'cls'. Rules within 'cls', if any, are not freed; this is the
> > - * caller's responsibility. */
> > + * caller's responsibility.
> > + * May only be called after all the readers have been terminated. */
> > void
> > classifier_destroy(struct classifier *cls_)
> > OVS_EXCLUDED(cls_->cls->mutex)
> > @@ -517,7 +517,6 @@ classifier_destroy(struct classifier *cls_)
> > struct cls_subtable *subtable, *next_subtable;
> > int i;
> >
> > - fat_rwlock_destroy(&cls_->rwlock);
> > if (!cls) {
> > return;
> > }
> > @@ -682,7 +681,10 @@ classifier_is_empty(const struct classifier *cls)
> > /* Returns the number of rules in 'cls'. */
> > int
> > classifier_count(const struct classifier *cls)
> > + OVS_NO_THREAD_SAFETY_ANALYSIS
> > {
> > + /* n_rules is an int, so in the presence of concurrent writers this will
> > + * return either the old or a new value. */
> > return cls->cls->n_rules;
> > }
> >
> > @@ -1711,6 +1713,33 @@ find_equal(struct cls_subtable *subtable, const struct miniflow *flow,
> > return NULL;
> > }
> >
> > +/*
> > + * As the readers are operating concurrently with the modifications, a
> > + * concurrent reader may or may not see the new rule, depending on how
> > + * the concurrent events overlap with each other. This is no
> > + * different from the former locked behavior, but there the visibility
> > + * of the new rule only depended on the timing of the locking
> > + * functions.
> > + *
> > + * The new rule is first added to the segment indices, so the readers
> > + * may find the rule in the indices before the rule is visible in the
> > + * subtables 'rules' map. This may result in us losing the
> > + * opportunity to quit lookups earlier, resulting in sub-optimal
> > + * wildcarding. This will be fixed by forthcoming revalidation always
> > + * scheduled after flow table changes.
> > + *
> > + * Similar behavior may happen due to us removing the overlapping rule
> > + * (if any) from the indices only after the new rule has been added.
> > + *
> > + * The subtable's max priority is updated only after the rule is
> > + * inserted, so the concurrent readers may not see the rule, as the
> > + * updated priority ordered subtable list will only be visible after
> > + * the subtable's max priority is updated.
> > + *
> > + * Similarly, the classifier's partitions for new rules are updated by
> > + * the caller after this function, so the readers may keep skipping
> > + * the subtable until they see the updated partitions.
> > + */
> > static struct cls_match *
> > insert_rule(struct cls_classifier *cls, struct cls_subtable *subtable,
> > struct cls_rule *new_rule)
> > diff --git a/lib/classifier.h b/lib/classifier.h
> > index 224d5bb..d0a408b 100644
> > --- a/lib/classifier.h
> > +++ b/lib/classifier.h
> > @@ -214,7 +214,6 @@
> > * by a single writer. */
> >
> > #include "cmap.h"
> > -#include "fat-rwlock.h"
> > #include "match.h"
> > #include "meta-flow.h"
> >
> > @@ -222,13 +221,9 @@
> > extern "C" {
> > #endif
> >
> > -/* Needed only for the lock annotation in struct classifier. */
> > -extern struct ovs_mutex ofproto_mutex;
> > -
> > /* Classifier internal data structures. */
> > struct cls_classifier;
> > struct cls_subtable;
> > -struct cls_partition;
> > struct cls_match;
> >
> > enum {
> > @@ -237,7 +232,6 @@ enum {
> >
> > /* A flow classifier. */
> > struct classifier {
> > - struct fat_rwlock rwlock OVS_ACQ_AFTER(ofproto_mutex);
> > struct cls_classifier *cls;
> > };
> >
> > @@ -266,38 +260,31 @@ bool cls_rule_is_catchall(const struct cls_rule *);
> > bool cls_rule_is_loose_match(const struct cls_rule *rule,
> > const struct minimatch *criteria);
> >
> > -void classifier_init(struct classifier *cls, const uint8_t *flow_segments);
> > +void classifier_init(struct classifier *, const uint8_t *flow_segments);
> > void classifier_destroy(struct classifier *);
> > -bool classifier_set_prefix_fields(struct classifier *cls,
> > +bool classifier_set_prefix_fields(struct classifier *,
> > const enum mf_field_id *trie_fields,
> > - unsigned int n_trie_fields)
> > - OVS_REQ_WRLOCK(cls->rwlock);
> > + unsigned int n_trie_fields);
> > +
> > +bool classifier_is_empty(const struct classifier *);
> > +int classifier_count(const struct classifier *);
> > +void classifier_insert(struct classifier *, struct cls_rule *);
> > +struct cls_rule *classifier_replace(struct classifier *, struct cls_rule *);
> >
> > -bool classifier_is_empty(const struct classifier *cls);
> > -int classifier_count(const struct classifier *cls)
> > - OVS_REQ_RDLOCK(cls->rwlock);
> > -void classifier_insert(struct classifier *cls, struct cls_rule *)
> > - OVS_REQ_WRLOCK(cls->rwlock);
> > -struct cls_rule *classifier_replace(struct classifier *cls, struct cls_rule *)
> > - OVS_REQ_WRLOCK(cls->rwlock);
> > -void classifier_remove(struct classifier *cls, struct cls_rule *)
> > - OVS_REQ_WRLOCK(cls->rwlock);
> > -struct cls_rule *classifier_lookup(const struct classifier *cls,
> > +void classifier_remove(struct classifier *, struct cls_rule *);
> > +struct cls_rule *classifier_lookup(const struct classifier *,
> > const struct flow *,
> > - struct flow_wildcards *)
> > - OVS_REQ_RDLOCK(cls->rwlock);
> > + struct flow_wildcards *);
> > void classifier_lookup_miniflow_batch(const struct classifier *cls,
> > const struct miniflow **flows,
> > - struct cls_rule **rules, size_t len)
> > - OVS_REQ_RDLOCK(cls->rwlock);
> > -bool classifier_rule_overlaps(const struct classifier *cls,
> > - const struct cls_rule *)
> > - OVS_REQ_RDLOCK(cls->rwlock);
> > + struct cls_rule **rules, size_t len);
> > +bool classifier_rule_overlaps(const struct classifier *,
> > + const struct cls_rule *);
> >
> > -struct cls_rule *classifier_find_rule_exactly(const struct classifier *cls,
> > +struct cls_rule *classifier_find_rule_exactly(const struct classifier *,
> > const struct cls_rule *);
> >
> > -struct cls_rule *classifier_find_match_exactly(const struct classifier *cls,
> > +struct cls_rule *classifier_find_match_exactly(const struct classifier *,
> > const struct match *,
> > unsigned int priority);
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 8447118..a96ce64 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -38,6 +38,7 @@
> > #include "dpif-provider.h"
> > #include "dummy.h"
> > #include "dynamic-string.h"
> > +#include "fat-rwlock.h"
> > #include "flow.h"
> > #include "cmap.h"
> > #include "latch.h"
> > @@ -125,7 +126,6 @@ struct dp_netdev_queue {
> > * dp_netdev_mutex (global)
> > * port_mutex
> > * flow_mutex
> > - * cls.rwlock
> > * queue_rwlock
> > */
> > struct dp_netdev {
> > @@ -136,16 +136,11 @@ struct dp_netdev {
> >
> > /* Flows.
> > *
> > - * Readers of 'cls' must take a 'cls->rwlock' read lock.
> > - *
> > - * Writers of 'flow_table' must take the 'flow_mutex'.
> > - *
> > - * Writers of 'cls' must take the 'flow_mutex' and then the 'cls->rwlock'
> > - * write lock. (The outer 'flow_mutex' allows writers to atomically
> > - * perform multiple operations on 'cls' and 'flow_table'.)
> > + * Writers of 'flow_table' must take the 'flow_mutex'. Corresponding
> > + * changes to 'cls' must be made while still holding the 'flow_mutex'.
> > */
> > struct ovs_mutex flow_mutex;
> > - struct classifier cls; /* Classifier. Protected by cls.rwlock. */
> > + struct classifier cls;
> > struct cmap flow_table OVS_GUARDED; /* Flow table. */
> >
> > /* Queues.
> > @@ -955,7 +950,6 @@ dp_netdev_flow_free(struct dp_netdev_flow *flow)
> >
> > static void
> > dp_netdev_remove_flow(struct dp_netdev *dp, struct dp_netdev_flow *flow)
> > - OVS_REQ_WRLOCK(dp->cls.rwlock)
> > OVS_REQUIRES(dp->flow_mutex)
> > {
> > struct cls_rule *cr = CONST_CAST(struct cls_rule *, &flow->cr);
> > @@ -972,11 +966,9 @@ dp_netdev_flow_flush(struct dp_netdev *dp)
> > struct dp_netdev_flow *netdev_flow, *next;
> >
> > ovs_mutex_lock(&dp->flow_mutex);
> > - fat_rwlock_wrlock(&dp->cls.rwlock);
> > CMAP_FOR_EACH_SAFE (netdev_flow, next, node, &dp->flow_table) {
> > dp_netdev_remove_flow(dp, netdev_flow);
> > }
> > - fat_rwlock_unlock(&dp->cls.rwlock);
> > ovs_mutex_unlock(&dp->flow_mutex);
> > }
> >
> > @@ -1073,7 +1065,6 @@ dp_netdev_flow_cast(const struct cls_rule *cr)
> >
> > static struct dp_netdev_flow *
> > dp_netdev_lookup_flow(const struct dp_netdev *dp, const struct miniflow *key)
> > - OVS_REQ_RDLOCK(dp->cls.rwlock)
> > {
> > struct dp_netdev_flow *netdev_flow;
> > struct cls_rule *rule;
> > @@ -1086,7 +1077,6 @@ dp_netdev_lookup_flow(const struct dp_netdev *dp, const struct miniflow *key)
> >
> > static struct dp_netdev_flow *
> > dp_netdev_find_flow(const struct dp_netdev *dp, const struct flow *flow)
> > - OVS_REQ_RDLOCK(dp->cls.rwlock)
> > {
> > struct dp_netdev_flow *netdev_flow;
> >
> > @@ -1225,9 +1215,7 @@ dpif_netdev_flow_get(const struct dpif *dpif,
> > return error;
> > }
> >
> > - fat_rwlock_rdlock(&dp->cls.rwlock);
> > netdev_flow = dp_netdev_find_flow(dp, &key);
> > - fat_rwlock_unlock(&dp->cls.rwlock);
> >
> > if (netdev_flow) {
> > if (stats) {
> > @@ -1271,10 +1259,8 @@ dp_netdev_flow_add(struct dp_netdev *dp, const struct flow *flow,
> > cmap_insert(&dp->flow_table,
> > CONST_CAST(struct cmap_node *, &netdev_flow->node),
> > flow_hash(flow, 0));
> > - fat_rwlock_wrlock(&dp->cls.rwlock);
> > classifier_insert(&dp->cls,
> > CONST_CAST(struct cls_rule *, &netdev_flow->cr));
> > - fat_rwlock_unlock(&dp->cls.rwlock);
> >
> > return 0;
> > }
> > @@ -1318,9 +1304,7 @@ dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put)
> > miniflow_init(&miniflow, &flow);
> >
> > ovs_mutex_lock(&dp->flow_mutex);
> > - fat_rwlock_rdlock(&dp->cls.rwlock);
> > netdev_flow = dp_netdev_lookup_flow(dp, &miniflow);
> > - fat_rwlock_unlock(&dp->cls.rwlock);
> > if (!netdev_flow) {
> > if (put->flags & DPIF_FP_CREATE) {
> > if (cmap_count(&dp->flow_table) < MAX_FLOWS) {
> > @@ -1382,7 +1366,6 @@ dpif_netdev_flow_del(struct dpif *dpif, const struct dpif_flow_del *del)
> > }
> >
> > ovs_mutex_lock(&dp->flow_mutex);
> > - fat_rwlock_wrlock(&dp->cls.rwlock);
> > netdev_flow = dp_netdev_find_flow(dp, &key);
> > if (netdev_flow) {
> > if (del->stats) {
> > @@ -1392,7 +1375,6 @@ dpif_netdev_flow_del(struct dpif *dpif, const struct dpif_flow_del *del)
> > } else {
> > error = ENOENT;
> > }
> > - fat_rwlock_unlock(&dp->cls.rwlock);
> > ovs_mutex_unlock(&dp->flow_mutex);
> >
> > return error;
> > @@ -1485,7 +1467,6 @@ dpif_netdev_flow_dump_next(struct dpif_flow_dump_thread *thread_,
> >
> > ovs_mutex_lock(&dump->mutex);
> > if (!dump->status) {
> > - fat_rwlock_rdlock(&dp->cls.rwlock);
> > for (n_flows = 0; n_flows < MIN(max_flows, FLOW_DUMP_MAX_BATCH);
> > n_flows++) {
> > struct cmap_node *node;
> > @@ -1498,7 +1479,6 @@ dpif_netdev_flow_dump_next(struct dpif_flow_dump_thread *thread_,
> > netdev_flows[n_flows] = CONTAINER_OF(node, struct dp_netdev_flow,
> > node);
> > }
> > - fat_rwlock_unlock(&dp->cls.rwlock);
> > }
> > ovs_mutex_unlock(&dump->mutex);
> >
> > @@ -2081,9 +2061,7 @@ dp_netdev_input(struct dp_netdev *dp, struct dpif_packet **packets, int cnt,
> > mfs[i] = &keys[i].flow;
> > }
> >
> > - fat_rwlock_rdlock(&dp->cls.rwlock);
> > classifier_lookup_miniflow_batch(&dp->cls, mfs, rules, cnt);
> > - fat_rwlock_unlock(&dp->cls.rwlock);
> >
> > n_batches = 0;
> > for (i = 0; i < cnt; i++) {
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index a0d5d47..3ddc1b9 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -3379,9 +3379,7 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, uint8_t table_id,
> > }
> >
> > retry:
> > - fat_rwlock_rdlock(&cls->rwlock);
> > cls_rule = classifier_lookup(cls, flow, wc);
> > - fat_rwlock_unlock(&cls->rwlock);
> >
> > rule = rule_dpif_cast(rule_from_cls_rule(cls_rule));
> > if (rule && take_ref) {
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index b020379..b6252d7 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -1214,12 +1214,11 @@ ofproto_configure_table(struct ofproto *ofproto, int table_id,
> > }
> >
> > table->max_flows = s->max_flows;
> > - fat_rwlock_wrlock(&table->cls.rwlock);
> > +
> > if (classifier_set_prefix_fields(&table->cls,
> > s->prefix_fields, s->n_prefix_fields)) {
> > /* XXX: Trigger revalidation. */
> > }
> > - fat_rwlock_unlock(&table->cls.rwlock);
> >
> > ovs_mutex_lock(&ofproto_mutex);
> > evict_rules_from_table(table, 0);
> > @@ -1551,9 +1550,7 @@ ofproto_get_memory_usage(const struct ofproto *ofproto, struct simap *usage)
> >
> > n_rules = 0;
> > OFPROTO_FOR_EACH_TABLE (table, ofproto) {
> > - fat_rwlock_rdlock(&table->cls.rwlock);
> > n_rules += classifier_count(&table->cls);
> > - fat_rwlock_unlock(&table->cls.rwlock);
> > }
> > simap_increase(usage, "rules", n_rules);
> >
> > @@ -1841,7 +1838,6 @@ ofproto_add_flow(struct ofproto *ofproto, const struct match *match,
> >
> > /* First do a cheap check whether the rule we're looking for already exists
> > * with the actions that we want. If it does, then we're done. */
> > - fat_rwlock_rdlock(&ofproto->tables[0].cls.rwlock);
> > rule = rule_from_cls_rule(classifier_find_match_exactly(
> > &ofproto->tables[0].cls, match, priority));
> > if (rule) {
> > @@ -1851,7 +1847,6 @@ ofproto_add_flow(struct ofproto *ofproto, const struct match *match,
> > } else {
> > must_add = true;
> > }
> > - fat_rwlock_unlock(&ofproto->tables[0].cls.rwlock);
> >
> > /* If there's no such rule or the rule doesn't have the actions we want,
> > * fall back to a executing a full flow mod. We can't optimize this at
> > @@ -1882,7 +1877,6 @@ ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm)
> > struct rule *rule;
> > bool done = false;
> >
> > - fat_rwlock_rdlock(&table->cls.rwlock);
> > rule = rule_from_cls_rule(classifier_find_match_exactly(&table->cls,
> > &fm->match,
> > fm->priority));
> > @@ -1907,7 +1901,6 @@ ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm)
> > }
> > ovs_mutex_unlock(&rule->mutex);
> > }
> > - fat_rwlock_unlock(&table->cls.rwlock);
> >
> > if (done) {
> > return 0;
> > @@ -1931,10 +1924,8 @@ ofproto_delete_flow(struct ofproto *ofproto,
> >
> > /* First do a cheap check whether the rule we're looking for has already
> > * been deleted. If so, then we're done. */
> > - fat_rwlock_rdlock(&cls->rwlock);
> > rule = rule_from_cls_rule(classifier_find_match_exactly(cls, target,
> > priority));
> > - fat_rwlock_unlock(&cls->rwlock);
> > if (!rule) {
> > return;
> > }
> > @@ -3118,9 +3109,7 @@ handle_table_stats_request(struct ofconn *ofconn,
> > ots[i].instructions = htonl(OFPIT11_ALL);
> > ots[i].config = htonl(OFPTC11_TABLE_MISS_MASK);
> > ots[i].max_entries = htonl(1000000); /* An arbitrary big number. */
> > - fat_rwlock_rdlock(&p->tables[i].cls.rwlock);
> > ots[i].active_count = htonl(classifier_count(&p->tables[i].cls));
> > - fat_rwlock_unlock(&p->tables[i].cls.rwlock);
> > }
> >
> > p->ofproto_class->get_tables(p, ots);
> > @@ -3553,10 +3542,8 @@ collect_rules_strict(struct ofproto *ofproto,
> > FOR_EACH_MATCHING_TABLE (table, criteria->table_id, ofproto) {
> > struct rule *rule;
> >
> > - fat_rwlock_rdlock(&table->cls.rwlock);
> > rule = rule_from_cls_rule(classifier_find_rule_exactly(
> > &table->cls, &criteria->cr));
> > - fat_rwlock_unlock(&table->cls.rwlock);
> > if (rule) {
> > collect_rule(rule, criteria, rules, &n_readonly);
> > }
> > @@ -4010,9 +3997,7 @@ add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
> > cls_rule_init(&cr, &fm->match, fm->priority);
> >
> > /* Transform "add" into "modify" if there's an existing identical flow. */
> > - fat_rwlock_rdlock(&table->cls.rwlock);
> > rule = rule_from_cls_rule(classifier_find_rule_exactly(&table->cls, &cr));
> > - fat_rwlock_unlock(&table->cls.rwlock);
> > if (rule) {
> > struct rule_collection rules;
> >
> > @@ -4029,13 +4014,7 @@ add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
> >
> > /* Check for overlap, if requested. */
> > if (fm->flags & OFPUTIL_FF_CHECK_OVERLAP) {
> > - bool overlaps;
> > -
> > - fat_rwlock_rdlock(&table->cls.rwlock);
> > - overlaps = classifier_rule_overlaps(&table->cls, &cr);
> > - fat_rwlock_unlock(&table->cls.rwlock);
> > -
> > - if (overlaps) {
> > + if (classifier_rule_overlaps(&table->cls, &cr)) {
> > cls_rule_destroy(&cr);
> > return OFPERR_OFPFMFC_OVERLAP;
> > }
> > @@ -4097,9 +4076,7 @@ add_flow(struct ofproto *ofproto, struct ofputil_flow_mod *fm,
> > meter_insert_rule(rule);
> > }
> >
> > - fat_rwlock_wrlock(&table->cls.rwlock);
> > classifier_insert(&table->cls, CONST_CAST(struct cls_rule *, &rule->cr));
> > - fat_rwlock_unlock(&table->cls.rwlock);
> >
> > error = ofproto->ofproto_class->rule_insert(rule);
> > if (error) {
> > @@ -6373,10 +6350,8 @@ oftable_init(struct oftable *table)
> > table->max_flows = UINT_MAX;
> > atomic_init(&table->config, (unsigned int)OFPROTO_TABLE_MISS_DEFAULT);
> >
> > - fat_rwlock_wrlock(&table->cls.rwlock);
> > classifier_set_prefix_fields(&table->cls, default_prefix_fields,
> > ARRAY_SIZE(default_prefix_fields));
> > - fat_rwlock_unlock(&table->cls.rwlock);
> >
> > atomic_init(&table->n_matched, 0);
> > atomic_init(&table->n_missed, 0);
> > @@ -6388,9 +6363,7 @@ oftable_init(struct oftable *table)
> > static void
> > oftable_destroy(struct oftable *table)
> > {
> > - fat_rwlock_rdlock(&table->cls.rwlock);
> > ovs_assert(classifier_is_empty(&table->cls));
> > - fat_rwlock_unlock(&table->cls.rwlock);
> > oftable_disable_eviction(table);
> > classifier_destroy(&table->cls);
> > free(table->name);
> > @@ -6483,9 +6456,7 @@ oftable_remove_rule__(struct ofproto *ofproto, struct rule *rule)
> > {
> > struct classifier *cls = &ofproto->tables[rule->table_id].cls;
> >
> > - fat_rwlock_wrlock(&cls->rwlock);
> > classifier_remove(cls, CONST_CAST(struct cls_rule *, &rule->cr));
> > - fat_rwlock_unlock(&cls->rwlock);
> >
> > cookies_remove(ofproto, rule);
> >
> > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> > index beabcf2..d601181 100644
> > --- a/ofproto/ofproto.h
> > +++ b/ofproto/ofproto.h
> > @@ -45,6 +45,9 @@ struct shash;
> > struct simap;
> > struct smap;
> >
> > +/* Needed for the lock annotations. */
> > +extern struct ovs_mutex ofproto_mutex;
> > +
> > struct ofproto_controller_info {
> > bool is_connected;
> > enum ofp12_controller_role role;
> > diff --git a/tests/test-classifier.c b/tests/test-classifier.c
> > index c63b7dc..1d5515c 100644
> > --- a/tests/test-classifier.c
> > +++ b/tests/test-classifier.c
> > @@ -400,7 +400,6 @@ get_value(unsigned int *x, unsigned n_values)
> >
> > static void
> > compare_classifiers(struct classifier *cls, struct tcls *tcls)
> > - OVS_REQ_RDLOCK(cls->rwlock)
> > {
> > static const int confidence = 500;
> > unsigned int i;
> > @@ -456,9 +455,7 @@ destroy_classifier(struct classifier *cls)
> > struct test_rule *rule, *next_rule;
> >
> > CLS_FOR_EACH_SAFE (rule, next_rule, cls_rule, cls) {
> > - fat_rwlock_wrlock(&cls->rwlock);
> > classifier_remove(cls, &rule->cls_rule);
> > - fat_rwlock_unlock(&cls->rwlock);
> > free_rule(rule);
> > }
> > classifier_destroy(cls);
> > @@ -515,7 +512,7 @@ verify_tries(struct classifier *cls_)
> >
> > static void
> > check_tables(const struct classifier *cls, int n_tables, int n_rules,
> > - int n_dups) OVS_EXCLUDED(cls->rwlock)
> > + int n_dups)
> > {
> > const struct cls_subtable *table;
> > struct test_rule *test_rule;
> > @@ -697,7 +694,6 @@ static enum mf_field_id trie_fields[2] = {
> >
> > static void
> > set_prefix_fields(struct classifier *cls)
> > - OVS_REQ_WRLOCK(cls->rwlock)
> > {
> > verify_tries(cls);
> > classifier_set_prefix_fields(cls, trie_fields, ARRAY_SIZE(trie_fields));
> > @@ -712,13 +708,11 @@ test_empty(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> > struct tcls tcls;
> >
> > classifier_init(&cls, flow_segment_u32s);
> > - fat_rwlock_wrlock(&cls.rwlock);
> > set_prefix_fields(&cls);
> > tcls_init(&tcls);
> > assert(classifier_is_empty(&cls));
> > assert(tcls_is_empty(&tcls));
> > compare_classifiers(&cls, &tcls);
> > - fat_rwlock_unlock(&cls.rwlock);
> > classifier_destroy(&cls);
> > tcls_destroy(&tcls);
> > }
> > @@ -745,23 +739,19 @@ test_single_rule(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> > hash_bytes(&wc_fields, sizeof wc_fields, 0), 0);
> >
> > classifier_init(&cls, flow_segment_u32s);
> > - fat_rwlock_wrlock(&cls.rwlock);
> > set_prefix_fields(&cls);
> > tcls_init(&tcls);
> >
> > tcls_rule = tcls_insert(&tcls, rule);
> > classifier_insert(&cls, &rule->cls_rule);
> > compare_classifiers(&cls, &tcls);
> > - fat_rwlock_unlock(&cls.rwlock);
> > check_tables(&cls, 1, 1, 0);
> >
> > - fat_rwlock_wrlock(&cls.rwlock);
> > classifier_remove(&cls, &rule->cls_rule);
> > tcls_remove(&tcls, tcls_rule);
> > assert(classifier_is_empty(&cls));
> > assert(tcls_is_empty(&tcls));
> > compare_classifiers(&cls, &tcls);
> > - fat_rwlock_unlock(&cls.rwlock);
> >
> > free_rule(rule);
> > classifier_destroy(&cls);
> > @@ -787,25 +777,21 @@ test_rule_replacement(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> > rule2->aux += 5;
> >
> > classifier_init(&cls, flow_segment_u32s);
> > - fat_rwlock_wrlock(&cls.rwlock);
> > set_prefix_fields(&cls);
> > tcls_init(&tcls);
> > tcls_insert(&tcls, rule1);
> > classifier_insert(&cls, &rule1->cls_rule);
> > compare_classifiers(&cls, &tcls);
> > - fat_rwlock_unlock(&cls.rwlock);
> > check_tables(&cls, 1, 1, 0);
> > tcls_destroy(&tcls);
> >
> > tcls_init(&tcls);
> > tcls_insert(&tcls, rule2);
> >
> > - fat_rwlock_wrlock(&cls.rwlock);
> > assert(test_rule_from_cls_rule(
> > classifier_replace(&cls, &rule2->cls_rule)) == rule1);
> > free_rule(rule1);
> > compare_classifiers(&cls, &tcls);
> > - fat_rwlock_unlock(&cls.rwlock);
> > check_tables(&cls, 1, 1, 0);
> >
> > tcls_destroy(&tcls);
> > @@ -904,16 +890,13 @@ test_many_rules_in_one_list (int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> > }
> >
> > classifier_init(&cls, flow_segment_u32s);
> > - fat_rwlock_wrlock(&cls.rwlock);
> > set_prefix_fields(&cls);
> > - fat_rwlock_unlock(&cls.rwlock);
> > tcls_init(&tcls);
> >
> > for (i = 0; i < ARRAY_SIZE(ops); i++) {
> > int j = ops[i];
> > int m, n;
> >
> > - fat_rwlock_wrlock(&cls.rwlock);
> > if (!tcls_rules[j]) {
> > struct test_rule *displaced_rule;
> >
> > @@ -937,7 +920,6 @@ test_many_rules_in_one_list (int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> > pri_rules[pris[j]] = -1;
> > }
> > compare_classifiers(&cls, &tcls);
> > - fat_rwlock_unlock(&cls.rwlock);
> >
> > n = 0;
> > for (m = 0; m < N_RULES; m++) {
> > @@ -946,14 +928,12 @@ test_many_rules_in_one_list (int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> > check_tables(&cls, n > 0, n, n - 1);
> > }
> >
> > - fat_rwlock_wrlock(&cls.rwlock);
> > for (i = 0; i < N_RULES; i++) {
> > if (rules[i]->cls_rule.cls_match) {
> > classifier_remove(&cls, &rules[i]->cls_rule);
> > }
> > free_rule(rules[i]);
> > }
> > - fat_rwlock_unlock(&cls.rwlock);
> > classifier_destroy(&cls);
> > tcls_destroy(&tcls);
> > } while (next_permutation(ops, ARRAY_SIZE(ops)));
> > @@ -1012,9 +992,7 @@ test_many_rules_in_one_table(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> > } while ((1 << count_ones(value_mask)) < N_RULES);
> >
> > classifier_init(&cls, flow_segment_u32s);
> > - fat_rwlock_wrlock(&cls.rwlock);
> > set_prefix_fields(&cls);
> > - fat_rwlock_unlock(&cls.rwlock);
> > tcls_init(&tcls);
> >
> > for (i = 0; i < N_RULES; i++) {
> > @@ -1027,20 +1005,16 @@ test_many_rules_in_one_table(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> > rules[i] = make_rule(wcf, priority, value_pats[i]);
> > tcls_rules[i] = tcls_insert(&tcls, rules[i]);
> >
> > - fat_rwlock_wrlock(&cls.rwlock);
> > classifier_insert(&cls, &rules[i]->cls_rule);
> > compare_classifiers(&cls, &tcls);
> > - fat_rwlock_unlock(&cls.rwlock);
> >
> > check_tables(&cls, 1, i + 1, 0);
> > }
> >
> > for (i = 0; i < N_RULES; i++) {
> > tcls_remove(&tcls, tcls_rules[i]);
> > - fat_rwlock_wrlock(&cls.rwlock);
> > classifier_remove(&cls, &rules[i]->cls_rule);
> > compare_classifiers(&cls, &tcls);
> > - fat_rwlock_unlock(&cls.rwlock);
> > free_rule(rules[i]);
> >
> > check_tables(&cls, i < N_RULES - 1, N_RULES - (i + 1), 0);
> > @@ -1080,9 +1054,7 @@ test_many_rules_in_n_tables(int n_tables)
> > shuffle(priorities, ARRAY_SIZE(priorities));
> >
> > classifier_init(&cls, flow_segment_u32s);
> > - fat_rwlock_wrlock(&cls.rwlock);
> > set_prefix_fields(&cls);
> > - fat_rwlock_unlock(&cls.rwlock);
> > tcls_init(&tcls);
> >
> > for (i = 0; i < MAX_RULES; i++) {
> > @@ -1092,10 +1064,8 @@ test_many_rules_in_n_tables(int n_tables)
> > int value_pat = random_uint32() & ((1u << CLS_N_FIELDS) - 1);
> > rule = make_rule(wcf, priority, value_pat);
> > tcls_insert(&tcls, rule);
> > - fat_rwlock_wrlock(&cls.rwlock);
> > classifier_insert(&cls, &rule->cls_rule);
> > compare_classifiers(&cls, &tcls);
> > - fat_rwlock_unlock(&cls.rwlock);
> > check_tables(&cls, -1, i + 1, -1);
> > }
> >
> > @@ -1107,16 +1077,12 @@ test_many_rules_in_n_tables(int n_tables)
> >
> > CLS_FOR_EACH_TARGET_SAFE (rule, next_rule, cls_rule, &cls,
> > &target->cls_rule) {
> > - fat_rwlock_wrlock(&cls.rwlock);
> > classifier_remove(&cls, &rule->cls_rule);
> > - fat_rwlock_unlock(&cls.rwlock);
> > free_rule(rule);
> > }
> >
> > tcls_delete_matches(&tcls, &target->cls_rule);
> > - fat_rwlock_rdlock(&cls.rwlock);
> > compare_classifiers(&cls, &tcls);
> > - fat_rwlock_unlock(&cls.rwlock);
> > check_tables(&cls, -1, -1, -1);
> > free_rule(target);
> > }
> > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> > index b001c24..de68f47 100644
> > --- a/utilities/ovs-ofctl.c
> > +++ b/utilities/ovs-ofctl.c
> > @@ -2319,9 +2319,7 @@ fte_free_all(struct classifier *cls)
> > struct fte *fte, *next;
> >
> > CLS_FOR_EACH_SAFE (fte, next, rule, cls) {
> > - fat_rwlock_wrlock(&cls->rwlock);
> > classifier_remove(cls, &fte->rule);
> > - fat_rwlock_unlock(&cls->rwlock);
> > fte_free(fte);
> > }
> > classifier_destroy(cls);
> > @@ -2342,9 +2340,7 @@ fte_insert(struct classifier *cls, const struct match *match,
> > cls_rule_init(&fte->rule, match, priority);
> > fte->versions[index] = version;
> >
> > - fat_rwlock_wrlock(&cls->rwlock);
> > old = fte_from_cls_rule(classifier_replace(cls, &fte->rule));
> > - fat_rwlock_unlock(&cls->rwlock);
> > if (old) {
> > fte_version_free(old->versions[index]);
> > fte->versions[!index] = old->versions[!index];
> > --
> > 1.7.10.4
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list