This incremental needed to satisfy GCC 4.9.2, due to ‘meter’ potentially being used uninitialized:
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 2e80db8..eb060d0 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -6564,7 +6564,7 @@ handle_meter_request(struct ofconn *ofconn, const struct ofp_header *request, struct ofproto *ofproto = ofconn_get_ofproto(ofconn); struct ovs_list replies; uint32_t meter_id; - struct meter *meter; + struct meter *meter = NULL; ofputil_decode_meter_request(request, &meter_id); Otherwise: Acked-by: Jarno Rajahalme <ja...@ovn.org> > On Apr 14, 2017, at 12:46 PM, Andy Zhou <az...@ovn.org> wrote: > > Currently, meters are stored in a fixed pointer array. It is not > very efficient since the controller, at least in theory, can > pick any meter id (up to the limits to uint32_t), not necessarily > within the lower end of a region, or in close range to each other. > In particular, OFPM_SLOWPATH and OFPM_CONTROLLER meters are specified > at the high region. > > Switching to using hmap. Ofproto layer does not restrict > the number of meters that controller can add, nor does it care > about the value of meter_id. Datapth limits the number of meters > ofproto layer can support at run time. > > Signed-off-by: Andy Zhou <az...@ovn.org> > --- > ofproto/ofproto-provider.h | 7 +- > ofproto/ofproto.c | 242 +++++++++++++++++++++++++++------------------ > 2 files changed, 146 insertions(+), 103 deletions(-) > > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index b7b12cdfd5f4..000326d7f79d 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -109,12 +109,9 @@ struct ofproto { > /* List of expirable flows, in all flow tables. */ > struct ovs_list expirable OVS_GUARDED_BY(ofproto_mutex); > > - /* Meter table. > - * OpenFlow meters start at 1. To avoid confusion we leave the first > - * pointer in the array un-used, and index directly with the OpenFlow > - * meter_id. */ > + /* Meter table. */ > struct ofputil_meter_features meter_features; > - struct meter **meters; /* 'meter_features.max_meter' + 1 pointers. */ > + struct hmap meters; /* uint32_t indexed 'struct meter *'. */ > > /* OpenFlow connections. */ > struct connmgr *connmgr; > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 7440d5b52092..8c4c7e67f213 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -281,7 +281,8 @@ static uint64_t pick_fallback_dpid(void); > static void ofproto_destroy__(struct ofproto *); > static void update_mtu(struct ofproto *, struct ofport *); > static void update_mtu_ofproto(struct ofproto *); > -static void meter_delete(struct ofproto *, uint32_t first, uint32_t last); > +static void meter_delete(struct ofproto *, uint32_t); > +static void meter_delete_all(struct ofproto *); > static void meter_insert_rule(struct rule *); > > /* unixctl. */ > @@ -566,8 +567,7 @@ ofproto_create(const char *datapath_name, const char > *datapath_type, > } else { > memset(&ofproto->meter_features, 0, sizeof ofproto->meter_features); > } > - ofproto->meters = xzalloc((ofproto->meter_features.max_meters + 1) > - * sizeof(struct meter *)); > + hmap_init(&ofproto->meters); > > /* Set the initial tables version. */ > ofproto_bump_tables_version(ofproto); > @@ -1635,12 +1635,8 @@ ofproto_destroy(struct ofproto *p, bool del) > return; > } > > - if (p->meters) { > - meter_delete(p, 1, p->meter_features.max_meters); > - p->meter_features.max_meters = 0; > - free(p->meters); > - p->meters = NULL; > - } > + meter_delete_all(p); > + hmap_destroy(&p->meters); > > ofproto_flush__(p); > HMAP_FOR_EACH_SAFE (ofport, next_ofport, hmap_node, &p->ports) { > @@ -6211,14 +6207,37 @@ handle_flow_monitor_cancel(struct ofconn *ofconn, > const struct ofp_header *oh) > * 'provider_meter_id' is for the provider's private use. > */ > struct meter { > + struct hmap_node node; /* In ofproto->meters. */ > long long int created; /* Time created. */ > struct ovs_list rules; /* List of "struct rule_dpif"s. */ > + uint32_t id; /* OpenFlow meter_id. */ > ofproto_meter_id provider_meter_id; > uint16_t flags; /* Meter flags. */ > uint16_t n_bands; /* Number of meter bands. */ > struct ofputil_meter_band *bands; > }; > > +static struct meter * > +ofproto_get_meter(const struct ofproto *ofproto, uint32_t meter_id) > +{ > + struct meter *meter; > + uint32_t hash = hash_int(meter_id, 0); > + > + HMAP_FOR_EACH_WITH_HASH (meter, node, hash, &ofproto->meters) { > + if (meter->id == meter_id) { > + return meter; > + } > + } > + > + return NULL; > +} > + > +static void > +ofproto_add_meter(struct ofproto *ofproto, struct meter *meter) > +{ > + hmap_insert(&ofproto->meters, &meter->node, hash_int(meter->id, 0)); > +} > + > /* > * This is used in instruction validation at flow set-up time, to map > * the OpenFlow meter ID to the corresponding datapath provider meter > @@ -6230,7 +6249,7 @@ ofproto_fix_meter_action(const struct ofproto *ofproto, > struct ofpact_meter *ma) > { > if (ma->meter_id && ma->meter_id <= ofproto->meter_features.max_meters) { > - const struct meter *meter = ofproto->meters[ma->meter_id]; > + const struct meter *meter = ofproto_get_meter(ofproto, ma->meter_id); > > if (meter && meter->provider_meter_id.uint32 != UINT32_MAX) { > /* Update the action with the provider's meter ID, so that we > @@ -6250,7 +6269,7 @@ meter_insert_rule(struct rule *rule) > { > const struct rule_actions *a = rule_get_actions(rule); > uint32_t meter_id = ofpacts_get_meter(a->ofpacts, a->ofpacts_len); > - struct meter *meter = rule->ofproto->meters[meter_id]; > + struct meter *meter = ofproto_get_meter(rule->ofproto, meter_id); > > ovs_list_insert(&meter->rules, &rule->meter_list_node); > } > @@ -6275,6 +6294,7 @@ meter_create(const struct ofputil_meter_config *config, > meter = xzalloc(sizeof *meter); > meter->provider_meter_id = provider_meter_id; > meter->created = time_msec(); > + meter->id = config->meter_id; > ovs_list_init(&meter->rules); > > meter_update(meter, config); > @@ -6283,31 +6303,46 @@ meter_create(const struct ofputil_meter_config > *config, > } > > static void > -meter_delete(struct ofproto *ofproto, uint32_t first, uint32_t last) > +meter_destroy(struct ofproto *ofproto, struct meter *meter) > OVS_REQUIRES(ofproto_mutex) > { > - for (uint32_t mid = first; mid <= last; ++mid) { > - struct meter *meter = ofproto->meters[mid]; > - if (meter) { > - /* First delete the rules that use this meter. */ > - if (!ovs_list_is_empty(&meter->rules)) { > - struct rule_collection rules; > - struct rule *rule; > + if (!ovs_list_is_empty(&meter->rules)) { > + struct rule_collection rules; > + struct rule *rule; > > - rule_collection_init(&rules); > + rule_collection_init(&rules); > + LIST_FOR_EACH (rule, meter_list_node, &meter->rules) { > + rule_collection_add(&rules, rule); > + } > + delete_flows__(&rules, OFPRR_METER_DELETE, NULL); > + } > > - LIST_FOR_EACH (rule, meter_list_node, &meter->rules) { > - rule_collection_add(&rules, rule); > - } > - delete_flows__(&rules, OFPRR_METER_DELETE, NULL); > - } > + ofproto->ofproto_class->meter_del(ofproto, meter->provider_meter_id); > + free(meter->bands); > + free(meter); > +} > > - ofproto->meters[mid] = NULL; > - ofproto->ofproto_class->meter_del(ofproto, > - meter->provider_meter_id); > - free(meter->bands); > - free(meter); > - } > +static void > +meter_delete(struct ofproto *ofproto, uint32_t meter_id) > + OVS_REQUIRES(ofproto_mutex) > +{ > + struct meter *meter = ofproto_get_meter(ofproto, meter_id); > + > + if (meter) { > + hmap_remove(&ofproto->meters, &meter->node); > + meter_destroy(ofproto, meter); > + } > +} > + > +static void > +meter_delete_all(struct ofproto *ofproto) > + OVS_REQUIRES(ofproto_mutex) > +{ > + struct meter *meter, *next; > + > + HMAP_FOR_EACH_SAFE (meter, next, node, &ofproto->meters) { > + hmap_remove(&ofproto->meters, &meter->node); > + meter_destroy(ofproto, meter); > } > } > > @@ -6315,10 +6350,10 @@ static enum ofperr > handle_add_meter(struct ofproto *ofproto, struct ofputil_meter_mod *mm) > { > ofproto_meter_id provider_meter_id = { UINT32_MAX }; > - struct meter **meterp = &ofproto->meters[mm->meter.meter_id]; > + struct meter *meter = ofproto_get_meter(ofproto, mm->meter.meter_id); > enum ofperr error; > > - if (*meterp) { > + if (meter) { > return OFPERR_OFPMMFC_METER_EXISTS; > } > > @@ -6326,7 +6361,8 @@ handle_add_meter(struct ofproto *ofproto, struct > ofputil_meter_mod *mm) > &mm->meter); > if (!error) { > ovs_assert(provider_meter_id.uint32 != UINT32_MAX); > - *meterp = meter_create(&mm->meter, provider_meter_id); > + meter = meter_create(&mm->meter, provider_meter_id); > + ofproto_add_meter(ofproto, meter); > } > return error; > } > @@ -6334,7 +6370,7 @@ handle_add_meter(struct ofproto *ofproto, struct > ofputil_meter_mod *mm) > static enum ofperr > handle_modify_meter(struct ofproto *ofproto, struct ofputil_meter_mod *mm) > { > - struct meter *meter = ofproto->meters[mm->meter.meter_id]; > + struct meter *meter = ofproto_get_meter(ofproto, mm->meter.meter_id); > enum ofperr error; > uint32_t provider_meter_id; > > @@ -6359,25 +6395,21 @@ handle_delete_meter(struct ofconn *ofconn, struct > ofputil_meter_mod *mm) > { > struct ofproto *ofproto = ofconn_get_ofproto(ofconn); > uint32_t meter_id = mm->meter.meter_id; > - enum ofperr error = 0; > - uint32_t first, last; > > - if (meter_id == OFPM13_ALL) { > - first = 1; > - last = ofproto->meter_features.max_meters; > - } else { > - if (!meter_id || meter_id > ofproto->meter_features.max_meters) { > - return 0; > + /* OpenFlow does not support Meter ID 0. */ > + if (meter_id) { > + ovs_mutex_lock(&ofproto_mutex); > + > + if (meter_id == OFPM13_ALL) { > + meter_delete_all(ofproto); > + } else { > + meter_delete(ofproto, meter_id); > } > - first = last = meter_id; > - } > > - /* Delete the meters. */ > - ovs_mutex_lock(&ofproto_mutex); > - meter_delete(ofproto, first, last); > - ovs_mutex_unlock(&ofproto_mutex); > + ovs_mutex_unlock(&ofproto_mutex); > + } > > - return error; > + return 0; > } > > static enum ofperr > @@ -6469,70 +6501,84 @@ handle_meter_features_request(struct ofconn *ofconn, > return 0; > } > > +static void > +meter_request_reply(struct ofproto *ofproto, struct meter *meter, > + enum ofptype type, struct ovs_list *replies) > +{ > + uint64_t bands_stub[256 / 8]; > + struct ofpbuf bands; > + > + ofpbuf_use_stub(&bands, bands_stub, sizeof bands_stub); > + > + if (type == OFPTYPE_METER_STATS_REQUEST) { > + struct ofputil_meter_stats stats; > + > + stats.meter_id = meter->id; > + > + /* Provider sets the packet and byte counts, we do the rest. */ > + stats.flow_count = ovs_list_size(&meter->rules); > + calc_duration(meter->created, time_msec(), > + &stats.duration_sec, &stats.duration_nsec); > + stats.n_bands = meter->n_bands; > + ofpbuf_clear(&bands); > + stats.bands = ofpbuf_put_uninit(&bands, meter->n_bands > + * sizeof *stats.bands); > + > + if (!ofproto->ofproto_class->meter_get(ofproto, > + meter->provider_meter_id, > + &stats, meter->n_bands)) { > + ofputil_append_meter_stats(replies, &stats); > + } > + } else { /* type == OFPTYPE_METER_CONFIG_REQUEST */ > + struct ofputil_meter_config config; > + > + config.meter_id = meter->id; > + config.flags = meter->flags; > + config.n_bands = meter->n_bands; > + config.bands = meter->bands; > + ofputil_append_meter_config(replies, &config); > + } > + > + ofpbuf_uninit(&bands); > +} > + > +static void > +meter_request_reply_all(struct ofproto *ofproto, enum ofptype type, > + struct ovs_list *replies) > +{ > + struct meter *meter; > + > + HMAP_FOR_EACH (meter, node, &ofproto->meters) { > + meter_request_reply(ofproto, meter, type, replies); > + } > +} > + > static enum ofperr > handle_meter_request(struct ofconn *ofconn, const struct ofp_header *request, > enum ofptype type) > { > struct ofproto *ofproto = ofconn_get_ofproto(ofconn); > struct ovs_list replies; > - uint64_t bands_stub[256 / 8]; > - struct ofpbuf bands; > - uint32_t meter_id, first, last; > + uint32_t meter_id; > + struct meter *meter; > > ofputil_decode_meter_request(request, &meter_id); > > - if (meter_id == OFPM13_ALL) { > - first = 1; > - last = ofproto->meter_features.max_meters; > - } else { > - if (!meter_id || meter_id > ofproto->meter_features.max_meters || > - !ofproto->meters[meter_id]) { > + if (meter_id != OFPM13_ALL) { > + meter = ofproto_get_meter(ofproto, meter_id); > + if (!meter) { > + /* Meter does not exist. */ > return OFPERR_OFPMMFC_UNKNOWN_METER; > } > - first = last = meter_id; > } > > - ofpbuf_use_stub(&bands, bands_stub, sizeof bands_stub); > ofpmp_init(&replies, request); > - > - for (meter_id = first; meter_id <= last; ++meter_id) { > - struct meter *meter = ofproto->meters[meter_id]; > - if (!meter) { > - continue; /* Skip non-existing meters. */ > - } > - if (type == OFPTYPE_METER_STATS_REQUEST) { > - struct ofputil_meter_stats stats; > - > - stats.meter_id = meter_id; > - > - /* Provider sets the packet and byte counts, we do the rest. */ > - stats.flow_count = ovs_list_size(&meter->rules); > - calc_duration(meter->created, time_msec(), > - &stats.duration_sec, &stats.duration_nsec); > - stats.n_bands = meter->n_bands; > - ofpbuf_clear(&bands); > - stats.bands > - = ofpbuf_put_uninit(&bands, > - meter->n_bands * sizeof *stats.bands); > - > - if (!ofproto->ofproto_class->meter_get(ofproto, > - meter->provider_meter_id, > - &stats, meter->n_bands)) { > - ofputil_append_meter_stats(&replies, &stats); > - } > - } else { /* type == OFPTYPE_METER_CONFIG_REQUEST */ > - struct ofputil_meter_config config; > - > - config.meter_id = meter_id; > - config.flags = meter->flags; > - config.n_bands = meter->n_bands; > - config.bands = meter->bands; > - ofputil_append_meter_config(&replies, &config); > - } > + if (meter_id == OFPM13_ALL) { > + meter_request_reply_all(ofproto, type, &replies); > + } else { > + meter_request_reply(ofproto, meter, type, &replies); > } > - > ofconn_send_replies(ofconn, &replies); > - ofpbuf_uninit(&bands); > return 0; > } > > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev