| Message ID | 1512485720-77815-1-git-send-email-michalx.weglicki@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] netdev: Custom statistics. | expand |
Hi Ben, Did you have a chance to look at this patch re-work? Thank you in advance! Br, Michal. > -----Original Message----- > From: Weglicki, MichalX > Sent: Tuesday, December 5, 2017 3:55 PM > To: dev@openvswitch.org > Cc: Weglicki, MichalX <michalx.weglicki@intel.com> > Subject: [PATCH v2] netdev: Custom statistics. > > - New get_custom_stats interface function is added to netdev. It > allows particular netdev implementation to expose custom > counters in dictionary format (counter name/counter value). > - New statistics are retrieved using experimenter code and > are printed as a result to ofctl dump-ports. > - New counters are available for OpenFlow 1.4+. > - New statistics are printed to output via ofctl only if those > are present in reply message. > - New statistics definition is added to include/openflow/intel-ext.h. > - Custom statistics are implemented only for dpdk-physical > port type. > - DPDK-physical implementation uses xstats to collect statistics. > Only dropped and error counters are exposed. > > v1->v2: > - Buffer overrun check in parse_intel_port_custom_property. > - ofputil_append_ofp14_port_stats uses "postappend" instead > of "reserve" during message creation. > - NEWS update. > - DPDK documentation update. > - Compilation and sparse warnings corrections. > > Signed-off-by: Michal Weglicki <michalx.weglicki@intel.com> > --- > Documentation/howto/dpdk.rst | 15 ++-- > NEWS | 6 ++ > include/openflow/intel-ext.h | 28 ++++++ > include/openvswitch/netdev.h | 17 ++++ > include/openvswitch/ofp-util.h | 1 + > lib/netdev-dpdk.c | 195 ++++++++++++++++++++++++++++++++++++++++- > lib/netdev-dummy.c | 1 + > lib/netdev-linux.c | 1 + > lib/netdev-provider.h | 13 +++ > lib/netdev-vport.c | 1 + > lib/netdev.c | 27 ++++++ > lib/netdev.h | 2 + > lib/ofp-print.c | 18 ++++ > lib/ofp-util.c | 119 +++++++++++++++++++++++-- > lib/util.c | 13 +++ > lib/util.h | 2 + > ofproto/ofproto-dpif.c | 13 +++ > ofproto/ofproto-provider.h | 4 + > ofproto/ofproto.c | 21 +++++ > ofproto/ofproto.h | 3 + > vswitchd/bridge.c | 24 +++++ > 21 files changed, 512 insertions(+), 12 deletions(-) > > diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst > index d123819..c99ec29 100644 > --- a/Documentation/howto/dpdk.rst > +++ b/Documentation/howto/dpdk.rst > @@ -311,12 +311,16 @@ performance of non-tunnel traffic, specifically for smaller size packet. > > .. _extended-statistics: > > -Extended Statistics > -------------------- > +Extended & Custom Statistics > +---------------------------- > > DPDK Extended Statistics API allows PMD to expose unique set of statistics. > The Extended statistics are implemented and supported only for DPDK physical > -and vHost ports. > +and vHost ports. Custom statistics are dynamic set of counters which can > +vary depenend on a driver. Those statistics are implemented > +for DPDK physical ports and contain all "dropped" and "error" > +counters from XSTATS. XSTATS counters list can be found here: > +<https://wiki.opnfv.org/display/fastpath/Collectd+Metrics+and+Events>`__. > > To enable statistics, you have to enable OpenFlow 1.4 support for OVS. > Configure bridge br0 to support OpenFlow version 1.4:: > @@ -333,8 +337,9 @@ Query the port statistics by explicitly specifying -O OpenFlow14 option:: > > $ ovs-ofctl -O OpenFlow14 dump-ports br0 > > -Note: vHost ports supports only partial statistics. RX packet size based > -counter are only supported and doesn't include TX packet size counters. > +Note about "Extended Statistics": vHost ports supports only partial > +statistics. RX packet size based counter are only supported and > +doesn't include TX packet size counters. > > .. _port-hotplug: > > diff --git a/NEWS b/NEWS > index 427c8f8..727142c 100644 > --- a/NEWS > +++ b/NEWS > @@ -13,6 +13,12 @@ Post-v2.8.0 > * ovn-ctl: New commands run_nb_ovsdb and run_sb_ovsdb. > - Linux kernel 4.13 > * Add support for compiling OVS with the latest Linux 4.13 kernel > + - DPDK: > + * Custom statistics: > + - DPDK physical ports now return custom set of "dropped" and "error" > + statistics. > + - ovs-ofctl dump-ports command now prints new of set custom statistics > + if available (for OpenFlow 1.4+). > > v2.8.0 - 31 Aug 2017 > -------------------- > diff --git a/include/openflow/intel-ext.h b/include/openflow/intel-ext.h > index 974e63e..3d73171 100644 > --- a/include/openflow/intel-ext.h > +++ b/include/openflow/intel-ext.h > @@ -27,9 +27,11 @@ > > enum intel_port_stats_subtype { > INTEL_PORT_STATS_RFC2819 = 1, > + INTEL_PORT_STATS_CUSTOM > }; > > #define INTEL_PORT_STATS_RFC2819_SIZE 184 > +#define INTEL_PORT_STATS_CUSTOM_SIZE 16 > > /* Struct implements custom property type based on > * 'ofp_prop_experimenter'. */ > @@ -70,4 +72,30 @@ struct intel_port_stats_rfc2819 { > OFP_ASSERT(sizeof (struct intel_port_stats_rfc2819) == > INTEL_PORT_STATS_RFC2819_SIZE); > > +/* Structure implements custom property type based on > + * 'ofp_prop_experimenter'. It contains custom > + * statistics in dictionary format */ > +struct intel_port_custom_stats { > + ovs_be16 type; /* OFPPSPT14_EXPERIMENTER. */ > + ovs_be16 length; /* Length in bytes of this property excluding > + * trailing padding. */ > + ovs_be32 experimenter; /* INTEL_VENDOR_ID. */ > + ovs_be32 exp_type; /* INTEL_PORT_STATS_*. */ > + > + uint8_t pad[2]; > + ovs_be16 stats_array_size; /* number of counters. */ > + > + /* Followed by: > + * - Exactly 'stats_array_size' array elements of > + * dynamic structure which contains: > + * - "NAME SIZE" - counter name size (number of characters) > + * - "COUNTER NAME" - Exact number of characters > + * defined by "NAME SIZE". > + * - "COUNTER VALUE" - ovs_be64 counter value, > + * - Zero or more bytes to fill out the > + * overall length in header.length. */ > +}; > +OFP_ASSERT(sizeof(struct intel_port_custom_stats) == > + INTEL_PORT_STATS_CUSTOM_SIZE); > + > #endif /* openflow/intel-ext.h */ > diff --git a/include/openvswitch/netdev.h b/include/openvswitch/netdev.h > index 50bafc3..e25c241 100644 > --- a/include/openvswitch/netdev.h > +++ b/include/openvswitch/netdev.h > @@ -27,6 +27,9 @@ extern "C" { > > struct netdev; > > +/* Maximum name length for custom statistics counters */ > +#define NETDEV_CUSTOM_STATS_NAME_SIZE 64 > + > /* Network device statistics. > * > * Values of unsupported statistics are set to all-1-bits (UINT64_MAX). */ > @@ -85,6 +88,18 @@ struct netdev_stats { > uint64_t rx_jabber_errors; > }; > > +/* Structure representation of custom statistics counter */ > +struct netdev_custom_counter { > + uint64_t value; > + char name[NETDEV_CUSTOM_STATS_NAME_SIZE]; > +}; > + > +/* Structure representation of custom statistics */ > +struct netdev_custom_stats { > + uint16_t size; > + struct netdev_custom_counter *counters; > +}; > + > /* Features. */ > enum netdev_features { > NETDEV_F_10MB_HD = 1 << 0, /* 10 Mb half-duplex rate support. */ > @@ -115,6 +130,8 @@ uint64_t netdev_features_to_bps(enum netdev_features features, > bool netdev_features_is_full_duplex(enum netdev_features features); > int netdev_set_advertisements(struct netdev *, enum netdev_features advertise); > > +void netdev_free_custom_stats_counters(struct netdev_custom_stats *); > + > #ifdef __cplusplus > } > #endif > diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h > index a9e57ed..296078a 100644 > --- a/include/openvswitch/ofp-util.h > +++ b/include/openvswitch/ofp-util.h > @@ -1174,6 +1174,7 @@ bool ofputil_parse_key_value(char **stringp, char **keyp, char **valuep); > struct ofputil_port_stats { > ofp_port_t port_no; > struct netdev_stats stats; > + struct netdev_custom_stats custom_stats; > uint32_t duration_sec; /* UINT32_MAX if unknown. */ > uint32_t duration_nsec; > }; > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index faff842..8acc5e0 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -415,6 +415,14 @@ struct netdev_dpdk { > * from the enum set 'dpdk_hw_ol_features' */ > uint32_t hw_ol_features; > ); > + > + PADDED_MEMBERS(CACHE_LINE_SIZE, > + /* Names of all XSTATS counters */ > + struct rte_eth_xstat_name *rte_xstats_names; > + int rte_xstats_names_size; > + int rte_xstats_ids_size; > + uint64_t *rte_xstats_ids; > + ); > }; > > struct netdev_rxq_dpdk { > @@ -897,6 +905,12 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, > > netdev_request_reconfigure(netdev); > > + dev->rte_xstats_names = NULL; > + dev->rte_xstats_names_size = 0; > + > + dev->rte_xstats_ids = NULL; > + dev->rte_xstats_ids_size = 0; > + > return 0; > } > > @@ -1135,6 +1149,128 @@ netdev_dpdk_dealloc(struct netdev *netdev) > rte_free(dev); > } > > +static void > +netdev_dpdk_clear_xstats(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex) > +{ > + /* If statistics are already allocated, we have to > + * reconfigure, as port_id could have been changed. */ > + if (dev->rte_xstats_names) { > + free(dev->rte_xstats_names); > + dev->rte_xstats_names = NULL; > + dev->rte_xstats_names_size = 0; > + } > + if (dev->rte_xstats_ids) { > + free(dev->rte_xstats_ids); > + dev->rte_xstats_ids = NULL; > + dev->rte_xstats_ids_size = 0; > + } > +} > + > +static const char* > +netdev_dpdk_get_xstat_name(struct netdev_dpdk *dev, uint64_t id) > +{ > + if (id >= dev->rte_xstats_names_size) { > + return "UNKNOWN"; > + } > + return dev->rte_xstats_names[id].name; > +} > + > +static bool > +netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) > + OVS_REQUIRES(dev->mutex) > +{ > + int rte_xstats_len; > + bool ret; > + struct rte_eth_xstat *rte_xstats; > + uint64_t id; > + int xstats_no; > + const char *name; > + > + > + /* Retrieving all XSTATS names. If something will go wrong > + * or amount of counters will be equal 0, rte_xstats_names > + * buffer will be marked as NULL, and any further xstats > + * query won't be performed (e.g. during netdev_dpdk_get_stats > + * execution). */ > + > + ret = false; > + rte_xstats = NULL; > + > + if (dev->rte_xstats_names == NULL || dev->rte_xstats_ids == NULL) { > + > + dev->rte_xstats_names_size = > + rte_eth_xstats_get_names(dev->port_id, NULL, 0); > + > + if (dev->rte_xstats_names_size < 0) { > + VLOG_WARN("Cannot get XSTATS for port: %"PRIu8, dev->port_id); > + dev->rte_xstats_names_size = 0; > + } else { > + /* Reserve memory for xstats names and values */ > + dev->rte_xstats_names = xcalloc(dev->rte_xstats_names_size, > + sizeof *dev->rte_xstats_names); > + > + if (dev->rte_xstats_names) { > + /* Retreive xstats names */ > + rte_xstats_len = > + rte_eth_xstats_get_names(dev->port_id, > + dev->rte_xstats_names, > + dev->rte_xstats_names_size); > + > + if (rte_xstats_len < 0) { > + VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, > + dev->port_id); > + goto out; > + } > + else if (rte_xstats_len != dev->rte_xstats_names_size) { > + VLOG_WARN("XSTATS size doesn't match for port: %"PRIu8, > + dev->port_id); > + goto out; > + } > + > + dev->rte_xstats_ids = xcalloc(dev->rte_xstats_names_size, > + sizeof(uint64_t)); > + > + /* We have to calculate number of counters */ > + rte_xstats = xcalloc(rte_xstats_len, sizeof *rte_xstats); > + memset(rte_xstats, 0xff, sizeof *rte_xstats * rte_xstats_len); > + > + /* Retreive xstats values */ > + if (rte_eth_xstats_get(dev->port_id, rte_xstats, > + rte_xstats_len) > 0) { > + dev->rte_xstats_ids_size = 0; > + xstats_no = 0; > + for (uint32_t i = 0 ; i < rte_xstats_len ; i++) { > + id = rte_xstats[i].id; > + name = netdev_dpdk_get_xstat_name(dev, id); > + /* We need to filter out everything except > + * dropped and error counters */ > + if (string_ends_with(name, "_errors") || > + string_ends_with(name, "_dropped")) { > + > + dev->rte_xstats_ids[xstats_no] = id; > + xstats_no++; > + } > + } > + dev->rte_xstats_ids_size = xstats_no; > + ret = true; > + } else { > + VLOG_WARN("Can't get XSTATS IDs for port: %"PRIu8, > + dev->port_id); > + } > + } > + } > + } else { > + /* Already configured */ > + ret = true; > + } > + > +out: > + if (!ret) { > + netdev_dpdk_clear_xstats(dev); > + } > + return ret; > +} > + > static int > netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args) > { > @@ -1307,6 +1443,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > dev->devargs = xstrdup(new_devargs); > dev->port_id = new_port_id; > netdev_request_reconfigure(&dev->up); > + netdev_dpdk_clear_xstats(dev); > err = 0; > } > } > @@ -2171,6 +2308,56 @@ out: > } > > static int > +netdev_dpdk_get_custom_stats(const struct netdev *netdev, > + struct netdev_custom_stats *custom_stats) > +{ > + > + uint32_t i; > + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > + int rte_xstats_ret; > + > + ovs_mutex_lock(&dev->mutex); > + > + if (netdev_dpdk_configure_xstats(dev)) { > + > + uint64_t *values = xcalloc(dev->rte_xstats_ids_size, > + sizeof(uint64_t)); > + > + rte_xstats_ret = > + rte_eth_xstats_get_by_id(dev->port_id, dev->rte_xstats_ids, > + values, dev->rte_xstats_ids_size); > + > + if (rte_xstats_ret > 0 && > + rte_xstats_ret <= dev->rte_xstats_ids_size) { > + > + custom_stats->size = rte_xstats_ret; > + custom_stats->counters = > + (struct netdev_custom_counter *) xcalloc(rte_xstats_ret, > + sizeof(struct netdev_custom_counter)); > + > + for (i = 0; i < rte_xstats_ret; i++) { > + strncpy(custom_stats->counters[i].name, > + netdev_dpdk_get_xstat_name(dev, dev->rte_xstats_ids[i]), > + NETDEV_CUSTOM_STATS_NAME_SIZE); > + custom_stats->counters[i].value = values[i]; > + } > + } else { > + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, > + dev->port_id); > + custom_stats->counters = NULL; > + custom_stats->size = 0; > + /* Let's clear statistics cache, so it will be > + * reconfigured */ > + netdev_dpdk_clear_xstats(dev); > + } > + } > + > + ovs_mutex_unlock(&dev->mutex); > + > + return 0; > +} > + > +static int > netdev_dpdk_get_features(const struct netdev *netdev, > enum netdev_features *current, > enum netdev_features *advertised, > @@ -3313,7 +3500,8 @@ unlock: > > #define NETDEV_DPDK_CLASS(NAME, INIT, CONSTRUCT, DESTRUCT, \ > SET_CONFIG, SET_TX_MULTIQ, SEND, \ > - GET_CARRIER, GET_STATS, \ > + GET_CARRIER, GET_STATS, \ > + GET_CUSTOM_STATS, \ > GET_FEATURES, GET_STATUS, \ > RECONFIGURE, RXQ_RECV) \ > { \ > @@ -3348,6 +3536,7 @@ unlock: > netdev_dpdk_get_carrier_resets, \ > netdev_dpdk_set_miimon, \ > GET_STATS, \ > + GET_CUSTOM_STATS, \ > GET_FEATURES, \ > NULL, /* set_advertisements */ \ > NULL, /* get_pt_mode */ \ > @@ -3397,6 +3586,7 @@ static const struct netdev_class dpdk_class = > netdev_dpdk_eth_send, > netdev_dpdk_get_carrier, > netdev_dpdk_get_stats, > + netdev_dpdk_get_custom_stats, > netdev_dpdk_get_features, > netdev_dpdk_get_status, > netdev_dpdk_reconfigure, > @@ -3413,6 +3603,7 @@ static const struct netdev_class dpdk_ring_class = > netdev_dpdk_ring_send, > netdev_dpdk_get_carrier, > netdev_dpdk_get_stats, > + netdev_dpdk_get_custom_stats, > netdev_dpdk_get_features, > netdev_dpdk_get_status, > netdev_dpdk_reconfigure, > @@ -3431,6 +3622,7 @@ static const struct netdev_class dpdk_vhost_class = > netdev_dpdk_vhost_get_stats, > NULL, > NULL, > + NULL, > netdev_dpdk_vhost_reconfigure, > netdev_dpdk_vhost_rxq_recv); > static const struct netdev_class dpdk_vhost_client_class = > @@ -3446,6 +3638,7 @@ static const struct netdev_class dpdk_vhost_client_class = > netdev_dpdk_vhost_get_stats, > NULL, > NULL, > + NULL, > netdev_dpdk_vhost_client_reconfigure, > netdev_dpdk_vhost_rxq_recv); > > diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c > index 246cdf1..1731b77 100644 > --- a/lib/netdev-dummy.c > +++ b/lib/netdev-dummy.c > @@ -1383,6 +1383,7 @@ netdev_dummy_update_flags(struct netdev *netdev_, > NULL, /* get_carrier_resets */ \ > NULL, /* get_miimon */ \ > netdev_dummy_get_stats, \ > + NULL, \ > \ > NULL, /* get_features */ \ > NULL, /* set_advertisements */ \ > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index e809b88..4e9be30 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -2853,6 +2853,7 @@ netdev_linux_update_flags(struct netdev *netdev_, enum netdev_flags off, > netdev_linux_get_carrier_resets, \ > netdev_linux_set_miimon_interval, \ > GET_STATS, \ > + NULL, \ > \ > GET_FEATURES, \ > netdev_linux_set_advertisements, \ > diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h > index 1720deb..d66fd5b 100644 > --- a/lib/netdev-provider.h > +++ b/lib/netdev-provider.h > @@ -459,6 +459,19 @@ struct netdev_class { > * (UINT64_MAX). */ > int (*get_stats)(const struct netdev *netdev, struct netdev_stats *); > > + /* Retrieves current device custom stats for 'netdev' into 'custom_stats'. > + * > + * A network device should return only available statistics (if any). > + * If there are not statistics available, empty array should be > + * returned. > + * > + * The caller initializes 'custom_stats' before calling this function. > + * The caller takes ownership over allocated array of counters inside > + * structure netdev_custom_stats. > + * */ > + int (*get_custom_stats)(const struct netdev *netdev, > + struct netdev_custom_stats *custom_stats); > + > /* Stores the features supported by 'netdev' into each of '*current', > * '*advertised', '*supported', and '*peer'. Each value is a bitmap of > * NETDEV_F_* bits. > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index 518058a..1a3322b 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -906,6 +906,7 @@ netdev_vport_get_ifindex(const struct netdev *netdev_) > NULL, /* get_carrier_resets */ \ > NULL, /* get_miimon */ \ > get_stats, \ > + NULL, \ > \ > NULL, /* get_features */ \ > NULL, /* set_advertisements */ \ > diff --git a/lib/netdev.c b/lib/netdev.c > index 2d69fe5..cd11930 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -1425,6 +1425,21 @@ netdev_get_stats(const struct netdev *netdev, struct netdev_stats *stats) > return error; > } > > +/* Retrieves current device custom stats for 'netdev'. */ > +int > +netdev_get_custom_stats(const struct netdev *netdev, > + struct netdev_custom_stats *custom_stats) > +{ > + int error; > + memset(custom_stats, 0, sizeof *custom_stats); > + error = (netdev->netdev_class->get_custom_stats > + ? netdev->netdev_class->get_custom_stats(netdev, custom_stats) > + : EOPNOTSUPP); > + > + return error; > +} > + > + > /* Attempts to set input rate limiting (policing) policy, such that up to > * 'kbits_rate' kbps of traffic is accepted, with a maximum accumulative burst > * size of 'kbits' kb. */ > @@ -2380,6 +2395,18 @@ netdev_ports_flow_get(const struct dpif_class *dpif_class, struct match *match, > return ENOENT; > } > > +void > +netdev_free_custom_stats_counters(struct netdev_custom_stats *custom_stats) > +{ > + if (custom_stats) { > + if (custom_stats->counters) { > + free(custom_stats->counters); > + custom_stats->counters = NULL; > + custom_stats->size = 0; > + } > + } > +} > + > #ifdef __linux__ > static void > netdev_ports_flow_init(void) > diff --git a/lib/netdev.h b/lib/netdev.h > index 3a545fe..34df7c9 100644 > --- a/lib/netdev.h > +++ b/lib/netdev.h > @@ -296,6 +296,8 @@ struct netdev *netdev_find_dev_by_in4(const struct in_addr *); > > /* Statistics. */ > int netdev_get_stats(const struct netdev *, struct netdev_stats *); > +int netdev_get_custom_stats(const struct netdev *, > + struct netdev_custom_stats *); > > /* Quality of service. */ > struct netdev_qos_capabilities { > diff --git a/lib/ofp-print.c b/lib/ofp-print.c > index 151d618..9a5768d 100644 > --- a/lib/ofp-print.c > +++ b/lib/ofp-print.c > @@ -1834,6 +1834,7 @@ ofp_print_ofpst_port_reply(struct ds *string, const struct ofp_header *oh, > const struct ofputil_port_map *port_map, > int verbosity) > { > + uint32_t i; > ds_put_format(string, " %"PRIuSIZE" ports\n", ofputil_count_port_stats(oh)); > if (verbosity < 1) { > return; > @@ -1946,6 +1947,23 @@ ofp_print_ofpst_port_reply(struct ds *string, const struct ofp_header *oh, > ds_put_cstr(string, "\n"); > ds_destroy(&string_ext_stats); > } > + > + if (ps.custom_stats.size) { > + ds_put_cstr(string, " CUSTOM Statistics "); > + for (i = 0 ; i < ps.custom_stats.size ; i++) { > + /* 3 counters in the row */ > + if (strlen(ps.custom_stats.counters[i].name)) { > + if ((i % 3) == 0) { > + ds_put_cstr(string, "\n"); > + ds_put_cstr(string, " "); > + } > + ds_put_format(string, "%s=%"PRIu64", ", > + ps.custom_stats.counters[i].name, > + ps.custom_stats.counters[i].value); > + } > + } > + ds_put_cstr(string, "\n"); > + } > } > } > > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > index 47f30c7..0652588 100644 > --- a/lib/ofp-util.c > +++ b/lib/ofp-util.c > @@ -7999,15 +7999,18 @@ ofputil_append_ofp14_port_stats(const struct ofputil_port_stats *ops, > { > struct ofp14_port_stats_prop_ethernet *eth; > struct intel_port_stats_rfc2819 *stats_rfc2819; > + struct intel_port_custom_stats *stats_custom; > struct ofp14_port_stats *ps14; > struct ofpbuf *reply; > + uint16_t i; > + uint64_t counter_value; > + size_t custom_stats_start, start_ofs; > > - reply = ofpmp_reserve(replies, sizeof *ps14 + sizeof *eth + > - sizeof *stats_rfc2819); > + reply = ofpbuf_from_list(ovs_list_back(replies)); > + start_ofs = reply->size; > > ps14 = ofpbuf_put_uninit(reply, sizeof *ps14); > - ps14->length = htons(sizeof *ps14 + sizeof *eth + > - sizeof *stats_rfc2819); > + > memset(ps14->pad, 0, sizeof ps14->pad); > ps14->port_no = ofputil_port_to_ofp11(ops->port_no); > ps14->duration_sec = htonl(ops->duration_sec); > @@ -8027,10 +8030,10 @@ ofputil_append_ofp14_port_stats(const struct ofputil_port_stats *ops, > eth->rx_crc_err = htonll(ops->stats.rx_crc_errors); > eth->collisions = htonll(ops->stats.collisions); > > - uint64_t prop_type = OFPPROP_EXP(INTEL_VENDOR_ID, > + uint64_t prop_type_stats = OFPPROP_EXP(INTEL_VENDOR_ID, > INTEL_PORT_STATS_RFC2819); > > - stats_rfc2819 = ofpprop_put_zeros(reply, prop_type, > + stats_rfc2819 = ofpprop_put_zeros(reply, prop_type_stats, > sizeof *stats_rfc2819); > > memset(stats_rfc2819->pad, 0, sizeof stats_rfc2819->pad); > @@ -8076,6 +8079,44 @@ ofputil_append_ofp14_port_stats(const struct ofputil_port_stats *ops, > htonll(ops->stats.rx_fragmented_errors); > stats_rfc2819->rx_jabber_errors = > htonll(ops->stats.rx_jabber_errors); > + > + if (ops->custom_stats.counters && ops->custom_stats.size) { > + > + custom_stats_start = reply->size; > + > + uint64_t prop_type_custom = OFPPROP_EXP(INTEL_VENDOR_ID, > + INTEL_PORT_STATS_CUSTOM); > + > + stats_custom = ofpprop_put_zeros(reply, prop_type_custom, > + sizeof *stats_custom); > + > + stats_custom->stats_array_size = htons(ops->custom_stats.size); > + memset(stats_custom->pad, 0, sizeof stats_custom->pad); > + > + for (i = 0 ; i < ops->custom_stats.size; i++) { > + uint8_t counter_size = strnlen(ops->custom_stats.counters[i].name, > + NETDEV_CUSTOM_STATS_NAME_SIZE); > + /* Counter name size */ > + ofpbuf_put(reply, &counter_size, sizeof(counter_size)); > + /* Counter name */ > + ofpbuf_put(reply, ops->custom_stats.counters[i].name, > + counter_size); > + /* Counter value */ > + counter_value = htonll(ops->custom_stats.counters[i].value); > + ofpbuf_put(reply, &counter_value, > + sizeof(ops->custom_stats.counters[i].value)); > + } > + > + ofpbuf_padto(reply, ROUND_UP(reply->size, 8)); > + stats_custom = ofpbuf_at_assert(reply, custom_stats_start, > + sizeof *stats_custom); > + stats_custom->length = htons(reply->size - custom_stats_start); > + } > + > + ps14 = ofpbuf_at_assert(reply, start_ofs, sizeof *ps14); > + ps14->length = htons(reply->size - start_ofs); > + > + ofpmp_postappend(replies, start_ofs); > } > > /* Encode a ports stat for 'ops' and append it to 'replies'. */ > @@ -8239,6 +8280,63 @@ parse_intel_port_stats_rfc2819_property(const struct ofpbuf *payload, > } > > static enum ofperr > +parse_intel_port_custom_property(const struct ofpbuf *payload, > + struct ofputil_port_stats *ops) > +{ > + uint16_t i; > + > + const struct intel_port_custom_stats *custom_stats = payload->data; > + > + ops->custom_stats.size = ntohs(custom_stats->stats_array_size); > + > + ops->custom_stats.counters = (struct netdev_custom_counter *) > + xcalloc(ops->custom_stats.size, > + sizeof(struct netdev_custom_counter)); > + > + uint16_t msg_size = ntohs(custom_stats->length); > + uint16_t current_len = sizeof( *custom_stats); > + uint8_t *current = (uint8_t *)payload->data + current_len; > + uint8_t string_size = 0; > + uint8_t value_size = 0; > + uint64_t counter_value = 0; > + > + for (i = 0 ; i < ops->custom_stats.size ; i++) { > + > + current_len += string_size + value_size; > + current += string_size + value_size; > + > + value_size = sizeof(uint64_t); > + /* Counter name size */ > + string_size = *current; > + > + /* Buffer overrun check */ > + if (current_len + string_size + value_size > msg_size) { > + VLOG_ERR("Custom statistics buffer overrun! " > + "Further message parsing is aborted."); > + break; > + } > + > + current++; > + current_len++; > + /* Counter name */ > + if (string_size > sizeof(ops->custom_stats.counters[i].name)) { > + VLOG_WARN("Counter name size too big! Only part " > + "of the name will be copied."); > + memcpy(ops->custom_stats.counters[i].name, current, > + sizeof(ops->custom_stats.counters[i].name)); > + } else { > + memcpy(ops->custom_stats.counters[i].name, current, string_size); > + } > + /* Counter value */ > + memcpy(&counter_value, current + string_size, > + value_size); > + ops->custom_stats.counters[i].value = ntohll(counter_value); > + } > + > + return 0; > +} > + > +static enum ofperr > parse_intel_port_stats_property(const struct ofpbuf *payload, > uint32_t exp_type, > struct ofputil_port_stats *ops) > @@ -8249,6 +8347,9 @@ parse_intel_port_stats_property(const struct ofpbuf *payload, > case INTEL_PORT_STATS_RFC2819: > error = parse_intel_port_stats_rfc2819_property(payload, ops); > break; > + case INTEL_PORT_STATS_CUSTOM: > + error = parse_intel_port_custom_property(payload, ops); > + break; > default: > error = OFPERR_OFPBPC_BAD_EXP_TYPE; > break; > @@ -8308,6 +8409,11 @@ ofputil_pull_ofp14_port_stats(struct ofputil_port_stats *ops, > INTEL_PORT_STATS_RFC2819, > ops); > break; > + case OFPPROP_EXP(INTEL_VENDOR_ID, INTEL_PORT_STATS_CUSTOM): > + error = parse_intel_port_stats_property(&payload, > + INTEL_PORT_STATS_CUSTOM, > + ops); > + break; > default: > error = OFPPROP_UNKNOWN(true, "port stats", type); > break; > @@ -8354,6 +8460,7 @@ ofputil_decode_port_stats(struct ofputil_port_stats *ps, struct ofpbuf *msg) > enum ofpraw raw; > > memset(&(ps->stats), 0xFF, sizeof (ps->stats)); > + memset(&(ps->custom_stats), 0, sizeof (ps->custom_stats)); > > error = (msg->header ? ofpraw_decode(&raw, msg->header) > : ofpraw_pull(&raw, msg)); > diff --git a/lib/util.c b/lib/util.c > index 2965656..462c1fa 100644 > --- a/lib/util.c > +++ b/lib/util.c > @@ -321,6 +321,19 @@ ovs_strzcpy(char *dst, const char *src, size_t size) > } > } > > +/* > + * Returns true if 'str' ends with given 'suffix'. > + */ > +int > +string_ends_with(const char * str, const char * suffix) > +{ > + int str_len = strlen(str); > + int suffix_len = strlen(suffix); > + > + return (str_len >= suffix_len) && > + (0 == strcmp(str + (str_len - suffix_len), suffix)); > +} > + > /* Prints 'format' on stderr, formatting it like printf() does. If 'err_no' is > * nonzero, then it is formatted with ovs_retval_to_string() and appended to > * the message inside parentheses. Then, terminates with abort(). > diff --git a/lib/util.h b/lib/util.h > index b01f421..e75f6ce 100644 > --- a/lib/util.h > +++ b/lib/util.h > @@ -157,6 +157,8 @@ void free_cacheline(void *); > void ovs_strlcpy(char *dst, const char *src, size_t size); > void ovs_strzcpy(char *dst, const char *src, size_t size); > > +int string_ends_with(const char * str, const char * suffix); > + > /* The C standards say that neither the 'dst' nor 'src' argument to > * memcpy() may be null, even if 'n' is zero. This wrapper tolerates > * the null case. */ > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index b99f04f..e53ee35 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -3775,6 +3775,18 @@ port_get_stats(const struct ofport *ofport_, struct netdev_stats *stats) > } > > static int > +port_get_custom_stats(const struct ofport *ofport_, > + struct netdev_custom_stats *custom_stats) > +{ > + struct ofport_dpif *ofport = ofport_dpif_cast(ofport_); > + int error; > + > + error = netdev_get_custom_stats(ofport->up.netdev, custom_stats); > + > + return error; > +} > + > +static int > port_get_lacp_stats(const struct ofport *ofport_, struct lacp_slave_stats *stats) > { > struct ofport_dpif *ofport = ofport_dpif_cast(ofport_); > @@ -5785,6 +5797,7 @@ const struct ofproto_class ofproto_dpif_class = { > port_del, > port_set_config, > port_get_stats, > + port_get_custom_stats, > port_dump_start, > port_dump_next, > port_dump_done, > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 9dc73c4..55e772e 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -1049,6 +1049,10 @@ struct ofproto_class { > int (*port_get_stats)(const struct ofport *port, > struct netdev_stats *stats); > > + /* Get port custom stats */ > + int (*port_get_custom_stats)(const struct ofport *port, > + struct netdev_custom_stats *custom_stats); > + > /* Port iteration functions. > * > * The client might not be entirely in control of the ports within an > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 82c2bb2..c76a9a1 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -2605,6 +2605,24 @@ ofproto_port_get_stats(const struct ofport *port, struct netdev_stats *stats) > return error; > } > > +int > +ofproto_port_get_custom_stats(const struct ofport *port, > + struct netdev_custom_stats *custom_stats) > +{ > + struct ofproto *ofproto = port->ofproto; > + int error; > + > + if (ofproto->ofproto_class->port_get_custom_stats) { > + memset(custom_stats, 0, sizeof *custom_stats); > + error = ofproto->ofproto_class->port_get_custom_stats(port, > + custom_stats); > + } else { > + error = EOPNOTSUPP; > + } > + > + return error; > +} > + > static int > update_port(struct ofproto *ofproto, const char *name) > { > @@ -3887,8 +3905,11 @@ append_port_stat(struct ofport *port, struct ovs_list *replies) > * 'stats' to all-1s, which is correct for OpenFlow, and > * netdev_get_stats() will log errors. */ > ofproto_port_get_stats(port, &ops.stats); > + ofproto_port_get_custom_stats(port, &ops.custom_stats); > > ofputil_append_port_stat(replies, &ops); > + > + netdev_free_custom_stats_counters(&ops.custom_stats); > } > > static void > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h > index 1e48e19..15e478d 100644 > --- a/ofproto/ofproto.h > +++ b/ofproto/ofproto.h > @@ -48,6 +48,7 @@ struct shash; > struct simap; > struct smap; > struct netdev_stats; > +struct netdev_custom_stats; > struct ovs_list; > struct lldp_status; > struct aa_settings; > @@ -297,6 +298,8 @@ int ofproto_port_del(struct ofproto *, ofp_port_t ofp_port); > void ofproto_port_set_config(struct ofproto *, ofp_port_t ofp_port, > const struct smap *cfg); > int ofproto_port_get_stats(const struct ofport *, struct netdev_stats *stats); > +int ofproto_port_get_custom_stats(const struct ofport *port, > + struct netdev_custom_stats *custom_stats); > > int ofproto_port_query_by_name(const struct ofproto *, const char *devname, > struct ofproto_port *); > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 630c6fa..1856908 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -2347,6 +2347,11 @@ iface_refresh_cfm_stats(struct iface *iface) > static void > iface_refresh_stats(struct iface *iface) > { > + struct netdev_custom_stats custom_stats; > + uint32_t i; > + int64_t *custom_values = NULL; > + char **custom_keys = NULL; > + > #define IFACE_STATS \ > IFACE_STAT(rx_packets, "rx_packets") \ > IFACE_STAT(tx_packets, "tx_packets") \ > @@ -2413,6 +2418,25 @@ iface_refresh_stats(struct iface *iface) > > ovsrec_interface_set_statistics(iface->cfg, keys, values, n); > #undef IFACE_STATS > + > + netdev_get_custom_stats(iface->netdev, &custom_stats); > + > + if (custom_stats.size && custom_stats.counters) { > + custom_values = xmalloc(custom_stats.size * sizeof *custom_values); > + custom_keys = xmalloc(custom_stats.size * sizeof(char *)); > + if (custom_values && custom_keys) { > + for (i = 0 ; i < custom_stats.size ; i++) { > + custom_values[i] = custom_stats.counters[i].value; > + custom_keys[i] = custom_stats.counters[i].name; > + } > + ovsrec_interface_set_statistics(iface->cfg, > + (const char **)custom_keys, > + custom_values, custom_stats.size); > + free(custom_values); > + free(custom_keys); > + } > + netdev_free_custom_stats_counters(&custom_stats); > + } > } > > static void > -- > 1.8.3.1
On 12/05/2017 02:55 PM, Michal Weglicki wrote: > - New get_custom_stats interface function is added to netdev. It > allows particular netdev implementation to expose custom > counters in dictionary format (counter name/counter value). > - New statistics are retrieved using experimenter code and > are printed as a result to ofctl dump-ports. > - New counters are available for OpenFlow 1.4+. > - New statistics are printed to output via ofctl only if those > are present in reply message. > - New statistics definition is added to include/openflow/intel-ext.h. > - Custom statistics are implemented only for dpdk-physical > port type. > - DPDK-physical implementation uses xstats to collect statistics. > Only dropped and error counters are exposed. > Hi Michal - why only dropped and error counters? why not just expose them all. For example, IIUC this would report management dropped packets but there would not be a stat for management rx/tx successful packets. Kevin. > v1->v2: > - Buffer overrun check in parse_intel_port_custom_property. > - ofputil_append_ofp14_port_stats uses "postappend" instead > of "reserve" during message creation. > - NEWS update. > - DPDK documentation update. > - Compilation and sparse warnings corrections. > > Signed-off-by: Michal Weglicki <michalx.weglicki@intel.com>
> -----Original Message----- > From: Kevin Traynor [mailto:ktraynor@redhat.com] > Sent: Tuesday, December 19, 2017 3:07 PM > To: Weglicki, MichalX <michalx.weglicki@intel.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2] netdev: Custom statistics. > > On 12/05/2017 02:55 PM, Michal Weglicki wrote: > > - New get_custom_stats interface function is added to netdev. It > > allows particular netdev implementation to expose custom > > counters in dictionary format (counter name/counter value). > > - New statistics are retrieved using experimenter code and > > are printed as a result to ofctl dump-ports. > > - New counters are available for OpenFlow 1.4+. > > - New statistics are printed to output via ofctl only if those > > are present in reply message. > > - New statistics definition is added to include/openflow/intel-ext.h. > > - Custom statistics are implemented only for dpdk-physical > > port type. > > - DPDK-physical implementation uses xstats to collect statistics. > > Only dropped and error counters are exposed. > > > > Hi Michal - why only dropped and error counters? why not just expose > them all. For example, IIUC this would report management dropped packets > but there would not be a stat for management rx/tx successful packets. > > Kevin. Hi Kevin - those counters were of biggest value to us at the point of making this patch, sending all counters (where for IXGBE is about 150) will produce some movement on the network. I think that biggest advantage of this particular patch is that it introduces a mechanism to expose any counters, counters list can be extended in the future if necessary. However I'm not sure if sending all counters is good idea, as there could be thousands of it in the future - in this solution, we have some kind of control over the data size. Michal. > > > v1->v2: > > - Buffer overrun check in parse_intel_port_custom_property. > > - ofputil_append_ofp14_port_stats uses "postappend" instead > > of "reserve" during message creation. > > - NEWS update. > > - DPDK documentation update. > > - Compilation and sparse warnings corrections. > > > > Signed-off-by: Michal Weglicki <michalx.weglicki@intel.com>
On 12/19/2017 02:35 PM, Weglicki, MichalX wrote: >> -----Original Message----- >> From: Kevin Traynor [mailto:ktraynor@redhat.com] >> Sent: Tuesday, December 19, 2017 3:07 PM >> To: Weglicki, MichalX <michalx.weglicki@intel.com>; dev@openvswitch.org >> Subject: Re: [ovs-dev] [PATCH v2] netdev: Custom statistics. >> >> On 12/05/2017 02:55 PM, Michal Weglicki wrote: >>> - New get_custom_stats interface function is added to netdev. It >>> allows particular netdev implementation to expose custom >>> counters in dictionary format (counter name/counter value). >>> - New statistics are retrieved using experimenter code and >>> are printed as a result to ofctl dump-ports. >>> - New counters are available for OpenFlow 1.4+. >>> - New statistics are printed to output via ofctl only if those >>> are present in reply message. >>> - New statistics definition is added to include/openflow/intel-ext.h. >>> - Custom statistics are implemented only for dpdk-physical >>> port type. >>> - DPDK-physical implementation uses xstats to collect statistics. >>> Only dropped and error counters are exposed. >>> >> >> Hi Michal - why only dropped and error counters? why not just expose >> them all. For example, IIUC this would report management dropped packets >> but there would not be a stat for management rx/tx successful packets. >> >> Kevin. > > Hi Kevin - those counters were of biggest value to us at the point of making this > patch, sending all counters (where for IXGBE is about 150) will produce > some movement on the network. I think that biggest advantage of this > particular patch is that it introduces a mechanism to expose > any counters, counters list can be extended in the future > if necessary. However I'm not sure if sending all counters > is good idea, as there could be thousands of it in the future - in this > solution, we have some kind of control over the data size. > Ok thanks, that makes sense. I would like to suggest that *_management_* be added as part of this as I think it's only 2 additional stats and I've seen at least one user saying they needed this information. > Michal. > >> >>> v1->v2: >>> - Buffer overrun check in parse_intel_port_custom_property. >>> - ofputil_append_ofp14_port_stats uses "postappend" instead >>> of "reserve" during message creation. >>> - NEWS update. >>> - DPDK documentation update. >>> - Compilation and sparse warnings corrections. >>> >>> Signed-off-by: Michal Weglicki <michalx.weglicki@intel.com> >
On Tue, Dec 05, 2017 at 02:55:20PM +0000, Michal Weglicki wrote: > - New get_custom_stats interface function is added to netdev. It > allows particular netdev implementation to expose custom > counters in dictionary format (counter name/counter value). > - New statistics are retrieved using experimenter code and > are printed as a result to ofctl dump-ports. > - New counters are available for OpenFlow 1.4+. > - New statistics are printed to output via ofctl only if those > are present in reply message. > - New statistics definition is added to include/openflow/intel-ext.h. > - Custom statistics are implemented only for dpdk-physical > port type. > - DPDK-physical implementation uses xstats to collect statistics. > Only dropped and error counters are exposed. > > v1->v2: > - Buffer overrun check in parse_intel_port_custom_property. > - ofputil_append_ofp14_port_stats uses "postappend" instead > of "reserve" during message creation. > - NEWS update. > - DPDK documentation update. > - Compilation and sparse warnings corrections. > > Signed-off-by: Michal Weglicki <michalx.weglicki@intel.com> Thank you for the updated patch. I still see some "sparse" warnings (there's an ovs-dev thread about trouble with "sparse"--maybe you should join it): ../lib/ofp-util.c:8105:27: warning: incorrect type in assignment (different base types) ../lib/ofp-util.c:8105:27: expected unsigned long long [unsigned] [usertype] counter_value ../lib/ofp-util.c:8105:27: got restricted ovs_be64 ../lib/ofp-util.c:8333:54: warning: incorrect type in argument 1 (different base types) ../lib/ofp-util.c:8333:54: expected restricted ovs_be64 [usertype] <noident> ../lib/ofp-util.c:8333:54: got unsigned long long [unsigned] [addressable] [usertype] counter_value I don't think that the new ofproto_class function port_get_custom_stats is needed. The generic ofproto code already knows that every port is associated with a netdev. I think that append_port_stats() in ofproto.c can just call netdev_get_custom_stats() directly rather than through this additional level of indirection. I don't like the idea implied in the code in a few places that name[] in struct netdev_custom_counter might not be null-terminated. I think that we should ensure that it is always null terminated. Otherwise there is a pitfall for carelessly written code. I would like to see some tests. The most obvious need is a new test for ofp-print.at that exercises parsing and printing a stats message that includes some of these custom stats. I have a few other minor suggestions. I'm appending them as an incremental patch. Many of these are minor style points. I think that most of them will be self-explanatory. Please let me know if they make sense. I'll look forward to v3. (After I'm happy with this, I'll probably ask Ian to review it to be added to his dpdk merge branch.) --8<--------------------------cut here-------------------------->8-- diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 07be4a21cd2c..3ccbec11f592 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1205,7 +1205,6 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) int xstats_no; const char *name; - /* Retrieving all XSTATS names. If something will go wrong * or amount of counters will be equal 0, rte_xstats_names * buffer will be marked as NULL, and any further xstats @@ -1216,7 +1215,6 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) rte_xstats = NULL; if (dev->rte_xstats_names == NULL || dev->rte_xstats_ids == NULL) { - dev->rte_xstats_names_size = rte_eth_xstats_get_names(dev->port_id, NULL, 0); @@ -1239,8 +1237,7 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, dev->port_id); goto out; - } - else if (rte_xstats_len != dev->rte_xstats_names_size) { + } else if (rte_xstats_len != dev->rte_xstats_names_size) { VLOG_WARN("XSTATS size doesn't match for port: %"PRIu8, dev->port_id); goto out; @@ -1250,7 +1247,7 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) sizeof(uint64_t)); /* We have to calculate number of counters */ - rte_xstats = xcalloc(rte_xstats_len, sizeof *rte_xstats); + rte_xstats = xmalloc(rte_xstats_len * sizeof *rte_xstats); memset(rte_xstats, 0xff, sizeof *rte_xstats * rte_xstats_len); /* Retreive xstats values */ @@ -1258,7 +1255,7 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) rte_xstats_len) > 0) { dev->rte_xstats_ids_size = 0; xstats_no = 0; - for (uint32_t i = 0 ; i < rte_xstats_len ; i++) { + for (uint32_t i = 0; i < rte_xstats_len; i++) { id = rte_xstats[i].id; name = netdev_dpdk_get_xstat_name(dev, id); /* We need to filter out everything except @@ -2337,7 +2334,6 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, ovs_mutex_lock(&dev->mutex); if (netdev_dpdk_configure_xstats(dev)) { - uint64_t *values = xcalloc(dev->rte_xstats_ids_size, sizeof(uint64_t)); @@ -2354,9 +2350,10 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, sizeof(struct netdev_custom_counter)); for (i = 0; i < rte_xstats_ret; i++) { - strncpy(custom_stats->counters[i].name, - netdev_dpdk_get_xstat_name(dev, dev->rte_xstats_ids[i]), - NETDEV_CUSTOM_STATS_NAME_SIZE); + ovs_strlcpy(custom_stats->counters[i].name, + netdev_dpdk_get_xstat_name(dev, + dev->rte_xstats_ids[i]), + NETDEV_CUSTOM_STATS_NAME_SIZE); custom_stats->counters[i].value = values[i]; } } else { diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 1a3322b7b87d..8a881f174a1a 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -906,7 +906,7 @@ netdev_vport_get_ifindex(const struct netdev *netdev_) NULL, /* get_carrier_resets */ \ NULL, /* get_miimon */ \ get_stats, \ - NULL, \ + NULL, /* get_custom_stats */ \ \ NULL, /* get_features */ \ NULL, /* set_advertisements */ \ diff --git a/lib/ofp-print.c b/lib/ofp-print.c index 9a5768d56866..dd320b0bc374 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -1950,10 +1950,10 @@ ofp_print_ofpst_port_reply(struct ds *string, const struct ofp_header *oh, if (ps.custom_stats.size) { ds_put_cstr(string, " CUSTOM Statistics "); - for (i = 0 ; i < ps.custom_stats.size ; i++) { + for (i = 0; i < ps.custom_stats.size; i++) { /* 3 counters in the row */ - if (strlen(ps.custom_stats.counters[i].name)) { - if ((i % 3) == 0) { + if (ps.custom_stats.counters[i].name[0]) { + if (i % 3 == 0) { ds_put_cstr(string, "\n"); ds_put_cstr(string, " "); } diff --git a/lib/ofp-util.c b/lib/ofp-util.c index 0652588d4a3c..597112e4f84e 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -8003,7 +8003,7 @@ ofputil_append_ofp14_port_stats(const struct ofputil_port_stats *ops, struct ofp14_port_stats *ps14; struct ofpbuf *reply; uint16_t i; - uint64_t counter_value; + ovs_be64 counter_value; size_t custom_stats_start, start_ofs; reply = ofpbuf_from_list(ovs_list_back(replies)); @@ -8081,21 +8081,18 @@ ofputil_append_ofp14_port_stats(const struct ofputil_port_stats *ops, htonll(ops->stats.rx_jabber_errors); if (ops->custom_stats.counters && ops->custom_stats.size) { - custom_stats_start = reply->size; uint64_t prop_type_custom = OFPPROP_EXP(INTEL_VENDOR_ID, - INTEL_PORT_STATS_CUSTOM); + INTEL_PORT_STATS_CUSTOM); stats_custom = ofpprop_put_zeros(reply, prop_type_custom, sizeof *stats_custom); stats_custom->stats_array_size = htons(ops->custom_stats.size); - memset(stats_custom->pad, 0, sizeof stats_custom->pad); - for (i = 0 ; i < ops->custom_stats.size; i++) { - uint8_t counter_size = strnlen(ops->custom_stats.counters[i].name, - NETDEV_CUSTOM_STATS_NAME_SIZE); + for (i = 0; i < ops->custom_stats.size; i++) { + uint8_t counter_size = strlen(ops->custom_stats.counters[i].name); /* Counter name size */ ofpbuf_put(reply, &counter_size, sizeof(counter_size)); /* Counter name */ @@ -8107,10 +8104,7 @@ ofputil_append_ofp14_port_stats(const struct ofputil_port_stats *ops, sizeof(ops->custom_stats.counters[i].value)); } - ofpbuf_padto(reply, ROUND_UP(reply->size, 8)); - stats_custom = ofpbuf_at_assert(reply, custom_stats_start, - sizeof *stats_custom); - stats_custom->length = htons(reply->size - custom_stats_start); + ofpprop_end(reply, custom_stats_start); } ps14 = ofpbuf_at_assert(reply, start_ofs, sizeof *ps14); @@ -8283,25 +8277,21 @@ static enum ofperr parse_intel_port_custom_property(const struct ofpbuf *payload, struct ofputil_port_stats *ops) { - uint16_t i; - const struct intel_port_custom_stats *custom_stats = payload->data; ops->custom_stats.size = ntohs(custom_stats->stats_array_size); - ops->custom_stats.counters = (struct netdev_custom_counter *) - xcalloc(ops->custom_stats.size, - sizeof(struct netdev_custom_counter)); + ops->custom_stats.counters = xcalloc(ops->custom_stats.size, + sizeof *ops->custom_stats.counters); uint16_t msg_size = ntohs(custom_stats->length); - uint16_t current_len = sizeof( *custom_stats); + uint16_t current_len = sizeof *custom_stats; uint8_t *current = (uint8_t *)payload->data + current_len; uint8_t string_size = 0; uint8_t value_size = 0; - uint64_t counter_value = 0; - - for (i = 0 ; i < ops->custom_stats.size ; i++) { + ovs_be64 counter_value = 0; + for (int i = 0; i < ops->custom_stats.size; i++) { current_len += string_size + value_size; current += string_size + value_size; @@ -8311,26 +8301,23 @@ parse_intel_port_custom_property(const struct ofpbuf *payload, /* Buffer overrun check */ if (current_len + string_size + value_size > msg_size) { - VLOG_ERR("Custom statistics buffer overrun! " - "Further message parsing is aborted."); + VLOG_WARN_RL(&bad_ofmsg_rl, "Custom statistics buffer overrun! " + "Further message parsing is aborted."); break; } current++; current_len++; - /* Counter name */ - if (string_size > sizeof(ops->custom_stats.counters[i].name)) { - VLOG_WARN("Counter name size too big! Only part " - "of the name will be copied."); - memcpy(ops->custom_stats.counters[i].name, current, - sizeof(ops->custom_stats.counters[i].name)); - } else { - memcpy(ops->custom_stats.counters[i].name, current, string_size); - } - /* Counter value */ - memcpy(&counter_value, current + string_size, - value_size); - ops->custom_stats.counters[i].value = ntohll(counter_value); + + /* Counter name. */ + struct netdev_custom_counter *c = &ops->custom_stats.counters[i]; + size_t len = MIN(string_size, sizeof c->name - 1); + memcpy(c->name, current, len); + c->name[len] = '\0'; + memcpy(&counter_value, current + string_size, value_size); + + /* Counter value. */ + c->value = ntohll(counter_value); } return 0; diff --git a/lib/util.c b/lib/util.c index 462c1fa05576..a4d22df0c359 100644 --- a/lib/util.c +++ b/lib/util.c @@ -325,7 +325,7 @@ ovs_strzcpy(char *dst, const char *src, size_t size) * Returns true if 'str' ends with given 'suffix'. */ int -string_ends_with(const char * str, const char * suffix) +string_ends_with(const char *str, const char *suffix) { int str_len = strlen(str); int suffix_len = strlen(suffix); diff --git a/lib/util.h b/lib/util.h index e75f6ce0d4d5..727c78aa542e 100644 --- a/lib/util.h +++ b/lib/util.h @@ -157,7 +157,7 @@ void free_cacheline(void *); void ovs_strlcpy(char *dst, const char *src, size_t size); void ovs_strzcpy(char *dst, const char *src, size_t size); -int string_ends_with(const char * str, const char * suffix); +int string_ends_with(const char *str, const char *suffix); /* The C standards say that neither the 'dst' nor 'src' argument to * memcpy() may be null, even if 'n' is zero. This wrapper tolerates
Hi Ben, I've just sent V3 of the patch, but please find my comments inline. > -----Original Message----- > From: Ben Pfaff [mailto:blp@ovn.org] > Sent: Wednesday, December 20, 2017 12:42 AM > To: Weglicki, MichalX <michalx.weglicki@intel.com> > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2] netdev: Custom statistics. > > On Tue, Dec 05, 2017 at 02:55:20PM +0000, Michal Weglicki wrote: > > - New get_custom_stats interface function is added to netdev. It > > allows particular netdev implementation to expose custom > > counters in dictionary format (counter name/counter value). > > - New statistics are retrieved using experimenter code and > > are printed as a result to ofctl dump-ports. > > - New counters are available for OpenFlow 1.4+. > > - New statistics are printed to output via ofctl only if those > > are present in reply message. > > - New statistics definition is added to include/openflow/intel-ext.h. > > - Custom statistics are implemented only for dpdk-physical > > port type. > > - DPDK-physical implementation uses xstats to collect statistics. > > Only dropped and error counters are exposed. > > > > v1->v2: > > - Buffer overrun check in parse_intel_port_custom_property. > > - ofputil_append_ofp14_port_stats uses "postappend" instead > > of "reserve" during message creation. > > - NEWS update. > > - DPDK documentation update. > > - Compilation and sparse warnings corrections. > > > > Signed-off-by: Michal Weglicki <michalx.weglicki@intel.com> > > Thank you for the updated patch. > > I still see some "sparse" warnings (there's an ovs-dev thread about > trouble with "sparse"--maybe you should join it): > > ../lib/ofp-util.c:8105:27: warning: incorrect type in assignment (different base types) > ../lib/ofp-util.c:8105:27: expected unsigned long long [unsigned] [usertype] counter_value > ../lib/ofp-util.c:8105:27: got restricted ovs_be64 > ../lib/ofp-util.c:8333:54: warning: incorrect type in argument 1 (different base types) > ../lib/ofp-util.c:8333:54: expected restricted ovs_be64 [usertype] <noident> > ../lib/ofp-util.c:8333:54: got unsigned long long [unsigned] [addressable] [usertype] counter_value > It is corrected. > I don't think that the new ofproto_class function port_get_custom_stats > is needed. The generic ofproto code already knows that every port is > associated with a netdev. I think that append_port_stats() in ofproto.c > can just call netdev_get_custom_stats() directly rather than through > this additional level of indirection. It is corrected. > > I don't like the idea implied in the code in a few places that name[] in > struct netdev_custom_counter might not be null-terminated. I think that > we should ensure that it is always null terminated. Otherwise there is > a pitfall for carelessly written code. To be honest I'm not sure what I could do here, each time statistics are Requested from netdev, whole buffer is set to "0", so even it Particular netdev implementation would return string which Is not null-terminated, it will be as all other characters in counter name field, will be "\0". When message is decoded from open flow buffer, proper check is done, so this part of the code is safe. If there is anything else you would like me to do, just let me know. > > I would like to see some tests. The most obvious need is a new test for > ofp-print.at that exercises parsing and printing a stats message that > includes some of these custom stats. I've added custom counters to current test case. > > I have a few other minor suggestions. I'm appending them as an > incremental patch. Many of these are minor style points. I think that > most of them will be self-explanatory. Please let me know if they make > sense. [MW] Those changes are applied. > > I'll look forward to v3. (After I'm happy with this, I'll probably ask > Ian to review it to be added to his dpdk merge branch.) > > --8<--------------------------cut here-------------------------->8-- > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 07be4a21cd2c..3ccbec11f592 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1205,7 +1205,6 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) > int xstats_no; > const char *name; > > - > /* Retrieving all XSTATS names. If something will go wrong > * or amount of counters will be equal 0, rte_xstats_names > * buffer will be marked as NULL, and any further xstats > @@ -1216,7 +1215,6 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) > rte_xstats = NULL; > > if (dev->rte_xstats_names == NULL || dev->rte_xstats_ids == NULL) { > - > dev->rte_xstats_names_size = > rte_eth_xstats_get_names(dev->port_id, NULL, 0); > > @@ -1239,8 +1237,7 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) > VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, > dev->port_id); > goto out; > - } > - else if (rte_xstats_len != dev->rte_xstats_names_size) { > + } else if (rte_xstats_len != dev->rte_xstats_names_size) { > VLOG_WARN("XSTATS size doesn't match for port: %"PRIu8, > dev->port_id); > goto out; > @@ -1250,7 +1247,7 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) > sizeof(uint64_t)); > > /* We have to calculate number of counters */ > - rte_xstats = xcalloc(rte_xstats_len, sizeof *rte_xstats); > + rte_xstats = xmalloc(rte_xstats_len * sizeof *rte_xstats); > memset(rte_xstats, 0xff, sizeof *rte_xstats * rte_xstats_len); > > /* Retreive xstats values */ > @@ -1258,7 +1255,7 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) > rte_xstats_len) > 0) { > dev->rte_xstats_ids_size = 0; > xstats_no = 0; > - for (uint32_t i = 0 ; i < rte_xstats_len ; i++) { > + for (uint32_t i = 0; i < rte_xstats_len; i++) { > id = rte_xstats[i].id; > name = netdev_dpdk_get_xstat_name(dev, id); > /* We need to filter out everything except > @@ -2337,7 +2334,6 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, > ovs_mutex_lock(&dev->mutex); > > if (netdev_dpdk_configure_xstats(dev)) { > - > uint64_t *values = xcalloc(dev->rte_xstats_ids_size, > sizeof(uint64_t)); > > @@ -2354,9 +2350,10 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, > sizeof(struct netdev_custom_counter)); > > for (i = 0; i < rte_xstats_ret; i++) { > - strncpy(custom_stats->counters[i].name, > - netdev_dpdk_get_xstat_name(dev, dev->rte_xstats_ids[i]), > - NETDEV_CUSTOM_STATS_NAME_SIZE); > + ovs_strlcpy(custom_stats->counters[i].name, > + netdev_dpdk_get_xstat_name(dev, > + dev->rte_xstats_ids[i]), > + NETDEV_CUSTOM_STATS_NAME_SIZE); > custom_stats->counters[i].value = values[i]; > } > } else { > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index 1a3322b7b87d..8a881f174a1a 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -906,7 +906,7 @@ netdev_vport_get_ifindex(const struct netdev *netdev_) > NULL, /* get_carrier_resets */ \ > NULL, /* get_miimon */ \ > get_stats, \ > - NULL, \ > + NULL, /* get_custom_stats */ \ > \ > NULL, /* get_features */ \ > NULL, /* set_advertisements */ \ > diff --git a/lib/ofp-print.c b/lib/ofp-print.c > index 9a5768d56866..dd320b0bc374 100644 > --- a/lib/ofp-print.c > +++ b/lib/ofp-print.c > @@ -1950,10 +1950,10 @@ ofp_print_ofpst_port_reply(struct ds *string, const struct ofp_header *oh, > > if (ps.custom_stats.size) { > ds_put_cstr(string, " CUSTOM Statistics "); > - for (i = 0 ; i < ps.custom_stats.size ; i++) { > + for (i = 0; i < ps.custom_stats.size; i++) { > /* 3 counters in the row */ > - if (strlen(ps.custom_stats.counters[i].name)) { > - if ((i % 3) == 0) { > + if (ps.custom_stats.counters[i].name[0]) { > + if (i % 3 == 0) { > ds_put_cstr(string, "\n"); > ds_put_cstr(string, " "); > } > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > index 0652588d4a3c..597112e4f84e 100644 > --- a/lib/ofp-util.c > +++ b/lib/ofp-util.c > @@ -8003,7 +8003,7 @@ ofputil_append_ofp14_port_stats(const struct ofputil_port_stats *ops, > struct ofp14_port_stats *ps14; > struct ofpbuf *reply; > uint16_t i; > - uint64_t counter_value; > + ovs_be64 counter_value; > size_t custom_stats_start, start_ofs; > > reply = ofpbuf_from_list(ovs_list_back(replies)); > @@ -8081,21 +8081,18 @@ ofputil_append_ofp14_port_stats(const struct ofputil_port_stats *ops, > htonll(ops->stats.rx_jabber_errors); > > if (ops->custom_stats.counters && ops->custom_stats.size) { > - > custom_stats_start = reply->size; > > uint64_t prop_type_custom = OFPPROP_EXP(INTEL_VENDOR_ID, > - INTEL_PORT_STATS_CUSTOM); > + INTEL_PORT_STATS_CUSTOM); > > stats_custom = ofpprop_put_zeros(reply, prop_type_custom, > sizeof *stats_custom); > > stats_custom->stats_array_size = htons(ops->custom_stats.size); > - memset(stats_custom->pad, 0, sizeof stats_custom->pad); > > - for (i = 0 ; i < ops->custom_stats.size; i++) { > - uint8_t counter_size = strnlen(ops->custom_stats.counters[i].name, > - NETDEV_CUSTOM_STATS_NAME_SIZE); > + for (i = 0; i < ops->custom_stats.size; i++) { > + uint8_t counter_size = strlen(ops->custom_stats.counters[i].name); > /* Counter name size */ > ofpbuf_put(reply, &counter_size, sizeof(counter_size)); > /* Counter name */ > @@ -8107,10 +8104,7 @@ ofputil_append_ofp14_port_stats(const struct ofputil_port_stats *ops, > sizeof(ops->custom_stats.counters[i].value)); > } > > - ofpbuf_padto(reply, ROUND_UP(reply->size, 8)); > - stats_custom = ofpbuf_at_assert(reply, custom_stats_start, > - sizeof *stats_custom); > - stats_custom->length = htons(reply->size - custom_stats_start); > + ofpprop_end(reply, custom_stats_start); > } > > ps14 = ofpbuf_at_assert(reply, start_ofs, sizeof *ps14); > @@ -8283,25 +8277,21 @@ static enum ofperr > parse_intel_port_custom_property(const struct ofpbuf *payload, > struct ofputil_port_stats *ops) > { > - uint16_t i; > - > const struct intel_port_custom_stats *custom_stats = payload->data; > > ops->custom_stats.size = ntohs(custom_stats->stats_array_size); > > - ops->custom_stats.counters = (struct netdev_custom_counter *) > - xcalloc(ops->custom_stats.size, > - sizeof(struct netdev_custom_counter)); > + ops->custom_stats.counters = xcalloc(ops->custom_stats.size, > + sizeof *ops->custom_stats.counters); > > uint16_t msg_size = ntohs(custom_stats->length); > - uint16_t current_len = sizeof( *custom_stats); > + uint16_t current_len = sizeof *custom_stats; > uint8_t *current = (uint8_t *)payload->data + current_len; > uint8_t string_size = 0; > uint8_t value_size = 0; > - uint64_t counter_value = 0; > - > - for (i = 0 ; i < ops->custom_stats.size ; i++) { > + ovs_be64 counter_value = 0; > > + for (int i = 0; i < ops->custom_stats.size; i++) { > current_len += string_size + value_size; > current += string_size + value_size; > > @@ -8311,26 +8301,23 @@ parse_intel_port_custom_property(const struct ofpbuf *payload, > > /* Buffer overrun check */ > if (current_len + string_size + value_size > msg_size) { > - VLOG_ERR("Custom statistics buffer overrun! " > - "Further message parsing is aborted."); > + VLOG_WARN_RL(&bad_ofmsg_rl, "Custom statistics buffer overrun! " > + "Further message parsing is aborted."); > break; > } > > current++; > current_len++; > - /* Counter name */ > - if (string_size > sizeof(ops->custom_stats.counters[i].name)) { > - VLOG_WARN("Counter name size too big! Only part " > - "of the name will be copied."); > - memcpy(ops->custom_stats.counters[i].name, current, > - sizeof(ops->custom_stats.counters[i].name)); > - } else { > - memcpy(ops->custom_stats.counters[i].name, current, string_size); > - } > - /* Counter value */ > - memcpy(&counter_value, current + string_size, > - value_size); > - ops->custom_stats.counters[i].value = ntohll(counter_value); > + > + /* Counter name. */ > + struct netdev_custom_counter *c = &ops->custom_stats.counters[i]; > + size_t len = MIN(string_size, sizeof c->name - 1); > + memcpy(c->name, current, len); > + c->name[len] = '\0'; > + memcpy(&counter_value, current + string_size, value_size); > + > + /* Counter value. */ > + c->value = ntohll(counter_value); > } > > return 0; > diff --git a/lib/util.c b/lib/util.c > index 462c1fa05576..a4d22df0c359 100644 > --- a/lib/util.c > +++ b/lib/util.c > @@ -325,7 +325,7 @@ ovs_strzcpy(char *dst, const char *src, size_t size) > * Returns true if 'str' ends with given 'suffix'. > */ > int > -string_ends_with(const char * str, const char * suffix) > +string_ends_with(const char *str, const char *suffix) > { > int str_len = strlen(str); > int suffix_len = strlen(suffix); > diff --git a/lib/util.h b/lib/util.h > index e75f6ce0d4d5..727c78aa542e 100644 > --- a/lib/util.h > +++ b/lib/util.h > @@ -157,7 +157,7 @@ void free_cacheline(void *); > void ovs_strlcpy(char *dst, const char *src, size_t size); > void ovs_strzcpy(char *dst, const char *src, size_t size); > > -int string_ends_with(const char * str, const char * suffix); > +int string_ends_with(const char *str, const char *suffix); > > /* The C standards say that neither the 'dst' nor 'src' argument to > * memcpy() may be null, even if 'n' is zero. This wrapper tolerates
On Mon, Jan 08, 2018 at 11:55:26AM +0000, Weglicki, MichalX wrote: > Hi Ben, > > I don't like the idea implied in the code in a few places that name[] in > > struct netdev_custom_counter might not be null-terminated. I think that > > we should ensure that it is always null terminated. Otherwise there is > > a pitfall for carelessly written code. > To be honest I'm not sure what I could do here, each time statistics are > Requested from netdev, whole buffer is set to "0", so even it > Particular netdev implementation would return string which > Is not null-terminated, it will be as all other characters in > counter name field, will be "\0". When message is decoded from > open flow buffer, proper check is done, so this part of the code is safe. > If there is anything else you would like me to do, just let me know. Maybe I misinterpreted some code. I'll take another look. Thanks for v3!
On 12/19/2017 03:00 PM, Kevin Traynor wrote: > On 12/19/2017 02:35 PM, Weglicki, MichalX wrote: >>> -----Original Message----- >>> From: Kevin Traynor [mailto:ktraynor@redhat.com] >>> Sent: Tuesday, December 19, 2017 3:07 PM >>> To: Weglicki, MichalX <michalx.weglicki@intel.com>; dev@openvswitch.org >>> Subject: Re: [ovs-dev] [PATCH v2] netdev: Custom statistics. >>> >>> On 12/05/2017 02:55 PM, Michal Weglicki wrote: >>>> - New get_custom_stats interface function is added to netdev. It >>>> allows particular netdev implementation to expose custom >>>> counters in dictionary format (counter name/counter value). >>>> - New statistics are retrieved using experimenter code and >>>> are printed as a result to ofctl dump-ports. >>>> - New counters are available for OpenFlow 1.4+. >>>> - New statistics are printed to output via ofctl only if those >>>> are present in reply message. >>>> - New statistics definition is added to include/openflow/intel-ext.h. >>>> - Custom statistics are implemented only for dpdk-physical >>>> port type. >>>> - DPDK-physical implementation uses xstats to collect statistics. >>>> Only dropped and error counters are exposed. >>>> >>> Hi Michal - why only dropped and error counters? why not just expose >>> them all. For example, IIUC this would report management dropped packets >>> but there would not be a stat for management rx/tx successful packets. >>> >>> Kevin. >> Hi Kevin - those counters were of biggest value to us at the point of making this >> patch, sending all counters (where for IXGBE is about 150) will produce >> some movement on the network. I think that biggest advantage of this >> particular patch is that it introduces a mechanism to expose >> any counters, counters list can be extended in the future >> if necessary. However I'm not sure if sending all counters >> is good idea, as there could be thousands of it in the future - in this >> solution, we have some kind of control over the data size. >> > Ok thanks, that makes sense. I would like to suggest that *_management_* > be added as part of this as I think it's only 2 additional stats and > I've seen at least one user saying they needed this information. > Hi Michal, this does not look to be in v3, do you think it should be added as a separate patch? or they should not be reported? thanks, Kevin.
On Mon, Jan 08, 2018 at 08:28:45AM -0800, Ben Pfaff wrote: > On Mon, Jan 08, 2018 at 11:55:26AM +0000, Weglicki, MichalX wrote: > > Hi Ben, > > > I don't like the idea implied in the code in a few places that name[] in > > > struct netdev_custom_counter might not be null-terminated. I think that > > > we should ensure that it is always null terminated. Otherwise there is > > > a pitfall for carelessly written code. > > To be honest I'm not sure what I could do here, each time statistics are > > Requested from netdev, whole buffer is set to "0", so even it > > Particular netdev implementation would return string which > > Is not null-terminated, it will be as all other characters in > > counter name field, will be "\0". When message is decoded from > > open flow buffer, proper check is done, so this part of the code is safe. > > If there is anything else you would like me to do, just let me know. > > Maybe I misinterpreted some code. I'll take another look. > > Thanks for v3! Actually, could you respond to Kevin Traynor first? https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/342747.html
> -----Original Message----- > From: Kevin Traynor [mailto:ktraynor@redhat.com] > Sent: Monday, January 8, 2018 7:08 PM > To: Weglicki, MichalX <michalx.weglicki@intel.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2] netdev: Custom statistics. > > On 12/19/2017 03:00 PM, Kevin Traynor wrote: > > On 12/19/2017 02:35 PM, Weglicki, MichalX wrote: > >>> -----Original Message----- > >>> From: Kevin Traynor [mailto:ktraynor@redhat.com] > >>> Sent: Tuesday, December 19, 2017 3:07 PM > >>> To: Weglicki, MichalX <michalx.weglicki@intel.com>; dev@openvswitch.org > >>> Subject: Re: [ovs-dev] [PATCH v2] netdev: Custom statistics. > >>> > >>> On 12/05/2017 02:55 PM, Michal Weglicki wrote: > >>>> - New get_custom_stats interface function is added to netdev. It > >>>> allows particular netdev implementation to expose custom > >>>> counters in dictionary format (counter name/counter value). > >>>> - New statistics are retrieved using experimenter code and > >>>> are printed as a result to ofctl dump-ports. > >>>> - New counters are available for OpenFlow 1.4+. > >>>> - New statistics are printed to output via ofctl only if those > >>>> are present in reply message. > >>>> - New statistics definition is added to include/openflow/intel-ext.h. > >>>> - Custom statistics are implemented only for dpdk-physical > >>>> port type. > >>>> - DPDK-physical implementation uses xstats to collect statistics. > >>>> Only dropped and error counters are exposed. > >>>> > >>> Hi Michal - why only dropped and error counters? why not just expose > >>> them all. For example, IIUC this would report management dropped packets > >>> but there would not be a stat for management rx/tx successful packets. > >>> > >>> Kevin. > >> Hi Kevin - those counters were of biggest value to us at the point of making this > >> patch, sending all counters (where for IXGBE is about 150) will produce > >> some movement on the network. I think that biggest advantage of this > >> particular patch is that it introduces a mechanism to expose > >> any counters, counters list can be extended in the future > >> if necessary. However I'm not sure if sending all counters > >> is good idea, as there could be thousands of it in the future - in this > >> solution, we have some kind of control over the data size. > >> > > Ok thanks, that makes sense. I would like to suggest that *_management_* > > be added as part of this as I think it's only 2 additional stats and > > I've seen at least one user saying they needed this information. > > > > Hi Michal, this does not look to be in v3, do you think it should be > added as a separate patch? or they should not be reported? So sorry Kevin, I've simply forgot to add it, I've just sent v4 where those counters are included. > > thanks, > Kevin. > >
On 01/09/2018 08:13 AM, Weglicki, MichalX wrote: > > >> -----Original Message----- >> From: Kevin Traynor [mailto:ktraynor@redhat.com] >> Sent: Monday, January 8, 2018 7:08 PM >> To: Weglicki, MichalX <michalx.weglicki@intel.com>; dev@openvswitch.org >> Subject: Re: [ovs-dev] [PATCH v2] netdev: Custom statistics. >> >> On 12/19/2017 03:00 PM, Kevin Traynor wrote: >>> On 12/19/2017 02:35 PM, Weglicki, MichalX wrote: >>>>> -----Original Message----- >>>>> From: Kevin Traynor [mailto:ktraynor@redhat.com] >>>>> Sent: Tuesday, December 19, 2017 3:07 PM >>>>> To: Weglicki, MichalX <michalx.weglicki@intel.com>; dev@openvswitch.org >>>>> Subject: Re: [ovs-dev] [PATCH v2] netdev: Custom statistics. >>>>> >>>>> On 12/05/2017 02:55 PM, Michal Weglicki wrote: >>>>>> - New get_custom_stats interface function is added to netdev. It >>>>>> allows particular netdev implementation to expose custom >>>>>> counters in dictionary format (counter name/counter value). >>>>>> - New statistics are retrieved using experimenter code and >>>>>> are printed as a result to ofctl dump-ports. >>>>>> - New counters are available for OpenFlow 1.4+. >>>>>> - New statistics are printed to output via ofctl only if those >>>>>> are present in reply message. >>>>>> - New statistics definition is added to include/openflow/intel-ext.h. >>>>>> - Custom statistics are implemented only for dpdk-physical >>>>>> port type. >>>>>> - DPDK-physical implementation uses xstats to collect statistics. >>>>>> Only dropped and error counters are exposed. >>>>>> >>>>> Hi Michal - why only dropped and error counters? why not just expose >>>>> them all. For example, IIUC this would report management dropped packets >>>>> but there would not be a stat for management rx/tx successful packets. >>>>> >>>>> Kevin. >>>> Hi Kevin - those counters were of biggest value to us at the point of making this >>>> patch, sending all counters (where for IXGBE is about 150) will produce >>>> some movement on the network. I think that biggest advantage of this >>>> particular patch is that it introduces a mechanism to expose >>>> any counters, counters list can be extended in the future >>>> if necessary. However I'm not sure if sending all counters >>>> is good idea, as there could be thousands of it in the future - in this >>>> solution, we have some kind of control over the data size. >>>> >>> Ok thanks, that makes sense. I would like to suggest that *_management_* >>> be added as part of this as I think it's only 2 additional stats and >>> I've seen at least one user saying they needed this information. >>> >> >> Hi Michal, this does not look to be in v3, do you think it should be >> added as a separate patch? or they should not be reported? > So sorry Kevin, I've simply forgot to add it, I've just sent v4 > where those counters are included. > Great, I thought maybe you preferred it to be a separate patch but it's a very minor addition, so as easy to add in now. Thanks! Kevin. >> >> thanks, >> Kevin. >> >> >
diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst index d123819..c99ec29 100644 --- a/Documentation/howto/dpdk.rst +++ b/Documentation/howto/dpdk.rst @@ -311,12 +311,16 @@ performance of non-tunnel traffic, specifically for smaller size packet. .. _extended-statistics: -Extended Statistics -------------------- +Extended & Custom Statistics +---------------------------- DPDK Extended Statistics API allows PMD to expose unique set of statistics. The Extended statistics are implemented and supported only for DPDK physical -and vHost ports. +and vHost ports. Custom statistics are dynamic set of counters which can +vary depenend on a driver. Those statistics are implemented +for DPDK physical ports and contain all "dropped" and "error" +counters from XSTATS. XSTATS counters list can be found here: +<https://wiki.opnfv.org/display/fastpath/Collectd+Metrics+and+Events>`__. To enable statistics, you have to enable OpenFlow 1.4 support for OVS. Configure bridge br0 to support OpenFlow version 1.4:: @@ -333,8 +337,9 @@ Query the port statistics by explicitly specifying -O OpenFlow14 option:: $ ovs-ofctl -O OpenFlow14 dump-ports br0 -Note: vHost ports supports only partial statistics. RX packet size based -counter are only supported and doesn't include TX packet size counters. +Note about "Extended Statistics": vHost ports supports only partial +statistics. RX packet size based counter are only supported and +doesn't include TX packet size counters. .. _port-hotplug: diff --git a/NEWS b/NEWS index 427c8f8..727142c 100644 --- a/NEWS +++ b/NEWS @@ -13,6 +13,12 @@ Post-v2.8.0 * ovn-ctl: New commands run_nb_ovsdb and run_sb_ovsdb. - Linux kernel 4.13 * Add support for compiling OVS with the latest Linux 4.13 kernel + - DPDK: + * Custom statistics: + - DPDK physical ports now return custom set of "dropped" and "error" + statistics. + - ovs-ofctl dump-ports command now prints new of set custom statistics + if available (for OpenFlow 1.4+). v2.8.0 - 31 Aug 2017 -------------------- diff --git a/include/openflow/intel-ext.h b/include/openflow/intel-ext.h index 974e63e..3d73171 100644 --- a/include/openflow/intel-ext.h +++ b/include/openflow/intel-ext.h @@ -27,9 +27,11 @@ enum intel_port_stats_subtype { INTEL_PORT_STATS_RFC2819 = 1, + INTEL_PORT_STATS_CUSTOM }; #define INTEL_PORT_STATS_RFC2819_SIZE 184 +#define INTEL_PORT_STATS_CUSTOM_SIZE 16 /* Struct implements custom property type based on * 'ofp_prop_experimenter'. */ @@ -70,4 +72,30 @@ struct intel_port_stats_rfc2819 { OFP_ASSERT(sizeof (struct intel_port_stats_rfc2819) == INTEL_PORT_STATS_RFC2819_SIZE); +/* Structure implements custom property type based on + * 'ofp_prop_experimenter'. It contains custom + * statistics in dictionary format */ +struct intel_port_custom_stats { + ovs_be16 type; /* OFPPSPT14_EXPERIMENTER. */ + ovs_be16 length; /* Length in bytes of this property excluding + * trailing padding. */ + ovs_be32 experimenter; /* INTEL_VENDOR_ID. */ + ovs_be32 exp_type; /* INTEL_PORT_STATS_*. */ + + uint8_t pad[2]; + ovs_be16 stats_array_size; /* number of counters. */ + + /* Followed by: + * - Exactly 'stats_array_size' array elements of + * dynamic structure which contains: + * - "NAME SIZE" - counter name size (number of characters) + * - "COUNTER NAME" - Exact number of characters + * defined by "NAME SIZE". + * - "COUNTER VALUE" - ovs_be64 counter value, + * - Zero or more bytes to fill out the + * overall length in header.length. */ +}; +OFP_ASSERT(sizeof(struct intel_port_custom_stats) == + INTEL_PORT_STATS_CUSTOM_SIZE); + #endif /* openflow/intel-ext.h */ diff --git a/include/openvswitch/netdev.h b/include/openvswitch/netdev.h index 50bafc3..e25c241 100644 --- a/include/openvswitch/netdev.h +++ b/include/openvswitch/netdev.h @@ -27,6 +27,9 @@ extern "C" { struct netdev; +/* Maximum name length for custom statistics counters */ +#define NETDEV_CUSTOM_STATS_NAME_SIZE 64 + /* Network device statistics. * * Values of unsupported statistics are set to all-1-bits (UINT64_MAX). */ @@ -85,6 +88,18 @@ struct netdev_stats { uint64_t rx_jabber_errors; }; +/* Structure representation of custom statistics counter */ +struct netdev_custom_counter { + uint64_t value; + char name[NETDEV_CUSTOM_STATS_NAME_SIZE]; +}; + +/* Structure representation of custom statistics */ +struct netdev_custom_stats { + uint16_t size; + struct netdev_custom_counter *counters; +}; + /* Features. */ enum netdev_features { NETDEV_F_10MB_HD = 1 << 0, /* 10 Mb half-duplex rate support. */ @@ -115,6 +130,8 @@ uint64_t netdev_features_to_bps(enum netdev_features features, bool netdev_features_is_full_duplex(enum netdev_features features); int netdev_set_advertisements(struct netdev *, enum netdev_features advertise); +void netdev_free_custom_stats_counters(struct netdev_custom_stats *); + #ifdef __cplusplus } #endif diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h index a9e57ed..296078a 100644 --- a/include/openvswitch/ofp-util.h +++ b/include/openvswitch/ofp-util.h @@ -1174,6 +1174,7 @@ bool ofputil_parse_key_value(char **stringp, char **keyp, char **valuep); struct ofputil_port_stats { ofp_port_t port_no; struct netdev_stats stats; + struct netdev_custom_stats custom_stats; uint32_t duration_sec; /* UINT32_MAX if unknown. */ uint32_t duration_nsec; }; diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index faff842..8acc5e0 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -415,6 +415,14 @@ struct netdev_dpdk { * from the enum set 'dpdk_hw_ol_features' */ uint32_t hw_ol_features; ); + + PADDED_MEMBERS(CACHE_LINE_SIZE, + /* Names of all XSTATS counters */ + struct rte_eth_xstat_name *rte_xstats_names; + int rte_xstats_names_size; + int rte_xstats_ids_size; + uint64_t *rte_xstats_ids; + ); }; struct netdev_rxq_dpdk { @@ -897,6 +905,12 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, netdev_request_reconfigure(netdev); + dev->rte_xstats_names = NULL; + dev->rte_xstats_names_size = 0; + + dev->rte_xstats_ids = NULL; + dev->rte_xstats_ids_size = 0; + return 0; } @@ -1135,6 +1149,128 @@ netdev_dpdk_dealloc(struct netdev *netdev) rte_free(dev); } +static void +netdev_dpdk_clear_xstats(struct netdev_dpdk *dev) OVS_REQUIRES(dev->mutex) +{ + /* If statistics are already allocated, we have to + * reconfigure, as port_id could have been changed. */ + if (dev->rte_xstats_names) { + free(dev->rte_xstats_names); + dev->rte_xstats_names = NULL; + dev->rte_xstats_names_size = 0; + } + if (dev->rte_xstats_ids) { + free(dev->rte_xstats_ids); + dev->rte_xstats_ids = NULL; + dev->rte_xstats_ids_size = 0; + } +} + +static const char* +netdev_dpdk_get_xstat_name(struct netdev_dpdk *dev, uint64_t id) +{ + if (id >= dev->rte_xstats_names_size) { + return "UNKNOWN"; + } + return dev->rte_xstats_names[id].name; +} + +static bool +netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) + OVS_REQUIRES(dev->mutex) +{ + int rte_xstats_len; + bool ret; + struct rte_eth_xstat *rte_xstats; + uint64_t id; + int xstats_no; + const char *name; + + + /* Retrieving all XSTATS names. If something will go wrong + * or amount of counters will be equal 0, rte_xstats_names + * buffer will be marked as NULL, and any further xstats + * query won't be performed (e.g. during netdev_dpdk_get_stats + * execution). */ + + ret = false; + rte_xstats = NULL; + + if (dev->rte_xstats_names == NULL || dev->rte_xstats_ids == NULL) { + + dev->rte_xstats_names_size = + rte_eth_xstats_get_names(dev->port_id, NULL, 0); + + if (dev->rte_xstats_names_size < 0) { + VLOG_WARN("Cannot get XSTATS for port: %"PRIu8, dev->port_id); + dev->rte_xstats_names_size = 0; + } else { + /* Reserve memory for xstats names and values */ + dev->rte_xstats_names = xcalloc(dev->rte_xstats_names_size, + sizeof *dev->rte_xstats_names); + + if (dev->rte_xstats_names) { + /* Retreive xstats names */ + rte_xstats_len = + rte_eth_xstats_get_names(dev->port_id, + dev->rte_xstats_names, + dev->rte_xstats_names_size); + + if (rte_xstats_len < 0) { + VLOG_WARN("Cannot get XSTATS names for port: %"PRIu8, + dev->port_id); + goto out; + } + else if (rte_xstats_len != dev->rte_xstats_names_size) { + VLOG_WARN("XSTATS size doesn't match for port: %"PRIu8, + dev->port_id); + goto out; + } + + dev->rte_xstats_ids = xcalloc(dev->rte_xstats_names_size, + sizeof(uint64_t)); + + /* We have to calculate number of counters */ + rte_xstats = xcalloc(rte_xstats_len, sizeof *rte_xstats); + memset(rte_xstats, 0xff, sizeof *rte_xstats * rte_xstats_len); + + /* Retreive xstats values */ + if (rte_eth_xstats_get(dev->port_id, rte_xstats, + rte_xstats_len) > 0) { + dev->rte_xstats_ids_size = 0; + xstats_no = 0; + for (uint32_t i = 0 ; i < rte_xstats_len ; i++) { + id = rte_xstats[i].id; + name = netdev_dpdk_get_xstat_name(dev, id); + /* We need to filter out everything except + * dropped and error counters */ + if (string_ends_with(name, "_errors") || + string_ends_with(name, "_dropped")) { + + dev->rte_xstats_ids[xstats_no] = id; + xstats_no++; + } + } + dev->rte_xstats_ids_size = xstats_no; + ret = true; + } else { + VLOG_WARN("Can't get XSTATS IDs for port: %"PRIu8, + dev->port_id); + } + } + } + } else { + /* Already configured */ + ret = true; + } + +out: + if (!ret) { + netdev_dpdk_clear_xstats(dev); + } + return ret; +} + static int netdev_dpdk_get_config(const struct netdev *netdev, struct smap *args) { @@ -1307,6 +1443,7 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, dev->devargs = xstrdup(new_devargs); dev->port_id = new_port_id; netdev_request_reconfigure(&dev->up); + netdev_dpdk_clear_xstats(dev); err = 0; } } @@ -2171,6 +2308,56 @@ out: } static int +netdev_dpdk_get_custom_stats(const struct netdev *netdev, + struct netdev_custom_stats *custom_stats) +{ + + uint32_t i; + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); + int rte_xstats_ret; + + ovs_mutex_lock(&dev->mutex); + + if (netdev_dpdk_configure_xstats(dev)) { + + uint64_t *values = xcalloc(dev->rte_xstats_ids_size, + sizeof(uint64_t)); + + rte_xstats_ret = + rte_eth_xstats_get_by_id(dev->port_id, dev->rte_xstats_ids, + values, dev->rte_xstats_ids_size); + + if (rte_xstats_ret > 0 && + rte_xstats_ret <= dev->rte_xstats_ids_size) { + + custom_stats->size = rte_xstats_ret; + custom_stats->counters = + (struct netdev_custom_counter *) xcalloc(rte_xstats_ret, + sizeof(struct netdev_custom_counter)); + + for (i = 0; i < rte_xstats_ret; i++) { + strncpy(custom_stats->counters[i].name, + netdev_dpdk_get_xstat_name(dev, dev->rte_xstats_ids[i]), + NETDEV_CUSTOM_STATS_NAME_SIZE); + custom_stats->counters[i].value = values[i]; + } + } else { + VLOG_WARN("Cannot get XSTATS values for port: %"PRIu8, + dev->port_id); + custom_stats->counters = NULL; + custom_stats->size = 0; + /* Let's clear statistics cache, so it will be + * reconfigured */ + netdev_dpdk_clear_xstats(dev); + } + } + + ovs_mutex_unlock(&dev->mutex); + + return 0; +} + +static int netdev_dpdk_get_features(const struct netdev *netdev, enum netdev_features *current, enum netdev_features *advertised, @@ -3313,7 +3500,8 @@ unlock: #define NETDEV_DPDK_CLASS(NAME, INIT, CONSTRUCT, DESTRUCT, \ SET_CONFIG, SET_TX_MULTIQ, SEND, \ - GET_CARRIER, GET_STATS, \ + GET_CARRIER, GET_STATS, \ + GET_CUSTOM_STATS, \ GET_FEATURES, GET_STATUS, \ RECONFIGURE, RXQ_RECV) \ { \ @@ -3348,6 +3536,7 @@ unlock: netdev_dpdk_get_carrier_resets, \ netdev_dpdk_set_miimon, \ GET_STATS, \ + GET_CUSTOM_STATS, \ GET_FEATURES, \ NULL, /* set_advertisements */ \ NULL, /* get_pt_mode */ \ @@ -3397,6 +3586,7 @@ static const struct netdev_class dpdk_class = netdev_dpdk_eth_send, netdev_dpdk_get_carrier, netdev_dpdk_get_stats, + netdev_dpdk_get_custom_stats, netdev_dpdk_get_features, netdev_dpdk_get_status, netdev_dpdk_reconfigure, @@ -3413,6 +3603,7 @@ static const struct netdev_class dpdk_ring_class = netdev_dpdk_ring_send, netdev_dpdk_get_carrier, netdev_dpdk_get_stats, + netdev_dpdk_get_custom_stats, netdev_dpdk_get_features, netdev_dpdk_get_status, netdev_dpdk_reconfigure, @@ -3431,6 +3622,7 @@ static const struct netdev_class dpdk_vhost_class = netdev_dpdk_vhost_get_stats, NULL, NULL, + NULL, netdev_dpdk_vhost_reconfigure, netdev_dpdk_vhost_rxq_recv); static const struct netdev_class dpdk_vhost_client_class = @@ -3446,6 +3638,7 @@ static const struct netdev_class dpdk_vhost_client_class = netdev_dpdk_vhost_get_stats, NULL, NULL, + NULL, netdev_dpdk_vhost_client_reconfigure, netdev_dpdk_vhost_rxq_recv); diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 246cdf1..1731b77 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -1383,6 +1383,7 @@ netdev_dummy_update_flags(struct netdev *netdev_, NULL, /* get_carrier_resets */ \ NULL, /* get_miimon */ \ netdev_dummy_get_stats, \ + NULL, \ \ NULL, /* get_features */ \ NULL, /* set_advertisements */ \ diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index e809b88..4e9be30 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2853,6 +2853,7 @@ netdev_linux_update_flags(struct netdev *netdev_, enum netdev_flags off, netdev_linux_get_carrier_resets, \ netdev_linux_set_miimon_interval, \ GET_STATS, \ + NULL, \ \ GET_FEATURES, \ netdev_linux_set_advertisements, \ diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h index 1720deb..d66fd5b 100644 --- a/lib/netdev-provider.h +++ b/lib/netdev-provider.h @@ -459,6 +459,19 @@ struct netdev_class { * (UINT64_MAX). */ int (*get_stats)(const struct netdev *netdev, struct netdev_stats *); + /* Retrieves current device custom stats for 'netdev' into 'custom_stats'. + * + * A network device should return only available statistics (if any). + * If there are not statistics available, empty array should be + * returned. + * + * The caller initializes 'custom_stats' before calling this function. + * The caller takes ownership over allocated array of counters inside + * structure netdev_custom_stats. + * */ + int (*get_custom_stats)(const struct netdev *netdev, + struct netdev_custom_stats *custom_stats); + /* Stores the features supported by 'netdev' into each of '*current', * '*advertised', '*supported', and '*peer'. Each value is a bitmap of * NETDEV_F_* bits. diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 518058a..1a3322b 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -906,6 +906,7 @@ netdev_vport_get_ifindex(const struct netdev *netdev_) NULL, /* get_carrier_resets */ \ NULL, /* get_miimon */ \ get_stats, \ + NULL, \ \ NULL, /* get_features */ \ NULL, /* set_advertisements */ \ diff --git a/lib/netdev.c b/lib/netdev.c index 2d69fe5..cd11930 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -1425,6 +1425,21 @@ netdev_get_stats(const struct netdev *netdev, struct netdev_stats *stats) return error; } +/* Retrieves current device custom stats for 'netdev'. */ +int +netdev_get_custom_stats(const struct netdev *netdev, + struct netdev_custom_stats *custom_stats) +{ + int error; + memset(custom_stats, 0, sizeof *custom_stats); + error = (netdev->netdev_class->get_custom_stats + ? netdev->netdev_class->get_custom_stats(netdev, custom_stats) + : EOPNOTSUPP); + + return error; +} + + /* Attempts to set input rate limiting (policing) policy, such that up to * 'kbits_rate' kbps of traffic is accepted, with a maximum accumulative burst * size of 'kbits' kb. */ @@ -2380,6 +2395,18 @@ netdev_ports_flow_get(const struct dpif_class *dpif_class, struct match *match, return ENOENT; } +void +netdev_free_custom_stats_counters(struct netdev_custom_stats *custom_stats) +{ + if (custom_stats) { + if (custom_stats->counters) { + free(custom_stats->counters); + custom_stats->counters = NULL; + custom_stats->size = 0; + } + } +} + #ifdef __linux__ static void netdev_ports_flow_init(void) diff --git a/lib/netdev.h b/lib/netdev.h index 3a545fe..34df7c9 100644 --- a/lib/netdev.h +++ b/lib/netdev.h @@ -296,6 +296,8 @@ struct netdev *netdev_find_dev_by_in4(const struct in_addr *); /* Statistics. */ int netdev_get_stats(const struct netdev *, struct netdev_stats *); +int netdev_get_custom_stats(const struct netdev *, + struct netdev_custom_stats *); /* Quality of service. */ struct netdev_qos_capabilities { diff --git a/lib/ofp-print.c b/lib/ofp-print.c index 151d618..9a5768d 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -1834,6 +1834,7 @@ ofp_print_ofpst_port_reply(struct ds *string, const struct ofp_header *oh, const struct ofputil_port_map *port_map, int verbosity) { + uint32_t i; ds_put_format(string, " %"PRIuSIZE" ports\n", ofputil_count_port_stats(oh)); if (verbosity < 1) { return; @@ -1946,6 +1947,23 @@ ofp_print_ofpst_port_reply(struct ds *string, const struct ofp_header *oh, ds_put_cstr(string, "\n"); ds_destroy(&string_ext_stats); } + + if (ps.custom_stats.size) { + ds_put_cstr(string, " CUSTOM Statistics "); + for (i = 0 ; i < ps.custom_stats.size ; i++) { + /* 3 counters in the row */ + if (strlen(ps.custom_stats.counters[i].name)) { + if ((i % 3) == 0) { + ds_put_cstr(string, "\n"); + ds_put_cstr(string, " "); + } + ds_put_format(string, "%s=%"PRIu64", ", + ps.custom_stats.counters[i].name, + ps.custom_stats.counters[i].value); + } + } + ds_put_cstr(string, "\n"); + } } } diff --git a/lib/ofp-util.c b/lib/ofp-util.c index 47f30c7..0652588 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -7999,15 +7999,18 @@ ofputil_append_ofp14_port_stats(const struct ofputil_port_stats *ops, { struct ofp14_port_stats_prop_ethernet *eth; struct intel_port_stats_rfc2819 *stats_rfc2819; + struct intel_port_custom_stats *stats_custom; struct ofp14_port_stats *ps14; struct ofpbuf *reply; + uint16_t i; + uint64_t counter_value; + size_t custom_stats_start, start_ofs; - reply = ofpmp_reserve(replies, sizeof *ps14 + sizeof *eth + - sizeof *stats_rfc2819); + reply = ofpbuf_from_list(ovs_list_back(replies)); + start_ofs = reply->size; ps14 = ofpbuf_put_uninit(reply, sizeof *ps14); - ps14->length = htons(sizeof *ps14 + sizeof *eth + - sizeof *stats_rfc2819); + memset(ps14->pad, 0, sizeof ps14->pad); ps14->port_no = ofputil_port_to_ofp11(ops->port_no); ps14->duration_sec = htonl(ops->duration_sec); @@ -8027,10 +8030,10 @@ ofputil_append_ofp14_port_stats(const struct ofputil_port_stats *ops, eth->rx_crc_err = htonll(ops->stats.rx_crc_errors); eth->collisions = htonll(ops->stats.collisions); - uint64_t prop_type = OFPPROP_EXP(INTEL_VENDOR_ID, + uint64_t prop_type_stats = OFPPROP_EXP(INTEL_VENDOR_ID, INTEL_PORT_STATS_RFC2819); - stats_rfc2819 = ofpprop_put_zeros(reply, prop_type, + stats_rfc2819 = ofpprop_put_zeros(reply, prop_type_stats, sizeof *stats_rfc2819); memset(stats_rfc2819->pad, 0, sizeof stats_rfc2819->pad); @@ -8076,6 +8079,44 @@ ofputil_append_ofp14_port_stats(const struct ofputil_port_stats *ops, htonll(ops->stats.rx_fragmented_errors); stats_rfc2819->rx_jabber_errors = htonll(ops->stats.rx_jabber_errors); + + if (ops->custom_stats.counters && ops->custom_stats.size) { + + custom_stats_start = reply->size; + + uint64_t prop_type_custom = OFPPROP_EXP(INTEL_VENDOR_ID, + INTEL_PORT_STATS_CUSTOM); + + stats_custom = ofpprop_put_zeros(reply, prop_type_custom, + sizeof *stats_custom); + + stats_custom->stats_array_size = htons(ops->custom_stats.size); + memset(stats_custom->pad, 0, sizeof stats_custom->pad); + + for (i = 0 ; i < ops->custom_stats.size; i++) { + uint8_t counter_size = strnlen(ops->custom_stats.counters[i].name, + NETDEV_CUSTOM_STATS_NAME_SIZE); + /* Counter name size */ + ofpbuf_put(reply, &counter_size, sizeof(counter_size)); + /* Counter name */ + ofpbuf_put(reply, ops->custom_stats.counters[i].name, + counter_size); + /* Counter value */ + counter_value = htonll(ops->custom_stats.counters[i].value); + ofpbuf_put(reply, &counter_value, + sizeof(ops->custom_stats.counters[i].value)); + } + + ofpbuf_padto(reply, ROUND_UP(reply->size, 8)); + stats_custom = ofpbuf_at_assert(reply, custom_stats_start, + sizeof *stats_custom); + stats_custom->length = htons(reply->size - custom_stats_start); + } + + ps14 = ofpbuf_at_assert(reply, start_ofs, sizeof *ps14); + ps14->length = htons(reply->size - start_ofs); + + ofpmp_postappend(replies, start_ofs); } /* Encode a ports stat for 'ops' and append it to 'replies'. */ @@ -8239,6 +8280,63 @@ parse_intel_port_stats_rfc2819_property(const struct ofpbuf *payload, } static enum ofperr +parse_intel_port_custom_property(const struct ofpbuf *payload, + struct ofputil_port_stats *ops) +{ + uint16_t i; + + const struct intel_port_custom_stats *custom_stats = payload->data; + + ops->custom_stats.size = ntohs(custom_stats->stats_array_size); + + ops->custom_stats.counters = (struct netdev_custom_counter *) + xcalloc(ops->custom_stats.size, + sizeof(struct netdev_custom_counter)); + + uint16_t msg_size = ntohs(custom_stats->length); + uint16_t current_len = sizeof( *custom_stats); + uint8_t *current = (uint8_t *)payload->data + current_len; + uint8_t string_size = 0; + uint8_t value_size = 0; + uint64_t counter_value = 0; + + for (i = 0 ; i < ops->custom_stats.size ; i++) { + + current_len += string_size + value_size; + current += string_size + value_size; + + value_size = sizeof(uint64_t); + /* Counter name size */ + string_size = *current; + + /* Buffer overrun check */ + if (current_len + string_size + value_size > msg_size) { + VLOG_ERR("Custom statistics buffer overrun! " + "Further message parsing is aborted."); + break; + } + + current++; + current_len++; + /* Counter name */ + if (string_size > sizeof(ops->custom_stats.counters[i].name)) { + VLOG_WARN("Counter name size too big! Only part " + "of the name will be copied."); + memcpy(ops->custom_stats.counters[i].name, current, + sizeof(ops->custom_stats.counters[i].name)); + } else { + memcpy(ops->custom_stats.counters[i].name, current, string_size); + } + /* Counter value */ + memcpy(&counter_value, current + string_size, + value_size); + ops->custom_stats.counters[i].value = ntohll(counter_value); + } + + return 0; +} + +static enum ofperr parse_intel_port_stats_property(const struct ofpbuf *payload, uint32_t exp_type, struct ofputil_port_stats *ops) @@ -8249,6 +8347,9 @@ parse_intel_port_stats_property(const struct ofpbuf *payload, case INTEL_PORT_STATS_RFC2819: error = parse_intel_port_stats_rfc2819_property(payload, ops); break; + case INTEL_PORT_STATS_CUSTOM: + error = parse_intel_port_custom_property(payload, ops); + break; default: error = OFPERR_OFPBPC_BAD_EXP_TYPE; break; @@ -8308,6 +8409,11 @@ ofputil_pull_ofp14_port_stats(struct ofputil_port_stats *ops, INTEL_PORT_STATS_RFC2819, ops); break; + case OFPPROP_EXP(INTEL_VENDOR_ID, INTEL_PORT_STATS_CUSTOM): + error = parse_intel_port_stats_property(&payload, + INTEL_PORT_STATS_CUSTOM, + ops); + break; default: error = OFPPROP_UNKNOWN(true, "port stats", type); break; @@ -8354,6 +8460,7 @@ ofputil_decode_port_stats(struct ofputil_port_stats *ps, struct ofpbuf *msg) enum ofpraw raw; memset(&(ps->stats), 0xFF, sizeof (ps->stats)); + memset(&(ps->custom_stats), 0, sizeof (ps->custom_stats)); error = (msg->header ? ofpraw_decode(&raw, msg->header) : ofpraw_pull(&raw, msg)); diff --git a/lib/util.c b/lib/util.c index 2965656..462c1fa 100644 --- a/lib/util.c +++ b/lib/util.c @@ -321,6 +321,19 @@ ovs_strzcpy(char *dst, const char *src, size_t size) } } +/* + * Returns true if 'str' ends with given 'suffix'. + */ +int +string_ends_with(const char * str, const char * suffix) +{ + int str_len = strlen(str); + int suffix_len = strlen(suffix); + + return (str_len >= suffix_len) && + (0 == strcmp(str + (str_len - suffix_len), suffix)); +} + /* Prints 'format' on stderr, formatting it like printf() does. If 'err_no' is * nonzero, then it is formatted with ovs_retval_to_string() and appended to * the message inside parentheses. Then, terminates with abort(). diff --git a/lib/util.h b/lib/util.h index b01f421..e75f6ce 100644 --- a/lib/util.h +++ b/lib/util.h @@ -157,6 +157,8 @@ void free_cacheline(void *); void ovs_strlcpy(char *dst, const char *src, size_t size); void ovs_strzcpy(char *dst, const char *src, size_t size); +int string_ends_with(const char * str, const char * suffix); + /* The C standards say that neither the 'dst' nor 'src' argument to * memcpy() may be null, even if 'n' is zero. This wrapper tolerates * the null case. */ diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index b99f04f..e53ee35 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3775,6 +3775,18 @@ port_get_stats(const struct ofport *ofport_, struct netdev_stats *stats) } static int +port_get_custom_stats(const struct ofport *ofport_, + struct netdev_custom_stats *custom_stats) +{ + struct ofport_dpif *ofport = ofport_dpif_cast(ofport_); + int error; + + error = netdev_get_custom_stats(ofport->up.netdev, custom_stats); + + return error; +} + +static int port_get_lacp_stats(const struct ofport *ofport_, struct lacp_slave_stats *stats) { struct ofport_dpif *ofport = ofport_dpif_cast(ofport_); @@ -5785,6 +5797,7 @@ const struct ofproto_class ofproto_dpif_class = { port_del, port_set_config, port_get_stats, + port_get_custom_stats, port_dump_start, port_dump_next, port_dump_done, diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 9dc73c4..55e772e 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -1049,6 +1049,10 @@ struct ofproto_class { int (*port_get_stats)(const struct ofport *port, struct netdev_stats *stats); + /* Get port custom stats */ + int (*port_get_custom_stats)(const struct ofport *port, + struct netdev_custom_stats *custom_stats); + /* Port iteration functions. * * The client might not be entirely in control of the ports within an diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 82c2bb2..c76a9a1 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -2605,6 +2605,24 @@ ofproto_port_get_stats(const struct ofport *port, struct netdev_stats *stats) return error; } +int +ofproto_port_get_custom_stats(const struct ofport *port, + struct netdev_custom_stats *custom_stats) +{ + struct ofproto *ofproto = port->ofproto; + int error; + + if (ofproto->ofproto_class->port_get_custom_stats) { + memset(custom_stats, 0, sizeof *custom_stats); + error = ofproto->ofproto_class->port_get_custom_stats(port, + custom_stats); + } else { + error = EOPNOTSUPP; + } + + return error; +} + static int update_port(struct ofproto *ofproto, const char *name) { @@ -3887,8 +3905,11 @@ append_port_stat(struct ofport *port, struct ovs_list *replies) * 'stats' to all-1s, which is correct for OpenFlow, and * netdev_get_stats() will log errors. */ ofproto_port_get_stats(port, &ops.stats); + ofproto_port_get_custom_stats(port, &ops.custom_stats); ofputil_append_port_stat(replies, &ops); + + netdev_free_custom_stats_counters(&ops.custom_stats); } static void diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h index 1e48e19..15e478d 100644 --- a/ofproto/ofproto.h +++ b/ofproto/ofproto.h @@ -48,6 +48,7 @@ struct shash; struct simap; struct smap; struct netdev_stats; +struct netdev_custom_stats; struct ovs_list; struct lldp_status; struct aa_settings; @@ -297,6 +298,8 @@ int ofproto_port_del(struct ofproto *, ofp_port_t ofp_port); void ofproto_port_set_config(struct ofproto *, ofp_port_t ofp_port, const struct smap *cfg); int ofproto_port_get_stats(const struct ofport *, struct netdev_stats *stats); +int ofproto_port_get_custom_stats(const struct ofport *port, + struct netdev_custom_stats *custom_stats); int ofproto_port_query_by_name(const struct ofproto *, const char *devname, struct ofproto_port *); diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 630c6fa..1856908 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -2347,6 +2347,11 @@ iface_refresh_cfm_stats(struct iface *iface) static void iface_refresh_stats(struct iface *iface) { + struct netdev_custom_stats custom_stats; + uint32_t i; + int64_t *custom_values = NULL; + char **custom_keys = NULL; + #define IFACE_STATS \ IFACE_STAT(rx_packets, "rx_packets") \ IFACE_STAT(tx_packets, "tx_packets") \ @@ -2413,6 +2418,25 @@ iface_refresh_stats(struct iface *iface) ovsrec_interface_set_statistics(iface->cfg, keys, values, n); #undef IFACE_STATS + + netdev_get_custom_stats(iface->netdev, &custom_stats); + + if (custom_stats.size && custom_stats.counters) { + custom_values = xmalloc(custom_stats.size * sizeof *custom_values); + custom_keys = xmalloc(custom_stats.size * sizeof(char *)); + if (custom_values && custom_keys) { + for (i = 0 ; i < custom_stats.size ; i++) { + custom_values[i] = custom_stats.counters[i].value; + custom_keys[i] = custom_stats.counters[i].name; + } + ovsrec_interface_set_statistics(iface->cfg, + (const char **)custom_keys, + custom_values, custom_stats.size); + free(custom_values); + free(custom_keys); + } + netdev_free_custom_stats_counters(&custom_stats); + } } static void
- New get_custom_stats interface function is added to netdev. It allows particular netdev implementation to expose custom counters in dictionary format (counter name/counter value). - New statistics are retrieved using experimenter code and are printed as a result to ofctl dump-ports. - New counters are available for OpenFlow 1.4+. - New statistics are printed to output via ofctl only if those are present in reply message. - New statistics definition is added to include/openflow/intel-ext.h. - Custom statistics are implemented only for dpdk-physical port type. - DPDK-physical implementation uses xstats to collect statistics. Only dropped and error counters are exposed. v1->v2: - Buffer overrun check in parse_intel_port_custom_property. - ofputil_append_ofp14_port_stats uses "postappend" instead of "reserve" during message creation. - NEWS update. - DPDK documentation update. - Compilation and sparse warnings corrections. Signed-off-by: Michal Weglicki <michalx.weglicki@intel.com> --- Documentation/howto/dpdk.rst | 15 ++-- NEWS | 6 ++ include/openflow/intel-ext.h | 28 ++++++ include/openvswitch/netdev.h | 17 ++++ include/openvswitch/ofp-util.h | 1 + lib/netdev-dpdk.c | 195 ++++++++++++++++++++++++++++++++++++++++- lib/netdev-dummy.c | 1 + lib/netdev-linux.c | 1 + lib/netdev-provider.h | 13 +++ lib/netdev-vport.c | 1 + lib/netdev.c | 27 ++++++ lib/netdev.h | 2 + lib/ofp-print.c | 18 ++++ lib/ofp-util.c | 119 +++++++++++++++++++++++-- lib/util.c | 13 +++ lib/util.h | 2 + ofproto/ofproto-dpif.c | 13 +++ ofproto/ofproto-provider.h | 4 + ofproto/ofproto.c | 21 +++++ ofproto/ofproto.h | 3 + vswitchd/bridge.c | 24 +++++ 21 files changed, 512 insertions(+), 12 deletions(-)