Re: request_firmware API exhaust memory

Kay Sievers
2010-04-27T13:34:34Z

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