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

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(a)juniper.net&gt; >> >> --- >> 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...

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

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?