From: Eric Dumazet <eric.dumazet@gmail.com>
To: Patrick McHardy <kaber@trash.net>
Cc: David Miller <davem@davemloft.net>,
netdev <netdev@vger.kernel.org>,
Jarek Poplawski <jarkao2@gmail.com>
Subject: [PATCH v3] net_sched: sch_sfq: fix allot handling
Date: Wed, 15 Dec 2010 18:30:27 +0100 [thread overview]
Message-ID: <1292434227.3427.377.camel@edumazet-laptop> (raw)
In-Reply-To: <4D08F91C.4070000@trash.net>
Le mercredi 15 décembre 2010 à 18:21 +0100, Patrick McHardy a écrit :
> On 15.12.2010 18:09, Eric Dumazet wrote:
> > Le mercredi 15 décembre 2010 à 18:03 +0100, Patrick McHardy a écrit :
> >
> >> Right, that's odd. It shouldn't be necessary anymore though since
> >> now we initialize allot in sfq_enqueue() for all new flows and
> >> increase allotment for all active flows once per round in sfq_dequeue().
> >> The above code causes a second increase for the flow following a flow
> >> which went inactive.
> >
> > Well, we do this in three places. Each time we 'select' a flow as the
> > next packet provider, we increase its allot by quantum.
> >
> > We could change this, adding quantum to the current slot when its allot
> > becomes negative (and we select the next slot for next round)
>
> Right, I again missed that 'a' refers to the next flow in the second
> condition in sfq_dequeue().
>
> I have to run now, will have another look at this tommorrow.
Here is v3 : Now we add a quantum only when current flow consumed all
its allot, not "in advance for next slot". this should use less cpu too.
Thanks
[PATCH v3] net_sched: sch_sfq: fix allot handling
When deploying SFQ/IFB here at work, I found the allot management was
pretty wrong in sfq, even changing allot from short to int...
We should init allot for each new flow, not using a previous value found
in slot.
Before patch, I saw bursts of several packets per flow, apparently
denying the default "quantum 1514" limit I had on my SFQ class.
class sfq 11:1 parent 11:
(dropped 0, overlimits 0 requeues 0)
backlog 0b 7p requeues 0
allot 11546
class sfq 11:46 parent 11:
(dropped 0, overlimits 0 requeues 0)
backlog 0b 1p requeues 0
allot -23873
class sfq 11:78 parent 11:
(dropped 0, overlimits 0 requeues 0)
backlog 0b 5p requeues 0
allot 11393
After patch, better fairness among each flow, allot limit being
respected, allot is positive :
class sfq 11:e parent 11:
(dropped 0, overlimits 0 requeues 86)
backlog 0b 3p requeues 86
allot 596
class sfq 11:94 parent 11:
(dropped 0, overlimits 0 requeues 0)
backlog 0b 3p requeues 0
allot 1468
class sfq 11:a4 parent 11:
(dropped 0, overlimits 0 requeues 0)
backlog 0b 4p requeues 0
allot 650
class sfq 11:bb parent 11:
(dropped 0, overlimits 0 requeues 0)
backlog 0b 3p requeues 0
allot 596
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/sched/sch_sfq.c | 20 ++++++++------------
1 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 3cf478d..7150705 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -270,7 +270,6 @@ static unsigned int sfq_drop(struct Qdisc *sch)
/* It is difficult to believe, but ALL THE SLOTS HAVE LENGTH 1. */
d = q->next[q->tail];
q->next[q->tail] = q->next[d];
- q->allot[q->next[d]] += q->quantum;
skb = q->qs[d].prev;
len = qdisc_pkt_len(skb);
__skb_unlink(skb, &q->qs[d]);
@@ -321,14 +320,13 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc *sch)
sfq_inc(q, x);
if (q->qs[x].qlen == 1) { /* The flow is new */
if (q->tail == SFQ_DEPTH) { /* It is the first flow */
- q->tail = x;
q->next[x] = x;
- q->allot[x] = q->quantum;
} else {
q->next[x] = q->next[q->tail];
q->next[q->tail] = x;
- q->tail = x;
}
+ q->tail = x;
+ q->allot[x] = q->quantum;
}
if (++sch->q.qlen <= q->limit) {
sch->bstats.bytes += qdisc_pkt_len(skb);
@@ -359,13 +357,13 @@ sfq_dequeue(struct Qdisc *sch)
{
struct sfq_sched_data *q = qdisc_priv(sch);
struct sk_buff *skb;
- sfq_index a, old_a;
+ sfq_index a, next_a;
/* No active slots */
if (q->tail == SFQ_DEPTH)
return NULL;
- a = old_a = q->next[q->tail];
+ a = q->next[q->tail];
/* Grab packet */
skb = __skb_dequeue(&q->qs[a]);
@@ -376,17 +374,15 @@ sfq_dequeue(struct Qdisc *sch)
/* Is the slot empty? */
if (q->qs[a].qlen == 0) {
q->ht[q->hash[a]] = SFQ_DEPTH;
- a = q->next[a];
- if (a == old_a) {
+ next_a = q->next[a];
+ if (a == next_a) {
q->tail = SFQ_DEPTH;
return skb;
}
- q->next[q->tail] = a;
- q->allot[a] += q->quantum;
+ q->next[q->tail] = next_a;
} else if ((q->allot[a] -= qdisc_pkt_len(skb)) <= 0) {
- q->tail = a;
- a = q->next[a];
q->allot[a] += q->quantum;
+ q->tail = a;
}
return skb;
}
next prev parent reply other threads:[~2010-12-15 17:30 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-12-15 14:03 [PATCH] net_sched: sch_sfq: fix allot handling Eric Dumazet
2010-12-15 16:03 ` Patrick McHardy
2010-12-15 16:27 ` Eric Dumazet
2010-12-15 16:40 ` [PATCH v2] " Eric Dumazet
2010-12-15 16:43 ` Patrick McHardy
2010-12-15 16:55 ` Eric Dumazet
2010-12-15 17:03 ` Patrick McHardy
2010-12-15 17:09 ` Eric Dumazet
2010-12-15 17:21 ` Patrick McHardy
2010-12-15 17:30 ` Eric Dumazet [this message]
2010-12-15 18:18 ` [PATCH net-next-2.6] net_sched: sch_sfq: add backlog info in sfq_dump_class_stats() Eric Dumazet
2010-12-15 19:10 ` Eric Dumazet
2010-12-16 8:16 ` Jarek Poplawski
2010-12-16 10:18 ` [PATCH v2 " Eric Dumazet
2010-12-16 11:03 ` [PATCH " Eric Dumazet
2010-12-16 13:09 ` Jarek Poplawski
2010-12-20 21:14 ` David Miller
2010-12-20 21:18 ` [PATCH v3] net_sched: sch_sfq: fix allot handling David Miller
2010-12-16 13:08 ` [PATCH v2] " Eric Dumazet
2010-12-17 16:52 ` [RFC PATCH] net_sched: sch_sfq: better struct layouts Eric Dumazet
2010-12-19 21:22 ` Jarek Poplawski
2010-12-20 17:02 ` [PATCH v2] " Eric Dumazet
2010-12-20 21:33 ` David Miller
2010-12-20 21:42 ` Eric Dumazet
2010-12-20 22:54 ` [PATCH v3 net-next-2.6] " Eric Dumazet
2010-12-21 5:33 ` David Miller
2010-12-20 22:55 ` [PATCH v2] " Jarek Poplawski
2010-12-20 23:16 ` [PATCH net-next-2.6] sch_sfq: allow big packets and be fair Eric Dumazet
2010-12-21 10:15 ` Jarek Poplawski
2010-12-21 10:30 ` Jarek Poplawski
2010-12-21 10:44 ` Eric Dumazet
2010-12-21 10:56 ` Jarek Poplawski
2010-12-21 10:57 ` Eric Dumazet
2010-12-21 11:39 ` Jarek Poplawski
2010-12-21 12:17 ` Jarek Poplawski
2010-12-21 13:04 ` [PATCH v2 " Eric Dumazet
2010-12-21 13:47 ` Jarek Poplawski
2010-12-28 21:46 ` David Miller
2010-12-29 7:53 ` [PATCH v3 " Eric Dumazet
2010-12-31 20:48 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1292434227.3427.377.camel@edumazet-laptop \
--to=eric.dumazet@gmail.com \
--cc=davem@davemloft.net \
--cc=jarkao2@gmail.com \
--cc=kaber@trash.net \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.