From patchwork Thu Aug 9 00:35:21 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Justin Pettit X-Patchwork-Id: 955293 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.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 41m8T723Hrz9s4Z for ; Thu, 9 Aug 2018 10:36:43 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 9BD35E31; Thu, 9 Aug 2018 00:36:08 +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 6AEC5C96 for ; Thu, 9 Aug 2018 00:36:06 +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 1BBD2737 for ; Thu, 9 Aug 2018 00:36:03 +0000 (UTC) X-Originating-IP: 66.170.99.2 Received: from localhost.localdomain (unknown [66.170.99.2]) (Authenticated sender: jpettit@ovn.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 91E751BF204 for ; Thu, 9 Aug 2018 00:36:01 +0000 (UTC) From: Justin Pettit To: dev@openvswitch.org Date: Wed, 8 Aug 2018 17:35:21 -0700 Message-Id: <20180809003522.10837-1-jpettit@ovn.org> X-Mailer: git-send-email 2.17.1 X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [PATCH 1/2] dpif: Don't pass in '*meter_id' to meter_set commands. 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 The original intent of the API appears to be that the underlying DPIF implementaion would choose a local meter id. However, neither of the existing datapath meter implementations (userspace or Linux) implemented that; they expected a valid meter id to be passed in, otherwise they returned an error. This commit follows the existing implementations and makes the API somewhat cleaner. Signed-off-by: Justin Pettit Acked-by: Ben Pfaff --- lib/dpif-netdev.c | 4 ++-- lib/dpif-netlink.c | 13 ++++++++----- lib/dpif-provider.h | 9 ++++----- lib/dpif.c | 14 ++++++-------- lib/dpif.h | 2 +- ofproto/ofproto-dpif.c | 2 +- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 26d07b39c9af..8c9062485bf7 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -5160,11 +5160,11 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_, /* Meter set/get/del processing is still single-threaded. */ static int -dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id *meter_id, +dpif_netdev_meter_set(struct dpif *dpif, ofproto_meter_id meter_id, struct ofputil_meter_config *config) { struct dp_netdev *dp = get_dp_netdev(dpif); - uint32_t mid = meter_id->uint32; + uint32_t mid = meter_id.uint32; struct dp_meter *meter; int i; diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index f669b1108d61..bf94e5413e71 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -3030,7 +3030,7 @@ dpif_netlink_meter_get_features(const struct dpif *dpif_, } static int -dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id *meter_id, +dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id meter_id, struct ofputil_meter_config *config) { struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); @@ -3057,9 +3057,8 @@ dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id *meter_id, dpif_netlink_meter_init(dpif, &buf, stub, sizeof stub, OVS_METER_CMD_SET); - if (meter_id->uint32 != UINT32_MAX) { - nl_msg_put_u32(&buf, OVS_METER_ATTR_ID, meter_id->uint32); - } + nl_msg_put_u32(&buf, OVS_METER_ATTR_ID, meter_id.uint32); + if (config->flags & OFPMF13_KBPS) { nl_msg_put_flag(&buf, OVS_METER_ATTR_KBPS); } @@ -3098,7 +3097,11 @@ dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id *meter_id, return error; } - meter_id->uint32 = nl_attr_get_u32(a[OVS_METER_ATTR_ID]); + if (nl_attr_get_u32(a[OVS_METER_ATTR_ID]) != meter_id.uint32) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + VLOG_INFO_RL(&rl, + "Kernel returned a different meter id than requested"); + } ofpbuf_delete(msg); return 0; } diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index 62b3598acfc5..8906d4e0a1e6 100644 --- a/lib/dpif-provider.h +++ b/lib/dpif-provider.h @@ -451,12 +451,11 @@ struct dpif_class { void (*meter_get_features)(const struct dpif *, struct ofputil_meter_features *); - /* Adds or modifies 'meter' in 'dpif'. If '*meter_id' is UINT32_MAX, - * adds a new meter, otherwise modifies an existing meter. + /* Adds or modifies the meter in 'dpif' with the given 'meter_id' + * and the configuration in 'config'. * - * If meter is successfully added, sets '*meter_id' to the new meter's - * meter id selected by 'dpif'. */ - int (*meter_set)(struct dpif *, ofproto_meter_id *meter_id, + * The meter id specified through 'config->meter_id' is ignored. */ + int (*meter_set)(struct dpif *, ofproto_meter_id meter_id, struct ofputil_meter_config *); /* Queries 'dpif' for meter stats with the given 'meter_id'. Stores diff --git a/lib/dpif.c b/lib/dpif.c index c267bcfb0c55..d799f972cbf6 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -1886,13 +1886,12 @@ dpif_meter_get_features(const struct dpif *dpif, } } -/* Adds or modifies meter identified by 'meter_id' in 'dpif'. If '*meter_id' - * is UINT32_MAX, adds a new meter, otherwise modifies an existing meter. +/* Adds or modifies the meter in 'dpif' with the given 'meter_id' and + * the configuration in 'config'. * - * If meter is successfully added, sets '*meter_id' to the new meter's - * meter number. */ + * The meter id specified through 'config->meter_id' is ignored. */ int -dpif_meter_set(struct dpif *dpif, ofproto_meter_id *meter_id, +dpif_meter_set(struct dpif *dpif, ofproto_meter_id meter_id, struct ofputil_meter_config *config) { COVERAGE_INC(dpif_meter_set); @@ -1918,11 +1917,10 @@ dpif_meter_set(struct dpif *dpif, ofproto_meter_id *meter_id, int error = dpif->dpif_class->meter_set(dpif, meter_id, config); if (!error) { VLOG_DBG_RL(&dpmsg_rl, "%s: DPIF meter %"PRIu32" set", - dpif_name(dpif), meter_id->uint32); + dpif_name(dpif), meter_id.uint32); } else { VLOG_WARN_RL(&error_rl, "%s: failed to set DPIF meter %"PRIu32": %s", - dpif_name(dpif), meter_id->uint32, ovs_strerror(error)); - meter_id->uint32 = UINT32_MAX; + dpif_name(dpif), meter_id.uint32, ovs_strerror(error)); } return error; } diff --git a/lib/dpif.h b/lib/dpif.h index 33d2d0bec333..bbdc3eb6cfb6 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -868,7 +868,7 @@ void dpif_print_packet(struct dpif *, struct dpif_upcall *); /* Meters. */ void dpif_meter_get_features(const struct dpif *, struct ofputil_meter_features *); -int dpif_meter_set(struct dpif *, ofproto_meter_id *meter_id, +int dpif_meter_set(struct dpif *, ofproto_meter_id meter_id, struct ofputil_meter_config *); int dpif_meter_get(const struct dpif *, ofproto_meter_id meter_id, struct ofputil_meter_stats *, uint16_t n_bands); diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index ad67e300a8be..e3abda571d31 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -5959,7 +5959,7 @@ meter_set(struct ofproto *ofproto_, ofproto_meter_id *meter_id, } } - switch (dpif_meter_set(ofproto->backer->dpif, meter_id, config)) { + switch (dpif_meter_set(ofproto->backer->dpif, *meter_id, config)) { case 0: return 0; case EFBIG: /* meter_id out of range */ From patchwork Thu Aug 9 00:35:22 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Justin Pettit X-Patchwork-Id: 955292 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.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 41m8SV5dcWz9s4Z for ; Thu, 9 Aug 2018 10:36:10 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 89C3DC96; Thu, 9 Aug 2018 00:36:06 +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 8D54BC96 for ; Thu, 9 Aug 2018 00:36:05 +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 7DB0F76D for ; Thu, 9 Aug 2018 00:36:04 +0000 (UTC) X-Originating-IP: 66.170.99.2 Received: from localhost.localdomain (unknown [66.170.99.2]) (Authenticated sender: jpettit@ovn.org) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 847851BF206 for ; Thu, 9 Aug 2018 00:36:02 +0000 (UTC) From: Justin Pettit To: dev@openvswitch.org Date: Wed, 8 Aug 2018 17:35:22 -0700 Message-Id: <20180809003522.10837-2-jpettit@ovn.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180809003522.10837-1-jpettit@ovn.org> References: <20180809003522.10837-1-jpettit@ovn.org> X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [PATCH 2/2] dpif-netlink: Probe for broken Linux meter implementations. 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 Meter support was introduced in Linux 4.15. In some versions of Linux 4.15, 4.16, and 4.17, there was a bug that never set the id when the meter was created, so all meters essentially had an id of zero. This commit adds a probe to check for that condition and disable meters on those kernels. Signed-off-by: Justin Pettit --- lib/dpif-netlink.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index bf94e5413e71..60ce1a6d22a3 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -2926,6 +2926,12 @@ dpif_netlink_ct_flush(struct dpif *dpif OVS_UNUSED, const uint16_t *zone, #define DP_SUPPORTED_METER_FLAGS_MASK \ (OFPMF13_STATS | OFPMF13_PKTPS | OFPMF13_KBPS | OFPMF13_BURST) +/* Meter support was introduced in Linux 4.15. In some versions of + * Linux 4.15, 4.16, and 4.17, there was a bug that never set the id + * when the meter was created, so all meters essentially had an id of + * zero. Check for that condition and disable meters on those kernels. */ +static bool probe_broken_meters(struct dpif *); + static void dpif_netlink_meter_init(struct dpif_netlink *dpif, struct ofpbuf *buf, void *stub, size_t size, uint32_t command) @@ -2977,6 +2983,11 @@ static void dpif_netlink_meter_get_features(const struct dpif *dpif_, struct ofputil_meter_features *features) { + if (probe_broken_meters((struct dpif *)dpif_)) { + features = NULL; + return; + } + struct ofpbuf buf, *msg; uint64_t stub[1024 / 8]; @@ -3033,6 +3044,10 @@ static int dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id meter_id, struct ofputil_meter_config *config) { + if (probe_broken_meters(dpif_)) { + return ENOMEM; + } + struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); struct ofpbuf buf, *msg; uint64_t stub[1024 / 8]; @@ -3206,6 +3221,45 @@ dpif_netlink_meter_del(struct dpif *dpif, ofproto_meter_id meter_id, OVS_METER_CMD_DEL); } +static bool +probe_broken_meters(struct dpif *dpif) +{ + static bool checked = false; + static bool broken_meters = false; + + if (checked) { + return broken_meters; + } + checked = true; + + /* This test is destructive if a probe occurs while ovs-vswitchd is + * running (e.g., an ovs-dpctl meter command is called), so choose a + * random high meter id to make this less likely to occur. */ + ofproto_meter_id id1 = { 54545401 }; + ofproto_meter_id id2 = { 54545402 }; + struct ofputil_meter_band band = {OFPMBT13_DROP, 0, 1, 0}; + struct ofputil_meter_config config1 = { 1, OFPMF13_KBPS, 1, &band}; + struct ofputil_meter_config config2 = { 2, OFPMF13_KBPS, 1, &band}; + + /* Try adding two meters and make sure that they both come back with + * the proper meter id. */ + dpif_netlink_meter_set(dpif, id1, &config1); + dpif_netlink_meter_set(dpif, id2, &config2); + + if (dpif_netlink_meter_get(dpif, id1, NULL, 0) + || dpif_netlink_meter_get(dpif, id2, NULL, 0)) { + VLOG_INFO("The kernel module has a broken meter implementation."); + broken_meters = true; + goto done; + } + + dpif_netlink_meter_del(dpif, id1, NULL, 0); + dpif_netlink_meter_del(dpif, id2, NULL, 0); + +done: + return broken_meters; +} + const struct dpif_class dpif_netlink_class = { "system",