On Fri, 2022-01-14 at 08:16 +0100, Christian König wrote:
Am 14.01.22 um 00:26 schrieb John Stultz:
On Thu, Jan 13, 2022 at 5:05 AM Christian König christian.koenig@amd.com wrote:
Am 13.01.22 um 14:00 schrieb Ruhl, Michael J:
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Ruhl, Michael J
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of guangming.cao@mediatek.com
- /*
- Invalid size check. The "len" should be less than
totalram.
- Without this check, once the invalid size allocation
runs on a process that
- can't be killed by OOM flow(such as "gralloc" on
Android devices), it will
- cause a kernel exception, and to make matters worse,
we can't find who are using
- so many memory with "dma_buf_debug_show" since the
relevant dma-buf hasn't exported.
- */
- if (len >> PAGE_SHIFT > totalram_pages())
If your "heap" is from cma, is this still a valid check?
And thinking a bit further, if I create a heap from something else (say device memory), you will need to be able to figure out the maximum allowable check for the specific heap.
Maybe the heap needs a callback for max size?
Yes, I agree with this solution. If dma-heap framework support this via adding a callback to support it, seems it's more clear than adding a limitation in dma-heap framework since each heap maybe has different limitation. If you prefer adding callback, I can update this patch and add totalram limitation to system dma-heap.
Thanks! Guangming
Well we currently maintain a separate allocator and don't use dma-heap, but yes we have systems with 16GiB device and only 8GiB system memory so that check here is certainly not correct.
Good point.
In general I would rather let the system run into -ENOMEM or -EINVAL from the allocator instead.
For system dma-heap, it doesn't know how memory is avaliable when allocating memory, so, use totalram_pages() just to prevent cases which will cause oom definitely.
Just like PAGE align, this check is can be used for all heaps since there is no dma-heap can alloc memory larger than totalram. Futhermore, if vendors implement a variety of dma-heap like system heap for special usages, seems need to add this check to each dma-heap, and I think this is unnecessary. If the dma-heap has it's own special limitations for size, and add it into heap implementation is good.
Thanks! Guangming
Probably the simpler solution is to push the allocation check to the heap driver, rather than doing it at the top level here.
For CMA or other contiguous heaps, letting the allocator fail is fast enough. For noncontiguous buffers, like the system heap, the allocation can burn a lot of time and consume a lot of memory (causing other trouble) before a large allocation might naturally fail.
Yeah, letting a alloc_page() loop run for a while is usually not nice at all :)
You can still do a sanity check here, e.g. the size should never have the most significant bit set for example.
Yes, this is a good solution. But if this a positive value, larger than totalram, it can also pass this check, and cause OOM after some time.
From dicussion above, seems finding a proper solution that can judge the size is valid or not for each dma-heap is more important.
Thanks! Guangming
Regards, Christian.
thanks -john