On Wed, Aug 26, 2009 at 10:27:41AM +0200, Johannes Weiner wrote: > [Cc netdev] > > On Wed, Aug 26, 2009 at 09:09:44AM +0300, Pekka Enberg wrote: > > On Tue, Aug 25, 2009 at 11:34 PM, Rafael J. Wysocki<rjw@xxxxxxx> wrote: > > > This message has been generated automatically as a part of a report > > > of recent regressions. > > > > > > The following bug entry is on the current list of known regressions > > > from 2.6.30. Please verify if it still should be listed and let me know > > > (either way). > > > > > > Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=14016 > > > Subject : mm/ipw2200 regression > > > Submitter : Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx> > > > Date : 2009-08-15 16:56 (11 days old) > > > References : http://marc.info/?l=linux-kernel&m=125036437221408&w=4 > > > > If am reading the page allocator dump correctly, there's plenty of > > pages left but we're unable to satisfy an order 6 allocation. There's > > no slab allocator involved so the page allocator changes that went > > into 2.6.31 seem likely. Mel, ideas? > > It's an atomic order-6 allocation, the chances for this to succeed > after some uptime become infinitesimal. The chunks > order-2 are > pretty much exhausted on this dump. > > 64 pages, presumably 256k, for fw->boot_size while current ipw > firmware images have ~188k. I don't know jack squat about this > driver, but given the field name and the struct: > > struct ipw_fw { > __le32 ver; > __le32 boot_size; > __le32 ucode_size; > __le32 fw_size; > u8 data[0]; > }; > > fw->boot_size alone being that big sounds a bit fishy to me. > Agreed. While there are a low number of order-6 pages free in the page allocation failure dump, there are not enough for watermarks to be satisified. As it's atomic, there is little that can be done from a VM perspective and it's the responsibility of the driver. I'm no driver expert but I'll have a go at fixing it anyway. My reading of this is that the firmware is being loaded from a workqueue and I am failing to see any restriction on sleeping in the path. It would appear that the driver just used the most convenient *_alloc_coherent function available forgetting that it assumes GFP_ATOMIC. Can someone who does know which way is up with a driver tell me why the patch below might not work? Bartlomiej, any chance you could give this a spin? Preferably, you'd have preempt enabled and CONFIG_DEBUG_SPINLOCK_SLEEP on as well because that combination will complain loudly if we really can't sleep in this path. ===== ipw2200: Avoid large GFP_ATOMIC allocation during firmware loading ipw2200 uses pci_alloc_consistent() to allocate a large coherent buffer for the loading of firmware which is an order-6 allocation of GFP_ATOMIC. At system start-up time, this is not a problem. However, the firmware on the card can get confused and the corrective action taken is to reload the firmware and reinit the card. High-order GFP_ATOMIC allocations of this type can and will fail when the system is already up and running. As the firmware is loaded from a workqueue, it should be possible for the driver to go to sleep. This patch converts the call of pci_alloc_consistent() which assumes GFP_ATOMIC to dma_alloc_coherent() which can specify its own flags. The big downside with this patch is that it uses GFP_REPEAT to avoid the driver unloading. There is potential that this will cause a reclaim storm as the machine tries to find a free order-6 buffer. A suggested alternative for the driver owner is in the comments. Signed-off-by: Mel Gorman <mel@xxxxxxxxx> --- drivers/net/wireless/ipw2x00/ipw2200.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c index 44c29b3..f2e251e 100644 --- a/drivers/net/wireless/ipw2x00/ipw2200.c +++ b/drivers/net/wireless/ipw2x00/ipw2200.c @@ -3167,7 +3167,19 @@ static int ipw_load_firmware(struct ipw_priv *priv, u8 * data, size_t len) u8 *shared_virt; IPW_DEBUG_TRACE("<< : \n"); - shared_virt = pci_alloc_consistent(priv->pci_dev, len, &shared_phys); + + /* + * This is a whopping large allocation, in or around order-6 so + * dma_alloc_coherent is used to specify the GFP_KERNEL|__GFP_REPEAT + * flags. Note that this action means the system could go into a + * reclaim loop until it cannot reclaim any more trying to satisfy + * the allocation. It would be preferable if one buffer is allocated + * at driver initialisation and reused when the firmware needs to + * be reloaded, overwriting the existing firmware each time + */ + shared_virt = dma_alloc_coherent( + priv->pci_dev == NULL ? NULL : &priv->pci_dev->dev, + len, &shared_phys, GFP_KERNEL|__GFP_REPEAT); if (!shared_virt) return -ENOMEM; -- To unsubscribe from this list: send the line "unsubscribe kernel-testers" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
