On Thu, Jan 19, 2023 at 04:20:56PM -0800, Isaac Manjarres wrote: > On Wed, Jan 18, 2023 at 05:16:46PM +0000, Catalin Marinas wrote: > > What I don't understand is why kmemleak scans such CMA regions. The only > > reason for a kmemleak_ignore_phys() call in cma_declare_contiguous_nid() > > is because the kmemleak_alloc_phys() hook was called on the > > memblock_alloc_range_nid() path, so we don't want this scanned. > The reason is because kmemleak_ignore_phys() is only called within > cma_declare_contiguous_nid(), which is not called for every CMA region. > > For instance, CMA regions which are specified through the devicetree > and not constrained to a fixed address are allocated through > early_init_dt_alloc_reserved_memory_arch(), which eventually calls > kmemleak_alloc_phys() through memblock_phys_alloc_range(). > > When the CMA region is constrained to a particular address, it is allocated > through early_init_dt_reserve_memory(), which is followed up by a call to > kmemleak_alloc_phys() due to this commit: > https://lore.kernel.org/all/20211123090641.3654006-1-calvinzhang.cool@xxxxxxxxx/T/#u Thanks for digging this out. This patch shouldn't have ended up upstream (commit 972fa3a7c17c "mm: kmemleak: alloc gray object for reserved region with direct map"). I thought both Calvin Zhang and I agreed that it's not the correct approach (not even sure there was a real problem to fix). Do you still get the any faults with the above commit reverted? I'd prefer this if it works rather than adding unnecessary kmemleak_alloc/free callbacks that pretty much cancel each-other. > I'm not sure if that commit is appropriate, given that reserved regions > that still have their direct mappings intact may be used for DMA, which > isn't appropriate for kmemleak scanning. It's not. I think it should be reverted. > > kmemleak would only scan such objects if it knows about them. So I think > > it's only the case where CMA does a memblock allocation. The > > kmemleak_ignore_phys() should tell kmemleak not to touch this region but > > it's probably better to just free it altogether (i.e. replace the ignore > > with the free kmemleak callback). Would this be sufficient for your > > scenario? > > I agree that freeing the kmemleak object is a better strategy. However, > replacing the call to kmemleak_ignore_phys() wouldn't be sufficient, > as there are other scenarios that would still leave behind kmemleak > objects to be scanned. That's why I ended up freeing the kmemleak object > in a path that is common for all CMA areas. The only reason for kmemleak_ignore_phys() was to counter the actual kmemleak_alloc() call from the memblock code on the CMA allocation. -- Catalin
