On Sat, Aug 03, 2024 at 04:51:45PM +0200, Marco Elver wrote:
>
> typo: convenience
>
> What do you mean by "object leak"?
>
It means an allocated object of slab memory which is considered orphan,
perhaps it's more clear to say "For a convenience of tracing memory leaks by kfence",
what do you think?
> From what I see the additional info is only printed on out-of-bounds access.
>
> Or do you mean when you inspect /sys/kernel/debug/kfence/objects? If
> so, that information would be useful in the commit message.
>
The extra elapsed time of current allocated object would be useful to figure out memory
leaks when inspect /sys/kernel/debug/kfence/objects.
> However, to detect leaks there are better tools than KFENCE. Have you
> tried KMEMLEAK? KFENCE is really not a good choice to manually look
> for old objects, which themselves are sampled, to find leaks.
> Have you been able to successfully debug a leak this way?
>
The kmemleak tool has limitations and drawbacks which cannot be used in productive environment
directly. KFENCE is a good choice to find leaks in productive environment.
> > alloacted objectes in kfence_print_stack().
>
> typo: allocated objects
>
Thank's for your comment.
>
> In principle, the additonal info is convenient, but I'd like to
> generalize if possible.
>
> > + u64 interval_nsec = local_clock() - meta->alloc_track.ts_nsec;
> > + unsigned long rem_interval_nsec = do_div(interval_nsec, NSEC_PER_SEC);
> > +
> > + seq_con_printf(seq, "%s by task %d on cpu %d at %lu.%06lus (age: %lu.%06lus):\n",
>
> I've found myself trying to figure out the elapsed time since the
> allocation or free, based on the current timestamp.
>
> So something that would be more helpful is if you just change the
> printed line for all alloc and free stack infos to say something like:
>
> seq_con_printf(seq, "%s by task %d on cpu %d at %lu.%06lus
> (%lu.%06lus ago):\n",
>
> So rather than saying this info is the "age", we just say the elapsed
> time. That generalizes this bit of info, and it'll be available for
> both alloc and free stacks.
>
> Does that work for you?
>
It does not work for me actually, since it's unintuitive to figure out memory leaks
by the elapsed time of allocated stacks when inspect /sys/kernel/debug/kfence/objects.
It's unnecessary to print the elapsed time of allocated stacks for the objects in KFENCE_OBJECT_FREED
state. For the elapsed time of free stacks, it seems no available scenarion currently.
BTW, The change from "age" to "ago" is okay to me!
> > show_alloc ? "allocated" : "freed", track->pid,
> > - track->cpu, (unsigned long)ts_sec, rem_nsec / 1000);
> > + track->cpu, (unsigned long)ts_sec, rem_nsec / 1000,
> > + (unsigned long)interval_nsec, rem_interval_nsec / 1000);
> > + } else
>
> Add braces {} even though it's a single statement - it spans several
> lines and the above is also {}-enclosed, so it looks balanced.
>
> But if you follow my suggestion, you won't have the else branch anymore.
>
I'm not opposed to convert a single statement, but it will have an effort to find leaks.
This change will be helpful to find leaks easier by grep "ago" keyword when inspect /sys/kernel/debug/kfence/objects.
Thanks
Qiwu
