Re: [libvirt] [PATCH v2 6/8] bandwidth: Create network bandwidth (un)plug functions

Network should be notified if we plug in or unplug an interface, so it can perform some action, e.g. set/unset network part of QoS. However, we are doing this in very early stage, so iface->ifname isn't filled in yet. So whenever we want to report an error, we must use a different identifier, e.g. the MAC address. --- src/Makefile.am | 7 +- src/conf/domain_conf.h | 1 + src/conf/network_conf.c | 21 ++++ src/conf/network_conf.h | 4 + src/libvirt_network.syms | 13 +++ src/libvirt_private.syms | 8 -- src/network/bridge_driver.c | 223 ++++++++++++++++++++++++++++++++++++++++++- 7 files changed, 266 insertions(+), 11 deletions(-) create mode 100644 src/libvirt_network.syms diff --git a/src/Makefile.am b/src/Makefile.am index 01cb995..04378d1 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1373,6 +1373,10 @@ if WITH_ATOMIC_OPS_PTHREAD USED_SYM_FILES += libvirt_atomic.syms endif +if WITH_NETWORK +USED_SYM_FILES += libvirt_network.syms +endif

+ EXTRA_DIST += \ libvirt_public.syms \ libvirt_private.syms \ @@ -1386,7 +1390,8 @@ EXTRA_DIST += \ libvirt_sasl.syms \ libvirt_vmx.syms \ libvirt_xenxs.syms \ - libvirt_libssh2.syms + libvirt_libssh2.syms \ + libvirt_network.syms GENERATED_SYM_FILES = libvirt.syms libvirt.def libvirt_qemu.def diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4ab15e9..b4d149b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -807,6 +807,7 @@ struct _virDomainActualNetDef { virNetDevVPortProfilePtr virtPortProfile; virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; + unsigned int class_id; /* class ID for bandwidth 'floor' */ }; /* Stores the virtual network interface configuration */ diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 2cb9e81..441ccdc 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -47,6 +47,9 @@ #define MAX_BRIDGE_ID 256 #define VIR_FROM_THIS VIR_FROM_NETWORK +#define NEXT_FREE_CLASS_ID 3 +/* currently, /sbin/tc implementation allows up 16 bits for minor class size */ +#define CLASS_ID_BITMAP_SIZE (1<<16) VIR_ENUM_IMPL(virNetworkForward, VIR_NETWORK_FORWARD_LAST, @@ -319,10 +322,28 @@ virNetworkAssignDef(virNetworkObjListPtr nets, virNetworkObjLock(network); network->def = def; + if (!(network->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE))) { + virReportOOMError(); + goto error; + } + + if (virBitmapSetBit(network->class_id, 0) < 0 || + virBitmapSetBit(network->class_id, 1) < 0 || + virBitmapSetBit(network->class_id, 2) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to initialize class id bitmap")); + goto error;

+ } + + network->def = def; nets->objs[nets->count] = network; nets->count++; return network; +error: + virNetworkObjUnlock(network); + virNetworkObjFree(network); + return NULL; } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 3e46304..20f8a51 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -38,6 +38,7 @@ # include "virnetdevvlan.h" # include "virmacaddr.h" # include "device_conf.h" +# include "bitmap.h" enum virNetworkForwardType { VIR_NETWORK_FORWARD_NONE = 0, @@ -221,6 +222,9 @@ struct _virNetworkObj { virNetworkDefPtr def; /* The current definition */ virNetworkDefPtr newDef; /* New definition to activate at shutdown */ + + virBitmapPtr class_id; /* bitmap of class IDs for QoS */ + unsigned long long floor_sum; /* sum of all 'floor'-s of attached NICs */ }; typedef struct _virNetworkObjList virNetworkObjList; diff --git a/src/libvirt_network.syms b/src/libvirt_network.syms new file mode 100644 index 0000000..fc8d415 --- /dev/null +++ b/src/libvirt_network.syms @@ -0,0 +1,13 @@ +# +# Network-specific symbols +# + +# virnetdevbandwidth.h +virNetDevBandwidthClear; +virNetDevBandwidthCopy; +virNetDevBandwidthEqual; +virNetDevBandwidthFree; +virNetDevBandwidthSet; +virNetDevBandwidthPlug; +virNetDevBandwidthUnplug; +virNetDevBandwidthUpdateRate; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 41e2629..f5648a0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1497,14 +1497,6 @@ virNetDevSetOnline; virNetDevValidateConfig; -# virnetdevbandwidth.h -virNetDevBandwidthClear; -virNetDevBandwidthCopy; -virNetDevBandwidthEqual; -virNetDevBandwidthFree; -virNetDevBandwidthSet; - -

