From d86fd2402e9d217f90e0a149d6486f392b34f7ba Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Mon, 2 Oct 2017 13:36:56 +0200 Subject: [PATCH] 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 Reviewed-by: John Ferlan --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 37 +++++++++++++- src/libxl/libxl_driver.c | 3 +- src/lxc/lxc_driver.c | 3 +- src/openvz/openvz_driver.c | 3 +- src/qemu/qemu_driver.c | 16 +++--- src/util/virnetdevtap.c | 99 ++++++++++++++++++++++++++------------ src/util/virnetdevtap.h | 3 +- src/xen/xen_hypervisor.c | 2 +- 9 files changed, 122 insertions(+), 46 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c502ed951d..54be9028d7 100644 --- 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 5f922068d5..a42efcfa68 100644 --- 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 a174d892e0..8483d6ecf7 100644 --- 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 c0ef0c2108..f0a22ce3eb 100644 --- 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 3c24020cea..11173898dc 100644 --- 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 1ab16e57c4..dcad01a01d 100644 --- 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 175dc2bfaa..a3ed59da85 100644 --- 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.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 0b17feb8d9..727368fe95 100644 --- 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 e18a4729db..70fdcbe554 100644 --- 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")); -- 2.39.5