From: Ben Pfaff Date: Thu, 28 Oct 2010 20:26:31 +0000 (-0700) Subject: classifier: Drop CLS_INC_* enumerations and related 'include' parameters. X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3c4486a5f784731b1cb289d187ad9d9e100407c3;p=openvswitch classifier: Drop CLS_INC_* enumerations and related 'include' parameters. 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. --- diff --git a/lib/classifier.c b/lib/classifier.c index 51d338b0..f568f619 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 *); static void 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 *, @@ -479,24 +477,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; @@ -600,14 +592,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 @@ -637,35 +628,28 @@ 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 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_, 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)) { - 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); - } - } - next_table = classifier_next_table(cls, table); - if (!--table->n_refs && !table->n_table_rules) { - destroy_table(cls, table); + FOR_EACH_RULE_IN_LIST_SAFE (rule, next_rule, head) { + callback(rule, aux); } - } else { - next_table = classifier_next_table(cls, table); + } + next_table = classifier_next_table(cls, table); + if (!--table->n_refs && !table->n_table_rules) { + destroy_table(cls, table); } } } @@ -719,14 +703,6 @@ 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 0d95a2ea..233893bb 100644 --- a/lib/classifier.h +++ b/lib/classifier.h @@ -67,12 +67,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, @@ -115,17 +109,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 61ef5819..4e49cb63 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -1376,7 +1376,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); @@ -2585,8 +2585,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 @@ -3418,10 +3417,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 @@ -3430,7 +3429,6 @@ handle_flow_stats_request(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); @@ -3438,13 +3436,17 @@ handle_flow_stats_request(struct ofconn *ofconn, fsr = (struct ofp_flow_stats_request *) osr->body; COVERAGE_INC(ofproto_flows_req); - cbdata.ofconn = ofconn; - cbdata.out_port = fsr->out_port; cbdata.msg = start_ofp_stats_reply(osr, 1024); - cls_rule_from_match(&fsr->match, 0, NXFF_OPENFLOW10, 0, &target); - classifier_for_each_match(&ofconn->ofproto->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.ofconn = ofconn; + cbdata.out_port = fsr->out_port; + cls_rule_from_match(&fsr->match, 0, NXFF_OPENFLOW10, 0, &target); + classifier_for_each_match(&ofconn->ofproto->cls, &target, + flow_stats_cb, &cbdata); + } + queue_tx(cbdata.msg, ofconn, ofconn->reply_counter); return 0; } @@ -3506,12 +3508,13 @@ handle_nxst_flow(struct ofconn *ofconn, struct ofpbuf *b) } COVERAGE_INC(ofproto_flows_req); - cbdata.ofconn = ofconn; - cbdata.out_port = nfsr->out_port; cbdata.msg = start_nxstats_reply(&nfsr->nsm, 1024); - classifier_for_each_match(&ofconn->ofproto->cls, &target, - table_id_to_include(nfsr->table_id), - nx_flow_stats_cb, &cbdata); + if (is_valid_table(nfsr->table_id)) { + cbdata.ofconn = ofconn; + cbdata.out_port = nfsr->out_port; + classifier_for_each_match(&ofconn->ofproto->cls, &target, + nx_flow_stats_cb, &cbdata); + } queue_tx(cbdata.msg, ofconn, ofconn->reply_counter); return 0; } @@ -3565,8 +3568,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 { @@ -3603,14 +3605,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); @@ -3989,7 +3993,7 @@ modify_flows_loose(struct ofconn *ofconn, struct flow_mod *fm) cbdata.fm = fm; cbdata.match = NULL; - classifier_for_each_match(&ofconn->ofproto->cls, &fm->cr, CLS_INC_ALL, + classifier_for_each_match(&ofconn->ofproto->cls, &fm->cr, modify_flows_cb, &cbdata); if (cbdata.match) { /* This credits the packet to whichever flow happened to happened to @@ -4080,8 +4084,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. */ @@ -4615,7 +4618,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 783c59e2..1308fa51 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); } @@ -885,12 +875,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);