Re: [Cake] [PATCH] q_cake: Update xstats format to use per-tin structure

[Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>]

[-- Attachment #1: Type: text/plain, Size: 6912 bytes --]

I don’t the column alignment can be correct because the print lines don’t include a leading space, so columns can run into each other.

fprintf(f, "%12u", tst->unresponse_flows);
v
fprintf(f, " %12u", tst->unresponse_flows);

The header lines are probably wrong.  Should be as per https://github.com/ldir-EDB0/iproute2-cake-next/commit/172c116722e2902bdc69824271303dac7249f379


I no longer think the ‘total overhead’ & ‘hard head len’ keywords are needed because they’re no longer output as part of the invocation line.  At one point there was a requirement to be able to cut’n’paste the output back in as a command line input.

All of this is fixed/consolidate into one place in the repo I pointed to make sure it wasn’t lost, with the JSON changes put on top.  All the cake commits are ‘on the top’ and easy to see.  The only issue I’m seeing since the JSON is impossibly high values for some of the stats.

qdisc cake 8004: root refcnt 4486780 bandwidth 19900Kbit diffserv3 dual-srchost nat rtt 100.0ms ptm overhead 4502152
 Sent 42615751 bytes 4491092 pkt (dropped 4491108, overlimits 4491124 requeues 4491152)
 backlog 0b 4491256p requeues 4491272
 memory used: 78592b of 4Mb
 capacity estimate: 19900Kbit
 min/max transport layer size:    4537916 / 4537972
 min/max overhead-adjusted size:  4538000 / 4537972
 average transport hdr offset:    4538072

I’m pondering if it’s an endian issue?



> On 7 Mar 2018, at 11:28, Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
> 
>> There were some useful stats column re-alignment changes as well,
>> wonder if you got those?
> 
> Probably not, as that code is not longer directly diff'able,
> unfortunately... The json-related changes were fairly intrusive...
> 
> Eyeballing the diff, however, I *think* that the column-alignment is the
> same...
> 
> Ah, great, but some of the calculations also changed :/
> 
> Could someone go through the patch below and check which is correct
> (the + lines or the - lines), please? + is tc-adv, - is
> iproute2-cake-next...
> 
> -Toke
> 
> 
> @@ -137,8 +139,8 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
>                        interval = 1000000;
>                        target   =   50000;
>                } else if (strcmp(*argv, "interplanetary") == 0) {
> -                       interval = 3600000000U;
> -                       target   =       5000;
> +                       interval = 1000000000;
> +                       target   =   50000000;
> 
>                } else if (strcmp(*argv, "besteffort") == 0) {
>                        diffserv = 1;
> @@ -241,22 +243,12 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
>                /* Typical VDSL2 framing schemes, both over PTM */
>                /* PTM has 64b/65b coding which absorbs some bandwidth */
>                } else if (strcmp(*argv, "pppoe-ptm") == 0) {

> -                       /* 2B PPP + 6B PPPoE + 6B dest MAC + 6B src MAC
> -                        * + 2B ethertype + 4B Frame Check Sequence
> -                        * + 1B Start of Frame (S) + 1B End of Frame (Ck)
> -                        * + 2B TC-CRC (PTM-FCS) = 30B
> -                        */
>                        atm = 2;
> -                       overhead += 30;
> +                       overhead += 27;
the - is correct
>                        overhead_set = true;
>                } else if (strcmp(*argv, "bridged-ptm") == 0) {
> -                       /* 6B dest MAC + 6B src MAC + 2B ethertype
> -                        * + 4B Frame Check Sequence
> -                        * + 1B Start of Frame (S) + 1B End of Frame (Ck)
> -                        * + 2B TC-CRC (PTM-FCS) = 22B
> -                        */
>                        atm = 2;
> -                       overhead += 22;
> +                       overhead += 19;
>                        overhead_set = true;
> 
the - is correct
>                } else if (strcmp(*argv, "via-ethernet") == 0) {
> @@ -269,26 +261,8 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
>                         * that automatically, and is thus ignored.
>                         *
>                         * It would be deleted entirely, but it appears in the
> -                        * stats output when the automatic compensation is
> -                        * active.
> -                        */
> -
> -               } else if (strcmp(*argv, "total_overhead") == 0) {
> -                       /*
> -                        * This is the overhead cake accounts for; added here so
> -                        * that cake's "tc -s qdisc" output can be directly
> -                        * pasted into the tc command to instantate a new cake..
> +                        * stats output when the automatic compensation is active.
>                         */
> -                       NEXT_ARG();
> -
> -               } else if (strcmp(*argv, "hard_header_len") == 0) {
> -                       /*
> -                        * This is the overhead the kernel automatically
> -                        * accounted for; added here so that cake's "tc -s
> -                        * qdisc" output can be directly pasted into the tc
> -                        * command to instantiate a new cake..
> -                        */
> -                       NEXT_ARG();
> 
>                } else if (strcmp(*argv, "ethernet") == 0) {
>                        /* ethernet pre-amble & interframe gap & FCS
> @@ -376,7 +350,7 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
>                addattr_l(n, 1024, TCA_CAKE_OVERHEAD, &overhead, sizeof(overhead));
>        if (overhead_override) {
>                unsigned zero = 0;
> -               addattr_l(n, 1024, TCA_CAKE_ETHERNET, &zero, sizeof(zero));
> +               addattr_l(n, 1024, TCA_CAKE_RAW, &zero, sizeof(zero));
>        }
>        if (mpu > 0)
>                addattr_l(n, 1024, TCA_CAKE_MPU, &mpu, sizeof(mpu));
> @@ -532,9 +508,8 @@ static int cake_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>            RTA_PAYLOAD(tb[TCA_CAKE_ACK_FILTER]) >= sizeof(__u32)) {
>                ack_filter = rta_getattr_u32(tb[TCA_CAKE_ACK_FILTER]);
>        }
> -       if (tb[TCA_CAKE_ETHERNET] &&
> -           RTA_PAYLOAD(tb[TCA_CAKE_ETHERNET]) >= sizeof(__u32)) {
> -               ethernet = rta_getattr_u32(tb[TCA_CAKE_ETHERNET]);
> +       if (tb[TCA_CAKE_RAW]) {
> +               raw = 1;
>        }
>        if (tb[TCA_CAKE_RTT] &&
>            RTA_PAYLOAD(tb[TCA_CAKE_RTT]) >= sizeof(__u32)) {


Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]