Skip to contentSkip to conversation

sched: Fix resource limiting in pfifo_fast

RFC PATCH In review Krishna Kumar 4 participants 7 messages Aug 30, 2009 – Aug 31, 2009

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Krishna Kumar

author
#0 lore raw

RFC PATCH 1/1 sched: Fix resource limiting in pfifo_fast

From: Krishna Kumar <[email protected]>

pfifo_fast_enqueue has this check:

        if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len) {

which allows each band to enqueue upto tx_queue_len skbs for a total of 3*tx_queue_len skbs. I am not sure if this was the intention of limiting in qdisc.

Patch compiled and 32 simultaneous netperf testing ran fine. Also:

# tc -s qdisc show dev eth2
qdisc pfifo_fast 0: root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 16835026752 bytes 373116 pkt (dropped 0, overlimits 0 requeues 25) 
 rate 0bit 0pps backlog 0b 0p requeues 25

(I am taking next week off, so sorry for any delay in responding)

Signed-off-by Krishna Kumar <[email protected]>

---

 net/sched/sch_generic.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
net/sched/sch_generic.c +4 -4
diff -ruNp org/net/sched/sch_generic.c new/net/sched/sch_generic.c
@@ -432,11 +432,11 @@ static inline struct sk_buff_head *band2
 
 static int pfifo_fast_enqueue(struct sk_buff *skb, struct Qdisc* qdisc)
 {
-	int band = prio2band[skb->priority & TC_PRIO_MAX];
-	struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
-	struct sk_buff_head *list = band2list(priv, band);
+	if (skb_queue_len(&qdisc->q) < qdisc_dev(qdisc)->tx_queue_len) {
+		int band = prio2band[skb->priority & TC_PRIO_MAX];
+		struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
+		struct sk_buff_head *list = band2list(priv, band);
 
-	if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len) {
 		priv->bitmap |= (1 << band);
 		qdisc->q.qlen++;
 		return __qdisc_enqueue_tail(skb, qdisc, list);
#1 lore raw
Quoted 8 lines · Krishna Kumar

From: Krishna Kumar <[email protected]>

pfifo_fast_enqueue has this check:

        if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len) {

which allows each band to enqueue upto tx_queue_len skbs for a total of 3*tx_queue_len skbs. I am not sure if this was the intention of limiting in qdisc.

Yes I noticed that and said to myself :
This was to let high priority packets have their own limit, independent on fact that low priority packets filled their queue.

Quoted 34 lines

Patch compiled and 32 simultaneous netperf testing ran fine. Also:

# tc -s qdisc show dev eth2
qdisc pfifo_fast 0: root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 16835026752 bytes 373116 pkt (dropped 0, overlimits 0 requeues 25) 
 rate 0bit 0pps backlog 0b 0p requeues 25

(I am taking next week off, so sorry for any delay in responding)

Signed-off-by Krishna Kumar <[email protected]>

---

 net/sched/sch_generic.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff -ruNp org/net/sched/sch_generic.c new/net/sched/sch_generic.c

net/sched/sch_generic.c +4 -4
@@ -432,11 +432,11 @@ static inline struct sk_buff_head *band2
 
 static int pfifo_fast_enqueue(struct sk_buff *skb, struct Qdisc* qdisc)
 {
-	int band = prio2band[skb->priority & TC_PRIO_MAX];
-	struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
-	struct sk_buff_head *list = band2list(priv, band);
+	if (skb_queue_len(&qdisc->q) < qdisc_dev(qdisc)->tx_queue_len) {
+		int band = prio2band[skb->priority & TC_PRIO_MAX];
+		struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
+		struct sk_buff_head *list = band2list(priv, band);
 
-	if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len) {
 		priv->bitmap |= (1 << band);
 		qdisc->q.qlen++;
 		return __qdisc_enqueue_tail(skb, qdisc, list);
#2 lore raw

Eric Dumazet wrote, On 08/30/2009 08:54 AM:

Quoted 6 lines
Quoted 5 lines · Krishna Kumar

From: Krishna Kumar <[email protected]>

pfifo_fast_enqueue has this check:

        if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len) {
Quoted 7 lines
Quoted 3 lines

which allows each band to enqueue upto tx_queue_len skbs for a total of 3*tx_queue_len skbs. I am not sure if this was the intention of limiting in qdisc.

Yes I noticed that and said to myself :
This was to let high priority packets have their own limit, independent on fact that low priority packets filled their queue.

Good point, but then logically it could be something like:
if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len / 3)

Of course, there is a backward compatibility question, plus an sch_prio consistency question.

Jarek P.

author
#5 lore raw
In reply to Eric Dumazet

I had thought of this reason before submitting. But I felt that if we are filling up the qdisc due to some problem at driver/device, then the issue should be handled at a different level (increase tx_queue_len, let packets drop and TCP handle it, etc).

So I am not sure if it is intentionally designed this way, or whether it needs fixing to reflect a maximum limit.

Thanks,

- KK

Quoted 9 lines

Eric Dumazet <[email protected]>
Re: [RFC PATCH] sched: Fix resource limiting in pfifo_fast

Quoted 5 lines · Krishna Kumar

From: Krishna Kumar <[email protected]>

pfifo_fast_enqueue has this check:

        if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len) {
Quoted 12 lines
Quoted 3 lines

which allows each band to enqueue upto tx_queue_len skbs for a total of 3*tx_queue_len skbs. I am not sure if this was the intention of limiting in qdisc.

Yes I noticed that and said to myself :
This was to let high priority packets have their own limit, independent on fact that low priority packets filled their queue.

Quoted 4 lines

Patch compiled and 32 simultaneous netperf testing ran fine. Also:

# tc -s qdisc show dev eth2
qdisc pfifo_fast 0: root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1

1

Quoted 1 line
Quoted 1 line

Sent 16835026752 bytes 373116 pkt (dropped 0, overlimits 0 requeues

25)

Quoted 10 lines
Quoted 10 lines

rate 0bit 0pps backlog 0b 0p requeues 25

(I am taking next week off, so sorry for any delay in responding)

Signed-off-by Krishna Kumar <[email protected]>

---

 net/sched/sch_generic.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Quoted 6 lines

net/sched/sch_generic.c 2009-08-30 11:21:50.000000000 +0530 static inline struct sk_buff_head *band2 · line 433

Quoted 6 lines

diff -ruNp org/net/sched/sch_generic.c new/net/sched/sch_generic.c

net/sched/sch_generic.c 2009-08-30 11:21:50.000000000 +0530 renamed from net/sched/sch_generic.c 2009-08-30 11:18:23.000000000 +0530 +0 -0
@@ -432,11 +432,11 @@ static inline struct sk_buff_head *band2
 
 static int pfifo_fast_enqueue(struct sk_buff *skb, struct Qdisc*

qdisc)

Quoted 18 lines

net/sched/sch_generic.c 2009-08-30 11:21:50.000000000 +0530 static inline struct sk_buff_head *band2 · line 442

Quoted 13 lines
net/sched/sch_generic.c 2009-08-30 11:21:50.000000000 +0530 renamed from net/sched/sch_generic.c 2009-08-30 11:18:23.000000000 +0530 +4 -4
 {
-   int band = prio2band[skb->priority & TC_PRIO_MAX];
-   struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
-   struct sk_buff_head *list = band2list(priv, band);
+   if (skb_queue_len(&qdisc->q) < qdisc_dev(qdisc)->tx_queue_len) {
+      int band = prio2band[skb->priority & TC_PRIO_MAX];
+      struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
+      struct sk_buff_head *list = band2list(priv, band);
 
-   if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len) {
       priv->bitmap |= (1 << band);
       qdisc->q.qlen++;
       return __qdisc_enqueue_tail(skb, qdisc, list);

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#3 lore raw
In reply to Jarek Poplawski
Quoted 13 lines

Jarek Poplawski <[email protected]>

Quoted 10 lines
Quoted 6 lines

pfifo_fast_enqueue has this check:

        if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len) {

which allows each band to enqueue upto tx_queue_len skbs for a total of 3*tx_queue_len skbs. I am not sure if this was the intention of limiting in qdisc.

Yes I noticed that and said to myself :
This was to let high priority packets have their own limit, independent on fact that low priority packets filled their queue.

Quoted 5 lines

Good point, but then logically it could be something like:
if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len / 3)

Of course, there is a backward compatibility question, plus an sch_prio consistency question.

Jarek, what is the consistency problem?

Thanks,

- KK

#4 lore raw
Quoted 13 lines · On Sun, Aug 30, 2009 at 02:16:13PM +0530, Krishna Kumar2 wrote:
Quoted 13 lines

Jarek Poplawski <[email protected]>

Quoted 10 lines
Quoted 6 lines

pfifo_fast_enqueue has this check:

        if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len) {

which allows each band to enqueue upto tx_queue_len skbs for a total of 3*tx_queue_len skbs. I am not sure if this was the intention of limiting in qdisc.

Yes I noticed that and said to myself :
This was to let high priority packets have their own limit, independent on fact that low priority packets filled their queue.

Quoted 7 lines
Quoted 5 lines

Good point, but then logically it could be something like:
if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len / 3)

Of course, there is a backward compatibility question, plus an sch_prio consistency question.

Jarek, what is the consistency problem?

Currently pfifo_fast and sch_prio behave similarly wrt. tx_queue_len, don't they?

Jarek P.

maintainer
#6 lore raw
In reply to Krishna Kumar

From: Krishna Kumar <[email protected]>
Date: Sun, 30 Aug 2009 11:53:44 +0530

Quoted 10 lines

From: Krishna Kumar <[email protected]>

pfifo_fast_enqueue has this check:

        if (skb_queue_len(list) < qdisc_dev(qdisc)->tx_queue_len) {

which allows each band to enqueue upto tx_queue_len skbs for a total of 3*tx_queue_len skbs. I am not sure if this was the intention of limiting in qdisc.

Patch compiled and 32 simultaneous netperf testing ran fine. Also:

Quoted 8 lines
# tc -s qdisc show dev eth2
qdisc pfifo_fast 0: root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 16835026752 bytes 373116 pkt (dropped 0, overlimits 0 requeues 25) 
 rate 0bit 0pps backlog 0b 0p requeues 25

(I am taking next week off, so sorry for any delay in responding)

Signed-off-by Krishna Kumar <[email protected]>

This is probably just a thinko, nice catch.

I think I'll apply this to net-next-2.6, thanks!