[PATCH] pkt_sched: fq: clear time_next_packet for reused flows

[PATCH] pkt_sched: fq: clear time_next_packet for reused flows

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

When a socket is freed/reallocated, we need to clear time_next_packet
or else we can inherit a prior value and delay first packets of the
new flow.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/sched/sch_fq.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index a9dfdda..fdc041c 100644
--- a/net/sched/sch_fq.c
+++ b/net/sched/sch_fq.c
@@ -255,6 +255,7 @@ static struct fq_flow *fq_classify(struct sk_buff
*skb, struct fq_sched_data *q)
 				     f->socket_hash != sk->sk_hash)) {
 				f->credit = q->initial_quantum;
 				f->socket_hash = sk->sk_hash;
+				f->time_next_packet = 0ULL;
 			}
 			return f;
 		}

Re: [PATCH] pkt_sched: fq: clear time_next_packet for reused flows

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 24 Oct 2013 04:01:48 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> When a socket is freed/reallocated, we need to clear time_next_packet
> or else we can inherit a prior value and delay first packets of the
> new flow.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Eric this has been mangled by your email client, please fix.

Re: [PATCH] pkt_sched: fq: clear time_next_packet for reused flows

[Vijay Subramanian]

On 24 October 2013 04:01, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> When a socket is freed/reallocated, we need to clear time_next_packet
> or else we can inherit a prior value and delay first packets of the
> new flow.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Eric,
By the way,  it looks like fq_flow_set_throttled() uses
f->time_next_packet in an incorrect way.
It is used as key to insert the flow into the delayed rb-tree but does
not seem to check for duplicates.
This could lead to rb tree corruption. I assume it is possible for
different flows to have same f->time_next_packet.
Do we at least we need a BUG_ON(f->time_next_packet ==
aux->time_next_packet) here?


Code in fq_flow_set_throttled():
        while (*p) {
                struct fq_flow *aux;

                parent = *p;
                aux = container_of(parent, struct fq_flow, rate_node);
                if (f->time_next_packet >= aux->time_next_packet)
                        p = &parent->rb_right;
                else
                        p = &parent->rb_left;
        }
        rb_link_node(&f->rate_node, parent, p);
        rb_insert_color(&f->rate_node, &q->delayed);

Thanks!
-vijay

[PATCH v2] pkt_sched: fq: clear time_next_packet for reused flows

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

When a socket is freed/reallocated, we need to clear time_next_packet
or else we can inherit a prior value and delay first packets of the
new flow.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
v2: wrapping issue

 net/sched/sch_fq.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index a9dfdda..fdc041c 100644
--- a/net/sched/sch_fq.c
+++ b/net/sched/sch_fq.c
@@ -255,6 +255,7 @@ static struct fq_flow *fq_classify(struct sk_buff *skb, struct fq_sched_data *q)
 				     f->socket_hash != sk->sk_hash)) {
 				f->credit = q->initial_quantum;
 				f->socket_hash = sk->sk_hash;
+				f->time_next_packet = 0ULL;
 			}
 			return f;
 		}

Re: [PATCH] pkt_sched: fq: clear time_next_packet for reused flows

[Eric Dumazet]

On Sun, 2013-10-27 at 15:06 -0700, Vijay Subramanian wrote:

> Eric,
> By the way,  it looks like fq_flow_set_throttled() uses
> f->time_next_packet in an incorrect way.
> It is used as key to insert the flow into the delayed rb-tree but does
> not seem to check for duplicates.
> This could lead to rb tree corruption. I assume it is possible for
> different flows to have same f->time_next_packet.
> Do we at least we need a BUG_ON(f->time_next_packet ==
> aux->time_next_packet) here?
> 

What exact RB corruptions do you see ?

RB trees allow items with the same key.

HTB and netem use this property and I never saw any corruption,
but you could be right so please elaborate, thanks !

Re: [PATCH v2] pkt_sched: fq: clear time_next_packet for reused flows

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 27 Oct 2013 16:26:43 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> When a socket is freed/reallocated, we need to clear time_next_packet
> or else we can inherit a prior value and delay first packets of the
> new flow.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.

Re: [PATCH] pkt_sched: fq: clear time_next_packet for reused flows

[Vijay Subramanian]

>>
>
> What exact RB corruptions do you see ?
>
> RB trees allow items with the same key.
>
Thanks for the clarification. I incorrectly thought duplicates are not allowed.
Sorry for the noise.

Vijay