[PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode

[PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode

[David Ward]

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 net/sched/sch_gred.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index e901583..fca73cd 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -176,7 +176,7 @@ static int gred_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 		skb->tc_index = (skb->tc_index & ~GRED_VQ_MASK) | dp;
 	}
 
-	/* sum up all the qaves of prios <= to ours to get the new qave */
+	/* sum up all the qaves of prios < ours to get the new qave */
 	if (!gred_wred_mode(t) && gred_rio_mode(t)) {
 		int i;
 
-- 
1.7.4.1

[PATCH 2/4] net_sched: gred: eliminate redundant DP prio comparisons

[David Ward]

Each pair of DPs only needs to be compared once when searching for
a non-unique prio value.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 net/sched/sch_gred.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index fca73cd..e19d4eb 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -102,9 +102,8 @@ static inline int gred_wred_mode_check(struct Qdisc *sch)
 		if (q == NULL)
 			continue;
 
-		for (n = 0; n < table->DPs; n++)
-			if (table->tab[n] && table->tab[n] != q &&
-			    table->tab[n]->prio == q->prio)
+		for (n = i + 1; n < table->DPs; n++)
+			if (table->tab[n] && table->tab[n]->prio == q->prio)
 				return 1;
 	}
 
-- 
1.7.4.1

[PATCH 3/4] net_sched: gred: fix qave reporting via netlink

[David Ward]

q->vars.qavg is a Wlog scaled value, but q->backlog is not. In order
to pass q->vars.qavg as the backlog value, we need to un-scale it.
Additionally, the qave value returned via netlink should not be Wlog
scaled, so we need to un-scale the result of red_calc_qavg().

This caused artificially high values for "Average Queue" to be shown
by 'tc -s -d qdisc', but did not affect the actual operation of GRED.

Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 net/sched/sch_gred.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index e19d4eb..b2570b5 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -534,6 +534,7 @@ static int gred_dump(struct Qdisc *sch, struct sk_buff *skb)
 	for (i = 0; i < MAX_DPs; i++) {
 		struct gred_sched_data *q = table->tab[i];
 		struct tc_gred_qopt opt;
+		unsigned long qavg;
 
 		memset(&opt, 0, sizeof(opt));
 
@@ -565,7 +566,9 @@ static int gred_dump(struct Qdisc *sch, struct sk_buff *skb)
 		if (gred_wred_mode(table))
 			gred_load_wred_set(table, q);
 
-		opt.qave = red_calc_qavg(&q->parms, &q->vars, q->vars.qavg);
+		qavg = red_calc_qavg(&q->parms, &q->vars,
+				     q->vars.qavg >> q->parms.Wlog);
+		opt.qave = qavg >> q->parms.Wlog;
 
 append_opt:
 		if (nla_append(skb, sizeof(opt), &opt) < 0)
-- 
1.7.4.1

[PATCH 4/4] net_sched: gred: actually perform idling in WRED mode

[David Ward]

gred_dequeue() and gred_drop() do not seem to get called when the
queue is empty, meaning that we never start idling while in WRED
mode. And since qidlestart is not stored by gred_store_wred_set(),
we would never stop idling while in WRED mode if we ever started.
This messes up the average queue size calculation that influences
packet marking/dropping behavior.

Now, we start WRED mode idling as we are removing the last packet
from the queue. Also we now actually stop WRED mode idling when we
are enqueuing a packet.

Cc: Bruce Osler <brosler@cisco.com>
Signed-off-by: David Ward <david.ward@ll.mit.edu>
---
 net/sched/sch_gred.c |   26 +++++++++++++++-----------
 1 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index b2570b5..d42234c 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -136,6 +136,7 @@ static inline void gred_store_wred_set(struct gred_sched *table,
 				       struct gred_sched_data *q)
 {
 	table->wred_set.qavg = q->vars.qavg;
+	table->wred_set.qidlestart = q->vars.qidlestart;
 }
 
 static inline int gred_use_ecn(struct gred_sched *t)
@@ -259,16 +260,18 @@ static struct sk_buff *gred_dequeue(struct Qdisc *sch)
 		} else {
 			q->backlog -= qdisc_pkt_len(skb);
 
-			if (!q->backlog && !gred_wred_mode(t))
-				red_start_of_idle_period(&q->vars);
+			if (gred_wred_mode(t)) {
+				if (!sch->qstats.backlog)
+					red_start_of_idle_period(&t->wred_set);
+			} else {
+				if (!q->backlog)
+					red_start_of_idle_period(&q->vars);
+			}
 		}
 
 		return skb;
 	}
 
