[PATCH 0/2] net-next codel enhancements

[PATCH 0/2] net-next codel enhancements

[Dave Taht]

These two patches have been extensively tested in openwrt
and cerowrt. Losing the fq_codel drop statistic as a bug.

Checking for a maxpacket size in the qdisc was somewhat 
correct for ns2 but not for Linux as all the current 
subsystems below htb/hfsc/adsl/bql buffer packets.

IMHO both of these patches are candidates for stable. 

[PATCH 1/2] fq_codel: keep dropped statistic around

[Dave Taht]

Having more accurate dropped information in this qdisc is useful.

Signed-off-by: Dave Taht <dave.taht@bufferbloat.net>
---
 net/sched/sch_fq_codel.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 5578628..437bc95 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -193,7 +193,6 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 		list_add_tail(&flow->flowchain, &q->new_flows);
 		q->new_flow_count++;
 		flow->deficit = q->quantum;
-		flow->dropped = 0;
 	}
 	if (++sch->q.qlen <= sch->limit)
 		return NET_XMIT_SUCCESS;
-- 
1.7.9.5

Re: [PATCH 1/2] fq_codel: keep dropped statistic around

[Eric Dumazet]

[-- Attachment #1.1: Type: text/plain, Size: 1031 bytes --]

On Oct 21, 2013 6:20 PM, "Dave Taht" <dave.taht@bufferbloat.net> wrote:
>
> Having more accurate dropped information in this qdisc is useful.
>
> Signed-off-by: Dave Taht <dave.taht@bufferbloat.net>
> ---
>  net/sched/sch_fq_codel.c |    1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
> index 5578628..437bc95 100644
> --- a/net/sched/sch_fq_codel.c
> +++ b/net/sched/sch_fq_codel.c
> @@ -193,7 +193,6 @@ static int fq_codel_enqueue(struct sk_buff *skb,
struct Qdisc *sch)
>                 list_add_tail(&flow->flowchain, &q->new_flows);
>                 q->new_flow_count++;
>                 flow->deficit = q->quantum;
> -               flow->dropped = 0;
>         }
>         if (++sch->q.qlen <= sch->limit)
>                 return NET_XMIT_SUCCESS;
> --
> 1.7.9.5
I am travelling to Edinburgh, so will be short.

Since fqcodel recycles a slot, we need to clear this counter. We do no know
if slot is reused by previous flow or a new flow hashing to same bucket.

[-- Attachment #1.2: Type: text/html, Size: 1411 bytes --]

[-- Attachment #2: Type: text/plain, Size: 140 bytes --]

_______________________________________________
Codel mailing list
Codel@lists.bufferbloat.net
https://lists.bufferbloat.net/listinfo/codel

Re: [PATCH 1/2] fq_codel: keep dropped statistic around

[Dave Taht]

On Mon, Oct 21, 2013 at 06:27:11PM -0700, Eric Dumazet wrote:
> On Oct 21, 2013 6:20 PM, "Dave Taht" <dave.taht@bufferbloat.net> wrote:
> >
> > Having more accurate dropped information in this qdisc is useful.
> >
> > Signed-off-by: Dave Taht <dave.taht@bufferbloat.net>
> > ---
> >  net/sched/sch_fq_codel.c |    1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
> > index 5578628..437bc95 100644
> > --- a/net/sched/sch_fq_codel.c
> > +++ b/net/sched/sch_fq_codel.c
> > @@ -193,7 +193,6 @@ static int fq_codel_enqueue(struct sk_buff *skb,
> struct Qdisc *sch)
> >                 list_add_tail(&flow->flowchain, &q->new_flows);
> >                 q->new_flow_count++;
> >                 flow->deficit = q->quantum;
> > -               flow->dropped = 0;
> >         }
> >         if (++sch->q.qlen <= sch->limit)
> >                 return NET_XMIT_SUCCESS;
> > --
> > 1.7.9.5
> I am travelling to Edinburgh, so will be short.

Wish I could have made it.

> Since fqcodel recycles a slot, we need to clear this counter. 

I prefer to think of it as a per "slot dropped counter" and to
have it retain the total number of drops in that slot since
qdisc initialization.

> We do no know
> if slot is reused by previous flow or a new flow hashing to same bucket.

There could also in this case be several flows hashing to the same bucket
and dropping packets. 

In most cases with the current zero-ing of "drop", polling "tc -s class" 
yields an unrevealing drop statistic of "0" for many workloads against
multiple streams at lower bandwidths.

with it not getting zeroed, as per this patch, clear patterns show over 
many seconds as queues empty, get filled by bursts, and get drops. 

This patch has been in openwrt and cerowrt since feburary.

[PATCH 2/2] codel: eliminate maxpacket as an inner bound

[Dave Taht]

As there is always at least one packet in the device driver or
rate limiter, the maxpacket bound (an artifact of the ns2 code)
is unneeded.

Also: in the case of TSO/GSO/GRO, it can scale well above
(to 64k) what codel's designers intended.

Someday the maxpacket variable could be eliminated entirely,
but for now it is a useful indicator of "oops, I didn't turn
off tso/gso/gro somewhere".

Signed-off-by: Dave Taht <dave.taht@bufferbloat.net>
---
 include/net/codel.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/codel.h b/include/net/codel.h
index 389cf62..319a296 100644
--- a/include/net/codel.h
+++ b/include/net/codel.h
@@ -225,7 +225,7 @@ static bool codel_should_drop(const struct sk_buff *skb,
 		stats->maxpacket = qdisc_pkt_len(skb);
 
 	if (codel_time_before(vars->ldelay, params->target) ||
-	    sch->qstats.backlog <= stats->maxpacket) {
+	    sch->qstats.backlog <= 0) {
 		/* went below - stay below for at least interval */
 		vars->first_above_time = 0;
 		return false;
-- 
1.7.9.5