Skip to content

Commit

Permalink
classifier: Retire partitions.
Browse files Browse the repository at this point in the history 
Classifier partitions allowed skipping subtables when if was known
from the flow's metadata field that the subtable cannot possibly
match.  This functionality was later implemented in a more general
fashion by staged lookup, where the first stage also covers the
metadata field, among the rest of the non-packet fields in the struct
flow.  While in theory skipping a subtable on the basis of the
metadata field alone could produce more effective wildcards, on the
basis of our testsuite coverage it does not seem to be the case, as
removing the partitioning feature did not result in any test failures.

Removing the partitioning feature makes classifier lookups roughly 20%
faster when a wildcard mask is not needed, and roughly 10% faster when
a wildcard mask is needed, as tested with the test-classifier
benchmark with one lookup thread.

Found by profiling with 'perf'.

Signed-off-by: Jarno Rajahalme <[email protected]>
Acked-by: Ben Pfaff <[email protected]>
  • Loading branch information
Jarno Rajahalme committed Aug 26, 2015
1 parent 5fcff47 commit a14502a
Show file tree
Hide file tree
Showing 5 changed files with 0 additions and 288 deletions.
2 changes: 0 additions & 2 deletions lib/automake.mk
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,6 @@ lib_libopenvswitch_la_SOURCES = \
lib/syslog-provider.h \
lib/table.c \
lib/table.h \
lib/tag.c \
lib/tag.h \
lib/timer.c \
lib/timer.h \
lib/timeval.c \
Expand Down
15 changes: 0 additions & 15 deletions lib/classifier-private.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
#include "flow.h"
#include "hash.h"
#include "rculist.h"
#include "tag.h"

/* Classifier internal definitions, subject to change at any time. */

Expand All @@ -40,7 +39,6 @@ struct cls_subtable {
* following data structures. */

/* These fields are accessed by readers who care about wildcarding. */
const tag_type tag; /* Tag generated from mask for partitioning. */
const uint8_t n_indices; /* How many indices to use. */
const struct flowmap index_maps[CLS_MAX_INDICES + 1]; /* Stage maps. */
unsigned int trie_plen[CLS_MAX_TRIES]; /* Trie prefix length in 'mask'
Expand All @@ -55,16 +53,6 @@ struct cls_subtable {
/* 'mask' must be the last field. */
};

/* Associates a metadata value (that is, a value of the OpenFlow 1.1+ metadata
* field) with tags for the "cls_subtable"s that contain rules that match that
* metadata value. */
struct cls_partition {
struct cmap_node cmap_node; /* In struct classifier's 'partitions' map. */
ovs_be64 metadata; /* metadata value for this partition. */
tag_type tags; /* OR of each flow's cls_subtable tag. */
struct tag_tracker tracker; /* Tracks the bits in 'tags'. */
};

/* Internal representation of a rule in a "struct cls_subtable".
*
* The 'next' member is an element in a singly linked, null-terminated list.
Expand All @@ -77,9 +65,6 @@ struct cls_match {
OVSRCU_TYPE(struct cls_match *) next; /* Equal, lower-priority matches. */
OVSRCU_TYPE(struct cls_conjunction_set *) conj_set;

/* Accessed only by writers. */
struct cls_partition *partition;

/* Accessed by readers interested in wildcarding. */
const int priority; /* Larger numbers are higher priorities. */
struct cmap_node index_nodes[CLS_MAX_INDICES]; /* Within subtable's
Expand Down
107 changes: 0 additions & 107 deletions lib/classifier.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,6 @@ classifier_init(struct classifier *cls, const uint8_t *flow_segments)
cls->n_rules = 0;
cmap_init(&cls->subtables_map);
pvector_init(&cls->subtables);
cmap_init(&cls->partitions);
cls->n_flow_segments = 0;
if (flow_segments) {
while (cls->n_flow_segments < CLS_MAX_INDICES
Expand All @@ -343,7 +342,6 @@ void
classifier_destroy(struct classifier *cls)
{
if (cls) {
struct cls_partition *partition;
struct cls_subtable *subtable;
int i;

Expand All @@ -356,11 +354,6 @@ classifier_destroy(struct classifier *cls)
}
cmap_destroy(&cls->subtables_map);

CMAP_FOR_EACH (partition, cmap_node, &cls->partitions) {
ovsrcu_postpone(free, partition);
}
cmap_destroy(&cls->partitions);

pvector_destroy(&cls->subtables);
}
}
Expand Down Expand Up @@ -493,43 +486,6 @@ classifier_count(const struct classifier *cls)
return cls->n_rules;
}

static uint32_t
hash_metadata(ovs_be64 metadata)
{
return hash_uint64((OVS_FORCE uint64_t) metadata);
}

static struct cls_partition *
find_partition(const struct classifier *cls, ovs_be64 metadata, uint32_t hash)
{
struct cls_partition *partition;

CMAP_FOR_EACH_WITH_HASH (partition, cmap_node, hash, &cls->partitions) {
if (partition->metadata == metadata) {
return partition;
}
}

return NULL;
}

static struct cls_partition *
create_partition(struct classifier *cls, struct cls_subtable *subtable,
ovs_be64 metadata)
{
uint32_t hash = hash_metadata(metadata);
struct cls_partition *partition = find_partition(cls, metadata, hash);
if (!partition) {
partition = xmalloc(sizeof *partition);
partition->metadata = metadata;
partition->tags = 0;
tag_tracker_init(&partition->tracker);
cmap_insert(&cls->partitions, &partition->cmap_node, hash);
}
tag_tracker_add(&partition->tracker, &partition->tags, subtable->tag);
return partition;
}

static inline ovs_be32 minimatch_get_ports(const struct minimatch *match)
{
/* Could optimize to use the same map if needed for fast path. */
Expand All @@ -545,9 +501,6 @@ subtable_replace_head_rule(struct classifier *cls OVS_UNUSED,
{
/* Rule's data is already in the tries. */

new->partition = head->partition; /* Steal partition, if any. */
head->partition = NULL;

for (int i = 0; i < subtable->n_indices; i++) {
cmap_replace(&subtable->indices[i], &head->index_nodes[i],
&new->index_nodes[i], ihash[i]);
Expand Down Expand Up @@ -629,17 +582,6 @@ classifier_replace(struct classifier *cls, const struct cls_rule *rule,
subtable->ports_mask_len);
}

/* Add rule to partitions.
*
* Concurrent readers might miss seeing the rule until this update,
* which might require being fixed up by revalidation later. */
new->partition = NULL;
if (minimask_get_metadata_mask(rule->match.mask) == OVS_BE64_MAX) {
ovs_be64 metadata = miniflow_get_metadata(rule->match.flow);

new->partition = create_partition(cls, subtable, metadata);
}

/* Add new node to segment indices.
*
* Readers may find the rule in the indices before the rule is visible
Expand Down Expand Up @@ -779,7 +721,6 @@ const struct cls_rule *
classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)
{
struct cls_match *rule, *prev, *next, *head;
struct cls_partition *partition;
struct cls_conjunction_set *conj_set;
struct cls_subtable *subtable;
uint32_t basis = 0, hash, ihash[CLS_MAX_INDICES];
Expand Down Expand Up @@ -858,17 +799,6 @@ classifier_remove(struct classifier *cls, const struct cls_rule *cls_rule)
}
n_rules = cmap_remove(&subtable->rules, &rule->cmap_node, hash);

partition = rule->partition;
if (partition) {
tag_tracker_subtract(&partition->tracker, &partition->tags,
subtable->tag);
if (!partition->tags) {
cmap_remove(&cls->partitions, &partition->cmap_node,
hash_metadata(partition->metadata));
ovsrcu_postpone(free, partition);
}
}

if (n_rules == 0) {
destroy_subtable(cls, subtable);
} else {
Expand Down Expand Up @@ -1017,11 +947,8 @@ classifier_lookup__(const struct classifier *cls, cls_version_t version,
struct flow *flow, struct flow_wildcards *wc,
bool allow_conjunctive_matches)
{
const struct cls_partition *partition;
struct trie_ctx trie_ctx[CLS_MAX_TRIES];
const struct cls_match *match;
tag_type tags;

/* Highest-priority flow in 'cls' that certainly matches 'flow'. */
const struct cls_match *hard = NULL;
int hard_pri = INT_MIN; /* hard ? hard->priority : INT_MIN. */
Expand All @@ -1040,30 +967,6 @@ classifier_lookup__(const struct classifier *cls, cls_version_t version,
* startup. */
atomic_thread_fence(memory_order_acquire);

/* Determine 'tags' such that, if 'subtable->tag' doesn't intersect them,
* then 'flow' cannot possibly match in 'subtable':
*
* - If flow->metadata maps to a given 'partition', then we can use
* 'tags' for 'partition->tags'.
*
* - If flow->metadata has no partition, then no rule in 'cls' has an
* exact-match for flow->metadata. That means that we don't need to
* search any subtable that includes flow->metadata in its mask.
*
* In either case, we always need to search any cls_subtables that do not
* include flow->metadata in its mask. One way to do that would be to
* check the "cls_subtable"s explicitly for that, but that would require an
* extra branch per subtable. Instead, we mark such a cls_subtable's
* 'tags' as TAG_ALL and make sure that 'tags' is never empty. This means
* that 'tags' always intersects such a cls_subtable's 'tags', so we don't
* need a special case.
*/
partition = (cmap_is_empty(&cls->partitions)
? NULL
: find_partition(cls, flow->metadata,
hash_metadata(flow->metadata)));
tags = partition ? partition->tags : TAG_ARBITRARY;

/* Initialize trie contexts for find_match_wc(). */
for (int i = 0; i < cls->n_tries; i++) {
trie_ctx_init(&trie_ctx[i], &cls->tries[i]);
Expand All @@ -1075,11 +978,6 @@ classifier_lookup__(const struct classifier *cls, cls_version_t version,
&cls->subtables) {
struct cls_conjunction_set *conj_set;

/* Skip subtables not in our partition. */
if (!tag_intersects(tags, subtable->tag)) {
continue;
}

/* Skip subtables with no match, or where the match is lower-priority
* than some certain match we've already found. */
match = find_match_wc(subtable, version, flow, trie_ctx, cls->n_tries,
Expand Down Expand Up @@ -1603,11 +1501,6 @@ insert_subtable(struct classifier *cls, const struct minimask *mask)
}
*CONST_CAST(uint8_t *, &subtable->n_indices) = index;

*CONST_CAST(tag_type *, &subtable->tag) =
(minimask_get_metadata_mask(mask) == OVS_BE64_MAX
? tag_create_deterministic(hash)
: TAG_ALL);

for (i = 0; i < cls->n_tries; i++) {
subtable->trie_plen[i] = minimask_get_prefix_len(mask,
cls->tries[i].field);
Expand Down
64 changes: 0 additions & 64 deletions lib/tag.c
View file

This file was deleted.

Loading

0 comments on commit a14502a

Please sign in to comment.