dpdkstat: fix retrieval of statistics

This change forces dpdkstat plugin to check if memory needs to be resized on
every DPDK_CMD_GET_STATS command, rather than resizing it only once at
initialization. Additionally it fixes incorrect metric retrieval logic, which
allows plugin to dispatch metrics only if number of retrieved extended statistics
is equal to allocated stats array size for port. Based on DPDK API documentation
rte_eth_xstats_get (and rte_eth_xstats_get_names) function can return positive
value lower than array size, which isn't considered a failure.

If primary DPDK process is restarted with a greater number of ports bound to
DPDK driver while collectd dpdkstat plugin is running, then memory isn't resized.
Due to insufficient memory allocation dpdkstat plugin is unable to gather metrics
from new ports.

Change-Id: I25c8995105a322474653bf7065c2228047f886b1
Signed-off-by: Przemyslaw Szczerbik <przemyslawx.szczerbik@intel.com>

diff --git a/src/dpdkstat.c b/src/dpdkstat.c
index 4c73eab..6b057f2 100644 (file)
--- a/src/dpdkstat.c
+++ b/src/dpdkstat.c
@@ -118,7 +118,7 @@ static int dpdk_stats_preinit(void) {
   if (ret != 0) {
     char errbuf[ERR_BUF_SIZE];
     ERROR("%s: failed to initialize %s helper(error: %s)", DPDK_STATS_PLUGIN,
-         g_shm_name, sstrerror(errno, errbuf, sizeof(errbuf)));
+          g_shm_name, sstrerror(errno, errbuf, sizeof(errbuf)));
     return ret;
   }
 
@@ -143,11 +143,9 @@ static int dpdk_stats_config(oconfig_item_t *ci) {
       ctx->config.enabled_port_mask = child->values[0].value.number;
       DEBUG("%s: Enabled Port Mask 0x%X", DPDK_STATS_PLUGIN,
             ctx->config.enabled_port_mask);
-    } else  if (strcasecmp("SharedMemObj", child->key) == 0) {
-      cf_util_get_string_buffer(child, g_shm_name,
-                                sizeof(g_shm_name));
-      DEBUG("%s: Shared memory object %s", DPDK_STATS_PLUGIN,
-            g_shm_name);
+    } else if (strcasecmp("SharedMemObj", child->key) == 0) {
+      cf_util_get_string_buffer(child, g_shm_name, sizeof(g_shm_name));
+      DEBUG("%s: Shared memory object %s", DPDK_STATS_PLUGIN, g_shm_name);
       dpdk_stats_reinit_helper();
     } else if (strcasecmp("EAL", child->key) == 0) {
       ret = dpdk_helper_eal_config_parse(g_hc, child);
@@ -180,73 +178,56 @@ static int dpdk_stats_config(oconfig_item_t *ci) {
 }
 
 static int dpdk_helper_stats_get(dpdk_helper_ctx_t *phc) {
-  dpdk_stats_ctx_t *ctx = DPDK_STATS_CTX_GET(phc);
-
-  /* get stats from DPDK */
-
-  uint8_t ports_count = rte_eth_dev_count();
-  if (ports_count == 0) {
-    DPDK_CHILD_LOG("%s: No DPDK ports available. "
-                   "Check bound devices to DPDK driver.\n",
-                   DPDK_STATS_PLUGIN);
-    return -ENODEV;
-  }
-
-  if (ports_count > RTE_MAX_ETHPORTS)
-    ports_count = RTE_MAX_ETHPORTS;
-
-  ctx->ports_count = ports_count;
-
   int len = 0;
   int ret = 0;
   int stats = 0;
+  dpdk_stats_ctx_t *ctx = DPDK_STATS_CTX_GET(phc);
 
-  for (uint8_t i = 0; i < ports_count; i++) {
+  /* get stats from DPDK */
+  for (uint8_t i = 0; i < ctx->ports_count; i++) {
     if (!(ctx->config.enabled_port_mask & (1 << i)))
       continue;
+
     ctx->port_read_time[i] = cdtime();
+    /* Store available stats array length for port */
     len = ctx->port_stats_count[i];
+
     ret = rte_eth_xstats_get(i, &ctx->xstats[stats], len);
-    if (ret < 0 || ret != len) {
-      DPDK_CHILD_LOG("%s: Error reading stats (port=%d; len=%d)\n",
-                     DPDK_STATS_PLUGIN, i, len);
+    if (ret < 0 || ret > len) {
+      DPDK_CHILD_LOG(DPDK_STATS_PLUGIN
+                     ": Error reading stats (port=%d; len=%d, ret=%d)\n",
+                     i, len, ret);
+      ctx->port_stats_count[i] = 0;
       return -1;
     }
 #if RTE_VERSION >= RTE_VERSION_16_07
     ret = rte_eth_xstats_get_names(i, &ctx->xnames[stats], len);
-    if (ret < 0 || ret != len) {
-      DPDK_CHILD_LOG("%s: Error reading stat names (port=%d; len=%d)\n",
-                     DPDK_STATS_PLUGIN, i, len);
+    if (ret < 0 || ret > len) {
+      DPDK_CHILD_LOG(DPDK_STATS_PLUGIN
+                     ": Error reading stat names (port=%d; len=%d ret=%d)\n",
+                     i, len, ret);
+      ctx->port_stats_count[i] = 0;
       return -1;
     }
 #endif
-    stats += len;
+    ctx->port_stats_count[i] = ret;
+    stats += ctx->port_stats_count[i];
   }
 
-  assert(stats == ctx->stats_count);
-
+  assert(stats <= ctx->stats_count);
   return 0;
 }
 
 static int dpdk_helper_stats_count_get(dpdk_helper_ctx_t *phc) {
-  dpdk_stats_ctx_t *ctx = DPDK_STATS_CTX_GET(phc);
-
-  uint8_t ports = rte_eth_dev_count();
-  if (ports == 0) {
-    DPDK_CHILD_LOG("%s: No DPDK ports available. "
-                   "Check bound devices to DPDK driver.\n",
-                   DPDK_STATS_PLUGIN);
+  uint8_t ports = dpdk_helper_eth_dev_count();
+  if (ports == 0)
     return -ENODEV;
-  }
-
-  if (ports > RTE_MAX_ETHPORTS)
-    ports = RTE_MAX_ETHPORTS;
 
+  dpdk_stats_ctx_t *ctx = DPDK_STATS_CTX_GET(phc);
   ctx->ports_count = ports;
 
   int len = 0;
   int stats_count = 0;
-
   for (int i = 0; i < ports; i++) {
     if (!(ctx->config.enabled_port_mask & (1 << i)))
       continue;
@@ -269,9 +250,12 @@ static int dpdk_helper_stats_count_get(dpdk_helper_ctx_t *phc) {
   return stats_count;
 }
 
+static int dpdk_stats_get_size(dpdk_helper_ctx_t *phc) {
+  return (dpdk_helper_data_size_get(phc) - sizeof(dpdk_stats_ctx_t));
+}
+
 int dpdk_helper_command_handler(dpdk_helper_ctx_t *phc, enum DPDK_CMD cmd) {
   /* this function is called from helper context */
-  int ret = 0;
 
   if (phc == NULL) {
     DPDK_CHILD_LOG("%s: Invalid argument(phc)\n", DPDK_STATS_PLUGIN);
@@ -283,34 +267,23 @@ int dpdk_helper_command_handler(dpdk_helper_ctx_t *phc, enum DPDK_CMD cmd) {
     return -EINVAL;
   }
 
-  dpdk_stats_ctx_t *ctx = DPDK_STATS_CTX_GET(phc);
-
-  if (ctx->stats_count == 0) {
-
-    int stats_count = dpdk_helper_stats_count_get(phc);
+  int stats_count = dpdk_helper_stats_count_get(phc);
+  if (stats_count < 0) {
+    return stats_count;
+  }
 
-    if (stats_count < 0) {
-      return stats_count;
-    }
+  DPDK_STATS_CTX_GET(phc)->stats_count = stats_count;
+  int stats_size = stats_count * DPDK_STATS_CTX_GET_XSTAT_SIZE;
 
-    int stats_size = stats_count * DPDK_STATS_CTX_GET_XSTAT_SIZE;
-    ctx->stats_count = stats_count;
-
-    if ((dpdk_helper_data_size_get(phc) - sizeof(dpdk_stats_ctx_t)) <
-        stats_size) {
-      DPDK_CHILD_LOG(
-          "%s:%s:%d not enough space for stats (available=%d, "
-          "needed=%d)\n",
-          DPDK_STATS_PLUGIN, __FUNCTION__, __LINE__,
-          (int)(dpdk_helper_data_size_get(phc) - sizeof(dpdk_stats_ctx_t)),
-          stats_size);
-      return -ENOBUFS;
-    }
+  if (dpdk_stats_get_size(phc) < stats_size) {
+    DPDK_CHILD_LOG(
+        DPDK_STATS_PLUGIN
+        ":%s:%d not enough space for stats (available=%d, needed=%d)\n",
+        __FUNCTION__, __LINE__, (int)dpdk_stats_get_size(phc), stats_size);
+    return -ENOBUFS;
   }
 
-  ret = dpdk_helper_stats_get(phc);
-
-  return ret;
+  return dpdk_helper_stats_get(phc);
 }
 
 static void dpdk_stats_resolve_cnt_type(char *cnt_type, size_t cnt_type_len,
@@ -515,8 +488,7 @@ static int dpdk_stats_shutdown(void) {
   ret = dpdk_helper_shutdown(g_hc);
   g_hc = NULL;
   if (ret != 0) {
-    ERROR("%s: failed to cleanup %s helper", DPDK_STATS_PLUGIN,
-          g_shm_name);
+    ERROR("%s: failed to cleanup %s helper", DPDK_STATS_PLUGIN, g_shm_name);
     return ret;
   }
 

diff --git a/src/utils_dpdk.c b/src/utils_dpdk.c
index 640f08b..e3c7379 100644 (file)
--- a/src/utils_dpdk.c
+++ b/src/utils_dpdk.c
@@ -38,6 +38,7 @@
 
 #include <rte_config.h>
 #include <rte_eal.h>
+#include <rte_ethdev.h>
 
 #include "common.h"
 #include "utils_dpdk.h"
@@ -850,3 +851,22 @@ uint128_t str_to_uint128(const char *str, int len) {
   }
   return lcore_mask;
 }
+
+uint8_t dpdk_helper_eth_dev_count() {
+  uint8_t ports = rte_eth_dev_count();
+  if (ports == 0) {
+    ERROR(
+        "%s:%d: No DPDK ports available. Check bound devices to DPDK driver.\n",
+        __FUNCTION__, __LINE__);
+    return ports;
+  }
+
+  if (ports > RTE_MAX_ETHPORTS) {
+    ERROR("%s:%d: Number of DPDK ports (%u) is greater than "
+          "RTE_MAX_ETHPORTS=%d. Ignoring extra ports\n",
+          __FUNCTION__, __LINE__, ports, RTE_MAX_ETHPORTS);
+    ports = RTE_MAX_ETHPORTS;
+  }
+
+  return ports;
+}

diff --git a/src/utils_dpdk.h b/src/utils_dpdk.h
index c9bb14b..f97a6b5 100644 (file)
--- a/src/utils_dpdk.h
+++ b/src/utils_dpdk.h
@@ -73,6 +73,7 @@ int dpdk_helper_command(dpdk_helper_ctx_t *phc, enum DPDK_CMD cmd, int *result,
                         cdtime_t cmd_wait_time);
 void *dpdk_helper_priv_get(dpdk_helper_ctx_t *phc);
 int dpdk_helper_data_size_get(dpdk_helper_ctx_t *phc);
+uint8_t dpdk_helper_eth_dev_count();
 
 /* forward declaration of handler function that is called by helper from
  * child process. not implemented in helper. must be provided by client. */