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

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

[Phil Sutter]

On Fri, Dec 18, 2015 at 04:16:38PM +0300, Dmitrii Shcherbakov 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@yandex.com>

Acked-by: Phil Sutter <phil@nwl.cc>

Re: [PATCH 2/2] [iproute2] tc/q_htb.c: rename b4 buffer to b3 to make its name more consistent

[Phil Sutter]

On Fri, Dec 18, 2015 at 04:16:39PM +0300, Dmitrii Shcherbakov wrote:
> b3 buffer has been deleted previously so b2 is followed by b4 which is not consistent
> 
> Signed-off-by: Dmitrii Shcherbakov <fw.dmitrii@yandex.com>

Acked-by: Phil Sutter <phil@nwl.cc>

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

[Jesper Dangaard Brouer]

On Fri, 18 Dec 2015 16:16:38 +0300 Dmitrii Shcherbakov <fw.dmitrii@yandex.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@yandex.com>
> ---

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

Thank you Dmitrii for cleaning this up :-)

-- 
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 2/2] [iproute2] tc/q_htb.c: rename b4 buffer to b3 to make its name more consistent

[Jesper Dangaard Brouer]

On Fri, 18 Dec 2015 16:16:39 +0300
Dmitrii Shcherbakov <fw.dmitrii@yandex.com> wrote:

> b3 buffer has been deleted previously so b2 is followed by b4 which is not consistent
> 
> Signed-off-by: Dmitrii Shcherbakov <fw.dmitrii@yandex.com>
> ---

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

-- 
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 1/2] [iproute2] tc/q_htb.c: remove printing of a deprecated overhead value previously encoded as a part of mpu field

[Dmitrii Shcherbakov]

Jesper,

> Thank you Dmitrii for cleaning this up :-)

You are welcome :^)

I should read more carefully: its what you asked from the beginning.

Thank you,
Dmitrii Shcherbakov

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

[Dmitrii Shcherbakov]

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@UX32LN:~/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@yandex.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@yandex.com>
> > Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > Acked-by: Phil Sutter <phil@nwl.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.

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

[Stephen Hemminger]

On Sat, 19 Dec 2015 18:25:52 +0300
Dmitrii Shcherbakov <fw.dmitrii@yandex.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@yandex.com>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Acked-by: Phil Sutter <phil@nwl.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.

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

[Stephen Hemminger]

On Sat, 19 Dec 2015 18:25:52 +0300
Dmitrii Shcherbakov <fw.dmitrii@yandex.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@yandex.com>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Acked-by: Phil Sutter <phil@nwl.cc>
> ---
>  tc/q_htb.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/tc/q_htb.c b/tc/q_htb.c
> index 7075a4c..e76d20a 100644
> --- a/tc/q_htb.c
> +++ b/tc/q_htb.c
> @@ -273,7 +273,6 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>  	__u64 rate64, ceil64;
>  	SPRINT_BUF(b1);
>  	SPRINT_BUF(b2);
> -	SPRINT_BUF(b3);
>  	SPRINT_BUF(b4);
>  
>  	if (opt == NULL)
> @@ -313,16 +312,14 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>  		if (linklayer > TC_LINKLAYER_ETHERNET || show_details)
>  			fprintf(f, "linklayer %s ", sprint_linklayer(linklayer, b4));
>  		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));

Not sure if this a good thing to do. There are people who screenscrape the output of
iproute2 commands, and this might break that.

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

[Dmitrii Shcherbakov]

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@yandex.com>
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Phil Sutter <phil@nwl.cc>
---
 tc/q_htb.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/tc/q_htb.c b/tc/q_htb.c
index 7075a4c..e76d20a 100644
--- a/tc/q_htb.c
+++ b/tc/q_htb.c
@@ -273,7 +273,6 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	__u64 rate64, ceil64;
 	SPRINT_BUF(b1);
 	SPRINT_BUF(b2);
-	SPRINT_BUF(b3);
 	SPRINT_BUF(b4);
 
 	if (opt == NULL)
@@ -313,16 +312,14 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 		if (linklayer > TC_LINKLAYER_ETHERNET || show_details)
 			fprintf(f, "linklayer %s ", sprint_linklayer(linklayer, b4));
 		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