This is however supported only on domain interfaces with type='network'. Moreover, target network needs to have at least inbound QoS set. This is required by hierarchical traffic shaping. >From now on, the required attribute for <inbound/> is either 'average' (old) or 'floor' (new). This new attribute can be used just for interfaces type of network (<interface type='network'/>) currently. --- docs/formatdomain.html.in | 20 ++++++++++++-- docs/schemas/networkcommon.rng | 5 +++ src/conf/domain_conf.c | 6 +++- src/conf/netdev_bandwidth_conf.c | 54 +++++++++++++++++++++++++++++++++----- src/conf/netdev_bandwidth_conf.h | 3 +- src/conf/network_conf.c | 4 +- src/util/virnetdevbandwidth.h | 1 + 7 files changed, 78 insertions(+), 15 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c8da33d..da8184a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3047,7 +3047,7 @@ qemu-kvm -net nic,model=? /dev/null &lt;source network='default'/&gt; &lt;target dev='vnet0'/&gt; <b>&lt;bandwidth&gt; - &lt;inbound average='1000' peak='5000' burst='1024'/&gt; + &lt;inbound average='1000' peak='5000' floor='200' burst='1024'/&gt; &lt;outbound average='128' peak='256' burst='256'/&gt; &lt;/bandwidth&gt;</b> &lt;/interface&gt; @@ -3062,14 +3062,28 @@ qemu-kvm -net nic,model=? /dev/null children element out result in no QoS applied on that traffic direction. So, when you want to shape only domain's incoming traffic, use <code>inbound</code> only, and vice versa. Each of these elements have one - mandatory attribute <code>average</code>. It specifies average bit rate on + mandatory attribute <code>average</code> (or <code>floor</code> as + described below). It specifies average bit rate on

interface being shaped. Then there are two optional attributes:

<code>peak</code>, which specifies maximum rate at which interface can send data, and <code>burst</code>, amount of bytes that can be burst at <code>peak</code> speed. Accepted values for attributes are integer numbers. The units for <code>average</code> and <code>peak</code> attributes are kilobytes per second, and for the <code>burst</code> just kilobytes. - <span class="since">Since 0.9.4</span> + <span class="since">Since 0.9.4</span> The <code>inbound</code> can + optionally have <code>floor</code> attribute. This is there for + guaranteeing minimal throughput for shaped interfaces. This, however, + requires that all traffic goes through one point where QoS decisions can + take place. That's why this attribute works only for virtual networks for + now (that is <code>&lt;interface type='network'/&gt;</code>.

Moreover, the + virtual network the interface is connected to is required to have at least + inbound Qos set (<code>average</code> at least). Moreover, with + <code>floor<code> attribute users don't need to specify + <code>average</code>. However, <code>peak</code> and <code>burst</code> + attributes still require <code>average</code>. Currently, linux kernel + doesn't allow egress qdiscs to have any classes therefore + <code>floor</code> can be applied only on <code>inbound</code> and not + </code>outbound</code>. <span class="since">Since 1.0.1</span> </p> <h5><a name="elementVlanTag">Setting VLAN tag (on supported network types only)</a></h5> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng index c7749e7..51ff759 100644 --- a/docs/schemas/networkcommon.rng +++ b/docs/schemas/networkcommon.rng @@ -149,6 +149,11 @@ </attribute> </optional> <optional> + <attribute name="floor"> + <ref name="speed"/> + </attribute> + </optional> + <optional> <attribute name='burst'> <ref name="BurstSize"/> </attribute> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 99f03a9..bf23b77 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4742,7 +4742,8 @@ virDomainActualNetDefParseXML(xmlNodePtr node, bandwidth_node = virXPathNode("./bandwidth", ctxt); if (bandwidth_node && - !(actual->bandwidth = virNetDevBandwidthParse(bandwidth_node))) + !(actual->bandwidth = virNetDevBandwidthParse(bandwidth_node, + actual->type))) goto error; vlanNode = virXPathNode("./vlan", ctxt); @@ -4930,7 +4931,8 @@ virDomainNetDefParseXML(virCapsPtr caps, goto error; } } else if (xmlStrEqual(cur->name, BAD_CAST "bandwidth")) { - if (!(def->bandwidth = virNetDevBandwidthParse(cur))) + if (!(def->bandwidth = virNetDevBandwidthParse(cur, + def->type)))

goto error; } else if (xmlStrEqual(cur->name, BAD_CAST "vlan")) { if (virNetDevVlanParse(cur, ctxt, &def->vlan) < 0) diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c index 261718f..38c5a5b 100644 --- a/src/conf/netdev_bandwidth_conf.c +++ b/src/conf/netdev_bandwidth_conf.c @@ -26,6 +26,7 @@ #include "virterror_internal.h" #include "util.h" #include "memory.h" +#include "domain_conf.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -36,6 +37,7 @@ virNetDevBandwidthParseRate(xmlNodePtr node, virNetDevBandwidthRatePtr rate) char *average = NULL; char *peak = NULL; char *burst = NULL; + char *floor = NULL; if (!node || !rate) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -46,6 +48,7 @@ virNetDevBandwidthParseRate(xmlNodePtr node, virNetDevBandwidthRatePtr rate) average = virXMLPropString(node, "average"); peak = virXMLPropString(node, "peak"); burst = virXMLPropString(node, "burst"); + floor = virXMLPropString(node, "floor"); if (average) { if (virStrToLong_ull(average, NULL, 10, &rate->average) < 0) { @@ -54,9 +57,15 @@ virNetDevBandwidthParseRate(xmlNodePtr node, virNetDevBandwidthRatePtr rate) average); goto cleanup; } - } else { + } else if (!floor) { virReportError(VIR_ERR_XML_DETAIL, "%s", - _("Missing mandatory average attribute")); + _("Missing mandatory average or floor attributes")); + goto cleanup; + } + + if ((peak || burst) && !average) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'peak' and 'burst' require 'average' attribute")); goto cleanup; }

@@ -74,12 +83,20 @@ virNetDevBandwidthParseRate(xmlNodePtr node, virNetDevBandwidthRatePtr rate) goto cleanup; } + if (floor && virStrToLong_ull(floor, NULL, 10, &rate->floor) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("could not convert %s"),

+ floor); + goto cleanup; + } + ret = 0; cleanup: VIR_FREE(average); VIR_FREE(peak); VIR_FREE(burst); + VIR_FREE(floor); return ret; } @@ -87,13 +104,17 @@ cleanup: /** * virNetDevBandwidthParse: * @node: XML node + * @net_type: one of virDomainNetType * - * Parse bandwidth XML and return pointer to structure + * Parse bandwidth XML and return pointer to structure. + * @net_type tell to which type will/is interface connected to. + * Pass -1 if this is not called on interface. * * Returns !NULL on success, NULL on error. */ virNetDevBandwidthPtr -virNetDevBandwidthParse(xmlNodePtr node) +virNetDevBandwidthParse(xmlNodePtr node, + int net_type) { virNetDevBandwidthPtr def = NULL; xmlNodePtr cur = node->children; @@ -144,6 +165,13 @@ virNetDevBandwidthParse(xmlNodePtr node) /* helper reported error for us */ goto error; } + + if (def->in->floor && net_type != VIR_DOMAIN_NET_TYPE_NETWORK) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("floor attribute is supported only for " + "interfaces of type network"));

