[PATCH 2.6] ixgb: fix ixgb_intr looping checks

[PATCH 2.6] ixgb: fix ixgb_intr looping checks

[Jesse Brandeburg]

Hey Jeff, we have a little problem with a patch that got accepted into -mm 
and is now in netdev, but never got copied to netdev mailing list.

This thread:
http://marc.theaimsgroup.com/?t=109149508900004&r=1&w=2

the last comment is entirely correct, we need to update the comment.  
this patch reverts the patch to ixgb that akpm added to do two for loops,
as we actually did intend to do the boolean '&' check, and both the
functions *should* be called every time through the loop.

Here is a patch that we would like applied, it undoes the change we don't 
like, it was checked to apply cleanly against the netdev-2.6 tree this 
morning, and reviewed by the team.

It also keeps the 10Gig (ixgb) driver consistent with our 1gig (e1000) 
code. 

This patch undoes a change that we believe will impact performance
adversely, by creating possibly too long a delay between servicing
completions. The comment pretty much explains it.  We need to call both
cleanup routines each pass through the loop, this time we have a comment
explaining why.

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>

--- netdev-2.6/drivers/net/ixgb/ixgb_main.c	2004-10-15 10:39:56.000000000 -0700
+++ netdev-2.6/drivers/net/ixgb/ixgb_main.c.new	2004-10-15 11:09:42.000000000 -0700
@@ -1613,13 +1613,14 @@ static irqreturn_t ixgb_intr(int irq, vo
 		__netif_rx_schedule(netdev);
 	}
 #else
-	for (i = 0; i < IXGB_MAX_INTR; i++)
-		if (ixgb_clean_rx_irq(adapter) == FALSE)
-			break;
-	for (i = 0; i < IXGB_MAX_INTR; i++)
-		if (ixgb_clean_tx_irq(adapter) == FALSE)
+	/* yes, that is actually a & and it is meant to make sure that
+	 * every pass through this for loop checks both receive and
+	 * transmit queues for completed descriptors, intended to
+	 * avoid starvation issues and assist tx/rx fairness. */
+	for(i = 0; i < IXGB_MAX_INTR; i++)
+		if(!ixgb_clean_rx_irq(adapter) &
+		   !ixgb_clean_tx_irq(adapter))
 			break;
-
 	/* if RAIDC:EN == 1 and ICR:RXDMT0 == 1, we need to
 	 * set IMS:RXDMT0 to 1 to restart the RBD timer (POLL)
 	 */

Re: [PATCH 2.6] ixgb: fix ixgb_intr looping checks

[Jesse Brandeburg]

whoops! including jeff this time...

On Tue, 2 Nov 2004, Jesse Brandeburg wrote:


Hey Jeff, we have a little problem with a patch that got accepted into
-mm
and is now in netdev, but never got copied to netdev mailing list.

This thread:
http://marc.theaimsgroup.com/?t=109149508900004&r=1&w=2

the last comment is entirely correct, we need to update the comment. 
this patch reverts the patch to ixgb that akpm added to do two for loops,
as we actually did intend to do the boolean '&' check, and both the
functions *should* be called every time through the loop.

Here is a patch that we would like applied, it undoes the change we don't
like, it was checked to apply cleanly against the netdev-2.6 tree this
morning, and reviewed by the team.

It also keeps the 10Gig (ixgb) driver consistent with our 1gig (e1000)
code.

This patch undoes a change that we believe will impact performance
adversely, by creating possibly too long a delay between servicing
completions. The comment pretty much explains it.  We need to call both
cleanup routines each pass through the loop, this time we have a comment
explaining why.

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>

--- netdev-2.6/drivers/net/ixgb/ixgb_main.c     2004-10-15
10:39:56.000000000 -0700
+++ netdev-2.6/drivers/net/ixgb/ixgb_main.c.new 2004-10-15
11:09:42.000000000 -0700
@@ -1613,13 +1613,14 @@ static irqreturn_t ixgb_intr(int irq, vo
                __netif_rx_schedule(netdev);
        }
 #else
-       for (i = 0; i < IXGB_MAX_INTR; i++)
-               if (ixgb_clean_rx_irq(adapter) == FALSE)
-                       break;
-       for (i = 0; i < IXGB_MAX_INTR; i++)
-               if (ixgb_clean_tx_irq(adapter) == FALSE)
+       /* yes, that is actually a & and it is meant to make sure that
+        * every pass through this for loop checks both receive and
+        * transmit queues for completed descriptors, intended to
+        * avoid starvation issues and assist tx/rx fairness. */
+       for(i = 0; i < IXGB_MAX_INTR; i++)
+               if(!ixgb_clean_rx_irq(adapter) &
+                  !ixgb_clean_tx_irq(adapter))
                        break;
