Hi,
On Mon, Feb 28, 2022 at 09:27:02PM +0000, Hyeonggon Yoo 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?
>
>
> By the way, I fixed this by allowing stack_depot_init() to be called in
> kmem_cache_init() too [1] and Marco suggested that calling
> stack_depot_init() depending on slub_debug parameter for simplicity. [2]
>
> I would prefer [2], Would you take a look?
>
> [1] https://lkml.org/lkml/2022/2/27/31
>
> [2] https://lkml.org/lkml/2022/2/28/717
I'm still looking at stack_depot_init() callers, but I think it's possible
to make stack_depot_early_init() a proper function that calls
memblock_alloc() and only use stack_depot_init() when slab is actually
available.
> > > 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
> > >>
> > >>
> > >
> >
>
> --
> Thank you, You are awesome!
> Hyeonggon :-)
--
Sincerely yours,
Mike.
