Re: iwlagn: order 2 page allocation failures

[reinette chatre]

Mel and Frans, 

Thank you very much for digging into this.

On Wed, 2009-09-09 at 09:55 -0700, Mel Gorman wrote:
> Conceivably a better candidate for this problem is commit
> 4752c93c30441f98f7ed723001b1a5e3e5619829 introduced in May 2009. If there
> are less than RX_QUEUE_SIZE/2 left, it starts replenishing buffers. Mohamed,
> is it absolutly necessary it use GFP_ATOMIC there? If an allocation fails,
> does it always mean frames are dropped or could it just replenish what it
> can and try again later printing a warning only if allocation failures are
> resulting in packet loss?

I agree that this patch may be the reason we are seeing this issue. We
would like to keep using GFP_ATOMIC here, but it is not necessary for an
allocation failure to be so noisy since the function doing the
allocation (iwl_rx_allocate) is always followed by a call to
iwl_rx_queue_restock which will schedule a refill if the buffers are
running low. We can thus use ___GFP_NOWARN for the allocations in
iwl_rx_allocate and leave it to the restocking to find the needed memory
when it tried its allocations using GFP_KERNEL.

I do think it is useful to let user know about these allocation
failures, even if it does not result in packet loss. Wrapping it in
net_ratelimit() will help though.

How about the patch below?

diff --git a/drivers/net/wireless/iwlwifi/iwl-rx.c b/drivers/net/wireless/iwlwifi/iwl-rx.c
index b90adcb..f0ee72e 100644
--- a/drivers/net/wireless/iwlwifi/iwl-rx.c
+++ b/drivers/net/wireless/iwlwifi/iwl-rx.c
@@ -252,10 +252,11 @@ void iwl_rx_allocate(struct iwl_priv *priv, gfp_t priority)
 
 		/* Alloc a new receive buffer */
 		skb = alloc_skb(priv->hw_params.rx_buf_size + 256,
-						priority);
+				priority | __GFP_NOWARN);
 
 		if (!skb) {
-			IWL_CRIT(priv, "Can not allocate SKB buffers\n");
+			if (net_ratelimit())
+				IWL_CRIT(priv, "Can not allocate SKB buffer.\n");
 			/* We don't reschedule replenish work here -- we will
 			 * call the restock method and if it still needs
 			 * more buffers it will schedule replenish */
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 0909668..5d9fb78 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -1147,10 +1147,10 @@ static void iwl3945_rx_allocate(struct iwl_priv *priv, gfp_t priority)
 		spin_unlock_irqrestore(&rxq->lock, flags);
 
 		/* Alloc a new receive buffer */
-		skb = alloc_skb(priv->hw_params.rx_buf_size, priority);
+		skb = alloc_skb(priv->hw_params.rx_buf_size, priority | __GFP_NOWARN);
 		if (!skb) {
 			if (net_ratelimit())
-				IWL_CRIT(priv, ": Can not allocate SKB buffers\n");
+				IWL_CRIT(priv, "Can not allocate SKB buffer.\n");
 			/* We don't reschedule replenish work here -- we will
 			 * call the restock method and if it still needs
 			 * more buffers it will schedule replenish */


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/