[ovs-dev] [cleanups 09/13] classifier: Drop CLS_INC_* enumerations and related 'include' parameters.
Ethan Jackson
ethan at nicira.com
Sat Nov 13 00:01:51 UTC 2010
Looks Good.
On Fri, Oct 29, 2010 at 4:37 PM, Ben Pfaff <blp at nicira.com> wrote:
> This type and these parameters were useful when ofproto had the need to
> separately traverse exact-match rules looking for subrules, but it no
> longer does that because subrules (now called "facets") are not kept in
> the classifier any longer. All the callers are now passing CLS_INC_ALL
> anyhow, so we might as well delete this feature and simplify the code.
> ---
> lib/classifier.c | 64 +++++++++++++-----------------------------
> lib/classifier.h | 12 ++------
> ofproto/ofproto.c | 64 +++++++++++++++++++++---------------------
> tests/test-classifier.c | 70 +++++++++++++++++++----------------------------
> 4 files changed, 83 insertions(+), 127 deletions(-)
>
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 0b450a2..c3c800d 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -34,8 +34,6 @@ static struct cls_table *classifier_next_table(const struct classifier *,
> const struct cls_table *);
> void classifier_destroy_table(struct classifier *, struct cls_table *);
>
> -static bool should_include(const struct cls_table *, int include);
> -
> static struct cls_rule *find_match(const struct cls_table *,
> const struct flow *);
> static struct cls_rule *find_equal(struct cls_table *, const struct flow *,
> @@ -471,24 +469,18 @@ classifier_remove(struct classifier *cls, struct cls_rule *rule)
>
> /* Finds and returns the highest-priority rule in 'cls' that matches 'flow'.
> * Returns a null pointer if no rules in 'cls' match 'flow'. If multiple rules
> - * of equal priority match 'flow', returns one arbitrarily.
> - *
> - * 'include' is a combination of CLS_INC_* values that specify tables to
> - * include in the search. */
> + * of equal priority match 'flow', returns one arbitrarily. */
> struct cls_rule *
> -classifier_lookup(const struct classifier *cls, const struct flow *flow,
> - int include)
> +classifier_lookup(const struct classifier *cls, const struct flow *flow)
> {
> struct cls_table *table;
> struct cls_rule *best;
>
> best = NULL;
> HMAP_FOR_EACH (table, hmap_node, &cls->tables) {
> - if (should_include(table, include)) {
> - struct cls_rule *rule = find_match(table, flow);
> - if (rule && (!best || rule->priority > best->priority)) {
> - best = rule;
> - }
> + struct cls_rule *rule = find_match(table, flow);
> + if (rule && (!best || rule->priority > best->priority)) {
> + best = rule;
> }
> }
> return best;
> @@ -591,14 +583,13 @@ classifier_rule_overlaps(const struct classifier *cls,
> void
> classifier_for_each_match(const struct classifier *cls_,
> const struct cls_rule *target,
> - int include, cls_cb_func *callback, void *aux)
> + cls_cb_func *callback, void *aux)
> {
> struct classifier *cls = (struct classifier *) cls_;
> struct cls_table *table, *next_table;
>
> for (table = classifier_first_table(cls); table; table = next_table) {
> - if (should_include(table, include)
> - && !flow_wildcards_has_extra(&table->wc, &target->wc)) {
> + if (!flow_wildcards_has_extra(&table->wc, &target->wc)) {
> /* We have eliminated the "no" case in the truth table above. Two
> * of the three remaining cases are trivial. We only need to check
> * the fourth case, where both 'rule' and 'target' require an exact
> @@ -628,12 +619,9 @@ classifier_for_each_match(const struct classifier *cls_,
>
> /* 'callback' is allowed to delete the rule that is passed as its argument, but
> * it must not delete (or move) any other rules in 'cls' that have the same
> - * wildcards as the argument rule.
> - *
> - * If 'include' is CLS_INC_EXACT then CLASSIFIER_FOR_EACH_EXACT_RULE(_SAFE) is
> - * probably easier to use. */
> + * wildcards as the argument rule. */
> void
> -classifier_for_each(const struct classifier *cls_, int include,
> +classifier_for_each(const struct classifier *cls_,
> void (*callback)(struct cls_rule *, void *aux),
> void *aux)
> {
> @@ -641,24 +629,20 @@ classifier_for_each(const struct classifier *cls_, int include,
> struct cls_table *table, *next_table;
>
> for (table = classifier_first_table(cls); table; table = next_table) {
> - if (should_include(table, include)) {
> - struct cls_rule *head, *next_head;
> + struct cls_rule *head, *next_head;
>
> - table->n_refs++;
> - HMAP_FOR_EACH_SAFE (head, next_head, hmap_node, &table->rules) {
> - struct cls_rule *rule, *next_rule;
> + table->n_refs++;
> + HMAP_FOR_EACH_SAFE (head, next_head, hmap_node, &table->rules) {
> + struct cls_rule *rule, *next_rule;
>
> - FOR_EACH_RULE_IN_LIST_SAFE (rule, next_rule, head) {
> - callback(rule, aux);
> - }
> + FOR_EACH_RULE_IN_LIST_SAFE (rule, next_rule, head) {
> + callback(rule, aux);
> }
> - next_table = classifier_next_table(cls, table);
> - if (!--table->n_refs && !table->n_table_rules) {
> - hmap_remove(&cls->tables, &table->hmap_node);
> - free(table);
> - }
> - } else {
> - next_table = classifier_next_table(cls, table);
> + }
> + next_table = classifier_next_table(cls, table);
> + if (!--table->n_refs && !table->n_table_rules) {
> + hmap_remove(&cls->tables, &table->hmap_node);
> + free(table);
> }
> }
> }
> @@ -712,14 +696,6 @@ classifier_destroy_table(struct classifier *cls, struct cls_table *table)
> free(table);
> }
>
> -/* Returns true if 'table' should be included by an operation with the
> - * specified 'include' (a combination of CLS_INC_*). */
> -static bool
> -should_include(const struct cls_table *table, int include)
> -{
> - return include & (table->wc.wildcards ? CLS_INC_WILD : CLS_INC_EXACT);
> -}
> -
> static struct cls_rule *
> find_match(const struct cls_table *table, const struct flow *flow)
> {
> diff --git a/lib/classifier.h b/lib/classifier.h
> index a5a013c..bec29c1 100644
> --- a/lib/classifier.h
> +++ b/lib/classifier.h
> @@ -66,12 +66,6 @@ struct cls_rule {
> unsigned int priority; /* Larger numbers are higher priorities. */
> };
>
> -enum {
> - CLS_INC_EXACT = 1 << 0, /* Include exact-match flows? */
> - CLS_INC_WILD = 1 << 1, /* Include flows with wildcards? */
> - CLS_INC_ALL = CLS_INC_EXACT | CLS_INC_WILD
> -};
> -
> void cls_rule_init(const struct flow *, const struct flow_wildcards *,
> unsigned int priority, struct cls_rule *);
> void cls_rule_init_exact(const struct flow *, unsigned int priority,
> @@ -114,17 +108,17 @@ struct cls_rule *classifier_insert(struct classifier *, struct cls_rule *);
> void classifier_insert_exact(struct classifier *, struct cls_rule *);
> void classifier_remove(struct classifier *, struct cls_rule *);
> struct cls_rule *classifier_lookup(const struct classifier *,
> - const struct flow *, int include);
> + const struct flow *);
> bool classifier_rule_overlaps(const struct classifier *,
> const struct cls_rule *);
>
> typedef void cls_cb_func(struct cls_rule *, void *aux);
>
> -void classifier_for_each(const struct classifier *, int include,
> +void classifier_for_each(const struct classifier *,
> cls_cb_func *, void *aux);
> void classifier_for_each_match(const struct classifier *,
> const struct cls_rule *,
> - int include, cls_cb_func *, void *aux);
> + cls_cb_func *, void *aux);
> struct cls_rule *classifier_find_rule_exactly(const struct classifier *,
> const struct cls_rule *);
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 78ae650..d5d4140 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1378,7 +1378,7 @@ ofproto_flush_flows(struct ofproto *ofproto)
> facet->installed = false;
> facet_remove(ofproto, facet);
> }
> - classifier_for_each(&ofproto->cls, CLS_INC_ALL, destroy_rule, ofproto);
> + classifier_for_each(&ofproto->cls, destroy_rule, ofproto);
> dpif_flow_flush(ofproto->dpif);
> if (ofproto->in_band) {
> in_band_flushed(ofproto->in_band);
> @@ -2599,8 +2599,7 @@ add_output_action(struct action_xlate_ctx *ctx, uint16_t port)
> static struct rule *
> rule_lookup(struct ofproto *ofproto, const struct flow *flow)
> {
> - return rule_from_cls_rule(classifier_lookup(&ofproto->cls, flow,
> - CLS_INC_ALL));
> + return rule_from_cls_rule(classifier_lookup(&ofproto->cls, flow));
> }
>
> static void
> @@ -3430,10 +3429,10 @@ flow_stats_cb(struct cls_rule *rule_, void *cbdata_)
> }
> }
>
> -static int
> -table_id_to_include(uint8_t table_id)
> +static bool
> +is_valid_table(uint8_t table_id)
> {
> - return table_id == 0 || table_id == 0xff ? CLS_INC_ALL : 0;
> + return table_id == 0 || table_id == 0xff;
> }
>
> static int
> @@ -3443,7 +3442,6 @@ handle_flow_stats_request(struct ofproto *p, struct ofconn *ofconn,
> {
> struct ofp_flow_stats_request *fsr;
> struct flow_stats_cbdata cbdata;
> - struct cls_rule target;
>
> if (arg_size != sizeof *fsr) {
> return ofp_mkerr(OFPET_BAD_REQUEST, OFPBRC_BAD_LEN);
> @@ -3451,14 +3449,17 @@ handle_flow_stats_request(struct ofproto *p, struct ofconn *ofconn,
> fsr = (struct ofp_flow_stats_request *) osr->body;
>
> COVERAGE_INC(ofproto_flows_req);
> - cbdata.ofproto = p;
> - cbdata.ofconn = ofconn;
> - cbdata.out_port = fsr->out_port;
> cbdata.msg = start_stats_reply(osr, 1024);
> - cls_rule_from_match(&fsr->match, 0, ofconn->flow_format, 0, &target);
> - classifier_for_each_match(&p->cls, &target,
> - table_id_to_include(fsr->table_id),
> - flow_stats_cb, &cbdata);
> + if (is_valid_table(fsr->table_id)) {
> + struct cls_rule target;
> +
> + cbdata.ofproto = p;
> + cbdata.ofconn = ofconn;
> + cbdata.out_port = fsr->out_port;
> + cls_rule_from_match(&fsr->match, 0, ofconn->flow_format, 0, &target);
> + classifier_for_each_match(&p->cls, &target, flow_stats_cb, &cbdata);
> + }
> +
> queue_tx(cbdata.msg, ofconn, ofconn->reply_counter);
> return 0;
> }
> @@ -3520,13 +3521,13 @@ handle_nxst_flow(struct ofproto *p, struct ofconn *ofconn, struct ofpbuf *b)
> }
>
> COVERAGE_INC(ofproto_flows_req);
> - cbdata.ofproto = p;
> - cbdata.ofconn = ofconn;
> - cbdata.out_port = nfsr->out_port;
> cbdata.msg = start_nxstats_reply(&nfsr->nsm, 1024);
> - classifier_for_each_match(&p->cls, &target,
> - table_id_to_include(nfsr->table_id),
> - nx_flow_stats_cb, &cbdata);
> + if (is_valid_table(nfsr->table_id)) {
> + cbdata.ofproto = p;
> + cbdata.ofconn = ofconn;
> + cbdata.out_port = nfsr->out_port;
> + classifier_for_each_match(&p->cls, &target, nx_flow_stats_cb, &cbdata);
> + }
> queue_tx(cbdata.msg, ofconn, ofconn->reply_counter);
> return 0;
> }
> @@ -3580,8 +3581,7 @@ ofproto_get_all_flows(struct ofproto *p, struct ds *results)
> cbdata.results = results;
>
> cls_rule_from_match(&match, 0, NXFF_OPENFLOW10, 0, &target);
> - classifier_for_each_match(&p->cls, &target, CLS_INC_ALL,
> - flow_stats_ds_cb, &cbdata);
> + classifier_for_each_match(&p->cls, &target, flow_stats_ds_cb, &cbdata);
> }
>
> struct aggregate_stats_cbdata {
> @@ -3618,14 +3618,16 @@ query_aggregate_stats(struct ofproto *ofproto, struct cls_rule *target,
> struct aggregate_stats_cbdata cbdata;
>
> COVERAGE_INC(ofproto_agg_request);
> - cbdata.ofproto = ofproto;
> - cbdata.out_port = out_port;
> cbdata.packet_count = 0;
> cbdata.byte_count = 0;
> cbdata.n_flows = 0;
> - classifier_for_each_match(&ofproto->cls, target,
> - table_id_to_include(table_id),
> - aggregate_stats_cb, &cbdata);
> + if (is_valid_table(table_id)) {
> + cbdata.ofproto = ofproto;
> + cbdata.out_port = out_port;
> +
> + classifier_for_each_match(&ofproto->cls, target,
> + aggregate_stats_cb, &cbdata);
> + }
>
> oasr->flow_count = htonl(cbdata.n_flows);
> oasr->packet_count = htonll(cbdata.packet_count);
> @@ -4002,8 +4004,7 @@ modify_flows_loose(struct ofproto *p, struct ofconn *ofconn,
> cbdata.fm = fm;
> cbdata.match = NULL;
>
> - classifier_for_each_match(&p->cls, &fm->cr, CLS_INC_ALL,
> - modify_flows_cb, &cbdata);
> + classifier_for_each_match(&p->cls, &fm->cr, modify_flows_cb, &cbdata);
> if (cbdata.match) {
> /* This credits the packet to whichever flow happened to happened to
> * match last. That's weird. Maybe we should do a lookup for the
> @@ -4093,8 +4094,7 @@ delete_flows_loose(struct ofproto *p, const struct flow_mod *fm)
> cbdata.ofproto = p;
> cbdata.out_port = htons(fm->out_port);
>
> - classifier_for_each_match(&p->cls, &fm->cr, CLS_INC_ALL,
> - delete_flows_cb, &cbdata);
> + classifier_for_each_match(&p->cls, &fm->cr, delete_flows_cb, &cbdata);
> }
>
> /* Implements OFPFC_DELETE_STRICT. */
> @@ -4628,7 +4628,7 @@ ofproto_expire(struct ofproto *ofproto)
>
> /* Expire OpenFlow flows whose idle_timeout or hard_timeout has passed. */
> cbdata.ofproto = ofproto;
> - classifier_for_each(&ofproto->cls, CLS_INC_ALL, rule_expire, &cbdata);
> + classifier_for_each(&ofproto->cls, rule_expire, &cbdata);
>
> /* Let the hook know that we're at a stable point: all outstanding data
> * in existing flows has been accounted to the account_cb. Thus, the
> diff --git a/tests/test-classifier.c b/tests/test-classifier.c
> index 45dd338..b6a8f66 100644
> --- a/tests/test-classifier.c
> +++ b/tests/test-classifier.c
> @@ -229,15 +229,13 @@ match(const struct cls_rule *wild, const struct flow *fixed)
> }
>
> static struct cls_rule *
> -tcls_lookup(const struct tcls *cls, const struct flow *flow, int include)
> +tcls_lookup(const struct tcls *cls, const struct flow *flow)
> {
> size_t i;
>
> for (i = 0; i < cls->n_rules; i++) {
> struct test_rule *pos = cls->rules[i];
> - uint32_t wildcards = pos->cls_rule.wc.wildcards;
> - if (include & (wildcards ? CLS_INC_WILD : CLS_INC_EXACT)
> - && match(&pos->cls_rule, flow)) {
> + if (match(&pos->cls_rule, flow)) {
> return &pos->cls_rule;
> }
> }
> @@ -245,17 +243,13 @@ tcls_lookup(const struct tcls *cls, const struct flow *flow, int include)
> }
>
> static void
> -tcls_delete_matches(struct tcls *cls,
> - const struct cls_rule *target,
> - int include)
> +tcls_delete_matches(struct tcls *cls, const struct cls_rule *target)
> {
> size_t i;
>
> for (i = 0; i < cls->n_rules; ) {
> struct test_rule *pos = cls->rules[i];
> - uint32_t wildcards = pos->cls_rule.wc.wildcards;
> - if (include & (wildcards ? CLS_INC_WILD : CLS_INC_EXACT)
> - && !flow_wildcards_has_extra(&pos->cls_rule.wc, &target->wc)
> + if (!flow_wildcards_has_extra(&pos->cls_rule.wc, &target->wc)
> && match(target, &pos->cls_rule.flow)) {
> tcls_remove(cls, pos);
> } else {
> @@ -264,20 +258,19 @@ tcls_delete_matches(struct tcls *cls,
> }
> }
>
> -static uint32_t nw_src_values[] = { CONSTANT_HTONL(0xc0a80001),
> +static ovs_be32 nw_src_values[] = { CONSTANT_HTONL(0xc0a80001),
> CONSTANT_HTONL(0xc0a04455) };
> -static uint32_t nw_dst_values[] = { CONSTANT_HTONL(0xc0a80002),
> +static ovs_be32 nw_dst_values[] = { CONSTANT_HTONL(0xc0a80002),
> CONSTANT_HTONL(0xc0a04455) };
> -static uint32_t tun_id_values[] = { 0, 0xffff0000 };
> -static uint16_t in_port_values[] = { CONSTANT_HTONS(1),
> - CONSTANT_HTONS(OFPP_LOCAL) };
> -static uint16_t dl_vlan_values[] = { CONSTANT_HTONS(101), CONSTANT_HTONS(0) };
> +static ovs_be32 tun_id_values[] = { 0, 0xffff0000 };
> +static uint16_t in_port_values[] = { 1, ODPP_LOCAL };
> +static ovs_be16 dl_vlan_values[] = { CONSTANT_HTONS(101), CONSTANT_HTONS(0) };
> static uint8_t dl_vlan_pcp_values[] = { 7, 0 };
> -static uint16_t dl_type_values[]
> +static ovs_be16 dl_type_values[]
> = { CONSTANT_HTONS(ETH_TYPE_IP), CONSTANT_HTONS(ETH_TYPE_ARP) };
> -static uint16_t tp_src_values[] = { CONSTANT_HTONS(49362),
> +static ovs_be16 tp_src_values[] = { CONSTANT_HTONS(49362),
> CONSTANT_HTONS(80) };
> -static uint16_t tp_dst_values[] = { CONSTANT_HTONS(6667), CONSTANT_HTONS(22) };
> +static ovs_be16 tp_dst_values[] = { CONSTANT_HTONS(6667), CONSTANT_HTONS(22) };
> static uint8_t dl_src_values[][6] = { { 0x00, 0x02, 0xe3, 0x0f, 0x80, 0xa4 },
> { 0x5e, 0x33, 0x7f, 0x5f, 0x1e, 0x99 } };
> static uint8_t dl_dst_values[][6] = { { 0x4a, 0x27, 0x71, 0xae, 0x64, 0xc1 },
> @@ -378,7 +371,6 @@ compare_classifiers(struct classifier *cls, struct tcls *tcls)
> struct cls_rule *cr0, *cr1;
> struct flow flow;
> unsigned int x;
> - int include;
>
> x = rand () % N_FLOW_VALUES;
> flow.nw_src = nw_src_values[get_value(&x, N_NW_SRC_VALUES)];
> @@ -398,21 +390,19 @@ compare_classifiers(struct classifier *cls, struct tcls *tcls)
> flow.nw_proto = nw_proto_values[get_value(&x, N_NW_PROTO_VALUES)];
> flow.nw_tos = nw_tos_values[get_value(&x, N_NW_TOS_VALUES)];
>
> - for (include = 1; include <= 3; include++) {
> - cr0 = classifier_lookup(cls, &flow, include);
> - cr1 = tcls_lookup(tcls, &flow, include);
> - assert((cr0 == NULL) == (cr1 == NULL));
> - if (cr0 != NULL) {
> - const struct test_rule *tr0 = test_rule_from_cls_rule(cr0);
> - const struct test_rule *tr1 = test_rule_from_cls_rule(cr1);
> -
> - assert(flow_equal(&cr0->flow, &cr1->flow));
> - assert(cr0->wc.wildcards == cr1->wc.wildcards);
> - assert(cr0->priority == cr1->priority);
> - /* Skip nw_src_mask and nw_dst_mask, because they are derived
> - * members whose values are used only for optimization. */
> - assert(tr0->aux == tr1->aux);
> - }
> + cr0 = classifier_lookup(cls, &flow);
> + cr1 = tcls_lookup(tcls, &flow);
> + assert((cr0 == NULL) == (cr1 == NULL));
> + if (cr0 != NULL) {
> + const struct test_rule *tr0 = test_rule_from_cls_rule(cr0);
> + const struct test_rule *tr1 = test_rule_from_cls_rule(cr1);
> +
> + assert(flow_equal(&cr0->flow, &cr1->flow));
> + assert(cr0->wc.wildcards == cr1->wc.wildcards);
> + assert(cr0->priority == cr1->priority);
> + /* Skip nw_src_mask and nw_dst_mask, because they are derived
> + * members whose values are used only for optimization. */
> + assert(tr0->aux == tr1->aux);
> }
> }
> }
> @@ -427,7 +417,7 @@ free_rule(struct cls_rule *cls_rule, void *cls)
> static void
> destroy_classifier(struct classifier *cls)
> {
> - classifier_for_each(cls, CLS_INC_ALL, free_rule, cls);
> + classifier_for_each(cls, free_rule, cls);
> classifier_destroy(cls);
> }
>
> @@ -868,12 +858,8 @@ test_many_rules_in_n_tables(int n_tables)
> while (!classifier_is_empty(&cls)) {
> struct test_rule *rule = xmemdup(tcls.rules[rand() % tcls.n_rules],
> sizeof(struct test_rule));
> - int include = rand() % 2 ? CLS_INC_WILD : CLS_INC_EXACT;
> - include |= (rule->cls_rule.wc.wildcards
> - ? CLS_INC_WILD : CLS_INC_EXACT);
> - classifier_for_each_match(&cls, &rule->cls_rule, include,
> - free_rule, &cls);
> - tcls_delete_matches(&tcls, &rule->cls_rule, include);
> + classifier_for_each_match(&cls, &rule->cls_rule, free_rule, &cls);
> + tcls_delete_matches(&tcls, &rule->cls_rule);
> compare_classifiers(&cls, &tcls);
> check_tables(&cls, -1, -1, -1);
> free(rule);
> --
> 1.7.1
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
>
More information about the dev
mailing list