Date:	Fri, 11 Sep 2009 09:45:42 +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 Thu, Sep 10, 2009 at 11:15:47AM -0700, reinette chatre wrote:
> > > 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 think so.
> 
> > > 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.
> 
> Good point.
> 
> > 
> > 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.
> 
> It seems that somebody did think about this in the initialization of
> max_pkt_size (which is priv->hw_params.rx_buf_size - 256). If we use
> max_pkt_size in the code that allocates the skb then the 256 added for
> alignment will not cause it to go to an order-2 allocation. I do not
> know why max_pkt_size is not used at the moment and will have to do some
> digging to find out.
> 

Thanks

> > > 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?
> 
> I like your suggestion. Considering the issue I would like to remove
> __GFP_NOWARN even if it is GFP_KERNEL ... I think it is actually even
> more of a problem if we are in GFP_KERNEL and not able to allocate
> memory when running low on buffers. Also, with the queue size of 256 I
> think we can use RX_LOW_WATERMARK here, which is 8.
> 

RX_LOW_WATERMARK sounds reasonable as if that watermark is reached, the
buffer count is pretty low. With order-2 allocations, I bet the system is
beginning to grind a bit to find contiguous pages at that point as well.

I agree that it's a greater problem if the system is unable to allocate
the pages as GFP_KERNEL - prehaps to the extent where it's worth
distinguishing between GFP_KERNEL and GFP_ATOMIC failures. If GFP_KERNEL
allocations are failure, packet loss is likely and the system may not
recover, particularly if there is no swap configured.

> > 
> 
> > 
> > >  		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?
> 
> Frans also had comments in this regard. Will do. 
> 
> > 
> > The IWL_CRIT() part even is a hint - there is no guarantee that the allocation
> > failure is really a critical problem.
> 
> Right.
> 
> How about this: 
> 
> >From bd2153dd9e4a0ad588adec38c580d67023d5587e Mon Sep 17 00:00:00 2001
> From: Reinette Chatre <reinette.chatre@...el.com>
> Date: Wed, 9 Sep 2009 15:41:00 -0700
> Subject: [PATCH] iwlwifi: reduce noise when skb allocation fails
> 
> Replenishment of receive buffers is done in the tasklet handling
> received frames as well as in a workqueue. When we are in the tasklet
> we cannot sleep and thus attempt atomic skb allocations. It is generally
> not a big problem if this fails since iwl_rx_allocate is always followed
> by a call to iwl_rx_queue_restock which will queue the work to replenish
> the buffers at a time when sleeping is allowed.
> 
> We thus add the __GFP_NOWARN to the skb allocation in iwl_rx_allocate to
> reduce the noise if such an allocation fails while we still have enough
> buffers. We do maintain the warning and the error message when we are low
> on buffers to communicate to the user that there is a potential problem with
> memory availability on system
> 
> This addresses issue reported upstream in thread "iwlagn: order 2 page
> allocation failures" in
> http://thread.gmane.org/gmane.linux.kernel.wireless.general/39187
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@...el.com>
> ---
>  drivers/net/wireless/iwlwifi/iwl-rx.c       |   12 +++++++++---
>  drivers/net/wireless/iwlwifi/iwl3945-base.c |    8 +++++++-
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/iwlwifi/iwl-rx.c b/drivers/net/wireless/iwlwifi/iwl-rx.c
> index b90adcb..cb50cc7 100644
> --- a/drivers/net/wireless/iwlwifi/iwl-rx.c
> +++ b/drivers/net/wireless/iwlwifi/iwl-rx.c
> @@ -250,12 +250,18 @@ void iwl_rx_allocate(struct iwl_priv *priv, gfp_t priority)
>  		}
>  		spin_unlock_irqrestore(&rxq->lock, flags);
>  
> +		if (rxq->free_count > RX_LOW_WATERMARK)
> +			priority |= __GFP_NOWARN;

Seems very reasonable.

>  		/* Alloc a new receive buffer */
> -		skb = alloc_skb(priv->hw_params.rx_buf_size + 256,
> -						priority);
> +		skb = alloc_skb(priv->hw_params.rx_buf_size + 256, priority);
>  

This change appears superflous. It don't change any functionality. Looks
like the style is just being made consistent with a similar code block
elsewhere.

>  		if (!skb) {
> -			IWL_CRIT(priv, "Can not allocate SKB buffers\n");
> +			if (net_ratelimit())
> +				IWL_DEBUG_INFO("Failed to allocate SKB buffer.\n");
> +			if ((rxq->free_count <= RX_LOW_WATERMARK) &&
> +			    net_ratelimit())
> +				IWL_CRIT(priv, "Failed to allocate SKB buffer. Only %u free buffers remaining\n",
> +					 rxq->free_count);


To get a good idea of how screwed we really are, how about?

				IWL_CRIT(priv, "Failed to allocate SKB buffer with %s. Only %u free buffers remaining\n",
					priority == GFP_ATOMIC ?  "GFP_ATOMIC" : "GFP_KERNEL",
					 rxq->free_count);

>  			/* 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..0d96b17 100644
> --- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
> +++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
> @@ -1146,11 +1146,17 @@ static void iwl3945_rx_allocate(struct iwl_priv *priv, gfp_t priority)
>  		}
>  		spin_unlock_irqrestore(&rxq->lock, flags);
>  
> +		if (rxq->free_count > RX_LOW_WATERMARK)
> +			priority |= __GFP_NOWARN;
>  		/* Alloc a new receive buffer */
>  		skb = alloc_skb(priv->hw_params.rx_buf_size, priority);
>  		if (!skb) {
>  			if (net_ratelimit())
> -				IWL_CRIT(priv, ": Can not allocate SKB buffers\n");
> +				IWL_DEBUG_INFO("Failed to allocate SKB buffer.\n");
> +			if ((rxq->free_count <= RX_LOW_WATERMARK) &&
> +			    net_ratelimit())
> +				IWL_CRIT(priv, "Failed to allocate SKB buffer. Only %u free buffers remaining\n",
> +					 rxq->free_count);
>  			/* We don't reschedule replenish work here -- we will
>  			 * call the restock method and if it still needs
>  			 * more buffers it will schedule replenish */

Otherwise, it looks just the finest and I think it will address the
problem to some extent - in that it won't print alarming messages when
they are not needed.

The additional changes with respect to GFP_ATOMIC are optional. Whether
you do it or not.

Acked-by: Mel Gorman <mel@....ul.ie>

Thanks very much.

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

Powered by blists - more mailing lists