[libvirt] [PATCH v1 11/11] bandwidth: Add documentation

[Laine Stump]

On 11/19/2012 11:51 AM, Michal Privoznik wrote:
> This approach implemented in previous patches is not trivial
> and deserves small description.
> ---
>  src/util/virnetdevbandwidth.c |   68 +++++++++++++++++++++++++++++++++++++---
>  1 files changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
> index b4ffc29..d32c7db 100644
> --- a/src/util/virnetdevbandwidth.c
> +++ b/src/util/virnetdevbandwidth.c
> @@ -92,13 +92,69 @@ virNetDevBandwidthSet(const char *ifname,
>          if (virCommandRun(cmd, NULL) < 0)
>              goto cleanup;
>  
> +        /* If we are creating hierarchical class, all non guaranteed traffic

    "creating *a* hierarchical class"

> +         * goes to 1:2 class which will adjust 'rate' dynamically as NICs with

   "goes to *the& 1:2 class"

> +         * guaranteed throughput are plugged and unplugged. Class 1:1 is there

    s/is there/exists/

> +         * so we don't exceed the maximum limit for network. For each NIC with

   "for *the* network"

> +         * guaranteed throughput a separate classid will be created.
> +         * NB '1:' is just a shorter notation of '1:0'.
> +         *
> +         * To get a picture how this works:
> +         *
> +         * +-----+     +---------+     +-----------+      +-----------+     +-----+
> +         * |     |     |  qdisc  |     | class 1:1 |      | class 1:2 |     |     |
> +         * | NIC |     | def 1:2 |     |   rate    |      |   rate    |     | sfq |
> +         * |     | --> |         | --> |   peak    | -+-> |   peak    | --> |     |
> +         * +-----+     +---------+     +-----------+  |   +-----------+     +-----+
> +         *                                            |
> +         *                                            |   +-----------+     +-----+
> +         *                                            |   | class 1:3 |     |     |
> +         *                                            |   |   rate    |     | sfq |
> +         *                                            +-> |   peak    | --> |     |
> +         *                                            |   +-----------+     +-----+
> +         *                                           ...
> +         *                                            |   +-----------+     +-----+
> +         *                                            |   | class 1:n |     |     |
> +         *                                            |   |   rate    |     | sfq |
> +         *                                            +-> |   peak    | --> |     |
> +         *                                                +-----------+     +-----+
> +         *
> +         * After the routing decision, when is it clear a packet is to be send

   s/send/sent/

> +         * via NIC, it is sent to root qdisc (queueing discipline). In this case

   "via *a particular* NIC, it is sent to *the* root qdisc" (for that NIC?)
> +         * HTB (Hierarchical Token Bucket). It has only one direct child class
> +         * (with id 1:1) which shapes the overall rate that is sent through NIC.

   "through the NIC"

> +         * This class have at least one child (1:2). This is meant for whole

     s/have/has/
     What does "is meant" mean here? A better phrase is needed
     s/whole/all/

> +         * non-privileged (non guaranteed) traffic from all domains. Then, for
> +         * each interface with guaranteed throughput a separate class (1:n) is

      put a comma after throughput

> +         * created. Imagine a class is a box. Whenever a packet ends up in a
> +         * class it is stored in this box until a kernel sends it in which case

   s/a kernel/the kernel/
   s/in which case/, then/

> +         * it is removed from box. Packets are placed into boxes based on rules
> +         * (filters) - e.g. depending on destination IP/MAC address. If there is
> +         * no rule to be applied, root qdisc have a default where such packets

   "*the* root qdisc *has* a default"

> +         * go (1:2 in this case). Packets come in over and over again and boxes
> +         * get filled more and more. Imagine that kernel sends packets just once

   s/kernel/the kernel/

> +         * a second. So it starts to traverse through this tree. It starts with
> +         * root qdisc and over 1:1 it gets to 1:2. It sends packets up to its
s/root/the root/
s/over/through/

What does "its" refer to? 1:2? If so, use "1:2's", because "its" is
ambiguous.

> +         * 'rate'. Then it takes 1:3 and again sends packets up to its 'rate'.

s/takes/moves to/
Again, "its" is kind of ambiguous. I *think* you mean 1:3's rate, but
because you've repeatedly referred to the kernel as "it", someone may be
confused into thinking that the kernel has a 'rate', or something
equally inane.


> +         * And the whole process is repeated until 1:n is processed. So now we

s/And the/The/

What does "until 1:n is processed" mean? That if there are "n" classes,
all of them will be processed? In that case, maybe you could change it
to something like "The whole process is repeated until all classes 1:1 -
1:n are processed."

> +         * have ensured each class its guaranteed bandwidth. If the sum of sent
> +         * data doesn't exceed 'rate' in 1:1 class, we can go further and send

   "exceed *the* 'rate'"

> +         * more packets. The rest of available bandwidth is distributed to

    s/to/to the/

> +         * 1:2,1:3...1:n classes by ratio of their 'rate'. As soon as root

   "as *the* root"

> +         * 'rate' limit is reached or there are no more packets to send, we stop
> +         * sending and wait another second. Each class has SFQ qdisc which

   "has *an* SFQ qdisc"

> +         * shuffles packets in boxes stochastically, so one sender could not

    s/could/can/

> +         * starve others.
> +         *
> +         * Therefore, whenever we want to plug a new guaranteed interface, we

   s/plug/plug in/

> +         * need to create a new class and adjust 'rate' of 1:2 class. When

    "adjust *the* 'rate' of *the* 1:2 class."

> +         * unplugging we do the exact opposite - remove associated class, and

   "remove *the* associated class"

> +         * adjust the 'rate'.
> +         *
> +         * This description is rather longer and you'd better read it before you
> +         * start digging into this :)

   "This description is rather long, but it is still a good idea to read
it before you dig into the code"


> +         */
>          if (hierarchical_class) {
> -            /* If we are creating hierarchical class, all non guaranteed traffic
> -             * goes to 1:2 class which will adjust 'rate' dynamically as NICs with
> -             * guaranteed throughput are plugged and unplugged. Class 1:1 is there
> -             * so we don't exceed the maximum limit for network. For each NIC with
> -             * guaranteed throughput a separate classid will be created.
> -             * NB '1:' is just a shorter notation of '1:0' */
>              virCommandFree(cmd);
>              cmd = virCommandNew(TC);
>              virCommandAddArgList(cmd, "class", "add", "dev", ifname, "parent",

ACK with those changes, but please squash it into the patch that adds
the code.