| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 17 Dec 2015 12:27:41 +0300
From: Dmitrii Shcherbakov <fw.dmitrii@...dex.com>
To: Jesper Dangaard Brouer <brouer@...hat.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Phil Sutter <psutter@...hat.com>,
Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: [PATCH][iproute2] tc/q_htb.c: Fix the MPU value output in 'tc -d class show dev <device_name> ' command
16.12.2015, 18:16, "Jesper Dangaard Brouer" <brouer@...hat.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@...2LN:~/Sources/iproute2/tc$ git branch
* (HEAD detached at 654ae88)
master
v3.18.0
16.12.2015, 18:16, "Jesper Dangaard Brouer" <brouer@...hat.com>:
> Hi Dmitrii,
>
> On Sat, 12 Dec 2015 14:09:39 +0300 Dmitrii Shcherbakov <fw.dmitrii@...dex.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@...dex.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@...dex.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists