request_firmware API exhaust memory

request_firmware API exhaust memory

[Tomas Winkler]

Lately we've been developing a device that rather more extensively
used request_firmware API in load and also using pm_notifiers to load
firmware. In some stress  testing the usage will exhaust the system
memory. First we suspected there is a memory leak in the
firwmare_class but eventually what is that udevd piles up it takes
time to collect the process. We loose ~ 500K for each
request/release_firmware iteration.
After the stress the process table will will look something like shown below,

If someone with more insight can point out what part of this firmware
load has to be fixed and what is the best strategy request_firmware
API used extensively.  I believe currently only Orinoco driver uses
pm_notifies to load firmware before S3/S4 but if there are more
drivers like that it might actually create a real problem during
suspend on some small systems.

root       514  0.0  0.0  15480  1116 ?        S<s  Apr01   0:01 /sbin/udevd -d
root     28989  0.0  0.0  15476   968 ?        S<   14:07   0:00 /sbin/udevd -d
root     28995  0.0  0.0  15476   976 ?        S<   14:07   0:00 /sbin/udevd -d
root     31616  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31622  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31627  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31633  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31638  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31644  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31649  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31655  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31660  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31666  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31671  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31677  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31682  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31688  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31693  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31699  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31704  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31710  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31715  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31721  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31726  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31732  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31737  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31743  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31748  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31754  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31759  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31765  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31770  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31776  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31781  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31787  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31792  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31798  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31803  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31809  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31814  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31820  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31825  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31831  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31836  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31842  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d
root     31847  0.0  0.0  15476   916 ?        S<   14:49   0:00 /sbin/udevd -d
root     31853  0.0  0.0  15476   888 ?        S<   14:49   0:00 /sbin/udevd -d

Re: request_firmware API exhaust memory

[Kay Sievers]

On Mon, Apr 19, 2010 at 14:20, Tomas Winkler <tomasw@gmail.com> wrote:
> Lately we've been developing a device that rather more extensively
> used request_firmware API in load and also using pm_notifiers to load
> firmware. In some stress  testing the usage will exhaust the system
> memory. First we suspected there is a memory leak in the
> firwmare_class but eventually what is that udevd piles up it takes
> time to collect the process.

What version of the kernel, and what version of udev?

> We loose ~ 500K for each
> request/release_firmware iteration.

You mean you lose 500k, not taken by any process?

> After the stress the process table will will look something like shown below,

What are these processes doing (strace)? Do they have child processes?
If yes, what are they doing (strace)?

Kay

Re: request_firmware API exhaust memory

[Greg KH]

On Mon, Apr 19, 2010 at 03:20:34PM +0300, Tomas Winkler wrote:
> Lately we've been developing a device that rather more extensively
> used request_firmware API in load and also using pm_notifiers to load
> firmware.

Do you have a pointer to your driver source anywhere that shows how you
are trying to use the firmware api in this manner?

thanks,

greg k-h

Re: request_firmware API exhaust memory

[Tomas Winkler]

