tc: show format ABI changed

tc: show format ABI changed

[Stephen Hemminger]

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.

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>

Re: tc: show format ABI changed

[Eric Dumazet]

Le mercredi 08 décembre 2010 à 14:51 -0800, Stephen Hemminger a écrit :
> 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.
> 
> 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.

Thanks.

I really prefer not giving false information to user.

If you change tc, please add an option so that user can ask tc not
invent "zero values"

Re: tc: show format ABI changed

[Jarek Poplawski]

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>