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

[imran.f.khan@oracle.com]

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@oracle.com>
>> ---
>>   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@oracle.com/

Thanks for review and feedback.

-- Imran