Re: iwlagn: order 2 page allocation failures

[Mel Gorman <mel@csn.ul.ie>]

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