On 20 March 2018 at 13:46, Ben Pfaff <[email protected]> wrote: > 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.metadat > a.present.map); > + spec->dst.field, &match.flow.tunnel.metadata.pr > esent.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.p > acket_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_versio > n(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_pres > ent_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.ta > b; > + 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_pres > ent_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.le > n; > } > } > } > @@ -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 >
I can't claim that my review would be enough to spot any potential issue, but for what it's worth I did test the patches: all 150K+ flows were processed correctly, and the memory savings were substantial. I did test against master and 2.7.3. Thanks, Armando Reviewed-by: Armando Migliaccio <[email protected]> Tested-by: Armando Migliaccio <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
