| Message ID | 1252606547.30150.304.camel@rc-desk (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thursday 10 September 2009, reinette chatre wrote: > From: Reinette Chatre <reinette.chatre@intel.com> > Date: Wed, 9 Sep 2009 15:41:00 -0700 > Subject: [PATCH] iwlwifi: reduce noise when skb allocation fails Looks good to me (from a user's perspective). IIUC the first message is now only displayed if IWL debugging is explicitly enabled, and the second only if there really is danger of it affecting data transfer and has been made more informative too. It seems to me that, with debugging enabled, the "Failed to allocate SKB buffer." message may get repeated, but I guess that's minor. One nitpick. As you've made the message into sentences, "Only %u free buffers remaining" should IMO also end with a period. Thanks, FJP -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2009-09-10 at 11:43 -0700, Frans Pop wrote: > Looks good to me (from a user's perspective). Thank you very much for looking at it. > IIUC the first message is now only displayed if IWL debugging is > explicitly enabled, Correct. User will have to enable debug flag of 0x1. > It seems to me that, with debugging enabled, the "Failed to allocate SKB > buffer." message may get repeated, but I guess that's minor. Yes, it will be repeated. I did add a "net_ratelimit()" to it so it should not be too overwhelming. I did not care to limit it even more since we are now talking about a debug message as opposed to an error message on the console. > One nitpick. As you've made the message into sentences, "Only %u free > buffers remaining" should IMO also end with a period. Sorry - will fix. I will not post a new version to this thread for this issue, but it will be fixed in the next version I send out. Reinette -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@intel.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@intel.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@csn.ul.ie> Thanks very much.
On Fri, 2009-09-11 at 01:45 -0700, Mel Gorman wrote: > 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@csn.ul.ie> Thank you very much. I'll make the changes you suggested. Reinette -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */