linux-kernel - Re: iwlagn: order 2 page allocation failures

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 09 Sep 2009 13:05:38 -0700
From:	reinette chatre <reinette.chatre@...el.com>
To:	Mel Gorman <mel@....ul.ie>
Cc:	Frans Pop <elendil@...net.nl>,
	Larry Finger <Larry.Finger@...inger.net>,
	"John W. Linville" <linville@...driver.com>,
	Pekka Enberg <penberg@...helsinki.fi>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
	"ipw3945-devel@...ts.sourceforge.net" 
	<ipw3945-devel@...ts.sourceforge.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"cl@...ux-foundation.org" <cl@...ux-foundation.org>,
	"Krauss, Assaf" <assaf.krauss@...el.com>,
	Johannes Berg <johannes@...solutions.net>,
	"Abbas, Mohamed" <mohamed.abbas@...el.com>
Subject: Re: iwlagn: order 2 page allocation failures

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/