[-- Attachment #1: Type: text/plain, Size: 1105 bytes --]

On Mon, Apr 19, 2010 at 5:59 PM, Greg KH <greg@kroah.com> wrote:
> On Mon, Apr 19, 2010 at 03:20:34PM +0300, Tomas Winkler wrote:
>> Lately we've been developing a device that rather more extensively
>> used request_firmware API in load and also using pm_notifiers to load
>> firmware.
>
> Do you have a pointer to your driver source anywhere that shows how you
> are trying to use the firmware api in this manner?

I've attached a very simple  test driver I'm using.  Just wanted to
eliminate anything else.
Bellow is a little script that loads and releases the firmware. My
previous observation was wrong.
The free memory gradually decreases regardless of number or dangling
udevd forks, which are eventually collected if the sleep period is
long enough ~10s.

testfw=${1:-test-fw600k.fw}
count=${2:-100}
s=${3:-10}
for ((i=0; i<$count ; i++)) ; do
        echo -n $testfw > /sys/devices/platform/fw-test/load_fw
        echo -n 1 > /sys/devices/platform/fw-test/release_fw
        sleep $s
        grep MemFree /proc/meminfo | awk '{print $2}'
        ps auxwww | grep udevd | wc -l
done

Thanks
Tomas

[-- Attachment #2: fw-test.c --]
[-- Type: application/octet-stream, Size: 3305 bytes --]

/*
 *  fw-test.c -FW Test Driver
 *
 *
 *  Copyright (C) 2010, Intel Corporation. All rights reserved.
 *
 *  This program is free software; you can redistribute it and/or modify
 *  it under the terms of the GNU General Public License as published by
 *  the Free Software Foundation; either version 2 of the License, or
 *  (at your option) any later version.
 *
 *  This program is distributed in the hope that it will be useful,
 *  but WITHOUT ANY WARRANTY; without even the implied warranty of
 *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *  GNU General Public License for more details.
 *
 *  You should have received a copy of the GNU General Public License
 *  along with this program; if not, write to the Free Software
 *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 *
 *
 *  TODO
 *
 */

#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/types.h>
#include <linux/proc_fs.h>
#include <linux/backlight.h>
#include <linux/platform_device.h>
#include <linux/sysfs.h>
#include <linux/firmware.h>


MODULE_AUTHOR("Tomas Winkler");
MODULE_DESCRIPTION("Firmware API Test Driver");
MODULE_LICENSE("GPL");

struct fw_test_dev {
	struct platform_device *p_dev;
	struct mutex mutex;
	const struct firmware *fw;
};

static struct fw_test_dev fw_test;

static ssize_t load_fw(struct device *d, struct device_attribute *attr,
		const char *buf, size_t count)
{
	struct fw_test_dev *priv = &fw_test;
	ssize_t ret;

	mutex_lock(&fw_test.mutex);
	ret = request_firmware(&priv->fw, buf, d);
	if (ret)
		printk(KERN_ERR "request fail %s ret=%zd", buf, ret);
	else 
		ret = count;
	mutex_unlock(&fw_test.mutex);

	printk(KERN_INFO "FW-TEST LOAD");
	return ret;
}

static ssize_t release_fw(struct device *d, struct device_attribute *attr,
		const char *buf, size_t count)
{
	struct fw_test_dev *priv = &fw_test;
	
	mutex_lock(&fw_test.mutex);
	release_firmware(priv->fw);
	priv->fw = NULL;
	mutex_unlock(&fw_test.mutex);
	printk(KERN_INFO "FW-TEST RELEASE");
	
	return count;
}

static DEVICE_ATTR(load_fw, S_IWUGO, NULL, load_fw);

static DEVICE_ATTR(release_fw, S_IWUGO, NULL, release_fw);

static struct attribute *fw_test_sysfs_entries[] = {
	&dev_attr_load_fw.attr,
	&dev_attr_release_fw.attr,
	NULL
};

static struct attribute_group fw_test_attribute_group = {
	.name = NULL,		/* put in device directory */
	.attrs = fw_test_sysfs_entries,
};

static void fw_test_exit(void)
{

	sysfs_remove_group(&fw_test.p_dev->dev.kobj, &fw_test_attribute_group);
	platform_device_unregister(fw_test.p_dev);
	printk(KERN_INFO "FW API Test Driver Unloaded");

	return;
}

static int __init fw_test_init(void)
{

	int ret;

	mutex_init(&fw_test.mutex);

	fw_test.p_dev = platform_device_register_simple("fw-test", -1, NULL, 0);
	if (IS_ERR(fw_test.p_dev)) {
		ret = PTR_ERR(fw_test.p_dev);
		printk(KERN_ERR "unable to register platform device\n");
		fw_test.p_dev = NULL;
		fw_test_exit();
		return ret;
	}
	ret = sysfs_create_group(&fw_test.p_dev->dev.kobj, &fw_test_attribute_group);
	if (ret) {
		printk(KERN_ERR "Failed to allocate SYSFS entry");
		fw_test.p_dev = NULL;
		fw_test_exit();
		return ret;
	}
	printk(KERN_INFO "FW API Test Driver Loaded");
	return 0;

}

module_init(fw_test_init);
module_exit(fw_test_exit);

Re: request_firmware API exhaust memory

[Greg KH]

On Thu, Apr 22, 2010 at 01:22:51AM +0300, Tomas Winkler wrote:
> On Mon, Apr 19, 2010 at 5:59 PM, Greg KH <greg@kroah.com> wrote:
> > On Mon, Apr 19, 2010 at 03:20:34PM +0300, Tomas Winkler wrote:
> >> Lately we've been developing a device that rather more extensively
> >> used request_firmware API in load and also using pm_notifiers to load
> >> firmware.
> >
> > Do you have a pointer to your driver source anywhere that shows how you
> > are trying to use the firmware api in this manner?
> 
> I've attached a very simple  test driver I'm using.  Just wanted to
> eliminate anything else.
> Bellow is a little script that loads and releases the firmware. My
> previous observation was wrong.
> The free memory gradually decreases regardless of number or dangling
> udevd forks, which are eventually collected if the sleep period is
> long enough ~10s.

That sounds normal for the free memory.  Kay, that's also to be expected
for the udevd forks as well, right?

thanks,

greg k-h

Re: request_firmware API exhaust memory

[Tomas Winkler]

On Sun, Apr 25, 2010 at 7:37 PM, Greg KH <greg@kroah.com> wrote:
> On Thu, Apr 22, 2010 at 01:22:51AM +0300, Tomas Winkler wrote:
>> On Mon, Apr 19, 2010 at 5:59 PM, Greg KH <greg@kroah.com> wrote:
>> > On Mon, Apr 19, 2010 at 03:20:34PM +0300, Tomas Winkler wrote:
>> >> Lately we've been developing a device that rather more extensively
>> >> used request_firmware API in load and also using pm_notifiers to load
>> >> firmware.
>> >
>> > Do you have a pointer to your driver source anywhere that shows how you
>> > are trying to use the firmware api in this manner?
>>
>> I've attached a very simple  test driver I'm using.  Just wanted to
>> eliminate anything else.
>> Bellow is a little script that loads and releases the firmware. My
>> previous observation was wrong.
>> The free memory gradually decreases regardless of number or dangling
>> udevd forks, which are eventually collected if the sleep period is
>> long enough ~10s.
>
> That sounds normal for the free memory.  Kay, that's also to be expected
> for the udevd forks as well, right?

Sorry maybe I was not clear what I mean that the memory will be
eventually exhausted and system will crash
Is this normal? Actually I less suspect now udevd as the same happens
on android platform where there is no udev


Tomas

Re: request_firmware API exhaust memory

[Greg KH]

On Sun, Apr 25, 2010 at 10:22:54PM +0300, Tomas Winkler wrote:
> On Sun, Apr 25, 2010 at 7:37 PM, Greg KH <greg@kroah.com> wrote:
> > On Thu, Apr 22, 2010 at 01:22:51AM +0300, Tomas Winkler wrote:
> >> On Mon, Apr 19, 2010 at 5:59 PM, Greg KH <greg@kroah.com> wrote:
> >> > On Mon, Apr 19, 2010 at 03:20:34PM +0300, Tomas Winkler wrote:
> >> >> Lately we've been developing a device that rather more extensively
> >> >> used request_firmware API in load and also using pm_notifiers to load
> >> >> firmware.
> >> >
> >> > Do you have a pointer to your driver source anywhere that shows how you
> >> > are trying to use the firmware api in this manner?
> >>
> >> I've attached a very simple ??test driver I'm using. ??Just wanted to
> >> eliminate anything else.
> >> Bellow is a little script that loads and releases the firmware. My
> >> previous observation was wrong.
> >> The free memory gradually decreases regardless of number or dangling
> >> udevd forks, which are eventually collected if the sleep period is
> >> long enough ~10s.
> >
> > That sounds normal for the free memory. ??Kay, that's also to be expected
> > for the udevd forks as well, right?
> 
> Sorry maybe I was not clear what I mean that the memory will be
> eventually exhausted and system will crash
> Is this normal?

Ah, no, that's not normal.  Have you run kmemleak on your module (or
test module) to verify that you are properly freeing up the memory?

> Actually I less suspect now udevd as the same happens on android
> platform where there is no udev

Which is a sad thing for a whole other range of issues...

thanks,

greg k-h

Re: request_firmware API exhaust memory

[Tomas Winkler]

On Sun, Apr 25, 2010 at 10:36 PM, Greg KH <greg@kroah.com> wrote:
> On Sun, Apr 25, 2010 at 10:22:54PM +0300, Tomas Winkler wrote:
>> On Sun, Apr 25, 2010 at 7:37 PM, Greg KH <greg@kroah.com> wrote:
>> > On Thu, Apr 22, 2010 at 01:22:51AM +0300, Tomas Winkler wrote:
>> >> On Mon, Apr 19, 2010 at 5:59 PM, Greg KH <greg@kroah.com> wrote:
>> >> > On Mon, Apr 19, 2010 at 03:20:34PM +0300, Tomas Winkler wrote:
>> >> >> Lately we've been developing a device that rather more extensively
>> >> >> used request_firmware API in load and also using pm_notifiers to load
>> >> >> firmware.
>> >> >
>> >> > Do you have a pointer to your driver source anywhere that shows how you
>> >> > are trying to use the firmware api in this manner?
>> >>
>> >> I've attached a very simple ??test driver I'm using. ??Just wanted to
>> >> eliminate anything else.
>> >> Bellow is a little script that loads and releases the firmware. My
>> >> previous observation was wrong.
>> >> The free memory gradually decreases regardless of number or dangling
>> >> udevd forks, which are eventually collected if the sleep period is
>> >> long enough ~10s.
>> >
>> > That sounds normal for the free memory. ??Kay, that's also to be expected
>> > for the udevd forks as well, right?
>>
>> Sorry maybe I was not clear what I mean that the memory will be
>> eventually exhausted and system will crash
>> Is this normal?
>
> Ah, no, that's not normal.  Have you run kmemleak on your module (or
> test module) to verify that you are properly freeing up the memory?

