SFQ depth limit

SFQ depth limit

[Denys Fedoryshchenko]

Hi

Is there any particular reason to limit SFQ depth to 127 packets?
#define SFQ_DEPTH               128

Just to buffer data on 1Gbps rate for 1 second i need 87k packets, let's say just for 10ms buffer - 873 packets, which is out of limit.
Also if i will use external SFQ flow classifier made by Patrick McHardy, let's say for 4096 ip's, i am not sure also if i hit this limit. 
As i understand, if i increase it - it will increase also data structures size in memory.

And probably i hit some unknown issue.
Example:
tc qdisc add dev eth0 handle 1 root sfq
tc filter add dev eth0 protocol ip pref 1 parent 1: handle 1 flow hash keys dst divisor 1024
MegaRouterCore-KARAM ~ #tc -s qdisc show dev eth0
qdisc sfq 1: root limit 127p quantum 1514b
 Sent 12062 bytes 89 pkt (dropped 3, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0
MegaRouterCore-KARAM ~ #tc -s qdisc show dev eth0
qdisc sfq 1: root limit 127p quantum 1514b
 Sent 12824 bytes 94 pkt (dropped 3, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0
MegaRouterCore-KARAM ~ #tc -s qdisc show dev eth0
qdisc sfq 1: root limit 127p quantum 1514b
 Sent 13586 bytes 99 pkt (dropped 3, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0
MegaRouterCore-KARAM ~ #tc -s -d qdisc show dev eth0
qdisc sfq 1: root limit 127p quantum 1514b flows 127/1024
 Sent 15552 bytes 114 pkt (dropped 8, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0
MegaRouterCore-KARAM ~ #tc -s -d qdisc show dev eth0
qdisc sfq 1: root limit 127p quantum 1514b flows 127/1024
 Sent 16232 bytes 118 pkt (dropped 9, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0

after that host is dead...



-- 
------
Technical Manager
Virtual ISP S.A.L.
Lebanon

[PATCH] Re: SFQ depth limit

[Jarek Poplawski]

Denys Fedoryshchenko wrote, On 06/28/2008 09:02 PM:

> Hi
> 
> Is there any particular reason to limit SFQ depth to 127 packets?
> #define SFQ_DEPTH               128
> 
> Just to buffer data on 1Gbps rate for 1 second i need 87k packets, let's say just for 10ms buffer - 873 packets, which is out of limit.
> Also if i will use external SFQ flow classifier made by Patrick McHardy, let's say for 4096 ip's, i am not sure also if i hit this limit. 
> As i understand, if i increase it - it will increase also data structures size in memory.

Hi,

Yes, this 128 limit is extremely low, but changing this is not enough:
you've to change at least sfq_index typedef too, eg. to unsigned short,
but I didn't check if this is all.

> And probably i hit some unknown issue.
> Example:
> tc qdisc add dev eth0 handle 1 root sfq
> tc filter add dev eth0 protocol ip pref 1 parent 1: handle 1 flow hash keys dst divisor 1024
...
> MegaRouterCore-KARAM ~ #tc -s -d qdisc show dev eth0
> qdisc sfq 1: root limit 127p quantum 1514b flows 127/1024
>  Sent 16232 bytes 118 pkt (dropped 9, overlimits 0 requeues 0)
>  rate 0bit 0pps backlog 0b 0p requeues 0
> 
> after that host is dead...

If it's with vanilla kernel something is wrong, but I couldn't
reproduce this: probably some additianal data is needed. BTW,
I see this "flows" number above isn't very useful, as it always
repeats "limit". I attach a patch proposal to change this.

Regards,
Jarek P.

----------------------->

sch_sfq: dump a real number of flows

Dump the "flows" number according to the number of active flows
instead of repeating the "limit".


Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 net/sched/sch_sfq.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index f0463d7..5bc78c3 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -536,7 +536,14 @@ static int sfq_dump(struct Qdisc *sch, struct sk_buff *skb)
 
 	opt.limit = q->limit;
 	opt.divisor = SFQ_HASH_DIVISOR;
-	opt.flows = q->limit;
+	opt.flows = 0;
+	if (q->tail != SFQ_DEPTH) {
+		unsigned int i;
+
+		for (i = 0; i < SFQ_HASH_DIVISOR; i++)
+			if (q->ht[i] != SFQ_DEPTH)
+				opt.flows++;
+	}
 
 	NLA_PUT(skb, TCA_OPTIONS, sizeof(opt), &opt);
 

Re: [PATCH] Re: SFQ depth limit

[Denys Fedoryshchenko]

On Sunday 29 June 2008 22:11, Jarek Poplawski wrote:
> If it's with vanilla kernel something is wrong, but I couldn't
> reproduce this: probably some additianal data is needed. BTW,
> I see this "flows" number above isn't very useful, as it always
> repeats "limit". I attach a patch proposal to change this.
No it is ok, since filter is for protocol IP, ARP will not work. Thats what i found. 
Incorrect QoS settings can cause network breakage, thats it :-)

I will try to test patch ASAP on real load.

-- 
------
Technical Manager
Virtual ISP S.A.L.
Lebanon

Re: [PATCH] Re: SFQ depth limit

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Sun, 29 Jun 2008 21:11:13 +0200

> sch_sfq: dump a real number of flows
> 
> Dump the "flows" number according to the number of active flows
> instead of repeating the "limit".
> 
> 
> Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Looks reasonable, applied, thanks Jarek.

Re: [PATCH] Re: SFQ depth limit

[Patrick McHardy]

David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Sun, 29 Jun 2008 21:11:13 +0200
>
>   
>> sch_sfq: dump a real number of flows
>>
>> Dump the "flows" number according to the number of active flows
>> instead of repeating the "limit".
>>     
>
> Looks reasonable, applied, thanks Jarek.

I'm not sure we should do this, this removes the symetry between
->init/change and ->dump. Its not a big deal in this case since
flows is unused in ->init anyway, but still its a not so nice
precedent. Such things should normally be put in the statistics.

Re: [PATCH] Re: SFQ depth limit

[Jarek Poplawski]

On Thu, Jul 24, 2008 at 10:45:39AM +0200, Patrick McHardy wrote:
> David Miller wrote:
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> Date: Sun, 29 Jun 2008 21:11:13 +0200
>>
>>   
>>> sch_sfq: dump a real number of flows
>>>
>>> Dump the "flows" number according to the number of active flows
>>> instead of repeating the "limit".
>>>     
>>
>> Looks reasonable, applied, thanks Jarek.
>
> I'm not sure we should do this, this removes the symetry between
> ->init/change and ->dump. Its not a big deal in this case since
> flows is unused in ->init anyway, but still its a not so nice
> precedent. Such things should normally be put in the statistics.
>

Hmm... I didn't even notice this. But I guess, this patch didn't
introduce this precedent since this non-config "flow" was dumped
before. Btw., I can see e.g. direct_pkts in htb_dump() which looks
like similar precedent. IMHO, printing the same thing 2x under
different names is worse precedent. Anyway, this patch isn't any
big deal, so I can send a revert.

Thanks,
Jarek P.

Re: [PATCH] Re: SFQ depth limit

[Patrick McHardy]

Jarek Poplawski wrote:
> On Thu, Jul 24, 2008 at 10:45:39AM +0200, Patrick McHardy wrote:
>>
>> I'm not sure we should do this, this removes the symetry between
>> ->init/change and ->dump. Its not a big deal in this case since
>> flows is unused in ->init anyway, but still its a not so nice
>> precedent. Such things should normally be put in the statistics.
>
> 
> Hmm... I didn't even notice this. But I guess, this patch didn't
> introduce this precedent since this non-config "flow" was dumped
> before. 

Yes, thats true, but see below.

> Btw., I can see e.g. direct_pkts in htb_dump() which looks
> like similar precedent.

Ugh, yes, that also should have been in the statistics.

> IMHO, printing the same thing 2x under
> different names is worse precedent. Anyway, this patch isn't any
> big deal, so I can send a revert.

I think its not exactly 2x the same thing. "flows" has a different
meaning than limit (its the depth), but because its hardcoded its
not parsed in ->init. And because of implementation details "flows"
is limited by "limit", which itself is limited by the hardcoded
depth, so they just happen to always have the same value. Using
the current number of *active* flows doesn't really match the
original meaning anymore. IIRC the ESFQ for SFQ patches even use
"flows" in ->init, so it might make things a bit more ugly if we
ever merge those patches.

[PATCH] pkt_sched: sch_sfq: revert "dump a real number of flows" patch

[Jarek Poplawski]

On Thu, Jul 24, 2008 at 11:57:15AM +0200, Patrick McHardy wrote:
...
> IIRC the ESFQ for SFQ patches even use
> "flows" in ->init, so it might make things a bit more ugly if we
> ever merge those patches.

This argument looks right to me.

David, please, apply this reverting patch.

Thanks,
Jarek P.

-------------------->

pkt_sched: sch_sfq: revert "dump a real number of flows" patch

Patrick McHardy <kaber@trash.net> noticed that since "flows" could be
used by sfq in the future as a parameter, we shouldn't currently
change it's meaning. This patch reverts my previous patch:
commit f867e6af94239a04ec23aeec2fcda5aa58e41db7


Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

diff -Nurp net-2.6-/net/sched/sch_sfq.c net-2.6+/net/sched/sch_sfq.c
--- net-2.6-/net/sched/sch_sfq.c	2008-07-24 10:26:08.000000000 +0000
+++ net-2.6+/net/sched/sch_sfq.c	2008-07-21 09:32:53.000000000 +0000
@@ -536,14 +536,7 @@ static int sfq_dump(struct Qdisc *sch, s
 
 	opt.limit = q->limit;
 	opt.divisor = SFQ_HASH_DIVISOR;
-	opt.flows = 0;
-	if (q->tail != SFQ_DEPTH) {
-		unsigned int i;
-
-		for (i = 0; i < SFQ_HASH_DIVISOR; i++)
-			if (q->ht[i] != SFQ_DEPTH)
-				opt.flows++;
-	}
+	opt.flows = q->limit;
 
 	NLA_PUT(skb, TCA_OPTIONS, sizeof(opt), &opt);
 

Re: [PATCH] pkt_sched: sch_sfq: revert "dump a real number of flows" patch

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Thu, 24 Jul 2008 10:53:52 +0000

> On Thu, Jul 24, 2008 at 11:57:15AM +0200, Patrick McHardy wrote:
> ...
> > IIRC the ESFQ for SFQ patches even use
> > "flows" in ->init, so it might make things a bit more ugly if we
> > ever merge those patches.
> 
> This argument looks right to me.
> 
> David, please, apply this reverting patch.

I've reverted the changeset using "git revert".

Thanks!

Re: [PATCH] pkt_sched: sch_sfq: revert "dump a real number of flows" patch

[Jarek Poplawski]

On Sat, Jul 26, 2008 at 02:29:27AM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Thu, 24 Jul 2008 10:53:52 +0000
> 
> > On Thu, Jul 24, 2008 at 11:57:15AM +0200, Patrick McHardy wrote:
> > ...
> > > IIRC the ESFQ for SFQ patches even use
> > > "flows" in ->init, so it might make things a bit more ugly if we
> > > ever merge those patches.
> > 
> > This argument looks right to me.
> > 
> > David, please, apply this reverting patch.
> 
> I've reverted the changeset using "git revert".
> 

Is it legal now? (I've seen this in Linus' tree already...)

Thanks,
Jarek P.

Re: [PATCH] pkt_sched: sch_sfq: revert "dump a real number of flows" patch

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Sat, 26 Jul 2008 12:58:26 +0200

> On Sat, Jul 26, 2008 at 02:29:27AM -0700, David Miller wrote:
> > From: Jarek Poplawski <jarkao2@gmail.com>
> > Date: Thu, 24 Jul 2008 10:53:52 +0000
> > 
> > > On Thu, Jul 24, 2008 at 11:57:15AM +0200, Patrick McHardy wrote:
> > > ...
> > > > IIRC the ESFQ for SFQ patches even use
> > > > "flows" in ->init, so it might make things a bit more ugly if we
> > > > ever merge those patches.
> > > 
> > > This argument looks right to me.
> > > 
> > > David, please, apply this reverting patch.
> > 
> > I've reverted the changeset using "git revert".
> > 
> 
> Is it legal now? (I've seen this in Linus' tree already...)

Absolutely, the SHA IDs will match, it will work perfectly
fine.