Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/756271/?format=api
{ "id": 756271, "url": "http://patchwork.ozlabs.org/api/patches/756271/?format=api", "web_url": "http://patchwork.ozlabs.org/project/openvswitch/patch/1493370083-7385-1-git-send-email-azhou@ovn.org/", "project": { "id": 47, "url": "http://patchwork.ozlabs.org/api/projects/47/?format=api", "name": "Open vSwitch", "link_name": "openvswitch", "list_id": "ovs-dev.openvswitch.org", "list_email": "ovs-dev@openvswitch.org", "web_url": "http://openvswitch.org/", "scm_url": "git@github.com:openvswitch/ovs.git", "webscm_url": "https://github.com/openvswitch/ovs", "list_archive_url": "", "list_archive_url_format": "", "commit_url_format": "" }, "msgid": "<1493370083-7385-1-git-send-email-azhou@ovn.org>", "list_archive_url": null, "date": "2017-04-28T09:01:19", "name": "[ovs-dev,action,upcall,meter,v2,1/5] ofproto: Store meters using hmap", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "66cc261064568302d954ba624437a4df45fc28fd", "submitter": { "id": 67699, "url": "http://patchwork.ozlabs.org/api/people/67699/?format=api", "name": "Andy Zhou", "email": "azhou@ovn.org" }, "delegate": null, "mbox": "http://patchwork.ozlabs.org/project/openvswitch/patch/1493370083-7385-1-git-send-email-azhou@ovn.org/mbox/", "series": [], "comments": "http://patchwork.ozlabs.org/api/patches/756271/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/756271/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<ovs-dev-bounces@openvswitch.org>", "X-Original-To": [ "incoming@patchwork.ozlabs.org", "dev@openvswitch.org" ], "Delivered-To": [ "patchwork-incoming@bilbo.ozlabs.org", "ovs-dev@mail.linuxfoundation.org" ], "Received": [ "from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3wDnqq34dZz9s89\n\tfor <incoming@patchwork.ozlabs.org>;\n\tFri, 28 Apr 2017 19:01:43 +1000 (AEST)", "from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id E57F6A5E;\n\tFri, 28 Apr 2017 09:01:40 +0000 (UTC)", "from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 039AC5AC\n\tfor <dev@openvswitch.org>; Fri, 28 Apr 2017 09:01:40 +0000 (UTC)", "from mail-pg0-f65.google.com (mail-pg0-f65.google.com\n\t[74.125.83.65])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 0379F176\n\tfor <dev@openvswitch.org>; Fri, 28 Apr 2017 09:01:38 +0000 (UTC)", "by mail-pg0-f65.google.com with SMTP id s1so1943672pgc.2\n\tfor <dev@openvswitch.org>; Fri, 28 Apr 2017 02:01:38 -0700 (PDT)", "from centos.hsd1.ca.comcast.net.\n\t([2601:647:4280:45d0:d6be:d9ff:fe69:1f8d])\n\tby smtp.gmail.com with ESMTPSA id\n\ts68sm9307944pfj.77.2017.04.28.02.01.37\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 28 Apr 2017 02:01:37 -0700 (PDT)" ], "X-Greylist": "whitelisted by SQLgrey-1.7.6", "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:from:to:cc:subject:date:message-id\n\t:content-transfer-encoding;\n\tbh=C5MCiqRNOCr7pVnsfhmvsHnQT43eWYZumi/jhafzsTQ=;\n\tb=qxr9x1DXyBDD1CyrWUbMz2oL53/A+KGAam0dv3X0+r2AxXCIjrHp3lujnQnUn91Oco\n\tnK33jUaVFAPH7KLD18GdQ7DyWGS6aEfyCb6kvqt33decGHTbQFZT+DuEFOfmW14Pjlgh\n\tILTg6W8iHMiskaQw2peOHOebByhvSQesHfW++j8h4Hizckm26NvRFDSW33H6cWvpOK6d\n\tsNXmuOXkAgESKLTTIaYhMKSYCA25Of/ZQKeu7Sjkhxrbicbr/jtogpnVS726eTDHONwR\n\tFu9yAr73JBlfVuAM3wxFjHWye3LKlW/0lPJKNbIrVGOHe/HNgOHecAo/iHkAtx3PsKtB\n\tXrKQ==", "X-Gm-Message-State": "AN3rC/5nFZQ5Qu3SirPLpGemDie/eIyAduXZCSHbM2pQiDHBbRID6LZI\n\tWN3AlN/CjTJt4A==", "X-Received": "by 10.84.133.132 with SMTP id f4mr13747909plf.94.1493370098484; \n\tFri, 28 Apr 2017 02:01:38 -0700 (PDT)", "From": "Andy Zhou <azhou@ovn.org>", "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,\n\tRCVD_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\n\tsmtp1.linux-foundation.org", "Subject": "[ovs-dev] [action upcall meter v2 1/5] ofproto: Store meters using\n\thmap", "X-BeenThere": "ovs-dev@openvswitch.org", "X-Mailman-Version": "2.1.12", "Precedence": "list", "List-Id": "<ovs-dev.openvswitch.org>", "List-Unsubscribe": "<https://mail.openvswitch.org/mailman/options/ovs-dev>,\n\t<mailto:ovs-dev-request@openvswitch.org?subject=unsubscribe>", "List-Archive": "<http://mail.openvswitch.org/pipermail/ovs-dev/>", "List-Post": "<mailto:ovs-dev@openvswitch.org>", "List-Help": "<mailto:ovs-dev-request@openvswitch.org?subject=help>", "List-Subscribe": "<https://mail.openvswitch.org/mailman/listinfo/ovs-dev>,\n\t<mailto:ovs-dev-request@openvswitch.org?subject=subscribe>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "7bit", "Sender": "ovs-dev-bounces@openvswitch.org", "Errors-To": "ovs-dev-bounces@openvswitch.org" }, "content": "Currently, meters are stored in a fixed pointer array. It is not\nvery efficient since the controller, at least in theory, can\npick any meter id (up to the limits to uint32_t), not necessarily\nwithin the lower end of a region, or in close range to each other.\nIn particular, OFPM_SLOWPATH and OFPM_CONTROLLER meters are specified\nat the high region.\n\nSwitching to using hmap. Ofproto layer does not restrict\nthe number of meters that controller can add, nor does it care\nabout the value of meter_id. Datapth limits the number of meters\nofproto layer can support at run time.\n\nSigned-off-by: Andy Zhou <azhou@ovn.org>\nAcked-by: Jarno Rajahalme <jarno@ovn.org>\n\n---\nv1->v2: Remove GCC 4.9.2. warning by always initialize meter\n---\n ofproto/ofproto-provider.h | 7 +-\n ofproto/ofproto.c | 249 +++++++++++++++++++++++++++------------------\n 2 files changed, 152 insertions(+), 104 deletions(-)", "diff": "diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h\nindex b7b12cdfd5f4..000326d7f79d 100644\n--- a/ofproto/ofproto-provider.h\n+++ b/ofproto/ofproto-provider.h\n@@ -109,12 +109,9 @@ struct ofproto {\n /* List of expirable flows, in all flow tables. */\n struct ovs_list expirable OVS_GUARDED_BY(ofproto_mutex);\n \n- /* Meter table.\n- * OpenFlow meters start at 1. To avoid confusion we leave the first\n- * pointer in the array un-used, and index directly with the OpenFlow\n- * meter_id. */\n+ /* Meter table. */\n struct ofputil_meter_features meter_features;\n- struct meter **meters; /* 'meter_features.max_meter' + 1 pointers. */\n+ struct hmap meters; /* uint32_t indexed 'struct meter *'. */\n \n /* OpenFlow connections. */\n struct connmgr *connmgr;\ndiff --git a/ofproto/ofproto.c b/ofproto/ofproto.c\nindex ca0f3e49bd67..5397afa3163d 100644\n--- a/ofproto/ofproto.c\n+++ b/ofproto/ofproto.c\n@@ -281,7 +281,8 @@ static uint64_t pick_fallback_dpid(void);\n static void ofproto_destroy__(struct ofproto *);\n static void update_mtu(struct ofproto *, struct ofport *);\n static void update_mtu_ofproto(struct ofproto *);\n-static void meter_delete(struct ofproto *, uint32_t first, uint32_t last);\n+static void meter_delete(struct ofproto *, uint32_t);\n+static void meter_delete_all(struct ofproto *);\n static void meter_insert_rule(struct rule *);\n \n /* unixctl. */\n@@ -566,8 +567,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type,\n } else {\n memset(&ofproto->meter_features, 0, sizeof ofproto->meter_features);\n }\n- ofproto->meters = xzalloc((ofproto->meter_features.max_meters + 1)\n- * sizeof(struct meter *));\n+ hmap_init(&ofproto->meters);\n \n /* Set the initial tables version. */\n ofproto_bump_tables_version(ofproto);\n@@ -1635,12 +1635,8 @@ ofproto_destroy(struct ofproto *p, bool del)\n return;\n }\n \n- if (p->meters) {\n- meter_delete(p, 1, p->meter_features.max_meters);\n- p->meter_features.max_meters = 0;\n- free(p->meters);\n- p->meters = NULL;\n- }\n+ meter_delete_all(p);\n+ hmap_destroy(&p->meters);\n \n ofproto_flush__(p);\n HMAP_FOR_EACH_SAFE (ofport, next_ofport, hmap_node, &p->ports) {\n@@ -6215,14 +6211,37 @@ handle_flow_monitor_cancel(struct ofconn *ofconn, const struct ofp_header *oh)\n * 'provider_meter_id' is for the provider's private use.\n */\n struct meter {\n+ struct hmap_node node; /* In ofproto->meters. */\n long long int created; /* Time created. */\n struct ovs_list rules; /* List of \"struct rule_dpif\"s. */\n+ uint32_t id; /* OpenFlow meter_id. */\n ofproto_meter_id provider_meter_id;\n uint16_t flags; /* Meter flags. */\n uint16_t n_bands; /* Number of meter bands. */\n struct ofputil_meter_band *bands;\n };\n \n+static struct meter *\n+ofproto_get_meter(const struct ofproto *ofproto, uint32_t meter_id)\n+{\n+ struct meter *meter;\n+ uint32_t hash = hash_int(meter_id, 0);\n+\n+ HMAP_FOR_EACH_WITH_HASH (meter, node, hash, &ofproto->meters) {\n+ if (meter->id == meter_id) {\n+ return meter;\n+ }\n+ }\n+\n+ return NULL;\n+}\n+\n+static void\n+ofproto_add_meter(struct ofproto *ofproto, struct meter *meter)\n+{\n+ hmap_insert(&ofproto->meters, &meter->node, hash_int(meter->id, 0));\n+}\n+\n /*\n * This is used in instruction validation at flow set-up time, to map\n * the OpenFlow meter ID to the corresponding datapath provider meter\n@@ -6233,8 +6252,8 @@ static bool\n ofproto_fix_meter_action(const struct ofproto *ofproto,\n struct ofpact_meter *ma)\n {\n- if (ma->meter_id && ma->meter_id <= ofproto->meter_features.max_meters) {\n- const struct meter *meter = ofproto->meters[ma->meter_id];\n+ if (ma->meter_id) {\n+ const struct meter *meter = ofproto_get_meter(ofproto, ma->meter_id);\n \n if (meter && meter->provider_meter_id.uint32 != UINT32_MAX) {\n /* Update the action with the provider's meter ID, so that we\n@@ -6254,7 +6273,7 @@ meter_insert_rule(struct rule *rule)\n {\n const struct rule_actions *a = rule_get_actions(rule);\n uint32_t meter_id = ofpacts_get_meter(a->ofpacts, a->ofpacts_len);\n- struct meter *meter = rule->ofproto->meters[meter_id];\n+ struct meter *meter = ofproto_get_meter(rule->ofproto, meter_id);\n \n ovs_list_insert(&meter->rules, &rule->meter_list_node);\n }\n@@ -6279,6 +6298,7 @@ meter_create(const struct ofputil_meter_config *config,\n meter = xzalloc(sizeof *meter);\n meter->provider_meter_id = provider_meter_id;\n meter->created = time_msec();\n+ meter->id = config->meter_id;\n ovs_list_init(&meter->rules);\n \n meter_update(meter, config);\n@@ -6287,31 +6307,46 @@ meter_create(const struct ofputil_meter_config *config,\n }\n \n static void\n-meter_delete(struct ofproto *ofproto, uint32_t first, uint32_t last)\n+meter_destroy(struct ofproto *ofproto, struct meter *meter)\n OVS_REQUIRES(ofproto_mutex)\n {\n- for (uint32_t mid = first; mid <= last; ++mid) {\n- struct meter *meter = ofproto->meters[mid];\n- if (meter) {\n- /* First delete the rules that use this meter. */\n- if (!ovs_list_is_empty(&meter->rules)) {\n- struct rule_collection rules;\n- struct rule *rule;\n+ if (!ovs_list_is_empty(&meter->rules)) {\n+ struct rule_collection rules;\n+ struct rule *rule;\n \n- rule_collection_init(&rules);\n+ rule_collection_init(&rules);\n+ LIST_FOR_EACH (rule, meter_list_node, &meter->rules) {\n+ rule_collection_add(&rules, rule);\n+ }\n+ delete_flows__(&rules, OFPRR_METER_DELETE, NULL);\n+ }\n \n- LIST_FOR_EACH (rule, meter_list_node, &meter->rules) {\n- rule_collection_add(&rules, rule);\n- }\n- delete_flows__(&rules, OFPRR_METER_DELETE, NULL);\n- }\n+ ofproto->ofproto_class->meter_del(ofproto, meter->provider_meter_id);\n+ free(meter->bands);\n+ free(meter);\n+}\n \n- ofproto->meters[mid] = NULL;\n- ofproto->ofproto_class->meter_del(ofproto,\n- meter->provider_meter_id);\n- free(meter->bands);\n- free(meter);\n- }\n+static void\n+meter_delete(struct ofproto *ofproto, uint32_t meter_id)\n+ OVS_REQUIRES(ofproto_mutex)\n+{\n+ struct meter *meter = ofproto_get_meter(ofproto, meter_id);\n+\n+ if (meter) {\n+ hmap_remove(&ofproto->meters, &meter->node);\n+ meter_destroy(ofproto, meter);\n+ }\n+}\n+\n+static void\n+meter_delete_all(struct ofproto *ofproto)\n+ OVS_REQUIRES(ofproto_mutex)\n+{\n+ struct meter *meter, *next;\n+\n+ HMAP_FOR_EACH_SAFE (meter, next, node, &ofproto->meters) {\n+ hmap_remove(&ofproto->meters, &meter->node);\n+ meter_destroy(ofproto, meter);\n }\n }\n \n@@ -6319,10 +6354,10 @@ static enum ofperr\n handle_add_meter(struct ofproto *ofproto, struct ofputil_meter_mod *mm)\n {\n ofproto_meter_id provider_meter_id = { UINT32_MAX };\n- struct meter **meterp = &ofproto->meters[mm->meter.meter_id];\n+ struct meter *meter = ofproto_get_meter(ofproto, mm->meter.meter_id);\n enum ofperr error;\n \n- if (*meterp) {\n+ if (meter) {\n return OFPERR_OFPMMFC_METER_EXISTS;\n }\n \n@@ -6330,7 +6365,8 @@ handle_add_meter(struct ofproto *ofproto, struct ofputil_meter_mod *mm)\n &mm->meter);\n if (!error) {\n ovs_assert(provider_meter_id.uint32 != UINT32_MAX);\n- *meterp = meter_create(&mm->meter, provider_meter_id);\n+ meter = meter_create(&mm->meter, provider_meter_id);\n+ ofproto_add_meter(ofproto, meter);\n }\n return error;\n }\n@@ -6338,7 +6374,7 @@ handle_add_meter(struct ofproto *ofproto, struct ofputil_meter_mod *mm)\n static enum ofperr\n handle_modify_meter(struct ofproto *ofproto, struct ofputil_meter_mod *mm)\n {\n- struct meter *meter = ofproto->meters[mm->meter.meter_id];\n+ struct meter *meter = ofproto_get_meter(ofproto, mm->meter.meter_id);\n enum ofperr error;\n uint32_t provider_meter_id;\n \n@@ -6363,25 +6399,21 @@ handle_delete_meter(struct ofconn *ofconn, struct ofputil_meter_mod *mm)\n {\n struct ofproto *ofproto = ofconn_get_ofproto(ofconn);\n uint32_t meter_id = mm->meter.meter_id;\n- enum ofperr error = 0;\n- uint32_t first, last;\n \n- if (meter_id == OFPM13_ALL) {\n- first = 1;\n- last = ofproto->meter_features.max_meters;\n- } else {\n- if (!meter_id || meter_id > ofproto->meter_features.max_meters) {\n- return 0;\n+ /* OpenFlow does not support Meter ID 0. */\n+ if (meter_id) {\n+ ovs_mutex_lock(&ofproto_mutex);\n+\n+ if (meter_id == OFPM13_ALL) {\n+ meter_delete_all(ofproto);\n+ } else {\n+ meter_delete(ofproto, meter_id);\n }\n- first = last = meter_id;\n- }\n \n- /* Delete the meters. */\n- ovs_mutex_lock(&ofproto_mutex);\n- meter_delete(ofproto, first, last);\n- ovs_mutex_unlock(&ofproto_mutex);\n+ ovs_mutex_unlock(&ofproto_mutex);\n+ }\n \n- return error;\n+ return 0;\n }\n \n static enum ofperr\n@@ -6473,70 +6505,89 @@ handle_meter_features_request(struct ofconn *ofconn,\n return 0;\n }\n \n+static void\n+meter_request_reply(struct ofproto *ofproto, struct meter *meter,\n+ enum ofptype type, struct ovs_list *replies)\n+{\n+ uint64_t bands_stub[256 / 8];\n+ struct ofpbuf bands;\n+\n+ ofpbuf_use_stub(&bands, bands_stub, sizeof bands_stub);\n+\n+ if (type == OFPTYPE_METER_STATS_REQUEST) {\n+ struct ofputil_meter_stats stats;\n+\n+ stats.meter_id = meter->id;\n+\n+ /* Provider sets the packet and byte counts, we do the rest. */\n+ stats.flow_count = ovs_list_size(&meter->rules);\n+ calc_duration(meter->created, time_msec(),\n+ &stats.duration_sec, &stats.duration_nsec);\n+ stats.n_bands = meter->n_bands;\n+ ofpbuf_clear(&bands);\n+ stats.bands = ofpbuf_put_uninit(&bands, meter->n_bands\n+ * sizeof *stats.bands);\n+\n+ if (!ofproto->ofproto_class->meter_get(ofproto,\n+ meter->provider_meter_id,\n+ &stats, meter->n_bands)) {\n+ ofputil_append_meter_stats(replies, &stats);\n+ }\n+ } else { /* type == OFPTYPE_METER_CONFIG_REQUEST */\n+ struct ofputil_meter_config config;\n+\n+ config.meter_id = meter->id;\n+ config.flags = meter->flags;\n+ config.n_bands = meter->n_bands;\n+ config.bands = meter->bands;\n+ ofputil_append_meter_config(replies, &config);\n+ }\n+\n+ ofpbuf_uninit(&bands);\n+}\n+\n+static void\n+meter_request_reply_all(struct ofproto *ofproto, enum ofptype type,\n+ struct ovs_list *replies)\n+{\n+ struct meter *meter;\n+\n+ HMAP_FOR_EACH (meter, node, &ofproto->meters) {\n+ meter_request_reply(ofproto, meter, type, replies);\n+ }\n+}\n+\n static enum ofperr\n handle_meter_request(struct ofconn *ofconn, const struct ofp_header *request,\n enum ofptype type)\n {\n struct ofproto *ofproto = ofconn_get_ofproto(ofconn);\n struct ovs_list replies;\n- uint64_t bands_stub[256 / 8];\n- struct ofpbuf bands;\n- uint32_t meter_id, first, last;\n+ uint32_t meter_id;\n+ struct meter *meter;\n \n ofputil_decode_meter_request(request, &meter_id);\n \n- if (meter_id == OFPM13_ALL) {\n- first = 1;\n- last = ofproto->meter_features.max_meters;\n- } else {\n- if (!meter_id || meter_id > ofproto->meter_features.max_meters ||\n- !ofproto->meters[meter_id]) {\n+ if (meter_id != OFPM13_ALL) {\n+ meter = ofproto_get_meter(ofproto, meter_id);\n+ if (!meter) {\n+ /* Meter does not exist. */\n return OFPERR_OFPMMFC_UNKNOWN_METER;\n }\n- first = last = meter_id;\n+ } else {\n+ meter = NULL; /* GCC 4.9.2 complains about 'meter' can\n+ otentially used uninitialized. Logically,\n+ this is not possible, since meter is only used\n+ when meter_id != OFPM13_ALL. */\n }\n \n- ofpbuf_use_stub(&bands, bands_stub, sizeof bands_stub);\n ofpmp_init(&replies, request);\n-\n- for (meter_id = first; meter_id <= last; ++meter_id) {\n- struct meter *meter = ofproto->meters[meter_id];\n- if (!meter) {\n- continue; /* Skip non-existing meters. */\n- }\n- if (type == OFPTYPE_METER_STATS_REQUEST) {\n- struct ofputil_meter_stats stats;\n-\n- stats.meter_id = meter_id;\n-\n- /* Provider sets the packet and byte counts, we do the rest. */\n- stats.flow_count = ovs_list_size(&meter->rules);\n- calc_duration(meter->created, time_msec(),\n- &stats.duration_sec, &stats.duration_nsec);\n- stats.n_bands = meter->n_bands;\n- ofpbuf_clear(&bands);\n- stats.bands\n- = ofpbuf_put_uninit(&bands,\n- meter->n_bands * sizeof *stats.bands);\n-\n- if (!ofproto->ofproto_class->meter_get(ofproto,\n- meter->provider_meter_id,\n- &stats, meter->n_bands)) {\n- ofputil_append_meter_stats(&replies, &stats);\n- }\n- } else { /* type == OFPTYPE_METER_CONFIG_REQUEST */\n- struct ofputil_meter_config config;\n-\n- config.meter_id = meter_id;\n- config.flags = meter->flags;\n- config.n_bands = meter->n_bands;\n- config.bands = meter->bands;\n- ofputil_append_meter_config(&replies, &config);\n- }\n+ if (meter_id == OFPM13_ALL) {\n+ meter_request_reply_all(ofproto, type, &replies);\n+ } else {\n+ meter_request_reply(ofproto, meter, type, &replies);\n }\n-\n ofconn_send_replies(ofconn, &replies);\n- ofpbuf_uninit(&bands);\n return 0;\n }\n \n", "prefixes": [ "ovs-dev", "action", "upcall", "meter", "v2", "1/5" ] }