yes, one of my college has run the kmemleak but didn't bring much
evidence.  I've also looked into slubinfo as suggested by Johannes
but don't see anything suspicions either but this is expected as
everything is allocated through kmalloc and alloc_pages.
I may rerun the kmemleak by myself but this shows up on too many
setups and kernels with also full driver and also my simple test
driver.

>
>> Actually I less suspect now udevd as the same happens on android
>> platform where there is no udev
>
> Which is a sad thing for a whole other range of issues...

Said thing is that I don't see where the memory goes.... Anyhow I will
try to run valgrin on udev just to be sure.

I'll be glad If someone can run my simple driver I posted and confirm
that sees the same problem

Thanks
Tomas

Re: request_firmware API exhaust memory

[Kay Sievers]

On Sun, Apr 25, 2010 at 22:09, Tomas Winkler <tomasw@gmail.com> wrote:
> Said thing is that I don't see where the memory goes.... Anyhow I will
> try to run valgrin on udev just to be sure.

Nah, that memory would be freed, if you kill all udev processes, which
it doesn't.

The many udev worker processes you see for a few seconds was caused by
udevd handling events with TIMEOUT= set special. We need to make sure,
that firmware events run immediately and don't wait for other
processes to finish. The logic who does that was always creating a new
worker. I changed this now, but this will not affect the underlying
problem you are seeing, it will just make the udev workers not grow in
a timeframe of less than 10 seconds. The change is here:
  http://git.kernel.org/?p=linux/hotplug/udev.git;a=commit;h=665ee17def2caa6811ae032ae68ebf8239a18cf8
but as mentioned, this change is unrelated to the memory leak you are seeing.

> I'll be glad If someone can run my simple driver I posted and confirm
> that sees the same problem

I can confirm that memory gets lost. I suspect for some reason the
firmware does not get properly cleaned up. If you increase the size of
the firmware image, it will leak memory much faster.

Kay

Re: request_firmware API exhaust memory

[Kay Sievers]

On Mon, 2010-04-26 at 12:38 +0200, Kay Sievers wrote:
> On Sun, Apr 25, 2010 at 22:09, Tomas Winkler <tomasw@gmail.com> wrote:
> > Said thing is that I don't see where the memory goes.... Anyhow I will
> > try to run valgrin on udev just to be sure.
> 
> Nah, that memory would be freed, if you kill all udev processes, which
> it doesn't.
> 
> The many udev worker processes you see for a few seconds was caused by
> udevd handling events with TIMEOUT= set special. We need to make sure,
> that firmware events run immediately and don't wait for other
> processes to finish. The logic who does that was always creating a new
> worker. I changed this now, but this will not affect the underlying
> problem you are seeing, it will just make the udev workers not grow in
> a timeframe of less than 10 seconds. The change is here:
>   http://git.kernel.org/?p=linux/hotplug/udev.git;a=commit;h=665ee17def2caa6811ae032ae68ebf8239a18cf8
> but as mentioned, this change is unrelated to the memory leak you are seeing.
> 
> > I'll be glad If someone can run my simple driver I posted and confirm
> > that sees the same problem
> 
> I can confirm that memory gets lost. I suspect for some reason the
> firmware does not get properly cleaned up. If you increase the size of
> the firmware image, it will leak memory much faster.

