Until now, struct ofputil_flow_mod, which represents an OpenFlow flow table modification request, has incorporated a struct match, which made the overall ofputil_flow_mod about 2.5 kB. This is OK for a small number of flows, but absurdly inflates memory requirements when there are hundreds of thousands of flows. This commit fixes the problem by changing struct match to struct minimatch inside ofputil_flow_mod, which reduces its size to about 100 bytes plus the actual size of the flow match (usually a few dozen bytes).
This affects memory usage of ovs-ofctl (when it adds a large number of flows) more than ovs-vswitchd. Reported-by: Michael Ben-Ami <[email protected]> Signed-off-by: Ben Pfaff <[email protected]> --- AUTHORS.rst | 1 + include/openvswitch/ofp-flow.h | 2 +- lib/learn.c | 12 ++++-- lib/learning-switch.c | 14 +++++-- lib/ofp-bundle.c | 1 + lib/ofp-flow.c | 89 ++++++++++++++++++++++++++---------------- ofproto/ofproto-dpif-xlate.c | 6 ++- ofproto/ofproto-dpif.c | 17 ++++---- ofproto/ofproto-dpif.h | 2 +- ofproto/ofproto-provider.h | 2 +- ofproto/ofproto.c | 49 ++++++++++++++--------- ovn/controller/ofctrl.c | 22 ++++++----- utilities/ovs-ofctl.c | 81 +++++++++++++++++++++++--------------- 13 files changed, 185 insertions(+), 113 deletions(-) diff --git a/AUTHORS.rst b/AUTHORS.rst index dc69ba3f3c1f..81f9b6f28b88 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -506,6 +506,7 @@ Marvin Pascual [email protected] Maxime Brun [email protected] Madhu Venugopal [email protected] Michael A. Collins [email protected] +Michael Ben-Ami [email protected] Michael Hu [email protected] Michael J. Smalley [email protected] Michael Mao [email protected] diff --git a/include/openvswitch/ofp-flow.h b/include/openvswitch/ofp-flow.h index 17d48f12e060..2ff2e45b66c4 100644 --- a/include/openvswitch/ofp-flow.h +++ b/include/openvswitch/ofp-flow.h @@ -73,7 +73,7 @@ void ofputil_flow_mod_flags_format(struct ds *, enum ofputil_flow_mod_flags); struct ofputil_flow_mod { struct ovs_list list_node; /* For queuing flow_mods. */ - struct match match; + struct minimatch match; int priority; /* Cookie matching. The flow_mod affects only flows that have cookies that diff --git a/lib/learn.c b/lib/learn.c index f5d15503fe79..c4d5b3e0c449 100644 --- a/lib/learn.c +++ b/lib/learn.c @@ -91,14 +91,17 @@ learn_check(const struct ofpact_learn *learn, const struct match *src_match) * 'ofpacts' and retains ownership of it. 'fm->ofpacts' will point into the * 'ofpacts' buffer. * + * The caller must eventually destroy fm->match. + * * The caller has to actually execute 'fm'. */ void learn_execute(const struct ofpact_learn *learn, const struct flow *flow, struct ofputil_flow_mod *fm, struct ofpbuf *ofpacts) { const struct ofpact_learn_spec *spec; + struct match match; - match_init_catchall(&fm->match); + match_init_catchall(&match); fm->priority = learn->priority; fm->cookie = htonll(0); fm->cookie_mask = htonll(0); @@ -140,10 +143,10 @@ learn_execute(const struct ofpact_learn *learn, const struct flow *flow, switch (spec->dst_type) { case NX_LEARN_DST_MATCH: - mf_write_subfield(&spec->dst, &value, &fm->match); - match_add_ethernet_prereq(&fm->match, spec->dst.field); + mf_write_subfield(&spec->dst, &value, &match); + match_add_ethernet_prereq(&match, spec->dst.field); mf_vl_mff_set_tlv_bitmap( - spec->dst.field, &fm->match.flow.tunnel.metadata.present.map); + spec->dst.field, &match.flow.tunnel.metadata.present.map); break; case NX_LEARN_DST_LOAD: @@ -173,6 +176,7 @@ learn_execute(const struct ofpact_learn *learn, const struct flow *flow, } } + minimatch_init(&fm->match, &match); fm->ofpacts = ofpacts->data; fm->ofpacts_len = ofpacts->size; } diff --git a/lib/learning-switch.c b/lib/learning-switch.c index 3a9e015bbe9c..04eaa6e8e73f 100644 --- a/lib/learning-switch.c +++ b/lib/learning-switch.c @@ -206,7 +206,6 @@ lswitch_handshake(struct lswitch *sw) output.max_len = OFP_DEFAULT_MISS_SEND_LEN; struct ofputil_flow_mod fm = { - .match = MATCH_CATCHALL_INITIALIZER, .priority = 0, .table_id = 0, .command = OFPFC_ADD, @@ -216,8 +215,10 @@ lswitch_handshake(struct lswitch *sw) .ofpacts = &output.ofpact, .ofpacts_len = sizeof output, }; - + minimatch_init_catchall(&fm.match); msg = ofputil_encode_flow_mod(&fm, protocol); + minimatch_destroy(&fm.match); + error = rconn_send(sw->rconn, msg, NULL); if (error) { VLOG_INFO_RL(&rl, "%s: failed to add default flow (%s)", @@ -595,11 +596,16 @@ process_packet_in(struct lswitch *sw, const struct ofp_header *oh) .ofpacts = ofpacts.data, .ofpacts_len = ofpacts.size, }; - match_init(&fm.match, &flow, &sw->wc); - ofputil_normalize_match_quiet(&fm.match); + + struct match match; + match_init(&match, &flow, &sw->wc); + ofputil_normalize_match_quiet(&match); + minimatch_init(&fm.match, &match); struct ofpbuf *buffer = ofputil_encode_flow_mod(&fm, sw->protocol); + minimatch_destroy(&fm.match); + queue_tx(sw, buffer); /* If the switch didn't buffer the packet, we need to send a copy. */ diff --git a/lib/ofp-bundle.c b/lib/ofp-bundle.c index 0921c81bfb15..8f07a30c51f7 100644 --- a/lib/ofp-bundle.c +++ b/lib/ofp-bundle.c @@ -33,6 +33,7 @@ ofputil_free_bundle_msgs(struct ofputil_bundle_msg *bms, size_t n_bms) switch ((int)bms[i].type) { case OFPTYPE_FLOW_MOD: free(CONST_CAST(struct ofpact *, bms[i].fm.ofpacts)); + minimatch_destroy(&bms[i].fm.match); break; case OFPTYPE_GROUP_MOD: ofputil_uninit_group_mod(&bms[i].gm); diff --git a/lib/ofp-flow.c b/lib/ofp-flow.c index cffadb30381b..d07556245b1f 100644 --- a/lib/ofp-flow.c +++ b/lib/ofp-flow.c @@ -139,6 +139,8 @@ ofputil_flow_mod_flags_format(struct ds *s, enum ofputil_flow_mod_flags flags) * The caller must initialize 'ofpacts' and retains ownership of it. * 'fm->ofpacts' will point into the 'ofpacts' buffer. * + * On success, the caller must eventually destroy fm->match. + * * Does not validate the flow_mod actions. The caller should do that, with * ofpacts_check(). */ enum ofperr @@ -152,6 +154,7 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm, { ovs_be16 raw_flags; enum ofperr error; + struct match match; struct ofpbuf b = ofpbuf_const_initializer(oh, ntohs(oh->length)); enum ofpraw raw = ofpraw_pull_assert(&b); if (raw == OFPRAW_OFPT11_FLOW_MOD) { @@ -160,7 +163,7 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm, ofm = ofpbuf_pull(&b, sizeof *ofm); - error = ofputil_pull_ofp11_match(&b, tun_table, vl_mff_map, &fm->match, + error = ofputil_pull_ofp11_match(&b, tun_table, vl_mff_map, &match, NULL); if (error) { return error; @@ -228,8 +231,8 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm, ofm = ofpbuf_pull(&b, sizeof *ofm); /* Translate the rule. */ - ofputil_match_from_ofp10_match(&ofm->match, &fm->match); - ofputil_normalize_match(&fm->match); + ofputil_match_from_ofp10_match(&ofm->match, &match); + ofputil_normalize_match(&match); /* OpenFlow 1.0 says that exact-match rules have to have the * highest possible priority. */ @@ -256,7 +259,7 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm, /* Dissect the message. */ nfm = ofpbuf_pull(&b, sizeof *nfm); error = nx_pull_match(&b, ntohs(nfm->match_len), - &fm->match, &fm->cookie, &fm->cookie_mask, + &match, &fm->cookie, &fm->cookie_mask, false, tun_table, vl_mff_map); if (error) { return error; @@ -269,6 +272,7 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm, * existing cookie. */ return OFPERR_NXBRC_NXM_INVALID; } + minimatch_init(&fm->match, &match); fm->priority = ntohs(nfm->priority); fm->new_cookie = nfm->cookie; fm->idle_timeout = ntohs(nfm->idle_timeout); @@ -298,11 +302,11 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm, /* Check for mismatched conntrack original direction tuple address fields * w.r.t. the IP version of the match. */ - if (((fm->match.wc.masks.ct_nw_src || fm->match.wc.masks.ct_nw_dst) - && fm->match.flow.dl_type != htons(ETH_TYPE_IP)) - || ((ipv6_addr_is_set(&fm->match.wc.masks.ct_ipv6_src) - || ipv6_addr_is_set(&fm->match.wc.masks.ct_ipv6_dst)) - && fm->match.flow.dl_type != htons(ETH_TYPE_IPV6))) { + if (((match.wc.masks.ct_nw_src || match.wc.masks.ct_nw_dst) + && match.flow.dl_type != htons(ETH_TYPE_IP)) + || ((ipv6_addr_is_set(&match.wc.masks.ct_ipv6_src) + || ipv6_addr_is_set(&match.wc.masks.ct_ipv6_dst)) + && match.flow.dl_type != htons(ETH_TYPE_IPV6))) { return OFPERR_OFPBAC_MATCH_INCONSISTENT; } @@ -339,9 +343,13 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm, : OFPERR_OFPFMFC_TABLE_FULL); } - return ofpacts_check_consistency(fm->ofpacts, fm->ofpacts_len, - &fm->match, max_port, - fm->table_id, max_table, protocol); + error = ofpacts_check_consistency(fm->ofpacts, fm->ofpacts_len, + &match, max_port, + fm->table_id, max_table, protocol); + if (!error) { + minimatch_init(&fm->match, &match); + } + return error; } static ovs_be16 @@ -363,6 +371,9 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm, ovs_be16 raw_flags = ofputil_encode_flow_mod_flags(fm->flags, version); struct ofpbuf *msg; + struct match match; + minimatch_expand(&fm->match, &match); + switch (protocol) { case OFPUTIL_P_OF11_STD: case OFPUTIL_P_OF12_OXM: @@ -407,7 +418,7 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm, } else { ofm->importance = 0; } - ofputil_put_ofp11_match(msg, &fm->match, protocol); + ofputil_put_ofp11_match(msg, &match, protocol); ofpacts_put_openflow_instructions(fm->ofpacts, fm->ofpacts_len, msg, version); break; @@ -420,7 +431,7 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm, msg = ofpraw_alloc(OFPRAW_OFPT10_FLOW_MOD, OFP10_VERSION, fm->ofpacts_len); ofm = ofpbuf_put_zeros(msg, sizeof *ofm); - ofputil_match_to_ofp10_match(&fm->match, &ofm->match); + ofputil_match_to_ofp10_match(&match, &ofm->match); ofm->cookie = fm->new_cookie; ofm->command = ofputil_tid_command(fm, protocol); ofm->idle_timeout = htons(fm->idle_timeout); @@ -444,7 +455,7 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm, nfm = ofpbuf_put_zeros(msg, sizeof *nfm); nfm->command = ofputil_tid_command(fm, protocol); nfm->cookie = fm->new_cookie; - match_len = nx_put_match(msg, &fm->match, fm->cookie, fm->cookie_mask); + match_len = nx_put_match(msg, &match, fm->cookie, fm->cookie_mask); nfm = msg->msg; nfm->idle_timeout = htons(fm->idle_timeout); nfm->hard_timeout = htons(fm->hard_timeout); @@ -536,7 +547,9 @@ ofputil_flow_mod_format(struct ds *s, const struct ofp_header *oh, /* nx_match_to_string() doesn't print priority. */ need_priority = true; } else { - match_format(&fm.match, port_map, s, fm.priority); + struct match match; + minimatch_expand(&fm.match, &match); + match_format(&match, port_map, s, fm.priority); /* match_format() does print priority. */ need_priority = false; @@ -589,6 +602,7 @@ ofputil_flow_mod_format(struct ds *s, const struct ofp_header *oh, }; ofpacts_format(fm.ofpacts, fm.ofpacts_len, &fp); ofpbuf_uninit(&ofpacts); + minimatch_destroy(&fm.match); return 0; } @@ -831,10 +845,13 @@ parse_ofp_flow_stats_request_str(struct ofputil_flow_stats_request *fsr, fsr->aggregate = aggregate; fsr->cookie = fm.cookie; fsr->cookie_mask = fm.cookie_mask; - fsr->match = fm.match; + minimatch_expand(&fm.match, &fsr->match); fsr->out_port = fm.out_port; fsr->out_group = fm.out_group; fsr->table_id = fm.table_id; + + minimatch_destroy(&fm.match); + return NULL; } @@ -1393,7 +1410,6 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string, } *fm = (struct ofputil_flow_mod) { - .match = MATCH_CATCHALL_INITIALIZER, .priority = OFP_DEFAULT_PRIORITY, .table_id = 0xff, .command = command, @@ -1413,19 +1429,20 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string, } } + struct match match = MATCH_CATCHALL_INITIALIZER; while (ofputil_parse_key_value(&string, &name, &value)) { const struct ofp_protocol *p; const struct mf_field *mf; char *error = NULL; if (ofp_parse_protocol(name, &p)) { - match_set_dl_type(&fm->match, htons(p->dl_type)); + match_set_dl_type(&match, htons(p->dl_type)); if (p->nw_proto) { - match_set_nw_proto(&fm->match, p->nw_proto); + match_set_nw_proto(&match, p->nw_proto); } - match_set_default_packet_type(&fm->match); + match_set_default_packet_type(&match); } else if (!strcmp(name, "eth")) { - match_set_packet_type(&fm->match, htonl(PT_ETH)); + match_set_packet_type(&match, htonl(PT_ETH)); } else if (fields & F_FLAGS && !strcmp(name, "send_flow_rem")) { fm->flags |= OFPUTIL_FF_SEND_FLOW_REM; } else if (fields & F_FLAGS && !strcmp(name, "check_overlap")) { @@ -1444,9 +1461,9 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string, /* ignore these fields. */ } else if ((mf = mf_from_name(name)) != NULL) { error = ofp_parse_field(mf, value, port_map, - &fm->match, usable_protocols); + &match, usable_protocols); } else if (strchr(name, '[')) { - error = parse_subfield(name, value, &fm->match, usable_protocols); + error = parse_subfield(name, value, &match, usable_protocols); } else { if (!*value) { return xasprintf("field %s missing value", name); @@ -1532,13 +1549,13 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string, } /* Copy ethertype to flow->dl_type for matches on packet_type * (OFPHTN_ETHERTYPE, ethertype). */ - if (fm->match.wc.masks.packet_type == OVS_BE32_MAX && - pt_ns(fm->match.flow.packet_type) == OFPHTN_ETHERTYPE) { - fm->match.flow.dl_type = pt_ns_type_be(fm->match.flow.packet_type); + if (match.wc.masks.packet_type == OVS_BE32_MAX && + pt_ns(match.flow.packet_type) == OFPHTN_ETHERTYPE) { + match.flow.dl_type = pt_ns_type_be(match.flow.packet_type); } /* Check for usable protocol interdependencies between match fields. */ - if (fm->match.flow.dl_type == htons(ETH_TYPE_IPV6)) { - const struct flow_wildcards *wc = &fm->match.wc; + if (match.flow.dl_type == htons(ETH_TYPE_IPV6)) { + const struct flow_wildcards *wc = &match.wc; /* Only NXM and OXM support matching L3 and L4 fields within IPv6. * * (IPv6 specific fields as well as arp_sha, arp_tha, nw_frag, and @@ -1574,7 +1591,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string, if (!error) { enum ofperr err; - err = ofpacts_check(ofpacts.data, ofpacts.size, &fm->match, + err = ofpacts_check(ofpacts.data, ofpacts.size, &match, OFPP_MAX, fm->table_id, 255, usable_protocols); if (!err && !*usable_protocols) { err = OFPERR_OFPBAC_MATCH_INCONSISTENT; @@ -1596,6 +1613,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string, fm->ofpacts_len = 0; fm->ofpacts = NULL; } + minimatch_init(&fm->match, &match); return NULL; } @@ -1613,7 +1631,10 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string, * name is treated as "add". * * Returns NULL if successful, otherwise a malloc()'d string describing the - * error. The caller is responsible for freeing the returned string. */ + * error. The caller is responsible for freeing the returned string. + * + * On success, the caller is responsible for freeing fm->ofpacts and + * fm->match. */ char * OVS_WARN_UNUSED_RESULT parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_, const struct ofputil_port_map *port_map, @@ -1657,8 +1678,9 @@ parse_ofp_flow_mod_str(struct ofputil_flow_mod *fm, const char *string, /* Normalize a copy of the match. This ensures that non-normalized * flows get logged but doesn't affect what gets sent to the switch, so * that the switch can do whatever it likes with the flow. */ - struct match match_copy = fm->match; - ofputil_normalize_match(&match_copy); + struct match match; + minimatch_expand(&fm->match, &match); + ofputil_normalize_match(&match); } return error; @@ -1716,6 +1738,7 @@ parse_ofp_flow_mod_file(const char *file_name, for (i = 0; i < *n_fms; i++) { free(CONST_CAST(struct ofpact *, (*fms)[i].ofpacts)); + minimatch_destroy(&(*fms)[i].match); } free(*fms); *fms = NULL; diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index bc6429c25346..a00bed704806 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -5019,7 +5019,9 @@ xlate_learn_action(struct xlate_ctx *ctx, const struct ofpact_learn *learn) if (OVS_UNLIKELY(ctx->xin->trace)) { struct ds s = DS_EMPTY_INITIALIZER; ds_put_format(&s, "table=%"PRIu8" ", fm.table_id); - match_format(&fm.match, NULL, &s, OFP_DEFAULT_PRIORITY); + minimatch_format(&fm.match, + ofproto_get_tun_tab(&ctx->xin->ofproto->up), + NULL, &s, OFP_DEFAULT_PRIORITY); ds_chomp(&s, ' '); ds_put_format(&s, " priority=%d", fm.priority); if (fm.new_cookie) { @@ -5090,6 +5092,8 @@ xlate_learn_action(struct xlate_ctx *ctx, const struct ofpact_learn *learn) xlate_report_error(ctx, "LEARN action execution failed (%s).", ofperr_to_string(error)); } + + minimatch_destroy(&fm.match); } else { xlate_report(ctx, OFT_WARN, "suppressing side effects, so learn action ignored"); diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index c92c5bea1725..1ed82d036528 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -5582,9 +5582,11 @@ odp_port_to_ofp_port(const struct ofproto_dpif *ofproto, odp_port_t odp_port) } } +/* 'match' is non-const to allow for temporary modifications. Any changes are + * restored before returning. */ int ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto, - const struct match *match, int priority, + struct match *match, int priority, uint16_t idle_timeout, const struct ofpbuf *ofpacts, struct rule **rulep) @@ -5595,7 +5597,6 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto, fm = (struct ofputil_flow_mod) { .buffer_id = UINT32_MAX, - .match = *match, .priority = priority, .table_id = TBL_INTERNAL, .command = OFPFC_ADD, @@ -5604,8 +5605,10 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto, .ofpacts = ofpacts->data, .ofpacts_len = ofpacts->size, }; - + minimatch_init(&fm.match, match); error = ofproto_flow_mod(&ofproto->up, &fm); + minimatch_destroy(&fm.match); + if (error) { VLOG_ERR_RL(&rl, "failed to add internal flow (%s)", ofperr_to_string(error)); @@ -5615,8 +5618,7 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto, rule = rule_dpif_lookup_in_table(ofproto, ofproto_dpif_get_tables_version(ofproto), - TBL_INTERNAL, &fm.match.flow, - &fm.match.wc); + TBL_INTERNAL, &match->flow, &match->wc); if (rule) { *rulep = &rule->up; } else { @@ -5634,7 +5636,6 @@ ofproto_dpif_delete_internal_flow(struct ofproto_dpif *ofproto, fm = (struct ofputil_flow_mod) { .buffer_id = UINT32_MAX, - .match = *match, .priority = priority, .table_id = TBL_INTERNAL, .out_port = OFPP_ANY, @@ -5642,8 +5643,10 @@ ofproto_dpif_delete_internal_flow(struct ofproto_dpif *ofproto, .flags = OFPUTIL_FF_HIDDEN_FIELDS | OFPUTIL_FF_NO_READONLY, .command = OFPFC_DELETE_STRICT, }; - + minimatch_init(&fm.match, match); error = ofproto_flow_mod(&ofproto->up, &fm); + minimatch_destroy(&fm.match); + if (error) { VLOG_ERR_RL(&rl, "failed to delete internal flow (%s)", ofperr_to_string(error)); diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index 90432fa2678b..47bf7f9f0ac2 100644 --- a/ofproto/ofproto-dpif.h +++ b/ofproto/ofproto-dpif.h @@ -335,7 +335,7 @@ struct ofport_dpif *ofp_port_to_ofport(const struct ofproto_dpif *, ofp_port_t); int ofproto_dpif_add_internal_flow(struct ofproto_dpif *, - const struct match *, int priority, + struct match *, int priority, uint16_t idle_timeout, const struct ofpbuf *ofpacts, struct rule **rulep); diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 92d4622b3290..d636fb35c4f9 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -1199,7 +1199,7 @@ struct ofproto_class { * * If this function is NULL then table 0 is always chosen. */ enum ofperr (*rule_choose_table)(const struct ofproto *ofproto, - const struct match *match, + const struct minimatch *match, uint8_t *table_idp); /* Life-cycle functions for a "struct rule". diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 57ce3c81fd22..36f4c0b16d48 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -133,7 +133,7 @@ static void eviction_group_remove_rule(struct rule *) OVS_REQUIRES(ofproto_mutex); static void rule_criteria_init(struct rule_criteria *, uint8_t table_id, - const struct match *match, int priority, + const struct minimatch *match, int priority, ovs_version_t version, ovs_be64 cookie, ovs_be64 cookie_mask, ofp_port_t out_port, uint32_t out_group); @@ -2104,7 +2104,6 @@ flow_mod_init(struct ofputil_flow_mod *fm, enum ofp_flow_mod_command command) { *fm = (struct ofputil_flow_mod) { - .match = *match, .priority = priority, .table_id = 0, .command = command, @@ -2114,6 +2113,7 @@ flow_mod_init(struct ofputil_flow_mod *fm, .ofpacts = CONST_CAST(struct ofpact *, ofpacts), .ofpacts_len = ofpacts_len, }; + minimatch_init(&fm->match, match); } static int @@ -2123,10 +2123,10 @@ simple_flow_mod(struct ofproto *ofproto, enum ofp_flow_mod_command command) { struct ofputil_flow_mod fm; - flow_mod_init(&fm, match, priority, ofpacts, ofpacts_len, command); - - return handle_flow_mod__(ofproto, &fm, NULL); + enum ofperr error = handle_flow_mod__(ofproto, &fm, NULL); + minimatch_destroy(&fm.match); + return error; } /* Adds a flow to OpenFlow flow table 0 in 'p' that matches 'cls_rule' and @@ -3173,12 +3173,12 @@ learned_cookies_flush(struct ofproto *ofproto, struct ovs_list *dead_cookies) { struct learned_cookie *c; + struct minimatch match; + + minimatch_init_catchall(&match); LIST_FOR_EACH_POP (c, u.list_node, dead_cookies) { struct rule_criteria criteria; struct rule_collection rules; - struct match match; - - match_init_catchall(&match); rule_criteria_init(&criteria, c->table_id, &match, 0, OVS_VERSION_MAX, c->cookie, OVS_BE64_MAX, OFPP_ANY, OFPG_ANY); rule_criteria_require_rw(&criteria, false); @@ -3188,6 +3188,7 @@ learned_cookies_flush(struct ofproto *ofproto, struct ovs_list *dead_cookies) free(c); } + minimatch_destroy(&match); } static enum ofperr @@ -4051,13 +4052,13 @@ next_matching_table(const struct ofproto *ofproto, * supplied as 0. */ static void rule_criteria_init(struct rule_criteria *criteria, uint8_t table_id, - const struct match *match, int priority, + const struct minimatch *match, int priority, ovs_version_t version, ovs_be64 cookie, ovs_be64 cookie_mask, ofp_port_t out_port, uint32_t out_group) { criteria->table_id = table_id; - cls_rule_init(&criteria->cr, match, priority); + cls_rule_init_from_minimatch(&criteria->cr, match, priority); criteria->version = version; criteria->cookie = cookie; criteria->cookie_mask = cookie_mask; @@ -4303,9 +4304,12 @@ handle_flow_stats_request(struct ofconn *ofconn, return error; } - rule_criteria_init(&criteria, fsr.table_id, &fsr.match, 0, OVS_VERSION_MAX, + struct minimatch match; + minimatch_init(&match, &fsr.match); + rule_criteria_init(&criteria, fsr.table_id, &match, 0, OVS_VERSION_MAX, fsr.cookie, fsr.cookie_mask, fsr.out_port, fsr.out_group); + minimatch_destroy(&match); ovs_mutex_lock(&ofproto_mutex); error = collect_rules_loose(ofproto, &criteria, &rules); @@ -4470,9 +4474,12 @@ handle_aggregate_stats_request(struct ofconn *ofconn, return error; } - rule_criteria_init(&criteria, request.table_id, &request.match, 0, + struct minimatch match; + minimatch_init(&match, &request.match); + rule_criteria_init(&criteria, request.table_id, &match, 0, OVS_VERSION_MAX, request.cookie, request.cookie_mask, request.out_port, request.out_group); + minimatch_destroy(&match); ovs_mutex_lock(&ofproto_mutex); error = collect_rules_loose(ofproto, &criteria, &rules); @@ -4746,22 +4753,22 @@ add_flow_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm, } if (!(fm->flags & OFPUTIL_FF_HIDDEN_FIELDS) - && !match_has_default_hidden_fields(&fm->match)) { + && !minimatch_has_default_hidden_fields(&fm->match)) { VLOG_WARN_RL(&rl, "%s: (add_flow) only internal flows can set " "non-default values to hidden fields", ofproto->name); return OFPERR_OFPBRC_EPERM; } if (!ofm->temp_rule) { - cls_rule_init(&cr, &fm->match, fm->priority); + cls_rule_init_from_minimatch(&cr, &fm->match, fm->priority); /* Allocate new rule. Destroys 'cr'. */ + uint64_t map = miniflow_get_tun_metadata_present_map(fm->match.flow); error = ofproto_rule_create(ofproto, &cr, table - ofproto->tables, fm->new_cookie, fm->idle_timeout, fm->hard_timeout, fm->flags, fm->importance, fm->ofpacts, - fm->ofpacts_len, - fm->match.flow.tunnel.metadata.present.map, + fm->ofpacts_len, map, fm->ofpacts_tlv_bitmap, &ofm->temp_rule); if (error) { return error; @@ -4958,7 +4965,7 @@ ofproto_flow_mod_init_for_learn(struct ofproto *ofproto, struct oftable *table = &ofproto->tables[fm->table_id]; struct rule *rule; - rule = rule_from_cls_rule(classifier_find_match_exactly( + rule = rule_from_cls_rule(classifier_find_minimatch_exactly( &table->cls, &fm->match, fm->priority, OVS_VERSION_MAX)); if (rule) { @@ -5108,12 +5115,14 @@ ofproto_flow_mod_learn(struct ofproto_flow_mod *ofm, bool keep_ref, if (limit) { struct rule_criteria criteria; struct rule_collection rules; - struct match match; + struct minimatch match; - match_init_catchall(&match); + minimatch_init_catchall(&match); rule_criteria_init(&criteria, rule->table_id, &match, 0, OVS_VERSION_MAX, rule->flow_cookie, OVS_BE64_MAX, OFPP_ANY, OFPG_ANY); + minimatch_destroy(&match); + rule_criteria_require_rw(&criteria, false); collect_rules_loose(rule->ofproto, &criteria, &rules); if (rule_collection_n(&rules) >= limit) { @@ -5797,6 +5806,7 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh) if (!error) { struct openflow_mod_requester req = { ofconn, oh }; error = handle_flow_mod__(ofproto, &fm, &req); + minimatch_destroy(&fm.match); } ofpbuf_uninit(&ofpacts); @@ -7921,6 +7931,7 @@ handle_bundle_add(struct ofconn *ofconn, const struct ofp_header *oh) ofproto->n_tables); if (!error) { error = ofproto_flow_mod_init(ofproto, &bmsg->ofm, &fm, NULL); + minimatch_destroy(&fm.match); } } else if (type == OFPTYPE_GROUP_MOD) { error = ofputil_decode_group_mod(badd.msg, &bmsg->ogm.gm); diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c index 8d6d1b6ae8c0..83340bbf2b30 100644 --- a/ovn/controller/ofctrl.c +++ b/ovn/controller/ofctrl.c @@ -58,7 +58,7 @@ struct ovn_flow { /* Key. */ uint8_t table_id; uint16_t priority; - struct match match; + struct minimatch match; /* Data. */ struct ofpact *ofpacts; @@ -373,14 +373,16 @@ error: static void run_S_CLEAR_FLOWS(void) { + VLOG_DBG("clearing all flows"); + /* Send a flow_mod to delete all flows. */ struct ofputil_flow_mod fm = { - .match = MATCH_CATCHALL_INITIALIZER, .table_id = OFPTT_ALL, .command = OFPFC_DELETE, }; + minimatch_init_catchall(&fm.match); queue_msg(encode_flow_mod(&fm)); - VLOG_DBG("clearing all flows"); + minimatch_destroy(&fm.match); /* Clear installed_flows, to match the state of the switch. */ ovn_flow_table_clear(&installed_flows); @@ -630,13 +632,12 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type) void ofctrl_add_flow(struct hmap *desired_flows, uint8_t table_id, uint16_t priority, uint64_t cookie, - const struct match *match, - const struct ofpbuf *actions) + const struct match *match, const struct ofpbuf *actions) { struct ovn_flow *f = xmalloc(sizeof *f); f->table_id = table_id; f->priority = priority; - f->match = *match; + minimatch_init(&f->match, match); f->ofpacts = xmemdup(actions->data, actions->size); f->ofpacts_len = actions->size; f->hmap_node.hash = ovn_flow_hash(f); @@ -665,7 +666,7 @@ static uint32_t ovn_flow_hash(const struct ovn_flow *f) { return hash_2words((f->table_id << 16) | f->priority, - match_hash(&f->match, 0)); + minimatch_hash(&f->match, 0)); } @@ -676,7 +677,7 @@ ofctrl_dup_flow(struct ovn_flow *src) struct ovn_flow *dst = xmalloc(sizeof *dst); dst->table_id = src->table_id; dst->priority = src->priority; - dst->match = src->match; + minimatch_clone(&dst->match, &src->match); dst->ofpacts = xmemdup(src->ofpacts, src->ofpacts_len); dst->ofpacts_len = src->ofpacts_len; dst->hmap_node.hash = ovn_flow_hash(dst); @@ -694,7 +695,7 @@ ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target) flow_table) { if (f->table_id == target->table_id && f->priority == target->priority - && match_equal(&f->match, &target->match)) { + && minimatch_equal(&f->match, &target->match)) { return f; } } @@ -707,7 +708,7 @@ ovn_flow_to_string(const struct ovn_flow *f) struct ds s = DS_EMPTY_INITIALIZER; ds_put_format(&s, "table_id=%"PRIu8", ", f->table_id); ds_put_format(&s, "priority=%"PRIu16", ", f->priority); - match_format(&f->match, NULL, &s, OFP_DEFAULT_PRIORITY); + minimatch_format(&f->match, NULL, NULL, &s, OFP_DEFAULT_PRIORITY); ds_put_cstr(&s, ", actions="); struct ofpact_format_params fp = { .s = &s }; ofpacts_format(f->ofpacts, f->ofpacts_len, &fp); @@ -728,6 +729,7 @@ static void ovn_flow_destroy(struct ovn_flow *f) { if (f) { + minimatch_destroy(&f->match); free(f->ofpacts); free(f); } diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index b5e2b409328e..4d9e8743e885 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -1747,6 +1747,7 @@ ofctl_flow_mod__(const char *remote, struct ofputil_flow_mod *fms, transact_noreply(vconn, ofputil_encode_flow_mod(fm, protocol)); free(CONST_CAST(struct ofpact *, fm->ofpacts)); + minimatch_destroy(&fm->match); } vconn_close(vconn); } @@ -3309,7 +3310,7 @@ struct fte_version { /* A FTE entry that has been queued for later insertion after all * flows have been scanned to correctly allocation tunnel metadata. */ struct fte_pending { - struct match *match; + struct minimatch match; int priority; struct fte_version *version; int index; @@ -3442,14 +3443,14 @@ fte_free_all(struct flow_tables *tables) * * Takes ownership of 'version'. */ static void -fte_insert(struct flow_tables *tables, const struct match *match, +fte_insert(struct flow_tables *tables, const struct minimatch *match, int priority, struct fte_version *version, int index) { struct classifier *cls = &tables->tables[version->table_id]; struct fte *old, *fte; fte = xzalloc(sizeof *fte); - cls_rule_init(&fte->rule, match, priority); + cls_rule_init_from_minimatch(&fte->rule, match, priority); fte->versions[index] = version; old = fte_from_cls_rule(classifier_replace(cls, &fte->rule, @@ -3498,40 +3499,45 @@ generate_tun_metadata(struct fte_state *state) * can just read the data from the match and rewrite it. On rewrite, it * will use the new table. */ static void -remap_match(struct fte_state *state, struct match *match) +remap_match(struct fte_state *state, struct minimatch *minimatch) { int i; - if (!match->tun_md.valid) { + if (!minimatch->tun_md || !minimatch->tun_md->valid) { return; } - struct tun_metadata flow = match->flow.tunnel.metadata; - struct tun_metadata flow_mask = match->wc.masks.tunnel.metadata; - memset(&match->flow.tunnel.metadata, 0, sizeof match->flow.tunnel.metadata); - memset(&match->wc.masks.tunnel.metadata, 0, - sizeof match->wc.masks.tunnel.metadata); - match->tun_md.valid = false; + struct match match; + minimatch_expand(minimatch, &match); + + struct tun_metadata flow = match.flow.tunnel.metadata; + struct tun_metadata flow_mask = match.wc.masks.tunnel.metadata; + memset(&match.flow.tunnel.metadata, 0, sizeof match.flow.tunnel.metadata); + memset(&match.wc.masks.tunnel.metadata, 0, + sizeof match.wc.masks.tunnel.metadata); + match.tun_md.valid = false; - match->flow.tunnel.metadata.tab = state->tun_tab; - match->wc.masks.tunnel.metadata.tab = match->flow.tunnel.metadata.tab; + match.flow.tunnel.metadata.tab = state->tun_tab; + match.wc.masks.tunnel.metadata.tab = match.flow.tunnel.metadata.tab; ULLONG_FOR_EACH_1 (i, flow_mask.present.map) { const struct mf_field *field = mf_from_id(MFF_TUN_METADATA0 + i); - int offset = match->tun_md.entry[i].loc.c.offset; - int len = match->tun_md.entry[i].loc.len; + int offset = match.tun_md.entry[i].loc.c.offset; + int len = match.tun_md.entry[i].loc.len; union mf_value value, mask; memset(&value, 0, field->n_bytes - len); - memset(&mask, match->tun_md.entry[i].masked ? 0 : 0xff, + memset(&mask, match.tun_md.entry[i].masked ? 0 : 0xff, field->n_bytes - len); memcpy(value.tun_metadata + field->n_bytes - len, flow.opts.u8 + offset, len); memcpy(mask.tun_metadata + field->n_bytes - len, flow_mask.opts.u8 + offset, len); - mf_set(field, &value, &mask, match, NULL); + mf_set(field, &value, &mask, &match, NULL); } + minimatch_destroy(minimatch); + minimatch_init(minimatch, &match); } /* In order to correctly handle tunnel metadata, we need to have @@ -3578,25 +3584,26 @@ fte_state_destroy(struct fte_state *state) * fte_state_init(). fte_queue() is the first pass to be called as each * flow is read from its source. */ static void -fte_queue(struct fte_state *state, const struct match *match, +fte_queue(struct fte_state *state, const struct minimatch *match, int priority, struct fte_version *version, int index) { struct fte_pending *pending = xmalloc(sizeof *pending); int i; - pending->match = xmemdup(match, sizeof *match); + minimatch_clone(&pending->match, match); pending->priority = priority; pending->version = version; pending->index = index; ovs_list_push_back(&state->fte_pending_list, &pending->list_node); - if (!match->tun_md.valid) { + if (!match->tun_md || !match->tun_md->valid) { return; } - ULLONG_FOR_EACH_1 (i, match->wc.masks.tunnel.metadata.present.map) { - if (match->tun_md.entry[i].loc.len > state->tun_metadata_size[i]) { - state->tun_metadata_size[i] = match->tun_md.entry[i].loc.len; + uint64_t map = miniflow_get_tun_metadata_present_map(&match->mask->masks); + ULLONG_FOR_EACH_1 (i, map) { + if (match->tun_md->entry[i].loc.len > state->tun_metadata_size[i]) { + state->tun_metadata_size[i] = match->tun_md->entry[i].loc.len; } } } @@ -3615,10 +3622,10 @@ fte_fill(struct fte_state *state, struct flow_tables *tables) flow_tables_defer(tables); LIST_FOR_EACH_POP(pending, list_node, &state->fte_pending_list) { - remap_match(state, pending->match); - fte_insert(tables, pending->match, pending->priority, pending->version, - pending->index); - free(pending->match); + remap_match(state, &pending->match); + fte_insert(tables, &pending->match, pending->priority, + pending->version, pending->index); + minimatch_destroy(&pending->match); free(pending); } @@ -3669,6 +3676,8 @@ read_flows_from_file(const char *filename, struct fte_state *state, int index) version->table_id = fm.table_id != OFPTT_ALL ? fm.table_id : 0; fte_queue(state, &fm.match, fm.priority, version, index); + + minimatch_destroy(&fm.match); } ds_destroy(&s); @@ -3714,7 +3723,10 @@ read_flows_from_switch(struct vconn *vconn, version->ofpacts = xmemdup(fs->ofpacts, fs->ofpacts_len); version->table_id = fs->table_id; - fte_queue(state, &fs->match, fs->priority, version, index); + struct minimatch match; + minimatch_init(&match, &fs->match); + fte_queue(state, &match, fs->priority, version, index); + minimatch_destroy(&match); } for (size_t i = 0; i < n_fses; i++) { @@ -3744,7 +3756,7 @@ fte_make_flow_mod(const struct fte *fte, int index, uint16_t command, .out_group = OFPG_ANY, .flags = version->flags, }; - minimatch_expand(&fte->rule.match, &fm.match); + minimatch_clone(&fm.match, &fte->rule.match); if (command == OFPFC_ADD || command == OFPFC_MODIFY || command == OFPFC_MODIFY_STRICT) { fm.ofpacts = version->ofpacts; @@ -3753,8 +3765,9 @@ fte_make_flow_mod(const struct fte *fte, int index, uint16_t command, fm.ofpacts = NULL; fm.ofpacts_len = 0; } - ofm = ofputil_encode_flow_mod(&fm, protocol); + minimatch_destroy(&fm.match); + ovs_list_push_back(packets, &ofm->list_node); } @@ -4028,6 +4041,7 @@ ofctl_parse_flows__(struct ofputil_flow_mod *fms, size_t n_fms, ofpbuf_delete(msg); free(CONST_CAST(struct ofpact *, fm->ofpacts)); + minimatch_destroy(&fm->match); } } @@ -4504,10 +4518,13 @@ ofctl_check_vlan(struct ovs_cmdl_context *ctx) if (error_s) { ovs_fatal(0, "%s", error_s); } + struct match fm_match; + minimatch_expand(&fm.match, &fm_match); printf("%04"PRIx16"/%04"PRIx16"\n", - ntohs(fm.match.flow.vlans[0].tci), - ntohs(fm.match.wc.masks.vlans[0].tci)); + ntohs(fm_match.flow.vlans[0].tci), + ntohs(fm_match.wc.masks.vlans[0].tci)); free(string_s); + minimatch_destroy(&fm.match); /* Convert to and from NXM. */ ofpbuf_init(&nxm, 0); -- 2.16.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
