[libvirt] [PATCH 5/7] virNetDevTapInterfaceStats: Allow caller to not swap the statistics
Michal Privoznik
mprivozn at redhat.com
Thu Oct 5 07:23:30 UTC 2017
On 10/05/2017 12:38 AM, John Ferlan wrote:
>
>
> 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 at redhat.com>
>> ---
>> 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 ;-)
Yeah, but I was too lazy to come up with yet another commit message. And
one can argue that without the rest of the patch these two changes are
not that much needed ;-)
>
> 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.
Ah right. Good catch.
>
>> }
>>
>> 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.
Yes. The command is run on the host. And yes, it doesn't always
correspond to "always from domain POV". Hence the bool @swapped.
Michal
More information about the libvir-list
mailing list