12:52 p.m.
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>
---
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
@@ -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;
-
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,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;
}
+ /* set network bandwidth */
+ if (virNetDevBandwidthSet(def->nets[i]->ifname,
+ virDomainNetGetActualBandwidth(def->nets[i]), false,
+ actualType) < 0)
+ goto cleanup;
(*veths)[(*nveths)-1] = veth;
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
@@ -2082,7 +2082,8 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
}
if (virNetDevBandwidthSet(network->def->bridge,
- network->def->bandwidth, true) < 0)
+ network->def->bandwidth, true,
+ VIR_DOMAIN_NET_TYPE_BRIDGE) < 0)
goto err5;
VIR_FREE(macTapIfName);
@@ -2090,7 +2091,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
return 0;
err5:
- virNetDevBandwidthClear(network->def->bridge);
+ virNetDevBandwidthClear(network->def->bridge, VIR_DOMAIN_NET_TYPE_BRIDGE);
err4:
if (!save_err)
@@ -2137,7 +2138,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
static int networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver
ATTRIBUTE_UNUSED,
virNetworkObjPtr network)
{
- virNetDevBandwidthClear(network->def->bridge);
+ virNetDevBandwidthClear(network->def->bridge, VIR_DOMAIN_NET_TYPE_BRIDGE);
if (network->radvdPid > 0) {
char *radvdpidbase;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index eb72451..a92fb29 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) {
@@ -7392,6 +7387,10 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
if (tapfd[0] < 0)
goto cleanup;
}
+ /* Set bandwidth */
+ if (virNetDevBandwidthSet(net->ifname, virDomainNetGetActualBandwidth(net),
+ false, actualType) < 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 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);
+
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);
+
qemuDomainSetFakeReboot(driver, vm, false);
@@ -10205,7 +10211,8 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
sizeof(*newBandwidth->out));
}
- if (virNetDevBandwidthSet(net->ifname, newBandwidth, false) < 0)
+ if (virNetDevBandwidthSet(net->ifname, newBandwidth, false,
+ virDomainNetGetActualType(net)) < 0)
goto cleanup;
virNetDevBandwidthFree(net->bandwidth);
@@ -18216,6 +18223,19 @@ qemuNodeAllocPages(virConnectPtr conn,
startCell, cellCount, add);
}
+/* Clear the bandwidth setting of all the network interfaces of a vm */
+static void
+qemuDomainClearNetBandwidth(virDomainObjPtr vm)
+{
+ size_t i;
+ int type;
+
+ for (i = 0; i < vm->def->nnets; i++) {
+ type = virDomainNetGetActualType(vm->def->nets[i]);
+ virNetDevBandwidthClear(vm->def->nets[i]->ifname, type);
+ }
+}
+
static virDriver qemuDriver = {
.no = VIR_DRV_QEMU,
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
@@ -947,6 +947,10 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd,
&vhostfdSize) < 0)
goto cleanup;
}
+ /* Set bandwidth */
+ if (virNetDevBandwidthSet(net->ifname, virDomainNetGetActualBandwidth(net),
+ false, actualType) < 0)
+ goto cleanup;
for (i = 0; i < tapfdSize; i++) {
if (virSecurityManagerSetTapFDLabel(driver->securityManager,
@@ -2214,7 +2218,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
if (needBandwidthSet) {
if (virNetDevBandwidthSet(newdev->ifname,
virDomainNetGetActualBandwidth(newdev),
- false) < 0)
+ false, newType) < 0)
goto cleanup;
needReplaceDevDef = true;
}
@@ -3481,6 +3485,7 @@ qemuDomainDetachNetDevice(virConnectPtr conn,
virDomainNetDefPtr detach = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
int rc;
+ int actualType;
if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0)
goto cleanup;
@@ -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;
+ }
qemuDomainMarkDeviceForRemoval(vm, &detach->info);
qemuDomainObjEnterMonitor(driver, vm);
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
@@ -27,6 +27,7 @@
#include "viralloc.h"
#include "virerror.h"
#include "virstring.h"
+#include "domain_conf.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -47,6 +48,7 @@ virNetDevBandwidthFree(virNetDevBandwidthPtr def)
* @ifname: on which interface
* @bandwidth: rates to set (may be NULL)
* @hierarchical_class: whether to create hierarchical class
+ * @type: interface type
*
* This function enables QoS on specified interface
* and set given traffic limits for both, incoming
@@ -60,7 +62,8 @@ virNetDevBandwidthFree(virNetDevBandwidthPtr def)
int
virNetDevBandwidthSet(const char *ifname,
virNetDevBandwidthPtr bandwidth,
- bool hierarchical_class)
+ bool hierarchical_class,
+ int type)
{
int ret = -1;
virCommandPtr cmd = NULL;
@@ -74,7 +77,14 @@ virNetDevBandwidthSet(const char *ifname,
goto cleanup;
}
- virNetDevBandwidthClear(ifname);
+ if (type != VIR_DOMAIN_NET_TYPE_ETHERNET &&
+ type != VIR_DOMAIN_NET_TYPE_BRIDGE &&
+ type != VIR_DOMAIN_NET_TYPE_NETWORK &&
+ type != VIR_DOMAIN_NET_TYPE_DIRECT)
+ /* bandwidth not set for interfaces other than the above */
+ return 0;
+
+ virNetDevBandwidthClear(ifname, type);
if (bandwidth->in && bandwidth->in->average) {
if (virAsprintf(&average, "%llukbps", bandwidth->in->average)
< 0)
@@ -245,6 +255,7 @@ virNetDevBandwidthSet(const char *ifname,
/**
* virNetDevBandwidthClear:
* @ifname: on which interface
+ * @type: interface tyoe
*
* This function tries to disable QoS on specified interface
* by deleting root and ingress qdisc. However, this may fail
@@ -253,12 +264,18 @@ virNetDevBandwidthSet(const char *ifname,
* Return 0 on success, -1 otherwise.
*/
int
-virNetDevBandwidthClear(const char *ifname)
+virNetDevBandwidthClear(const char *ifname, int type)
{
int ret = 0;
int dummy; /* for ignoring the exit status */
virCommandPtr cmd = NULL;
+ if (type != VIR_DOMAIN_NET_TYPE_ETHERNET &&
+ type != VIR_DOMAIN_NET_TYPE_BRIDGE &&
+ type != VIR_DOMAIN_NET_TYPE_NETWORK &&
+ type != VIR_DOMAIN_NET_TYPE_DIRECT)
+ return ret;
+
cmd = virCommandNew(TC);
virCommandAddArgList(cmd, "qdisc", "del", "dev",
ifname, "root", NULL);
diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h
index 4606a07..cc43444 100644
--- a/src/util/virnetdevbandwidth.h
+++ b/src/util/virnetdevbandwidth.h
@@ -43,11 +43,12 @@ struct _virNetDevBandwidth {
void virNetDevBandwidthFree(virNetDevBandwidthPtr def);
-int virNetDevBandwidthSet(const char *ifname,
+int virNetDevBandwidthSet(const char *name,
virNetDevBandwidthPtr bandwidth,
- bool hierarchical_class)
+ bool hierarchical_class,
+ int type)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
-int virNetDevBandwidthClear(const char *ifname)
+int virNetDevBandwidthClear(const char *name, int type)
ATTRIBUTE_NONNULL(1);
int virNetDevBandwidthCopy(virNetDevBandwidthPtr *dest,
const virNetDevBandwidth *src)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index c83341c..956a96b 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -811,7 +811,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
char **res_ifname,
virNetDevVPortProfileOp vmOp,
char *stateDir,
- virNetDevBandwidthPtr bandwidth,
unsigned int flags)
{
const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
@@ -925,14 +924,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
rc = 0;
}
- if (virNetDevBandwidthSet(cr_ifname, bandwidth, false) < 0) {
- if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP)
- VIR_FORCE_CLOSE(rc); /* sets rc to -1 */
- else
- rc = -1;
- goto disassociate_exit;
- }
-
if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_CREATE ||
vmOp == VIR_NETDEV_VPORT_PROFILE_OP_RESTORE) {
/* Only directly register upon a create or restore (restarting
@@ -1076,7 +1067,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname
ATTRIBUTE_UNUSED,
char **res_ifname ATTRIBUTE_UNUSED,
virNetDevVPortProfileOp vmop
ATTRIBUTE_UNUSED,
char *stateDir ATTRIBUTE_UNUSED,
- virNetDevBandwidthPtr bandwidth
ATTRIBUTE_UNUSED,
unsigned int flags)
{
virCheckFlags(0, -1);
diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h
index 41aa4e2..f08d32b 100644
--- a/src/util/virnetdevmacvlan.h
+++ b/src/util/virnetdevmacvlan.h
@@ -68,7 +68,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname,
char **res_ifname,
virNetDevVPortProfileOp vmop,
char *stateDir,
- virNetDevBandwidthPtr bandwidth,
unsigned int flags)
ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(6)
ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(10) ATTRIBUTE_RETURN_CHECK;
diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c
index 384991e..db1f557 100644
--- a/tests/virnetdevbandwidthtest.c
+++ b/tests/virnetdevbandwidthtest.c
@@ -79,7 +79,8 @@ testVirNetDevBandwidthSet(const void *data)
virCommandSetDryRun(&buf, NULL, NULL);
- if (virNetDevBandwidthSet(iface, band, info->hierarchical_class) < 0)
+ if (virNetDevBandwidthSet(iface, band, info->hierarchical_class,
+ VIR_DOMAIN_NET_TYPE_ETHERNET) < 0)
goto cleanup;
if (!(actual_cmd = virBufferContentAndReset(&buf))) {
--
1.9.1
3:51 p.m.
Should I recreate this patch on the top of the latest tree and resubmit?
Or, is there anything that I missed out? Any feedback will be highly
appreciated. Thanks.
Anirban
On 9/26/14, 10:52 AM, "Anirban Chakraborty" <abchak(a)juniper.net> 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>
---
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
@@ -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;
-
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,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;
}
+ /* set network bandwidth */
+ if (virNetDevBandwidthSet(def->nets[i]->ifname,
+ virDomainNetGetActualBandwidth(def->nets[i]), false,
+ actualType) < 0)
+ goto cleanup;
(*veths)[(*nveths)-1] = veth;
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
@@ -2082,7 +2082,8 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr
driver,
}
if (virNetDevBandwidthSet(network->def->bridge,
- network->def->bandwidth, true) < 0)
+ network->def->bandwidth, true,
+ VIR_DOMAIN_NET_TYPE_BRIDGE) < 0)
goto err5;
VIR_FREE(macTapIfName);
@@ -2090,7 +2091,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr
driver,
return 0;
err5:
- virNetDevBandwidthClear(network->def->bridge);
+ virNetDevBandwidthClear(network->def->bridge,
VIR_DOMAIN_NET_TYPE_BRIDGE);
err4:
if (!save_err)
@@ -2137,7 +2138,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr
driver,
static int networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver
ATTRIBUTE_UNUSED,
virNetworkObjPtr network)
{
- virNetDevBandwidthClear(network->def->bridge);
+ virNetDevBandwidthClear(network->def->bridge,
VIR_DOMAIN_NET_TYPE_BRIDGE);
if (network->radvdPid > 0) {
char *radvdpidbase;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index eb72451..a92fb29 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) {
@@ -7392,6 +7387,10 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
if (tapfd[0] < 0)
goto cleanup;
}
+ /* Set bandwidth */
+ if (virNetDevBandwidthSet(net->ifname,
virDomainNetGetActualBandwidth(net),
+ false, actualType) < 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 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);
+
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);
+
qemuDomainSetFakeReboot(driver, vm, false);
@@ -10205,7 +10211,8 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
sizeof(*newBandwidth->out));
}
- if (virNetDevBandwidthSet(net->ifname, newBandwidth, false) < 0)
+ if (virNetDevBandwidthSet(net->ifname, newBandwidth, false,
+ virDomainNetGetActualType(net)) < 0)
goto cleanup;
virNetDevBandwidthFree(net->bandwidth);
@@ -18216,6 +18223,19 @@ qemuNodeAllocPages(virConnectPtr conn,
startCell, cellCount, add);
}
+/* Clear the bandwidth setting of all the network interfaces of a vm */
+static void
+qemuDomainClearNetBandwidth(virDomainObjPtr vm)
+{
+ size_t i;
+ int type;
+
+ for (i = 0; i < vm->def->nnets; i++) {
+ type = virDomainNetGetActualType(vm->def->nets[i]);
+ virNetDevBandwidthClear(vm->def->nets[i]->ifname, type);
+ }
+}
+
static virDriver qemuDriver = {
.no = VIR_DRV_QEMU,
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
@@ -947,6 +947,10 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd,
&vhostfdSize) < 0)
goto cleanup;
}
+ /* Set bandwidth */
+ if (virNetDevBandwidthSet(net->ifname,
virDomainNetGetActualBandwidth(net),
+ false, actualType) < 0)
+ goto cleanup;
for (i = 0; i < tapfdSize; i++) {
if (virSecurityManagerSetTapFDLabel(driver->securityManager,
@@ -2214,7 +2218,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
if (needBandwidthSet) {
if (virNetDevBandwidthSet(newdev->ifname,
virDomainNetGetActualBandwidth(newdev),
- false) < 0)
+ false, newType) < 0)
goto cleanup;
needReplaceDevDef = true;
}
@@ -3481,6 +3485,7 @@ qemuDomainDetachNetDevice(virConnectPtr conn,
virDomainNetDefPtr detach = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
int rc;
+ int actualType;
if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0)
goto cleanup;
@@ -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;
+ }
qemuDomainMarkDeviceForRemoval(vm, &detach->info);
qemuDomainObjEnterMonitor(driver, vm);
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
@@ -27,6 +27,7 @@
#include "viralloc.h"
#include "virerror.h"
#include "virstring.h"
+#include "domain_conf.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -47,6 +48,7 @@ virNetDevBandwidthFree(virNetDevBandwidthPtr def)
* @ifname: on which interface
* @bandwidth: rates to set (may be NULL)
* @hierarchical_class: whether to create hierarchical class
+ * @type: interface type
*
* This function enables QoS on specified interface
* and set given traffic limits for both, incoming
@@ -60,7 +62,8 @@ virNetDevBandwidthFree(virNetDevBandwidthPtr def)
int
virNetDevBandwidthSet(const char *ifname,
virNetDevBandwidthPtr bandwidth,
- bool hierarchical_class)
+ bool hierarchical_class,
+ int type)
{
int ret = -1;
virCommandPtr cmd = NULL;
@@ -74,7 +77,14 @@ virNetDevBandwidthSet(const char *ifname,
goto cleanup;
}
- virNetDevBandwidthClear(ifname);
+ if (type != VIR_DOMAIN_NET_TYPE_ETHERNET &&
+ type != VIR_DOMAIN_NET_TYPE_BRIDGE &&
+ type != VIR_DOMAIN_NET_TYPE_NETWORK &&
+ type != VIR_DOMAIN_NET_TYPE_DIRECT)
+ /* bandwidth not set for interfaces other than the above */
+ return 0;
+
+ virNetDevBandwidthClear(ifname, type);
if (bandwidth->in && bandwidth->in->average) {
if (virAsprintf(&average, "%llukbps", bandwidth->in->average)
<
0)
@@ -245,6 +255,7 @@ virNetDevBandwidthSet(const char *ifname,
/**
* virNetDevBandwidthClear:
* @ifname: on which interface
+ * @type: interface tyoe
*
* This function tries to disable QoS on specified interface
* by deleting root and ingress qdisc. However, this may fail
@@ -253,12 +264,18 @@ virNetDevBandwidthSet(const char *ifname,
* Return 0 on success, -1 otherwise.
*/
int
-virNetDevBandwidthClear(const char *ifname)
+virNetDevBandwidthClear(const char *ifname, int type)
{
int ret = 0;
int dummy; /* for ignoring the exit status */
virCommandPtr cmd = NULL;
+ if (type != VIR_DOMAIN_NET_TYPE_ETHERNET &&
+ type != VIR_DOMAIN_NET_TYPE_BRIDGE &&
+ type != VIR_DOMAIN_NET_TYPE_NETWORK &&
+ type != VIR_DOMAIN_NET_TYPE_DIRECT)
+ return ret;
+
cmd = virCommandNew(TC);
virCommandAddArgList(cmd, "qdisc", "del", "dev",
ifname, "root",
NULL);
diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h
index 4606a07..cc43444 100644
--- a/src/util/virnetdevbandwidth.h
+++ b/src/util/virnetdevbandwidth.h
@@ -43,11 +43,12 @@ struct _virNetDevBandwidth {
void virNetDevBandwidthFree(virNetDevBandwidthPtr def);
-int virNetDevBandwidthSet(const char *ifname,
+int virNetDevBandwidthSet(const char *name,
virNetDevBandwidthPtr bandwidth,
- bool hierarchical_class)
+ bool hierarchical_class,
+ int type)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
-int virNetDevBandwidthClear(const char *ifname)
+int virNetDevBandwidthClear(const char *name, int type)
ATTRIBUTE_NONNULL(1);
int virNetDevBandwidthCopy(virNetDevBandwidthPtr *dest,
const virNetDevBandwidth *src)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index c83341c..956a96b 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -811,7 +811,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char
*tgifname,
char **res_ifname,
virNetDevVPortProfileOp vmOp,
char *stateDir,
- virNetDevBandwidthPtr
bandwidth,
unsigned int flags)
{
const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
@@ -925,14 +924,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const
char *tgifname,
rc = 0;
}
- if (virNetDevBandwidthSet(cr_ifname, bandwidth, false) < 0) {
- if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP)
- VIR_FORCE_CLOSE(rc); /* sets rc to -1 */
- else
- rc = -1;
- goto disassociate_exit;
- }
-
if (vmOp == VIR_NETDEV_VPORT_PROFILE_OP_CREATE ||
vmOp == VIR_NETDEV_VPORT_PROFILE_OP_RESTORE) {
/* Only directly register upon a create or restore (restarting
@@ -1076,7 +1067,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const
char *ifname ATTRIBUTE_UNUSED,
char **res_ifname
ATTRIBUTE_UNUSED,
virNetDevVPortProfileOp vmop
ATTRIBUTE_UNUSED,
char *stateDir
ATTRIBUTE_UNUSED,
- virNetDevBandwidthPtr
bandwidth ATTRIBUTE_UNUSED,
unsigned int flags)
{
virCheckFlags(0, -1);
diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h
index 41aa4e2..f08d32b 100644
--- a/src/util/virnetdevmacvlan.h
+++ b/src/util/virnetdevmacvlan.h
@@ -68,7 +68,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char
*ifname,
char **res_ifname,
virNetDevVPortProfileOp vmop,
char *stateDir,
- virNetDevBandwidthPtr
bandwidth,
unsigned int flags)
ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(6)
ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(10) ATTRIBUTE_RETURN_CHECK;
diff --git a/tests/virnetdevbandwidthtest.c
b/tests/virnetdevbandwidthtest.c
index 384991e..db1f557 100644
--- a/tests/virnetdevbandwidthtest.c
+++ b/tests/virnetdevbandwidthtest.c
@@ -79,7 +79,8 @@ testVirNetDevBandwidthSet(const void *data)
virCommandSetDryRun(&buf, NULL, NULL);
- if (virNetDevBandwidthSet(iface, band, info->hierarchical_class) < 0)
+ if (virNetDevBandwidthSet(iface, band, info->hierarchical_class,
+ VIR_DOMAIN_NET_TYPE_ETHERNET) < 0)
goto cleanup;
if (!(actual_cmd = virBufferContentAndReset(&buf))) {
--
1.9.1
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
1:07 a.m.
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>
---
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.
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?
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.
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.
Martin
1:10 p.m.
On 10/5/14, 11:07 PM, "Martin Kletzander" <mkletzan(a)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(a)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
1:16 p.m.
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>
>
> ---
> 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?
Unfortunately, it cannot be
done with Set(), I guess, but feel free to correct me if I'm wrong.
> 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?
> 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.
> 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.
Martin
3:11 p.m.
On 10/6/14, 11:16 AM, "Laine Stump" <laine(a)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(a)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
3734
days inactive
3744
days old
5 comments
3 participants
participants (3)
-
Anirban Chakraborty -
Laine Stump -
Martin Kletzander