[libvirt] [PATCH v2] network: Add network bandwidth support for ethernet interfaces
Anirban Chakraborty
abchak at juniper.net
Mon Oct 6 18:10:09 UTC 2014
On 10/5/14, 11:07 PM, "Martin Kletzander" <mkletzan at redhat.com> 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.
>
>>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. Unfortunately, it cannot be
>done with Set(), I guess, but feel free to correct me if I'm wrong.
Yeah, Clear() could have device definition as input without any
requirement for type, however the same can not be done for Set() as it
needs bandwidth and a boolean (hierarchical_class) parameters. I'll change
Clear() to what you have suggested.
>
>>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>index 117138a..02eb680 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);
>>+
>
>Whey don't you define the function up here in the file somewhere so
>that you don't have to do static prototypes?
Will do.
>
>>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>index d631887..4002afb 100644
>>--- a/src/qemu/qemu_hotplug.c
>>+++ b/src/qemu/qemu_hotplug.c
>>@@ -3517,6 +3522,13 @@ qemuDomainDetachNetDevice(virConnectPtr conn,
>> }
>> }
>>
>>+ actualType = virDomainNetGetActualType(detach);
>>+ if (virNetDevBandwidthClear(detach->ifname, actualType) < 0) {
>>+ virReportError(VIR_ERR_OPERATION_FAILED,
>>+ _("cannot clear bandwidth setting for device :
>>%s"),
>>+ detach->ifname);
>>+ goto cleanup;
>>+ }
>
>This is the only place you are checking for an error and this one
>actually doesn't make much sense. Warning would be better, I guess.
Ok, will change it to warning.
>
>>diff --git a/src/util/virnetdevbandwidth.c
>>b/src/util/virnetdevbandwidth.c
>>index 5fa231a..34a224e 100644
>>--- a/src/util/virnetdevbandwidth.c
>>+++ b/src/util/virnetdevbandwidth.c
>>@@ -245,6 +255,7 @@ virNetDevBandwidthSet(const char *ifname,
>> /**
>> * virNetDevBandwidthClear:
>> * @ifname: on which interface
>>+ * @type: interface tyoe
>
>s/tyoe/type/
>
>The rest looks fine, although having second opinion from anyone else
>more familiar with libvirt's networking would be good. And as I
>mentioned, splitting the patch into 2 or 3 logical ones would be nice.
Ok, I¹ll break up the patch as follows:
1. Change code to prepare it for addition of bandwidth for ethernet type
2. Add code to set and clear bandwidth for ethernet type interface
Anirban
More information about the libvir-list
mailing list