| Message ID | 5917221449918579@web5j.yandex.ru |
|---|---|
| State | Deferred, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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));
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(-)