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]
Message-ID: <20090910090206.GA22276@csn.ul.ie>
Date:	Thu, 10 Sep 2009 10:02:06 +0100
From:	Mel Gorman <mel@....ul.ie>
To:	reinette chatre <reinette.chatre@...el.com>
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

On Wed, Sep 09, 2009 at 01:05:38PM -0700, reinette chatre wrote:
> 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.

Right, so it's a "refill now if you can and defer further refilling
until later".

> 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.
> 

Would it be possible to use __GFP_NOWARN *unless* this allocation is
necessary to receive the packet?

> 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.
> 

If it does not distinguish between failures causing packet loss and just a
temporary issue, I'd be worried the messages would generate bug reports and
we genuinely won't know if it's a real problem or not.

As a total aside, there is still the problem that the driver is depending on
order-2 allocations. On systems without swap, the allocation problem could be
more severe as there are fewer pages the system can use to regain contiguity.

> 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);
>  

So, would it be possible here to only remove __GFP_NOWARN if this is GFP_ATOMIC
(implying we have to refill now) and the rxq->free_count is really small
e.g. <= 2?

>  		if (!skb) {
> -			IWL_CRIT(priv, "Can not allocate SKB buffers\n");
> +			if (net_ratelimit())
> +				IWL_CRIT(priv, "Can not allocate SKB buffer.\n");

Similarly, could the message either be supressed when there is enough
buffers in the RX queue or differenciate between enough buffers and
things getting tight possibly causing packet loss?

The IWL_CRIT() part even is a hint - there is no guarantee that the allocation
failure is really a critical problem.

>  			/* 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 */
> 
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab
--
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/