netdev - Re: [PATCH 1/2] [iproute2] tc/q_htb.c: remove printing of a deprecated overhead value previously encoded as a part of mpu field

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1456144880.20204.19.camel@yandex.ru>
Date:	Mon, 22 Feb 2016 15:41:20 +0300
From:	Dmitrii Shcherbakov <fw.dmitrii@...dex.ru>
To:	Stephen Hemminger <stephen@...workplumber.org>,
	Dmitrii Shcherbakov <fw.dmitrii@...dex.com>
Cc:	netdev@...r.kernel.org, Jesper Dangaard Brouer <brouer@...hat.com>,
	Phil Sutter <phil@....cc>
Subject: Re: [PATCH 1/2] [iproute2] tc/q_htb.c: remove printing of a
 deprecated overhead value previously encoded as a part of mpu field

Thanks!

I also noticed that there seems to be no kernel code to set the MPU
values.

# latest commit at the time of writing
administrator@...2LN:~/src/linux$ git rev-parse HEAD
64291f7db5bd8150a74ad2036f1037e6a0428df2

Some cscope output below:

0 include/uapi/linux/pkt_sched.h 84 struct tc_ratespec {

A ratespec structure that holds an mpu value:

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


0 include/net/sch_generic.h psched_l2t_ns         766 static inline u64
psched_l2t_ns(const struct psched_ratecfg *r,

An L2T (Length to Time) conversion function which returns an amount of
time in nanoseconds needed to send a packet of a specific size.
Here the length is first modified by a manually configured overhead
value - could be a good place to round the length up to mpu if it is
smaller than mpu:

static inline u64 psched_l2t_ns(const struct psched_ratecfg *r,
				unsigned int len)
{
	len += r->overhead;

	if (unlikely(r->linklayer == TC_LINKLAYER_ATM))
		return ((u64)(DIV_ROUND_UP(len,48)*53) * r->mult) >> r
->shift;

	return ((u64)len * r->mult) >> r->shift;
}

Seems like psched_l2t_ns is used in both htb and tbf and affects the
amount of tokens that are going to be consumed by a packet of a certain
size:

C symbol: psched_l2t_ns

  File                      Function              Line
0 include/net/sch_generic.h psched_l2t_ns         766 static inline u64
psched_l2t_ns(const struct psched_ratecfg *r,
1 net/sched/act_police.c    tcf_act_police_locate 221 police
->tcfp_mtu_ptoks = (s64) psched_l2t_ns(&police->peak,
2 net/sched/act_police.c    tcf_act_police        289 ptoks -= (s64)
psched_l2t_ns(&police->peak,
3 net/sched/act_police.c    tcf_act_police        295 toks -= (s64)
psched_l2t_ns(&police->rate, qdisc_pkt_len(skb));
4 net/sched/sch_htb.c       htb_accnt_tokens      613 toks -= (s64)
psched_l2t_ns(&cl->rate, bytes);
5 net/sched/sch_htb.c       htb_accnt_ctokens     626 toks -= (s64)
psched_l2t_ns(&cl->ceil, bytes);
6 net/sched/sch_tbf.c       tbf_dequeue           249 ptoks -= (s64)
psched_l2t_ns(&q->peak, len);
7 net/sched/sch_tbf.c       tbf_dequeue           254 toks -= (s64)
psched_l2t_ns(&q->rate, len);
8 net/sched/sch_tbf.c       tbf_change            351 buffer = psched_l
2t_ns(&rate, max_size);
9 net/sched/sch_tbf.c       tbf_change            370 mtu =
psched_l2t_ns(&peak, pburst);

Also found a patch (https://patchwork.ozlabs.org/patch/501666/) that
was intended to fix setting of mpu values which never got pushed.

Printing definitely deserved a fix but there is some more work to it at
the kernel side.

On Ср., 2016-02-17 at 17:51 -0800, Stephen Hemminger wrote:
> On Sat, 19 Dec 2015 18:25:52 +0300
> Dmitrii Shcherbakov <fw.dmitrii@...dex.com> wrote:
> 
> > Remove printing according to the previously used encoding of mpu
> > and overhead values within the tc_ratespec's mpu field. This
> > encoding is no longer being used as a separate 'overhead' field in
> > the ratespec structure has been introduced.
> > 
> > Signed-off-by: Dmitrii Shcherbakov <fw.dmitrii@...dex.com>
> > Acked-by: Jesper Dangaard Brouer <brouer@...hat.com>
> > Acked-by: Phil Sutter <phil@....cc>
> 
> Both applied.
> 
> I had to fix up the commit logs.
> Many tools don't like long subject/summary lines, and it is standard
> practice
> to wrap text in commit body at 80 characters.