From patchwork Fri Apr 28 06:28:37 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Andy Zhou X-Patchwork-Id: 756236 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wDkSC3Nqdz9s84 for ; Fri, 28 Apr 2017 16:29:31 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 4A00F9FA; Fri, 28 Apr 2017 06:29:27 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 2E2BC6C for ; Fri, 28 Apr 2017 06:29:25 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 9F7AFA4 for ; Fri, 28 Apr 2017 06:29:23 +0000 (UTC) Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) by relay8-d.mail.gandi.net (Postfix) with ESMTPS id 798F2406CF for ; Fri, 28 Apr 2017 08:29:22 +0200 (CEST) Received: from mfilter33-d.gandi.net (mfilter33-d.gandi.net [217.70.178.164]) by relay5-d.mail.gandi.net (Postfix) with ESMTP id 3C39241C093 for ; Fri, 28 Apr 2017 08:29:22 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mfilter33-d.gandi.net Received: from relay5-d.mail.gandi.net ([IPv6:::ffff:217.70.183.197]) by mfilter33-d.gandi.net (mfilter33-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id EvI4vb7v9ZRp for ; Fri, 28 Apr 2017 08:29:20 +0200 (CEST) X-Originating-IP: 74.125.83.41 Received: from mail-pg0-f41.google.com (mail-pg0-f41.google.com [74.125.83.41]) (Authenticated sender: azhou@ovn.org) by relay5-d.mail.gandi.net (Postfix) with ESMTPSA id CC6F341C091 for ; Fri, 28 Apr 2017 08:29:19 +0200 (CEST) Received: by mail-pg0-f41.google.com with SMTP id t7so9930527pgt.3 for ; Thu, 27 Apr 2017 23:29:19 -0700 (PDT) X-Gm-Message-State: AN3rC/5+RAXCq9SYmYbSEQkkxJ5r49uo/l+uQ+MbEcYDws8q9c+VELnU XxITRZMSNSAPvuqkItZ8Q+0kuWOs6w== X-Received: by 10.84.175.67 with SMTP id s61mr12677119plb.43.1493360957997; Thu, 27 Apr 2017 23:29:17 -0700 (PDT) MIME-Version: 1.0 Received: by 10.100.180.134 with HTTP; Thu, 27 Apr 2017 23:28:37 -0700 (PDT) In-Reply-To: <1310739B-D430-4746-856F-3D5F92F2E031@ovn.org> References: <1492199182-4479-1-git-send-email-azhou@ovn.org> <1310739B-D430-4746-856F-3D5F92F2E031@ovn.org> From: Andy Zhou Date: Thu, 27 Apr 2017 23:28:37 -0700 X-Gmail-Original-Message-ID: Message-ID: To: Jarno Rajahalme X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: "" Subject: Re: [ovs-dev] [action upcall meters 1/5] ofproto: Store meters using hmap X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org On Thu, Apr 27, 2017 at 3:14 PM, Jarno Rajahalme 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. How about I fold the following to document the reason why meter is set to NULL: > > Otherwise: > > Acked-by: Jarno Rajahalme Thanks for the review. > >> On Apr 14, 2017, at 12:46 PM, Andy Zhou 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 >> --- >> 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 --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, + this is not possible, since meter is only used + when meter_id != OFPM13_ALL. */ } ofpmp_init(&replies, request);