* [PATCH] fq_codel: dont reinit flow state
@ 2012-09-01 13:19 Eric Dumazet
2012-09-01 18:12 ` Dave Taht
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Eric Dumazet @ 2012-09-01 13:19 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Dave Taht
When fq_codel builds a new flow, it should not reset codel state.
Codel algo needs to get previous values (lastcount, drop_next) to get
proper behavior.
Signed-off-by: Dave Taht <dave.taht@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/sched/sch_fq_codel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 9fc1c62..4e606fc 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -191,7 +191,6 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch)
if (list_empty(&flow->flowchain)) {
list_add_tail(&flow->flowchain, &q->new_flows);
- codel_vars_init(&flow->cvars);
q->new_flow_count++;
flow->deficit = q->quantum;
flow->dropped = 0;
@@ -418,6 +417,7 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt)
struct fq_codel_flow *flow = q->flows + i;
INIT_LIST_HEAD(&flow->flowchain);
+ codel_vars_init(&flow->cvars);
}
}
if (sch->limit >= 1)
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] fq_codel: dont reinit flow state
2012-09-01 13:19 [PATCH] fq_codel: dont reinit flow state Eric Dumazet
@ 2012-09-01 18:12 ` Dave Taht
2012-09-02 23:17 ` Eric Dumazet
2012-09-03 0:08 ` Dave Taht
2012-09-03 18:37 ` David Miller
2 siblings, 1 reply; 5+ messages in thread
From: Dave Taht @ 2012-09-01 18:12 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
partial NAK.
There is no need to reset dropped either, and resetting quantum or not
is an interesting question.
(in both cases they can be initted at init time)
Lastly, stats->maxpacket is presently a shared resource in fq_codel,
and the initial setting of 256 was an artifact some test ns2 code
(according to kathie). In the case of ack traffic owning a queue, this
can be 64 or 66 bytes (or larger on ipv6 acks).
It's totally unclear if this distinction is important in fq_codel at
present. :/. Obviously in codel which is a pure FIFO, maxpacket will
hit the max size really fast.
A related problem on maxpacket is if someone turns off tso/gso/ufo, it
remains set to the largest packet seen forever until the qdisc is
re-initialized.
This leads to really puzzling behavior... (I'll submit a patch for
this shortly but not change fq_codel's
centralized stats)
I'd be happy (for now) if dropped was preserved,
and to fiddle with the other ideas a while longer.
On Sat, Sep 1, 2012 at 6:19 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> When fq_codel builds a new flow, it should not reset codel state.
>
> Codel algo needs to get previous values (lastcount, drop_next) to get
> proper behavior.
>
> Signed-off-by: Dave Taht <dave.taht@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> net/sched/sch_fq_codel.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
> index 9fc1c62..4e606fc 100644
> --- a/net/sched/sch_fq_codel.c
> +++ b/net/sched/sch_fq_codel.c
> @@ -191,7 +191,6 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>
> if (list_empty(&flow->flowchain)) {
> list_add_tail(&flow->flowchain, &q->new_flows);
> - codel_vars_init(&flow->cvars);
> q->new_flow_count++;
> flow->deficit = q->quantum;
> flow->dropped = 0;
> @@ -418,6 +417,7 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt)
> struct fq_codel_flow *flow = q->flows + i;
>
> INIT_LIST_HEAD(&flow->flowchain);
> + codel_vars_init(&flow->cvars);
> }
> }
> if (sch->limit >= 1)
>
>
--
Dave Täht
http://www.bufferbloat.net/projects/cerowrt/wiki - "3.3.8-17 is out
with fq_codel!"
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] fq_codel: dont reinit flow state
2012-09-01 18:12 ` Dave Taht
@ 2012-09-02 23:17 ` Eric Dumazet
0 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2012-09-02 23:17 UTC (permalink / raw)
To: Dave Taht; +Cc: David Miller, netdev
On Sat, 2012-09-01 at 11:12 -0700, Dave Taht wrote:
> partial NAK.
Kind of weird to NACK a patch you are the author.
It makes both of us plain fools on netdev, dont do that.
> There is no need to reset dropped either, and resetting quantum or not
> is an interesting question.
>
Yes but totally unrelated to this patch.
It would be good you pollute threads.
Open a new thread, using RFC attribute or something.
> (in both cases they can be initted at init time)
>
> Lastly, stats->maxpacket is presently a shared resource in fq_codel,
> and the initial setting of 256 was an artifact some test ns2 code
> (according to kathie). In the case of ack traffic owning a queue, this
> can be 64 or 66 bytes (or larger on ipv6 acks).
>
I really hope you understand how this maxpacket thing is useless ?
In the real life, we dont send only 66 bytes packets.
maxpacket purpose is to relax codel so that it doesnt drop packet if
the qdisc backlog is small (so about to be empty).
Rename this maxpacket to 'relax_codel_under_that_size' if you want to.
> It's totally unclear if this distinction is important in fq_codel at
> present. :/. Obviously in codel which is a pure FIFO, maxpacket will
> hit the max size really fast.
>
> A related problem on maxpacket is if someone turns off tso/gso/ufo, it
> remains set to the largest packet seen forever until the qdisc is
> re-initialized.
> This leads to really puzzling behavior... (I'll submit a patch for
> this shortly but not change fq_codel's
> centralized stats)
>
> I'd be happy (for now) if dropped was preserved,
> and to fiddle with the other ideas a while longer.
>
There is no dropped field, what are you talking about ?
If you are now unsure of the patch, I can repost the same patch but
as the author.
Its a real obvious one. After an idle period, CODEL needs to compute the
delay between now and previous 'drop_next'
Codel doesnt call codel_vars_init() but in the qdisc init.
fq_codel being FQ + CODEL, it is an error to call codel_vars_init()
outside of init path.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fq_codel: dont reinit flow state
2012-09-01 13:19 [PATCH] fq_codel: dont reinit flow state Eric Dumazet
2012-09-01 18:12 ` Dave Taht
@ 2012-09-03 0:08 ` Dave Taht
2012-09-03 18:37 ` David Miller
2 siblings, 0 replies; 5+ messages in thread
From: Dave Taht @ 2012-09-03 0:08 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
On Sat, Sep 1, 2012 at 6:19 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> When fq_codel builds a new flow, it should not reset codel state.
>
> Codel algo needs to get previous values (lastcount, drop_next) to get
> proper behavior.
>
> Signed-off-by: Dave Taht <dave.taht@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> net/sched/sch_fq_codel.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
> index 9fc1c62..4e606fc 100644
> --- a/net/sched/sch_fq_codel.c
> +++ b/net/sched/sch_fq_codel.c
> @@ -191,7 +191,6 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>
> if (list_empty(&flow->flowchain)) {
> list_add_tail(&flow->flowchain, &q->new_flows);
> - codel_vars_init(&flow->cvars);
> q->new_flow_count++;
> flow->deficit = q->quantum;
> flow->dropped = 0;
> @@ -418,6 +417,7 @@ static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt)
> struct fq_codel_flow *flow = q->flows + i;
>
> INIT_LIST_HEAD(&flow->flowchain);
> + codel_vars_init(&flow->cvars);
> }
> }
> if (sch->limit >= 1)
>
>
I can live with this as is. flow->dropped will probably go away in the
long term anyway.
Acked-by: Dave Taht <dave.taht@bufferbloat.net>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] fq_codel: dont reinit flow state
2012-09-01 13:19 [PATCH] fq_codel: dont reinit flow state Eric Dumazet
2012-09-01 18:12 ` Dave Taht
2012-09-03 0:08 ` Dave Taht
@ 2012-09-03 18:37 ` David Miller
2 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2012-09-03 18:37 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, dave.taht
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 01 Sep 2012 06:19:57 -0700
> When fq_codel builds a new flow, it should not reset codel state.
>
> Codel algo needs to get previous values (lastcount, drop_next) to get
> proper behavior.
>
> Signed-off-by: Dave Taht <dave.taht@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied to 'net', thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-09-03 18:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-01 13:19 [PATCH] fq_codel: dont reinit flow state Eric Dumazet
2012-09-01 18:12 ` Dave Taht
2012-09-02 23:17 ` Eric Dumazet
2012-09-03 0:08 ` Dave Taht
2012-09-03 18:37 ` 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).