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

Re: [libvirt] [PATCH v1 09/11] network: Allow class ID to be reused



On 11/19/2012 11:51 AM, Michal Privoznik wrote:
> With current implementation, class ID is just incremented. This can
> lead to its exhaustion as tc accepts only 16 bits long identifiers.
> Hence, it's better if we allow class ID to be reused. To keep track
> which IDs are free and which are taken virBitmap is used. This requires
> network status file to change a bit: from <class_id next="5"/> to
> <class_id bitmap="0-4"/>. But since the previous format hasn't been
> released, it doesn't really matter.

Heh. Well, there you have it. :-) You've already implemented what I
suggested in the review of 5/10. But rather than introducing one
implementation that we need to review, then almost immediately replacing
it with something else, why not just implement it this way to begin with?

> ---
>  src/conf/network_conf.c     |   34 +++++++++++++++++++++++++----
>  src/conf/network_conf.h     |    3 +-
>  src/network/bridge_driver.c |   49 ++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 72 insertions(+), 14 deletions(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 9c2e4d4..a41119c 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -47,6 +47,8 @@
>  
>  #define MAX_BRIDGE_ID 256
>  #define VIR_FROM_THIS VIR_FROM_NETWORK
> +/* currently, /sbin/tc implementation allows up 16 bits for minor class size */
> +#define CLASS_ID_BITMAP_SIZE (1<<16)
>  
>  VIR_ENUM_IMPL(virNetworkForward,
>                VIR_NETWORK_FORWARD_LAST,
> @@ -317,13 +319,29 @@ virNetworkAssignDef(virNetworkObjListPtr nets,
>          return NULL;
>      }
>      virNetworkObjLock(network);
> -    network->def = def;
> -    network->class_id = 3; /* next free class id */
>  
> +    if (!(network->class_id = virBitmapNew(CLASS_ID_BITMAP_SIZE))) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +
> +    if (virBitmapSetBit(network->class_id, 0) < 0 ||
> +        virBitmapSetBit(network->class_id, 1) < 0 ||
> +        virBitmapSetBit(network->class_id, 2) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("unable to initialize class id bitmap"));
> +        goto error;
> +    }
> +
> +    network->def = def;
>      nets->objs[nets->count] = network;
>      nets->count++;
>  
>      return network;
> +error:
> +    virNetworkObjUnlock(network);
> +    virNetworkObjFree(network);
> +    return NULL;
>  
>  }
>  
> @@ -1673,9 +1691,10 @@ virNetworkObjUpdateParseFile(const char *filename,
>          char *floor_sum = NULL;
>  
>          ctxt->node = node;
> -        class_id = virXPathString("string(./class_id[1]/@next)", ctxt);
> +        class_id = virXPathString("string(./class_id[1]/@bitmap)", ctxt);
>          if (class_id &&
> -            virStrToLong_ui(class_id, NULL, 10, &net->class_id) < 0) {
> +            virBitmapParse(class_id, ',',
> +                           &net->class_id, CLASS_ID_BITMAP_SIZE) < 0) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("Malformed 'class_id' attribute: %s"),
>                             class_id);
> @@ -2054,10 +2073,15 @@ virNetworkObjFormat(virNetworkObjPtr net,
>                      unsigned int flags)
>  {
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    char *class_id = virBitmapFormat(net->class_id);
> +
> +    if (!class_id)
> +        goto no_memory;
>  
>      virBufferAddLit(&buf, "<networkstatus>\n");
> -    virBufferAsprintf(&buf, "  <class_id next='%u'/>\n", net->class_id);
> +    virBufferAsprintf(&buf, "  <class_id bitmap='%s'/>\n", class_id);
>      virBufferAsprintf(&buf, "  <floor sum='%llu'/>\n", net->floor_sum);
> +    VIR_FREE(class_id);
>  
>      virBufferAdjustIndent(&buf, 2);
>      if (virNetworkDefFormatInternal(&buf, net->def, flags) < 0)
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index efa9dea..6f6b221 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -38,6 +38,7 @@
>  # include "virnetdevvlan.h"
>  # include "virmacaddr.h"
>  # include "device_conf.h"
> +# include "bitmap.h"
>  
>  enum virNetworkForwardType {
>      VIR_NETWORK_FORWARD_NONE   = 0,
> @@ -222,7 +223,7 @@ struct _virNetworkObj {
>      virNetworkDefPtr def; /* The current definition */
>      virNetworkDefPtr newDef; /* New definition to activate at shutdown */
>  
> -    unsigned int class_id; /* next class ID for QoS */
> +    virBitmapPtr class_id; /* bitmap of class IDs for QoS */
>      unsigned long long floor_sum; /* sum of all 'floor'-s of attached NICs */
>  };
>  
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 5a0f43f..8341a78 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -4249,6 +4249,31 @@ cleanup:
>      return ret;
>  }
>  
> +/**
> + * networkNextClassID:
> + * @net: network object
> + *
> + * Find next free class ID. @net is supposed
> + * to be locked already. If there is a free ID,
> + * it is marked as used and returned.
> + *
> + * Returns next free class ID or -1 if none is available.
> + */
> +static ssize_t
> +networkNextClassID(virNetworkObjPtr net)
> +{
> +    size_t ret = 0;
> +    bool is_set = false;
> +
> +    while (virBitmapGetBit(net->class_id, ret, &is_set) == 0 && is_set)
> +        ret++;
> +
> +    if (is_set || virBitmapSetBit(net->class_id, ret) < 0)
> +        return -1;
> +
> +    return ret;
> +}
> +
>  int
>  networkNotifyPlug(virNetworkPtr network,
>                    virDomainNetDefPtr iface)
> @@ -4258,6 +4283,7 @@ networkNotifyPlug(virNetworkPtr network,
>      int ret = -1;
>      int plug_ret;
>      unsigned long long new_rate = 0;
> +    ssize_t class_id = 0;
>  
>      networkDriverLock(driver);
>      net = virNetworkFindByUUID(&driver->networks, network->uuid);
> @@ -4274,15 +4300,21 @@ networkNotifyPlug(virNetworkPtr network,
>          goto cleanup;
>      }
>  
> +    /* generate new class_id */
> +    if ((class_id = networkNextClassID(net)) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Could not generate next class ID"));
> +        goto cleanup;
> +    }
> +
>      plug_ret = virNetDevBandwidthPlug(net->def->bridge,
>                                        net->def->bandwidth,
>                                        iface->ifname,
>                                        &iface->mac,
>                                        iface->bandwidth,
> -                                      net->class_id);
> +                                      class_id);
>      if (plug_ret < 0) {
> -        ignore_value(virNetDevBandwidthUnplug(net->def->bridge,
> -                                              net->class_id));
> +        ignore_value(virNetDevBandwidthUnplug(net->def->bridge, class_id));
>          goto cleanup;
>      }
>  
> @@ -4296,17 +4328,15 @@ networkNotifyPlug(virNetworkPtr network,
>      }
>  
>      /* QoS was set, generate new class ID */
> -    iface->class_id = net->class_id;
> -    net->class_id++;
> +    iface->class_id = 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--;
> +        ignore_value(virBitmapClearBit(net->class_id, class_id));
>          net->floor_sum -= iface->bandwidth->in->floor;
>          iface->class_id = 0;
> -        ignore_value(virNetDevBandwidthUnplug(net->def->bridge,
> -                                              net->class_id));
> +        ignore_value(virNetDevBandwidthUnplug(net->def->bridge, class_id));
>          goto cleanup;
>      }
>      /* update rate for non guaranteed NICs */
> @@ -4348,9 +4378,12 @@ networkNotifyUnplug(virDomainNetDefPtr iface)
>              goto cleanup;
>          /* update sum of 'floor'-s of attached NICs */
>          net->floor_sum -= iface->bandwidth->in->floor;
> +        /* return class ID */
> +        ignore_value(virBitmapClearBit(net->class_id, iface->class_id));
>          /* update status file */
>          if (virNetworkSaveStatus(NETWORK_STATE_DIR, net) < 0) {
>              net->floor_sum += iface->bandwidth->in->floor;
> +            ignore_value(virBitmapSetBit(net->class_id, iface->class_id));
>              goto cleanup;
>          }
>          /* update rate for non guaranteed NICs */


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