Return-path: Received: from mga14.intel.com ([143.182.124.37]:25958 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752660AbZIIUFg (ORCPT ); Wed, 9 Sep 2009 16:05:36 -0400 Subject: Re: iwlagn: order 2 page allocation failures From: reinette chatre To: Mel Gorman Cc: Frans Pop , Larry Finger , "John W. Linville" , Pekka Enberg , "linux-kernel@vger.kernel.org" , "linux-wireless@vger.kernel.org" , "ipw3945-devel@lists.sourceforge.net" , Andrew Morton , "cl@linux-foundation.org" , "Krauss, Assaf" , Johannes Berg , "Abbas, Mohamed" In-Reply-To: <20090909165545.GK24614@csn.ul.ie> References: <200909060941.01810.elendil@planet.nl> <4AA67139.80301@lwfinger.net> <20090909150418.GI24614@csn.ul.ie> <200909091759.33655.elendil@planet.nl> <20090909165545.GK24614@csn.ul.ie> Content-Type: text/plain Date: Wed, 09 Sep 2009 13:05:38 -0700 Message-Id: <1252526738.30150.91.camel@rc-desk> Mime-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org List-ID: 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 */