Re: [RFC PATCH 2/2] mm, slub: Use stackdepot to store user information for slub object.

[imran . f . khan]

On 31/8/21 10:06 pm, Vlastimil Babka wrote:
> On 8/31/21 08:25, Imran Khan wrote:
> > SLAB_STORE_USER causes information about allocating and freeing context
> > of a slub object, to be stored in metadata area in a couple of struct
> > track objects. These objects store allocation and/or freeing stack trace
> > in an array. This may result in same stack trace getting stored in metadata
> > area of multiple objects.
> > STACKDEPOT can be used to store unique stack traces without any
> > duplication,so use STACKDEPOT to store allocation and/or freeing stack
> > traces as well.
> > This results in low memory footprint, as we are not storing multiple
> > copies of the same stack trace for an allocation or free.
> >
> > Signed-off-by: Imran Khan <imran.f.khan@xxxxxxxxxx>
> > ---
> >   mm/slub.c | 87 ++++++++++++++++++++++++++++++-------------------------
> >   1 file changed, 48 insertions(+), 39 deletions(-)
[...]
> > +
> > +static void print_stack(depot_stack_handle_t stack)
> > +{
> > +	unsigned long *entries;
> > +	unsigned int nr_entries;
> > +
> > +	nr_entries = stack_depot_fetch(stack, &entries);
> > +	stack_trace_print(entries, nr_entries, 0);
> > +}
>
> This function could become part of stackdepot itself?
Okay. I have made this function part of stackdepot in my new patch set.
Please see [1].
> > +#endif
> > +
> >   static struct track *get_track(struct kmem_cache *s, void *object,
> >   	enum track_item alloc)
[...]
> > @@ -4297,19 +4310,15 @@ void kmem_obj_info(struct kmem_obj_info *kpp, void *object, struct page *page)
> >   	objp = fixup_red_left(s, objp);
> >   	trackp = get_track(s, objp, TRACK_ALLOC);
> >   	kpp->kp_ret = (void *)trackp->addr;
> > -#ifdef CONFIG_STACKTRACE
> > -	for (i = 0; i < KS_ADDRS_COUNT && i < TRACK_ADDRS_COUNT; i++) {
> > -		kpp->kp_stack[i] = (void *)trackp->addrs[i];
> > -		if (!kpp->kp_stack[i])
> > -			break;
> > -	}
> > +#ifdef CONFIG_STACKDEPOT
> > +	nr_entries = stack_depot_fetch(trackp->stack, &entries);
> > +	for (i = 0; i < nr_entries; i++)
> > +		kpp->kp_stack[i] = (void *)entries[i];
>
> Hmm, in case stack_depot_save() fails and returns a zero handle (e.g. due to
> enomem) this seems to rely on stack_depot_fetch() returning gracefully with
> zero nr_entries for a zero handle. But I don't see such guarantee?
> stack_depot_init() isn't creating such entry and stack_depot_save() doesn't
> have such check. So it will work accidentally, or return garbage? But it
> would be IMHO useful to add such guarantee to stackdepot one way or another.
I have addressed this scenario as well in my new patch set. Please see [1].
Since both of the changes suggested here pertain to stackdepot and are 
unrelated to SLUB, I have posted those changes in a separate thread [1].

> >   	trackp = get_track(s, objp, TRACK_FREE);
> > -	for (i = 0; i < KS_ADDRS_COUNT && i < TRACK_ADDRS_COUNT; i++) {
> > -		kpp->kp_free_stack[i] = (void *)trackp->addrs[i];
> > -		if (!kpp->kp_free_stack[i])
> > -			break;
> > -	}
> > +	nr_entries = stack_depot_fetch(trackp->stack, &entries);
> > +	for (i = 0; i < nr_entries; i++)
> > +		kpp->kp_free_stack[i] = (void *)entries[i];
> >   #endif
> >   #endif
> >   }
[1] 
https://lore.kernel.org/lkml/20210901051914.971603-1-imran.f.khan@xxxxxxxxxx/

Thanks for review and feedback.

-- Imran