[PATCH net] fq_codel: fix NET_XMIT_CN behavior

[PATCH net] fq_codel: fix NET_XMIT_CN behavior

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

My prior attempt to fix the backlogs of parents failed.

If we return NET_XMIT_CN, our parents wont increase their backlog,
so our qdisc_tree_reduce_backlog() should take this into account.

Fixes: 9d18562a2278 ("fq_codel: add batch ability to fq_codel_drop()")
Fixes: 2ccccf5fb43f ("net_sched: update hierarchical backlog too")
Reported-by: Stas Nichiporovich <stasn77@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: WANG Cong <xiyou.wangcong@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_fq_codel.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 6883a8971562..c57ec480a2da 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -240,11 +240,19 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	q->drop_overlimit += prev_qlen - sch->q.qlen;
 	if (memory_limited)
 		q->drop_overmemory += prev_qlen - sch->q.qlen;
-	/* As we dropped packet(s), better let upper stack know this */
-	qdisc_tree_reduce_backlog(sch, prev_qlen - sch->q.qlen,
-				  prev_backlog - sch->qstats.backlog);
-
-	return ret == idx ? NET_XMIT_CN : NET_XMIT_SUCCESS;
+	/* As we dropped packet(s), better let upper stack know this.
+	 * If we dropped a packet for this flow, return NET_XMIT_CN,
+	 * but in this case, our parents wont increase their backlogs.
+	 */
+	prev_qlen -= sch->q.qlen;
+	prev_backlog -= sch->qstats.backlog;
+	if (ret == idx) {
+		qdisc_tree_reduce_backlog(sch, prev_qlen - 1,
+					  prev_backlog - qdisc_pkt_len(skb));
+		return NET_XMIT_CN;
+	}
+	qdisc_tree_reduce_backlog(sch, prev_qlen, prev_backlog);
+	return NET_XMIT_SUCCESS;
 }
 
 /* This is the specific function called from codel_dequeue()

Re: [net] fq_codel: fix NET_XMIT_CN behavior

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> My prior attempt to fix the backlogs of parents failed.
> 
> If we return NET_XMIT_CN, our parents wont increase their backlog,
> so our qdisc_tree_reduce_backlog() should take this into account.

[..]

> diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
> index 6883a8971562..c57ec480a2da 100644
> --- a/net/sched/sch_fq_codel.c
> +++ b/net/sched/sch_fq_codel.c
> @@ -240,11 +240,19 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>  	q->drop_overlimit += prev_qlen - sch->q.qlen;
>  	if (memory_limited)
>  		q->drop_overmemory += prev_qlen - sch->q.qlen;
> -	/* As we dropped packet(s), better let upper stack know this */
> -	qdisc_tree_reduce_backlog(sch, prev_qlen - sch->q.qlen,
> -				  prev_backlog - sch->qstats.backlog);
> -
> -	return ret == idx ? NET_XMIT_CN : NET_XMIT_SUCCESS;
> +	/* As we dropped packet(s), better let upper stack know this.
> +	 * If we dropped a packet for this flow, return NET_XMIT_CN,
> +	 * but in this case, our parents wont increase their backlogs.
> +	 */
> +	prev_qlen -= sch->q.qlen;
> +	prev_backlog -= sch->qstats.backlog;
> +	if (ret == idx) {
> +		qdisc_tree_reduce_backlog(sch, prev_qlen - 1,
> +					  prev_backlog - qdisc_pkt_len(skb));
> +		return NET_XMIT_CN;

Is skb still valid here? AFAICS its possible that fq_codel_drop() drops it.

Other than that this looks good, thanks Eric!

Re: [net] fq_codel: fix NET_XMIT_CN behavior

[Eric Dumazet]

On Sat, 2016-06-04 at 21:03 +0200, Florian Westphal wrote:

> Is skb still valid here? AFAICS its possible that fq_codel_drop() drops it.
> 
> Other than that this looks good, thanks Eric!

You are are right. skb is guaranteed to be valid only when we return
NET_XMIT_SUCCESS.

I'll send a V2, thanks !

[PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

My prior attempt to fix the backlogs of parents failed.

If we return NET_XMIT_CN, our parents wont increase their backlog,
so our qdisc_tree_reduce_backlog() should take this into account.

v2: Florian Westphal pointed out that we could drop the packet,
so we need to save qdisc_pkt_len(skb) in a temp variable before
calling fq_codel_drop()

Fixes: 9d18562a2278 ("fq_codel: add batch ability to fq_codel_drop()")
Fixes: 2ccccf5fb43f ("net_sched: update hierarchical backlog too")
Reported-by: Stas Nichiporovich <stasn77@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: WANG Cong <xiyou.wangcong@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_fq_codel.c |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 6883a8971562..fff7867f4a4f 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -199,6 +199,7 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	unsigned int idx, prev_backlog, prev_qlen;
 	struct fq_codel_flow *flow;
 	int uninitialized_var(ret);
+	unsigned int pkt_len;
 	bool memory_limited;
 
 	idx = fq_codel_classify(skb, sch, &ret);
@@ -230,6 +231,8 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	prev_backlog = sch->qstats.backlog;
 	prev_qlen = sch->q.qlen;
 
+	/* save this packet length as it might be dropped by fq_codel_drop() */
+	pkt_len = qdisc_pkt_len(skb);
 	/* fq_codel_drop() is quite expensive, as it performs a linear search
 	 * in q->backlogs[] to find a fat flow.
 	 * So instead of dropping a single packet, drop half of its backlog
@@ -237,14 +240,23 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	 */
 	ret = fq_codel_drop(sch, q->drop_batch_size);
 
-	q->drop_overlimit += prev_qlen - sch->q.qlen;
+	prev_qlen -= sch->q.qlen;
+	prev_backlog -= sch->qstats.backlog;
+	q->drop_overlimit += prev_qlen;
 	if (memory_limited)
-		q->drop_overmemory += prev_qlen - sch->q.qlen;
-	/* As we dropped packet(s), better let upper stack know this */
-	qdisc_tree_reduce_backlog(sch, prev_qlen - sch->q.qlen,
-				  prev_backlog - sch->qstats.backlog);
+		q->drop_overmemory += prev_qlen;
 
-	return ret == idx ? NET_XMIT_CN : NET_XMIT_SUCCESS;
+	/* As we dropped packet(s), better let upper stack know this.
+	 * If we dropped a packet for this flow, return NET_XMIT_CN,
+	 * but in this case, our parents wont increase their backlogs.
+	 */
+	if (ret == idx) {
+		qdisc_tree_reduce_backlog(sch, prev_qlen - 1,
+					  prev_backlog - pkt_len);
+		return NET_XMIT_CN;
+	}
+	qdisc_tree_reduce_backlog(sch, prev_qlen, prev_backlog);
+	return NET_XMIT_SUCCESS;
 }
 
 /* This is the specific function called from codel_dequeue()

Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[Jamal Hadi Salim]

On 16-06-04 03:55 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> My prior attempt to fix the backlogs of parents failed.
>
> If we return NET_XMIT_CN, our parents wont increase their backlog,
> so our qdisc_tree_reduce_backlog() should take this into account.
>
> v2: Florian Westphal pointed out that we could drop the packet,
> so we need to save qdisc_pkt_len(skb) in a temp variable before
> calling fq_codel_drop()
>
> Fixes: 9d18562a2278 ("fq_codel: add batch ability to fq_codel_drop()")
> Fixes: 2ccccf5fb43f ("net_sched: update hierarchical backlog too")
> Reported-by: Stas Nichiporovich <stasn77@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: WANG Cong <xiyou.wangcong@gmail.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[Cong Wang]

On Sat, Jun 4, 2016 at 12:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> My prior attempt to fix the backlogs of parents failed.
>
> If we return NET_XMIT_CN, our parents wont increase their backlog,
> so our qdisc_tree_reduce_backlog() should take this into account.
>

Acked-by: Cong Wang <xiyou.wangcong@gmail.com>

Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[Eric Dumazet]

On Sun, 2016-06-05 at 13:30 -0700, Cong Wang wrote:
> On Sat, Jun 4, 2016 at 12:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > My prior attempt to fix the backlogs of parents failed.
> >
> > If we return NET_XMIT_CN, our parents wont increase their backlog,
> > so our qdisc_tree_reduce_backlog() should take this into account.
> >
> 
> Acked-by: Cong Wang <xiyou.wangcong@gmail.com>

BTW, we might need a similar fix in sch_fq.c

Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[Eric Dumazet]

On Sun, 2016-06-05 at 13:54 -0700, Eric Dumazet wrote:

> BTW, we might need a similar fix in sch_fq.c

I meant sch_sfq.c

Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[Cong Wang]

On Sun, Jun 5, 2016 at 1:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2016-06-05 at 13:54 -0700, Eric Dumazet wrote:
>
>> BTW, we might need a similar fix in sch_fq.c
>
> I meant sch_sfq.c
>

Potentially all of the following:

net/sched/sch_choke.c:  return NET_XMIT_CN;
net/sched/sch_fifo.c:   return NET_XMIT_CN;
net/sched/sch_generic.c:        return NET_XMIT_CN;
net/sched/sch_gred.c:   return NET_XMIT_CN;
net/sched/sch_hhf.c:            return NET_XMIT_CN;
net/sched/sch_red.c:    return NET_XMIT_CN;
net/sched/sch_sfb.c:    return NET_XMIT_CN;
net/sched/sch_sfq.c:            return NET_XMIT_CN;

Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[Jamal Hadi Salim]

On 16-06-05 07:30 PM, Cong Wang wrote:

> Potentially all of the following:
>
> net/sched/sch_choke.c:  return NET_XMIT_CN;
> net/sched/sch_fifo.c:   return NET_XMIT_CN;
> net/sched/sch_generic.c:        return NET_XMIT_CN;
> net/sched/sch_gred.c:   return NET_XMIT_CN;
> net/sched/sch_hhf.c:            return NET_XMIT_CN;
> net/sched/sch_red.c:    return NET_XMIT_CN;
> net/sched/sch_sfb.c:    return NET_XMIT_CN;
> net/sched/sch_sfq.c:            return NET_XMIT_CN;
>

As long as we are not loosing the stat that the packet
is dropped. Some qdiscs have a counter which indicates
congestion drops(look at RED variants of early drops);
maybe codel needs one. The parent also must account for
childs drops.
BTW, returning NET_XMIT_CN could be confusing to tcp;
it does not mean that the packet that we are getting return
code for was dropped; it could mean _another_ packet in
the queue was dropped.

cheers,
jamal

Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[Florian Westphal]

Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> BTW, returning NET_XMIT_CN could be confusing to tcp;
> it does not mean that the packet that we are getting return
> code for was dropped; it could mean _another_ packet in
> the queue was dropped.

Yes, but we currently conceal NET_XMIT_CN from upper layer (tcp)
via the net_xmit_* macros:

#define net_xmit_eval(e)       ((e) == NET_XMIT_CN ? 0 : (e))
#define net_xmit_errno(e)      ((e) != NET_XMIT_CN ? -ENOBUFS : 0)

Might be worth changing this so tcp reduces cwnd in _CN case too.

Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[Eric Dumazet]

On Mon, 2016-06-06 at 13:42 +0200, Florian Westphal wrote:
> Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > BTW, returning NET_XMIT_CN could be confusing to tcp;
> > it does not mean that the packet that we are getting return
> > code for was dropped; it could mean _another_ packet in
> > the queue was dropped.
> 
> Yes, but we currently conceal NET_XMIT_CN from upper layer (tcp)
> via the net_xmit_* macros:
> 
> #define net_xmit_eval(e)       ((e) == NET_XMIT_CN ? 0 : (e))
> #define net_xmit_errno(e)      ((e) != NET_XMIT_CN ? -ENOBUFS : 0)
> 
> Might be worth changing this so tcp reduces cwnd in _CN case too.

It always had been the case.

tcp_transmit_skb() calls tcp_enter_cwr(), unless someone broke this.

Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2016-06-06 at 13:42 +0200, Florian Westphal wrote:
> > Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > > BTW, returning NET_XMIT_CN could be confusing to tcp;
> > > it does not mean that the packet that we are getting return
> > > code for was dropped; it could mean _another_ packet in
> > > the queue was dropped.
> > 
> > Yes, but we currently conceal NET_XMIT_CN from upper layer (tcp)
> > via the net_xmit_* macros:
> > 
> > #define net_xmit_eval(e)       ((e) == NET_XMIT_CN ? 0 : (e))
> > #define net_xmit_errno(e)      ((e) != NET_XMIT_CN ? -ENOBUFS : 0)
> > 
> > Might be worth changing this so tcp reduces cwnd in _CN case too.
> 
> It always had been the case.
> 
> tcp_transmit_skb() calls tcp_enter_cwr(), unless someone broke this.

No, you're right.  We hide error in ip_send_skb but that function is
not used in tcp case.

So all is good here.

Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[Jamal Hadi Salim]

On 16-06-06 12:18 PM, Florian Westphal wrote:
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Mon, 2016-06-06 at 13:42 +0200, Florian Westphal wrote:
>>> Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>> BTW, returning NET_XMIT_CN could be confusing to tcp;
>>>> it does not mean that the packet that we are getting return
>>>> code for was dropped; it could mean _another_ packet in
>>>> the queue was dropped.
>>>
>>> Yes, but we currently conceal NET_XMIT_CN from upper layer (tcp)
>>> via the net_xmit_* macros:
>>>
>>> #define net_xmit_eval(e)       ((e) == NET_XMIT_CN ? 0 : (e))
>>> #define net_xmit_errno(e)      ((e) != NET_XMIT_CN ? -ENOBUFS : 0)
>>>
>>> Might be worth changing this so tcp reduces cwnd in _CN case too.
>>
>> It always had been the case.
>>
>> tcp_transmit_skb() calls tcp_enter_cwr(), unless someone broke this.
>
> No, you're right.  We hide error in ip_send_skb but that function is
> not used in tcp case.
>
> So all is good here.
>

Just making sure because that little gem is not obvious;->

Eric, on the NET_XMIT_CN digression:
depends on the qdisc scheduler.
NET_XMIT_CN could mean one of two things:
"There is congestion. The packet you sent was dropped locally" and
"There is congestion. The packet you sent was not dropped locally"
Yes, they both react with tcp_enter_cwr() and from a macroscopic
view it probably doesnt matter as much because tcp state machine will
kick in at some point. I think better use could be made of such
knowledge.

cheers,
jamal

Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[Eric Dumazet]

On Mon, 2016-06-06 at 06:49 -0400, Jamal Hadi Salim wrote:
> On 16-06-05 07:30 PM, Cong Wang wrote:
> 
> > Potentially all of the following:
> >
> > net/sched/sch_choke.c:  return NET_XMIT_CN;
> > net/sched/sch_fifo.c:   return NET_XMIT_CN;
> > net/sched/sch_generic.c:        return NET_XMIT_CN;
> > net/sched/sch_gred.c:   return NET_XMIT_CN;
> > net/sched/sch_hhf.c:            return NET_XMIT_CN;
> > net/sched/sch_red.c:    return NET_XMIT_CN;
> > net/sched/sch_sfb.c:    return NET_XMIT_CN;
> > net/sched/sch_sfq.c:            return NET_XMIT_CN;
> >
> 
> As long as we are not loosing the stat that the packet
> is dropped. Some qdiscs have a counter which indicates
> congestion drops(look at RED variants of early drops);
> maybe codel needs one. The parent also must account for
> childs drops.
> BTW, returning NET_XMIT_CN could be confusing to tcp;
> it does not mean that the packet that we are getting return
> code for was dropped; it could mean _another_ packet in
> the queue was dropped.

NET_XMIT_CN does not confuse TCP at all.

Look, the issue here is the accuracy of byte backlog in upper qdiscs,
after Cong patch.

When at enqueue(sch, P2) we drop P1, but enqueue P2, we need to take
care of reporting to upper qdisc qdisc_pkt_len(P2) - qdisc_pkt_len(P1)

We used to not call qdisc_tree_reduce_backlog() in this case, since it
was qdisc_tree_decrease_qlen(sch, delta_packets) before Cong patch.

We now need to call qdisc_tree_reduce_backlog(sch, 0, delta_bytes)

Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 04 Jun 2016 12:55:13 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> My prior attempt to fix the backlogs of parents failed.
> 
> If we return NET_XMIT_CN, our parents wont increase their backlog,
> so our qdisc_tree_reduce_backlog() should take this into account.
> 
> v2: Florian Westphal pointed out that we could drop the packet,
> so we need to save qdisc_pkt_len(skb) in a temp variable before
> calling fq_codel_drop()
> 
> Fixes: 9d18562a2278 ("fq_codel: add batch ability to fq_codel_drop()")
> Fixes: 2ccccf5fb43f ("net_sched: update hierarchical backlog too")
> Reported-by: Stas Nichiporovich <stasn77@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.