[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v1 01/11] bandwidth: add new 'floor' attribute



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
>        &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

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>&lt;interface type='network'/&gt;</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).


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]