sis900: eth0: NULL pointer encountered in Rx ring, skipping

sis900: eth0: NULL pointer encountered in Rx ring, skipping

[Lennert Buytenhek]

Hi,

I can make one of my machines, which has an on-board sis900 NIC, lose
network connectivity within seconds by doing something like
"nc -l -p 6666 > /dev/null" and then blasting some data towards it.  When
this happens, syslog fills up with these messages:

	eth0: NULL pointer encountered in Rx ring, skipping

It seems like the driver+NIC combo doesn't handle any kind of load very
well in general -- whenever it receives more than just a few tens of
packets per second, the machine's ping time turns rather awful:  (Two
machines on 100Mb/s with an el-cheapo 8-port 100Mb/s switch between
them.  Other machines on the same switch don't show any of the same
problems.)

	64 bytes from 10.0.0.4: icmp_seq=212 ttl=64 time=128 ms
	64 bytes from 10.0.0.4: icmp_seq=213 ttl=64 time=124 ms
	64 bytes from 10.0.0.4: icmp_seq=214 ttl=64 time=119 ms
	64 bytes from 10.0.0.4: icmp_seq=215 ttl=64 time=124 ms

When it locks up, 'ifconfig eth0 down up' suffices to make it come back
to life.  I haven't checked whether it's just RX that stops working or
whether it's both RX and TX.

Any ideas?  This is on the Fedora Core 3 update kernel, 2.6.10-based.


cheers,
Lennert

Re: sis900: eth0: NULL pointer encountered in Rx ring, skipping

[Lennert Buytenhek]

On Tue, Jan 18, 2005 at 11:13:58AM +0100, Lennert Buytenhek wrote:

> I can make one of my machines, which has an on-board sis900 NIC, lose
> network connectivity within seconds by doing something like
> "nc -l -p 6666 > /dev/null" and then blasting some data towards it.  When
> this happens, syslog fills up with these messages:
> 
> 	eth0: NULL pointer encountered in Rx ring, skipping

Just before it starts spewing these it says:

	eth0: Memory squeeze,deferring packet.

So apparently it just doesn't deal with OOM very well.


--L

Re: sis900: eth0: NULL pointer encountered in Rx ring, skipping

[Eric Lemoine]

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

On Tue, 18 Jan 2005 11:21:39 +0100, Lennert Buytenhek
<buytenh@wantstofly.org> wrote:
> On Tue, Jan 18, 2005 at 11:13:58AM +0100, Lennert Buytenhek wrote:
> 
> > I can make one of my machines, which has an on-board sis900 NIC, lose
> > network connectivity within seconds by doing something like
> > "nc -l -p 6666 > /dev/null" and then blasting some data towards it.  When
> > this happens, syslog fills up with these messages:
> >
> >       eth0: NULL pointer encountered in Rx ring, skipping
> 
> Just before it starts spewing these it says:
> 
>         eth0: Memory squeeze,deferring packet.
> 
> So apparently it just doesn't deal with OOM very well.

There are dev_alloc_skb()'s failing due to memory shortage. I think
anyhow that the sis9000 driver doesnt handle "memory squeeze"
situations right. Why not throwing current rx packet away and reuse
ring entry instead of leaving holes (null skb's) in the ring. I would
suggest a patch to the sis900 driver around that attached.

Warning: patch compiling but untested - dont have hardware handy.  

-- 
Eric

[-- Attachment #2: sis900.patch --]
[-- Type: application/octet-stream, Size: 3038 bytes --]

===== drivers/net/sis900.c 1.56 vs edited =====
--- 1.56/drivers/net/sis900.c	2004-08-25 09:15:20 +02:00
+++ edited/drivers/net/sis900.c	2005-01-19 21:48:27 +01:00
@@ -1650,7 +1650,7 @@
 			/* reset buffer descriptor state */
 			sis_priv->rx_ring[entry].cmdsts = RX_BUF_SIZE;
 		} else {
-			struct sk_buff * skb;
+			struct sk_buff * skb, *new_skb;
 
 			/* This situation should never happen, but due to
 			   some unknow bugs, it is possible that
@@ -1662,11 +1662,30 @@
 				break;
 			}
 
+			skb = sis_priv->rx_skbuff[entry];
+
+			if ((new_skb = dev_alloc_skb(RX_BUF_SIZE)) == NULL) {
+				printk(KERN_INFO "%s: memory shortage, rx packet dropped\n",
+					net_dev->name);
+
+				sis_priv->rx_ring[entry].cmdsts = RX_BUF_SIZE;
+				sis_priv->stats.rx_dropped++;
+				sis_priv->dirty_rx++; /* dirty no longer needed? */
+				break; /* next */
+			}
+
 			pci_unmap_single(sis_priv->pci_dev, 
 				sis_priv->rx_ring[entry].bufptr, RX_BUF_SIZE, 
 				PCI_DMA_FROMDEVICE);
+
+			new_skb->dev = net_dev;
+			sis_priv->rx_skbuff[entry] = new_skb;
+			sis_priv->rx_ring[entry].cmdsts = RX_BUF_SIZE;
+                	sis_priv->rx_ring[entry].bufptr = 
+				pci_map_single(sis_priv->pci_dev, new_skb->tail, 
+					RX_BUF_SIZE, PCI_DMA_FROMDEVICE);
+
 			/* give the socket buffer to upper layers */
-			skb = sis_priv->rx_skbuff[entry];
 			skb_put(skb, rx_size);
 			skb->protocol = eth_type_trans(skb, net_dev);
 			netif_rx(skb);
@@ -1678,29 +1697,6 @@
 			sis_priv->stats.rx_bytes += rx_size;
 			sis_priv->stats.rx_packets++;
 
-			/* refill the Rx buffer, what if there is not enought memory for
-			   new socket buffer ?? */
-			if ((skb = dev_alloc_skb(RX_BUF_SIZE)) == NULL) {
-				/* not enough memory for skbuff, this makes a "hole"
-				   on the buffer ring, it is not clear how the
-				   hardware will react to this kind of degenerated
-				   buffer */
-				printk(KERN_INFO "%s: Memory squeeze,"
-				       "deferring packet.\n",
-				       net_dev->name);
-				sis_priv->rx_skbuff[entry] = NULL;
-				/* reset buffer descriptor state */
-				sis_priv->rx_ring[entry].cmdsts = 0;
-				sis_priv->rx_ring[entry].bufptr = 0;
-				sis_priv->stats.rx_dropped++;
-				break;
-			}
-			skb->dev = net_dev;
-			sis_priv->rx_skbuff[entry] = skb;
-			sis_priv->rx_ring[entry].cmdsts = RX_BUF_SIZE;
-                	sis_priv->rx_ring[entry].bufptr = 
-				pci_map_single(sis_priv->pci_dev, skb->tail, 
-					RX_BUF_SIZE, PCI_DMA_FROMDEVICE);
 			sis_priv->dirty_rx++;
 		}
 		sis_priv->cur_rx++;
