[ovs-dev] [PATCH v2] netdev: Custom statistics.
Ben Pfaff
blp at ovn.org
Tue Dec 19 23:42:10 UTC 2017
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 at 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
More information about the dev
mailing list