[PATCH net-next v2] net_sched: increase drop count when packets are dropped

[PATCH net-next v2] net_sched: increase drop count when packets are dropped

[Yang Yingliang]

When we change limit in qdisc, if there're too many packets in the queue,
some packets will be dropped but the drop count is not increased.

E.g.:
 One terminal run the following command:
 # tc qdisc add dev eth4 root handle 1: htb default 1
 # tc class add dev eth4 parent 1:0 classid 1:1 htb rate 800mbit ceil 800mbit
 # tc qdisc add dev eth4 parent 1:1 handle 11: fq
 Then use iperf to send packets
 
 Another run the following scripts:
 #!/bin/sh
 int=1
 while(( $int<=5 ))
 do
 tc qdisc replace dev eth4 parent 1:1 handle 11: fq limit 10
 tc qdisc replace dev eth4 parent 1:1 handle 11: fq limit 100000
 done

 Then show the qdisc :
 # tc -s -d qdisc show dev eth4
qdisc htb 1: root refcnt 2 r2q 10 default 1 direct_packets_stat 0 ver 3.17 direct_qlen 1000
 Sent 1770938310 bytes 1175683 pkt (_dropped_ 447, overlimits 12423 requeues 0) 
 backlog 0b 0p requeues 0 
qdisc fq 11: parent 1:1 limit 100000p flow_limit 100p buckets 1024 quantum 3028 initial_quantum 15140 
 Sent 1770970104 bytes 1175704 pkt (_dropped_ 426, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0 
  4 flows (3 inactive, 0 throttled)
  0 gc, 0 highprio, 6748 throttled, 162 flows_plimit

The drop count of parent and leaf is not equal, becasue it only increase
the drop count of parent(htb) but not leaf(fq) in fq_change().

With this patch, they will be equal :
 # tc -s -d qdisc show dev eth4
qdisc htb 1: root refcnt 2 r2q 10 default 1 direct_packets_stat 0 ver 3.17 direct_qlen 1000
 Sent 977241352 bytes 648678 pkt (_dropped_ 512, overlimits 14209 requeues 0) 
 backlog 0b 0p requeues 0 
qdisc fq 11: parent 1:1 limit 100000p flow_limit 100p buckets 1024 quantum 3028 initial_quantum 15140 
 Sent 977274660 bytes 648700 pkt (_dropped_ 512, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0 
  5 flows (2 inactive, 0 throttled)
  0 gc, 0 highprio, 7550 throttled, 58 flows_plimit

So replace kfree_skb() with qdisc_drop() which will increase the
drop count. hhf and fq_codel have the same problem, fix them too.
Besides, fq_codel and hhf have a member drop_overlimit which means
drop count because of overlimit, increase it too.

Cc: Eric Dumazet <edumazet@google.com>
Cc: Terry Lam <vtlam@google.com>
Cc: Nandita Dukkipati <nanditad@google.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>

---
Change note:

Modify the changelog and add the description of test.
---
 net/sched/sch_fq.c       | 2 +-
 net/sched/sch_fq_codel.c | 3 ++-
 net/sched/sch_hhf.c      | 3 ++-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index ba648e189f2e..1e350406aa53 100644
--- a/net/sched/sch_fq.c
+++ b/net/sched/sch_fq.c
@@ -756,7 +756,7 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt)
 
 		if (!skb)
 			break;
-		kfree_skb(skb);
+		qdisc_drop(skb, sch);
 		drop_count++;
 	}
 	qdisc_tree_decrease_qlen(sch, drop_count);
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 0bf432c782c1..bcfe4594470f 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -344,7 +344,8 @@ static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt)
 	while (sch->q.qlen > sch->limit) {
 		struct sk_buff *skb = fq_codel_dequeue(sch);
 
-		kfree_skb(skb);
+		qdisc_drop(skb, sch);
+		q->drop_overlimit++;
 		q->cstats.drop_count++;
 	}
 	qdisc_tree_decrease_qlen(sch, q->cstats.drop_count);
diff --git a/net/sched/sch_hhf.c b/net/sched/sch_hhf.c
index 6aab8619bbb0..71542d2cecac 100644
--- a/net/sched/sch_hhf.c
+++ b/net/sched/sch_hhf.c
@@ -593,7 +593,8 @@ static int hhf_change(struct Qdisc *sch, struct nlattr *opt)
 	while (sch->q.qlen > sch->limit) {
 		struct sk_buff *skb = hhf_dequeue(sch);
 
-		kfree_skb(skb);
+		qdisc_drop(skb, sch);
+		q->drop_overlimit++;
 	}
 	qdisc_tree_decrease_qlen(sch, qlen - sch->q.qlen);
 
-- 
1.8.0

Re: [PATCH net-next v2] net_sched: increase drop count when packets are dropped

[Eric Dumazet]

On Mon, 2014-05-26 at 10:22 +0800, Yang Yingliang wrote:
> When we change limit in qdisc, if there're too many packets in the queue,
> some packets will be dropped but the drop count is not increased.

> So replace kfree_skb() with qdisc_drop() which will increase the
> drop count. hhf and fq_codel have the same problem, fix them too.
> Besides, fq_codel and hhf have a member drop_overlimit which means
> drop count because of overlimit, increase it too.
> 
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Terry Lam <vtlam@google.com>
> Cc: Nandita Dukkipati <nanditad@google.com>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> 
> ---

Problem with this patch is that you missed that all these qdiscs use a
regular dequeue() operation in this path.

So every in excess packet is going to be accounted twice after your
patch.

Once as 'normally dequeued', another time as 'dropped'

Your patch shifts a problem into another.

The example in your changelog shows this new problem.

Really I do not feel its needed to spend hours on work on this minor
problem : We would have to add parameters (slowing down the fast path),
or copy/paste the regular dequeue code (code bloat and maintenance
hassle)

So I wont Ack this patch, sorry.