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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 10/6/14, 3:43 AM, "Michal Privoznik" <mprivozn@xxxxxxxxxx> wrote:

>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@xxxxxxxxxxx>
>> ---
>>   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).

Will do.

>
>>   /* 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.

Thanks for pointing it out. I am taking care of it in my next patch.

>
>>       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.
Yeah, this was not right, for some reason it escaped my attention.

>
>> +           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.

Got it, thanks for catching it.

>
>>       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.

I¹ll take care of it in my next patch. Thanks.

Anirban


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]