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

[Vlastimil Babka <vbabka@suse.cz>]

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?

> 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@gmail.com/
>> >> [2] https://lore.kernel.org/all/20210521121127.24653-1-glittao@gmail.com/
>> >> 
>> >> 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
>> >> 
>> >> 
>> > 
>> 
>