added enabled_all_classes to show all classes of an interface #1427

Open
wants to merge 4 commits into
from

Projects

None yet

2 participants

@t-h-e
t-h-e commented Dec 23, 2016

default is no

#1375

@t-h-e
t-h-e commented Jan 3, 2017

I have added qdiscs and renamed the flag "enabled_all_classes" to "enabled_all_classes_qdiscs".
If the flag is set to no, everything should work as it always has.
If it is set to yes, all classes and qdiscs are displayed in the chart. Therefore, the Inbound and Outbound gauge charts are not correct anymore and the chart types should be changed to line to show accurate statistics.

src/plugin_tc.c
- dropped_sum += c->dropped;
- tokens_sum += c->tokens;
- ctokens_sum += c->ctokens;
+ debug(D_TC_LOOP, "TC: Device '%s', class '%s', isleaf=%s, isqdisc=%s, hasparent=%s bytes=%d, packtes=%d, dropped=%d, tokens=%d, ctokens=%d", d->name?d->name:d->id, c->name?c->name:c->id, c->isleaf?"true":"false", c->isqdisc?"true":"false", c->hasparent?"true":"false", c->bytes, c->packets, c->dropped, c->tokens, c->ctokens);
@ktsaou
ktsaou Jan 6, 2017 Member

This gives warnings - please compile with -Wall -Wextra -Wformat-signedness.
All %d should be %llu.

src/plugin_tc.c
- tokens_sum += c->tokens;
- ctokens_sum += c->ctokens;
+ debug(D_TC_LOOP, "TC: Device '%s', class '%s', isleaf=%s, isqdisc=%s, hasparent=%s bytes=%d, packtes=%d, dropped=%d, tokens=%d, ctokens=%d", d->name?d->name:d->id, c->name?c->name:c->id, c->isleaf?"true":"false", c->isqdisc?"true":"false", c->hasparent?"true":"false", c->bytes, c->packets, c->dropped, c->tokens, c->ctokens);
+ if(unlikely(c->updated && c->isqdisc && !c->parentid)) {
@ktsaou
ktsaou Jan 6, 2017 Member

this will prevent the identification of the root class, if classes are given.

src/plugin_tc.c
+ if(strcmp(words[0], "qdisc") == 0) {
@ktsaou
ktsaou Jan 6, 2017 Member

this can be if(first_hash == QDISC_HASH)

@ktsaou
Member
ktsaou commented Jan 6, 2017

nice job!

One problem: the code was calculating the sum of bytes, packets etc. Now I see that you preferred to use the root qdisc instead of summing up all visible classes/qdiscs.

This change, will not work for if the tc-qos-helper sends classes instead of qdiscs.

Also, keep in mind that tc is hierarchical. So, classes can have classes which can have classes and so on. The code was skipping classes that had both a parent and children (i.e. !isleaf && hasparent). This is crucial I think for both classes and qdiscs if we want to the chart to be realistic (i.e. when !enabled_all_classes_qdiscs).

@ktsaou
Member
ktsaou commented Jan 6, 2017

note to me

Once this is merged, I see the code does a lot of duplicate checks for updating the names of dimensions, calling frequently strcmp().

A simple fix is to make sure class->name_updated is set to 1 only when the name is really updated (i.e. is not the same with class->id and not the same with any previous value of class->name). This fix has to be made in tc_device_set_class_name().

Once we have a good class->name_updated, we could replace all these:

if(unlikely(c->name_updated && c->name && strcmp(c->id, c->name) != 0))

which are executed for each dimension and each chart, with:

if(unlikely(c->name_updated))
@t-h-e t-h-e small adjustments to plugin_tc.c
according to @ktsaou comments.
bb1e13a
@t-h-e
t-h-e commented Jan 17, 2017

the code was calculating the sum of bytes, packets etc.

The values of these variables are only shown in the debug information and are only used to check if something has changed since last time. Everything will work as before. Qdisc information will always be parsed, independent of the flag enabled_all_classes_qdiscs.

Also, keep in mind that tc is hierarchical.

Classes can have classes in most cases. For drr this is not the case. Although you can call tc to add a drr class to another drr class, in the end the "leaf" class will still be attached to the qdisc and not to the other class. Therefor, no QoS information is shown for drr if it is used as root qdisc.

Another problem arises, if someone is not using QDISC_ID:1 as classid for a class which is directly attached to the qdisc. Then classes, which are actually not leaf classes, are still displayed. Parsing qdisc might be necessary to properly check the hierarchy and to avoid this problem. See:

INTDEV=wlp4s0
LOCALDOWN=100000
tc qdisc add dev $INTDEV root handle 1: htb default 9
tc class add dev $INTDEV parent 1: classid 1:1 htb rate ${LOCALDOWN}kbit
tc class add dev $INTDEV parent 1:1 classid 1:9 htb rate ${LOCALDOWN}kbit
tc qdisc add dev $INTDEV parent 1:9 handle 5: htb default 9
# if the root class of a qdisc does not start with 1, multiple "leaf" classes are displayed
tc class add dev $INTDEV parent 5: classid 5:3 htb rate ${LOCALDOWN}kbit
tc class add dev $INTDEV parent 5:3 classid 5:9 htb rate ${LOCALDOWN}kbit
tc qdisc add dev $INTDEV parent 5:9 fq_codel quantum 300 ecn

If enabled_all_classes_qdiscs is set to true, the charts should be generally filtered with data-dimensions and set to line, otherwise the chart might show redundant information and/or show completelly wrong accumulated values as all values from all classes and qdiscs will be summed that are displayed.

The other comments have been fixed. Thanks for your feedback.

@ktsaou
Member
ktsaou commented Jan 17, 2017

Thanks. ok.

You probably missed this:

One problem: the code was calculating the sum of bytes, packets etc. Now I see that you preferred to use the root qdisc instead of summing up all visible classes/qdiscs.

This change, will not work for if the tc-qos-helper sends classes instead of qdiscs.

The above refers to this code fragment (at it was before this PR):

    for(c = d->classes ; c ; c = c->next) {
        // debug(D_TC_LOOP, "TC: Device '%s', class '%s', isLeaf=%d, HasParent=%d, Seen=%d", d->name?d->name:d->id, c->name?c->name:c->id, c->isleaf, c->hasparent, c->seen);
        if(unlikely(c->updated && c->isleaf && c->hasparent)) {
            active_classes++;
            bytes_sum += c->bytes;
            packets_sum += c->packets;
            dropped_sum += c->dropped;
            tokens_sum += c->tokens;
            ctokens_sum += c->ctokens;
        }
    }

The purpose of the above is to calculate the a few sums, so that later we can decide if there is meaning to enable the charts (i.e. if there are no dropped packets, this chart will not be shown - it will not even occupy any memory).

The code that replaced this, does this:

if(unlikely(c->updated && c->isqdisc && !c->parentid)) {

The above will not work for classes, since it requires c->isqdisc to be set.

@ktsaou
Member
ktsaou commented Jan 17, 2017

if enabled_all_classes_qdiscs is set to true, the charts should be generally filtered with data-dimensions and set to line,

You can change RRDSET_TYPE_STACKED to RRDSET_TYPE_LINE when enabled_all_classes_qdiscs is set. Do not change the plugin_tc_cpu chart though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment