Re: [libvirt] [PATCH 5/7] virNetDevTapInterfaceStats: Allow caller to not swap the statistics

On 10/02/2017 11:05 AM, Michal Privoznik wrote: > 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 > have caller chose if the stats returned should be straight copy s/have/allow/ s/chose/to choose BTW: My first read of this thought "caller" is user configurable :-) > or swapped. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > 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 | 18 +++++---- > src/util/virnetdevtap.c | 99 +++++++++++++++++++++++++++++++--------------- > src/util/virnetdevtap.h | 3 +- > src/xen/xen_hypervisor.c | 2 +- > 9 files changed, 123 insertions(+), 47 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 2289399cd..6569b94fa 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -26729,7 +26729,7 @@ virDomainStateReasonFromString(virDomainState state, const char *reason) > */ > > virDomainNetType > -virDomainNetGetActualType(virDomainNetDefPtr iface) > +virDomainNetGetActualType(const virDomainNetDef *iface) Sigh, separable along w/ domain_conf.h change... > { > 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 9ba84a94d..6eda21ef7 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -3021,7 +3021,7 @@ int virDomainGraphicsListenAppendSocket(virDomainGraphicsDefPtr def, > const char *socket) > ATTRIBUTE_NONNULL(1); > > -virDomainNetType virDomainNetGetActualType(virDomainNetDefPtr iface); > +virDomainNetType virDomainNetGetActualType(const virDomainNetDef *iface); e.g. this is separate and unrelated needs it's own already R-B patch ;-)

The rest is "this" patch specific... > const char *virDomainNetGetActualBridgeName(virDomainNetDefPtr iface); > int virDomainNetGetActualBridgeMACTableManager(virDomainNetDefPtr iface); > const char *virDomainNetGetActualDirectDev(virDomainNetDefPtr iface); > @@ -3393,4 +3393,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/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 1ab16e57c..a46409d70 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,29 +19560,30 @@ 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, > + if (virNetDevOpenvswitchInterfaceStats(net->ifname, > &tmp) < 0) { This line may as well move to previous line as well for just one line. > virResetLastError(); > continue; > } > } else { > - if (virNetDevTapInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) { > + if (virNetDevTapInterfaceStats(net->ifname, &tmp, > + !virDomainNetTypeSharesHostView(net)) < 0) > virResetLastError(); > - continue; > - } > + continue; Is this one right? We won't QEMU_ADD_NET_PARAM when actual_type != VIR_DOMAIN_NET_TYPE_VHOSTUSER *and* virNetDevTapInterfaceStats succeeds? Looks like a bad transposition of extracted patches.

> } > > QEMU_ADD_NET_PARAM(record, maxparams, i, > diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c > index 175dc2bfa..a3ed59da8 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<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. Maybe it's just a wording thing, but "dom0 / hypervisor" from the above note don't directly correspond to "are always from domain POV" described above - at least as how domain gets generally described... The command is run on the host right? So let's just be clear.