I guess, the assumption that vfree() will free pages which are allocated
by custom code, and not by vmalloc(), is not true.

The attached changes seem to fix the issue for me.

The custom page array mangling was introduced by David as an optimization
with commit 6e03a201bbe8137487f340d26aa662110e324b20 and this should be
checked, and if needed be fixed.

Cheers,
Kay


diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 985da11..fe4e872 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -162,7 +162,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 			mutex_unlock(&fw_lock);
 			break;
 		}
-		vfree(fw_priv->fw->data);
+		vunmap(fw_priv->fw->data);
 		fw_priv->fw->data = NULL;
 		for (i = 0; i < fw_priv->nr_pages; i++)
 			__free_page(fw_priv->pages[i]);
@@ -176,7 +176,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 		break;
 	case 0:
 		if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
-			vfree(fw_priv->fw->data);
+			vunmap(fw_priv->fw->data);
 			fw_priv->fw->data = vmap(fw_priv->pages,
 						 fw_priv->nr_pages,
 						 0, PAGE_KERNEL_RO);
@@ -184,9 +184,6 @@ static ssize_t firmware_loading_store(struct device *dev,
 				dev_err(dev, "%s: vmap() failed\n", __func__);
 				goto err;
 			}
-			/* Pages will be freed by vfree() */
-			fw_priv->page_array_size = 0;
-			fw_priv->nr_pages = 0;
 			complete(&fw_priv->completion);
 			clear_bit(FW_STATUS_LOADING, &fw_priv->status);
 			break;
@@ -578,7 +575,7 @@ release_firmware(const struct firmware *fw)
 			if (fw->data == builtin->data)
 				goto free_fw;
 		}
-		vfree(fw->data);
+		vunmap(fw->data);
 	free_fw:
 		kfree(fw);
 	}

Re: request_firmware API exhaust memory

[Tomas Winkler]

On Mon, Apr 26, 2010 at 6:19 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Mon, 2010-04-26 at 12:38 +0200, Kay Sievers wrote:
>> On Sun, Apr 25, 2010 at 22:09, Tomas Winkler <tomasw@gmail.com> wrote:
>> > Said thing is that I don't see where the memory goes.... Anyhow I will
>> > try to run valgrin on udev just to be sure.
>>
>> Nah, that memory would be freed, if you kill all udev processes, which
>> it doesn't.
>>
>> The many udev worker processes you see for a few seconds was caused by
>> udevd handling events with TIMEOUT= set special. We need to make sure,
>> that firmware events run immediately and don't wait for other
>> processes to finish. The logic who does that was always creating a new
>> worker. I changed this now, but this will not affect the underlying
>> problem you are seeing, it will just make the udev workers not grow in
>> a timeframe of less than 10 seconds. The change is here:
>>   http://git.kernel.org/?p=linux/hotplug/udev.git;a=commit;h=665ee17def2caa6811ae032ae68ebf8239a18cf8
>> but as mentioned, this change is unrelated to the memory leak you are seeing.
>>
>> > I'll be glad If someone can run my simple driver I posted and confirm
>> > that sees the same problem
>>
>> I can confirm that memory gets lost. I suspect for some reason the
>> firmware does not get properly cleaned up. If you increase the size of
>> the firmware image, it will leak memory much faster.
>
> I guess, the assumption that vfree() will free pages which are allocated
> by custom code, and not by vmalloc(), is not true.
>
> The attached changes seem to fix the issue for me.
>
> The custom page array mangling was introduced by David as an optimization
> with commit 6e03a201bbe8137487f340d26aa662110e324b20 and this should be
> checked, and if needed be fixed.
>
> Cheers,
> Kay
>
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 985da11..fe4e872 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -162,7 +162,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>                        mutex_unlock(&fw_lock);
>                        break;
>                }
> -               vfree(fw_priv->fw->data);
> +               vunmap(fw_priv->fw->data);
>                fw_priv->fw->data = NULL;
>                for (i = 0; i < fw_priv->nr_pages; i++)
>                        __free_page(fw_priv->pages[i]);
> @@ -176,7 +176,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>                break;
>        case 0:
>                if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
> -                       vfree(fw_priv->fw->data);
> +                       vunmap(fw_priv->fw->data);
>                        fw_priv->fw->data = vmap(fw_priv->pages,
>                                                 fw_priv->nr_pages,
>                                                 0, PAGE_KERNEL_RO);
> @@ -184,9 +184,6 @@ static ssize_t firmware_loading_store(struct device *dev,
>                                dev_err(dev, "%s: vmap() failed\n", __func__);
>                                goto err;
>                        }
> -                       /* Pages will be freed by vfree() */
> -                       fw_priv->page_array_size = 0;
> -                       fw_priv->nr_pages = 0;
>                        complete(&fw_priv->completion);
>                        clear_bit(FW_STATUS_LOADING, &fw_priv->status);
>                        break;
> @@ -578,7 +575,7 @@ release_firmware(const struct firmware *fw)
>                        if (fw->data == builtin->data)
>                                goto free_fw;
>                }
> -               vfree(fw->data);
> +               vunmap(fw->data);
>        free_fw:
>                kfree(fw);
>        }

