On 07/18/2011 04:05 PM, Michal Privoznik wrote:

Define new 'bandwidth' element with possible child element 'inbound' and 'outbound' addressing incoming and outgoing traffic respectively:

<bandwidth> <inbound average='1000' peak='2000' burst='5120'/> <outbound average='500'/> </bandwidth>

Leaving any element out means not to shape traffic in that direction. The units for average and peak (rate) are in kilobytes per second, for burst (size) are just in kilobytes. This element can be inserted into domain's 'interface' and 'network'. --- docs/formatdomain.html.in | 33 +++++++++++++++++++++++++++ docs/formatnetwork.html.in | 30 +++++++++++++++++++++++++ docs/schemas/bandwidth.rng | 52 ++++++++++++++++++++++++++++++++++++++++++++ docs/schemas/domain.rng | 4 +++ docs/schemas/network.rng | 4 +++ 5 files changed, 123 insertions(+), 0 deletions(-) create mode 100644 docs/schemas/bandwidth.rng

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index a54ee6a..1fbca9d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1825,6 +1825,39 @@ qemu-kvm -net nic,model=? /dev/null <span class="since">Since 0.8.8</span> </p>

+<h5><a name="elementQoS">Quality of service</a></h5> + +<pre> + ... +<devices> +<interface type='network'> +<source network='default'/> +<target dev='vnet0'/> +<b><bandwidth> +<inbound average='1000' peak='5000' burst='1024'/> +<outbound average='128' peak='256' burst='256'/> +</bandwidth></b> +</interface> +<devices> + ...</pre> + +<p> + This part of interface XML provides setting quality of service. Incoming + and outgoing traffic can be shaped independently. The +<code>bandwidth</code> element can have at most one<code>inbound</code> + and at most one<code>outbound</code> child elements. Leaving any of these + 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 + 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> +</p> <h4><a name="elementsInput">Input devices</a></h4>

<p> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index f9421c3..67dbc5b 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -102,6 +102,36 @@ 0.4.2</span></dd> </dl>

+<h5><a name="elementQoS">Quality of service</a></h5> + +<pre> + ... +<forward mode='nat' dev='eth0'/> +<b><bandwidth> +<inbound average='1000' peak='5000' burst='5120'/> +<outbound average='128' peak='256' burst='256'/> +</bandwidth></b> +<mac address='00:16:3E:5D:C7:9E'/> + ...</pre> + +<p> + This part of network XML provides setting quality of service. Incoming + and outgoing traffic can be shaped independently. The +<code>bandwidth</code> element can have at most one<code>inbound</code> + and at most one<code>outbound</code> child elements. Leaving any of these + children element out result in no QoS applied on that traffic direction. + So, when you want to shape only network'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 + interface being shaped. Then there are two optional attributes: +<code>peak</code>, which specifies maximum rate at which bridge 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. + The rate is shared equally within domains connected to the network. +<span class="since">Since 0.9.4</span> +</p> <h3><a name="elementsAddress">Addressing</a></h3>

<p> diff --git a/docs/schemas/bandwidth.rng b/docs/schemas/bandwidth.rng new file mode 100644 index 0000000..6719bcc --- /dev/null +++ b/docs/schemas/bandwidth.rng @@ -0,0 +1,52 @@ +<!-- A Relax NG schema for the domain and network bandwidth XML format --> +<grammar xmlns="http://relaxng.org/ns/structure/1.0" + datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> + +<define name="bandwidth"> +<element name="bandwidth"> +<interleave> +<optional> +<element name="inbound"> +<ref name="bandwidth-attributes"/> +<empty/> +</element> +</optional> +<optional> +<element name="outbound"> +<ref name="bandwidth-attributes"/> +<empty/> +</element> +</optional> +</interleave> +</element> +</define> + +<define name="bandwidth-attributes"> +<attribute name="average"> +<ref name="speed"/> +</attribute> +<optional> +<attribute name="peak"> +<ref name="speed"/> +</attribute> +</optional> +<optional> +<attribute name='burst'> +<ref name="BurstSize"/> +</attribute> +</optional> +</define> + +<define name="speed"> +<data type="unsignedInt"> +<param name="pattern">[0-9]+</param> +<param name="minInclusive">1</param> +</data> +</define> +<define name="BurstSize"> +<data type="unsignedInt"> +<param name="pattern">[0-9]+</param> +<param name="minInclusive">1</param> +</data> +</define> +</grammar> diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 8a4e3fe..a43a937 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -7,6 +7,7 @@

<include href='basictypes.rng'/> <include href='storageencryption.rng'/> +<include href='bandwidth.rng'/>

