From 5ca75531afd1244898ee7c8d2307857ba8260cba Mon Sep 17 00:00:00 2001

Message-Id: <5ca75531afd1244898ee7c8d2307857ba8260cba@dist-git>

From: Michal Privoznik <mprivozn@redhat.com>

Date: Wed, 9 Oct 2019 14:27:41 +0200

Subject: [PATCH] virNetDevOpenvswitchInterfaceStats: Optimize for speed

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

We run 'ovs-vsctl' nine times (first to find if interface is

there and then eight times = for each stats member separately).

This is very inefficient. I've found a way to run it once and

with a bit of help from virJSON module we can parse out stats

we need.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

(cherry picked from commit c297eab52599c91a4cb26b66dbdfe9d07c3142d3)

https://bugzilla.redhat.com/show_bug.cgi?id=1759904

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Message-Id: <9904d3bee93a9e103012d088b77999f023acaa2b.1570623892.git.mprivozn@redhat.com>

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

---

 src/util/virnetdevopenvswitch.c | 119 +++++++++++++++++++++-----------

 1 file changed, 78 insertions(+), 41 deletions(-)

diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c

index f36916a300..fc92ab2e7f 100644

--- a/src/util/virnetdevopenvswitch.c

+++ b/src/util/virnetdevopenvswitch.c

@@ -35,6 +35,7 @@

 #include "virmacaddr.h"

 #include "virstring.h"

 #include "virlog.h"

+#include "virjson.h"

 #define VIR_FROM_THIS VIR_FROM_NONE

