Hello Guangming, On Wed, 5 Jan 2022 at 12:05, <guangming.cao@xxxxxxxxxxxx> wrote: > > From: Guangming.Cao <guangming.cao@xxxxxxxxxxxx> > > On Tue, 2022-01-04 at 08:47 +0100, Christian K鰊ig 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, > > Christian. > > > 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. Thank you for the patch! I think just adding the reasoning above as the commit message and a comment in the code should be enough for now; the debug parts may be easy to add in case someone runs into issues. > Thanks :) > Guangming Best, Sumit. > > > > > > > > > > > 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 > > > > -- Thanks and regards, Sumit Semwal (he / him) Tech Lead - LCG, Vertical Technologies Linaro.org │ Open source software for ARM SoCs
