Re: memory leak in DMA buffers allocation?

[olafBuddenhagen]

Hi,

On Sun, Feb 07, 2010 at 10:08:27PM +0800, Da Zheng wrote:

> I wrote a RPC in GNUMach to allocate contiguous physical memory for
> DMA buffers. The code is shown as below. I have been using this
> function for quite a long time, but it seems it can cause some kind of
> memory leak. If I run the pcnet32 driver, which uses this function to
> allocate DMA buffers, for many times, the system seems to run out of
> memory. I checked the code, but didn't find the place which could
> cause memory leak. After a process is terminated, the memory and
> relevant objects allocated by vm_dma_buff_alloc should be freed
> automatically. I don't have much experience of Mach development. Maybe
> that's why I fail to find the problem.

I can't really tell what is happening there, without digging much deeper
into the code -- which I have no time for presently.

Also, I'm not sure it's even worthwhile, considering that it's just a
temporary solution anyways: I think when you implement it "properly" --
seperating the physical page allocation from the virtual mapping (and
using the standard mechanism for the latter) -- it should become much
clearer... And there is a good chance that the bug will just go away in
the process :-)

Seems strange though that you didn't see the problem with your original
pcnet32 port; only with the DDE-based one...

> antrik said this implementation isn't consistent with Mach's memory
> management and I should make it work like device_map, so there will be
> a pager in the kernel. I basically agree with that. What should the
> interface be? any suggestions?

It should be something along these lines I think:

   kern_return_t mach_get_dma_pages (
      host_priv_t host_priv,
      vm_size_t size,
      mach_port_t *pager,
      vm_offset_t *phys_addr,
      mach_dma_flags_t flags
   )

The last parameter would contain various options, like restricting to
first 32 MiB of RAM (for ISA DMA) or first 4 GiB (for many other
devices). Not sure whether we actually need any other options --
otherwise, perhaps this should get a more specific name.

"pager" returns a memory object, which can then be used in a vm_map()
call. "phys_addr" returns the physical address of the allocated region,
used to program the DMA hardware.

BTW, I'm not sure about handling scatter-gather DMA. This still needs
the physical addresses to be stable during the transfer, and available
to the driver; but the region needn't be contiguous... So I guess we
could just use "normal" memory (ideally passed directly from/to the
clients); wire it; and use some other special function to obtain the
physical addresses of each page.

> +             kfree (pages, npages * sizeof (vm_page_t));
> +             vm_pages_release (npages, pages, TRUE);

This looks wrong -- freeing the "pages" array before using it to free
the actual pages... But I doubt it is related to the problem you are
experiencing, considering that this is just an error path.

(Also, I don't think the extra vm_pages_release() function is really
useful, considering that it's just a very simple loop, and used only in
a single place... But here I am nitpicking again :-) )

-antrik-