+ goto error; + } } if (out) { @@ -156,6 +184,13 @@ virNetDevBandwidthParse(xmlNodePtr node) /* helper reported error for us */ goto error; } + + if (def->out->floor) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'floor' attribute allowed " + "only in <inbound> element")); + goto error; + } } return def; @@ -175,13 +210,18 @@ virNetDevBandwidthRateFormat(virNetDevBandwidthRatePtr def, if (!def) return 0; - if (def->average) { - virBufferAsprintf(buf, " <%s average='%llu'", elem_name, - def->average); + if (def->average || def->floor) { + virBufferAsprintf(buf, " <%s", elem_name); + + if (def->average) + virBufferAsprintf(buf, " average='%llu'", def->average); if (def->peak) virBufferAsprintf(buf, " peak='%llu'", def->peak); + if (def->floor) + virBufferAsprintf(buf, " floor='%llu'", def->floor); + if (def->burst) virBufferAsprintf(buf, " burst='%llu'", def->burst); virBufferAddLit(buf, "/>\n"); diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h index bca5c50..0080165 100644 --- a/src/conf/netdev_bandwidth_conf.h +++ b/src/conf/netdev_bandwidth_conf.h @@ -28,7 +28,8 @@ # include "buf.h" # include "xml.h" -virNetDevBandwidthPtr virNetDevBandwidthParse(xmlNodePtr node) +virNetDevBandwidthPtr virNetDevBandwidthParse(xmlNodePtr node, + int net_type) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int virNetDevBandwidthFormat(virNetDevBandwidthPtr def, virBufferPtr buf) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 228951d..41831e0 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1186,7 +1186,7 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def, bandwidth_node = virXPathNode("./bandwidth", ctxt); if (bandwidth_node && - !(def->bandwidth = virNetDevBandwidthParse(bandwidth_node))) { + !(def->bandwidth = virNetDevBandwidthParse(bandwidth_node, -1))) { goto error; } @@ -1262,7 +1262,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->domain = virXPathString("string(./domain[1]/@name)", ctxt); if ((bandwidthNode = virXPathNode("./bandwidth", ctxt)) != NULL && - (def->bandwidth = virNetDevBandwidthParse(bandwidthNode)) == NULL) + (def->bandwidth = virNetDevBandwidthParse(bandwidthNode, -1)) == NULL) goto error; vlanNode = virXPathNode("./vlan", ctxt); diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h index e046230..35f8b89 100644 --- a/src/util/virnetdevbandwidth.h +++ b/src/util/virnetdevbandwidth.h @@ -30,6 +30,7 @@ typedef virNetDevBandwidthRate *virNetDevBandwidthRatePtr; struct _virNetDevBandwidthRate { unsigned long long average; /* kbytes/s */ unsigned long long peak; /* kbytes/s */ + unsigned long long floor; /* kbytes/s */ unsigned long long burst; /* kbytes */ };

