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

Re: [libvirt] [PATCH] network: Add bandwidth support to ethernet interface




On 11/3/14, 2:58 AM, "Michal Privoznik" <mprivozn redhat com> wrote:

>On 30.10.2014 00:56, Anirban Chakraborty wrote:
>><snip>
>>
>> +static inline bool virNetDevSupportBandwidth(virDomainNetType type)
>> +{
>> +    switch (type) {
>> +    case VIR_DOMAIN_NET_TYPE_BRIDGE:
>> +    case VIR_DOMAIN_NET_TYPE_NETWORK:
>> +    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_INTERNAL:
>> +    case VIR_DOMAIN_NET_TYPE_HOSTDEV:
>> +    case VIR_DOMAIN_NET_TYPE_LAST:
>> +        break;
>> +    }
>> +    return false;
>> +}
>> +
>
>I've got a feeling that this should go to src/util/virnetdevbandwidth*
>among with the rest of virNetDevBandwitdh functions.

I thought about moving this and the qemuDomainClearNetBandwidth() to
src/util/virnetdevbandwidth.h earlier, however, these functions need
reference to virDomainNetType and virDomainObjPtr which are defined in
conf/domain_conf.h and apparently src/util/*.h can not have any reference
to files from conf/.

>
>>   #endif /* __DOMAIN_CONF_H */
>> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
>> index 979382b..8a21af4 100644
>> --- a/src/lxc/lxc_driver.c
>> +++ b/src/lxc/lxc_driver.c
>> @@ -72,6 +72,7 @@
>>   #include "viraccessapicheck.h"
>>   #include "viraccessapichecklxc.h"
>>   #include "virhostdev.h"
>> +#include "qemu/qemu_command.h"
>
>This is not allowed. In case somebody is building with LXC but without
>QEMU ..

Thanks for pointing it out.

>
>>
>>   #define VIR_FROM_THIS VIR_FROM_LXC
>>
>> @@ -4634,6 +4635,8 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm,
>>
>>       detach = vm->def->nets[detachidx];
>>
>> +    qemuDomainClearNetBandwidth(vm);
>> +
>
>.. this is going to be an undefined reference.
>
>>       switch (virDomainNetGetActualType(detach)) {
>>       case VIR_DOMAIN_NET_TYPE_BRIDGE:
>>       case VIR_DOMAIN_NET_TYPE_NETWORK:
>> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
>> index ed30c37..3192011 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;
>> -
>
>
>No, this function is called from two places:
>1) when domain is starting up
>2) on NIC hotplug
>
>you are covering 1) but removing already working 2). We can't lose that
>functionality.

Got it. Thanks.

>
>>       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,14 +325,13 @@ 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)
>> +            cfg->stateDir, 0) < 0)
>>           goto cleanup;
>>
>
>Same comment applies here.

Thanks.

>
>>       ret = res_ifname;
>> @@ -450,6 +445,11 @@ static int
>>virLXCProcessSetupInterfaces(virConnectPtr conn,
>>               goto cleanup;
>>           }
>>
>> +        /* set network bandwidth */
>> +        if (virNetDevBandwidthSet(def->nets[i]->ifname,
>> +                virDomainNetGetActualBandwidth(def->nets[i]), false) <
>>0)
>> +           goto cleanup;
>> +
>
>Shouldn't this be guarded with virNetDevSupportBandwidth()? The problem
>is, there's a switch() just before this where all the unsupported NIC
>types are rejected (ETHERNET is rejected too btw). What will come
>through is DIRECT type. I'm not saying that we should not set bandwidth
>there, but this patch aims at ethernet.

Agreed. Will take care of it next version of the patch.

>
>>           (*veths)[(*nveths)-1] = veth;
>>
>>           /* Make sure all net definitions will have a name in the
>>container */
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 2e5af4f..7922672 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,11 +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) {
>>           goto cleanup;
>> @@ -7427,6 +7421,13 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>>               goto cleanup;
>>       }
>>
>> +    /* Set Bandwidth */
>> +    if (virNetDevSupportBandwidth(actualType) &&
>> +           virNetDevBandwidthSet(net->ifname,
>> +           virDomainNetGetActualBandwidth(net),
>> +           false) < 0)
>> +        goto cleanup;
>
>There's no guarantee that net->ifname is filled in here:
>
>virsh # start migt10
>error: Failed to start domain migt10
>error: internal error: Child process (/sbin/tc qdisc add dev) unexpected
>exit status 255: Command line is not complete. Try option "help"

I will check for empty string here.

>
>And I have to mention weird code formatting.

I am assuming that you are referring to misaligned arguments to function
virNetDevSupportBandwidth() above. They should follow the first char of
the opening parentheses, right?

>
>> +
>>       if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>>            actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>>            actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
>> @@ -12463,3 +12464,15 @@ virDomainDefPtr
>>qemuParseCommandLinePid(virCapsPtr qemuCaps,
>>       virStringFreeList(progenv);
>>       return def;
>>   }
>> +
>> +void qemuDomainClearNetBandwidth(virDomainObjPtr vm)
>> +{
>> +    size_t i;
>> +    virDomainNetType type;
>> +
>> +    for (i = 0; i < vm->def->nnets; i++) {
>> +        type = virDomainNetGetActualType(vm->def->nets[i]);
>> +        if (virNetDevSupportBandwidth(type))
>> +            virNetDevBandwidthClear(vm->def->nets[i]->ifname);
>> +    }
>> +}
>
>Having this in qemu specific code feels strange, esp. when the code is
>to be called from other drivers too. How about moving it under
>src/util/virnetdevbandwidth*?

As mentioned above, I was not sure if I could put them in the file you
mentioned because virDomaiObjPtr will need reference to conf/domain_conf.h
file and I think that file can not be included in
src/util/virnetdevbandwidth.*.


>
>> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
>> index aa40c9e..7963a91 100644
>> --- a/src/qemu/qemu_command.h
>> +++ b/src/qemu/qemu_command.h
>> @@ -277,4 +277,6 @@ int qemuCheckDiskConfig(virDomainDiskDefPtr disk);
>>
>>   bool
>>   qemuCheckFips(void);
>> +
>> +void qemuDomainClearNetBandwidth(virDomainObjPtr vm);
>>   #endif /* __QEMU_COMMAND_H__*/
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 2eaf77d..6ef1132 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -2196,6 +2196,9 @@ qemuDomainDestroyFlags(virDomainPtr dom,
>>       if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0)
>>           goto cleanup;
>>
>> +    /* Clear network bandwidth */
>> +    qemuDomainClearNetBandwidth(vm);
>> +
>
>This is not needed. qemuProcessStop() will take care of that.

Ok, thanks.

>
>>       qemuDomainSetFakeReboot(driver, vm, false);
>>
>>
>><snip>
>
>BTW: it would be nice if you can version you patches. I mean, this is
>what, 4th or 5th version? Say that in subject explicitly please. You
>know, in the prefix: [PATCH v5] network: ...

I was doing it earlier and then dropped it. I¹ll resin the patch
addressing all your comments and send it out. However, please let me know
if I should move the above functions (virNetDevBandwidthSet etc.) in
src/util/virnetdevbandwidth.* and add #include "conf/domain_conf.h" in
virnetdevbandwidth.h file.

Thanks,
Anirban



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