Thanks for your effort I will test it tomorrow
Tomas

Re: request_firmware API exhaust memory

[Sujith Manoharan]

On Mon, Apr 26, 2010 at 8:49 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> I guess, the assumption that vfree() will free pages which are allocated
> by custom code, and not by vmalloc(), is not true.
>
> The attached changes seem to fix the issue for me.

I have the same issue with ath9k_htc. Repeated module load/unload
results in an eventual panic.

I checked your patch (on top of John Linville's wireless-testing tree)
and was unable to load the driver as FW download to the target failed
after the first iteration.

> -                       /* Pages will be freed by vfree() */
> -                       fw_priv->page_array_size = 0;
> -                       fw_priv->nr_pages = 0;

Am completely ignorant about this code, but this seemed a bit fishy,
so I removed this chunk from the patch and tried. It seemed to be working.

But then, I got this trace. (Which was present before your patch too).
I do see ath9k_htc_txep() in the trace, but am still not sure it
there's a memory leak in the driver.

udevd version: 151

[  606.888511] udevd[4744]: segfault at 7f0fdb19bf80 ip
00007f0fdb4f8231 sp 00007fff9a0cd590 error 5 in
ld-2.11.1.so[7f0fdb4ef000+1e000]
[  606.888556] BUG: Bad page map in process udevd
pte:ffff88007b8b18e0 pmd:7c7e9067
[  606.888559] addr:00007f0fdb095000 vm_flags:08000070 anon_vma:(null)
mapping:ffff88007cc4c998 index:108
[  606.888565] vma->vm_ops->fault: filemap_fault+0x0/0x480
[  606.888569] vma->vm_file->f_op->mmap: generic_file_mmap+0x0/0x60
[  606.888573] Pid: 4744, comm: udevd Tainted: G      D    2.6.34-rc5-wl #32
[  606.888575] Call Trace:
[  606.888581]  [<ffffffff810ad644>] print_bad_pte+0x1a4/0x210
[  606.888584]  [<ffffffff810ae6e1>] unmap_vmas+0x4c1/0x970
[  606.888589]  [<ffffffff810b3b8d>] exit_mmap+0xdd/0x1a0
[  606.888594]  [<ffffffff810396d8>] mmput+0x48/0x100
[  606.888597]  [<ffffffff8103e1c1>] exit_mm+0x101/0x130
[  606.888600]  [<ffffffff8104058c>] do_exit+0x6ac/0x770
[  606.888603]  [<ffffffff8104069c>] do_group_exit+0x4c/0xc0
[  606.888607]  [<ffffffff8104c6f7>] get_signal_to_deliver+0x307/0x4c0
[  606.888612]  [<ffffffff81002180>] do_signal+0x70/0x7c0
[  606.888616]  [<ffffffff81026b9a>] ? do_page_fault+0xca/0x3e0
[  606.888621]  [<ffffffff81301575>] ? printk+0x3c/0x3f
[  606.888624]  [<ffffffff810d9790>] ? do_unlinkat+0x60/0x1c0
[  606.888628]  [<ffffffff81002925>] do_notify_resume+0x55/0x80
[  606.888632]  [<ffffffff81304942>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[  606.888635]  [<ffffffff81305a66>] retint_signal+0x46/0x90
[  606.888638] swap_free: Bad swap file entry c07fffffffd023c3
[  606.888641] BUG: Bad page map in process udevd
pte:ffffffffa04786b0 pmd:7c7e9067
[  606.888644] addr:00007f0fdb096000 vm_flags:08000070 anon_vma:(null)
mapping:ffff88007cc4c998 index:109
[  606.888646] vma->vm_ops->fault: filemap_fault+0x0/0x480
[  606.888649] vma->vm_file->f_op->mmap: generic_file_mmap+0x0/0x60
[  606.888652] Pid: 4744, comm: udevd Tainted: G    B D    2.6.34-rc5-wl #32
[  606.888654] Call Trace:
[  606.888657]  [<ffffffff810ad644>] print_bad_pte+0x1a4/0x210
[  606.888660]  [<ffffffff810ae6e1>] unmap_vmas+0x4c1/0x970
[  606.888668]  [<ffffffffa04786b0>] ? ath9k_htc_txep+0x0/0xe0 [ath9k_htc]
[  606.888672]  [<ffffffff810b3b8d>] exit_mmap+0xdd/0x1a0
[  606.888675]  [<ffffffff810396d8>] mmput+0x48/0x100
[  606.888678]  [<ffffffff8103e1c1>] exit_mm+0x101/0x130
[  606.888681]  [<ffffffff8104058c>] do_exit+0x6ac/0x770
[  606.888684]  [<ffffffff8104069c>] do_group_exit+0x4c/0xc0
[  606.888688]  [<ffffffff8104c6f7>] get_signal_to_deliver+0x307/0x4c0
[  606.888692]  [<ffffffff81002180>] do_signal+0x70/0x7c0
[  606.888695]  [<ffffffff81026b9a>] ? do_page_fault+0xca/0x3e0
[  606.888698]  [<ffffffff81301575>] ? printk+0x3c/0x3f
[  606.888701]  [<ffffffff810d9790>] ? do_unlinkat+0x60/0x1c0
[  606.888705]  [<ffffffff81002925>] do_notify_resume+0x55/0x80
[  606.888708]  [<ffffffff81304942>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[  606.888711]  [<ffffffff81305a66>] retint_signal+0x46/0x90


Sujith

Re: request_firmware API exhaust memory

[Tomas Winkler]

On Mon, Apr 26, 2010 at 6:19 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Mon, 2010-04-26 at 12:38 +0200, Kay Sievers wrote:
>> On Sun, Apr 25, 2010 at 22:09, Tomas Winkler <tomasw@gmail.com> wrote:
>> > Said thing is that I don't see where the memory goes.... Anyhow I will
>> > try to run valgrin on udev just to be sure.
>>
>> Nah, that memory would be freed, if you kill all udev processes, which
>> it doesn't.
>>
>> The many udev worker processes you see for a few seconds was caused by
>> udevd handling events with TIMEOUT= set special. We need to make sure,
>> that firmware events run immediately and don't wait for other
>> processes to finish. The logic who does that was always creating a new
>> worker. I changed this now, but this will not affect the underlying
>> problem you are seeing, it will just make the udev workers not grow in
>> a timeframe of less than 10 seconds. The change is here:
>>   http://git.kernel.org/?p=linux/hotplug/udev.git;a=commit;h=665ee17def2caa6811ae032ae68ebf8239a18cf8
>> but as mentioned, this change is unrelated to the memory leak you are seeing.
>>
>> > I'll be glad If someone can run my simple driver I posted and confirm
>> > that sees the same problem
>>
>> I can confirm that memory gets lost. I suspect for some reason the
>> firmware does not get properly cleaned up. If you increase the size of
>> the firmware image, it will leak memory much faster.
>
> I guess, the assumption that vfree() will free pages which are allocated
> by custom code, and not by vmalloc(), is not true.
>
> The attached changes seem to fix the issue for me.
>
> The custom page array mangling was introduced by David as an optimization
> with commit 6e03a201bbe8137487f340d26aa662110e324b20 and this should be
> checked, and if needed be fixed.
>
> Cheers,
> Kay
>
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 985da11..fe4e872 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -162,7 +162,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>                        mutex_unlock(&fw_lock);
>                        break;
>                }
> -               vfree(fw_priv->fw->data);
> +               vunmap(fw_priv->fw->data);
>                fw_priv->fw->data = NULL;
>                for (i = 0; i < fw_priv->nr_pages; i++)
>                        __free_page(fw_priv->pages[i]);
> @@ -176,7 +176,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>                break;
>        case 0:
>                if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
> -                       vfree(fw_priv->fw->data);
> +                       vunmap(fw_priv->fw->data);
>                        fw_priv->fw->data = vmap(fw_priv->pages,
>                                                 fw_priv->nr_pages,
>                                                 0, PAGE_KERNEL_RO);
> @@ -184,9 +184,6 @@ static ssize_t firmware_loading_store(struct device *dev,
>                                dev_err(dev, "%s: vmap() failed\n", __func__);
>                                goto err;
>                        }
> -                       /* Pages will be freed by vfree() */
> -                       fw_priv->page_array_size = 0;
> -                       fw_priv->nr_pages = 0;
>                        complete(&fw_priv->completion);
>                        clear_bit(FW_STATUS_LOADING, &fw_priv->status);
>                        break;
> @@ -578,7 +575,7 @@ release_firmware(const struct firmware *fw)
>                        if (fw->data == builtin->data)
>                                goto free_fw;
>                }
> -               vfree(fw->data);
> +               vunmap(fw->data);
>        free_fw:
>                kfree(fw);
>        }
>

