Re: [libvirt] [PATCH-RFC-V2] qemu: Add network bandwidth setting for ethernet interfaces

On 18.09.2014 01:33, Anirban Chakraborty wrote: > V2: > Consolidate calls to virNetDevBandwidthSet > Clear bandwidth settings when the interface is detached or domain >destroyed > > V1: > Ethernet interfaces in libvirt currently do not support bandwidth >setting. > For example, following xml file for an interface will not apply these > settings to corresponding qdiscs. > > <interface type="ethernet"> > <mac address="02:36:1d:18:2a:e4"/> > <model type="virtio"/> > <script path=""/> > <target dev="tap361d182a-e4"/> > <bandwidth> > <inbound average="984" peak="1024" burst="64"/> > <outbound average="2000" peak="2048" burst="128"/> > </bandwidth> > </interface> > > Signed-off-by: Anirban Chakraborty <abchak(a)juniper.net&gt; > --- > src/conf/domain_conf.h | 7 +++++++ > src/lxc/lxc_process.c | 27 ++++++++++++++------------- > src/qemu/qemu_command.c | 9 ++++----- > src/qemu/qemu_driver.c | 21 +++++++++++++++++++++ > src/qemu/qemu_hotplug.c | 13 +++++++++++++ > src/util/virnetdevmacvlan.c | 10 ---------- > src/util/virnetdevmacvlan.h | 1 - > 7 files changed, 59 insertions(+), 29 deletions(-) > > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 640a4c5..3c950f1 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -829,6 +829,13 @@ typedef enum { > VIR_DOMAIN_NET_TYPE_LAST > } virDomainNetType; > > +/* check bandwidth configuration for a network interface */ > +#define NETDEVIF_SUPPORT_BANDWIDTH(type) \ > + ((type == VIR_DOMAIN_NET_TYPE_ETHERNET || \ > + type == VIR_DOMAIN_NET_TYPE_NETWORK || \ > + type == VIR_DOMAIN_NET_TYPE_BRIDGE || \ > + type == VIR_DOMAIN_NET_TYPE_DIRECT) ? true : false) > + I'd rather turn this into a function (possibly inline function).

> /* the backend driver used for virtio interfaces */ > typedef enum { > VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT, /* prefer kernel, fall back >to user */ > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index ed30c37..5ef91e8 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -274,11 +274,6 @@ char >*virLXCProcessSetupInterfaceBridged(virConnectPtr conn, > if (virNetDevSetOnline(parentVeth, true) < 0) > goto cleanup; > > - if (virNetDevBandwidthSet(net->ifname, > - virDomainNetGetActualBandwidth(net), > - false) < 0) > - goto cleanup; > - Well, the virLXCProcessSetupInterfaceBridged is used elsewhere in the code too. For instance after this patch the QoS is not applied on interfaces hotplugged into an LXC container.

> if (net->filter && > virDomainConfNWFilterInstantiate(conn, vm->uuid, net) < 0) > goto cleanup; > @@ -300,6 +295,7 @@ char >*virLXCProcessSetupInterfaceDirect(virConnectPtr conn, > virNetDevBandwidthPtr bw; > virNetDevVPortProfilePtr prof; > virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); > + const char *linkdev = virDomainNetGetActualDirectDev(net); > > /* XXX how todo bandwidth controls ? > * Since the 'net-ifname' is about to be moved to a different > @@ -329,16 +325,15 @@ char >*virLXCProcessSetupInterfaceDirect(virConnectPtr conn, > > if (virNetDevMacVLanCreateWithVPortProfile( > net->ifname, &net->mac, > - virDomainNetGetActualDirectDev(net), > + linkdev, > virDomainNetGetActualDirectMode(net), > false, def->uuid, > - virDomainNetGetActualVirtPortProfile(net), > + prof, > &res_ifname, > VIR_NETDEV_VPORT_PROFILE_OP_CREATE, > cfg->stateDir, > - virDomainNetGetActualBandwidth(net), 0) < 0) > + 0) < 0) > goto cleanup; > - > ret = res_ifname; > > cleanup: > @@ -368,6 +363,7 @@ static int >virLXCProcessSetupInterfaces(virConnectPtr conn, > int ret = -1; > size_t i; > size_t niface = 0; > + int actualType; > > for (i = 0; i < def->nnets; i++) { > char *veth = NULL; > @@ -381,7 +377,8 @@ static int >virLXCProcessSetupInterfaces(virConnectPtr conn, > if (VIR_EXPAND_N(*veths, *nveths, 1) < 0) > goto cleanup; > > - switch (virDomainNetGetActualType(def->nets[i])) { > + actualType = virDomainNetGetActualType(def->nets[i]); > + switch (actualType) { > case VIR_DOMAIN_NET_TYPE_NETWORK: { > virNetworkPtr network; > char *brname = NULL; > @@ -444,11 +441,15 @@ static int >virLXCProcessSetupInterfaces(virConnectPtr conn, > case VIR_DOMAIN_NET_TYPE_LAST: > virReportError(VIR_ERR_INTERNAL_ERROR, > _("Unsupported network type %s"), > - virDomainNetTypeToString( > - virDomainNetGetActualType(def->nets[i]) > - )); > + virDomainNetTypeToString(actualType)); > goto cleanup; > } > + /* set network bandwidth */ > + if (NETDEVIF_SUPPORT_BANDWIDTH(actualType) && >virNetDevBandwidthSet( > + def->nets[i]->ifname, virDomainNetGetActualBandwidth( > + def->nets[i]), > + false) < 0) I must say this formatting looks very strange to me in libvirt connotations.

> + goto cleanup; > > (*veths)[(*nveths)-1] = veth; > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index f2e6e5a..404c51b 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -191,7 +191,6 @@ qemuPhysIfaceConnect(virDomainDefPtr def, > virDomainNetGetActualVirtPortProfile(net), > &res_ifname, > vmop, cfg->stateDir, > - virDomainNetGetActualBandwidth(net), > macvlan_create_flags); > if (rc >= 0) { > virDomainAuditNetDevice(def, net, res_ifname, true); > @@ -371,10 +370,6 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, > &net->mac) < 0) > goto cleanup; > > - if (virNetDevBandwidthSet(net->ifname, > - virDomainNetGetActualBandwidth(net), > - false) < 0) > - goto cleanup; > > if (net->filter && > virDomainConfNWFilterInstantiate(conn, def->uuid, net) < 0) { > @@ -7313,6 +7308,10 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, > if (tapfd[0] < 0) > goto cleanup; > } > + /* Set bandwidth */ > + if (NETDEVIF_SUPPORT_BANDWIDTH(actualType) && >virNetDevBandwidthSet( > + net->ifname, virDomainNetGetActualBandwidth(net), false) < >0) > + goto cleanup; > > if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK || > actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 5fd89a3..f5a5cbe 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -186,6 +186,9 @@ qemuVMFilterRebuild(virDomainObjListIterator iter, >void *data) > return virDomainObjListForEach(qemu_driver->domains, iter, data); > } > > +static void > +qemuDomainClearNetBandwidth(virDomainObjPtr vm); > + > static virNWFilterCallbackDriver qemuCallbackDriver = { > .name = QEMU_DRIVER_NAME, > .vmFilterRebuild = qemuVMFilterRebuild, > @@ -2196,6 +2199,9 @@ qemuDomainDestroyFlags(virDomainPtr dom, > if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0) > goto cleanup; > > + /* Clear network bandwidth settings */ > + qemuDomainClearNetBandwidth(vm); > + No. This clears QoS only if the destroy API is called. However, domain may terminate by other means where the QoS should be cleared too, e.g. 'sudo poweroff' caled from within the domain. So this call needs to go into qemuProcessStop.

> qemuDomainSetFakeReboot(driver, vm, false); > > > @@ -17952,6 +17958,21 @@ qemuConnectGetAllDomainStats(virConnectPtr >conn, > return ret; > } > > +/* Clear the bandwidth setting of all the network interfaces of a vm */ > +static void > +qemuDomainClearNetBandwidth(virDomainObjPtr vm) > +{ > + size_t i; > + int actualType; > + > + for (i = 0; i < vm->def->nnets; i++) { > + virDomainNetDefPtr net = vm->def->nets[i]; > + actualType = virDomainNetGetActualType(net); > + if (NETDEVIF_SUPPORT_BANDWIDTH(actualType)) > + virNetDevBandwidthClear(net->ifname); > + } > +} > + Okay. Previously QoS was set on TAP devices only. So there was no need to call virNetDevBandwidthClear as the TAP devices were removed with qemu process tearing down. However, this clears just qemu domains and left LXC containers uncleared.