| Message ID | 20180809003522.10837-1-jpettit@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/2] dpif: Don't pass in '*meter_id' to meter_set commands. | expand |
On Wed, Aug 08, 2018 at 05:35:21PM -0700, Justin Pettit wrote: > 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 <jpettit@ovn.org> My suggestions are below in patch form. The change to probe_meters() is because I'm concerned about the thread-safety guarantee that dpif.h promises. It's easy to make this thread safe so we might as well. diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 60ce1a6d22a3..d9b404c2bb92 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -53,6 +53,7 @@ #include "openvswitch/ofpbuf.h" #include "openvswitch/poll-loop.h" #include "openvswitch/shash.h" +#include "openvswitch/thread.h" #include "openvswitch/vlog.h" #include "packets.h" #include "random.h" @@ -2983,7 +2984,7 @@ static void dpif_netlink_meter_get_features(const struct dpif *dpif_, struct ofputil_meter_features *features) { - if (probe_broken_meters((struct dpif *)dpif_)) { + if (probe_broken_meters(CONST_CAST(struct dpif *, dpif_))) { features = NULL; return; } @@ -3222,16 +3223,8 @@ dpif_netlink_meter_del(struct dpif *dpif, ofproto_meter_id meter_id, } static bool -probe_broken_meters(struct dpif *dpif) +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. */ @@ -3249,17 +3242,29 @@ probe_broken_meters(struct dpif *dpif) 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; + return true; } dpif_netlink_meter_del(dpif, id1, NULL, 0); dpif_netlink_meter_del(dpif, id2, NULL, 0); -done: - return broken_meters; + return false; } +static bool +probe_broken_meters(struct dpif *dpif) +{ + /* This is a once-only test because currently OVS only has at most a single + * Netlink capable datapath on any given platform. */ + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; + + static bool broken_meters = false; + if (ovsthread_once_start(&once)) { + broken_meters = probe_broken_meters__(dpif); + ovsthread_once_done(&once); + } + return broken_meters; +} const struct dpif_class dpif_netlink_class = { "system",
On Wed, Aug 08, 2018 at 05:35:21PM -0700, Justin Pettit wrote: > 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 <jpettit@ovn.org> For the series (please consider the comments on patch 2): Acked-by: Ben Pfaff <blp@ovn.org>
> On Aug 14, 2018, at 1:55 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Wed, Aug 08, 2018 at 05:35:21PM -0700, Justin Pettit wrote: >> 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 <jpettit@ovn.org> > > For the series (please consider the comments on patch 2): > Acked-by: Ben Pfaff <blp@ovn.org> Thanks! I pushed the changes with your suggestion to master and branch-2.10. --Justin
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 */
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 <jpettit@ovn.org> --- 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(-)