The difference between vfree and vunmap is that vfree request for
deallocating the pages while vunmap leaves the  pages allocated. I
don't think you can replace vfree with vunmap the way you did.
The transition from vmalloc to alloc_pages were done by the patch
bellow.  I think this needs some more review.

commit 6e03a201bbe8137487f340d26aa662110e324b20
Author: David Woodhouse <dwmw2@infradead.org>
Date:   Thu Apr 9 22:04:07 2009 -0700

    firmware: speed up request_firmware(), v3

    Rather than calling vmalloc() repeatedly to grow the firmware image as
    we receive data from userspace, just allocate and fill individual pages.
    Then vmap() the whole lot in one go when we're done.

    A quick test with a 337KiB iwlagn firmware shows the time taken for
    request_firmware() going from ~32ms to ~5ms after I apply this patch.

    [v2: define PAGE_KERNEL_RO as PAGE_KERNEL where necessary, use min_t()]
    [v3: kunmap() takes the struct page *, not the virtual address]

    Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
    Tested-by: Sachin Sant <sachinp@in.ibm.com

Re: request_firmware API exhaust memory

[Tomas Winkler]

On Tue, Apr 27, 2010 at 2:18 PM, Tomas Winkler <tomasw@gmail.com> wrote:
> On Mon, Apr 26, 2010 at 6:19 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> On Mon, 2010-04-26 at 12:38 +0200, Kay Sievers wrote:
>>> On Sun, Apr 25, 2010 at 22:09, Tomas Winkler <tomasw@gmail.com> wrote:
>>> > Said thing is that I don't see where the memory goes.... Anyhow I will
>>> > try to run valgrin on udev just to be sure.
>>>
>>> Nah, that memory would be freed, if you kill all udev processes, which
>>> it doesn't.
>>>
>>> The many udev worker processes you see for a few seconds was caused by
>>> udevd handling events with TIMEOUT= set special. We need to make sure,
>>> that firmware events run immediately and don't wait for other
>>> processes to finish. The logic who does that was always creating a new
>>> worker. I changed this now, but this will not affect the underlying
>>> problem you are seeing, it will just make the udev workers not grow in
>>> a timeframe of less than 10 seconds. The change is here:
>>>   http://git.kernel.org/?p=linux/hotplug/udev.git;a=commit;h=665ee17def2caa6811ae032ae68ebf8239a18cf8
>>> but as mentioned, this change is unrelated to the memory leak you are seeing.
>>>
>>> > I'll be glad If someone can run my simple driver I posted and confirm
>>> > that sees the same problem
>>>
>>> I can confirm that memory gets lost. I suspect for some reason the
>>> firmware does not get properly cleaned up. If you increase the size of
>>> the firmware image, it will leak memory much faster.
>>
>> I guess, the assumption that vfree() will free pages which are allocated
>> by custom code, and not by vmalloc(), is not true.
>>
>> The attached changes seem to fix the issue for me.
>>
>> The custom page array mangling was introduced by David as an optimization
>> with commit 6e03a201bbe8137487f340d26aa662110e324b20 and this should be
>> checked, and if needed be fixed.
>>
>> Cheers,
>> Kay
>>
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index 985da11..fe4e872 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -162,7 +162,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>>                        mutex_unlock(&fw_lock);
>>                        break;
>>                }
>> -               vfree(fw_priv->fw->data);
>> +               vunmap(fw_priv->fw->data);
>>                fw_priv->fw->data = NULL;
>>                for (i = 0; i < fw_priv->nr_pages; i++)
>>                        __free_page(fw_priv->pages[i]);
>> @@ -176,7 +176,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>>                break;
>>        case 0:
>>                if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
>> -                       vfree(fw_priv->fw->data);
>> +                       vunmap(fw_priv->fw->data);
>>                        fw_priv->fw->data = vmap(fw_priv->pages,
>>                                                 fw_priv->nr_pages,
>>                                                 0, PAGE_KERNEL_RO);
>> @@ -184,9 +184,6 @@ static ssize_t firmware_loading_store(struct device *dev,
>>                                dev_err(dev, "%s: vmap() failed\n", __func__);
>>                                goto err;
>>                        }
>> -                       /* Pages will be freed by vfree() */
>> -                       fw_priv->page_array_size = 0;
>> -                       fw_priv->nr_pages = 0;
>>                        complete(&fw_priv->completion);
>>                        clear_bit(FW_STATUS_LOADING, &fw_priv->status);
>>                        break;
>> @@ -578,7 +575,7 @@ release_firmware(const struct firmware *fw)
>>                        if (fw->data == builtin->data)
>>                                goto free_fw;
>>                }
>> -               vfree(fw->data);
>> +               vunmap(fw->data);
>>        free_fw:
>>                kfree(fw);
>>        }
>>
>
> The difference between vfree and vunmap is that vfree request for
> deallocating the pages while vunmap leaves the  pages allocated. I
> don't think you can replace vfree with vunmap the way you did.
> The transition from vmalloc to alloc_pages were done by the patch
> bellow.  I think this needs some more review.
>
> commit 6e03a201bbe8137487f340d26aa662110e324b20
> Author: David Woodhouse <dwmw2@infradead.org>
> Date:   Thu Apr 9 22:04:07 2009 -0700
>
>    firmware: speed up request_firmware(), v3
>
>    Rather than calling vmalloc() repeatedly to grow the firmware image as
>    we receive data from userspace, just allocate and fill individual pages.
>    Then vmap() the whole lot in one go when we're done.
>
>    A quick test with a 337KiB iwlagn firmware shows the time taken for
>    request_firmware() going from ~32ms to ~5ms after I apply this patch.
>
>    [v2: define PAGE_KERNEL_RO as PAGE_KERNEL where necessary, use min_t()]
>    [v3: kunmap() takes the struct page *, not the virtual address]
>
>    Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
>    Tested-by: Sachin Sant <sachinp@in.ibm.com