# virnetdevbridge.h virNetDevBridgeAddPort; virNetDevBridgeCreate; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 63be5e2..15cbc87 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -121,6 +121,11 @@ static int networkShutdownNetworkExternal(struct network_driver *driver, static void networkReloadIptablesRules(struct network_driver *driver); static void networkRefreshDaemons(struct network_driver *driver); +static int networkPlugBandwidth(virNetworkObjPtr net, + virDomainNetDefPtr iface); +static int networkUnplugBandwidth(virNetworkObjPtr net, + virDomainNetDefPtr iface); + static struct network_driver *driverState = NULL; static char * @@ -3469,6 +3474,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) enum virDomainNetType actualType = iface->type; virNetworkObjPtr network = NULL; virNetworkDefPtr netdef = NULL; + virNetDevBandwidthPtr bandwidth = NULL; virPortGroupDefPtr portgroup = NULL; virNetDevVPortProfilePtr virtport = iface->virtPortProfile; virNetDevVlanPtr vlan = NULL; @@ -3507,7 +3513,13 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) * (already in NetDef). Otherwise, if there is bandwidth info in * the portgroup, fill that into the ActualDef. */ - if (portgroup && !iface->bandwidth) { + + if (iface->bandwidth) + bandwidth = iface->bandwidth; + else if (portgroup && portgroup->bandwidth) + bandwidth = portgroup->bandwidth; + + if (bandwidth) { if (!iface->data.network.actual && (VIR_ALLOC(iface->data.network.actual) < 0)) { virReportOOMError(); @@ -3515,7 +3527,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) } if (virNetDevBandwidthCopy(&iface->data.network.actual->bandwidth, - portgroup->bandwidth) < 0) + bandwidth) < 0) goto error; } @@ -3528,6 +3540,10 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) */ if (iface->data.network.actual) iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_NETWORK; + + if (networkPlugBandwidth(network, iface) < 0) + goto error; + } else if ((netdef->forwardType == VIR_NETWORK_FORWARD_BRIDGE) && netdef->bridge) { @@ -4039,6 +4055,12 @@ networkReleaseActualDevice(virDomainNetDefPtr iface) } netdef = network->def; + if ((netdef->forwardType == VIR_NETWORK_FORWARD_NONE || + netdef->forwardType == VIR_NETWORK_FORWARD_NAT || + netdef->forwardType == VIR_NETWORK_FORWARD_ROUTE) && + networkUnplugBandwidth(network, iface) < 0) + goto error;

+ if ((!iface->data.network.actual) || ((actualType != VIR_DOMAIN_NET_TYPE_DIRECT) && (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV))) { @@ -4242,3 +4264,200 @@ cleanup: error: goto cleanup; } + +/** + * networkCheckBandwidth: + * @net: network QoS + * @iface: interface QoS + * @new_rate: new rate for non guaranteed class + * + * Returns: -1 if plugging would overcommit network QoS + * 0 if plugging is safe (@new_rate updated) + * 1 if no QoS is set (@new_rate untouched) + */ +static int +networkCheckBandwidth(virNetworkObjPtr net, + virDomainNetDefPtr iface, + unsigned long long *new_rate) +{ + int ret = -1; + virNetDevBandwidthPtr netBand = net->def->bandwidth; + virNetDevBandwidthPtr ifaceBand = iface->bandwidth; + unsigned long long tmp_floor_sum = net->floor_sum; + unsigned long long tmp_new_rate = 0; + char ifmac[VIR_MAC_STRING_BUFLEN]; + + if (!ifaceBand || !ifaceBand->in || !ifaceBand->in->floor || + !netBand || !netBand->in) + return 1; + + virMacAddrFormat(&iface->mac, ifmac); + + tmp_new_rate = netBand->in->average; + tmp_floor_sum += ifaceBand->in->floor; + + /* check against peak */ + if (netBand->in->peak) { + tmp_new_rate = netBand->in->peak; + if (tmp_floor_sum > netBand->in->peak) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Cannot plug '%s' interface into '%s' because it " + "would overcommit 'peak' on network '%s'"), + ifmac, + net->def->bridge, + net->def->name); + goto cleanup; + } + } else if (tmp_floor_sum > netBand->in->average) { + /* tmp_floor_sum can be between 'average' and 'peak' iff 'peak' is set. + * Otherwise, tmp_floor_sum must be below 'average'. */ + virReportError(VIR_ERR_OPERATION_INVALID, + _("Cannot plug '%s' interface into '%s' because it " + "would overcommit 'average' on network '%s'"), + ifmac, + net->def->bridge, + net->def->name); + goto cleanup; + } + + *new_rate = tmp_new_rate; + ret = 0; + +cleanup: + return ret; +} + +/** + * networkNextClassID: + * @net: network object + * + * Find next free class ID. @net is supposed + * to be locked already. If there is a free ID, + * it is marked as used and returned. + * + * Returns next free class ID or -1 if none is available. + */ +static ssize_t +networkNextClassID(virNetworkObjPtr net) +{ + size_t ret = 0; + bool is_set = false; + + while (virBitmapGetBit(net->class_id, ret, &is_set) == 0 && is_set) + ret++; + + if (is_set || virBitmapSetBit(net->class_id, ret) < 0) + return -1; + + return ret; +} + +static int +networkPlugBandwidth(virNetworkObjPtr net, + virDomainNetDefPtr iface) +{ + int ret = -1; + int plug_ret; + unsigned long long new_rate = 0; + ssize_t class_id = 0; + char ifmac[VIR_MAC_STRING_BUFLEN]; + + /* XXX iface->ifname is not filled in, it gets allocated later, + * but not in this function. Use MAC in error messages instead */

+ if ((plug_ret = networkCheckBandwidth(net, iface, &new_rate)) < 0) { + /* helper reported error */ + goto cleanup; + } + + if (plug_ret > 0) { + /* no QoS needs to be set; claim success */ + ret = 0; + goto cleanup; + } + + virMacAddrFormat(&iface->mac, ifmac); + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK || + !iface->data.network.actual) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot set bandwidth on interface '%s' of type %d"), + ifmac, iface->type); + goto cleanup; + } + + /* generate new class_id */ + if ((class_id = networkNextClassID(net)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not generate next class ID")); + goto cleanup; + } + + plug_ret = virNetDevBandwidthPlug(net->def->bridge, + net->def->bandwidth, + &iface->mac, + iface->bandwidth, + class_id); + if (plug_ret < 0) { + ignore_value(virNetDevBandwidthUnplug(net->def->bridge, class_id)); + goto cleanup; + } + + if (plug_ret > 0) { + /* Well, this is embarrassing. The networkCheckBandwidth helper + * says we need to set a QoS, but virNetDevBandwidthPlug says + * we don't need to. It's almost certainly a bug then. */ + VIR_WARN("Something strange happened. You may want to upgrade libvirt");

+ ret = 0; + goto cleanup; + } + + /* QoS was set, generate new class ID */ + iface->data.network.actual->class_id = class_id; + /* update sum of 'floor'-s of attached NICs */ + net->floor_sum += iface->bandwidth->in->floor; + /* update rate for non guaranteed NICs */ + new_rate -= net->floor_sum; + if (virNetDevBandwidthUpdateRate(net->def->bridge, "1:2", + net->def->bandwidth, new_rate) < 0) + VIR_WARN("Unable to update rate for 1:2 class on %s bridge", + net->def->bridge);

+ + ret = 0; + +cleanup: + return ret; +} + +static int +networkUnplugBandwidth(virNetworkObjPtr net, + virDomainNetDefPtr iface) +{ + int ret = 0; + unsigned long long new_rate; + + if (iface->data.network.actual && + iface->data.network.actual->class_id) { + /* we must remove class from bridge */ + new_rate = net->def->bandwidth->in->average; + + if (net->def->bandwidth->in->peak > 0) + new_rate = net->def->bandwidth->in->peak; + + ret = virNetDevBandwidthUnplug(net->def->bridge, + iface->data.network.actual->class_id); + if (ret < 0) + goto cleanup; + /* update sum of 'floor'-s of attached NICs */ + net->floor_sum -= iface->bandwidth->in->floor; + /* update rate for non guaranteed NICs */ + new_rate -= net->floor_sum; + if (virNetDevBandwidthUpdateRate(net->def->bridge, "1:2", + net->def->bandwidth, new_rate) < 0) + VIR_WARN("Unable to update rate for 1:2 class on %s bridge", + net->def->bridge);

+ /* no class is associated any longer */ + iface->data.network.actual->class_id = 0; + } + +cleanup: + return ret; +}