Re[2]: Kernel oops while routing

[Ricardo Scop <scop@digitel.com.br>]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

Dan,

I apologize for the delay. We were conducting some more tests so as to
not make any more false alarms :-) about kernel crashes, memory leaks
and/or performance problems in the linuxppc port to our 8255 hardware
platform.

So, after a _carefull_ test period, these are our findings:

1 - Andy's patch (which is attached) works well and does _not_
append any performance penalties in our tests (we were having PHY
negatiation problems there, again :-/ ).

2 - We _did_ have a memory leak which was causing a kernel crash after
a while, and it _was_ solved by Andy's patch (thanks, Andy!). I believe
it's still on linuxppc_2_4, _2_4_devel and _2_5. It goes like this:

     - in fcc_enet_start_xmit, after setting up another bd and
     incrementing bdp, the next bd's tx-ready bit is tested in order
     to stop the xmit queue if it is set, ok? But, sometimes, the CPM
     may already have cleared this bit _and_ the corresponding
     interrupt has not been serviced yet (because we're in a
     spin_lock_irq); so, netif_stop_queue is not called in this case,
     nor is tx_full set;

     - next, the interrupt is serviced, but then curr_tx equals
     dirty_tx _and_ tx_full is not set, so no sk_buffers are freed!

     - next time fcc_enet_start_xmit is called, tx_ready bit is still
     cleared and the next bd is used, but the corresponding sk_buffer
     wasn't freed, and it's pointer is now lost;

     - cep->lock can't help with this problem, because the CPM is not
     bothered by that 8-). AFAIK, Andy's solution is a good one.

So, we're offering this patch to the public list (with Andy's
blessing :-). I can provide any other details about our tests, if
required.

Thenks,

Ricardo Scop                            mailto:scop@vanet.com.br
R SCOP Consulting

------------------------------------------------------------------
"What's money? A man is a success if he gets up in the morning and
goes to bed at night and in between does what he wants to do."
~Bob Dylan

Thursday, November 29, 2001, 7:42:24 PM, you wrote:

DM> Ricardo Scop wrote:


>> I'm kind of lost with this performance variations. As far as I could
>> see, the patch did not insert much processing overhead, so...

DM> Perhaps if someone would post the patch for the rest of us to see we
DM> could be of some assistance.


DM>         -- Dan

[-- Attachment #2: patch-2.41.16-pre1-fcc_enet --]
[-- Type: application/octet-stream, Size: 2359 bytes --]

Index: arch/ppc/8260_io/fcc_enet.c
===================================================================
RCS file: /var/cvs/kernel/arch/ppc/8260_io/fcc_enet.c,v
retrieving revision 1.1.1.1.4.2
diff -u -r1.1.1.1.4.2 fcc_enet.c
--- arch/ppc/8260_io/fcc_enet.c	4 Sep 2001 16:37:18 -0000	1.1.1.1.4.2
+++ arch/ppc/8260_io/fcc_enet.c	27 Nov 2001 18:42:43 -0000
@@ -300,7 +300,7 @@
 	volatile fcc_t	*fccp;
 	volatile fcc_enet_t	*ep;
 	struct	net_device_stats stats;
-	uint	tx_full;
+	uint	tx_free;
 	spinlock_t lock;
 
 #ifdef	CONFIG_USE_MDIO
@@ -360,9 +360,9 @@
 	bdp = cep->cur_tx;
 
 #ifndef final_version
-	if (bdp->cbd_sc & BD_ENET_TX_READY) {
+	if (!cep->tx_free || (bdp->cbd_sc & BD_ENET_TX_READY)) {
 		/* Ooops.  All transmit buffers are full.  Bail out.
-		 * This should not happen, since cep->tx_full should be set.
+		 * This should not happen, since the tx queue should be stopped.
 		 */
 		printk("%s: tx queue full!.\n", dev->name);
 		return 1;
@@ -407,10 +407,8 @@
 	else
 		bdp++;
 
-	if (bdp->cbd_sc & BD_ENET_TX_READY) {
+	if (!--cep->tx_free)
 		netif_stop_queue(dev);
-		cep->tx_full = 1;
-	}
 
 	cep->cur_tx = (cbd_t *)bdp;
 
@@ -431,8 +429,8 @@
 	{
 		int	i;
 		cbd_t	*bdp;
-		printk(" Ring data dump: cur_tx %p%s cur_rx %p.\n",
-		       cep->cur_tx, cep->tx_full ? " (full)" : "",
+		printk(" Ring data dump: cur_tx %p tx_free %d cur_rx %p.\n",
+		       cep->cur_tx, cep->tx_free,
 		       cep->cur_rx);
 		bdp = cep->tx_bd_base;
 		printk(" Tx @base %p :\n", bdp);
@@ -450,7 +448,7 @@
 			       bdp->cbd_bufaddr);
 	}
 #endif
-	if (!cep->tx_full)
+	if (cep->tx_free)
 		netif_wake_queue(dev);
 }
 
@@ -492,7 +490,7 @@
 	    spin_lock(&cep->lock);
 	    bdp = cep->dirty_tx;
 	    while ((bdp->cbd_sc&BD_ENET_TX_READY)==0) {
-		if ((bdp==cep->cur_tx) && (cep->tx_full == 0))
+		if (cep->tx_free == TX_RING_SIZE)
 		    break;
 
 		if (bdp->cbd_sc & BD_ENET_TX_HB)	/* No heartbeat */
@@ -546,8 +544,7 @@
 		/* Since we have freed up a buffer, the ring is no longer
 		 * full.
 		 */
-		if (cep->tx_full) {
-			cep->tx_full = 0;
+		if (!cep->tx_free++) {
 			if (netif_queue_stopped(dev)) {
 				netif_wake_queue(dev);
 			}
@@ -1529,6 +1526,7 @@
 #endif
 
 	cep->dirty_tx = cep->cur_tx = cep->tx_bd_base;
+	cep->tx_free = TX_RING_SIZE;
 	cep->cur_rx = cep->rx_bd_base;
 
 	ep->fen_genfcc.fcc_rstate = (CPMFCR_GBL | CPMFCR_EB) << 24;