Re: iwlagn: order 2 page allocation failures
From: Mel Gorman
Date: Thu Sep 10 2009 - 05:03:52 EST
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@xxxxxxxxxxxxxxx
More majordomo info at
http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
http://www.tux.org/lkml/