[PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command

[PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command

[Dmitrii Shcherbakov]

Hello,

I noticed a quite an old change which I have a few questions about so maybe somebody could help me out. There are a few lines of code in tc/q_htb.c which were committed originally back in 2004 and have not changed since related to MPU (Minimum Packet Unit). I asked Stephen - this is probably the code that pre-dated git so I am not sure exactly who to ask:

commit a166d246d84e7d0ba784e210515708006e16a366
Author: osdl.net!shemminger <osdl.net!shemminger>
Date:   Fri Jul 30 20:26:15 2004 +0000

    mpu support

@@ -243,10 +264,16 @@ static int htb_print_opt(struct qdisc_util *qu,
FILE *f, struct rtattr *opt)
            fprintf(f, "ceil %s ", sprint_rate(hopt->ceil.rate, b1));
            cbuffer =
((double)hopt->ceil.rate*tc_core_tick2usec(hopt->cbuffer))/1000000;
            if (show_details) {
-               fprintf(f, "burst %s/%u mpu %s ", sprint_size(buffer, b1),
-                       1<<hopt->rate.cell_log,
sprint_size(hopt->rate.mpu, b2));
-               fprintf(f, "cburst %s/%u mpu %s ", sprint_size(cbuffer, b1),
-                       1<<hopt->ceil.cell_log,
sprint_size(hopt->ceil.mpu, b2));
+               fprintf(f, "burst %s/%u mpu %s overhead %s ",
+                       sprint_size(buffer, b1),
+                       1<<hopt->rate.cell_log,
+                       sprint_size(hopt->rate.mpu&0xFF, b2),
+                       sprint_size((hopt->rate.mpu>>8)&0xFF, b3));
+               fprintf(f, "cburst %s/%u mpu %s overhead %s ",
+                       sprint_size(cbuffer, b1),
+                       1<<hopt->ceil.cell_log,
+                       sprint_size(hopt->ceil.mpu&0xFF, b2),
+                       sprint_size((hopt->ceil.mpu>>8)&0xFF, b3));

I noticed that there is a separate configurable value called 'overhead' in q_htb.c now which does not have anything to do with 'overhead = hopt->rate.mpu>>8)&0xFF' that is being printed out in the above lines:

static void explain(void)
{
    ...
        " overhead per-packet size overhead used in rate computations\n"
    ...

In the code fragment from the commit (for some reason I don't understand) 'rate.mpu' is sliced into two parts called 'mpu' and 'overhead'. So, basically, to retrieve the original MPU value from the output of `tc -d class show dev <device_name>` I need to do the following: real_mpu = mpu + overhead<<8.

I also have a hard time understanding whether it is possible to have a condition where rate.mpu and ceil.mpu have different values - the outputs I have seen always showed the same values for both and I do not see anything in the code that sets them to different values (if so, maybe the duplicate output should be removed).

As a reference, here is a link for a line in tc/q_tbf.c for the same functionality (show_details) that does not have anything like that for the MPU value:
https://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/tree/tc/q_tbf.c#n279

|fprintf(f, "burst %s/%u mpu %s ", sprint_size(buffer, b1),|



Please take a look at my patch if you have time for it as this would simplify testing in the project I work on and make the output of the command less ambiguous for other people who may use HTB.

Thank you,
Dmitrii Shcherbakov

>From 26c62f17e1d6ebca9f236000806a63363ab70640 Mon Sep 17 00:00:00 2001
From: Dmitrii Shcherbakov <fw.dmitrii@yandex.com>
Date: Sat, 12 Dec 2015 02:04:09 +0300
Subject: [PATCH] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev
 <device_name> ' command

This patch removes slicing of 'rate.mpu' into two parts called 'mpu' and 'overhead' alleviating the need to perform calculations to get the original MPU value after it is configured as follows: real_mpu = mpu + overhead<<8.

Signed-off-by: Dmitrii Shcherbakov <fw.dmitrii@yandex.com>
---
 tc/q_htb.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/tc/q_htb.c b/tc/q_htb.c
index 7075a4c..f810329 100644
--- a/tc/q_htb.c
+++ b/tc/q_htb.c
@@ -274,7 +274,6 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	SPRINT_BUF(b1);
 	SPRINT_BUF(b2);
 	SPRINT_BUF(b3);
-	SPRINT_BUF(b4);
 
 	if (opt == NULL)
 		return 0;
@@ -311,18 +310,16 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 		cbuffer = tc_calc_xmitsize(ceil64, hopt->cbuffer);
 		linklayer = (hopt->rate.linklayer & TC_LINKLAYER_MASK);
 		if (linklayer > TC_LINKLAYER_ETHERNET || show_details)
-			fprintf(f, "linklayer %s ", sprint_linklayer(linklayer, b4));
+			fprintf(f, "linklayer %s ", sprint_linklayer(linklayer, b3));
 		if (show_details) {
-			fprintf(f, "burst %s/%u mpu %s overhead %s ",
+			fprintf(f, "burst %s/%u mpu %s ",
 				sprint_size(buffer, b1),
 				1<<hopt->rate.cell_log,
-				sprint_size(hopt->rate.mpu&0xFF, b2),
-				sprint_size((hopt->rate.mpu>>8)&0xFF, b3));
-			fprintf(f, "cburst %s/%u mpu %s overhead %s ",
+				sprint_size(hopt->rate.mpu, b2));
+			fprintf(f, "cburst %s/%u mpu %s ",
 				sprint_size(cbuffer, b1),
 				1<<hopt->ceil.cell_log,
-				sprint_size(hopt->ceil.mpu&0xFF, b2),
-				sprint_size((hopt->ceil.mpu>>8)&0xFF, b3));
+				sprint_size(hopt->ceil.mpu, b2));
 			fprintf(f, "level %d ", (int)hopt->level);
 		} else {
 			fprintf(f, "burst %s ", sprint_size(buffer, b1));
-- 
2.5.0

Re: [PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command

[Jesper Dangaard Brouer]

Hi Dmitrii,

On Sat, 12 Dec 2015 14:09:39 +0300 Dmitrii Shcherbakov <fw.dmitrii@yandex.com> wrote:

> I noticed a quite an old change which I have a few questions about so
> maybe somebody could help me out. There are a few lines of code in
> tc/q_htb.c which were committed originally back in 2004 and have not
> changed since related to MPU (Minimum Packet Unit). I asked Stephen -
> this is probably the code that pre-dated git so I am not sure exactly
> who to ask:

AFAIKR I was involved in these overhead changes back in 2004, and also
around 2007 where we introduced a "real" overhead parameter.

We "moved" the overhead parameter to be a "real" struct member (in my commit
e08b09983fe9 ("[NET_SCHED]: Making rate table lookups more flexible."))

Thus, this encoding of overhead in the "mpu" should be obsolete.

struct tc_ratespec {
	unsigned char	cell_log;
	__u8		linklayer; /* lower 4 bits */
	unsigned short	overhead;
	short		cell_align;
	unsigned short	mpu;
	__u32		rate;
};

(sometimes accessed via  struct qdisc_rate_table->rate.overhead)

In newer kernels, this tc_ratespec, is converted and transfered to
struct psched_ratecfg (in functions psched_ratecfg_precompute() and
psched_ratecfg_getrate()).


> I noticed that there is a separate configurable value called
> 'overhead' in q_htb.c now which does not have anything to do with
> 'overhead = hopt->rate.mpu>>8)&0xFF' that is being printed out in the
> above lines:

Looking through both the kernel and iproute2 code, it looks like only
the "real" overhead is used. I cannot find any users of the encoded
overhead into the mpu value.  Thus, your patch should be okay...


> static void explain(void)
> {
>     ...
>         " overhead per-packet size overhead used in rate computations\n"
>     ...
>
 
> In the code fragment from the commit (for some reason I don't
> understand) 'rate.mpu' is sliced into two parts called 'mpu' and
> 'overhead'. So, basically, to retrieve the original MPU value from
> the output of `tc -d class show dev <device_name>` I need to do the
> following: real_mpu = mpu + overhead<<8.
> 

> I also have a hard time understanding whether it is possible to have
> a condition where rate.mpu and ceil.mpu have different values - the
> outputs I have seen always showed the same values for both and I do
> not see anything in the code that sets them to different values (if
> so, maybe the duplicate output should be removed).

Maybe. But do consider backwards compat issues,if you have this output
(e.g. like scripts parsing this output). 

When trying to understand this code, keep in mind that we are trying
to keep backward compatible with older kernels.  Thus, this printing
might be have been left here to keep compat with older kernels, but I
think we can remove it now.


> Please take a look at my patch if you have time for it as this would
> simplify testing in the project I work on and make the output of the
> command less ambiguous for other people who may use HTB.

Reviewed below
 
> Thank you,
> Dmitrii Shcherbakov
> 
> From 26c62f17e1d6ebca9f236000806a63363ab70640 Mon Sep 17 00:00:00 2001
> From: Dmitrii Shcherbakov <fw.dmitrii@yandex.com>
> Date: Sat, 12 Dec 2015 02:04:09 +0300
> Subject: [PATCH] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev
>  <device_name> ' command
> 
> This patch removes slicing of 'rate.mpu' into two parts called 'mpu'
> and 'overhead' alleviating the need to perform calculations to get
> the original MPU value after it is configured as follows: real_mpu =
> mpu + overhead<<8.
> 
> Signed-off-by: Dmitrii Shcherbakov <fw.dmitrii@yandex.com>
> ---
>  tc/q_htb.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/tc/q_htb.c b/tc/q_htb.c
> index 7075a4c..f810329 100644
> --- a/tc/q_htb.c
> +++ b/tc/q_htb.c
> @@ -274,7 +274,6 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>  	SPRINT_BUF(b1);
>  	SPRINT_BUF(b2);
>  	SPRINT_BUF(b3);
> -	SPRINT_BUF(b4);
>  
>  	if (opt == NULL)
>  		return 0;
> @@ -311,18 +310,16 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>  		cbuffer = tc_calc_xmitsize(ceil64, hopt->cbuffer);
>  		linklayer = (hopt->rate.linklayer & TC_LINKLAYER_MASK);
>  		if (linklayer > TC_LINKLAYER_ETHERNET || show_details)
> -			fprintf(f, "linklayer %s ", sprint_linklayer(linklayer, b4));
> +			fprintf(f, "linklayer %s ", sprint_linklayer(linklayer, b3));

I don't think your patch should contain this cleanup of "b4".
Submit another patch with that cleanup change, please.


>  		if (show_details) {
> -			fprintf(f, "burst %s/%u mpu %s overhead %s ",
> +			fprintf(f, "burst %s/%u mpu %s ",
>  				sprint_size(buffer, b1),
>  				1<<hopt->rate.cell_log,
> -				sprint_size(hopt->rate.mpu&0xFF, b2),
> -				sprint_size((hopt->rate.mpu>>8)&0xFF, b3));
> -			fprintf(f, "cburst %s/%u mpu %s overhead %s ",
> +				sprint_size(hopt->rate.mpu, b2));
> +			fprintf(f, "cburst %s/%u mpu %s ",
>  				sprint_size(cbuffer, b1),
>  				1<<hopt->ceil.cell_log,
> -				sprint_size(hopt->ceil.mpu&0xFF, b2),
> -				sprint_size((hopt->ceil.mpu>>8)&0xFF, b3));
> +				sprint_size(hopt->ceil.mpu, b2));
>  			fprintf(f, "level %d ", (int)hopt->level);
>  		} else {
>  			fprintf(f, "burst %s ", sprint_size(buffer, b1));

Other part of patch looks good.

Please resubmit patch "officially" and Cc me, then I will review and ACK.
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command

[Phil Sutter]

On Wed, Dec 16, 2015 at 04:15:58PM +0100, Jesper Dangaard Brouer wrote:
> When trying to understand this code, keep in mind that we are trying
> to keep backward compatible with older kernels.  Thus, this printing
> might be have been left here to keep compat with older kernels, but I
> think we can remove it now.

For a given kernel which still uses the upper eight bits of 'mpu' as
overhead value, iproute2 has become incompatible since commit
bccd014b866da ("Overhead calculation is now done in the kernel.") as
that eliminated the encoding while setting. So in order to stay
compatible, it should have left it in place while assigning to
opt.*.overhead in addition to that. Or the other way round, do the same
change to the output path as well.

So from a compatibility point of view, things are incompatible anyway.
Nobody complained, so I'd assume there are no users requiring this
(anymore). In fact, I couldn't even find the related change from using
'mpu>>8' to 'overhead' on kernel side.

Dmitrii, did iproute2 without your change even print the overhead as set
by you before? Looking at the code, I'd assume not.

Cheers, Phil

Re: [PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command

[Dmitrii Shcherbakov]

Phil,

> Dmitrii, did iproute2 without your change even print the overhead as
set by you before? Looking at the code, I'd assume not.

I used an iproute2 provided by the distribution and an OpenVZ 3.10 kernel
so there are differences there but I
noticed the 'manual' overhead-related code is included there. I am going
to get an upstream kernel and iproute2 to check tomorrow. At least they
print mpu consistently.

The following output is for non-upstream kernels and iproute2 provided
by the distributions so it probably won't be 100% useful to answer your
question.

This is what I have on a quite an old ovz kernel at home:

[root@localhost ~]# uname -r
3.10.0-229.7.2.vz7.8.8
[root@localhost ~]# tc qdisc add dev eth0 root handle 1: htb default 12
[root@localhost ~]# tc class add dev eth0 parent 1: classid 1:1 htb rate
100kbps ceil 100kbps mpu 256 overhead 64
[root@localhost ~]# tc -d class show dev eth0
class htb 1:1 root prio 0 quantum 10000 rate 800000bit overhead 64 ceil
800000bit burst 1600b/1 mpu 0b overhead 1b cburst 1600b/1 mpu 0b
overhead 1b level 0

And this is the same on ubuntu 14.04 with 3.19 kernel (looks like the
mpu value is not configured properly which is strange, though 'manual'
overhead is there).

administrator@ubuntu-q87:~$ uname -r
3.19.0-33-generic

administrator@ubuntu-q87:~$ sudo tc qdisc add dev eth0 root handle 1:
htb default 12
administrator@ubuntu-q87:~$ sudo tc class add dev eth0 parent 1: classid
1:1 htb rate 100kbps ceil 100kbps mpu 256 overhead 64
administrator@ubuntu-q87:~$ tc -d class show dev eth0
class htb 1:1 root prio 0 quantum 10000 rate 800000bit overhead 64 ceil
800000bit linklayer ethernet burst 1600b/1 mpu 0b overhead 0b cburst
1600b/1 mpu 0b overhead 0b level 0

Thanks,
Dima

Re: [PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command

[Dmitrii Shcherbakov]

Phil,

> Dmitrii, did iproute2 without your change even print the overhead as set
> by you before? Looking at the code, I'd assume not.

Tried building iproute2 (as of tag 4.2) and using the upstream linux kernel (also tag 4.2 - 64291f7db5bd8150a74ad2036f1037e6a0428df2):

~/src/iproute2/tc$ uname -r
4.2.0-040200-generic

~/src/iproute2/tc$ grep -inP 'htb' /boot/config-4.2.0-040200-generic 
1331:CONFIG_NET_SCH_HTB=m

~/src/iproute2/tc$ lsmod | grep htb
sch_htb                24576  1

~/src/iproute2/tc$ ./tc -d class show dev eth0

~/src/iproute2/tc$ sudo ./tc qdisc add dev eth0 root handle 1: htb default 12

~/src/iproute2/tc$ sudo ./tc class add dev eth0 parent 1: classid 1:1 htb rate 100kbps ceil 100kbps mpu 256 overhead 64

~/src/iproute2/tc$ tc -d class show dev eth0
class htb 1:1 root prio 0 quantum 10000 rate 800Kbit overhead 64 ceil 800Kbit linklayer ethernet burst 1600b/1 mpu 0b overhead 0b cburst 1600b/1 mpu 0b overhead 0b level 0

~/src/iproute2/tc$ lsmod | grep htb
sch_htb                24576  1

So it looks like the overhead is being set correctly, but the mpu is not, even though the respective kernel module is loaded judging by what I see.

Regards,
Dmitrii Shcherbakov

Re: [PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command

[Phil Sutter]

On Fri, Dec 18, 2015 at 07:39:25PM +0300, Dmitrii Shcherbakov wrote:
> > Dmitrii, did iproute2 without your change even print the overhead as set
> > by you before? Looking at the code, I'd assume not.
> 
> Tried building iproute2 (as of tag 4.2) and using the upstream linux kernel (also tag 4.2 - 64291f7db5bd8150a74ad2036f1037e6a0428df2):

This is without your patch, right?

[...]

> ~/src/iproute2/tc$ sudo ./tc class add dev eth0 parent 1: classid 1:1 htb rate 100kbps ceil 100kbps mpu 256 overhead 64

Setting an mpu of 256 is suitable to get 0 as output value, as the code
before your patch ANDs it with 0xff.

> So it looks like the overhead is being set correctly, but the mpu is not, even though the respective kernel module is loaded judging by what I see.

To really know what is being set, you would have to look at the kernel
variables not what iproute prints. This is nitpicking mostly, but
relevant in this case as your patches to fix iproute's output show.

Cheers, Phil

Re: [PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command

[Dmitrii Shcherbakov]

Phil,

18.12.2015, 19:55, "Phil Sutter" <phil@nwl.cc>:
> On Fri, Dec 18, 2015 at 07:39:25PM +0300, Dmitrii Shcherbakov wrote:
>>  > Dmitrii, did iproute2 without your change even print the overhead as set
>>  > by you before? Looking at the code, I'd assume not.
>>
>>  Tried building iproute2 (as of tag 4.2) and using the upstream linux kernel (also tag 4.2 - 64291f7db5bd8150a74ad2036f1037e6a0428df2):
>
> This is without your patch, right?

Yes (ec4ef6aebd5a52ab1acf1f5be1749320b3188659). 

>
>>  ~/src/iproute2/tc$ sudo ./tc class add dev eth0 parent 1: classid 1:1 htb rate 100kbps ceil 100kbps mpu 256 overhead 64
>
> Setting an mpu of 256 is suitable to get 0 as output value, as the code
> before your patch ANDs it with 0xff.

True, but then I think I would get the 'old' (encoded) overhead output of '1b' then and the first 8 bits which are treated as mpu would be 0 anyway. And its 0 for both mpu and overhead ("mpu 0b overhead 0b") which is strange. I am going to have to take a look at the kernel state with gdb.

What I would expect instead (notice 'overhead 1b'):

[root@localhost ~]# tc -d class show dev eth0
[root@localhost ~]# tc qdisc add dev eth0 root handle 1: htb default 12
[root@localhost ~]# tc class add dev eth0 parent 1: classid 1:1 htb rate 100kbps ceil 100kbps mpu 256 overhead 64
[root@localhost ~]# tc -d class show dev eth0
class htb 1:1 root prio 0 quantum 10000 rate 800000bit overhead 64 ceil 800000bit burst 1600b/1 mpu 0b overhead 1b cburst 1600b/1 mpu 0b overhead 1b level 0  

>
>>  So it looks like the overhead is being set correctly, but the mpu is not, even though the respective kernel module is loaded judging by what I see.
>
> To really know what is being set, you would have to look at the kernel
> variables not what iproute prints. This is nitpicking mostly, but
> relevant in this case as your patches to fix iproute's output show.
>
> Cheers, Phil

I am going to try and take a look at it. I have not delved into the kernel's network subsystem so it may take some time.

Thanks,
Dima

Re: [PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command

[Stephen Hemminger]

On Sat, 19 Dec 2015 00:45:40 +0300
Dmitrii Shcherbakov <fw.dmitrii@yandex.com> wrote:

> Phil,
> 
> 18.12.2015, 19:55, "Phil Sutter" <phil@nwl.cc>:
> > On Fri, Dec 18, 2015 at 07:39:25PM +0300, Dmitrii Shcherbakov wrote:
> >>  > Dmitrii, did iproute2 without your change even print the overhead as set
> >>  > by you before? Looking at the code, I'd assume not.
> >>
> >>  Tried building iproute2 (as of tag 4.2) and using the upstream linux kernel (also tag 4.2 - 64291f7db5bd8150a74ad2036f1037e6a0428df2):
> >
> > This is without your patch, right?
> 
> Yes (ec4ef6aebd5a52ab1acf1f5be1749320b3188659). 
> 
> >
> >>  ~/src/iproute2/tc$ sudo ./tc class add dev eth0 parent 1: classid 1:1 htb rate 100kbps ceil 100kbps mpu 256 overhead 64
> >
> > Setting an mpu of 256 is suitable to get 0 as output value, as the code
> > before your patch ANDs it with 0xff.
> 
> True, but then I think I would get the 'old' (encoded) overhead output of '1b' then and the first 8 bits which are treated as mpu would be 0 anyway. And its 0 for both mpu and overhead ("mpu 0b overhead 0b") which is strange. I am going to have to take a look at the kernel state with gdb.
> 
> What I would expect instead (notice 'overhead 1b'):
> 
> [root@localhost ~]# tc -d class show dev eth0
> [root@localhost ~]# tc qdisc add dev eth0 root handle 1: htb default 12
> [root@localhost ~]# tc class add dev eth0 parent 1: classid 1:1 htb rate 100kbps ceil 100kbps mpu 256 overhead 64
> [root@localhost ~]# tc -d class show dev eth0
> class htb 1:1 root prio 0 quantum 10000 rate 800000bit overhead 64 ceil 800000bit burst 1600b/1 mpu 0b overhead 1b cburst 1600b/1 mpu 0b overhead 1b level 0  
> 
> >
> >>  So it looks like the overhead is being set correctly, but the mpu is not, even though the respective kernel module is loaded judging by what I see.
> >
> > To really know what is being set, you would have to look at the kernel
> > variables not what iproute prints. This is nitpicking mostly, but
> > relevant in this case as your patches to fix iproute's output show.
> >
> > Cheers, Phil
> 
> I am going to try and take a look at it. I have not delved into the kernel's network subsystem so it may take some time.
> 
> Thanks,
> Dima

I am going to hold off on this, if it hasn't been urgent for years, there is no
requirement to do it right away. If you come to a conclusion let me know.

It might be possible to troll back through Internet to get more data on this...

Re: [PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command

[Dmitrii Shcherbakov]

16.12.2015, 18:16, "Jesper Dangaard Brouer" <brouer@redhat.com>:

>  I don't think your patch should contain this cleanup of "b4".
>  Submit another patch with that cleanup change, please.
>
>>                    if (show_details) {
>>   - fprintf(f, "burst %s/%u mpu %s overhead %s ",
>>   + fprintf(f, "burst %s/%u mpu %s ",
>>                                    sprint_size(buffer, b1),
>>                                    1<<hopt->rate.cell_log,
>>   - sprint_size(hopt->rate.mpu&0xFF, b2),
>>   - sprint_size((hopt->rate.mpu>>8)&0xFF, b3));
>>   - fprintf(f, "cburst %s/%u mpu %s overhead %s ",
>>   + sprint_size(hopt->rate.mpu, b2));
>>   + fprintf(f, "cburst %s/%u mpu %s ",
>>                                    sprint_size(cbuffer, b1),
>>                                    1<<hopt->ceil.cell_log,
>>   - sprint_size(hopt->ceil.mpu&0xFF, b2),
>>   - sprint_size((hopt->ceil.mpu>>8)&0xFF, b3));
>>   + sprint_size(hopt->ceil.mpu, b2));
>>                            fprintf(f, "level %d ", (int)hopt->level);
>>                    } else {
>>                            fprintf(f, "burst %s ", sprint_size(buffer, b1));


Ok, but I don't see any other usage of b4:

administrator@UX32LN:~/Sources/iproute2/tc$ git branch
* (HEAD detached at 654ae88)
  master
  v3.18.0


16.12.2015, 18:16, "Jesper Dangaard Brouer" <brouer@redhat.com>:
> Hi Dmitrii,
>
> On Sat, 12 Dec 2015 14:09:39 +0300 Dmitrii Shcherbakov <fw.dmitrii@yandex.com> wrote:
>
>>  I noticed a quite an old change which I have a few questions about so
>>  maybe somebody could help me out. There are a few lines of code in
>>  tc/q_htb.c which were committed originally back in 2004 and have not
>>  changed since related to MPU (Minimum Packet Unit). I asked Stephen -
>>  this is probably the code that pre-dated git so I am not sure exactly
>>  who to ask:
>
> AFAIKR I was involved in these overhead changes back in 2004, and also
> around 2007 where we introduced a "real" overhead parameter.
>
> We "moved" the overhead parameter to be a "real" struct member (in my commit
> e08b09983fe9 ("[NET_SCHED]: Making rate table lookups more flexible."))
>
> Thus, this encoding of overhead in the "mpu" should be obsolete.
>
> struct tc_ratespec {
>         unsigned char cell_log;
>         __u8 linklayer; /* lower 4 bits */
>         unsigned short overhead;
>         short cell_align;
>         unsigned short mpu;
>         __u32 rate;
> };
>
> (sometimes accessed via struct qdisc_rate_table->rate.overhead)
>
> In newer kernels, this tc_ratespec, is converted and transfered to
> struct psched_ratecfg (in functions psched_ratecfg_precompute() and
> psched_ratecfg_getrate()).
>
>>  I noticed that there is a separate configurable value called
>>  'overhead' in q_htb.c now which does not have anything to do with
>>  'overhead = hopt->rate.mpu>>8)&0xFF' that is being printed out in the
>>  above lines:
>
> Looking through both the kernel and iproute2 code, it looks like only
> the "real" overhead is used. I cannot find any users of the encoded
> overhead into the mpu value. Thus, your patch should be okay...
>
>>  static void explain(void)
>>  {
>>      ...
>>          " overhead per-packet size overhead used in rate computations\n"
>>      ...
>
>>  In the code fragment from the commit (for some reason I don't
>>  understand) 'rate.mpu' is sliced into two parts called 'mpu' and
>>  'overhead'. So, basically, to retrieve the original MPU value from
>>  the output of `tc -d class show dev <device_name>` I need to do the
>>  following: real_mpu = mpu + overhead<<8.
>
>>  I also have a hard time understanding whether it is possible to have
>>  a condition where rate.mpu and ceil.mpu have different values - the
>>  outputs I have seen always showed the same values for both and I do
>>  not see anything in the code that sets them to different values (if
>>  so, maybe the duplicate output should be removed).
>
> Maybe. But do consider backwards compat issues,if you have this output
> (e.g. like scripts parsing this output).
>
> When trying to understand this code, keep in mind that we are trying
> to keep backward compatible with older kernels. Thus, this printing
> might be have been left here to keep compat with older kernels, but I
> think we can remove it now.
>
>>  Please take a look at my patch if you have time for it as this would
>>  simplify testing in the project I work on and make the output of the
>>  command less ambiguous for other people who may use HTB.
>
> Reviewed below
>
>>  Thank you,
>>  Dmitrii Shcherbakov
>>
>>  From 26c62f17e1d6ebca9f236000806a63363ab70640 Mon Sep 17 00:00:00 2001
>>  From: Dmitrii Shcherbakov <fw.dmitrii@yandex.com>
>>  Date: Sat, 12 Dec 2015 02:04:09 +0300
>>  Subject: [PATCH] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev
>>   <device_name> ' command
>>
>>  This patch removes slicing of 'rate.mpu' into two parts called 'mpu'
>>  and 'overhead' alleviating the need to perform calculations to get
>>  the original MPU value after it is configured as follows: real_mpu =
>>  mpu + overhead<<8.
>>
>>  Signed-off-by: Dmitrii Shcherbakov <fw.dmitrii@yandex.com>
>>  ---
>>   tc/q_htb.c | 13 +++++--------
>>   1 file changed, 5 insertions(+), 8 deletions(-)
>>
>>  diff --git a/tc/q_htb.c b/tc/q_htb.c
>>  index 7075a4c..f810329 100644
>>  --- a/tc/q_htb.c
>>  +++ b/tc/q_htb.c
>>  @@ -274,7 +274,6 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>>           SPRINT_BUF(b1);
>>           SPRINT_BUF(b2);
>>           SPRINT_BUF(b3);
>>  - SPRINT_BUF(b4);
>>
>>           if (opt == NULL)
>>                   return 0;
>>  @@ -311,18 +310,16 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>>                   cbuffer = tc_calc_xmitsize(ceil64, hopt->cbuffer);
>>                   linklayer = (hopt->rate.linklayer & TC_LINKLAYER_MASK);
>>                   if (linklayer > TC_LINKLAYER_ETHERNET || show_details)
>>  - fprintf(f, "linklayer %s ", sprint_linklayer(linklayer, b4));
>>  + fprintf(f, "linklayer %s ", sprint_linklayer(linklayer, b3));
>
> I don't think your patch should contain this cleanup of "b4".
> Submit another patch with that cleanup change, please.
>
>>                   if (show_details) {
>>  - fprintf(f, "burst %s/%u mpu %s overhead %s ",
>>  + fprintf(f, "burst %s/%u mpu %s ",
>>                                   sprint_size(buffer, b1),
>>                                   1<<hopt->rate.cell_log,
>>  - sprint_size(hopt->rate.mpu&0xFF, b2),
>>  - sprint_size((hopt->rate.mpu>>8)&0xFF, b3));
>>  - fprintf(f, "cburst %s/%u mpu %s overhead %s ",
>>  + sprint_size(hopt->rate.mpu, b2));
>>  + fprintf(f, "cburst %s/%u mpu %s ",
>>                                   sprint_size(cbuffer, b1),
>>                                   1<<hopt->ceil.cell_log,
>>  - sprint_size(hopt->ceil.mpu&0xFF, b2),
>>  - sprint_size((hopt->ceil.mpu>>8)&0xFF, b3));
>>  + sprint_size(hopt->ceil.mpu, b2));
>>                           fprintf(f, "level %d ", (int)hopt->level);
>>                   } else {
>>                           fprintf(f, "burst %s ", sprint_size(buffer, b1));
>
> Other part of patch looks good.
>
> Please resubmit patch "officially" and Cc me, then I will review and ACK.
> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   Author of http://www.iptv-analyzer.org
>   LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command

[Dmitrii Shcherbakov]

Jesper,

Sorry, it seems that I accidentally sent out an unfinished e-mail.

>I don't think your patch should contain this cleanup of "b4".

It seems that b3 is only used for the legacy overhead part and if I remove it, b3 is not going to be used. So I figured I remove b4 put b3 instead.

administrator@ubuntu:~/src/iproute2/tc$ git rev-parse HEAD
654ae881de57467642c8c2ed16ffc3a8d57fafa2

administrator@ubuntu:~/src/iproute2/tc$ grep -nHP -B 3 'b3' q_htb.c
q_htb.c-273-	__u64 rate64, ceil64;
q_htb.c-274-	SPRINT_BUF(b1);
q_htb.c-275-	SPRINT_BUF(b2);
q_htb.c:276:	SPRINT_BUF(b3);
--
q_htb.c-317-				sprint_size(buffer, b1),
q_htb.c-318-				1<<hopt->rate.cell_log,
q_htb.c-319-				sprint_size(hopt->rate.mpu&0xFF, b2),
q_htb.c:320:				sprint_size((hopt->rate.mpu>>8)&0xFF, b3));
--
q_htb.c-322-				sprint_size(cbuffer, b1),
q_htb.c-323-				1<<hopt->ceil.cell_log,
q_htb.c-324-				sprint_size(hopt->ceil.mpu&0xFF, b2),
q_htb.c:325:				sprint_size((hopt->ceil.mpu>>8)&0xFF, b3));
administrator@ubuntu:~/src/iproute2/tc$ grep -nHP -B 3 'b4' q_htb.c
q_htb.c-274-	SPRINT_BUF(b1);
q_htb.c-275-	SPRINT_BUF(b2);
q_htb.c-276-	SPRINT_BUF(b3);
q_htb.c:277:	SPRINT_BUF(b4);
--
q_htb.c-311-		cbuffer = tc_calc_xmitsize(ceil64, hopt->cbuffer);
q_htb.c-312-		linklayer = (hopt->rate.linklayer & TC_LINKLAYER_MASK);
q_htb.c-313-		if (linklayer > TC_LINKLAYER_ETHERNET || show_details)
q_htb.c:314:			fprintf(f, "linklayer %s ", sprint_linklayer(linklayer, b4));

Thanks,
Dima

Re: [PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command

[Phil Sutter]

On Thu, Dec 17, 2015 at 12:45:33PM +0300, Dmitrii Shcherbakov wrote:
> >I don't think your patch should contain this cleanup of "b4".
> 
> It seems that b3 is only used for the legacy overhead part and if I remove it, b3 is not going to be used. So I figured I remove b4 put b3 instead.

No worries, your intentions are good per se. All he wants from you is to
split changes up: a dedicated patch for mpu->overhead conversion and a
second one dedicated for SPRINT_BUF variable cleanup.

HTH, Phil