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 */
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list