@@ -1708,6 +1704,7 @@
 		rx_status = sis_priv->rx_ring[entry].cmdsts;
 	} // while
 
+#if 0 /* no need to refill skbs are added to the rx ring as packets are received */
 	/* refill the Rx buffer, what if the rate of refilling is slower than 
 	   consuming ?? */
 	for (;sis_priv->cur_rx - sis_priv->dirty_rx > 0; sis_priv->dirty_rx++) {
@@ -1735,6 +1732,7 @@
 					RX_BUF_SIZE, PCI_DMA_FROMDEVICE);
 		}
 	}
+#endif
 	/* re-enable the potentially idle receive state matchine */
 	outl(RxENA | inl(ioaddr + cr), ioaddr + cr );
 

Re: sis900: eth0: NULL pointer encountered in Rx ring, skipping

[Francois Romieu]

Eric Lemoine <eric.lemoine@gmail.com> :
[...]
> There are dev_alloc_skb()'s failing due to memory shortage. I think
> anyhow that the sis9000 driver doesnt handle "memory squeeze"
> situations right. Why not throwing current rx packet away and reuse
> ring entry instead of leaving holes (null skb's) in the ring. I would
> suggest a patch to the sis900 driver around that attached.

Imho it should not be necessary as long as the hardware is not completely
b0rken. I'd rather see how the driver behaves when it is really short of
memory (i.e. no entry is available for DMA in the ring). Patch below.


Make the driver own a Rx descriptor entry as long as it is not refilled.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>

diff -puN drivers/net/sis900.c~sis900-000 drivers/net/sis900.c
--- linux-2.6.11-rc1-bk6/drivers/net/sis900.c~sis900-000	2005-01-19 22:28:38.633282552 +0100
+++ linux-2.6.11-rc1-bk6-fr/drivers/net/sis900.c	2005-01-19 22:34:16.362996145 +0100
@@ -1643,13 +1643,15 @@ static int sis900_rx(struct net_device *
 	long ioaddr = net_dev->base_addr;
 	unsigned int entry = sis_priv->cur_rx % NUM_RX_DESC;
 	u32 rx_status = sis_priv->rx_ring[entry].cmdsts;
+	int rx_left;
 
 	if (sis900_debug > 3)
 		printk(KERN_INFO "sis900_rx, cur_rx:%4.4d, dirty_rx:%4.4d "
 		       "status:0x%8.8x\n",
 		       sis_priv->cur_rx, sis_priv->dirty_rx, rx_status);
 
-	while (rx_status & OWN) {
+	rx_left = NUM_RX_DESC + sis_priv->dirty_rx - sis_priv->cur_rx;
+	while ((rx_left > 0) && (rx_status & OWN)) {
 		unsigned int rx_size;
 
 		rx_size = (rx_status & DSIZE) - CRC_SIZE;
@@ -1712,7 +1714,7 @@ static int sis900_rx(struct net_device *
 				       net_dev->name);
 				sis_priv->rx_skbuff[entry] = NULL;
 				/* reset buffer descriptor state */
-				sis_priv->rx_ring[entry].cmdsts = 0;
+				sis_priv->rx_ring[entry].cmdsts = OWN;
 				sis_priv->rx_ring[entry].bufptr = 0;
 				sis_priv->stats.rx_dropped++;
 				break;
@@ -1728,6 +1730,8 @@ static int sis900_rx(struct net_device *
 		sis_priv->cur_rx++;
 		entry = sis_priv->cur_rx % NUM_RX_DESC;
+		rmb();
 		rx_status = sis_priv->rx_ring[entry].cmdsts;
+		rx_left--;
 	} // while
 
 	/* refill the Rx buffer, what if the rate of refilling is slower

_