Re: [PATCH v4 1/4] mm/slub: enable debugging memory wasting of kmalloc

[Feng Tang]

On Thu, Sep 01, 2022 at 10:01:13PM +0800, Hyeonggon Yoo wrote:
> On Mon, Aug 29, 2022 at 03:56:15PM +0800, Feng Tang wrote:
> > kmalloc's API family is critical for mm, with one nature that it will
> > round up the request size to a fixed one (mostly power of 2). Say
> > when user requests memory for '2^n + 1' bytes, actually 2^(n+1) bytes
> > could be allocated, so in worst case, there is around 50% memory
> > space waste.
> > 
> > The wastage is not a big issue for requests that get allocated/freed
> > quickly, but may cause problems with objects that have longer life
> > time.
> > 
> > We've met a kernel boot OOM panic (v5.10), and from the dumped slab
> > info:
> > 
> >     [   26.062145] kmalloc-2k            814056KB     814056KB
> > 
> > From debug we found there are huge number of 'struct iova_magazine',
> > whose size is 1032 bytes (1024 + 8), so each allocation will waste
> > 1016 bytes. Though the issue was solved by giving the right (bigger)
> > size of RAM, it is still nice to optimize the size (either use a
> > kmalloc friendly size or create a dedicated slab for it).
> > 
> > And from lkml archive, there was another crash kernel OOM case [1]
> > back in 2019, which seems to be related with the similar slab waste
> > situation, as the log is similar:
> > 
> >     [    4.332648] iommu: Adding device 0000:20:02.0 to group 16
> >     [    4.338946] swapper/0 invoked oom-killer: gfp_mask=0x6040c0(GFP_KERNEL|__GFP_COMP), nodemask=(null), order=0, oom_score_adj=0
> >     ...
> >     [    4.857565] kmalloc-2048           59164KB      59164KB
> > 
> > The crash kernel only has 256M memory, and 59M is pretty big here.
> > (Note: the related code has been changed and optimised in recent
> > kernel [2], these logs are just picked to demo the problem, also
> > a patch changing its size to 1024 bytes has been merged)
> > 
> > So add an way to track each kmalloc's memory waste info, and
> > leverage the existing SLUB debug framework (specifically
> > SLUB_STORE_USER) to show its call stack of original allocation,
> > so that user can evaluate the waste situation, identify some hot
> > spots and optimize accordingly, for a better utilization of memory.
> > 
> > The waste info is integrated into existing interface:
> > '/sys/kernel/debug/slab/kmalloc-xx/alloc_traces', one example of
> > 'kmalloc-4k' after boot is:
> > 
> > 126 ixgbe_alloc_q_vector+0xa5/0x4a0 [ixgbe] waste=233856/1856 age=1493302/1493830/1494358 pid=1284 cpus=32 nodes=1
> >         __slab_alloc.isra.86+0x52/0x80
> >         __kmalloc_node+0x143/0x350
> >         ixgbe_alloc_q_vector+0xa5/0x4a0 [ixgbe]
> >         ixgbe_init_interrupt_scheme+0x1a6/0x730 [ixgbe]
> >         ixgbe_probe+0xc8e/0x10d0 [ixgbe]
> >         local_pci_probe+0x42/0x80
> >         work_for_cpu_fn+0x13/0x20
> >         process_one_work+0x1c5/0x390
> > 
> > which means in 'kmalloc-4k' slab, there are 126 requests of
> > 2240 bytes which got a 4KB space (wasting 1856 bytes each
> > and 233856 bytes in total). And when system starts some real
> > workload like multiple docker instances, there are more
> > severe waste.
> > 
> > [1]. https://lkml.org/lkml/2019/8/12/266
> > [2]. https://lore.kernel.org/lkml/2920df89-9975-5785-f79b-257d3052dfaf@xxxxxxxxxx/
> > 
> > [Thanks Hyeonggon for pointing out several bugs about sorting/format]
> > [Thanks Vlastimil for suggesting way to reduce memory usage of
> >  orig_size and keep it only for kmalloc objects]
> > 
> > Signed-off-by: Feng Tang <feng.tang@xxxxxxxxx>
> > Cc: Robin Murphy <robin.murphy@xxxxxxx>
> > Cc: John Garry <john.garry@xxxxxxxxxx>
> > Cc: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
> > ---
> >  include/linux/slab.h |  2 +
> >  mm/slub.c            | 94 +++++++++++++++++++++++++++++++++++++-------
> >  2 files changed, 81 insertions(+), 15 deletions(-)
> 
> 
> Would you update Documentation/mm/slub.rst as well?
> (alloc_traces part)
 
Sure, will do.

> [...]
> 
> >   */
> >  static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> > -			  unsigned long addr, struct kmem_cache_cpu *c)
> > +			  unsigned long addr, struct kmem_cache_cpu *c, unsigned int orig_size)
> >  {
> >  	void *freelist;
> >  	struct slab *slab;
> > @@ -3115,6 +3158,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> >  
> >  		if (s->flags & SLAB_STORE_USER)
> >  			set_track(s, freelist, TRACK_ALLOC, addr);
> > +		set_orig_size(s, freelist, orig_size);
> >  
> >  		return freelist;
> >  	}
> > @@ -3140,6 +3184,8 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> >  		 */
> >  		if (s->flags & SLAB_STORE_USER)
> >  			set_track(s, freelist, TRACK_ALLOC, addr);
> > +		set_orig_size(s, freelist, orig_size);
> > +
> >  		return freelist;
> >  	}
> 
> 
> This patch is okay but with patch 4, init_object() initializes redzone/poison area
> using s->object_size, and init_kmalloc_object() fixes redzone/poison area using orig_size.
> Why not do it in init_object() in the first time?
> 
> Also, updating redzone/poison area after alloc_single_from_new_slab()
> (outside list_lock, after adding slab to list) will introduce races with validation.
> 
> So I think doing set_orig_size()/init_kmalloc_object() in alloc_debug_processing() would make more sense.

Yes, this makes sense, and in v3, kmalloc redzone/poison setup was
done in alloc_debug_processing() (through init_object()). When
rebasing to v4, I met the classical problem: how to pass 'orig_size'
parameter :)

In latest 'for-next' branch, one call path for alloc_debug_processing()
is
  ___slab_alloc
    get_partial
      get_any_partial
        get_partial_node
          alloc_debug_processing

Adding 'orig_size' paramter to all these function looks horrible, and
I couldn't figure out a good way and chosed to put those ops after
'set_track()'

Thanks,
Feng

> I can miss something, please kindly let me know if I did ;)
> 
> Anything else looks good to me.
> Thanks!
> 
> -- 
> Thanks,
> Hyeonggon