[PATCH 2/2] qdisc_restart - couple of optimizations.

[PATCH 2/2] qdisc_restart - couple of optimizations.

[Krishna Kumar]

- netif_queue_stopped need not be called inside qdisc_restart as
  it has been called already in qdisc_run() before the first skb
  is sent, and in __qdisc_run() after each intermediate skb is 
  sent (note : we are the only sender, so the queue cannot get
  stopped while the tx lock was got in the ~LLTX case).

- BUG_ON((int) q->q.qlen < 0) was a relic from old times when -1
  meant more packets are available, and __qdisc_run used to loop
  when qdisc_restart() returned -1. During those days, it was
  necessary to make sure that qlen is never less than zero, since
  __qdisc_run would get into an infinite loop if no packets are on
  the queue and this bug in qdisc was there (and worse - no more
  skbs could ever get queue'd as we hold the queue lock too). With
  Herbert's recent change to return values, this check is not
  required.  Hopefully Herbert can validate this change. If at all
  this is required, it should be added to skb_dequeue (in failure
  case), and not to qdisc_qlen.

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
diff -ruNp org/net/sched/sch_generic.c new/net/sched/sch_generic.c
--- org/net/sched/sch_generic.c 2007-06-11 13:12:11.000000000 +0530
+++ new/net/sched/sch_generic.c 2007-06-11 15:37:48.000000000 +0530
@@ -61,7 +61,6 @@ void qdisc_unlock_tree(struct net_device
 
 static inline int qdisc_qlen(struct Qdisc *q)
 {
-	BUG_ON((int) q->q.qlen < 0);
 	return q->q.qlen;
 }
 
@@ -167,9 +166,7 @@ static inline int qdisc_restart(struct n
 	/* And release queue */
 	spin_unlock(&dev->queue_lock);
 
-	ret = NETDEV_TX_BUSY;
-	if (!netif_queue_stopped(dev))
-		ret = dev_hard_start_xmit(skb, dev);
+	ret = dev_hard_start_xmit(skb, dev);
 
 	if (!lockless)
 		netif_tx_unlock(dev);

RE: [PATCH 2/2] qdisc_restart - couple of optimizations.

[Waskiewicz Jr, Peter P]

> - netif_queue_stopped need not be called inside qdisc_restart as
>   it has been called already in qdisc_run() before the first skb
>   is sent, and in __qdisc_run() after each intermediate skb is
>   sent (note : we are the only sender, so the queue cannot get
>   stopped while the tx lock was got in the ~LLTX case).

I somewhat disagree here.  The underlying driver can conceivably stop
the device queue even if the stack holds the queue lock during an
interrupt to clean Tx descriptors, and it finds it's out of them or
needs to grab the device for whatever reason.  Granted this is a corner
case, and the net effect would be a simple requeue of the skb, but
checking the status of the queue at the last possible moment before
entering the driver could alleviate the requeue in the time between
->dequeue() from the qdisc, and hard_start_xmit() if an event like I
mentioned happened.

I'm ok with it either way, especially since this is a corner case.  But
it does need to be considered that it can happen.

Cheers,

-PJ Waskiewicz

RE: [PATCH 2/2] qdisc_restart - couple of optimizations.

[Krishna Kumar2]

Hi Peter,

Thanks for your feedback.

> > - netif_queue_stopped need not be called inside qdisc_restart as
> >   it has been called already in qdisc_run() before the first skb
> >   is sent, and in __qdisc_run() after each intermediate skb is
> >   sent (note : we are the only sender, so the queue cannot get
> >   stopped while the tx lock was got in the ~LLTX case).
>
> I somewhat disagree here.  The underlying driver can conceivably stop
> the device queue even if the stack holds the queue lock during an
> interrupt to clean Tx descriptors, and it finds it's out of them or
> needs to grab the device for whatever reason.  Granted this is a corner
> case, and the net effect would be a simple requeue of the skb, but
> checking the status of the queue at the last possible moment before
> entering the driver could alleviate the requeue in the time between
> ->dequeue() from the qdisc, and hard_start_xmit() if an event like I
> mentioned happened.

After seeing a few drivers, I understand that the rx intr (to clean
TX descriptors) can only enable the queue and not stop the queue (as
it will normally not queue any packets and only clean up the sent
ones). I don't find any way that the driver can stop the queue once
the top layer determines it is OK to send and is enqueing.

It is a wasted check for almost every packet (and IMHO opinion, for
every packet). And as you said - if a driver were written differently
to stop the queue even in the clean path, then that rare event (should
be very rare as we checked for stop_queue just a few instrutions
earlier) will result in a requeue, but the normal path is not penalized.

Thanks,

- KK

> I'm ok with it either way, especially since this is a corner case.  But
> it does need to be considered that it can happen.

Re: [PATCH 2/2] qdisc_restart - couple of optimizations.

[Herbert Xu]

On Wed, Jun 13, 2007 at 10:51:00AM -0700, Waskiewicz Jr, Peter P wrote:
> 
> I somewhat disagree here.  The underlying driver can conceivably stop
> the device queue even if the stack holds the queue lock during an
> interrupt to clean Tx descriptors, and it finds it's out of them or
> needs to grab the device for whatever reason.  Granted this is a corner
> case, and the net effect would be a simple requeue of the skb, but
> checking the status of the queue at the last possible moment before
> entering the driver could alleviate the requeue in the time between
> ->dequeue() from the qdisc, and hard_start_xmit() if an event like I
> mentioned happened.

IMHO this scenario occurs so infrequently that the check isn't worth it
especially since the driver has to be able to deal with us calling it
after netif_stop_queue() anyway.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PATCH 2/2] qdisc_restart - couple of optimizations.

[Waskiewicz Jr, Peter P]

> IMHO this scenario occurs so infrequently that the check 
> isn't worth it especially since the driver has to be able to 
> deal with us calling it after netif_stop_queue() anyway.

That sounds just fine to me.  Thanks Krishna and Herbert for weighing in
on this.

-PJ Waskiewicz

Re: [PATCH 2/2] qdisc_restart - couple of optimizations.

[Herbert Xu]

On Wed, Jun 13, 2007 at 02:10:49PM +0530, Krishna Kumar wrote:
> 
> - BUG_ON((int) q->q.qlen < 0) was a relic from old times when -1
>   meant more packets are available, and __qdisc_run used to loop
>   when qdisc_restart() returned -1. During those days, it was
>   necessary to make sure that qlen is never less than zero, since
>   __qdisc_run would get into an infinite loop if no packets are on
>   the queue and this bug in qdisc was there (and worse - no more
>   skbs could ever get queue'd as we hold the queue lock too). With
>   Herbert's recent change to return values, this check is not
>   required.  Hopefully Herbert can validate this change. If at all
>   this is required, it should be added to skb_dequeue (in failure
>   case), and not to qdisc_qlen.

Yes I agree that this check is no longer critical.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 2/2 - rev2] qdisc_restart - couple of optimizations.

[Krishna Kumar]

(Same from previous patch, resending for completion)

Changes :

- netif_queue_stopped need not be called inside qdisc_restart as
  it has been called already in qdisc_run() before the first skb
  is sent, and in __qdisc_run() after each intermediate skb is 
  sent (note : we are the only sender, so the queue cannot get
  stopped while the tx lock was got in the ~LLTX case).

- BUG_ON((int) q->q.qlen < 0) was a relic from old times when -1
  meant more packets are available, and __qdisc_run used to loop
  when qdisc_restart() returned -1. During those days, it was
  necessary to make sure that qlen is never less than zero, since
  __qdisc_run would get into an infinite loop if no packets are on
  the queue and this bug in qdisc was there (and worse - no more
  skbs could ever get queue'd as we hold the queue lock too). With
  Herbert's recent change to return values, this check is not
  required.  Hopefully Herbert can validate this change. If at all
  this is required, it should be added to skb_dequeue (in failure
  case), and not to qdisc_qlen.

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---
diff -ruNp org/net/sched/sch_generic.c new/net/sched/sch_generic.c
--- org/net/sched/sch_generic.c 2007-06-11 13:12:11.000000000 +0530
+++ new/net/sched/sch_generic.c 2007-06-11 15:37:48.000000000 +0530
@@ -61,7 +61,6 @@ void qdisc_unlock_tree(struct net_device
 
 static inline int qdisc_qlen(struct Qdisc *q)
 {
-	BUG_ON((int) q->q.qlen < 0);
 	return q->q.qlen;
 }
 
@@ -167,9 +166,7 @@ static inline int qdisc_restart(struct n
 	/* And release queue */
 	spin_unlock(&dev->queue_lock);
 
-	ret = NETDEV_TX_BUSY;
-	if (!netif_queue_stopped(dev))
-		ret = dev_hard_start_xmit(skb, dev);
+	ret = dev_hard_start_xmit(skb, dev);
 
 	if (!lockless)
 		netif_tx_unlock(dev);

Re: [PATCH 2/2 - rev2] qdisc_restart - couple of optimizations.

[David Miller]

From: Krishna Kumar <kumarkr@linux.vnet.ibm.com>
Date: Mon, 18 Jun 2007 10:24:11 +0530

> (Same from previous patch, resending for completion)
> 
> Changes :
> 
> - netif_queue_stopped need not be called inside qdisc_restart as
>   it has been called already in qdisc_run() before the first skb
>   is sent, and in __qdisc_run() after each intermediate skb is 
>   sent (note : we are the only sender, so the queue cannot get
>   stopped while the tx lock was got in the ~LLTX case).
> 
> - BUG_ON((int) q->q.qlen < 0) was a relic from old times when -1
>   meant more packets are available, and __qdisc_run used to loop
>   when qdisc_restart() returned -1. During those days, it was
>   necessary to make sure that qlen is never less than zero, since
>   __qdisc_run would get into an infinite loop if no packets are on
>   the queue and this bug in qdisc was there (and worse - no more
>   skbs could ever get queue'd as we hold the queue lock too). With
>   Herbert's recent change to return values, this check is not
>   required.  Hopefully Herbert can validate this change. If at all
>   this is required, it should be added to skb_dequeue (in failure
>   case), and not to qdisc_qlen.
> 
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>

Also applied to net-2.6.23, thanks a lot!