Re: tc: show format ABI changed

[Jarek Poplawski <jarkao2@gmail.com>]

On Wed, Dec 08, 2010 at 02:51:36PM -0800, Stephen Hemminger wrote:
> Although well intentioned, the following patch should have not
> been applied since it changes the kernel ABI. It broke some scripts
> parsing the output format of tc commands.

I doubt you can blame this patch for changing any ABI. If there is
such a thing documented you would probably point us there. Otherwise,
it should be seen in the code, and tc has it all conditional:

        if (tbs[TCA_STATS_RATE_EST]) {
		...
                fprintf(fp, "\n%srate %s %upps ",
                        prefix, sprint_rate(re.bps, b1), re.pps);
        }
 
        if (tbs[TCA_STATS_QUEUE]) {
		...
                if (!tbs[TCA_STATS_RATE_EST])
                        fprintf(fp, "\n%s", prefix);
                fprintf(fp, "backlog %s %up requeues %u ",
                        sprint_size(q.backlog, b1), q.qlen, q.requeues);
        }

which suggests scripts should use similar logic.

Anyway, these are tc's printouts and it should handle source data
errors too. Since it's accepted it can't be wrong ;-)

Jarek P.

> 
> Before HTB would report bogus zero values, now it reports
> nothing and that changes the output format.  Like the empty
> fields in /proc, I argue we can't play fast and loose with
> netlink responses.
> 
> Not a big deal to fix the script in this case, in this case
> so don't revert it.
> 
> commit d250a5f90e53f5e150618186230795352d154c88
> Author: Eric Dumazet <eric.dumazet@gmail.com>
> Date:   Fri Oct 2 10:32:18 2009 +0000
> 
>     pkt_sched: gen_estimator: Dont report fake rate estimators
>     
>     Jarek Poplawski a écrit :
>     >
>     >
>     > Hmm... So you made me to do some "real" work here, and guess what?:
>     > there is one serious checkpatch warning! ;-) Plus, this new parameter
>     > should be added to the function description. Otherwise:
>     > Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
>     >
>     > Thanks,
>     > Jarek P.
>     >
>     > PS: I guess full "Don't" would show we really mean it...
>     
>     Okay :) Here is the last round, before the night !
>     
>     Thanks again
>     
>     [RFC] pkt_sched: gen_estimator: Don't report fake rate estimators
>     
>     We currently send TCA_STATS_RATE_EST elements to netlink users, even if no estimator
>     is running.
>     
>     # tc -s -d qdisc
>     qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
>      Sent 112833764978 bytes 1495081739 pkt (dropped 0, overlimits 0 requeues 0)
>      rate 0bit 0pps backlog 0b 0p requeues 0
>     
>     User has no way to tell if the "rate 0bit 0pps" is a real estimation, or a fake
>     one (because no estimator is active)
>     
>     After this patch, tc command output is :
>     $ tc -s -d qdisc
>     qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
>      Sent 561075 bytes 1196 pkt (dropped 0, overlimits 0 requeues 0)
>      backlog 0b 0p requeues 0
>     
>     We add a parameter to gnet_stats_copy_rate_est() function so that
>     it can use gen_estimator_active(bstats, r), as suggested by Jarek.
>     
>     This parameter can be NULL if check is not necessary, (htb for
>     example has a mandatory rate estimator)
>     
>     Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>     Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>