Re: request_firmware API exhaust memory

[Tomas Winkler]

On Tue, Apr 27, 2010 at 2:18 PM, Tomas Winkler <tomasw@xxxxxxxxx> wrote:
> On Mon, Apr 26, 2010 at 6:19 PM, Kay Sievers <kay.sievers@xxxxxxxx> wrote:
>> On Mon, 2010-04-26 at 12:38 +0200, Kay Sievers wrote:
>>> On Sun, Apr 25, 2010 at 22:09, Tomas Winkler <tomasw@xxxxxxxxx> 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@xxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> Â ÂTested-by: Sachin Sant <sachinp@xxxxxxxxxx


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

Tomas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/