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

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode
@ 2012-09-13 15:22 David Ward
  2012-09-13 15:22 ` [PATCH 2/4] net_sched: gred: eliminate redundant DP prio comparisons David Ward
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: David Ward @ 2012-09-13 15:22 UTC (permalink / raw)
  To: netdev; +Cc: Bruce Osler, Cyril Chemparathy, Jamal Hadi Salim, 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/4] net_sched: gred: eliminate redundant DP prio comparisons
  2012-09-13 15:22 [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode David Ward
@ 2012-09-13 15:22 ` David Ward
  2012-09-13 18:00   ` Jamal Hadi Salim
  2012-09-13 15:22 ` [PATCH 3/4] net_sched: gred: fix qave reporting via netlink David Ward
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: David Ward @ 2012-09-13 15:22 UTC (permalink / raw)
  To: netdev; +Cc: Bruce Osler, Cyril Chemparathy, Jamal Hadi Salim, 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] net_sched: gred: eliminate redundant DP prio comparisons
  2012-09-13 15:22 ` [PATCH 2/4] net_sched: gred: eliminate redundant DP prio comparisons David Ward
@ 2012-09-13 18:00   ` Jamal Hadi Salim
  2012-09-13 20:10     ` David Miller
  0 siblings, 1 reply; 14+ messages in thread
From: Jamal Hadi Salim @ 2012-09-13 18:00 UTC (permalink / raw)
  To: David Ward; +Cc: netdev, Bruce Osler, Cyril Chemparathy

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>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] net_sched: gred: eliminate redundant DP prio comparisons
  2012-09-13 18:00   ` Jamal Hadi Salim
@ 2012-09-13 20:10     ` David Miller
  0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2012-09-13 20:10 UTC (permalink / raw)
  To: jhs; +Cc: david.ward, netdev, brosler, cyril

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 3/4] net_sched: gred: fix qave reporting via netlink
  2012-09-13 15:22 [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode David Ward
  2012-09-13 15:22 ` [PATCH 2/4] net_sched: gred: eliminate redundant DP prio comparisons David Ward
@ 2012-09-13 15:22 ` David Ward
  2012-09-13 18:01   ` Jamal Hadi Salim
  2012-09-13 15:22 ` [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode David Ward
  2012-09-13 17:59 ` [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode Jamal Hadi Salim
  3 siblings, 1 reply; 14+ messages in thread
From: David Ward @ 2012-09-13 15:22 UTC (permalink / raw)
  To: netdev; +Cc: Bruce Osler, Cyril Chemparathy, Jamal Hadi Salim, 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] net_sched: gred: fix qave reporting via netlink
  2012-09-13 15:22 ` [PATCH 3/4] net_sched: gred: fix qave reporting via netlink David Ward
@ 2012-09-13 18:01   ` Jamal Hadi Salim
  2012-09-13 20:10     ` David Miller
  0 siblings, 1 reply; 14+ messages in thread
From: Jamal Hadi Salim @ 2012-09-13 18:01 UTC (permalink / raw)
  To: David Ward; +Cc: netdev, Bruce Osler, Cyril Chemparathy

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>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/4] net_sched: gred: fix qave reporting via netlink
  2012-09-13 18:01   ` Jamal Hadi Salim
