| Message ID | 1252526738.30150.91.camel@rc-desk (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Reinette, On Wednesday 09 September 2009, reinette chatre wrote: > 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. 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. > > 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. > > How about the patch below? A couple of comments/suggestions. If the driver recovers cleanly from the error, shouldn't the priority of the message be lowered from CRIT to INFO. Or maybe even DEBUG; AFAIK most distros will have DEBUG_KERNEL enabled by default. This means it will still show up in logs, but not the console. Shouldn't the message include some indication that the driver will recover by itself and that no user action is needed? This will help avoid users (unnecessarily) reporting this as a bug when it does occur. I expect that users would still report it if they get flooded with the message (or ratelimit warnings for it), which I think is what you'd want. And maybe s/Can not/Cannot/ or s/Can not/Failed to/. Cheers, 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 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 */ > >
On Thu, 2009-09-10 at 02:02 -0700, Mel Gorman wrote: > > 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. I looked more at the implementation and hardware interface but I do not see a way around this. We have to provide 8k buffer to device, and we have to make sure it is aligned. Do you have any suggestions? 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 02:14:50PM -0700, reinette chatre wrote: > On Thu, 2009-09-10 at 02:02 -0700, Mel Gorman wrote: > > > > > 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. > > I looked more at the implementation and hardware interface but I do not > see a way around this. We have to provide 8k buffer to device, and we > have to make sure it is aligned. > That would imply an order-1 allocation instead of an order-2 though so it would appear than we are being worse than we have to. It would appear to be because of this +256 bytes that goes onto every buffer. > Do you have any suggestions? > Nothing concrete. Finding an alternative to having the socket buffer 8192+256 to make it an order-1 allocation would be an improvement but I don't know how that should be tackled. Lacking the hardware, I can't experiment myself :(
On Fri, 2009-09-11 at 16:47 +0800, Mel Gorman wrote: > On Thu, Sep 10, 2009 at 02:14:50PM -0700, reinette chatre wrote: > > On Thu, 2009-09-10 at 02:02 -0700, Mel Gorman wrote: > > > > > > > > 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. > > > > I looked more at the implementation and hardware interface but I do not > > see a way around this. We have to provide 8k buffer to device, and we > > have to make sure it is aligned. > > > > That would imply an order-1 allocation instead of an order-2 though so > it would appear than we are being worse than we have to. It would appear > to be because of this +256 bytes that goes onto every buffer. > > > Do you have any suggestions? > > > > Nothing concrete. Finding an alternative to having the socket buffer > 8192+256 to make it an order-1 allocation would be an improvement but I > don't know how that should be tackled. Lacking the hardware, I can't > experiment myself :( Essentially, the hardware only requires an order-1 allocation aligned on 256 bytes boundary. But as it is used as an SKB, a trailing struct skb_shared_info is added. This forces us to both increase the order and do alignment ourselves. I believe some improvement could be done here. But it should not be an easy one. BTW, does SLAB/SLUB guarantee size of multiple PAGE_SIZE __kmalloc() allocation align on PAGE_SIZE (or 256 bytes) boundary? Thanks, -yi -- 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 Mon, Sep 14, 2009 at 11:01:10AM +0800, Zhu Yi wrote: > On Fri, 2009-09-11 at 16:47 +0800, Mel Gorman wrote: > > On Thu, Sep 10, 2009 at 02:14:50PM -0700, reinette chatre wrote: > > > On Thu, 2009-09-10 at 02:02 -0700, Mel Gorman wrote: > > > > > > > > > > > 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. > > > > > > I looked more at the implementation and hardware interface but I do not > > > see a way around this. We have to provide 8k buffer to device, and we > > > have to make sure it is aligned. > > > > > > > That would imply an order-1 allocation instead of an order-2 though so > > it would appear than we are being worse than we have to. It would appear > > to be because of this +256 bytes that goes onto every buffer. > > > > > Do you have any suggestions? > > > > > > > Nothing concrete. Finding an alternative to having the socket buffer > > 8192+256 to make it an order-1 allocation would be an improvement but I > > don't know how that should be tackled. Lacking the hardware, I can't > > experiment myself :( > > Essentially, the hardware only requires an order-1 allocation aligned on > 256 bytes boundary. But as it is used as an SKB, a trailing struct > skb_shared_info is added. This forces us to both increase the order and > do alignment ourselves. I believe some improvement could be done here. > But it should not be an easy one. > Probably not. I can only assume that moving the location of skb_shared_info such that it is sometimes located after the buffer and sometimes allocated via a separate kmalloc() would be a significant undertaking. > BTW, does SLAB/SLUB guarantee size of multiple PAGE_SIZE __kmalloc() > allocation align on PAGE_SIZE (or 256 bytes) boundary? > Page-aligned.
On Mon, 14 Sep 2009, Zhu Yi wrote: > BTW, does SLAB/SLUB guarantee size of multiple PAGE_SIZE __kmalloc() > allocation align on PAGE_SIZE (or 256 bytes) boundary? Page allocators guarantee page aligned data. Slab allocators do not. You can create a slab that aligns objects on 256 byte boundary if you want. -- 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 Mon, Sep 14, 2009 at 11:42:15AM -0400, Christoph Lameter wrote: > On Mon, 14 Sep 2009, Zhu Yi wrote: > > > BTW, does SLAB/SLUB guarantee size of multiple PAGE_SIZE __kmalloc() > > allocation align on PAGE_SIZE (or 256 bytes) boundary? > > Page allocators guarantee page aligned data. Slab allocators do not. > > You can create a slab that aligns objects on 256 byte boundary if you > want. > The allocation of buffers >= PAGE_SIZE even when allocated with kmalloc() are page-aligned though right? It's not an explicit guarantee but is it not always the case?
On Mon, 14 Sep 2009, Mel Gorman wrote: > The allocation of buffers >= PAGE_SIZE even when allocated with > kmalloc() are page-aligned though right? It's not an explicit guarantee > but is it not always the case? Its typically the case but debug options may misalign the object. The guarantee for kmalloc caches is to be cacheline aligned. -- 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
[ Cc netdev and change the subject ] On Mon, 2009-09-14 at 21:06 +0800, Mel Gorman wrote: > > Essentially, the hardware only requires an order-1 allocation aligned on > > 256 bytes boundary. But as it is used as an SKB, a trailing struct > > skb_shared_info is added. This forces us to both increase the order and > > do alignment ourselves. I believe some improvement could be done here. > > But it should not be an easy one. > > > > Probably not. I can only assume that moving the location of > skb_shared_info such that it is sometimes located after the buffer and > sometimes allocated via a separate kmalloc() would be a significant > undertaking. Shall I propose below function as a variant to alloc_skb()? struct sk_buff *alloc_skb_attach_buff(void *data_buff, unsigned int real_size, unsigned int size, gfp_t mask); If real_size >= size + sizeof(struct skb_shared_info), we can still put the shinfo at the end of the buffer. Otherwise we can allocate from a new slab that put sk_buff and shinfo together. Of course, kfree_skbmem() needs to recognize it. This way, device drivers can allocate the Rx buffers with their own size and alignment requirement. i.e. do an order-1 page allocation directly with free_pages() in the iwlagn driver for a 256 bytes aligned 8K Rx buffer. After DMA is finished, drivers can use the above function to assemble an skb based on the Rx buffer. It should resolve the problem for requiring an order-2 allocation by alloc_skb() in the first place. Comments are welcome. Thanks, -yi -- 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
From: Zhu Yi <yi.zhu@intel.com> Date: Tue, 15 Sep 2009 16:30:20 +0800 > This way, device drivers can allocate the Rx buffers with their own size > and alignment requirement. i.e. do an order-1 page allocation directly > with free_pages() in the iwlagn driver for a 256 bytes aligned 8K Rx > buffer. After DMA is finished, drivers can use the above function to > assemble an skb based on the Rx buffer. It should resolve the problem > for requiring an order-2 allocation by alloc_skb() in the first place. You can create paged RX skbs just like drivers such as niu.c and others already do, there is no need for special APIs for this. -- 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 Tue, 2009-09-15 at 16:33 +0800, David Miller wrote: > From: Zhu Yi <yi.zhu@intel.com> > Date: Tue, 15 Sep 2009 16:30:20 +0800 > > > This way, device drivers can allocate the Rx buffers with their own size > > and alignment requirement. i.e. do an order-1 page allocation directly > > with free_pages() in the iwlagn driver for a 256 bytes aligned 8K Rx > > buffer. After DMA is finished, drivers can use the above function to > > assemble an skb based on the Rx buffer. It should resolve the problem > > for requiring an order-2 allocation by alloc_skb() in the first place. > > You can create paged RX skbs just like drivers such as niu.c > and others already do, there is no need for special APIs for > this. Thanks. So we can put the 8K buffer into 2 skb_shinfo()->frags[] slots and set nr_frags to 2, right? Is this supported allover the network code already? At a first glance, I didn't find any frags handling in mac80211 stack. Thanks, -yi -- 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
From: Zhu Yi <yi.zhu@intel.com> Date: Tue, 15 Sep 2009 16:57:29 +0800 > Thanks. So we can put the 8K buffer into 2 skb_shinfo()->frags[] slots > and set nr_frags to 2, right? Is this supported allover the network code > already? At a first glance, I didn't find any frags handling in mac80211 > stack. You have to pre-pull the link level protocol headers into the linear area, but that's it. Again, see niu.c for details, it does: static void niu_rx_skb_append(struct sk_buff *skb, struct page *page, u32 offset, u32 size) { int i = skb_shinfo(skb)->nr_frags; skb_frag_t *frag = &skb_shinfo(skb)->frags[i]; frag->page = page; frag->page_offset = offset; frag->size = size; skb->len += size; skb->data_len += size; skb->truesize += size; skb_shinfo(skb)->nr_frags = i + 1; } to add pages to SKBs and then at the end it goes: skb_reserve(skb, NET_IP_ALIGN); __pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX)); Right before giving the SKB to the networking stack. NIU_RXPULL_MAX should be a value that will be large enough to cover the largest possible link level header. -- 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 Tue, 2009-09-15 at 17:09 +0800, David Miller wrote: > From: Zhu Yi <yi.zhu@intel.com> > Date: Tue, 15 Sep 2009 16:57:29 +0800 > > > Thanks. So we can put the 8K buffer into 2 skb_shinfo()->frags[] slots > > and set nr_frags to 2, right? Is this supported allover the network code > > already? At a first glance, I didn't find any frags handling in mac80211 > > stack. > > You have to pre-pull the link level protocol headers into the > linear area, but that's it. > > Again, see niu.c for details, it does: > > static void niu_rx_skb_append(struct sk_buff *skb, struct page *page, > u32 offset, u32 size) > { > int i = skb_shinfo(skb)->nr_frags; > skb_frag_t *frag = &skb_shinfo(skb)->frags[i]; > > frag->page = page; > frag->page_offset = offset; > frag->size = size; > > skb->len += size; > skb->data_len += size; > skb->truesize += size; > > skb_shinfo(skb)->nr_frags = i + 1; > } > > to add pages to SKBs and then at the end it goes: > > skb_reserve(skb, NET_IP_ALIGN); > __pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX)); > > Right before giving the SKB to the networking stack. NIU_RXPULL_MAX > should be a value that will be large enough to cover the largest > possible link level header. I see. Thanks for this info. I'll try implementing the same for iwlagn. Thanks, -yi -- 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 Tue, 2009-09-15 at 17:15 +0800, Zhu Yi wrote: > On Tue, 2009-09-15 at 17:09 +0800, David Miller wrote: > > From: Zhu Yi <yi.zhu@intel.com> > > Date: Tue, 15 Sep 2009 16:57:29 +0800 > > > > > Thanks. So we can put the 8K buffer into 2 skb_shinfo()->frags[] slots > > > and set nr_frags to 2, right? Is this supported allover the network code > > > already? At a first glance, I didn't find any frags handling in mac80211 > > > stack. > > > > You have to pre-pull the link level protocol headers into the > > linear area, but that's it. > > > > Again, see niu.c for details, it does: > > > > static void niu_rx_skb_append(struct sk_buff *skb, struct page *page, > > u32 offset, u32 size) > > { > > int i = skb_shinfo(skb)->nr_frags; > > skb_frag_t *frag = &skb_shinfo(skb)->frags[i]; > > > > frag->page = page; > > frag->page_offset = offset; > > frag->size = size; > > > > skb->len += size; > > skb->data_len += size; > > skb->truesize += size; > > > > skb_shinfo(skb)->nr_frags = i + 1; > > } > > > > to add pages to SKBs and then at the end it goes: > > > > skb_reserve(skb, NET_IP_ALIGN); > > __pskb_pull_tail(skb, min(len, NIU_RXPULL_MAX)); > > > > Right before giving the SKB to the networking stack. NIU_RXPULL_MAX > > should be a value that will be large enough to cover the largest > > possible link level header. > > I see. Thanks for this info. I'll try implementing the same for iwlagn. Hold, mac80211 can't cope with that at this point for sw crypto and possibly other things. johannes
From: Johannes Berg <johannes@sipsolutions.net> Date: Tue, 15 Sep 2009 08:30:31 -0700 > Hold, mac80211 can't cope with that at this point for sw crypto and > possibly other things. Then it should skb_linearize() at input, or similar. -- 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 Tue, 2009-09-15 at 14:16 -0700, David Miller wrote: > From: Johannes Berg <johannes@sipsolutions.net> > Date: Tue, 15 Sep 2009 08:30:31 -0700 > > > Hold, mac80211 can't cope with that at this point for sw crypto and > > possibly other things. > > Then it should skb_linearize() at input, or similar. Not all that much won then since that again means an order-2 allocation. Fewer, certainly, as most packets are actually much smaller than that. johannes
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); if (!skb) { - IWL_CRIT(priv, "Can not allocate SKB buffers\n"); + if (net_ratelimit()) + 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 */ 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 */