From patchwork Fri Apr 28 09:01:19 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andy Zhou X-Patchwork-Id: 756271 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 3wDnqq34dZz9s89 for ; Fri, 28 Apr 2017 19:01:43 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id E57F6A5E; Fri, 28 Apr 2017 09:01:40 +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 039AC5AC for ; Fri, 28 Apr 2017 09:01:40 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.7.6 Received: from mail-pg0-f65.google.com (mail-pg0-f65.google.com [74.125.83.65]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 0379F176 for ; Fri, 28 Apr 2017 09:01:38 +0000 (UTC) Received: by mail-pg0-f65.google.com with SMTP id s1so1943672pgc.2 for ; Fri, 28 Apr 2017 02:01:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id :content-transfer-encoding; bh=C5MCiqRNOCr7pVnsfhmvsHnQT43eWYZumi/jhafzsTQ=; b=qxr9x1DXyBDD1CyrWUbMz2oL53/A+KGAam0dv3X0+r2AxXCIjrHp3lujnQnUn91Oco nK33jUaVFAPH7KLD18GdQ7DyWGS6aEfyCb6kvqt33decGHTbQFZT+DuEFOfmW14Pjlgh ILTg6W8iHMiskaQw2peOHOebByhvSQesHfW++j8h4Hizckm26NvRFDSW33H6cWvpOK6d sNXmuOXkAgESKLTTIaYhMKSYCA25Of/ZQKeu7Sjkhxrbicbr/jtogpnVS726eTDHONwR Fu9yAr73JBlfVuAM3wxFjHWye3LKlW/0lPJKNbIrVGOHe/HNgOHecAo/iHkAtx3PsKtB XrKQ== X-Gm-Message-State: AN3rC/5nFZQ5Qu3SirPLpGemDie/eIyAduXZCSHbM2pQiDHBbRID6LZI WN3AlN/CjTJt4A== X-Received: by 10.84.133.132 with SMTP id f4mr13747909plf.94.1493370098484; Fri, 28 Apr 2017 02:01:38 -0700 (PDT) Received: from centos.hsd1.ca.comcast.net. ([2601:647:4280:45d0:d6be:d9ff:fe69:1f8d]) by smtp.gmail.com with ESMTPSA id s68sm9307944pfj.77.2017.04.28.02.01.37 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 28 Apr 2017 02:01:37 -0700 (PDT) From: Andy Zhou To: dev@openvswitch.org Date: Fri, 28 Apr 2017 02:01:19 -0700 Message-Id: <1493370083-7385-1-git-send-email-azhou@ovn.org> X-Mailer: git-send-email 1.8.3.1 X-Spam-Status: No, score=-1.4 required=5.0 tests=BAYES_00,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM autolearn=no version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [action upcall meter v2 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: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org 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 Acked-by: Jarno Rajahalme --- v1->v2: Remove GCC 4.9.2. warning by always initialize meter --- ofproto/ofproto-provider.h | 7 +- ofproto/ofproto.c | 249 +++++++++++++++++++++++++++------------------ 2 files changed, 152 insertions(+), 104 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 ca0f3e49bd67..5397afa3163d 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) { @@ -6215,14 +6211,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 @@ -6233,8 +6252,8 @@ static bool 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]; + if (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 @@ -6254,7 +6273,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); } @@ -6279,6 +6298,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); @@ -6287,31 +6307,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); } } @@ -6319,10 +6354,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; } @@ -6330,7 +6365,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; } @@ -6338,7 +6374,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; @@ -6363,25 +6399,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 @@ -6473,70 +6505,89 @@ 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; + } 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. */ } - 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; }