Messages in this thread | | | Date | Fri, 18 Feb 2022 11:54:54 +0100 | Subject | Re: [PATCH 2/9] lib/ref_tracker: compact stacktraces before printing | From | Andrzej Hajda <> |
| |
On 17.02.2022 16:23, Eric Dumazet wrote: > On Thu, Feb 17, 2022 at 6:05 AM Andrzej Hajda <andrzej.hajda@intel.com> wrote: >> In cases references are taken alternately on multiple exec paths leak >> report can grow substantially, sorting and grouping leaks by stack_handle >> allows to compact it. >> >> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> >> Reviewed-by: Chris Wilson <chris.p.wilson@intel.com> >> --- >> lib/ref_tracker.c | 35 +++++++++++++++++++++++++++-------- >> 1 file changed, 27 insertions(+), 8 deletions(-) >> >> diff --git a/lib/ref_tracker.c b/lib/ref_tracker.c >> index 1b0c6d645d64a..0e9c7d2828ccb 100644 >> --- a/lib/ref_tracker.c >> +++ b/lib/ref_tracker.c >> @@ -1,5 +1,6 @@ >> // SPDX-License-Identifier: GPL-2.0-or-later >> #include <linux/export.h> >> +#include <linux/list_sort.h> >> #include <linux/ref_tracker.h> >> #include <linux/slab.h> >> #include <linux/stacktrace.h> >> @@ -14,23 +15,41 @@ struct ref_tracker { >> depot_stack_handle_t free_stack_handle; >> }; >> >> +static int ref_tracker_cmp(void *priv, const struct list_head *a, const struct list_head *b) >> +{ >> + const struct ref_tracker *ta = list_entry(a, const struct ref_tracker, head); >> + const struct ref_tracker *tb = list_entry(b, const struct ref_tracker, head); >> + >> + return ta->alloc_stack_handle - tb->alloc_stack_handle; >> +} >> + >> void __ref_tracker_dir_print(struct ref_tracker_dir *dir, >> unsigned int display_limit) >> { >> + unsigned int i = 0, count = 0; >> struct ref_tracker *tracker; >> - unsigned int i = 0; >> + depot_stack_handle_t stack; >> >> lockdep_assert_held(&dir->lock); >> >> + if (list_empty(&dir->list)) >> + return; >> + >> + list_sort(NULL, &dir->list, ref_tracker_cmp); > What is going to be the cost of sorting a list with 1,000,000 items in it ?
Do we really have such cases?
> > I just want to make sure we do not trade printing at most ~10 references > (from netdev_wait_allrefs()) to a soft lockup :/ with no useful info > if something went terribly wrong. > > I suggest that you do not sort a potential big list, and instead > attempt to allocate an array of @display_limits 'struct stack_counts' > > I suspect @display_limits will always be kept to a reasonable value > (less than 100 ?)
I though rather about 16 :) In theory everything is possible, but do we have real case examples which could lead to 100 stack traces? Maybe some frameworks used by multiple consumers (drivers) ???
> > struct stack_counts { > depot_stack_handle_t stack_handle; > unsigned int count; > } > > Then, iterating the list and update the array (that you can keep > sorted by ->stack_handle) > > Then after iterating, print the (at_most) @display_limits handles > found in the temp array.
OK, could be faster and less invasive. Other solution would be keeping the array in dir and update in every tracker alloc/free, this way we avoid iteration over potentially big list, but it would cost memory and since printing is rather rare I am not sure if it is worth.
I will try your proposition.
Regards Andrzej
> >> + >> list_for_each_entry(tracker, &dir->list, head) { >> - if (i < display_limit) { >> - pr_err("leaked reference.\n"); >> - if (tracker->alloc_stack_handle) >> - stack_depot_print(tracker->alloc_stack_handle); >> - i++; >> - } else { >> + if (i++ >= display_limit) >> break; >> - } >> + if (!count++) >> + stack = tracker->alloc_stack_handle; >> + if (stack == tracker->alloc_stack_handle && >> + !list_is_last(&tracker->head, &dir->list)) >> + continue; >> + >> + pr_err("leaked %d references.\n", count); >> + if (stack) >> + stack_depot_print(stack); >> + count = 0; >> } >> } >> EXPORT_SYMBOL(__ref_tracker_dir_print); >> -- >> 2.25.1 >>
| |