-
        /* if RAIDC:EN == 1 and ICR:RXDMT0 == 1, we need to
         * set IMS:RXDMT0 to 1 to restart the RBD timer (POLL)
         */

Re: [PATCH 2.6] ixgb: fix ixgb_intr looping checks

[Jeff Garzik]

I'm OK with the patch, but it doesn't apply for me...

Can you resend against 2.6.10-rc1-bk14?

	Jeff

Re: [PATCH 2.6] ixgb: fix ixgb_intr looping checks

[Jesse Brandeburg]

Try again:
This patch undoes a change that we believe will impact performance adversely,
by creating possibly too long a delay between servicing completions.
The comment pretty much explains it.  We need to call both cleanup routines each
pass through the loop, this time we have a comment explaining why.

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>

===== drivers/net/ixgb/ixgb_main.c 1.21 vs edited =====
--- 1.21/drivers/net/ixgb/ixgb_main.c	2004-11-04 21:00:33 -08:00
+++ edited/drivers/net/ixgb/ixgb_main.c	2004-11-08 13:58:01 -08:00
@@ -1613,13 +1613,14 @@
 		__netif_rx_schedule(netdev);
 	}
 #else
-	for (i = 0; i < IXGB_MAX_INTR; i++)
-		if (ixgb_clean_rx_irq(adapter) == FALSE)
+	/* yes, that is actually a & and it is meant to make sure that
+	 * every pass through this for loop checks both receive and
+	 * transmit queues for completed descriptors, intended to
+	 * avoid starvation issues and assist tx/rx fairness. */
+	for(i = 0; i < IXGB_MAX_INTR; i++)
+		if(!ixgb_clean_rx_irq(adapter) &
+		   !ixgb_clean_tx_irq(adapter))
 			break;
-	for (i = 0; i < IXGB_MAX_INTR; i++)
-		if (ixgb_clean_tx_irq(adapter) == FALSE)
-			break;
-
 	/* if RAIDC:EN == 1 and ICR:RXDMT0 == 1, we need to
 	 * set IMS:RXDMT0 to 1 to restart the RBD timer (POLL)
 	 */

Re: [PATCH 2.6] ixgb: fix ixgb_intr looping checks

[Andrew Morton]

Jesse Brandeburg <jesse.brandeburg@intel.com> wrote:
>
> This patch undoes a change that we believe will impact performance adversely,
> by creating possibly too long a delay between servicing completions.

Maybe.  But now take a look at how much additional pointless work will be
done in the common case.  For instance, every tx completion will incur a
call to ixgb_clean_rx_irq(), which then calls ixgb_alloc_rx_buffers().

There's quite a bit which can be optimised here.

Re: [PATCH 2.6] ixgb: fix ixgb_intr looping checks

[Jesse Brandeburg]

On Mon, 8 Nov 2004, Andrew Morton wrote:
> > This patch undoes a change that we believe will impact performance
> adversely,
> > by creating possibly too long a delay between servicing completions.
> 
> Maybe.  But now take a look at how much additional pointless work will be
> done in the common case.  For instance, every tx completion will incur a
> call to ixgb_clean_rx_irq(), which then calls ixgb_alloc_rx_buffers().

Is your common case transmits only? It seems there will always be more
than just one transmit and one receive to service in an interrupt.  The
issue with the current code is that the inner functions can loop
(clean_[tx|rx]) over many packets on their own, so I describe the current
code a little differently: "every group of tx completions, will then make
sure to call clean_rx to balance tx/rx work."  The way that you proposed
seems as if it may starve one side or the other if there are many packets
going in one direction. If I'm missing something let me know.

> There's quite a bit which can be optimised here.

Appreciate your input Andrew, currently we're just trying to keep the
e1000 and ixgb drivers the same, however it is possible that your solution
is better.  We are going to do a little experiment here over the next
couple of days to try performance with both your suggestion and with our
old method.  We'll reply with our results.

Jesse

Re: [PATCH 2.6] ixgb: fix ixgb_intr looping checks

[Andrew Morton]

Jesse Brandeburg <jesse.brandeburg@intel.com> wrote:
>
> On Mon, 8 Nov 2004, Andrew Morton wrote:
>  > > This patch undoes a change that we believe will impact performance
>  > adversely,
>  > > by creating possibly too long a delay between servicing completions.
>  > 
>  > Maybe.  But now take a look at how much additional pointless work will be
>  > done in the common case.  For instance, every tx completion will incur a
>  > call to ixgb_clean_rx_irq(), which then calls ixgb_alloc_rx_buffers().
> 
>  Is your common case transmits only?

hm, good question.  Usually not, I guess.  IIRC TCP normally runs with
2Tx:1Rx.  NFS-over-UDP will send and receive a lot of back-to-back frames.