I put several network-related rng definitions that I use in both domain.rng and network.rng (for the virtual switch patches) into a file called networkcommon.rng.Maybe you should put the bandwidth rng definitions there as well (assuming my patches get pushed). Othewise, if you keep your own separate file you'll need to add that file in the following places in order for "make rpm" to succeed: docs/schemas/Makefile.am libvirt.spec.in mingw32-libvirt.spec.in (Since there's at least one other place I'd like to see the QoS integrated with my patches, I think it would be best to assume their existence, and use networkcommon.rng).

<!-- description element, maybe placed anywhere under the root @@ -1170,6 +1171,9 @@ <optional> <ref name="deviceBoot"/> </optional> +<optional> +<ref name="bandwidth"/> +</optional> </interleave> </define> <define name="virtualPortProfile"> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 6d9f06b..2456ed1 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -7,6 +7,7 @@ </start>

<include href='basictypes.rng'/> +<include href='bandwidth.rng' />

<define name="network">

@@ -110,6 +111,9 @@ </zeroOrMore> </element> </optional> +<optional> +<ref name="bandwidth"/> +</optional>

<!--<ip> element --> <zeroOrMore>

On 07/18/2011 04:05 PM, Michal Privoznik wrote:

--- src/util/network.h | 14 ++++++++++++++ 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/src/util/network.h b/src/util/network.h index ed0b78c..af32558 100644 --- a/src/util/network.h +++ b/src/util/network.h @@ -45,6 +45,20 @@ typedef struct {

typedef virSocketAddr *virSocketAddrPtr;

+typedef struct { + unsigned long average; /* kbytes/s */ + unsigned long peak; /* kbytes/s */ + unsigned long burst; /* kbytes */ +} virRate; + +typedef virRate *virRatePtr; + +typedef struct { + virRate in, out; +} virBandwidth; + +typedef virBandwidth *virBandwidthPtr; + int virSocketParseAddr (const char *val, virSocketAddrPtr addr, int hint);

ACK. (Actually, you probably didn't need to break down the patches quite so much - a single patch that did the RNG, docs, data definitions, parser/formatter, and test cases, followed by another patch to hook the new data into the functionality probably would have been adequate.)

On 07/18/2011 04:05 PM, Michal Privoznik wrote:

These functions parse given XML node and store the output at given pointer. Unknown elements are silently ignored. Attributes must be integer and must fit in unsigned long. --- src/conf/domain_conf.c | 3 + src/conf/domain_conf.h | 1 + src/conf/network_conf.c | 5 ++ src/conf/network_conf.h | 1 + src/libvirt_private.syms | 1 + src/util/network.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/network.h | 2 + 7 files changed, 125 insertions(+), 0 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8c3e44e..0d8c7e7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2854,6 +2854,9 @@ virDomainNetDefParseXML(virCapsPtr caps, if (virDomainDeviceBootParseXML(cur,&def->bootIndex, bootMap)) goto error; + } else if (xmlStrEqual(cur->name, BAD_CAST "bandwidth")) { + if (virBandwidthDefParseNode(cur,&def->bandwidth)< 0) + goto error; } } cur = cur->next; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 172d3c2..06cea02 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -398,6 +398,7 @@ struct _virDomainNetDef { virDomainDeviceInfo info; char *filter; virNWFilterHashTablePtr filterparams; + virBandwidth bandwidth; };

enum virDomainChrDeviceType { diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index ae479bf..c9929e2 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -742,6 +742,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) char *tmp; xmlNodePtr *ipNodes = NULL; xmlNodePtr dnsNode = NULL; + xmlNodePtr bandwidthNode = NULL; int nIps;

if (VIR_ALLOC(def)< 0) { @@ -777,6 +778,10 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) /* Parse network domain information */ def->domain = virXPathString("string(./domain[1]/@name)", ctxt);

+ if ((bandwidthNode = virXPathNode("./bandwidth", ctxt)) != NULL&& + virBandwidthDefParseNode(bandwidthNode,&def->bandwidth)< 0) + goto error; + /* Parse bridge information */ def->bridge = virXPathString("string(./bridge[1]/@name)", ctxt); tmp = virXPathString("string(./bridge[1]/@stp)", ctxt); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 5edcf27..565a464 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -127,6 +127,7 @@ struct _virNetworkDef { virNetworkIpDefPtr ips; /* ptr to array of IP addresses on this network */

virNetworkDNSDefPtr dns; /* ptr to dns related configuration */ + virBandwidth bandwidth; };

Having this contained in the parent object rather than pointed to by it makes memory management easier, but it means that there needs to be some "special" value of the data that means "not present". Currently you're checking for bandwidth.in.average != 0 and bandwidth.out.average != 0. AS long as those would never be acceptable values (for example - is it possible that average would be 0 and burst would be something non-default? I don't even know, just asking). Additionally, once you can have bandwidth information in a portgroup (which is stored in the <network> config, but used by the domain's <interface> config), you'll need to be either allocating a virBandwidth and passing it back from some network/portgroup helper function, or you'll need to have the helper function fill in an existing one that you pass to it. Anyway, having this contained in rather than pointing to will work for now, but you may want to change it in the future.

typedef struct _virNetworkObj virNetworkObj; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3e3b1dd..12db3d7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -700,6 +700,7 @@ nlComm;

# network.h +virBandwidthDefParseNode; virSocketAddrBroadcast; virSocketAddrBroadcastByPrefix; virSocketAddrIsNetmask; diff --git a/src/util/network.c b/src/util/network.c index eb16e0c..ce949c7 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -21,6 +21,9 @@ virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__)

+#define virBandwidthError(code, ...) \ + virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__)

Is having this separate macro defined really necessary? It's identical to virSocketError() which is already defined in network.c.

/* * Helpers to extract the IP arrays from the virSocketAddrPtr * That part is the less portable of the module @@ -674,3 +677,112 @@ virSocketAddrPrefixToNetmask(unsigned int prefix, error: return result; } + +static int +virBandwidthParseChildDefNode(xmlNodePtr node, virRatePtr rate) +{ + int ret = -1; + char *average = NULL; + char *peak = NULL; + char *burst = NULL; + + if (!node || !rate) + return -1;

You're returning an error indication without any log message. virsh will show this as "unknown error", which is very difficult to diagnose.

+ + average = virXMLPropString(node, "average"); + peak = virXMLPropString(node, "peak"); + burst = virXMLPropString(node, "burst"); + + if (average) { + if (virStrToLong_ul(average, NULL, 10,&rate->average)< 0) { + virBandwidthError(VIR_ERR_CONFIG_UNSUPPORTED, + _("could not convert %s"), + average); + goto cleanup; + } + } else { + virBandwidthError(VIR_ERR_XML_DETAIL, "%s", + _("Missing mandatory average attribute")); + goto cleanup; + } + + if (peak&& virStrToLong_ul(peak, NULL, 10,&rate->peak)< 0) { + virBandwidthError(VIR_ERR_CONFIG_UNSUPPORTED, + _("could not convert %s"), + peak); + goto cleanup; + } + + if (burst&& virStrToLong_ul(burst, NULL, 10,&rate->burst)< 0) { + virBandwidthError(VIR_ERR_CONFIG_UNSUPPORTED, + _("could not convert %s"), + burst); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(average); + VIR_FREE(peak); + VIR_FREE(burst); + + return ret; +} + +/** + * virBandwidthDefParseNode: + * @node: XML node + * @def: where to store the parsed result + * + * Parse bandwidth XML and store into given pointer + * + * Returns 0 on success, -1 on error. + */ +int +virBandwidthDefParseNode(xmlNodePtr node, virBandwidthPtr def) +{ + int ret = -1; + xmlNodePtr cur = node->children; + xmlNodePtr in = NULL, out = NULL; + + if (!node || !def || + !xmlStrEqual(node->name, BAD_CAST "bandwidth")) + return -1;

Again returning an error without logging it.

+ + memset(def, 0, sizeof(*def)); + while (cur) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "inbound")) { + if (in) { + virBandwidthError(VIR_ERR_XML_DETAIL, "%s", + _("Only one child<inbound> " + "element allowed")); + goto cleanup; + } + in = cur; + } else if (xmlStrEqual(cur->name, BAD_CAST "outbound")) { + if (out) { + virBandwidthError(VIR_ERR_XML_DETAIL, "%s", + _("Only one child<outbound> " + "element allowed")); + goto cleanup; + } + out = cur; + } + /* Silently ignore unknown elements */ + } + cur = cur->next; + } + + if (in&& virBandwidthParseChildDefNode(in,&def->in)< 0) + goto cleanup; + + if (out&& virBandwidthParseChildDefNode(out,&def->out)< 0) + goto cleanup; + + ret = 0; + +cleanup: + return ret; +} diff --git a/src/util/network.h b/src/util/network.h index af32558..54f7aad 100644 --- a/src/util/network.h +++ b/src/util/network.h @@ -20,6 +20,7 @@ # endif # include<netdb.h> # include<netinet/in.h> +# include "xml.h"