It looks like to me that  fw_realloc_buffer missing some 'vREmap' code

Tomas

Re: request_firmware API exhaust memory

[Kay Sievers]

On Tue, Apr 27, 2010 at 13:18, Tomas Winkler <tomasw@gmail.com> wrote:

> The difference between vfree and vunmap is that vfree request for
> deallocating the pages while vunmap leaves the  pages allocated.

No, that's not the problem, as described in the earlier mail. Only if
the are vmalloc'd they will ever get free'd. The kernel will not take
over custom pages you just mapped. They will never get touched by
vfree. The page count in the vm area will always be zero.

> I
> don't think you can replace vfree with vunmap the way you did.
> The transition from vmalloc to alloc_pages were done by the patch
> bellow.

Sure, if you do vmap, you need to do vunmap, not vfree. But the pages
you need to take care of yourself, like you have allocated them. vfree
in that context makes only sense if you use vmalloc.

The patch I posted makes the issue go away. It's still not the right
fix, because the pages are only get freed when the device id cleaned
up, not on calling release_firmware. But it should illustrate the
underlying issue, and that there is no leaked memory anymore.

>  I think this needs some more review.

If David does not fix it, it probably just needs to be reverted. And
instead of implementing our own "memory management", we should rather
add a vrealloc(), and the firmware loader should use that.

Thanks,
Kay

Re: request_firmware API exhaust memory

[David Woodhouse]

On Tue, 2010-04-27 at 14:05 +0200, Kay Sievers wrote:
> 
> The patch I posted makes the issue go away. It's still not the right
> fix, because the pages are only get freed when the device id cleaned
> up, not on calling release_firmware. But it should illustrate the
> underlying issue, and that there is no leaked memory anymore.
> 
> >  I think this needs some more review.
> 
> If David does not fix it, it probably just needs to be reverted. And
> instead of implementing our own "memory management", we should rather
> add a vrealloc(), and the firmware loader should use that. 

The whole point was to avoid the vrealloc(). We really don't want to be
screwing with page tables, globally, for each page written from
userspace.

This untested patch attempts to put the page array into the 'struct
firmware' so that we can free it from release_firmware().

