Skip to contentSkip to conversation

: fix queue limits in multiple qdiscs

PATCH In review Patrick McHardy 3 participants 5 messages Nov 13, 2003 – Nov 19, 2003

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 : fix queue limits in multiple qdiscs

This patch fixes multiple qdiscs exceeding their limits:

pfifo/pfifo_fast: by one packet
bfifo/red/gred: by one full-sized packet in bytes

RED and GRED should never reach their limits so this part is more cosmetic than fix.

Best regards,
Patrick

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1431 -> 1.1432
# net/sched/sch_generic.c 1.9 -> 1.10

#	net/sched/sch_teql.c	1.8     -> 1.9    
#	net/sched/sch_fifo.c	1.6     -> 1.7    
#	net/sched/sch_gred.c	1.12    -> 1.13   
#	 net/sched/sch_red.c	1.8     -> 1.9    

#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/11/12 [email protected] 1.1432
# Fix queue limits in multiple qdiscs
# --------------------------------------------
#

net/sched/sch_fifo.c +2 -2
diff -Nru a/net/sched/sch_fifo.c b/net/sched/sch_fifo.c
@@ -47,7 +47,7 @@
 {
 	struct fifo_sched_data *q = (struct fifo_sched_data *)sch->data;
 
-	if (sch->stats.backlog <= q->limit) {
+	if (sch->stats.backlog + skb->len <= q->limit) {
 		__skb_queue_tail(&sch->q, skb);
 		sch->stats.backlog += skb->len;
 		sch->stats.bytes += skb->len;
@@ -108,7 +108,7 @@
 {
 	struct fifo_sched_data *q = (struct fifo_sched_data *)sch->data;
 
-	if (sch->q.qlen <= q->limit) {
+	if (sch->q.qlen < q->limit) {
 		__skb_queue_tail(&sch->q, skb);
 		sch->stats.bytes += skb->len;
 		sch->stats.packets++;
net/sched/sch_generic.c +1 -1
diff -Nru a/net/sched/sch_generic.c b/net/sched/sch_generic.c
@@ -275,7 +275,7 @@
 	list = ((struct sk_buff_head*)qdisc->data) +
 		prio2band[skb->priority&TC_PRIO_MAX];
 
-	if (list->qlen <= qdisc->dev->tx_queue_len) {
+	if (list->qlen < qdisc->dev->tx_queue_len) {
 		__skb_queue_tail(list, skb);
 		qdisc->q.qlen++;
 		qdisc->stats.bytes += skb->len;
net/sched/sch_gred.c +2 -2
diff -Nru a/net/sched/sch_gred.c b/net/sched/sch_gred.c
@@ -110,7 +110,7 @@
 	unsigned long	qave=0;	
 	int i=0;
 
-	if (!t->initd && skb_queue_len(&sch->q) <= sch->dev->tx_queue_len) {
+	if (!t->initd && skb_queue_len(&sch->q) < sch->dev->tx_queue_len) {
 		D2PRINTK("NO GRED Queues setup yet! Enqueued anyway\n");
 		goto do_enqueue;
 	}
@@ -175,7 +175,7 @@
 	if ((q->qave+qave) < q->qth_min) {
 		q->qcount = -1;
 enqueue:
-		if (q->backlog <= q->limit) {
+		if (q->backlog + skb->len <= q->limit) {
 			q->backlog += skb->len;
 do_enqueue:
 			__skb_queue_tail(&sch->q, skb);
net/sched/sch_red.c +1 -1
diff -Nru a/net/sched/sch_red.c b/net/sched/sch_red.c
@@ -257,7 +257,7 @@
 	if (q->qave < q->qth_min) {
 		q->qcount = -1;
 enqueue:
-		if (sch->stats.backlog <= q->limit) {
+		if (sch->stats.backlog + skb->len <= q->limit) {
 			__skb_queue_tail(&sch->q, skb);
 			sch->stats.backlog += skb->len;
 			sch->stats.bytes += skb->len;
#1 lore raw

valid but Too cosmetic, IMO; (who cares when you are off by a few bytes?)

cheers,
jamal

Quoted 97 lines · Patrick McHardy

This patch fixes multiple qdiscs exceeding their limits:

pfifo/pfifo_fast: by one packet
bfifo/red/gred: by one full-sized packet in bytes

RED and GRED should never reach their limits so this part is more cosmetic than fix.

Best regards,
Patrick

______________________________________________________________________

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1431 -> 1.1432
# net/sched/sch_generic.c 1.9 -> 1.10

#	net/sched/sch_teql.c	1.8     -> 1.9    
#	net/sched/sch_fifo.c	1.6     -> 1.7    
#	net/sched/sch_gred.c	1.12    -> 1.13   
#	 net/sched/sch_red.c	1.8     -> 1.9    

#
# The following is the BitKeeper ChangeSet Log # --------------------------------------------
# 03/11/12 [email protected] 1.1432
# Fix queue limits in multiple qdiscs
# -------------------------------------------- #
diff -Nru a/net/sched/sch_fifo.c b/net/sched/sch_fifo.c

net/sched/sch_fifo.c +2 -2
@@ -47,7 +47,7 @@
 {
 	struct fifo_sched_data *q = (struct fifo_sched_data *)sch->data;
 
-	if (sch->stats.backlog <= q->limit) {
+	if (sch->stats.backlog + skb->len <= q->limit) {
 		__skb_queue_tail(&sch->q, skb);
 		sch->stats.backlog += skb->len;
 		sch->stats.bytes += skb->len;
@@ -108,7 +108,7 @@
 {
 	struct fifo_sched_data *q = (struct fifo_sched_data *)sch->data;
 
-	if (sch->q.qlen <= q->limit) {
+	if (sch->q.qlen < q->limit) {
 		__skb_queue_tail(&sch->q, skb);
 		sch->stats.bytes += skb->len;
 		sch->stats.packets++;

diff -Nru a/net/sched/sch_generic.c b/net/sched/sch_generic.c

net/sched/sch_generic.c +1 -1
@@ -275,7 +275,7 @@
 	list = ((struct sk_buff_head*)qdisc->data) +
 		prio2band[skb->priority&TC_PRIO_MAX];
 
-	if (list->qlen <= qdisc->dev->tx_queue_len) {
+	if (list->qlen < qdisc->dev->tx_queue_len) {
 		__skb_queue_tail(list, skb);
 		qdisc->q.qlen++;
 		qdisc->stats.bytes += skb->len;

diff -Nru a/net/sched/sch_gred.c b/net/sched/sch_gred.c

net/sched/sch_gred.c +2 -2
@@ -110,7 +110,7 @@
 	unsigned long	qave=0;	
 	int i=0;
 
-	if (!t->initd && skb_queue_len(&sch->q) <= sch->dev->tx_queue_len) {
+	if (!t->initd && skb_queue_len(&sch->q) < sch->dev->tx_queue_len) {
 		D2PRINTK("NO GRED Queues setup yet! Enqueued anyway\n");
 		goto do_enqueue;
 	}
@@ -175,7 +175,7 @@
 	if ((q->qave+qave) < q->qth_min) {
 		q->qcount = -1;
 enqueue:
-		if (q->backlog <= q->limit) {
+		if (q->backlog + skb->len <= q->limit) {
 			q->backlog += skb->len;
 do_enqueue:
 			__skb_queue_tail(&sch->q, skb);

diff -Nru a/net/sched/sch_red.c b/net/sched/sch_red.c

net/sched/sch_red.c +1 -1
@@ -257,7 +257,7 @@
 	if (q->qave < q->qth_min) {
 		q->qcount = -1;
 enqueue:
-		if (sch->stats.backlog <= q->limit) {
+		if (sch->stats.backlog + skb->len <= q->limit) {
 			__skb_queue_tail(&sch->q, skb);
 			sch->stats.backlog += skb->len;
 			sch->stats.bytes += skb->len;
#2 lore raw

I don't have strong feelings about this, I just kept wondering why tc displayes "pfifo limit 10 backlog 11".

Best regards,
Patrick

Quoted 6 lines · jamal

valid but Too cosmetic, IMO; (who cares when you are off by a few bytes?)

cheers,
jamal

#3 lore raw

Like i said it is valid - but i wouldnt go out of my way to fix it (especially when 2.6 is frozen).

cheers,
jamal

Quoted 17 lines · Patrick McHardy

I don't have strong feelings about this, I just kept wondering why tc displayes "pfifo limit 10 backlog 11".

Best regards,
Patrick

Quoted 6 lines · jamal

valid but Too cosmetic, IMO; (who cares when you are off by a few bytes?)

cheers,
jamal

#4 lore raw
Quoted 2 lines · On 17 Nov 2003 10:01:24 -0500 jamal <[email protected]> wrote:

Like i said it is valid - but i wouldnt go out of my way to fix it (especially when 2.6 is frozen).

I agree with this sentiment, but only in part.

Considering:

1) The output from 'tc' is very confusing and quite stupid.
2) Patrick did all the work to fix this bug.
3) His patch is %100 straight forward, simply, and easy to verify.

I am going to apply his patch.

Thanks Patrick.