Re: [PATCH 0/5] SLUB debugfs improvements based on stackdepot

[Mike Rapoport]

On Tue, Mar 01, 2022 at 12:38:11AM +0100, Vlastimil Babka wrote:
> On 2/28/22 21:01, Mike Rapoport wrote:
> > On Mon, Feb 28, 2022 at 08:10:18PM +0100, Vlastimil Babka wrote:
> >> On 2/26/22 08:19, Hyeonggon Yoo wrote:
> >> > On Fri, Feb 25, 2022 at 07:03:13PM +0100, Vlastimil Babka wrote:
> >> >> Hi,
> >> >> 
> >> >> this series combines and revives patches from Oliver's last year
> >> >> bachelor thesis (where I was the advisor) that make SLUB's debugfs
> >> >> files alloc_traces and free_traces more useful.
> >> >> The resubmission was blocked on stackdepot changes that are now merged,
> >> >> as explained in patch 2.
> >> >> 
> >> > 
> >> > Hello. I just started review/testing this series.
> >> > 
> >> > it crashed on my system (arm64)
> >> 
> >> Hmm, interesting. On x86_64 this works for me and stackdepot is allocated
> >> from memblock. arm64 must have memblock freeing happen earlier or something.
> >> (CCing memblock experts)
> >> 
> >> > I ran with boot parameter slub_debug=U, and without KASAN.
> >> > So CONFIG_STACKDEPOT_ALWAYS_INIT=n.
> >> > 
> >> > void * __init memblock_alloc_try_nid(
> >> >                         phys_addr_t size, phys_addr_t align,
> >> >                         phys_addr_t min_addr, phys_addr_t max_addr,
> >> >                         int nid)
> >> > {
> >> >         void *ptr;
> >> > 
> >> >         memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa %pS\n",
> >> >                      __func__, (u64)size, (u64)align, nid, &min_addr,
> >> >                      &max_addr, (void *)_RET_IP_);
> >> >         ptr = memblock_alloc_internal(size, align,
> >> >                                            min_addr, max_addr, nid, false);
> >> >         if (ptr)
> >> >                 memset(ptr, 0, size); <--- Crash Here
> >> > 
> >> >         return ptr;
> >> > }
> >> > 
> >> > It crashed during create_boot_cache() -> stack_depot_init() ->
> >> > memblock_alloc().
> >> > 
> >> > I think That's because, in kmem_cache_init(), both slab and memblock is not
> >> > available. (AFAIU memblock is not available after mem_init() because of
> >> > memblock_free_all(), right?)
> >> 
> >> Hm yes I see, even in x86_64 version mem_init() calls memblock_free_all().
> >> But then, I would expect stack_depot_init() to detect that memblock_alloc()
> >> returns NULL, we print ""Stack Depot hash table allocation failed,
> >> disabling" and disable it. Instead it seems memblock_alloc() returns
> >> something that's already potentially used by somebody else? Sounds like a bug?
> > 
> > If stack_depot_init() is called from kmem_cache_init(), there will be a
> > confusion what allocator should be used because we use slab_is_available()
> > to stop using memblock and start using kmalloc() instead in both
> > stack_depot_init() and in memblock.
> 
> I did check that stack_depot_init() is called from kmem_cache_init()
> *before* we make slab_is_available() true, hence assumed that memblock would
> be still available at that point and expected no confusion. But seems if
> memblock is already beyond memblock_free_all() then it being still available
> is just an illusion?

Yeah, it appears it is an illusion :)

I think we have to deal with allocations that happen between
memblock_free_all() and slab_is_available() at the memblock level and then
figure out the where to put stack_depot_init() and how to allocate memory
there.

I believe something like this (untested) patch below addresses the first
issue. As for stack_depot_init() I'm still trying to figure out the
possible call paths, but it seems we can use stack_depot_early_init() for
SLUB debugging case. I'll try to come up with something Really Soon (tm).

diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 50ad19662a32..4ea89d44d22a 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -90,6 +90,7 @@ struct memblock_type {
  */
 struct memblock {
 	bool bottom_up;  /* is bottom up direction? */
+	bool mem_freed;
 	phys_addr_t current_limit;
 	struct memblock_type memory;
 	struct memblock_type reserved;
diff --git a/mm/memblock.c b/mm/memblock.c
index b12a364f2766..60196dc4980e 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -120,6 +120,7 @@ struct memblock memblock __initdata_memblock = {
 	.reserved.name		= "reserved",
 
 	.bottom_up		= false,
+	.mem_freed		= false,
 	.current_limit		= MEMBLOCK_ALLOC_ANYWHERE,
 };
 
@@ -1487,6 +1488,13 @@ static void * __init memblock_alloc_internal(
 	if (WARN_ON_ONCE(slab_is_available()))
 		return kzalloc_node(size, GFP_NOWAIT, nid);
 
+	if (memblock.mem_freed) {
+		unsigned int order = get_order(size);
+
+		pr_warn("memblock: allocating from buddy\n");
+		return __alloc_pages_node(nid, order, GFP_KERNEL);
+	}
+
 	if (max_addr > memblock.current_limit)
 		max_addr = memblock.current_limit;
 
@@ -2116,6 +2124,7 @@ void __init memblock_free_all(void)
 
 	pages = free_low_memory_core_early();
 	totalram_pages_add(pages);
+	memblock.mem_freed = true;
 }
 
 #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_ARCH_KEEP_MEMBLOCK)
 
> > Hyeonggon, did you run your tests with panic on warn at any chance?
> >  
> >> > Thanks!
> >> > 
> >> > /*
> >> >  * Set up kernel memory allocators
> >> >  */
> >> > static void __init mm_init(void)
> >> > {
> >> >         /*
> >> >          * page_ext requires contiguous pages,
> >> >          * bigger than MAX_ORDER unless SPARSEMEM.
> >> >          */
> >> >         page_ext_init_flatmem();
> >> >         init_mem_debugging_and_hardening();
> >> >         kfence_alloc_pool();
> >> >         report_meminit();
> >> >         stack_depot_early_init();
> >> >         mem_init();
> >> >         mem_init_print_info();
> >> >         kmem_cache_init();
> >> >         /*
> >> >          * page_owner must be initialized after buddy is ready, and also after
> >> >          * slab is ready so that stack_depot_init() works properly
> >> >          */)
> >> > 
> >> >> Patch 1 is a new preparatory cleanup.
> >> >> 
> >> >> Patch 2 originally submitted here [1], was merged to mainline but
> >> >> reverted for stackdepot related issues as explained in the patch.
> >> >> 
> >> >> Patches 3-5 originally submitted as RFC here [2]. In this submission I
> >> >> have omitted the new file 'all_objects' (patch 3/3 in [2]) as it might
> >> >> be considered too intrusive so I will postpone it for later. The docs
> >> >> patch is adjusted accordingly.
> >> >> 
> >> >> Also available in git, based on v5.17-rc1:
> >> >> https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/linux.git/log/?h=slub-stackdepot-v1
> >> >> 
> >> >> I'd like to ask for some review before I add this to the slab tree.
> >> >> 
> >> >> [1] https://lore.kernel.org/all/20210414163434.4376-1-glittao@xxxxxxxxx/
> >> >> [2] https://lore.kernel.org/all/20210521121127.24653-1-glittao@xxxxxxxxx/
> >> >> 
> >> >> Oliver Glitta (4):
> >> >>   mm/slub: use stackdepot to save stack trace in objects
> >> >>   mm/slub: aggregate and print stack traces in debugfs files
> >> >>   mm/slub: sort debugfs output by frequency of stack traces
> >> >>   slab, documentation: add description of debugfs files for SLUB caches
> >> >> 
> >> >> Vlastimil Babka (1):
> >> >>   mm/slub: move struct track init out of set_track()
> >> >> 
> >> >>  Documentation/vm/slub.rst |  61 +++++++++++++++
> >> >>  init/Kconfig              |   1 +
> >> >>  mm/slub.c                 | 152 +++++++++++++++++++++++++-------------
> >> >>  3 files changed, 162 insertions(+), 52 deletions(-)
> >> >> 
> >> >> -- 
> >> >> 2.35.1
> >> >> 
> >> >> 
> >> > 
> >> 
> > 
> 

-- 
Sincerely yours,
Mike.