On 11/19/2012 11:51 AM, Michal Privoznik wrote:
> 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
> <source network='default'/>
> <target dev='vnet0'/>
> <b><bandwidth>
> - <inbound average='1000' peak='5000' burst='1024'/>
> + <inbound average='1000' peak='5000' floor='200' burst='1024'/>
> <outbound average='128' peak='256' burst='256'/>
> </bandwidth></b>
> </interface>
> @@ -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
s|It|<code>average</code>|
> interface being shaped. Then there are two optional attributes:
*the* interface being shaped
> <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><interface type='network'/></code>.
with a forward type of route, nat, or no forward at all (i.e. forward
type can't be 'bridge' or 'hostdev')
> 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)))
Of course the nominal type of an interface could be "network", but when
it's actually allocated it could end up being one with fordward
mode='bridge|hostdev', so you need to validate again at runtime. I
haven't looked ahead yet to see if you're doing that.
> 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;
> }
I didn't understand if floor and average were mutually exclusive. If
that's the case, then you need to check for it here. Otherwise, I just
need to understand better :-)
>
> @@ -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"),
mention in the log that it this was for the floor attribute, e.g.
"could not convert bandwidth floor value '%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"));
It would be nice to have a slightly different error message when someone
tried to specify floor on a network's bandwidth (like "setting the floor
attribute is not supported for an entire network's bandwidth". (I don't
really like the wording of what I just said, but you get the idea. MY
coffee hasn't kicked in yet :-). I guess you could do this based on the
magic value net_type == -1, although that sounds a bit kludgy).
> + 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 */
> };
>
ACK with the minor things fixed (the bit about having a different log
message for network vs. interface is optional; or maybe you can think of
a single message that works better for both cases).
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list