16.12.2015, 18:16, "Jesper Dangaard Brouer" <bro...@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" <bro...@redhat.com>: > Hi Dmitrii, > > On Sat, 12 Dec 2015 14:09:39 +0300 Dmitrii Shcherbakov > <fw.dmit...@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.dmit...@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.dmit...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html