On Mon, Aug 05, 2024 at 04:18:47PM +0200, Marco Elver wrote: > On Mon, 5 Aug 2024 at 16:06, chenqiwu <qiwuchen55@xxxxxxxxx> wrote: > > > > On Mon, Aug 05, 2024 at 08:50:57AM +0200, Marco Elver wrote: > > > > > > The "allocated/freed" info is superfluous, as freed objects will have > > > a free stack. > > > > > > Consider a slightly better script vs. just using grep. > > Well, I think using grep is eaiser than a script to find leaks by a > > large number of alloc tracks. > > Sure. But a slightly more complex script is a better trade-off vs. > impacting _all_ KFENCE users world-wide with slightly less readable > error reports. > > > > /sys/kernel/debug/kfence/objects is of secondary concern and was added > > > primarily as a debugging aid for KFENCE developers. We never thought > > > it could be used to look for leaks, but good you found another use for > > > it. ;-) > > > The priority is to keep regular error reports generated by KFENCE > > > readable. Adding this "allocated/freed" info just makes the line > > > longer and is not useful. > > > > > How about print meta->state directly to get the object state for its > > alloc/free track? > > - seq_con_printf(seq, "%s by task %d on cpu %d at %lu.%06lus:\n", > > + seq_con_printf(seq, "%s by task %d on cpu %d at %lu.%06lus (%lu.%06lus ago) state %d:\n", > > 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, > > + meta->state); > > > I'm happy with the "(%lu.%06lus ago)" part alone. > > If it's still a not good idea, I will follow your suggestion and resend > > it as v2. > > No, that's just making it more ugly for no reason. It's replicating > the state info (just like before) for alloc and free stacks and > generally does not add anything useful. > > See, we are writing code that is deployed on millions of machines, and > KFENCE error reports do appear in the wild occasionally. We have to > optimize for the common case. > > Your change might be useful for you, which is a relatively unique > usecase. The common use case of KFENCE is to detect memory-safety > errors, and good error reports are a major feature of KFENCE. All > information is already present in the reports (and > /sys/kernel/debug/kfence/objects). > > I argue that you are able to write a slightly more complex script that > simply looks for the free stack right after the allocation stack to > determine if an object is live or freed. Maybe doing it in bash won't > work so nicely, but a small Python script can easily do that job. Once > you have that Python script you might even do further processing, sort > things by age, size, etc. etc., and then print whole stack traces. > Just grep can't do that. So if you want something useful, you'd have > to give up on grep sooner or later. Well, I will consider a script to realize my request based on your suggestion. Please help review patch v2 for KFENCE common case which updates the commit message. Thanks Qiwu