On 11/19/2012 11:51 AM, Michal Privoznik wrote: > These classes can borrow unused bandwidth. Basically, > only egress qdsics can have classes, therefore we can > do this kind of traffic shaping only on host's outgoing, > that is domain's incoming traffic. > --- > src/lxc/lxc_process.c | 3 ++- > src/network/bridge_driver.c | 3 ++- > src/qemu/qemu_command.c | 3 ++- > src/qemu/qemu_driver.c | 2 +- > src/util/virnetdevbandwidth.c | 33 +++++++++++++++++++++++++++------ > src/util/virnetdevbandwidth.h | 4 +++- > src/util/virnetdevmacvlan.c | 2 +- > 7 files changed, 38 insertions(+), 12 deletions(-) > > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index 079bc3a..91ce2d3 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -335,7 +335,8 @@ static int virLXCProcessSetupInterfaceBridged(virConnectPtr conn, > goto cleanup; > > if (virNetDevBandwidthSet(net->ifname, > - virDomainNetGetActualBandwidth(net)) < 0) { > + virDomainNetGetActualBandwidth(net), > + false) < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("cannot set bandwidth limits on %s"), > net->ifname); > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index c153d36..256b601 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -2240,7 +2240,8 @@ networkStartNetworkVirtual(struct network_driver *driver, > VIR_FORCE_CLOSE(tapfd); > } > > - if (virNetDevBandwidthSet(network->def->bridge, network->def->bandwidth) < 0) { > + if (virNetDevBandwidthSet(network->def->bridge, > + network->def->bandwidth, true) < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("cannot set bandwidth limits on %s"), > network->def->bridge); > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 22bb209..c1f8680 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -292,7 +292,8 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, > > if (tapfd >= 0 && > virNetDevBandwidthSet(net->ifname, > - virDomainNetGetActualBandwidth(net)) < 0) { > + virDomainNetGetActualBandwidth(net), > + false) < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("cannot set bandwidth limits on %s"), > net->ifname); > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 595c452..9ae0c1b 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -8960,7 +8960,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, > sizeof(*newBandwidth->out)); > } > > - if (virNetDevBandwidthSet(net->ifname, newBandwidth) < 0) { > + if (virNetDevBandwidthSet(net->ifname, newBandwidth, false) < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("cannot set bandwidth limits on %s"), > device); > diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c > index bddb788..fcc6b56 100644 > --- a/src/util/virnetdevbandwidth.c > +++ b/src/util/virnetdevbandwidth.c > @@ -45,17 +45,21 @@ virNetDevBandwidthFree(virNetDevBandwidthPtr def) > * virNetDevBandwidthSet: > * @ifname: on which interface > * @bandwidth: rates to set (may be NULL) > + * @hierarchical_class: whether to create hierarchical class > * > * This function enables QoS on specified interface > * and set given traffic limits for both, incoming > * and outgoing traffic. Any previous setting get > - * overwritten. > + * overwritten. If @hierarchical_class is TRUE, create > + * hierarchical class. It is used to guarantee minimal > + * throughput ('floor' attribute in NIC). > * > * Return 0 on success, -1 otherwise. > */ > int > virNetDevBandwidthSet(const char *ifname, > - virNetDevBandwidthPtr bandwidth) > + virNetDevBandwidthPtr bandwidth, > + bool hierarchical_class) > { > int ret = -1; > virCommandPtr cmd = NULL; > @@ -71,7 +75,7 @@ virNetDevBandwidthSet(const char *ifname, > > virNetDevBandwidthClear(ifname); > > - if (bandwidth->in) { > + if (bandwidth->in && bandwidth->in->average) { > if (virAsprintf(&average, "%llukbps", bandwidth->in->average) < 0) > goto cleanup; > if (bandwidth->in->peak && > @@ -83,15 +87,32 @@ virNetDevBandwidthSet(const char *ifname, > > cmd = virCommandNew(TC); > virCommandAddArgList(cmd, "qdisc", "add", "dev", ifname, "root", > - "handle", "1:", "htb", "default", "1", NULL); > + "handle", "1:", "htb", "default", > + hierarchical_class ? "2" : "1", NULL); Don't you really only need to do this if floor has been set? If you create the hierarchical class and don't need it, does this result in any extra overhead? If so, I think you should do (hierarchical_class && bandwidth->floor).

> if (virCommandRun(cmd, NULL) < 0) > goto cleanup; > > + if (hierarchical_class) { similar > + /* If we are creating hierarchical class, all non guaranteed traffic > + * goes to 1:2 class which will adjust 'rate' dynamically as NICs with > + * guaranteed throughput are plugged and unplugged. Class 1:1 is there > + * so we don't exceed the maximum limit for network. For each NIC with > + * guaranteed throughput a separate classid will be created. > + * NB '1:' is just a shorter notation of '1:0' */ I have to admit that I don't follow the full discussion of the details of this, but trust that you've done some testing to verify that the results are as desired when floor is specified, and identical to previous code if floor isn't specified.

> + virCommandFree(cmd); > + cmd = virCommandNew(TC); > + virCommandAddArgList(cmd, "class", "add", "dev", ifname, "parent", > + "1:", "classid", "1:1", "htb", "rate", average, > + "ceil", peak ? peak : average, NULL); > + if (virCommandRun(cmd, NULL) < 0) > + goto cleanup; > + } > virCommandFree(cmd); > cmd = virCommandNew(TC); > virCommandAddArgList(cmd,"class", "add", "dev", ifname, "parent", > - "1:", "classid", "1:1", "htb", NULL); > - virCommandAddArgList(cmd, "rate", average, NULL); > + hierarchical_class ? "1:1" : "1:", "classid", > + hierarchical_class ? "1:2" : "1:1", "htb", > + "rate", average, NULL); > > if (peak) > virCommandAddArgList(cmd, "ceil", peak, NULL); > diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h > index 35f8b89..d308ab2 100644 > --- a/src/util/virnetdevbandwidth.h > +++ b/src/util/virnetdevbandwidth.h > @@ -42,7 +42,9 @@ struct _virNetDevBandwidth { > > void virNetDevBandwidthFree(virNetDevBandwidthPtr def); > > -int virNetDevBandwidthSet(const char *ifname, virNetDevBandwidthPtr bandwidth) > +int virNetDevBandwidthSet(const char *ifname, > + virNetDevBandwidthPtr bandwidth, > + bool hierarchical_class) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; > int virNetDevBandwidthClear(const char *ifname) > ATTRIBUTE_NONNULL(1); > diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c > index bd26ba9..cfe3f87 100644 > --- a/src/util/virnetdevmacvlan.c > +++ b/src/util/virnetdevmacvlan.c > @@ -925,7 +925,7 @@ create_name: > rc = 0; > } > > - if (virNetDevBandwidthSet(cr_ifname, bandwidth) < 0) { > + if (virNetDevBandwidthSet(cr_ifname, bandwidth, false) < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("cannot set bandwidth limits on %s"), > cr_ifname); Huh. I thought I had remembered that bandwidth (like iptables rules) couldn't be applied to macvtap interfaces. Is that wrong? (unrelated to this patchset anyway, just curious).

ACK with the above questions appropriately answered. -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

These set bridge part of QoS when bringing domain's interface up. Long story short, if there's a 'floor' set, a new QoS class is created. ClassID MUST be unique within the bridge and should be kept for unplug phase. --- po/POTFILES.in | 1 + src/util/virnetdevbandwidth.c | 178 +++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevbandwidth.h | 14 +++ 3 files changed, 193 insertions(+), 0 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 9768528..f51e2c7 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -154,6 +154,7 @@ src/util/virhash.c src/util/virkeyfile.c src/util/virlockspace.c src/util/virnetdev.c +src/util/virnetdevbandwidth.c src/util/virnetdevbridge.c src/util/virnetdevmacvlan.c src/util/virnetdevopenvswitch.c diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index fcc6b56..9597dcd 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -285,3 +285,181 @@ virNetDevBandwidthEqual(virNetDevBandwidthPtr a, return true; } + +/* + * virNetDevBandwidthPlug: + * @brname: name of the bridge + * @net_bandwidth: QoS settings on @brname + * @ifname: interface being plugged into @brname + * @ifmac: MAC of @ifname + * @bandwidth: QoS settings for @ifname + * @id: unique ID (MUST be greater than 2)

+ * + * Set bridge part of interface QoS settings, e.g. guaranteed + * bandwidth. @id is an unique ID (among @brname) from which + * other identifiers for class, qdisc and filter are derived. + * However, two classes were already set up (by + * virNetDevBandwidthSet). That's why this @id MUST be greater + * than 2. You may want to keep passed @id, as it is used later + * by virNetDevBandwidthUnplug. + * + * Returns: + * 1 if there is nothing to set + * 0 if QoS set successfully + * -1 otherwise. + */ +int +virNetDevBandwidthPlug(const char *brname, + virNetDevBandwidthPtr net_bandwidth, + const char *ifname, + const virMacAddrPtr ifmac_ptr, + virNetDevBandwidthPtr bandwidth, + unsigned int id) +{ + int ret = -1; + virCommandPtr cmd = NULL; + char *class_id = NULL; + char *qdisc_id = NULL; + char *filter_id = NULL; + char *floor = NULL; + char *ceil = NULL; + unsigned char ifmac[VIR_MAC_BUFLEN]; + char *mac[2]; + + if (!bandwidth || !bandwidth->in || !bandwidth->in->floor) { + /* nothing to set */ + return 1; + } + + if (!net_bandwidth || !net_bandwidth->in) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Bridge '%s' has no QoS set, therefore " + "unable to set 'floor' on '%s'"), + brname, ifname); + return -1; + } + + virMacAddrGetRaw(ifmac_ptr, ifmac); + + if (virAsprintf(&class_id, "1:%x", id) < 0 || + virAsprintf(&qdisc_id, "%x:", id) < 0 || + virAsprintf(&filter_id, "%u", id) < 0 || + virAsprintf(&mac[0], "0x%02x%02x%02x%02x", ifmac[2], + ifmac[3], ifmac[4], ifmac[5]) < 0 || + virAsprintf(&mac[1], "0x%02x%02x", ifmac[0], ifmac[1]) < 0 || + virAsprintf(&floor, "%llukbps", bandwidth->in->floor) < 0 || + virAsprintf(&ceil, "%llukbps", net_bandwidth->in->peak ? + net_bandwidth->in->peak : + net_bandwidth->in->average) < 0) { + virReportOOMError(); + goto cleanup; + } + + cmd = virCommandNew(TC); + virCommandAddArgList(cmd, "class", "add", "dev", brname, "parent", "1:1", + "classid", class_id, "htb", "rate", floor, + "ceil", ceil, NULL); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + virCommandFree(cmd); + cmd = virCommandNew(TC); + virCommandAddArgList(cmd, "qdisc", "add", "dev", brname, "parent", + class_id, "handle", qdisc_id, "sfq", "perturb", + "10", NULL); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + virCommandFree(cmd); + cmd = virCommandNew(TC); + /* Okay, this not nice. But since libvirt does not know anything about + * interface IP address(es), and tc fw filter simply refuse to use ebtables + * marks, we need to use u32 selector to match MAC address. + * If libvirt will ever know something, remove this FIXME

+ */ + virCommandAddArgList(cmd, "filter", "add", "dev", brname, "protocol", "ip", + "prio", filter_id, "u32", + "match", "u16", "0x0800", "0xffff", "at", "-2", + "match", "u32", mac[0], "0xffffffff", "at", "-12", + "match", "u16", mac[1], "0xffff", "at", "-14", + "flowid", class_id, NULL); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; + +cleanup: + VIR_FREE(ceil); + VIR_FREE(floor); + VIR_FREE(mac[1]); + VIR_FREE(mac[0]);

+ VIR_FREE(filter_id); + VIR_FREE(qdisc_id); + VIR_FREE(class_id); + virCommandFree(cmd);

+ return ret; +} + +/* + * virNetDevBandwidthUnplug: + * @brname: from which bridge are we unplugging + * @id: unique identifier (MUST be greater than 2) + * + * Remove QoS settings from bridge. + * + * Returns 0 on success, -1 otherwise. + */ +int +virNetDevBandwidthUnplug(const char *brname, + unsigned int id)

+{ + int ret = -1; + int cmd_ret = 0; + virCommandPtr cmd = NULL; + char *class_id = NULL; + char *qdisc_id = NULL; + char *filter_id = NULL; + + if (virAsprintf(&class_id, "1:%x", id) < 0 || + virAsprintf(&qdisc_id, "%x:", id) < 0 || + virAsprintf(&filter_id, "%u", id) < 0) { + virReportOOMError(); + goto cleanup; + } + + cmd = virCommandNew(TC); + virCommandAddArgList(cmd, "qdisc", "del", "dev", brname, + "handle", qdisc_id, NULL); + + /* Don't threat tc errors as fatal, but + * try to remove as much as possible */

+ if (virCommandRun(cmd, &cmd_ret) < 0) + goto cleanup; + + virCommandFree(cmd); + cmd = virCommandNew(TC); + virCommandAddArgList(cmd, "filter", "del", "dev", brname, + "prio", filter_id, NULL); + + if (virCommandRun(cmd, &cmd_ret) < 0) + goto cleanup;

+ + cmd = virCommandNew(TC); + virCommandAddArgList(cmd, "class", "del", "dev", brname, + "classid", class_id, NULL); + + if (virCommandRun(cmd, &cmd_ret) < 0) + goto cleanup;

+ VIR_FREE(filter_id); + VIR_FREE(qdisc_id); + VIR_FREE(class_id); + virCommandFree(cmd);

This will be used whenever a NIC with guaranteed throughput is to be plugged into a bridge. It will adjust the average throughput of non guaranteed NICs (classid 1:2) to meet new requirements. --- src/util/virnetdevbandwidth.c | 49 +++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevbandwidth.h | 6 +++++ 2 files changed, 55 insertions(+), 0 deletions(-) diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index 9597dcd..3abe7e2 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -463,3 +463,52 @@ cleanup: virCommandFree(cmd); return ret; } + +/** + * virNetDevBandwidthUpdateRate: + * @ifname: interface name + * @classid: ID of class to update + * @new_rate: new rate + * + * This function updates the 'rate' attribute of HTB class. + * It can be used whenever a new interface is plugged to a + * bridge to adjust average throughput of non guaranteed + * NICs.

+ * + * Returns 0 on success, -1 otherwise. + */ +int +virNetDevBandwidthUpdateRate(const char *ifname, + const char *class_id, + virNetDevBandwidthPtr bandwidth, + unsigned long long new_rate) +{ + int ret = -1; + virCommandPtr cmd = NULL; + char *rate = NULL; + char *ceil = NULL; + + if (virAsprintf(&rate, "%llukbps", new_rate) < 0 || + virAsprintf(&ceil, "%llukbps", bandwidth->in->peak ? + bandwidth->in->peak : + bandwidth->in->average) < 0) { + virReportOOMError(); + goto cleanup; + } + + cmd = virCommandNew(TC); + virCommandAddArgList(cmd, "class", "change", "dev", ifname, + "classid", class_id, "htb", "rate", rate, + "ceil", ceil, NULL); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; + +cleanup: + virCommandFree(cmd); + VIR_FREE(rate); + VIR_FREE(ceil); + return ret; +} diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h index 19eb418..a5d595e 100644 --- a/src/util/virnetdevbandwidth.h +++ b/src/util/virnetdevbandwidth.h @@ -67,4 +67,10 @@ int virNetDevBandwidthUnplug(const char *brname, unsigned int id) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virNetDevBandwidthUpdateRate(const char *ifname, + const char *class_id, + virNetDevBandwidthPtr bandwidth, + unsigned long long new_rate) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) + ATTRIBUTE_RETURN_CHECK; #endif /* __VIR_NETDEV_BANDWIDTH_H__ */

On 11/19/2012 11:51 AM, Michal Privoznik wrote: > 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. > --- > src/Makefile.am | 7 ++- > src/conf/domain_conf.h | 1 + > src/conf/network_conf.c | 1 + > src/conf/network_conf.h | 3 + > src/libvirt_network.syms | 8 ++ > src/network/bridge_driver.c | 165 +++++++++++++++++++++++++++++++++++++++++++ > src/network/bridge_driver.h | 10 ++- > src/qemu/qemu_command.c | 32 ++++++--- > src/qemu/qemu_process.c | 12 +++ > 9 files changed, 225 insertions(+), 14 deletions(-) > create mode 100644 src/libvirt_network.syms >

> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h > index 3e46304..8a8d1e4 100644 > --- a/src/conf/network_conf.h > +++ b/src/conf/network_conf.h > @@ -221,6 +221,9 @@ struct _virNetworkObj { > > virNetworkDefPtr def; /* The current definition */ > virNetworkDefPtr newDef; /* New definition to activate at shutdown */ > + > + unsigned int class_id; /* next class ID for QoS */ Okay, so this is just a monotonically increasing counter so that each interface gets a new and different class_id. Maybe you should call it "nextFreeClassID" or something like that, so that everyone understands it's not a class id used by the network. Or you might want to do this with a bitmap so you can re-use id's of interfaces that are disconnected. (can virBitmaps being dynamically expanded?) > + unsigned long long floor_sum; /* sum of all 'floor'-s of attached NICs */ And you keep track of the sum of all floors here. So, what happens if libvirtd is restarted? It looks like they both go back to their initial values.

And what about hotplug? This is a similar problem to the pool of interfaces used by macvtap/hostdev modes - a network with an interface pool needs to have the "inuse" counters of each interface refreshed whenever libvirtd restarts. So the necessary hooks are already in place: networkAllocateActualDevice - called any time an interface with type='network' is initially allocated and connected. networkNotifyActualDevice - called for each type='network' interface of each active domain any time libvirtd restarts networkReleaseActualDevice - called for every type='network' interface as it is being disconnected and destroyed. These are called for *all* type='network' interfaces, not just those that need to allocate a physical netdev from a pool. Rather than adding your own new hooks (and figuring out all the places they need to be called), I think it would be better to use the existing hooks (perhaps calling a reduced/renamed version of your functions, which can possibly be moved over to ). That will have two advantages: 1) It will assure that the bandwidth functions are called at all the right times, including hotplug/unplug, and libvirtd restart 2) It will continue the process of consolidating all network-related functionality need for these three events into 3 functions which may some day be moved into their own daemom with a public (within libvirt anyway) API accessible via RPC (thus allowing non-privileged libvirts to have full networking functionality).

Note that you will probably want to save the interface class_id in the iface->actual (as a matter of fact, in iface->data.network.actual->bandwidth, which can be retrieved with virDomainNetGetActualBandwidth()) so that it's saved properly in the interface's state without polluting the interface's config. Then it will be read back from the state file during the restart and available during networkNotifyActualDevice() of each interface. I guess you can then re-set the network->class_id by checking interface->class_id and setting network->class_id = MAX(network->class_id, iface->class_id + 1); for each encountered interface (or add it to a bitmap, if a) bitmaps can be enlarged dynamically or b) we can determine a reasonable maximum limit on number of active domains). At the same time, you'll recompute the network->floor_sum iteratively as the network is called with a notify for each interface.

> ...

To summarize: I think this needs to be refactored to be integrated into the network*ActualDevice() functions so that the bookkeeping is properly handled in all cases, including hotplug and libvirtd restart. -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Currently, we are only keeping a inactive XML configuration in status dir. This is no longer enough as we need to keep this class_id attribute so we don't overwrite old entries when the daemon restarts.

However, since there has already been a libvirt release

which has just <network/> as root element, and we want to keep things compatible, detect that loaded status file is older one, and don't scream about it. --- src/conf/network_conf.c | 199 ++++++++++++++++++++++++++++++++++--------- src/conf/network_conf.h | 2 + src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 9 +-- 4 files changed, 165 insertions(+), 46 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 80189e6..9c2e4d4 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1645,6 +1645,78 @@ cleanup: return def; } +int +virNetworkObjUpdateParseFile(const char *filename, + virNetworkObjPtr net) +{ + int ret = -1; + xmlDocPtr xml = NULL; + xmlNodePtr node = NULL; + virNetworkDefPtr tmp = NULL; + xmlXPathContextPtr ctxt = NULL; + + xml = virXMLParse(filename, NULL, _("(network status)")); + if (!xml) + return -1; + + ctxt = xmlXPathNewContext(xml); + if (ctxt == NULL) { + virReportOOMError(); + goto cleanup; + } + + node = xmlDocGetRootElement(xml); + if (xmlStrEqual(node->name, BAD_CAST "networkstatus")) { + /* Newer network status file. Contains useful + * info which are not to be found in bare config XML */ + char *class_id = NULL; + char *floor_sum = NULL; + + ctxt->node = node; + class_id = virXPathString("string(./class_id[1]/@next)", ctxt); + if (class_id && + virStrToLong_ui(class_id, NULL, 10, &net->class_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed 'class_id' attribute: %s"), + class_id); + VIR_FREE(class_id); + goto cleanup; + } + VIR_FREE(class_id); + + floor_sum = virXPathString("string(./floor[1]/@sum)", ctxt); + if (floor_sum && + virStrToLong_ull(floor_sum, NULL, 10, &net->floor_sum) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed 'floor_sum' attribute: %s"), + floor_sum); + VIR_FREE(floor_sum); + } + VIR_FREE(floor_sum); + } + + node = virXPathNode("//network", ctxt); + if (!node) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not find any 'network' element")); + goto cleanup; + } + + ctxt->node = node; + tmp = virNetworkDefParseXML(ctxt); + + if (tmp) { + net->newDef = net->def; + net->def = tmp; + } + + ret = 0; + +cleanup: + xmlXPathFreeContext(ctxt); + return ret; +} + static int virNetworkDNSDefFormat(virBufferPtr buf, virNetworkDNSDefPtr def) @@ -1823,24 +1895,26 @@ virPortGroupDefFormat(virBufferPtr buf, return 0; } -char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) +static int +virNetworkDefFormatInternal(virBufferPtr buf, + const virNetworkDefPtr def, + unsigned int flags) { - virBuffer buf = VIR_BUFFER_INITIALIZER; unsigned char *uuid; char uuidstr[VIR_UUID_STRING_BUFLEN]; int ii; - virBufferAddLit(&buf, "<network"); + virBufferAddLit(buf, "<network"); if (!(flags & VIR_NETWORK_XML_INACTIVE) && (def->connections > 0)) { - virBufferAsprintf(&buf, " connections='%d'", def->connections); + virBufferAsprintf(buf, " connections='%d'", def->connections); } - virBufferAddLit(&buf, ">\n"); - virBufferAdjustIndent(&buf, 2); - virBufferEscapeString(&buf, "<name>%s</name>\n", def->name); + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + virBufferEscapeString(buf, "<name>%s</name>\n", def->name); uuid = def->uuid; virUUIDFormat(uuid, uuidstr); - virBufferAsprintf(&buf, "<uuid>%s</uuid>\n", uuidstr); + virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuidstr); if (def->forwardType != VIR_NETWORK_FORWARD_NONE) { const char *dev = NULL; @@ -1854,40 +1928,40 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) def->forwardType, def->name); goto error; } - virBufferAddLit(&buf, "<forward"); - virBufferEscapeString(&buf, " dev='%s'", dev); - virBufferAsprintf(&buf, " mode='%s'", mode); + virBufferAddLit(buf, "<forward"); + virBufferEscapeString(buf, " dev='%s'", dev); + virBufferAsprintf(buf, " mode='%s'", mode); if (def->forwardType == VIR_NETWORK_FORWARD_HOSTDEV) { if (def->managed == 1) - virBufferAddLit(&buf, " managed='yes'"); + virBufferAddLit(buf, " managed='yes'"); else - virBufferAddLit(&buf, " managed='no'"); + virBufferAddLit(buf, " managed='no'"); } - virBufferAsprintf(&buf, "%s>\n", + virBufferAsprintf(buf, "%s>\n", (def->nForwardIfs || def->nForwardPfs) ? "" : "/"); - virBufferAdjustIndent(&buf, 2); + virBufferAdjustIndent(buf, 2); /* For now, hard-coded to at most 1 forwardPfs */ if (def->nForwardPfs) - virBufferEscapeString(&buf, "<pf dev='%s'/>\n", + virBufferEscapeString(buf, "<pf dev='%s'/>\n", def->forwardPfs[0].dev); if (def->nForwardIfs && (!def->nForwardPfs || !(flags & VIR_NETWORK_XML_INACTIVE))) { for (ii = 0; ii < def->nForwardIfs; ii++) { if (def->forwardType != VIR_NETWORK_FORWARD_HOSTDEV) { - virBufferEscapeString(&buf, "<interface dev='%s'", + virBufferEscapeString(buf, "<interface dev='%s'", def->forwardIfs[ii].device.dev); if (!(flags & VIR_NETWORK_XML_INACTIVE) && (def->forwardIfs[ii].connections > 0)) { - virBufferAsprintf(&buf, " connections='%d'", + virBufferAsprintf(buf, " connections='%d'", def->forwardIfs[ii].connections); } - virBufferAddLit(&buf, "/>\n"); + virBufferAddLit(buf, "/>\n"); } else { if (def->forwardIfs[ii].type == VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI) { - if (virDevicePCIAddressFormat(&buf, + if (virDevicePCIAddressFormat(buf, def->forwardIfs[ii].device.pci, true) < 0) goto error; @@ -1895,67 +1969,111 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) } } } - virBufferAdjustIndent(&buf, -2); + virBufferAdjustIndent(buf, -2); if (def->nForwardPfs || def->nForwardIfs) - virBufferAddLit(&buf, "</forward>\n"); + virBufferAddLit(buf, "</forward>\n"); } if (def->forwardType == VIR_NETWORK_FORWARD_NONE || - def->forwardType == VIR_NETWORK_FORWARD_NAT || - def->forwardType == VIR_NETWORK_FORWARD_ROUTE) { + def->forwardType == VIR_NETWORK_FORWARD_NAT || + def->forwardType == VIR_NETWORK_FORWARD_ROUTE) { - virBufferAddLit(&buf, "<bridge"); + virBufferAddLit(buf, "<bridge"); if (def->bridge) - virBufferEscapeString(&buf, " name='%s'", def->bridge); - virBufferAsprintf(&buf, " stp='%s' delay='%ld' />\n", + virBufferEscapeString(buf, " name='%s'", def->bridge); + virBufferAsprintf(buf, " stp='%s' delay='%ld' />\n", def->stp ? "on" : "off", def->delay); } else if (def->forwardType == VIR_NETWORK_FORWARD_BRIDGE && def->bridge) { - virBufferEscapeString(&buf, "<bridge name='%s' />\n", def->bridge); + virBufferEscapeString(buf, "<bridge name='%s' />\n", def->bridge); } if (def->mac_specified) { char macaddr[VIR_MAC_STRING_BUFLEN]; virMacAddrFormat(&def->mac, macaddr); - virBufferAsprintf(&buf, "<mac address='%s'/>\n", macaddr); + virBufferAsprintf(buf, "<mac address='%s'/>\n", macaddr); } if (def->domain) - virBufferAsprintf(&buf, "<domain name='%s'/>\n", def->domain); + virBufferAsprintf(buf, "<domain name='%s'/>\n", def->domain); - if (virNetworkDNSDefFormat(&buf, def->dns) < 0) + if (virNetworkDNSDefFormat(buf, def->dns) < 0) goto error; - if (virNetDevVlanFormat(&def->vlan, &buf) < 0) + if (virNetDevVlanFormat(&def->vlan, buf) < 0) goto error; - if (virNetDevBandwidthFormat(def->bandwidth, &buf) < 0) + if (virNetDevBandwidthFormat(def->bandwidth, buf) < 0) goto error; for (ii = 0; ii < def->nips; ii++) { - if (virNetworkIpDefFormat(&buf, &def->ips[ii]) < 0) + if (virNetworkIpDefFormat(buf, &def->ips[ii]) < 0) goto error; } - if (virNetDevVPortProfileFormat(def->virtPortProfile, &buf) < 0) + if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0) goto error; for (ii = 0; ii < def->nPortGroups; ii++) - if (virPortGroupDefFormat(&buf, &def->portGroups[ii]) < 0) + if (virPortGroupDefFormat(buf, &def->portGroups[ii]) < 0) goto error; + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</network>\n"); + + return 0; + +error: + return -1; +} + +char * +virNetworkDefFormat(virNetworkDefPtr def, + unsigned int flags) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (virNetworkDefFormatInternal(&buf, def, flags) < 0) + goto error; + + if (virBufferError(&buf)) + goto no_memory; + + return virBufferContentAndReset(&buf); + +no_memory: + virReportOOMError(); +error: + virBufferFreeAndReset(&buf); + return NULL; +} + +static char * +virNetworkObjFormat(virNetworkObjPtr net, + unsigned int flags) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&buf, "<networkstatus>\n"); + virBufferAsprintf(&buf, " <class_id next='%u'/>\n", net->class_id); + virBufferAsprintf(&buf, " <floor sum='%llu'/>\n", net->floor_sum); + + virBufferAdjustIndent(&buf, 2); + if (virNetworkDefFormatInternal(&buf, net->def, flags) < 0) + goto error; + virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</network>\n"); + virBufferAddLit(&buf, "</networkstatus>"); if (virBufferError(&buf)) goto no_memory; return virBufferContentAndReset(&buf); - no_memory: +no_memory: virReportOOMError(); - error: +error: virBufferFreeAndReset(&buf); return NULL; } @@ -2027,9 +2145,10 @@ int virNetworkSaveStatus(const char *statusDir, virNetworkObjPtr network) { int ret = -1; + int flags = 0; char *xml; - if (!(xml = virNetworkDefFormat(network->def, 0))) + if (!(xml = virNetworkObjFormat(network, flags))) goto cleanup; if (virNetworkSaveXML(statusDir, network->def, xml)) diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 8a8d1e4..efa9dea 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -270,6 +270,8 @@ virNetworkDefPtr virNetworkDefParseString(const char *xmlStr); virNetworkDefPtr virNetworkDefParseFile(const char *filename); virNetworkDefPtr virNetworkDefParseNode(xmlDocPtr xml, xmlNodePtr root); +int virNetworkObjUpdateParseFile(const char *filename, + virNetworkObjPtr net); char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5a07139..3ed0c97 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -858,6 +858,7 @@ virNetworkObjSetDefTransient; virNetworkObjUnlock; virNetworkObjUnsetDefTransient; virNetworkObjUpdate; +virNetworkObjUpdateParseFile; virNetworkRemoveInactive; virNetworkSaveConfig; virNetworkSaveStatus; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 44fa56e..8dc9d19 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -236,7 +236,6 @@ networkFindActiveConfigs(struct network_driver *driver) { for (i = 0 ; i < driver->networks.count ; i++) { virNetworkObjPtr obj = driver->networks.objs[i]; - virNetworkDefPtr tmp; char *config; virNetworkObjLock(obj); @@ -254,12 +253,10 @@ networkFindActiveConfigs(struct network_driver *driver) { } /* Try and load the live config */ - tmp = virNetworkDefParseFile(config); + if (virNetworkObjUpdateParseFile(config, obj) < 0) + VIR_WARN("Unable to update config of '%s' network", + obj->def->name); VIR_FREE(config); - if (tmp) { - obj->newDef = obj->def; - obj->def = tmp; - } /* If bridge exists, then mark it active */ if (obj->def->bridge &&

Interfaces keeps a class_id, which is an ID from which bridge part of QoS settings is derived. We need to store class_id in domain status file, so we can later pass it to virNetDevBandwidthUnplug.

--- src/conf/domain_conf.c | 4 +- src/qemu/qemu_domain.c | 66 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bf23b77..37a8875 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10242,7 +10242,7 @@ static virDomainObjPtr virDomainObjParseXML(virCapsPtr caps, VIR_FREE(nodes); if (caps->privateDataXMLParse && - ((caps->privateDataXMLParse)(ctxt, obj->privateData)) < 0) + ((caps->privateDataXMLParse)(ctxt, obj)) < 0) goto error; return obj; @@ -14212,7 +14212,7 @@ static char *virDomainObjFormat(virCapsPtr caps, } if (caps->privateDataXMLFormat && - ((caps->privateDataXMLFormat)(&buf, obj->privateData)) < 0) + ((caps->privateDataXMLFormat)(&buf, obj)) < 0) goto error; virBufferAdjustIndent(&buf, 2); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e0d6951..5312946 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -258,9 +258,12 @@ static void qemuDomainObjPrivateFree(void *data) static int qemuDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) { - qemuDomainObjPrivatePtr priv = data; + virDomainObjPtr vm = data; + qemuDomainObjPrivatePtr priv = vm->privateData; const char *monitorpath; enum qemuDomainJob job; + bool do_class_id = false; + int i; /* priv->monitor_chr is set only for qemu */ if (priv->monConfig) { @@ -283,7 +286,6 @@ static int qemuDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) if (priv->nvcpupids) { - int i; virBufferAddLit(buf, " <vcpus>\n"); for (i = 0 ; i < priv->nvcpupids ; i++) { virBufferAsprintf(buf, " <vcpu pid='%d'/>\n", priv->vcpupids[i]); @@ -292,7 +294,6 @@ static int qemuDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) } if (priv->caps) { - int i; virBufferAddLit(buf, " <qemuCaps>\n"); for (i = 0 ; i < QEMU_CAPS_LAST ; i++) { if (qemuCapsGet(priv->caps, i)) { @@ -326,15 +327,36 @@ static int qemuDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) if (priv->fakeReboot) virBufferAsprintf(buf, " <fakereboot/>\n"); + for (i = 0; i < vm->def->nnets; i++) { + if (vm->def->nets[i]->class_id) { + do_class_id = true; + break; + } + } + + if (do_class_id) { + virBufferAddLit(buf, " <class_id>\n"); + for (; i < vm->def->nnets; i++) { + virDomainNetDefPtr iface = vm->def->nets[i]; + if (iface->class_id) { + virBufferAsprintf(buf, " <interface alias='%s' " + "class_id='%u'/>\n", + iface->info.alias, iface->class_id); + } + } + virBufferAddLit(buf, " </class_id>\n"); + } + return 0; } static int qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data) { - qemuDomainObjPrivatePtr priv = data; + virDomainObjPtr vm = data; + qemuDomainObjPrivatePtr priv = vm->privateData; char *monitorpath; char *tmp; - int n, i; + int n, i, ii; xmlNodePtr *nodes = NULL; qemuCapsPtr caps = NULL; @@ -471,6 +493,40 @@ static int qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data) priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1; + if ((n = virXPathNodeSet("./class_id/interface", ctxt, &nodes)) < 0) + goto error; + + for (i = 0; i < n; i++) { + char *alias = virXMLPropString(nodes[i], "alias"); + char *class_id = virXMLPropString(nodes[i], "class_id"); + virDomainNetDefPtr iface = NULL; + if (alias && class_id) { + for (ii = 0; ii < vm->def->nnets; ii++) { + if (STREQ(vm->def->nets[ii]->info.alias, alias)) { + iface = vm->def->nets[ii]; + break; + } + } + + if (!iface) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No such interface '%s'"), + alias); + VIR_FREE(alias); + VIR_FREE(class_id); + goto error; + } + + if (virStrToLong_ui(class_id, NULL, 10, &iface->class_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Malformed class_id attribute: %s"), + class_id); + } + } + VIR_FREE(alias); + VIR_FREE(class_id); + } + return 0; error:

Status file keeps track of class_id and floor_sum. It's better to keep it updated in case libvirtd is killed.

--- src/network/bridge_driver.c | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8dc9d19..5a0f43f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4300,6 +4300,15 @@ networkNotifyPlug(virNetworkPtr network, net->class_id++; /* update sum of 'floor'-s of attached NICs */ net->floor_sum += iface->bandwidth->in->floor; + /* update status file */ + if (virNetworkSaveStatus(NETWORK_STATE_DIR, net) < 0) { + net->class_id--; + net->floor_sum -= iface->bandwidth->in->floor; + iface->class_id = 0; + ignore_value(virNetDevBandwidthUnplug(net->def->bridge, + net->class_id)); + goto cleanup; + } /* update rate for non guaranteed NICs */ new_rate -= net->floor_sum; if (virNetDevBandwidthUpdateRate(net->def->bridge, "1:2", @@ -4339,6 +4348,11 @@ networkNotifyUnplug(virDomainNetDefPtr iface) goto cleanup; /* update sum of 'floor'-s of attached NICs */ net->floor_sum -= iface->bandwidth->in->floor; + /* update status file */ + if (virNetworkSaveStatus(NETWORK_STATE_DIR, net) < 0) { + net->floor_sum += iface->bandwidth->in->floor; + goto cleanup; + } /* update rate for non guaranteed NICs */ new_rate -= net->floor_sum; if (virNetDevBandwidthUpdateRate(net->def->bridge, "1:2",

This approach implemented in previous patches is not trivial and deserves small description. --- src/util/virnetdevbandwidth.c | 68 +++++++++++++++++++++++++++++++++++++--- 1 files changed, 62 insertions(+), 6 deletions(-) diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index b4ffc29..d32c7db 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -92,13 +92,69 @@ virNetDevBandwidthSet(const char *ifname, if (virCommandRun(cmd, NULL) < 0) goto cleanup; + /* If we are creating hierarchical class, all non guaranteed traffic

+ * goes to 1:2 class which will adjust 'rate' dynamically as NICs with

+ * guaranteed throughput are plugged and unplugged. Class 1:1 is there

+ * so we don't exceed the maximum limit for network. For each NIC with

+ * guaranteed throughput a separate classid will be created. + * NB '1:' is just a shorter notation of '1:0'. + * + * To get a picture how this works: + * + * +-----+ +---------+ +-----------+ +-----------+ +-----+ + * | | | qdisc | | class 1:1 | | class 1:2 | | | + * | NIC | | def 1:2 | | rate | | rate | | sfq | + * | | --> | | --> | peak | -+-> | peak | --> | | + * +-----+ +---------+ +-----------+ | +-----------+ +-----+ + * | + * | +-----------+ +-----+ + * | | class 1:3 | | | + * | | rate | | sfq | + * +-> | peak | --> | | + * | +-----------+ +-----+ + * ... + * | +-----------+ +-----+ + * | | class 1:n | | | + * | | rate | | sfq | + * +-> | peak | --> | | + * +-----------+ +-----+ + * + * After the routing decision, when is it clear a packet is to be send

+ * via NIC, it is sent to root qdisc (queueing discipline). In this case

+ * HTB (Hierarchical Token Bucket). It has only one direct child class + * (with id 1:1) which shapes the overall rate that is sent through NIC.

+ * This class have at least one child (1:2). This is meant for whole

+ * non-privileged (non guaranteed) traffic from all domains. Then, for + * each interface with guaranteed throughput a separate class (1:n) is

+ * created. Imagine a class is a box. Whenever a packet ends up in a + * class it is stored in this box until a kernel sends it in which case

+ * it is removed from box. Packets are placed into boxes based on rules + * (filters) - e.g. depending on destination IP/MAC address. If there is + * no rule to be applied, root qdisc have a default where such packets

+ * go (1:2 in this case). Packets come in over and over again and boxes + * get filled more and more. Imagine that kernel sends packets just once

+ * a second. So it starts to traverse through this tree. It starts with + * root qdisc and over 1:1 it gets to 1:2. It sends packets up to its

+ * 'rate'. Then it takes 1:3 and again sends packets up to its 'rate'.

+ * And the whole process is repeated until 1:n is processed. So now we

+ * have ensured each class its guaranteed bandwidth. If the sum of sent + * data doesn't exceed 'rate' in 1:1 class, we can go further and send

+ * more packets. The rest of available bandwidth is distributed to

+ * 1:2,1:3...1:n classes by ratio of their 'rate'. As soon as root

+ * 'rate' limit is reached or there are no more packets to send, we stop + * sending and wait another second. Each class has SFQ qdisc which

+ * shuffles packets in boxes stochastically, so one sender could not

+ * starve others. + * + * Therefore, whenever we want to plug a new guaranteed interface, we

+ * need to create a new class and adjust 'rate' of 1:2 class. When

+ * unplugging we do the exact opposite - remove associated class, and

+ * adjust the 'rate'. + * + * This description is rather longer and you'd better read it before you + * start digging into this :)

+ */ if (hierarchical_class) { - /* If we are creating hierarchical class, all non guaranteed traffic - * goes to 1:2 class which will adjust 'rate' dynamically as NICs with - * guaranteed throughput are plugged and unplugged. Class 1:1 is there - * so we don't exceed the maximum limit for network. For each NIC with - * guaranteed throughput a separate classid will be created. - * NB '1:' is just a shorter notation of '1:0' */ virCommandFree(cmd); cmd = virCommandNew(TC); virCommandAddArgList(cmd, "class", "add", "dev", ifname, "parent",