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

Signed-off-by: David Ward <redacted>
---
 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

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 <redacted>
Signed-off-by: David Ward <redacted>
---
 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

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 <redacted>
---
 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

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

Signed-off-by: David Ward <redacted>
---
 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

On 12-09-13 11:22 AM, David Ward wrote:

Signed-off-by: David Ward <redacted>

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

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 <redacted>

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

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 <redacted>

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

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 <redacted>
Signed-off-by: David Ward <redacted>

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

On 13/09/12 14:08, Jamal Hadi Salim wrote:

On 12-09-13 11:22 AM, David Ward wrote:

quoted

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 <redacted>
Signed-off-by: David Ward <redacted>

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

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:

quoted

Signed-off-by: David Ward <redacted>

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

Applied.

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:

quoted

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

Signed-off-by: David Ward <redacted>

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

Applied.

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:

quoted

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 <redacted>

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

Applied.

From: "Ward, David - 0663 - MITLL" <redacted>
Date: Thu, 13 Sep 2012 15:38:26 -0400

On 13/09/12 14:08, Jamal Hadi Salim wrote:

quoted

On 12-09-13 11:22 AM, David Ward wrote:

quoted

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 <redacted>
Signed-off-by: David Ward <redacted>

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.

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