Re: [PATCH] pch_gbe: don't come up if we can't allocate buffers

On Thu, Oct 25, 2012 at 10:40:58AM +0200, Eric Dumazet wrote:

On Thu, 2012-10-25 at 09:10 +0200, Veaceslav Falico wrote:

quoted

Currently, even if pch_gbe_alloc_tx/rx_buffers (even partially) fail, we
bring the interface up. It mihgt confuse the user (if he requested specific
amount of buffers) and can lead to different bugs.

Add error handling to these functions and clean after them in case of
failure. It also resolves a possible NULL pointer dereference in
pch_gbe_alloc_tx_buffers(), in case of netdev_alloc_skb() failure. It's ok
to (paritally) fail for pch_gbe_alloc_rx_buffers() in some situation, so
don't clean inside and rather handle skb freeing outside of the function.

This patch also adds tx_alloc_buff_failed ethtool counter.

This should not be needed :

quoted

-static void pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter,
+static int pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter,
 					struct pch_gbe_tx_ring *tx_ring)
 {
 	struct pch_gbe_buffer *buffer_info;

@@ -1506,6 +1521,7 @@ static void pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter,
 	unsigned int i;
 	unsigned int bufsz;
 	struct pch_gbe_tx_desc *tx_desc;
+	int err;

 	bufsz =
 	    adapter->hw.mac.max_frame_size + PCH_GBE_DMA_ALIGN + NET_IP_ALIGN;

@@ -1513,12 +1529,26 @@ static void pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter,
 	for (i = 0; i < tx_ring->count; i++) {
 		buffer_info = &tx_ring->buffer_info[i];
 		skb = netdev_alloc_skb(adapter->netdev, bufsz);
+		if (unlikely(!skb)) {
+			adapter->stats.tx_alloc_buff_failed++;
+			err = -ENOMEM;
+			goto freeall;
+		}

Hmmm

These skbs should be allocated using GFP_KERNEL, to lower risk of OOM,
using a mere alloc_skb(bufsz, GFP_KERNEL)

Only skbs for rx 'need' netdev_alloc_skb() to add a NET_SKB_PAD
headroom.

skb = alloc_skb(bufsz, GFP_KERNEL);
if (!skb)
	goto fail;
skb_reserve(skb, NET_IP_ALIGN);

And BTW, we dont really need skbs here, only a bounce buffer so that we
can copy frames on it... (kmalloc() instead of alloc_skb())

quoted

 		skb_reserve(skb, PCH_GBE_DMA_ALIGN);

PCH_GBE_DMA_ALIGN is zero ...

quoted

 		buffer_info->skb = skb;
 		tx_desc = PCH_GBE_TX_DESC(*tx_ring, i);
 		tx_desc->gbec_status = (DSC_INIT16);
 	}
-	return;
+	return 0;
+
+freeall:
+	for (i--; i >= 0; i--) {
+		dev_kfree_skb_any(buffer_info->skb);
+		buffer_info->skb = NULL;
+		tx_desc = PCH_GBE_TX_DESC(*tx_ring, i);
+		tx_desc->gbec_status = 0;
+	}
+	return err;
 }

Also I would call pch_gbe_alloc_tx_buffers() _after_
pch_gbe_alloc_rx_buffers() (Since rx allocations probably are using
GFP_ATOMIC allocations)

Agree with all, will update the patch.

Thank you very much for the review!