> On Apr 27, 2017, at 11:28 PM, Andy Zhou <az...@ovn.org> wrote:
>
> On Thu, Apr 27, 2017 at 3:14 PM, Jarno Rajahalme <ja...@ovn.org
> <mailto:ja...@ovn.org>> wrote:
>> 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);
>>
>
> Thanks for letting me know. I don't have gcc-4.9.2 installed on my system.
>
> I am not sure GCC is right here. meter can only be used when meter_id
> != OFPM13_ALL,
> but meter is always set in this case.
>
You are right, but GCC 4.9.2 is not clever enough to figure that out.
> How about I fold the following to document the reason why meter is set to
> NULL:
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index c5c841a11ae5..41f1a74b194e 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -6618,6 +6618,11 @@ handle_meter_request(struct ofconn *ofconn, const
> struct
> /* Meter does not exist. */
> return OFPERR_OFPMMFC_UNKNOWN_METER;
> }
> + } else {
> + meter = NULL; /* GCC 4.9.2 complains about 'meter' can
> + otentially used uninitialized. Logically,
Add the leading ‘p’ :-)
> + this is not possible, since meter is only used
> + when meter_id != OFPM13_ALL. */
> }
>
> ofpmp_init(&replies, request);
>
>>
>> Otherwise:
>>
>> Acked-by: Jarno Rajahalme <ja...@ovn.org <mailto:ja...@ovn.org>>
>
> Thanks for the review.
>>
>>> 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