@@ -339,58 +340,94 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,

                                    virDomainInterfaceStatsPtr stats)

 {

     virCommandPtr cmd = NULL;

-    char *output;

-    char *tmp;

-    bool gotStats = false;

+    VIR_AUTOFREE(char *) output = NULL;

+    virJSONValuePtr jsonStats = NULL;

+    virJSONValuePtr jsonMap = NULL;

+    size_t i;

     int ret = -1;

-    /* Just ensure the interface exists in ovs */

     cmd = virCommandNew(OVSVSCTL);

     virNetDevOpenvswitchAddTimeout(cmd);

-    virCommandAddArgList(cmd, "get", "Interface", ifname, "name", NULL);

+    virCommandAddArgList(cmd, "--if-exists", "--format=list", "--data=json",

+                         "--no-headings", "--columns=statistics", "list",

+                         "Interface", ifname, NULL);

     virCommandSetOutputBuffer(cmd, &output);

-    if (virCommandRun(cmd, NULL) < 0) {

+    /* The above command returns either:

+     * 1) empty string if @ifname doesn't exist, or

+     * 2) a JSON array, for instance:

+     *    ["map",[["collisions",0],["rx_bytes",0],["rx_crc_err",0],["rx_dropped",0],

+     *    ["rx_errors",0],["rx_frame_err",0],["rx_over_err",0],["rx_packets",0],

+     *    ["tx_bytes",12406],["tx_dropped",0],["tx_errors",0],["tx_packets",173]]]

+     */

+

+    if (virCommandRun(cmd, NULL) < 0 ||

+        STREQ_NULLABLE(output, "")) {

         /* no ovs-vsctl or interface 'ifname' doesn't exists in ovs */

         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

                        _("Interface not found"));

         goto cleanup;

     }

-#define GET_STAT(name, member) \

-    do { \

-        VIR_FREE(output); \

-        virCommandFree(cmd); \

-        cmd = virCommandNew(OVSVSCTL); \

-        virNetDevOpenvswitchAddTimeout(cmd); \

-        virCommandAddArgList(cmd, "--if-exists", "get", "Interface", \

-                             ifname, "statistics:" name, NULL); \

-        virCommandSetOutputBuffer(cmd, &output); \

-        if (virCommandRun(cmd, NULL) < 0 || !output || !*output || *output == '\n') { \

-            stats->member = -1; \

-        } else { \

-            if (virStrToLong_ll(output, &tmp, 10, &stats->member) < 0 || \

-                *tmp != '\n') { \

-                virReportError(VIR_ERR_INTERNAL_ERROR, "%s", \

-                               _("Fail to parse ovs-vsctl output")); \

-                goto cleanup; \

-            } \

-            gotStats = true; \

-        } \

-    } while (0)

-

-    /* The TX/RX fields appear to be swapped here

-     * because this is the host view. */

-    GET_STAT("rx_bytes", tx_bytes);

-    GET_STAT("rx_packets", tx_packets);

-    GET_STAT("rx_errors", tx_errs);

-    GET_STAT("rx_dropped", tx_drop);

-    GET_STAT("tx_bytes", rx_bytes);

-    GET_STAT("tx_packets", rx_packets);

-    GET_STAT("tx_errors", rx_errs);

-    GET_STAT("tx_dropped", rx_drop);

-

-    if (!gotStats) {

+    if (!(jsonStats = virJSONValueFromString(output)) ||

+        !virJSONValueIsArray(jsonStats) ||

+        !(jsonMap = virJSONValueArrayGet(jsonStats, 1))) {

+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

+                       _("Unable to parse ovs-vsctl output"));

+        goto cleanup;

+    }

+

+    stats->rx_bytes = stats->rx_packets = stats->rx_errs = stats->rx_drop = -1;

+    stats->tx_bytes = stats->tx_packets = stats->tx_errs = stats->tx_drop = -1;

+

+    for (i = 0; i < virJSONValueArraySize(jsonMap); i++) {

+        virJSONValuePtr item = virJSONValueArrayGet(jsonMap, i);

+        virJSONValuePtr jsonKey;

+        virJSONValuePtr jsonVal;

+        const char *key;

+        long long val;

+

+        if (!item ||

+            (!(jsonKey = virJSONValueArrayGet(item, 0))) ||

+            (!(jsonVal = virJSONValueArrayGet(item, 1))) ||

+            (!(key = virJSONValueGetString(jsonKey))) ||

+            (virJSONValueGetNumberLong(jsonVal, &val) < 0)) {

+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

+                           _("Malformed ovs-vsctl output"));

+            goto cleanup;

+        }

+

+        /* The TX/RX fields appear to be swapped here

+         * because this is the host view. */

+        if (STREQ(key, "rx_bytes")) {

+            stats->tx_bytes = val;

+        } else if (STREQ(key, "rx_packets")) {

+            stats->tx_packets = val;

+        } else if (STREQ(key, "rx_errors")) {

+            stats->tx_errs = val;

+        } else if (STREQ(key, "rx_dropped")) {

+            stats->tx_drop = val;

+        } else if (STREQ(key, "tx_bytes")) {

+            stats->rx_bytes = val;

+        } else if (STREQ(key, "tx_packets")) {

+            stats->rx_packets = val;

+        } else if (STREQ(key, "tx_errors")) {

+            stats->rx_errs = val;

+        } else if (STREQ(key, "tx_dropped")) {

+            stats->rx_drop = val;

+        } else {

+            VIR_DEBUG("Unused ovs-vsctl stat key=%s val=%lld", key, val);

+        }

+    }

+

+    if (stats->rx_bytes == -1 &&

+        stats->rx_packets == -1 &&

+        stats->rx_errs == -1 &&

+        stats->rx_drop == -1 &&

+        stats->tx_bytes == -1 &&

+        stats->tx_packets == -1 &&

+        stats->tx_errs == -1 &&

+        stats->tx_drop == -1) {

         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

                        _("Interface doesn't have any statistics"));

         goto cleanup;

@@ -399,7 +436,7 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,

     ret = 0;

  cleanup:

-    VIR_FREE(output);

+    virJSONValueFree(jsonStats);

     virCommandFree(cmd);

     return ret;

 }

-- 

2.23.0