@ 2012-09-13 20:10     ` David Miller
  0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2012-09-13 20:10 UTC (permalink / raw)
  To: jhs; +Cc: david.ward, netdev, brosler, cyril

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode
  2012-09-13 15:22 [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode David Ward
  2012-09-13 15:22 ` [PATCH 2/4] net_sched: gred: eliminate redundant DP prio comparisons David Ward
  2012-09-13 15:22 ` [PATCH 3/4] net_sched: gred: fix qave reporting via netlink David Ward
@ 2012-09-13 15:22 ` David Ward
  2012-09-13 18:08   ` Jamal Hadi Salim
  2012-09-13 17:59 ` [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode Jamal Hadi Salim
  3 siblings, 1 reply; 14+ messages in thread
From: David Ward @ 2012-09-13 15:22 UTC (permalink / raw)
  To: netdev; +Cc: Bruce Osler, Cyril Chemparathy, Jamal Hadi Salim, 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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode
  2012-09-13 15:22 ` [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode David Ward
@ 2012-09-13 18:08   ` Jamal Hadi Salim
  2012-09-13 19:38     ` Ward, David - 0663 - MITLL
  0 siblings, 1 reply; 14+ messages in thread
From: Jamal Hadi Salim @ 2012-09-13 18:08 UTC (permalink / raw)
  To: David Ward; +Cc: netdev, Bruce Osler, Cyril Chemparathy

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode
  2012-09-13 18:08   ` Jamal Hadi Salim
@ 2012-09-13 19:38     ` Ward, David - 0663 - MITLL
  2012-09-13 20:10       ` David Miller
  2012-09-13 20:37       ` Jamal Hadi Salim
  0 siblings, 2 replies; 14+ messages in thread
From: Ward, David - 0663 - MITLL @ 2012-09-13 19:38 UTC (permalink / raw)
  To: Jamal Hadi Salim; +Cc: netdev@vger.kernel.org, Bruce Osler, Cyril Chemparathy

[-- 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 --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode
  2012-09-13 19:38     ` Ward, David - 0663 - MITLL
@ 2012-09-13 20:10       ` David Miller
  2012-09-13 20:37       ` Jamal Hadi Salim
  1 sibling, 0 replies; 14+ messages in thread
From: David Miller @ 2012-09-13 20:10 UTC (permalink / raw)
  To: david.ward; +Cc: jhs, netdev, brosler, cyril

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode
  2012-09-13 19:38     ` Ward, David - 0663 - MITLL
  2012-09-13 20:10       ` David Miller
@ 2012-09-13 20:37       ` Jamal Hadi Salim
  1 sibling, 0 replies; 14+ messages in thread
From: Jamal Hadi Salim @ 2012-09-13 20:37 UTC (permalink / raw)
  To: Ward, David - 0663 - MITLL
  Cc: netdev@vger.kernel.org, Bruce Osler, Cyril Chemparathy

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode
  2012-09-13 15:22 [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode David Ward
                   ` (2 preceding siblings ...)
  2012-09-13 15:22 ` [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode David Ward
@ 2012-09-13 17:59 ` Jamal Hadi Salim
  2012-09-13 20:10   ` David Miller
  3 siblings, 1 reply; 14+ messages in thread
From: Jamal Hadi Salim @ 2012-09-13 17:59 UTC (permalink / raw)
  To: David Ward; +Cc: netdev, Bruce Osler, Cyril Chemparathy

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>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode
  2012-09-13 17:59 ` [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode Jamal Hadi Salim
@ 2012-09-13 20:10   ` David Miller
  0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2012-09-13 20:10 UTC (permalink / raw)
  To: jhs; +Cc: david.ward, netdev, brosler, cyril

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2012-09-13 20:37 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-13 15:22 [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode David Ward
2012-09-13 15:22 ` [PATCH 2/4] net_sched: gred: eliminate redundant DP prio comparisons David Ward
2012-09-13 18:00   ` Jamal Hadi Salim
2012-09-13 20:10     ` David Miller
2012-09-13 15:22 ` [PATCH 3/4] net_sched: gred: fix qave reporting via netlink David Ward
2012-09-13 18:01   ` Jamal Hadi Salim
2012-09-13 20:10     ` David Miller
2012-09-13 15:22 ` [PATCH 4/4] net_sched: gred: actually perform idling in WRED mode David Ward
2012-09-13 18:08   ` Jamal Hadi Salim
2012-09-13 19:38     ` Ward, David - 0663 - MITLL
2012-09-13 20:10       ` David Miller
2012-09-13 20:37       ` Jamal Hadi Salim
2012-09-13 17:59 ` [PATCH 1/4] net_sched: gred: correct comment about qavg calculation in RIO mode Jamal Hadi Salim
2012-09-13 20:10   ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).