| Message ID | 1370208781.24311.89.camel@edumazet-glaptop |
|---|---|
| State | Accepted, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
On 06/02/2013 02:33 PM, Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > "tc class show dev ..." omits the overhead attribute for HTB. > > After patch I have : > > tc class add dev $DEV parent 1: classid 1:1 est 1sec 4sec htb \ > rate 12Mbit mtu 1500 quantum 1514 overhead 20 > > tc class show dev $DEV > class htb 1:1 root prio 0 rate 12000Kbit overhead 20 ceil 12000Kbit > burst 1500b cburst 1500b > > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > tc/q_htb.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tc/q_htb.c b/tc/q_htb.c > index caa47c2..e6b09bb 100644 > --- a/tc/q_htb.c > +++ b/tc/q_htb.c > @@ -264,6 +264,8 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt) > fprintf(f, "quantum %d ", (int)hopt->quantum); > } > fprintf(f, "rate %s ", sprint_rate(hopt->rate.rate, b1)); > + if (hopt->rate.overhead) > + fprintf(f, "overhead %u ", hopt->rate.overhead); Is it (still) possible to have a negative overhead? http://www.linksysinfo.org/index.php?threads/speedmod-with-tc-atm-qos-patch-for-adsl.31541/ rick jones -- 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 Mon, 2013-06-03 at 08:45 -0700, Rick Jones wrote: > Is it (still) possible to have a negative overhead? > > http://www.linksysinfo.org/index.php?threads/speedmod-with-tc-atm-qos-patch-for-adsl.31541/ overhead always has been unsigned in the kernel. What you describe is a userland hack in tc command. (or a bug) -- 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 03.06.2013 18:45, Rick Jones wrote: > On 06/02/2013 02:33 PM, Eric Dumazet wrote: >> From: Eric Dumazet <edumazet@google.com> >> >> "tc class show dev ..." omits the overhead attribute for HTB. >> >> After patch I have : >> >> tc class add dev $DEV parent 1: classid 1:1 est 1sec 4sec htb \ >> rate 12Mbit mtu 1500 quantum 1514 overhead 20 >> >> tc class show dev $DEV >> class htb 1:1 root prio 0 rate 12000Kbit overhead 20 ceil 12000Kbit >> burst 1500b cburst 1500b >> >> Signed-off-by: Eric Dumazet <edumazet@google.com> >> --- >> tc/q_htb.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/tc/q_htb.c b/tc/q_htb.c >> index caa47c2..e6b09bb 100644 >> --- a/tc/q_htb.c >> +++ b/tc/q_htb.c >> @@ -264,6 +264,8 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt) >> fprintf(f, "quantum %d ", (int)hopt->quantum); >> } >> fprintf(f, "rate %s ", sprint_rate(hopt->rate.rate, b1)); >> + if (hopt->rate.overhead) >> + fprintf(f, "overhead %u ", hopt->rate.overhead); > > Is it (still) possible to have a negative overhead? > > http://www.linksysinfo.org/index.php?threads/speedmod-with-tc-atm-qos-patch-for-adsl.31541/ > You can use 'tc-stab' for negative overhead. http://stuff.onse.fi/man?program=tc-stab§ion=8 -Jussi -- 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 Mon, 03 Jun 2013 08:56:02 -0700 Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Mon, 2013-06-03 at 08:45 -0700, Rick Jones wrote: > > > Is it (still) possible to have a negative overhead? > > > > http://www.linksysinfo.org/index.php?threads/speedmod-with-tc-atm-qos-patch-for-adsl.31541/ > > overhead always has been unsigned in the kernel. > > What you describe is a userland hack in tc command. > (or a bug) Rick is referencing Russell Stuart's patches, where a negative overhead was possible. http://ace-host.stuart.id.au/russell/files/tc/tc-atm/#history But my patches got accepted into the kernel, where a negative overhead was not possible. In retrospect, we should have supported a negative overhead. A negative overhead *is* a valid use-case, and we should work towards supporting this. E.g. by changing the recent added "u16 overhead" in struct psched_ratecfg to be "s16" (ref [1]) ? [1] commit 01cb71d2d47 (net_sched: restore "overhead xxx" handling) https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=01cb71d2d47b78354358e4bb938bb06323e17498
On Tue, 2013-06-04 at 13:11 +0200, Jesper Dangaard Brouer wrote: > On Mon, 03 Jun 2013 08:56:02 -0700 > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > On Mon, 2013-06-03 at 08:45 -0700, Rick Jones wrote: > > > > > Is it (still) possible to have a negative overhead? > > > > > > http://www.linksysinfo.org/index.php?threads/speedmod-with-tc-atm-qos-patch-for-adsl.31541/ > > > > overhead always has been unsigned in the kernel. > > > > What you describe is a userland hack in tc command. > > (or a bug) > > Rick is referencing Russell Stuart's patches, where a negative overhead > was possible. > http://ace-host.stuart.id.au/russell/files/tc/tc-atm/#history > > But my patches got accepted into the kernel, where a negative > overhead was not possible. In retrospect, we should have supported a > negative overhead. > > A negative overhead *is* a valid use-case, and we should work towards > supporting this. E.g. by changing the recent added "u16 overhead" in > struct psched_ratecfg to be "s16" (ref [1]) ? > > > [1] commit 01cb71d2d47 (net_sched: restore "overhead xxx" handling) > https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=01cb71d2d47b78354358e4bb938bb06323e17498 > Again, you describe something that Vimal patch didn't broke, and should be addressed on net-next. My concern was restoring the overhead attribute that Vimal broke, and this one was unsigned 16bits. Allowing a negative offset is not free, it adds a conditional test, because (len + overhead) could be negative. Please note that a negative offset is possible with the STAB infra. -- 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 Tue, 04 Jun 2013 06:58:31 -0700 Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Tue, 2013-06-04 at 13:11 +0200, Jesper Dangaard Brouer wrote: > > On Mon, 03 Jun 2013 08:56:02 -0700 > > Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > > On Mon, 2013-06-03 at 08:45 -0700, Rick Jones wrote: > > > > > > > Is it (still) possible to have a negative overhead? > > > > > > > > http://www.linksysinfo.org/index.php?threads/speedmod-with-tc-atm-qos-patch-for-adsl.31541/ > > > > > > overhead always has been unsigned in the kernel. > > > > > > What you describe is a userland hack in tc command. > > > (or a bug) > > > > Rick is referencing Russell Stuart's patches, where a negative > > overhead was possible. > > http://ace-host.stuart.id.au/russell/files/tc/tc-atm/#history > > > > But my patches got accepted into the kernel, where a negative > > overhead was not possible. In retrospect, we should have supported a > > negative overhead. > > > > A negative overhead *is* a valid use-case, and we should work > > towards supporting this. E.g. by changing the recent added "u16 > > overhead" in struct psched_ratecfg to be "s16" (ref [1]) ? (My statement should have been corrected to "s16"->"s32", never mind) > > > > [1] commit 01cb71d2d47 (net_sched: restore "overhead xxx" handling) > > https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=01cb71d2d47b78354358e4bb938bb06323e17498 > > > > Again, you describe something that Vimal patch didn't broke, and > should be addressed on net-next. Yes, I know, I also said "work towards supporting this", meaning "net-next". > My concern was restoring the overhead attribute that Vimal broke, and > this one was unsigned 16bits. > > Allowing a negative offset is not free, it adds a conditional test, > because (len + overhead) could be negative. Yes, I do realize that. But Vimal patch actually also broke the "mpu" (Minimum Packet Unit) feature. And we could combine this, and get negative offset fix by a max(mpu,len), where mpu would be default init'ed.
On Sun, 02 Jun 2013 14:33:01 -0700 Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > "tc class show dev ..." omits the overhead attribute for HTB. > > After patch I have : > > tc class add dev $DEV parent 1: classid 1:1 est 1sec 4sec htb \ > rate 12Mbit mtu 1500 quantum 1514 overhead 20 > > tc class show dev $DEV > class htb 1:1 root prio 0 rate 12000Kbit overhead 20 ceil 12000Kbit > burst 1500b cburst 1500b > > Signed-off-by: Eric Dumazet <edumazet@google.com> Applied. And after that fixed indentation of q_htb.c -- 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, 2013-06-07 at 08:56 -0700, Stephen Hemminger wrote: > Applied. > And after that fixed indentation of q_htb.c Yes, I saw the indentation was strange indeed ;) Thanks -- 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 caa47c2..e6b09bb 100644 --- a/tc/q_htb.c +++ b/tc/q_htb.c @@ -264,6 +264,8 @@ static int htb_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt) fprintf(f, "quantum %d ", (int)hopt->quantum); } fprintf(f, "rate %s ", sprint_rate(hopt->rate.rate, b1)); + if (hopt->rate.overhead) + fprintf(f, "overhead %u ", hopt->rate.overhead); buffer = tc_calc_xmitsize(hopt->rate.rate, hopt->buffer); fprintf(f, "ceil %s ", sprint_rate(hopt->ceil.rate, b1)); cbuffer = tc_calc_xmitsize(hopt->ceil.rate, hopt->cbuffer);