On Thu, 2009-09-10 at 02:02 -0700, Mel Gorman 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.
>
> > 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.
>
> > 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@xxxxxxxxx>
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@xxxxxxxxx>
---
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;
/* 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);
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);
/* 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 */
--
1.5.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