It would actually be nice if we could make that the _primary_ method of
returning data to drivers, and we could ditch the vmap() requirement
altogether... drivers which really need it to be virtually contiguous
can depend on CONFIG_MMU and do the vmap() for themselves.

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 985da11..cc9a79b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -162,8 +162,13 @@ static ssize_t firmware_loading_store(struct device *dev,
 			mutex_unlock(&fw_lock);
 			break;
 		}
-		vfree(fw_priv->fw->data);
+		vunmap(fw_priv->fw->data);
 		fw_priv->fw->data = NULL;
+		if (fw_priv->fw->pages) {
+			for (i = 0; i < PFN_UP(fw_priv->fw->size); i++)
+				__free_page(fw_priv->fw->pages[i]);
+			kfree(fw_priv->fw->pages);
+		}
 		for (i = 0; i < fw_priv->nr_pages; i++)
 			__free_page(fw_priv->pages[i]);
 		kfree(fw_priv->pages);
@@ -176,7 +181,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 		break;
 	case 0:
 		if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
-			vfree(fw_priv->fw->data);
+			vunmap(fw_priv->fw->data);
 			fw_priv->fw->data = vmap(fw_priv->pages,
 						 fw_priv->nr_pages,
 						 0, PAGE_KERNEL_RO);
@@ -184,7 +189,9 @@ static ssize_t firmware_loading_store(struct device *dev,
 				dev_err(dev, "%s: vmap() failed\n", __func__);
 				goto err;
 			}
-			/* Pages will be freed by vfree() */
+			/* Pages are now owned by 'struct firmware' */
+			fw_priv->fw->pages = fw_priv->pages;
+			fw_priv->pages = NULL;
 			fw_priv->page_array_size = 0;
 			fw_priv->nr_pages = 0;
 			complete(&fw_priv->completion);
@@ -571,6 +578,7 @@ void
 release_firmware(const struct firmware *fw)
 {
 	struct builtin_fw *builtin;
+	int i;
 
 	if (fw) {
 		for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
@@ -578,7 +586,12 @@ release_firmware(const struct firmware *fw)
 			if (fw->data == builtin->data)
 				goto free_fw;
 		}
-		vfree(fw->data);
+		vunmap(fw->data);
+		if (fw->pages) {
+			for (i = 0; i < PFN_UP(fw->size); i++)
+				__free_page(fw->pages[i]);
+			kfree(fw->pages);
+		}
 	free_fw:
 		kfree(fw);
 	}


-- 
dwmw2

Re: request_firmware API exhaust memory

[Kay Sievers]

On Tue, Apr 27, 2010 at 14:43, David Woodhouse <dwmw2@infradead.org> wrote:
> On Tue, 2010-04-27 at 14:05 +0200, Kay Sievers wrote:
>>
>> The patch I posted makes the issue go away. It's still not the right
>> fix, because the pages are only get freed when the device id cleaned
>> up, not on calling release_firmware. But it should illustrate the
>> underlying issue, and that there is no leaked memory anymore.
>>
>> >  I think this needs some more review.
>>
>> If David does not fix it, it probably just needs to be reverted. And
>> instead of implementing our own "memory management", we should rather
>> add a vrealloc(), and the firmware loader should use that.
>
> The whole point was to avoid the vrealloc(). We really don't want to be
> screwing with page tables, globally, for each page written from
> userspace.

Yeah. I guess the problem with the old code before you made if faster
was that it was doing vmalloc()->memcpy()->vfree() in a loop. A
vrealloc(), which we don't have today, could be made reasonable fast,
I guess.

> This untested patch attempts to put the page array into the 'struct
> firmware' so that we can free it from release_firmware().

Looks good. Seems to work without problems and without leaking memory.

Misses only the member in the struct firmware though. :)

> It would actually be nice if we could make that the _primary_ method of
> returning data to drivers, and we could ditch the vmap() requirement
> altogether... drivers which really need it to be virtually contiguous
> can depend on CONFIG_MMU and do the vmap() for themselves.

Yeah, we could just do that in a firmware_get_data() accessor function
and call vmap() there on-demand. That way we would just account and
copy pages in the firmware class until the data is actually accessed.
Existing users would just need to change the direct ->data access to
firmware_get_data().

Thanks,
Kay

Re: request_firmware API exhaust memory

[Kay Sievers]

On Tue, Apr 27, 2010 at 15:34, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Tue, Apr 27, 2010 at 14:43, David Woodhouse <dwmw2@infradead.org> wrote:
>> This untested patch attempts to put the page array into the 'struct
>> firmware' so that we can free it from release_firmware().
>
> Looks good. Seems to work without problems and without leaking memory.
>
> Misses only the member in the struct firmware though. :)

Thomas, any chance to test David's patch, if that solves the issues you've seen?

Just add the missing line:

--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -12,6 +12,7 @@
 struct firmware {
        size_t size;
        const u8 *data;
+       struct page **pages;
 };


Thanks,
Kay

Re: request_firmware API exhaust memory

[Tomas Winkler]

On Wed, Apr 28, 2010 at 11:23 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Tue, Apr 27, 2010 at 15:34, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> On Tue, Apr 27, 2010 at 14:43, David Woodhouse <dwmw2@infradead.org> wrote:
>>> This untested patch attempts to put the page array into the 'struct
>>> firmware' so that we can free it from release_firmware().
>>
>> Looks good. Seems to work without problems and without leaking memory.
>>
>> Misses only the member in the struct firmware though. :)
>
> Thomas, any chance to test David's patch, if that solves the issues you've seen?
>
> Just add the missing line:
>
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -12,6 +12,7 @@
>  struct firmware {
>        size_t size;
>        const u8 *data;
> +       struct page **pages;
>  };
>
>
In progress. Will post results later today (timezone:GMT+2)

Thanks
Tomas