typedef struct { union { @@ -104,4 +105,5 @@ int virSocketAddrPrefixToNetmask(unsigned int prefix, virSocketAddrPtr netmask, int family);

+int virBandwidthDefParseNode(xmlNodePtr node, virBandwidthPtr def); #endif /* __VIR_NETWORK_H__ */

You need to log an error message in the two indicated places. I would also suggest using the existing virSocketError() macro rather than defining a new one, and you may want to think about making the bandwidth in the virNetworkDef into a virBandwidthPtr (although I'm really fine with it either way).

On 07/18/2011 04:05 PM, Michal Privoznik wrote:

--- src/conf/domain_conf.c | 3 ++ src/conf/network_conf.c | 3 ++ src/libvirt_private.syms | 1 + src/util/network.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/network.h | 4 ++ 5 files changed, 83 insertions(+), 0 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0d8c7e7..7bc6c1a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8839,6 +8839,9 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferAddLit(buf, "</tune>\n"); }

+ if (virBandwidrhDefFormat(buf,&def->bandwidth, " ")< 0) Why the strange spelling here? ("Bandwidrh")? I'm guessing it's a typo (since r and t are right next to each other); fortunately you're consistent about it, and have committed the same typo everywhere :-)

ACK with the typo fixed (although as I said in an earlier patch, I think several of these can be combined into one)

+ return -1; + if (virDomainDeviceInfoFormat(buf,&def->info, flags)< 0) return -1;

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c9929e2..43145b1 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1093,6 +1093,9 @@ char *virNetworkDefFormat(const virNetworkDefPtr def) if (virNetworkDNSDefFormat(&buf, def->dns)< 0) goto error;

+ if (virBandwidrhDefFormat(&buf,&def->bandwidth, " ")< 0) + goto error; + for (ii = 0; ii< def->nips; ii++) { if (virNetworkIpDefFormat(&buf,&def->ips[ii])< 0) goto error; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 12db3d7..1cc9bca 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -700,6 +700,7 @@ nlComm;

# network.h +virBandwidrhDefFormat; virBandwidthDefParseNode; virSocketAddrBroadcast; virSocketAddrBroadcastByPrefix; diff --git a/src/util/network.c b/src/util/network.c index ce949c7..58c0492 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -786,3 +786,75 @@ virBandwidthDefParseNode(xmlNodePtr node, virBandwidthPtr def) cleanup: return ret; } + +static int +virBandwidthChildDefFormat(virBufferPtr buf, + virRatePtr def, + const char *elem_name) +{ + if (!buf || !def || !elem_name) + return -1; + + if (def->average) { + virBufferAsprintf(buf, "<%s average='%lu'", elem_name, def->average); + + if (def->peak) + virBufferAsprintf(buf, " peak='%lu'", def->peak); + + if (def->burst) + virBufferAsprintf(buf, " burst='%lu'", def->burst); + virBufferAddLit(buf, "/>\n"); + } + + return 0; +} + +/** + * virBandwidrhDefFormat: + * @buf: Buffer to print to + * @def: Data source + * @indent: prepend all lines printed with this + * + * Formats bandwidth and prepend each line with @indent. + * Passing NULL to @indent is equivalent passing "". + * + * Returns 0 on success, else -1. + */ +int +virBandwidrhDefFormat(virBufferPtr buf, + virBandwidthPtr def, + const char *indent) +{ + int ret = -1; + + if (!buf || !def) + goto cleanup; + + if (!indent) + indent = ""; + + if (!def->in.average&& !def->out.average) { + ret = 0; + goto cleanup; + } + + virBufferAsprintf(buf, "%s<bandwidth>\n", indent); + if (def->in.average) { + virBufferAsprintf(buf, "%s ", indent); + if (virBandwidthChildDefFormat(buf,&def->in, "inbound")< 0) + goto cleanup; + } + + if (def->out.average) { + virBufferAsprintf(buf, "%s ", indent); + if (virBandwidthChildDefFormat(buf,&def->out, "outbound")< 0) + goto cleanup; + } + + virBufferAsprintf(buf, "%s</bandwidth>\n", indent); + + ret = 0; + +cleanup: + return ret; +} diff --git a/src/util/network.h b/src/util/network.h index 54f7aad..98e3082 100644 --- a/src/util/network.h +++ b/src/util/network.h @@ -21,6 +21,7 @@ # include<netdb.h> # include<netinet/in.h> # include "xml.h" +# include "buf.h"

typedef struct { union { @@ -106,4 +107,7 @@ int virSocketAddrPrefixToNetmask(unsigned int prefix, int family);

int virBandwidthDefParseNode(xmlNodePtr node, virBandwidthPtr def); +int virBandwidrhDefFormat(virBufferPtr buf, + virBandwidthPtr def, + const char *indent); #endif /* __VIR_NETWORK_H__ */

On 07/18/2011 04:05 PM, Michal Privoznik wrote:

These function executes 'tc' with appropriate arguments to set desired QoS setting on interface or bridge during its creation. --- configure.ac | 4 + src/libvirt_private.syms | 2 + src/network/bridge_driver.c | 12 +++ src/qemu/qemu_command.c | 10 +++- src/util/macvtap.c | 12 +++- src/util/macvtap.h | 5 +- src/util/network.c | 161 +++++++++++++++++++++++++++++++++++++++++++ src/util/network.h | 3 + 8 files changed, 205 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac index e9d5be4..2b864cf 100644 --- a/configure.ac +++ b/configure.ac @@ -165,6 +165,8 @@ AC_PATH_PROG([RADVD], [radvd], [radvd], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([BRCTL], [brctl], [brctl], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) +AC_PATH_PROG([TC], [tc], [tc], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH])

You may also want to add a note to the comment in libvirt.spec.in where we add "Requires: iproute" that we now need it for /sbin/tc, in addition to /sbin/ip.

AC_PATH_PROG([UDEVADM], [udevadm], [], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([UDEVSETTLE], [udevsettle], [], @@ -178,6 +180,8 @@ AC_DEFINE_UNQUOTED([RADVD],["$RADVD"], [Location or name of the radvd program]) AC_DEFINE_UNQUOTED([BRCTL],["$BRCTL"], [Location or name of the brctl program (see bridge-utils)]) +AC_DEFINE_UNQUOTED([TC],["$TC"], + [Location or name of the tc profram (see iproute2)]) if test -n "$UDEVADM"; then AC_DEFINE_UNQUOTED([UDEVADM],["$UDEVADM"], [Location or name of the udevadm program]) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1cc9bca..ff398d7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -702,6 +702,8 @@ nlComm; # network.h virBandwidrhDefFormat; virBandwidthDefParseNode; +virBandwidthDisable; +virBandwidthEnable; virSocketAddrBroadcast; virSocketAddrBroadcastByPrefix; virSocketAddrIsNetmask; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0a12bc0..47f9799 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1804,6 +1804,13 @@ networkStartNetworkDaemon(struct network_driver *driver, if (v6present&& networkStartRadvd(network)< 0) goto err4;

In my vswitch patches I've renamed networkStartNetworkDaemon to networkStartNetworkVirtual, and added separate functions for host bridge-based networks and macvtap networks, so 1) there will be a slight merge conflict, and 2) you will want to add a call to virBandwidthEnable at least to the host bridge version (I'm not sure how you can implement a network-wide bandwidth limit for the macvtap-based networks, since they can have multiple interfaces. Also, what happens when libvirt is restarted? Do we need to "reload" the tc configuration as we do with iptables rules (in case some other application has messed with them)?

+ if (virBandwidthEnable(&network->def->bandwidth, network->def->bridge)< 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot set bandwidth limits on %s"), + network->def->bridge); + goto err5; + } + /* Persist the live configuration now we have bridge info */ if (virNetworkSaveConfig(NETWORK_STATE_DIR, network->def)< 0) { goto err5; @@ -1895,6 +1902,11 @@ static int networkShutdownNetworkDaemon(struct network_driver *driver, unlink(stateFile); VIR_FREE(stateFile);

+ if (virBandwidthDisable(network->def->bridge, true)< 0) { + VIR_WARN("Failed to disable QoS on %s", + network->def->name); + } + if (network->radvdPid> 0) { char *radvdpidbase;

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a3bce4a..78cb865 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -128,7 +128,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def, rc = openMacvtapTap(net->ifname, net->mac, net->data.direct.linkdev, net->data.direct.mode, vnet_hdr, def->uuid, &net->data.direct.virtPortProfile,&res_ifname, - vmop, driver->stateDir); + vmop, driver->stateDir,&net->bandwidth); if (rc>= 0) { virDomainAuditNetDevice(def, net, res_ifname, true); VIR_FREE(net->ifname); @@ -291,6 +291,14 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, } }

+ if (virBandwidthEnable(&net->bandwidth, net->ifname)< 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot set bandwidth limits on %s"), + net->ifname); + VIR_FORCE_CLOSE(tapfd); + goto cleanup; + } + if (tapfd>= 0) { if ((net->filter)&& (net->ifname)) { err = virDomainConfNWFilterInstantiate(conn, net); diff --git a/src/util/macvtap.c b/src/util/macvtap.c index 30343c8..bb7784c 100644 --- a/src/util/macvtap.c +++ b/src/util/macvtap.c @@ -266,7 +266,8 @@ openMacvtapTap(const char *tgifname, virVirtualPortProfileParamsPtr virtPortProfile, char **res_ifname, enum virVMOperationType vmOp, - char *stateDir) + char *stateDir, + virBandwidthPtr bandwidth) { const char *type = "macvtap"; int c, rc; @@ -360,6 +361,15 @@ create_name: } else goto disassociate_exit;

+ if (virBandwidthEnable(bandwidth, cr_ifname)< 0) { + macvtapError(VIR_ERR_INTERNAL_ERROR, + _("cannot set bandwidth limits on %s"), + cr_ifname); + rc = -1; + goto disassociate_exit; + } + +

I thought somebody had said something earlier about bandwitdh control not working on macvtap interfaces... Have you tried this and seen it work?

return rc;

disassociate_exit: diff --git a/src/util/macvtap.h b/src/util/macvtap.h index 1b85989..84bbc92 100644 --- a/src/util/macvtap.h +++ b/src/util/macvtap.h @@ -24,7 +24,7 @@ # define __UTIL_MACVTAP_H__

# include<config.h> - +# include "network.h"

enum virVirtualPortType { VIR_VIRTUALPORT_NONE, @@ -95,7 +95,8 @@ int openMacvtapTap(const char *ifname, virVirtualPortProfileParamsPtr virtPortProfile, char **res_ifname, enum virVMOperationType vmop, - char *stateDir); + char *stateDir, + virBandwidthPtr bandwidth);

void delMacvtap(const char *ifname, const unsigned char *macaddress, diff --git a/src/util/network.c b/src/util/network.c index 58c0492..0a5652c 100644 --- a/src/util/network.c +++ b/src/util/network.c @@ -15,6 +15,7 @@ #include "network.h" #include "util.h" #include "virterror_internal.h" +#include "command.h"

#define VIR_FROM_THIS VIR_FROM_NONE #define virSocketError(code, ...) \ @@ -858,3 +859,163 @@ virBandwidrhDefFormat(virBufferPtr buf, cleanup: return ret; } + +/** + * virBandwidthEnable: + * @bandwidth: rates to set + * @iface: on which interface + * + * This function enables QoS on specified interface + * and set given traffic limits for both, incoming + * and outgoing traffic. Any previous setting get + * overwritten. + * + * Return 0 on success, -1 otherwise. + */ +int +virBandwidthEnable(virBandwidthPtr bandwidth, + const char *iface) +{ + int ret = -1; + virCommandPtr cmd = NULL; + char *average = NULL; + char *peak = NULL; + char *burst = NULL; + + if (!bandwidth || !iface) + return -1; + + if (!bandwidth->in.average&& + !bandwidth->out.average) { + /* nothing to be enabled */ + ret = 0; + goto cleanup; + } + + if (virBandwidthDisable(iface, true)< 0) + goto cleanup; + + if (bandwidth->in.average) { + if (virAsprintf(&average, "%lukbps", bandwidth->in.average)< 0) + goto cleanup; + if (bandwidth->in.peak&& + (virAsprintf(&peak, "%lukbps", bandwidth->in.peak)< 0)) + goto cleanup; + if (bandwidth->in.burst&& + (virAsprintf(&burst, "%lukb", bandwidth->in.burst)< 0)) + goto cleanup; + + cmd = virCommandNew(TC); + virCommandAddArgList(cmd, "qdisc", "add", "dev", iface, "root", + "handle", "1:", "htb", "default", "1", NULL); + if (virCommandRun(cmd, NULL)< 0) + goto cleanup; + + virCommandFree(cmd); + cmd = virCommandNew(TC); + virCommandAddArgList(cmd,"class", "add", "dev", iface, "parent", + "1:", "classid", "1:1", "htb", NULL); + virCommandAddArgList(cmd, "rate", average, NULL); + + if (peak) + virCommandAddArgList(cmd, "ceil", peak, NULL); + if (burst) + virCommandAddArgList(cmd, "burst", burst, NULL); + + if (virCommandRun(cmd, NULL)< 0) + goto cleanup; + + virCommandFree(cmd); + cmd = virCommandNew(TC); + virCommandAddArgList(cmd,"filter", "add", "dev", iface, "parent", + "1:0", "protocol", "ip", "handle", "1", "fw", + "flowid", "1", NULL);

I haven't used tc, but I trust these are all the right commands :-)

+ + if (virCommandRun(cmd, NULL)< 0) + goto cleanup; + + virCommandFree(cmd); + VIR_FREE(average); + VIR_FREE(peak); + VIR_FREE(burst); + } + + if (bandwidth->out.average) { + if (virAsprintf(&average, "%lukbps", bandwidth->out.average)< 0) + goto cleanup; + if (virAsprintf(&burst, "%lukb", bandwidth->out.burst ? + bandwidth->out.burst : bandwidth->out.average)< 0) + goto cleanup; + + cmd = virCommandNew(TC); + virCommandAddArgList(cmd, "qdisc", "add", "dev", iface, + "ingress", NULL); + + if (virCommandRun(cmd, NULL)< 0) + goto cleanup; + + virCommandFree(cmd); + cmd = virCommandNew(TC); + virCommandAddArgList(cmd, "filter", "add", "dev", iface, "parent", + "ffff:", "protocol", "ip", "u32", "match", "ip", + "src", "0.0.0.0/0", "police", "rate", average, + "burst", burst, "drop", "flowid", ":1", NULL); + + if (virCommandRun(cmd, NULL)< 0) + goto cleanup; + } + + ret = 0; + +cleanup: + virCommandFree(cmd); + VIR_FREE(average); + VIR_FREE(peak); + VIR_FREE(burst); + return ret; +} + +/** + * virBandwidthDisable: + * @iface: on which interface + * @may_fail: should be unsuccessful disable considered fatal? + * + * This function tries to disable QoS on specified interface + * by deleting root and ingress qdisc. However, this may fail + * if we try to remove the default one. + * + * Return 0 on success, -1 otherwise. + */ +int +virBandwidthDisable(const char *iface, + bool may_fail) +{ + int ret = -1; + int status; + virCommandPtr cmd = NULL; + + if (!iface) + return -1; + + cmd = virCommandNew(TC); + virCommandAddArgList(cmd, "qdisc", "del", "dev", iface, "root", NULL); + + if ((virCommandRun(cmd,&status)< 0) || + (!may_fail&& status)) + goto cleanup; + + virCommandFree(cmd); + + cmd = virCommandNew(TC); + virCommandAddArgList(cmd, "qdisc", "del", "dev", iface, "ingress", NULL); + + if ((virCommandRun(cmd,&status)< 0) || + (!may_fail&& status)) + goto cleanup; + + ret = 0; + +cleanup: + virCommandFree(cmd); + return ret; +} diff --git a/src/util/network.h b/src/util/network.h index 98e3082..17eab40 100644 --- a/src/util/network.h +++ b/src/util/network.h @@ -110,4 +110,7 @@ int virBandwidthDefParseNode(xmlNodePtr node, virBandwidthPtr def); int virBandwidrhDefFormat(virBufferPtr buf, virBandwidthPtr def, const char *indent); + +int virBandwidthEnable(virBandwidthPtr bandwidth, const char *iface); +int virBandwidthDisable(const char *iface, bool may_fail); #endif /* __VIR_NETWORK_H__ */

ACK modulo any rebasing that will need to be done (and assuming that nothing needs to be done in parallel with reloading iptables filters)

On 07/18/2011 04:05 PM, Michal Privoznik wrote:

--- tests/networkxml2xmlin/bandwidth-network.xml | 16 ++++++++++++++++ tests/networkxml2xmlout/bandwidth-network.xml | 16 ++++++++++++++++ tests/networkxml2xmltest.c | 1 + 3 files changed, 33 insertions(+), 0 deletions(-) create mode 100644 tests/networkxml2xmlin/bandwidth-network.xml create mode 100644 tests/networkxml2xmlout/bandwidth-network.xml

Again, I think this should be included with the RNG, parse, formatter, and docs in a single self-contained unit.

diff --git a/tests/networkxml2xmlin/bandwidth-network.xml b/tests/networkxml2xmlin/bandwidth-network.xml new file mode 100644 index 0000000..555ee18 --- /dev/null +++ b/tests/networkxml2xmlin/bandwidth-network.xml @@ -0,0 +1,16 @@ +<network> +<name>test-net</name> +<uuid>986fed9e-a488-186d-ef2d-17ebfd1993f8</uuid> +<forward mode='nat'/> +<bridge name='virbr1' stp='on' delay='0' /> +<mac address='52:54:00:E6:A2:C9'/> +<bandwidth> +<inbound average='1000' peak='2000' burst='1024'/> +<outbound average='2000'/> +</bandwidth> +<ip address='192.168.120.1' netmask='255.255.255.0'> +<dhcp> +<range start='192.168.120.2' end='192.168.120.254' /> +</dhcp> +</ip> +</network> diff --git a/tests/networkxml2xmlout/bandwidth-network.xml b/tests/networkxml2xmlout/bandwidth-network.xml new file mode 100644 index 0000000..555ee18 --- /dev/null +++ b/tests/networkxml2xmlout/bandwidth-network.xml @@ -0,0 +1,16 @@ +<network> +<name>test-net</name> +<uuid>986fed9e-a488-186d-ef2d-17ebfd1993f8</uuid> +<forward mode='nat'/> +<bridge name='virbr1' stp='on' delay='0' /> +<mac address='52:54:00:E6:A2:C9'/> +<bandwidth> +<inbound average='1000' peak='2000' burst='1024'/> +<outbound average='2000'/> +</bandwidth> +<ip address='192.168.120.1' netmask='255.255.255.0'> +<dhcp> +<range start='192.168.120.2' end='192.168.120.254' /> +</dhcp> +</ip> +</network> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 065166d..9dac7b4 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -88,6 +88,7 @@ mymain(void) DO_TEST("netboot-proxy-network"); DO_TEST("nat-network-dns-txt-record"); DO_TEST("nat-network-dns-hosts"); + DO_TEST("bandwidth-network");

return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); }

On 07/18/2011 04:05 PM, Michal Privoznik wrote:

--- tests/domainschemadata/domain-bandwidth.xml | 72 +++++++++++++++++++++++++++ 1 files changed, 72 insertions(+), 0 deletions(-) create mode 100644 tests/domainschemadata/domain-bandwidth.xml

Same comment applies as for 6/7 - this can go into one patch that combines all domain XML changes (domain.rng, domain_conf.c, formatdomain.html.in, and this).

diff --git a/tests/domainschemadata/domain-bandwidth.xml b/tests/domainschemadata/domain-bandwidth.xml new file mode 100644 index 0000000..852c97b --- /dev/null +++ b/tests/domainschemadata/domain-bandwidth.xml @@ -0,0 +1,72 @@ +<domain type='kvm'> +<name>f14-60</name> +<uuid>38644c45-5227-a936-3b38-bc4f72c355bb</uuid> +<memory>1048576</memory> +<currentMemory>1048576</currentMemory> +<vcpu>2</vcpu> +<os> +<type arch='x86_64' machine='pc-0.13'>hvm</type> +<boot dev='cdrom'/> +<boot dev='hd'/> +<bootmenu enable='yes'/> +</os> +<features> +<acpi/> +<apic/> +<pae/> +</features> +<clock offset='utc'/> +<on_poweroff>destroy</on_poweroff> +<on_reboot>restart</on_reboot> +<on_crash>restart</on_crash> +<devices> +<emulator>/usr/bin/qemu-kvm</emulator> +<disk type='file' device='disk'> +<driver name='qemu' type='qcow2'/> +<source file='/var/lib/libvirt/images/f14-6.img'/> +<target dev='vda' bus='virtio'/> +<address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> +</disk> +<disk type='file' device='cdrom'> +<driver name='qemu' type='raw'/> +<source file='/home/zippy/tmp/Fedora-14-x86_64-Live-KDE.iso'/> +<target dev='hdc' bus='ide'/> +<readonly/> +<address type='drive' controller='0' bus='1' unit='0'/> +</disk> +<controller type='ide' index='0'> +<address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> +</controller> +<controller type='virtio-serial' index='0'> +<address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> +</controller> +<interface type='network'> +<mac address='52:54:00:24:a5:9f'/> +<source network='default'/> +<bandwidth> +<inbound average='1000' peak='4000' burst='1024'/> +<outbound average='128' peak='256' burst='32768'/> +</bandwidth> +<address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> +</interface> +<serial type='pty'> +<target port='0'/> +</serial> +<console type='pty'> +<target type='serial' port='0'/> +</console> +<input type='tablet' bus='usb'/> +<input type='mouse' bus='ps2'/> +<graphics type='vnc' port='-1' autoport='yes'/> +<sound model='ac97'> +<address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> +</sound> +<video> +<model type='vga' vram='9216' heads='1'/> +<address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> +</video> +<memballoon model='virtio'> +<address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> +</memballoon> +</devices> +</domain>

On 21.07.2011 05:15, Taku Izumi wrote:

On Thu, 21 Jul 2011 10:52:32 +0800 Osier Yang<jyang@redhat.com> wrote:

...

于 2011年07月21日 09:35, Taku Izumi 写道:

...

On Wed, 20 Jul 2011 10:17:14 +0200 Michal Privoznik<mprivozn@redhat.com> wrote:

...

That's strange. Works for me. Can you please run: cd tests; VIR_TEST_DEBUG=1 ./domainschematest or ./networkschematest It should help me to trace error. Btw: I suppose current master runs without these errors for you. Thank you for your help. I tried again after getting the latest libvirt, but I could't solve this problem (make rpm failed..)

According to the following result, it seems to pass each test, but "make rpm" failed in my environment.

What's the error of "make rpm"?

I wrote the phenomena at my first mail. "networkschematest" and "domainschematest" failed.

The summary is as follows:

... TEST: networkschematest !!!!!!!!!!!!!!!!! 17 FAILED ... TEST: domainschematest !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! 40 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! 80 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! 120 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! 160 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! 200 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! 240 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! 280 !!!!!!!!!!!!!!!! 296 FAILED ... ======================================= 2 of 58 tests failed (2 tests were not run) Please report to libvir-list@redhat.com ======================================= make[2]: *** [check-TESTS] Error 1 make[2]: Leaving directory `/root/rpmbuild/BUILD/libvirt-0.9.3/tests' make[1]: *** [check-am] Error 2 make[1]: Leaving directory `/root/rpmbuild/BUILD/libvirt-0.9.3/tests' error: Bad exit status from /var/tmp/rpm-tmp.VfChp7 (%check)

RPM build errors: Bad exit status from /var/tmp/rpm-tmp.VfChp7 (%check) make: *** [rpm] エラー 1

This is the patch that will solve your problem: diff --git a/docs/schemas/Makefile.am b/docs/schemas/Makefile.am index 5ef7737..a08fb43 100644 --- a/docs/schemas/Makefile.am +++ b/docs/schemas/Makefile.am @@ -2,6 +2,7 @@ schemadir = $(pkgdatadir)/schemas schema_DATA = \ + bandwidth.rng \ basictypes.rng \ capability.rng \ domain.rng \ I'll squash it into my patchset as well. Michal