| Message ID | 20250103110150.1352226-1-guoweikang.kernel@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm/memmap: Prevent double scanning of memmap by kmemleak | expand |
On Fri, Jan 03, 2025 at 07:01:50PM +0800, Guo Weikang wrote: > diff --git a/mm/mm_init.c b/mm/mm_init.c > index 24b68b425afb..71b58f5f2492 100644 > --- a/mm/mm_init.c > +++ b/mm/mm_init.c > @@ -1580,6 +1580,10 @@ static void __init free_area_init_core(struct pglist_data *pgdat) > } > } > > +/* > + * Kmemleak will explicitly scan mem_map by traversing all valid `struct *page`, > + * so memblock does not need to be added to the scan list. > + */ > void __init *memmap_alloc(phys_addr_t size, phys_addr_t align, > phys_addr_t min_addr, int nid, bool exact_nid) > { Nitpick: normally I'd place the comment in the code, before the 'if' statement. We keep the comments above functions for a description of the function. > @@ -1587,11 +1591,11 @@ void __init *memmap_alloc(phys_addr_t size, phys_addr_t align, > > if (exact_nid) > ptr = memblock_alloc_exact_nid_raw(size, align, min_addr, > - MEMBLOCK_ALLOC_ACCESSIBLE, > + MEMBLOCK_ALLOC_NOLEAKTRACE, > nid); > else > ptr = memblock_alloc_try_nid_raw(size, align, min_addr, > - MEMBLOCK_ALLOC_ACCESSIBLE, > + MEMBLOCK_ALLOC_NOLEAKTRACE, > nid); > > if (ptr && size > 0) > diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c > index cec67c5f37d8..903a5422907b 100644 > --- a/mm/sparse-vmemmap.c > +++ b/mm/sparse-vmemmap.c > @@ -27,7 +27,7 @@ > #include <linux/spinlock.h> > #include <linux/vmalloc.h> > #include <linux/sched.h> > - > +#include "internal.h" > #include <asm/dma.h> > #include <asm/pgalloc.h> Another nit: if the original authors of this file preferred the includes to be in blocks, I'd keep the internal.h include as a separate block after the asm/ includes. Apart from these minor things, the patch looks fine. Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> Thanks.
Thanskļ¼ Catalin Catalin Marinas <catalin.marinas@arm.com> wrote on Friday, 3 January 2025 at 22:28 > > On Fri, Jan 03, 2025 at 07:01:50PM +0800, Guo Weikang wrote: > > diff --git a/mm/mm_init.c b/mm/mm_init.c > > index 24b68b425afb..71b58f5f2492 100644 > > --- a/mm/mm_init.c > > +++ b/mm/mm_init.c > > @@ -1580,6 +1580,10 @@ static void __init free_area_init_core(struct pglist_data *pgdat) > > } > > } > > > > +/* > > + * Kmemleak will explicitly scan mem_map by traversing all valid `struct *page`, > > + * so memblock does not need to be added to the scan list. > > + */ > > void __init *memmap_alloc(phys_addr_t size, phys_addr_t align, > > phys_addr_t min_addr, int nid, bool exact_nid) > > { > > Nitpick: normally I'd place the comment in the code, before the 'if' > statement. We keep the comments above functions for a description of > the function. > Thank you for the suggestion! I agree that placing the comment before the 'if' statement is a better approach. I will update the patch accordingly. > > @@ -1587,11 +1591,11 @@ void __init *memmap_alloc(phys_addr_t size, phys_addr_t align, > > > > if (exact_nid) > > ptr = memblock_alloc_exact_nid_raw(size, align, min_addr, > > - MEMBLOCK_ALLOC_ACCESSIBLE, > > + MEMBLOCK_ALLOC_NOLEAKTRACE, > > nid); > > else > > ptr = memblock_alloc_try_nid_raw(size, align, min_addr, > > - MEMBLOCK_ALLOC_ACCESSIBLE, > > + MEMBLOCK_ALLOC_NOLEAKTRACE, > > nid); > > > > if (ptr && size > 0) > > diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c > > index cec67c5f37d8..903a5422907b 100644 > > --- a/mm/sparse-vmemmap.c > > +++ b/mm/sparse-vmemmap.c > > @@ -27,7 +27,7 @@ > > #include <linux/spinlock.h> > > #include <linux/vmalloc.h> > > #include <linux/sched.h> > > - > > +#include "internal.h" > > #include <asm/dma.h> > > #include <asm/pgalloc.h> > > Another nit: if the original authors of this file preferred the includes > to be in blocks, I'd keep the internal.h include as a separate block > after the asm/ includes. > Here is the order I actually refer to `mm/sparse.c`, but after looking at other files, I think your suggestion is better. > Apart from these minor things, the patch looks fine. > Tanks! > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> > > Thanks. > > -- > Catalin --- Guo
diff --git a/include/linux/memblock.h b/include/linux/memblock.h index dee628350cd1..e79eb6ac516f 100644 --- a/include/linux/memblock.h +++ b/include/linux/memblock.h @@ -378,6 +378,10 @@ static inline int memblock_get_region_node(const struct memblock_region *r) /* Flags for memblock allocation APIs */ #define MEMBLOCK_ALLOC_ANYWHERE (~(phys_addr_t)0) #define MEMBLOCK_ALLOC_ACCESSIBLE 0 +/* + * MEMBLOCK_ALLOC_NOLEAKTRACE avoids kmemleak tracing. It implies + * MEMBLOCK_ALLOC_ACCESSIBLE + */ #define MEMBLOCK_ALLOC_NOLEAKTRACE 1 /* We are using top down, so it is safe to use 0 here */ diff --git a/mm/mm_init.c b/mm/mm_init.c index 24b68b425afb..71b58f5f2492 100644 --- a/mm/mm_init.c +++ b/mm/mm_init.c @@ -1580,6 +1580,10 @@ static void __init free_area_init_core(struct pglist_data *pgdat) } } +/* + * Kmemleak will explicitly scan mem_map by traversing all valid `struct *page`, + * so memblock does not need to be added to the scan list. + */ void __init *memmap_alloc(phys_addr_t size, phys_addr_t align, phys_addr_t min_addr, int nid, bool exact_nid) { @@ -1587,11 +1591,11 @@ void __init *memmap_alloc(phys_addr_t size, phys_addr_t align, if (exact_nid) ptr = memblock_alloc_exact_nid_raw(size, align, min_addr, - MEMBLOCK_ALLOC_ACCESSIBLE, + MEMBLOCK_ALLOC_NOLEAKTRACE, nid); else ptr = memblock_alloc_try_nid_raw(size, align, min_addr, - MEMBLOCK_ALLOC_ACCESSIBLE, + MEMBLOCK_ALLOC_NOLEAKTRACE, nid); if (ptr && size > 0) diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c index cec67c5f37d8..903a5422907b 100644 --- a/mm/sparse-vmemmap.c +++ b/mm/sparse-vmemmap.c @@ -27,7 +27,7 @@ #include <linux/spinlock.h> #include <linux/vmalloc.h> #include <linux/sched.h> - +#include "internal.h" #include <asm/dma.h> #include <asm/pgalloc.h> @@ -42,8 +42,7 @@ static void * __ref __earlyonly_bootmem_alloc(int node, unsigned long align, unsigned long goal) { - return memblock_alloc_try_nid_raw(size, align, goal, - MEMBLOCK_ALLOC_ACCESSIBLE, node); + return memmap_alloc(size, align, goal, node, false); } void * __meminit vmemmap_alloc_block(unsigned long size, int node)
kmemleak explicitly scans the mem_map through the valid struct page objects. However, memmap_alloc() was also adding this memory to the gray object list, causing it to be scanned twice. Removes memmap_alloc() from the scan list and adds a comment to clarify the behavior. Link: https://lore.kernel.org/lkml/CAOm6qn=FVeTpH54wGDFMHuCOeYtvoTx30ktnv9-w3Nh8RMofEA@mail.gmail.com/ Signed-off-by: Guo Weikang <guoweikang.kernel@gmail.com> --- v1 -> v2: Fix CI: Recover __earlyonly_bootmem_alloc that is marked with __ref to prevent init section mismatch WARNING. Links: https://lore.kernel.org/oe-kbuild-all/202501021601.jub4p3EM-lkp@intel.com/ --- include/linux/memblock.h | 4 ++++ mm/mm_init.c | 8 ++++++-- mm/sparse-vmemmap.c | 5 ++--- 3 files changed, 12 insertions(+), 5 deletions(-)