diff mbox

[ovs-dev,action,upcall,meters,1/5] ofproto: Store meters using hmap

Message ID 1310739B-D430-4746-856F-3D5F92F2E031@ovn.org
State Not Applicable
Headers show

Commit Message

Jarno Rajahalme April 27, 2017, 10:14 p.m. UTC 
This incremental needed to satisfy GCC 4.9.2, due to ‘meter’ potentially being used uninitialized:


Otherwise:

Acked-by: Jarno Rajahalme <jarno@ovn.org>

> On Apr 14, 2017, at 12:46 PM, Andy Zhou <azhou@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 <azhou@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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox

Patch

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);