virNetDevTapInterfaceStats: Allow caller to not swap the statistics

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

The comment in virNetDevTapInterfaceStats() implementation for
Linux states that packets transmitted by domain are received by
the host and vice versa. Well, this is true but not for all types
of interfaces. For instance, for macvtaps when TAP device is
hooked right onto a physical device any packet that domain sends
looks also like a packet sent to the host. Therefore, we should
allow caller to chose if the stats returned should be straight
copy or swapped.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c502ed951db6799d77b6687b42ea876227f61b21..54be9028d72b03f740fb93991df00e79093b47d0 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -26781,7 +26781,7 @@ virDomainStateReasonFromString(virDomainState state, const char *reason)
  */
 
 virDomainNetType
-virDomainNetGetActualType(virDomainNetDefPtr iface)
+virDomainNetGetActualType(const virDomainNetDef *iface)
 {
     if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
         return iface->type;

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 5f922068d5b2280ee46bc224ccaba18e83c49abd..a42efcfa68e35cfebf7c5b97d1b66cfa4923d231 100644 (file)
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3022,7 +3022,7 @@ int virDomainGraphicsListenAppendSocket(virDomainGraphicsDefPtr def,
                                         const char *socket)
             ATTRIBUTE_NONNULL(1);
 
-virDomainNetType virDomainNetGetActualType(virDomainNetDefPtr iface);
+virDomainNetType virDomainNetGetActualType(const virDomainNetDef *iface);
 const char *virDomainNetGetActualBridgeName(virDomainNetDefPtr iface);
 int virDomainNetGetActualBridgeMACTableManager(virDomainNetDefPtr iface);
 const char *virDomainNetGetActualDirectDev(virDomainNetDefPtr iface);
@@ -3394,4 +3394,39 @@ virDomainGenerateMachineName(const char *drivername,
                              int id,
                              const char *name,
                              bool privileged);
+/**
+ * virDomainNetTypeSharesHostView:
+ * @net: interface
+ *
+ * Some types of interfaces "share" the host view. For instance,
+ * for macvtap interface, every domain RX is the host RX too. And
+ * every domain TX is host TX too. IOW, for some types of
+ * interfaces guest and host are on the same side of RX/TX
+ * barrier. This is important so that we set up QoS correctly and
+ * report proper stats.
+ */
+static inline bool
+virDomainNetTypeSharesHostView(const virDomainNetDef *net)
+{
+    virDomainNetType actualType = virDomainNetGetActualType(net);
+    switch (actualType) {
+    case VIR_DOMAIN_NET_TYPE_DIRECT:
+    case VIR_DOMAIN_NET_TYPE_ETHERNET:
+        return true;
+    case VIR_DOMAIN_NET_TYPE_USER:
+    case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
+    case VIR_DOMAIN_NET_TYPE_SERVER:
+    case VIR_DOMAIN_NET_TYPE_CLIENT:
+    case VIR_DOMAIN_NET_TYPE_MCAST:
+    case VIR_DOMAIN_NET_TYPE_NETWORK:
+    case VIR_DOMAIN_NET_TYPE_BRIDGE:
+    case VIR_DOMAIN_NET_TYPE_INTERNAL:
+    case VIR_DOMAIN_NET_TYPE_HOSTDEV:
+    case VIR_DOMAIN_NET_TYPE_UDP:
+    case VIR_DOMAIN_NET_TYPE_LAST:
+        break;
+    }
+    return false;
+}
+
 #endif /* __DOMAIN_CONF_H */

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index a174d892e053a960ac98f9c8937e50c352538e5b..8483d6ecf78eba08647ac1a85de2ae7b0b0de47f 100644 (file)
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -4985,7 +4985,8 @@ libxlDomainInterfaceStats(virDomainPtr dom,
         goto endjob;
     }
 
-    if (virNetDevTapInterfaceStats(path, stats) < 0)
+    if (virNetDevTapInterfaceStats(path, stats,
+                                   !virDomainNetTypeSharesHostView(net)) < 0)
         goto endjob;
 
     ret = 0;

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index c0ef0c2108c5e165576319434f92fab5f19abb3e..f0a22ce3eb50bf7fa7381853bb3ce42e848ea7a8 100644 (file)
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -2878,7 +2878,8 @@ lxcDomainInterfaceStats(virDomainPtr dom,
         goto endjob;
     }
 
-    if (virNetDevTapInterfaceStats(path, stats) < 0)
+    if (virNetDevTapInterfaceStats(path, stats,
+                                   !virDomainNetTypeSharesHostView(net)) < 0)
         goto endjob;
 
     ret = 0;

diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 3c24020cea8d7418cce90d35d5fcd92536b7f772..11173898dca134d18088292625711fe43679b4da 100644 (file)
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -2012,7 +2012,8 @@ openvzDomainInterfaceStats(virDomainPtr dom,
         goto cleanup;
     }
 
-    if (virNetDevTapInterfaceStats(path, stats) < 0)
+    if (virNetDevTapInterfaceStats(path, stats,
+                                   !virDomainNetTypeSharesHostView(net)) < 0)
         goto cleanup;
 
     ret = 0;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1ab16e57c4cb946a57fcab01cdaa6532b498dfc0..dcad01a01debdc2d306c937d00e54e301587f691 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11040,7 +11040,8 @@ qemuDomainInterfaceStats(virDomainPtr dom,
         if (virNetDevOpenvswitchInterfaceStats(path, stats) < 0)
             goto cleanup;
     } else {
-        if (virNetDevTapInterfaceStats(path, stats) < 0)
+        if (virNetDevTapInterfaceStats(path, stats,
+                                       !virDomainNetTypeSharesHostView(net)) < 0)
             goto cleanup;
     }
 
@@ -19559,26 +19560,27 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
 
     /* Check the path is one of the domain's network interfaces. */
     for (i = 0; i < dom->def->nnets; i++) {
+        virDomainNetDefPtr net = dom->def->nets[i];
         virDomainNetType actualType;
 
-        if (!dom->def->nets[i]->ifname)
+        if (!net->ifname)
             continue;
 
         memset(&tmp, 0, sizeof(tmp));
 
-        actualType = virDomainNetGetActualType(dom->def->nets[i]);
+        actualType = virDomainNetGetActualType(net);
 
         QEMU_ADD_NAME_PARAM(record, maxparams,
-                            "net", "name", i, dom->def->nets[i]->ifname);
+                            "net", "name", i, net->ifname);
 
         if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
-            if (virNetDevOpenvswitchInterfaceStats(dom->def->nets[i]->ifname,
-                                                   &tmp) < 0) {
+            if (virNetDevOpenvswitchInterfaceStats(net->ifname, &tmp) < 0) {
                 virResetLastError();
                 continue;
             }
         } else {
-            if (virNetDevTapInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) {
+            if (virNetDevTapInterfaceStats(net->ifname, &tmp,
+                                           !virDomainNetTypeSharesHostView(net)) < 0) {
                 virResetLastError();
                 continue;
             }

diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index 175dc2bfaae389258275808847c91b527a42857e..a3ed59da85021d914734d162922fe892b8a80635 100644 (file)
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -676,14 +676,27 @@ int virNetDevTapCreateInBridgePort(const char *brname,
 }
 
 /*-------------------- interface stats --------------------*/
-/* Just reads the named interface, so not Xen or QEMU-specific.
- * NB. Caller must check that libvirt user is trying to query
- * the interface of a domain they own.  We do no such checking.
+
+/**
+ * virNetDevTapInterfaceStats:
+ * @ifname: interface
+ * @stats: where to store statistics
+ * @swapped: whether to swap RX/TX fields
+ *
+ * Fetch RX/TX statistics for given named interface (@ifname) and
+ * store them at @stats. The returned statistics are always from
+ * domain POV. Because in some cases this means swapping RX/TX in
+ * the stats and in others this means no swapping (consider TAP
+ * vs macvtap) caller might choose if the returned stats should
+ * be @swapped or not.
+ *
+ * Returns 0 on success, -1 otherwise (with error reported).
  */
 #ifdef __linux__
 int
 virNetDevTapInterfaceStats(const char *ifname,
-                           virDomainInterfaceStatsPtr stats)
+                           virDomainInterfaceStatsPtr stats,
+                           bool swapped)
 {
     int ifname_len;
     FILE *fp;
@@ -718,30 +731,35 @@ virNetDevTapInterfaceStats(const char *ifname,
         *colon = '\0';
         if (colon-ifname_len >= line &&
             STREQ(colon-ifname_len, ifname)) {
-            /* IMPORTANT NOTE!
-             * /proc/net/dev vif<domid>.nn sees the network from the point
-             * of view of dom0 / hypervisor.  So bytes TRANSMITTED by dom0
-             * are bytes RECEIVED by the domain.  That's why the TX/RX fields
-             * appear to be swapped here.
-             */
             if (sscanf(colon+1,
                        "%lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld %lld",
-                       &tx_bytes, &tx_packets, &tx_errs, &tx_drop,
-                       &dummy, &dummy, &dummy, &dummy,
                        &rx_bytes, &rx_packets, &rx_errs, &rx_drop,
+                       &dummy, &dummy, &dummy, &dummy,
+                       &tx_bytes, &tx_packets, &tx_errs, &tx_drop,
                        &dummy, &dummy, &dummy, &dummy) != 16)
                 continue;
 
-            stats->rx_bytes = rx_bytes;
-            stats->rx_packets = rx_packets;
-            stats->rx_errs = rx_errs;
-            stats->rx_drop = rx_drop;
-            stats->tx_bytes = tx_bytes;
-            stats->tx_packets = tx_packets;
-            stats->tx_errs = tx_errs;
-            stats->tx_drop = tx_drop;
-            VIR_FORCE_FCLOSE(fp);
+            if (swapped) {
+                stats->rx_bytes = tx_bytes;
+                stats->rx_packets = tx_packets;
+                stats->rx_errs = tx_errs;
+                stats->rx_drop = tx_drop;
+                stats->tx_bytes = rx_bytes;
+                stats->tx_packets = rx_packets;
+                stats->tx_errs = rx_errs;
+                stats->tx_drop = rx_drop;
+            } else {
+                stats->rx_bytes = rx_bytes;
+                stats->rx_packets = rx_packets;
+                stats->rx_errs = rx_errs;
+                stats->rx_drop = rx_drop;
+                stats->tx_bytes = tx_bytes;
+                stats->tx_packets = tx_packets;
+                stats->tx_errs = tx_errs;
+                stats->tx_drop = tx_drop;
+            }
 
+            VIR_FORCE_FCLOSE(fp);
             return 0;
         }
     }
@@ -754,7 +772,8 @@ virNetDevTapInterfaceStats(const char *ifname,
 #elif defined(HAVE_GETIFADDRS) && defined(AF_LINK)
 int
 virNetDevTapInterfaceStats(const char *ifname,
-                           virDomainInterfaceStatsPtr stats)
+                           virDomainInterfaceStatsPtr stats,
+                           bool swapped)
 {
     struct ifaddrs *ifap, *ifa;
     struct if_data *ifd;
@@ -775,18 +794,33 @@ virNetDevTapInterfaceStats(const char *ifname,
 
         if (STREQ(ifa->ifa_name, ifname)) {
             ifd = (struct if_data *)ifa->ifa_data;
-            stats->tx_bytes = ifd->ifi_ibytes;
-            stats->tx_packets = ifd->ifi_ipackets;
-            stats->tx_errs = ifd->ifi_ierrors;
-            stats->tx_drop = ifd->ifi_iqdrops;
-            stats->rx_bytes = ifd->ifi_obytes;
-            stats->rx_packets = ifd->ifi_opackets;
-            stats->rx_errs = ifd->ifi_oerrors;
+            if (swapped) {
+                stats->tx_bytes = ifd->ifi_ibytes;
+                stats->tx_packets = ifd->ifi_ipackets;
+                stats->tx_errs = ifd->ifi_ierrors;
+                stats->tx_drop = ifd->ifi_iqdrops;
+                stats->rx_bytes = ifd->ifi_obytes;
+                stats->rx_packets = ifd->ifi_opackets;
+                stats->rx_errs = ifd->ifi_oerrors;
 # ifdef HAVE_STRUCT_IF_DATA_IFI_OQDROPS
-            stats->rx_drop = ifd->ifi_oqdrops;
+                stats->rx_drop = ifd->ifi_oqdrops;
 # else
-            stats->rx_drop = 0;
+                stats->rx_drop = 0;
 # endif
+            } else {
+                stats->tx_bytes = ifd->ifi_obytes;
+                stats->tx_packets = ifd->ifi_opackets;
+                stats->tx_errs = ifd->ifi_oerrors;
+# ifdef HAVE_STRUCT_IF_DATA_IFI_OQDROPS
+                stats->tx_drop = ifd->ifi_oqdrops;
+# else
+                stats->tx_drop = 0;
+# endif
+                stats->rx_bytes = ifd->ifi_ibytes;
+                stats->rx_packets = ifd->ifi_ipackets;
+                stats->rx_errs = ifd->ifi_ierrors;
+                stats->rx_drop = ifd->ifi_iqdrops;
+            }
 
             ret = 0;
             break;
@@ -803,7 +837,8 @@ virNetDevTapInterfaceStats(const char *ifname,
 #else
 int
 virNetDevTapInterfaceStats(const char *ifname ATTRIBUTE_UNUSED,
-                           virDomainInterfaceStatsPtr stats ATTRIBUTE_UNUSED)
+                           virDomainInterfaceStatsPtr stats ATTRIBUTE_UNUSED,
+                           bool swapped ATTRIBUTE_UNUSED)
 {
     virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                    _("interface stats not implemented on this platform"));

diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
index 0b17feb8d94d0306598f8de8247fca251c235ade..727368fe95877502f47d2d7bddcb111d8e8cf73f 100644 (file)
--- a/src/util/virnetdevtap.h
+++ b/src/util/virnetdevtap.h
@@ -92,7 +92,8 @@ int virNetDevTapCreateInBridgePort(const char *brname,
     ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE;
 
 int virNetDevTapInterfaceStats(const char *ifname,
-                               virDomainInterfaceStatsPtr stats)
+                               virDomainInterfaceStatsPtr stats,
+                               bool swapped)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
 
 #endif /* __VIR_NETDEV_TAP_H__ */

diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index e18a4729dbf2a4e0fd5b80e4f149cc80274938ad..70fdcbe55417f4ab3bc9b8c1135f0e143f2638bc 100644 (file)
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1468,7 +1468,7 @@ xenHypervisorDomainInterfaceStats(virDomainDefPtr def,
         return -1;
     }
 
-    return virNetDevTapInterfaceStats(path, stats);
+    return virNetDevTapInterfaceStats(path, stats, true);
 #else
     virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                    _("/proc/net/dev: Interface not found"));