-	if (gred_wred_mode(t) && !red_is_idling(&t->wred_set))
-		red_start_of_idle_period(&t->wred_set);
-
 	return NULL;
 }
 
@@ -290,19 +293,20 @@ static unsigned int gred_drop(struct Qdisc *sch)
 			q->backlog -= len;
 			q->stats.other++;
 
-			if (!q->backlog && !gred_wred_mode(t))
-				red_start_of_idle_period(&q->vars);
+			if (gred_wred_mode(t)) {
+				if (!sch->qstats.backlog)
+					red_start_of_idle_period(&t->wred_set);
+			} else {
+				if (!q->backlog)
+					red_start_of_idle_period(&q->vars);
+			}
 		}
 
 		qdisc_drop(skb, sch);
 		return len;
 	}
 
-	if (gred_wred_mode(t) && !red_is_idling(&t->wred_set))
-		red_start_of_idle_period(&t->wred_set);
-
 	return 0;
-
 }
 
 static void gred_reset(struct Qdisc *sch)
-- 
1.7.4.1

Re: [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode

[Jamal Hadi Salim]

On 12-09-13 11:22 AM, David Ward wrote:
> Signed-off-by: David Ward <david.ward@ll.mit.edu>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Re: [PATCH 2/4] net_sched: gred: eliminate redundant DP prio comparisons

[Jamal Hadi Salim]

On 12-09-13 11:22 AM, David Ward wrote:
> Each pair of DPs only needs to be compared once when searching for
> a non-unique prio value.
>
> Signed-off-by: David Ward <david.ward@ll.mit.edu>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Re: [PATCH 3/4] net_sched: gred: fix qave reporting via netlink

[Jamal Hadi Salim]

On 12-09-13 11:22 AM, David Ward wrote:
> q->vars.qavg is a Wlog scaled value, but q->backlog is not. In order
> to pass q->vars.qavg as the backlog value, we need to un-scale it.
> Additionally, the qave value returned via netlink should not be Wlog
> scaled, so we need to un-scale the result of red_calc_qavg().
>
> This caused artificially high values for "Average Queue" to be shown
> by 'tc -s -d qdisc', but did not affect the actual operation of GRED.
>
> Signed-off-by: David Ward <david.ward@ll.mit.edu>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Re: [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode

[Jamal Hadi Salim]

On 12-09-13 11:22 AM, David Ward wrote:
> gred_dequeue() and gred_drop() do not seem to get called when the
> queue is empty, meaning that we never start idling while in WRED
> mode. And since qidlestart is not stored by gred_store_wred_set(),
> we would never stop idling while in WRED mode if we ever started.
> This messes up the average queue size calculation that influences
> packet marking/dropping behavior.
>
> Now, we start WRED mode idling as we are removing the last packet
> from the queue. Also we now actually stop WRED mode idling when we
> are enqueuing a packet.
>
> Cc: Bruce Osler <brosler@cisco.com>
> Signed-off-by: David Ward <david.ward@ll.mit.edu>

This is one is not so obvious. Iam assuming you vetted it via some tests.
In which case:
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode

[Ward, David - 0663 - MITLL]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

On 13/09/12 14:08, Jamal Hadi Salim wrote:
> On 12-09-13 11:22 AM, David Ward wrote:
>> gred_dequeue() and gred_drop() do not seem to get called when the
>> queue is empty, meaning that we never start idling while in WRED
>> mode. And since qidlestart is not stored by gred_store_wred_set(),
>> we would never stop idling while in WRED mode if we ever started.
>> This messes up the average queue size calculation that influences
>> packet marking/dropping behavior.
>>
>> Now, we start WRED mode idling as we are removing the last packet
>> from the queue. Also we now actually stop WRED mode idling when we
>> are enqueuing a packet.
>>
>> Cc: Bruce Osler <brosler@cisco.com>
>> Signed-off-by: David Ward <david.ward@ll.mit.edu>
> This is one is not so obvious. Iam assuming you vetted it via some tests.
> In which case:
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> cheers,
> jamal
>

Before applying this patch, the average queue size (as seen with "tc -s 
qdisc") remained constant forever after I stopped sending any packets 
through the interface -- it didn't taper off as you would expect.  After 
the patch, the average queue size will now taper off if packets are not 
being sent.

David


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4571 bytes --]

Re: [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Thu, 13 Sep 2012 13:59:24 -0400

> On 12-09-13 11:22 AM, David Ward wrote:
>> Signed-off-by: David Ward <david.ward@ll.mit.edu>
> 
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Applied.

Re: [PATCH 2/4] net_sched: gred: eliminate redundant DP prio comparisons

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Thu, 13 Sep 2012 14:00:30 -0400

> On 12-09-13 11:22 AM, David Ward wrote:
>> Each pair of DPs only needs to be compared once when searching for
>> a non-unique prio value.
>>
>> Signed-off-by: David Ward <david.ward@ll.mit.edu>
> 
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Applied.

Re: [PATCH 3/4] net_sched: gred: fix qave reporting via netlink

[David Miller]

From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Thu, 13 Sep 2012 14:01:59 -0400

> On 12-09-13 11:22 AM, David Ward wrote:
>> q->vars.qavg is a Wlog scaled value, but q->backlog is not. In order
>> to pass q->vars.qavg as the backlog value, we need to un-scale it.
>> Additionally, the qave value returned via netlink should not be Wlog
>> scaled, so we need to un-scale the result of red_calc_qavg().
>>
>> This caused artificially high values for "Average Queue" to be shown
>> by 'tc -s -d qdisc', but did not affect the actual operation of GRED.
>>
>> Signed-off-by: David Ward <david.ward@ll.mit.edu>
> 
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Applied.

Re: [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode

[David Miller]

From: "Ward, David - 0663 - MITLL" <david.ward@ll.mit.edu>
Date: Thu, 13 Sep 2012 15:38:26 -0400

> On 13/09/12 14:08, Jamal Hadi Salim wrote:
>> On 12-09-13 11:22 AM, David Ward wrote:
>>> gred_dequeue() and gred_drop() do not seem to get called when the
>>> queue is empty, meaning that we never start idling while in WRED
>>> mode. And since qidlestart is not stored by gred_store_wred_set(),
>>> we would never stop idling while in WRED mode if we ever started.
>>> This messes up the average queue size calculation that influences
>>> packet marking/dropping behavior.
>>>
>>> Now, we start WRED mode idling as we are removing the last packet
>>> from the queue. Also we now actually stop WRED mode idling when we
>>> are enqueuing a packet.
>>>
>>> Cc: Bruce Osler <brosler@cisco.com>
>>> Signed-off-by: David Ward <david.ward@ll.mit.edu>
>> This is one is not so obvious. Iam assuming you vetted it via some
>> tests.
>> In which case:
>> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> cheers,
>> jamal
>>
> 
> Before applying this patch, the average queue size (as seen with "tc
> -s qdisc") remained constant forever after I stopped sending any
> packets through the interface -- it didn't taper off as you would
> expect.  After the patch, the average queue size will now taper off if
> packets are not being sent.

Applied.

Re: [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode

[Jamal Hadi Salim]

On 12-09-13 03:38 PM, Ward, David - 0663 - MITLL wrote:
> Before applying this patch, the average queue size (as seen with "tc 
> -s qdisc") remained constant forever after I stopped sending any 
> packets through the interface -- it didn't taper off as you would 
> expect.  After the patch, the average queue size will now taper off if 
> packets are not being sent.
>


That makes sense. thanks

cheers,
jamal