[libvirt] [PATCH v2] network: Add network bandwidth support for ethernet interfaces

[Anirban Chakraborty]

On 10/6/14, 11:16 AM, "Laine Stump" <laine at laine.org> wrote:

>On 10/06/2014 02:07 AM, Martin Kletzander wrote:
>> On Fri, Sep 26, 2014 at 10:52:57AM -0700, Anirban Chakraborty wrote:
>>> V2:
>>> Addressed comments raised in review of V1.
>>> 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 at juniper.net>
>>>
>>> ---
>>> src/lxc/lxc_process.c          | 26 +++++++++++++-------------
>>> src/network/bridge_driver.c    |  7 ++++---
>>> src/qemu/qemu_command.c        |  9 ++++-----
>>> src/qemu/qemu_driver.c         | 22 +++++++++++++++++++++-
>>> src/qemu/qemu_hotplug.c        | 14 +++++++++++++-
>>> src/util/virnetdevbandwidth.c  | 23 ++++++++++++++++++++---
>>> src/util/virnetdevbandwidth.h  |  7 ++++---
>>> src/util/virnetdevmacvlan.c    | 10 ----------
>>> src/util/virnetdevmacvlan.h    |  1 -
>>> tests/virnetdevbandwidthtest.c |  3 ++-
>>> 10 files changed, 81 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
>>> index ed30c37..7f7e4ad 100644
>>> --- a/src/lxc/lxc_process.c
>>> +++ b/src/lxc/lxc_process.c
>>> @@ -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,14 @@ 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;
>>>         }
>>
>> Hunks like these (that do not have any functional impact) could be
>> separated to ease the review.
>
>I second that - sometimes I spend time trying to figure out why a change
>was needed, and when I don't find a reason I assume that I must be
>confused about something...

I¹ll break up the patch into two pieces (one for the refactoring part and
the other to add the support to ethernet type interfaces), which should
ease out these things.

>
>>
>>
>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>>> index 979fb13..2e1f821 100644
>>> --- a/src/network/bridge_driver.c
>>> +++ b/src/network/bridge_driver.c
>>> @@ -2090,7 +2091,7 @@
>>> networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
>>>     return 0;
>>>
>>>  err5:
>>> -    virNetDevBandwidthClear(network->def->bridge);
>>> +    virNetDevBandwidthClear(network->def->bridge,
>>> VIR_DOMAIN_NET_TYPE_BRIDGE);
>>
>> You could change the virNetDevBandwidthClear() function to take the
>> device definition as an argument and you wouldn't have to supply
>> additional information for that device.
>
>Actually you can't do that, because functions in the util directory
>cannot #include anything from the conf directory.
>
>For that matter, even what you've done here (using
>VIR_DOMAIN_NET_TYPE_*) is illegal, for the same reason. It looks like
>you're only using the type to decide if you want to return early. Going
>quickly through the places where virNetDevBandwidth(Set|Clear) are
>called, I think in most cases you wouldn't even get there if you didn't
>have the right kind of interface, so you can likely just add in the
>check in the couple of places where it is ambiguous.

Ok, will do that. Thanks for pointing it out.

>
>BTW, I notice that you're allowing setting of bandwidth for macvtap
>interfaces (VIR_DOMAIN_NET_TYPE_DIRECT). This was not originally
>supported by the macvtap code in the kernels, although it has recently
>been added (e.g. it is in Fedora 20, but not in RHEL7.0 or CentOS7.0).
>Was this intentional, or accidental?

It appears to me that the existing code sets bandwidth for
VIR_DOMAIN_NET_TYPE_DIRECT device type. qemuBuildInterfaceCommandLine()
has code to create macvtap device (qemuPhysIfaceConnect) for
VIR_DOMAIN_NET_TYPE_DIRECT. Subsequently, qemuPhysIfaceConnect() calls
virNetDevMacVLanCreateWithVPortProfile(), which calls
virNetDevBandwidthSet() to set the bandwidth.

Anirban