On Tue, 2022-01-04 at 08:47 +0100, Christian König wrote: > Am 03.01.22 um 19:57 schrieb John Stultz: > > On Mon, Dec 27, 2021 at 1:52 AM <guangming.cao@xxxxxxxxxxxx> wrote: > > > From: Guangming <Guangming.Cao@xxxxxxxxxxxx> > > > > > > > Thanks for submitting this! > > > > > Add a size check for allcation since the allocation size is > > > > nit: "allocation" above. > > > > > always less than the total DRAM size. > > > > In general, it might be good to add more context to the commit > > message > > to better answer *why* this change is needed rather than what the > > change is doing. ie: What negative thing happens without this > > change? > > And so how does this change avoid or improve things? > > Completely agree, just one little addition: Could you also add this > why > as comment to the code? > > When we stumble over this five years from now it is absolutely not > obvious why we do this. Thanks for your reply! I will update the related reason in the patch later. The reason for adding this check is that we met a case that the user sent an invalid size(It seems it's a negative value, MSB is 0xff, it's larger than DRAM size after convert it to size_t) to dma-heap to alloc memory, and this allocation was running on a process(such as "gralloc" on Android device) can't be killed by OOM flow, and we also couldn't find the related dmabuf in "dma_buf_debug_show" because the related dmabuf was not exported yet since the allocation is still on going. Since this invalid argument case can be prevented at dma-heap side, so, I added this size check, and moreover, to let debug it easily, I also added logs when size is bigger than a threshold we set in mtk system heap. If you think that print logs in dma-heap framework is better, I will update it in next version. If you have better solution(such as dump the size under allocating in "dma_buf_debug_show", which maybe need add global variable to record it), please kindly let me know, thanks :) > > Thanks, > Christian. > > > > > > > > Signed-off-by: Guangming <Guangming.Cao@xxxxxxxxxxxx> > > > Signed-off-by: jianjiao zeng <jianjiao.zeng@xxxxxxxxxxxx> > > > --- > > > v2: 1. update size limitation as total_dram page size. > > > 2. update commit message > > > --- > > > drivers/dma-buf/dma-heap.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma- > > > heap.c > > > index 56bf5ad01ad5..e39d2be98d69 100644 > > > --- a/drivers/dma-buf/dma-heap.c > > > +++ b/drivers/dma-buf/dma-heap.c > > > @@ -55,6 +55,8 @@ static int dma_heap_buffer_alloc(struct > > > dma_heap *heap, size_t len, > > > struct dma_buf *dmabuf; > > > int fd; > > > > > > + if (len / PAGE_SIZE > totalram_pages()) > > > + return -EINVAL; > > > > This seems sane. I know ION used to have some 1/2 of memory cap to > > avoid unnecessary memory pressure on crazy allocations. > > > > Could you send again with an improved commit message? > > > > thanks > > -john > >
