* [PATCH v2 0/2] perf hist: Fix bogus profiles when filters are enabled @ 2025-01-13 13:34 Dmitry Vyukov 2025-01-13 13:34 ` [PATCH v2 1/2] perf hist: Deduplicate cmp/sort/collapse code Dmitry Vyukov 2025-01-13 13:34 ` [PATCH v2 2/2] perf hist: Fix bogus profiles when filters are enabled Dmitry Vyukov 0 siblings, 2 replies; 5+ messages in thread From: Dmitry Vyukov @ 2025-01-13 13:34 UTC (permalink / raw) To: namhyung, irogers; +Cc: linux-perf-users, linux-kernel, Dmitry Vyukov *** BLURB HERE *** Dmitry Vyukov (2): perf hist: Deduplicate cmp/sort/collapse code perf hist: Fix bogus profiles when filters are enabled tools/perf/util/hist.c | 114 +++++++++++++++++++---------------------- tools/perf/util/hist.h | 14 +++-- 2 files changed, 59 insertions(+), 69 deletions(-) base-commit: 5bc55a333a2f7316b58edc7573e8e893f7acb532 -- 2.47.1.688.g23fc6f90ad-goog ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] perf hist: Deduplicate cmp/sort/collapse code 2025-01-13 13:34 [PATCH v2 0/2] perf hist: Fix bogus profiles when filters are enabled Dmitry Vyukov @ 2025-01-13 13:34 ` Dmitry Vyukov 2025-01-15 1:07 ` Namhyung Kim 2025-01-13 13:34 ` [PATCH v2 2/2] perf hist: Fix bogus profiles when filters are enabled Dmitry Vyukov 1 sibling, 1 reply; 5+ messages in thread From: Dmitry Vyukov @ 2025-01-13 13:34 UTC (permalink / raw) To: namhyung, irogers; +Cc: linux-perf-users, linux-kernel, Dmitry Vyukov Application of cmp/sort/collapse fmt callbacks is duplicated 6 times. Factor it into a common helper function. NFC. Signed-off-by: Dmitry Vyukov <dvyukov@google.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Ian Rogers <irogers@google.com> Cc: linux-perf-users@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- Changes in v2: - add perf_hpp_fmt_cmp_t typedef to util/hist.h - use the typedef for perf_hpp_fmt::{cmp,collapse,sort} --- tools/perf/util/hist.c | 103 +++++++++++++++++------------------------ tools/perf/util/hist.h | 14 +++--- 2 files changed, 49 insertions(+), 68 deletions(-) diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index fff1345658016..a4b8ec3805565 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -32,6 +32,9 @@ #include <linux/time64.h> #include <linux/zalloc.h> +static int64_t hist_entry__cmp(struct hist_entry *left, struct hist_entry *right); +static int64_t hist_entry__collapse(struct hist_entry *left, struct hist_entry *right); + static bool hists__filter_entry_by_dso(struct hists *hists, struct hist_entry *he); static bool hists__filter_entry_by_thread(struct hists *hists, @@ -1292,19 +1295,26 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al, return err; } -int64_t -hist_entry__cmp(struct hist_entry *left, struct hist_entry *right) +static int64_t +hist_entry__cmp_impl(struct perf_hpp_list *hpp_list, struct hist_entry *left, + struct hist_entry *right, unsigned long fn_offset, + bool ignore_dynamic, bool ignore_skipped) { struct hists *hists = left->hists; struct perf_hpp_fmt *fmt; int64_t cmp = 0; + perf_hpp_fmt_cmp_t fn; - hists__for_each_sort_list(hists, fmt) { - if (perf_hpp__is_dynamic_entry(fmt) && + perf_hpp_list__for_each_sort_list(hpp_list, fmt) { + if (ignore_dynamic && perf_hpp__is_dynamic_entry(fmt) && !perf_hpp__defined_dynamic_entry(fmt, hists)) continue; - cmp = fmt->cmp(fmt, left, right); + if (ignore_skipped && perf_hpp__should_skip(fmt, hists)) + continue; + + fn = *(perf_hpp_fmt_cmp_t *)(((char *)fmt) + fn_offset); + cmp = fn(fmt, left, right); if (cmp) break; } @@ -1313,23 +1323,33 @@ hist_entry__cmp(struct hist_entry *left, struct hist_entry *right) } int64_t -hist_entry__collapse(struct hist_entry *left, struct hist_entry *right) +hist_entry__cmp(struct hist_entry *left, struct hist_entry *right) { - struct hists *hists = left->hists; - struct perf_hpp_fmt *fmt; - int64_t cmp = 0; + return hist_entry__cmp_impl(left->hists->hpp_list, left, right, + offsetof(struct perf_hpp_fmt, cmp), true, false); +} - hists__for_each_sort_list(hists, fmt) { - if (perf_hpp__is_dynamic_entry(fmt) && - !perf_hpp__defined_dynamic_entry(fmt, hists)) - continue; +static int64_t +hist_entry__sort(struct hist_entry *left, struct hist_entry *right) +{ + return hist_entry__cmp_impl(left->hists->hpp_list, left, right, + offsetof(struct perf_hpp_fmt, sort), false, true); +} - cmp = fmt->collapse(fmt, left, right); - if (cmp) - break; - } +int64_t +hist_entry__collapse(struct hist_entry *left, struct hist_entry *right) +{ + return hist_entry__cmp_impl(left->hists->hpp_list, left, right, + offsetof(struct perf_hpp_fmt, collapse), true, false); +} - return cmp; +static int64_t +hist_entry__collapse_hierarchy(struct perf_hpp_list *hpp_list, + struct hist_entry *left, + struct hist_entry *right) +{ + return hist_entry__cmp_impl(hpp_list, left, right, + offsetof(struct perf_hpp_fmt, collapse), false, false); } void hist_entry__delete(struct hist_entry *he) @@ -1503,14 +1523,7 @@ static struct hist_entry *hierarchy_insert_entry(struct hists *hists, while (*p != NULL) { parent = *p; iter = rb_entry(parent, struct hist_entry, rb_node_in); - - cmp = 0; - perf_hpp_list__for_each_sort_list(hpp_list, fmt) { - cmp = fmt->collapse(fmt, iter, he); - if (cmp) - break; - } - + cmp = hist_entry__collapse_hierarchy(hpp_list, iter, he); if (!cmp) { he_stat__add_stat(&iter->stat, &he->stat); return iter; @@ -1730,24 +1743,6 @@ int hists__collapse_resort(struct hists *hists, struct ui_progress *prog) return 0; } -static int64_t hist_entry__sort(struct hist_entry *a, struct hist_entry *b) -{ - struct hists *hists = a->hists; - struct perf_hpp_fmt *fmt; - int64_t cmp = 0; - - hists__for_each_sort_list(hists, fmt) { - if (perf_hpp__should_skip(fmt, a->hists)) - continue; - - cmp = fmt->sort(fmt, a, b); - if (cmp) - break; - } - - return cmp; -} - static void hists__reset_filter_stats(struct hists *hists) { hists->nr_non_filtered_entries = 0; @@ -2449,21 +2444,15 @@ static struct hist_entry *add_dummy_hierarchy_entry(struct hists *hists, struct rb_node **p; struct rb_node *parent = NULL; struct hist_entry *he; - struct perf_hpp_fmt *fmt; bool leftmost = true; p = &root->rb_root.rb_node; while (*p != NULL) { - int64_t cmp = 0; + int64_t cmp; parent = *p; he = rb_entry(parent, struct hist_entry, rb_node_in); - - perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) { - cmp = fmt->collapse(fmt, he, pair); - if (cmp) - break; - } + cmp = hist_entry__collapse_hierarchy(he->hpp_list, he, pair); if (!cmp) goto out; @@ -2521,16 +2510,10 @@ static struct hist_entry *hists__find_hierarchy_entry(struct rb_root_cached *roo while (n) { struct hist_entry *iter; - struct perf_hpp_fmt *fmt; - int64_t cmp = 0; + int64_t cmp; iter = rb_entry(n, struct hist_entry, rb_node_in); - perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) { - cmp = fmt->collapse(fmt, iter, he); - if (cmp) - break; - } - + cmp = hist_entry__collapse_hierarchy(he->hpp_list, iter, he); if (cmp < 0) n = n->rb_left; else if (cmp > 0) diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h index 1131056924d9c..46c8373e31465 100644 --- a/tools/perf/util/hist.h +++ b/tools/perf/util/hist.h @@ -342,8 +342,6 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al, struct perf_hpp; struct perf_hpp_fmt; -int64_t hist_entry__cmp(struct hist_entry *left, struct hist_entry *right); -int64_t hist_entry__collapse(struct hist_entry *left, struct hist_entry *right); int hist_entry__transaction_len(void); int hist_entry__sort_snprintf(struct hist_entry *he, char *bf, size_t size, struct hists *hists); @@ -452,6 +450,9 @@ struct perf_hpp { bool skip; }; +typedef int64_t (*perf_hpp_fmt_cmp_t)( + struct perf_hpp_fmt *, struct hist_entry *, struct hist_entry *); + struct perf_hpp_fmt { const char *name; int (*header)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp, @@ -463,12 +464,9 @@ struct perf_hpp_fmt { struct hist_entry *he); int (*entry)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp, struct hist_entry *he); - int64_t (*cmp)(struct perf_hpp_fmt *fmt, - struct hist_entry *a, struct hist_entry *b); - int64_t (*collapse)(struct perf_hpp_fmt *fmt, - struct hist_entry *a, struct hist_entry *b); - int64_t (*sort)(struct perf_hpp_fmt *fmt, - struct hist_entry *a, struct hist_entry *b); + perf_hpp_fmt_cmp_t cmp; + perf_hpp_fmt_cmp_t collapse; + perf_hpp_fmt_cmp_t sort; bool (*equal)(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b); void (*free)(struct perf_hpp_fmt *fmt); -- 2.47.1.688.g23fc6f90ad-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] perf hist: Deduplicate cmp/sort/collapse code 2025-01-13 13:34 ` [PATCH v2 1/2] perf hist: Deduplicate cmp/sort/collapse code Dmitry Vyukov @ 2025-01-15 1:07 ` Namhyung Kim 2025-01-15 8:02 ` Dmitry Vyukov 0 siblings, 1 reply; 5+ messages in thread From: Namhyung Kim @ 2025-01-15 1:07 UTC (permalink / raw) To: Dmitry Vyukov; +Cc: irogers, linux-perf-users, linux-kernel On Mon, Jan 13, 2025 at 02:34:14PM +0100, Dmitry Vyukov wrote: > Application of cmp/sort/collapse fmt callbacks is duplicated 6 times. > Factor it into a common helper function. NFC. > > Signed-off-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Ian Rogers <irogers@google.com> > Cc: linux-perf-users@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > > --- > > Changes in v2: > - add perf_hpp_fmt_cmp_t typedef to util/hist.h > - use the typedef for perf_hpp_fmt::{cmp,collapse,sort} > --- > tools/perf/util/hist.c | 103 +++++++++++++++++------------------------ > tools/perf/util/hist.h | 14 +++--- > 2 files changed, 49 insertions(+), 68 deletions(-) > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c > index fff1345658016..a4b8ec3805565 100644 > --- a/tools/perf/util/hist.c > +++ b/tools/perf/util/hist.c > @@ -32,6 +32,9 @@ > #include <linux/time64.h> > #include <linux/zalloc.h> > > +static int64_t hist_entry__cmp(struct hist_entry *left, struct hist_entry *right); > +static int64_t hist_entry__collapse(struct hist_entry *left, struct hist_entry *right); > + > static bool hists__filter_entry_by_dso(struct hists *hists, > struct hist_entry *he); > static bool hists__filter_entry_by_thread(struct hists *hists, > @@ -1292,19 +1295,26 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al, > return err; > } > > -int64_t > -hist_entry__cmp(struct hist_entry *left, struct hist_entry *right) > +static int64_t > +hist_entry__cmp_impl(struct perf_hpp_list *hpp_list, struct hist_entry *left, > + struct hist_entry *right, unsigned long fn_offset, > + bool ignore_dynamic, bool ignore_skipped) Maybe it's a matter of tastes, but I think it could be simpler like below. > { > struct hists *hists = left->hists; > struct perf_hpp_fmt *fmt; > int64_t cmp = 0; > + perf_hpp_fmt_cmp_t fn; perf_hpp_fmt_cmp_t *fn; > > - hists__for_each_sort_list(hists, fmt) { > - if (perf_hpp__is_dynamic_entry(fmt) && > + perf_hpp_list__for_each_sort_list(hpp_list, fmt) { > + if (ignore_dynamic && perf_hpp__is_dynamic_entry(fmt) && > !perf_hpp__defined_dynamic_entry(fmt, hists)) > continue; > > - cmp = fmt->cmp(fmt, left, right); > + if (ignore_skipped && perf_hpp__should_skip(fmt, hists)) > + continue; > + > + fn = *(perf_hpp_fmt_cmp_t *)(((char *)fmt) + fn_offset); > + cmp = fn(fmt, left, right); fn = (void *)fmt + fn_offset; cmp = (*fn)(fmt, left, right); Otherwise, looks good to me. Thanks, Namhyung > if (cmp) > break; > } > @@ -1313,23 +1323,33 @@ hist_entry__cmp(struct hist_entry *left, struct hist_entry *right) > } > > int64_t > -hist_entry__collapse(struct hist_entry *left, struct hist_entry *right) > +hist_entry__cmp(struct hist_entry *left, struct hist_entry *right) > { > - struct hists *hists = left->hists; > - struct perf_hpp_fmt *fmt; > - int64_t cmp = 0; > + return hist_entry__cmp_impl(left->hists->hpp_list, left, right, > + offsetof(struct perf_hpp_fmt, cmp), true, false); > +} > > - hists__for_each_sort_list(hists, fmt) { > - if (perf_hpp__is_dynamic_entry(fmt) && > - !perf_hpp__defined_dynamic_entry(fmt, hists)) > - continue; > +static int64_t > +hist_entry__sort(struct hist_entry *left, struct hist_entry *right) > +{ > + return hist_entry__cmp_impl(left->hists->hpp_list, left, right, > + offsetof(struct perf_hpp_fmt, sort), false, true); > +} > > - cmp = fmt->collapse(fmt, left, right); > - if (cmp) > - break; > - } > +int64_t > +hist_entry__collapse(struct hist_entry *left, struct hist_entry *right) > +{ > + return hist_entry__cmp_impl(left->hists->hpp_list, left, right, > + offsetof(struct perf_hpp_fmt, collapse), true, false); > +} > > - return cmp; > +static int64_t > +hist_entry__collapse_hierarchy(struct perf_hpp_list *hpp_list, > + struct hist_entry *left, > + struct hist_entry *right) > +{ > + return hist_entry__cmp_impl(hpp_list, left, right, > + offsetof(struct perf_hpp_fmt, collapse), false, false); > } > > void hist_entry__delete(struct hist_entry *he) > @@ -1503,14 +1523,7 @@ static struct hist_entry *hierarchy_insert_entry(struct hists *hists, > while (*p != NULL) { > parent = *p; > iter = rb_entry(parent, struct hist_entry, rb_node_in); > - > - cmp = 0; > - perf_hpp_list__for_each_sort_list(hpp_list, fmt) { > - cmp = fmt->collapse(fmt, iter, he); > - if (cmp) > - break; > - } > - > + cmp = hist_entry__collapse_hierarchy(hpp_list, iter, he); > if (!cmp) { > he_stat__add_stat(&iter->stat, &he->stat); > return iter; > @@ -1730,24 +1743,6 @@ int hists__collapse_resort(struct hists *hists, struct ui_progress *prog) > return 0; > } > > -static int64_t hist_entry__sort(struct hist_entry *a, struct hist_entry *b) > -{ > - struct hists *hists = a->hists; > - struct perf_hpp_fmt *fmt; > - int64_t cmp = 0; > - > - hists__for_each_sort_list(hists, fmt) { > - if (perf_hpp__should_skip(fmt, a->hists)) > - continue; > - > - cmp = fmt->sort(fmt, a, b); > - if (cmp) > - break; > - } > - > - return cmp; > -} > - > static void hists__reset_filter_stats(struct hists *hists) > { > hists->nr_non_filtered_entries = 0; > @@ -2449,21 +2444,15 @@ static struct hist_entry *add_dummy_hierarchy_entry(struct hists *hists, > struct rb_node **p; > struct rb_node *parent = NULL; > struct hist_entry *he; > - struct perf_hpp_fmt *fmt; > bool leftmost = true; > > p = &root->rb_root.rb_node; > while (*p != NULL) { > - int64_t cmp = 0; > + int64_t cmp; > > parent = *p; > he = rb_entry(parent, struct hist_entry, rb_node_in); > - > - perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) { > - cmp = fmt->collapse(fmt, he, pair); > - if (cmp) > - break; > - } > + cmp = hist_entry__collapse_hierarchy(he->hpp_list, he, pair); > if (!cmp) > goto out; > > @@ -2521,16 +2510,10 @@ static struct hist_entry *hists__find_hierarchy_entry(struct rb_root_cached *roo > > while (n) { > struct hist_entry *iter; > - struct perf_hpp_fmt *fmt; > - int64_t cmp = 0; > + int64_t cmp; > > iter = rb_entry(n, struct hist_entry, rb_node_in); > - perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) { > - cmp = fmt->collapse(fmt, iter, he); > - if (cmp) > - break; > - } > - > + cmp = hist_entry__collapse_hierarchy(he->hpp_list, iter, he); > if (cmp < 0) > n = n->rb_left; > else if (cmp > 0) > diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h > index 1131056924d9c..46c8373e31465 100644 > --- a/tools/perf/util/hist.h > +++ b/tools/perf/util/hist.h > @@ -342,8 +342,6 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al, > struct perf_hpp; > struct perf_hpp_fmt; > > -int64_t hist_entry__cmp(struct hist_entry *left, struct hist_entry *right); > -int64_t hist_entry__collapse(struct hist_entry *left, struct hist_entry *right); > int hist_entry__transaction_len(void); > int hist_entry__sort_snprintf(struct hist_entry *he, char *bf, size_t size, > struct hists *hists); > @@ -452,6 +450,9 @@ struct perf_hpp { > bool skip; > }; > > +typedef int64_t (*perf_hpp_fmt_cmp_t)( > + struct perf_hpp_fmt *, struct hist_entry *, struct hist_entry *); > + > struct perf_hpp_fmt { > const char *name; > int (*header)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp, > @@ -463,12 +464,9 @@ struct perf_hpp_fmt { > struct hist_entry *he); > int (*entry)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp, > struct hist_entry *he); > - int64_t (*cmp)(struct perf_hpp_fmt *fmt, > - struct hist_entry *a, struct hist_entry *b); > - int64_t (*collapse)(struct perf_hpp_fmt *fmt, > - struct hist_entry *a, struct hist_entry *b); > - int64_t (*sort)(struct perf_hpp_fmt *fmt, > - struct hist_entry *a, struct hist_entry *b); > + perf_hpp_fmt_cmp_t cmp; > + perf_hpp_fmt_cmp_t collapse; > + perf_hpp_fmt_cmp_t sort; > bool (*equal)(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b); > void (*free)(struct perf_hpp_fmt *fmt); > > -- > 2.47.1.688.g23fc6f90ad-goog > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] perf hist: Deduplicate cmp/sort/collapse code 2025-01-15 1:07 ` Namhyung Kim @ 2025-01-15 8:02 ` Dmitry Vyukov 0 siblings, 0 replies; 5+ messages in thread From: Dmitry Vyukov @ 2025-01-15 8:02 UTC (permalink / raw) To: Namhyung Kim; +Cc: irogers, linux-perf-users, linux-kernel On Wed, 15 Jan 2025 at 02:07, Namhyung Kim <namhyung@kernel.org> wrote: > > On Mon, Jan 13, 2025 at 02:34:14PM +0100, Dmitry Vyukov wrote: > > Application of cmp/sort/collapse fmt callbacks is duplicated 6 times. > > Factor it into a common helper function. NFC. > > > > Signed-off-by: Dmitry Vyukov <dvyukov@google.com> > > Cc: Namhyung Kim <namhyung@kernel.org> > > Cc: Ian Rogers <irogers@google.com> > > Cc: linux-perf-users@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > > > --- > > > > Changes in v2: > > - add perf_hpp_fmt_cmp_t typedef to util/hist.h > > - use the typedef for perf_hpp_fmt::{cmp,collapse,sort} > > --- > > tools/perf/util/hist.c | 103 +++++++++++++++++------------------------ > > tools/perf/util/hist.h | 14 +++--- > > 2 files changed, 49 insertions(+), 68 deletions(-) > > > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c > > index fff1345658016..a4b8ec3805565 100644 > > --- a/tools/perf/util/hist.c > > +++ b/tools/perf/util/hist.c > > @@ -32,6 +32,9 @@ > > #include <linux/time64.h> > > #include <linux/zalloc.h> > > > > +static int64_t hist_entry__cmp(struct hist_entry *left, struct hist_entry *right); > > +static int64_t hist_entry__collapse(struct hist_entry *left, struct hist_entry *right); > > + > > static bool hists__filter_entry_by_dso(struct hists *hists, > > struct hist_entry *he); > > static bool hists__filter_entry_by_thread(struct hists *hists, > > @@ -1292,19 +1295,26 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al, > > return err; > > } > > > > -int64_t > > -hist_entry__cmp(struct hist_entry *left, struct hist_entry *right) > > +static int64_t > > +hist_entry__cmp_impl(struct perf_hpp_list *hpp_list, struct hist_entry *left, > > + struct hist_entry *right, unsigned long fn_offset, > > + bool ignore_dynamic, bool ignore_skipped) > > Maybe it's a matter of tastes, but I think it could be simpler like > below. > > > { > > struct hists *hists = left->hists; > > struct perf_hpp_fmt *fmt; > > int64_t cmp = 0; > > + perf_hpp_fmt_cmp_t fn; > > perf_hpp_fmt_cmp_t *fn; > > > > > - hists__for_each_sort_list(hists, fmt) { > > - if (perf_hpp__is_dynamic_entry(fmt) && > > + perf_hpp_list__for_each_sort_list(hpp_list, fmt) { > > + if (ignore_dynamic && perf_hpp__is_dynamic_entry(fmt) && > > !perf_hpp__defined_dynamic_entry(fmt, hists)) > > continue; > > > > - cmp = fmt->cmp(fmt, left, right); > > + if (ignore_skipped && perf_hpp__should_skip(fmt, hists)) > > + continue; > > + > > + fn = *(perf_hpp_fmt_cmp_t *)(((char *)fmt) + fn_offset); > > + cmp = fn(fmt, left, right); > > fn = (void *)fmt + fn_offset; > cmp = (*fn)(fmt, left, right); Done in v3. > Otherwise, looks good to me. Thanks for the review. > Thanks, > Namhyung > > > if (cmp) > > break; > > } > > @@ -1313,23 +1323,33 @@ hist_entry__cmp(struct hist_entry *left, struct hist_entry *right) > > } > > > > int64_t > > -hist_entry__collapse(struct hist_entry *left, struct hist_entry *right) > > +hist_entry__cmp(struct hist_entry *left, struct hist_entry *right) > > { > > - struct hists *hists = left->hists; > > - struct perf_hpp_fmt *fmt; > > - int64_t cmp = 0; > > + return hist_entry__cmp_impl(left->hists->hpp_list, left, right, > > + offsetof(struct perf_hpp_fmt, cmp), true, false); > > +} > > > > - hists__for_each_sort_list(hists, fmt) { > > - if (perf_hpp__is_dynamic_entry(fmt) && > > - !perf_hpp__defined_dynamic_entry(fmt, hists)) > > - continue; > > +static int64_t > > +hist_entry__sort(struct hist_entry *left, struct hist_entry *right) > > +{ > > + return hist_entry__cmp_impl(left->hists->hpp_list, left, right, > > + offsetof(struct perf_hpp_fmt, sort), false, true); > > +} > > > > - cmp = fmt->collapse(fmt, left, right); > > - if (cmp) > > - break; > > - } > > +int64_t > > +hist_entry__collapse(struct hist_entry *left, struct hist_entry *right) > > +{ > > + return hist_entry__cmp_impl(left->hists->hpp_list, left, right, > > + offsetof(struct perf_hpp_fmt, collapse), true, false); > > +} > > > > - return cmp; > > +static int64_t > > +hist_entry__collapse_hierarchy(struct perf_hpp_list *hpp_list, > > + struct hist_entry *left, > > + struct hist_entry *right) > > +{ > > + return hist_entry__cmp_impl(hpp_list, left, right, > > + offsetof(struct perf_hpp_fmt, collapse), false, false); > > } > > > > void hist_entry__delete(struct hist_entry *he) > > @@ -1503,14 +1523,7 @@ static struct hist_entry *hierarchy_insert_entry(struct hists *hists, > > while (*p != NULL) { > > parent = *p; > > iter = rb_entry(parent, struct hist_entry, rb_node_in); > > - > > - cmp = 0; > > - perf_hpp_list__for_each_sort_list(hpp_list, fmt) { > > - cmp = fmt->collapse(fmt, iter, he); > > - if (cmp) > > - break; > > - } > > - > > + cmp = hist_entry__collapse_hierarchy(hpp_list, iter, he); > > if (!cmp) { > > he_stat__add_stat(&iter->stat, &he->stat); > > return iter; > > @@ -1730,24 +1743,6 @@ int hists__collapse_resort(struct hists *hists, struct ui_progress *prog) > > return 0; > > } > > > > -static int64_t hist_entry__sort(struct hist_entry *a, struct hist_entry *b) > > -{ > > - struct hists *hists = a->hists; > > - struct perf_hpp_fmt *fmt; > > - int64_t cmp = 0; > > - > > - hists__for_each_sort_list(hists, fmt) { > > - if (perf_hpp__should_skip(fmt, a->hists)) > > - continue; > > - > > - cmp = fmt->sort(fmt, a, b); > > - if (cmp) > > - break; > > - } > > - > > - return cmp; > > -} > > - > > static void hists__reset_filter_stats(struct hists *hists) > > { > > hists->nr_non_filtered_entries = 0; > > @@ -2449,21 +2444,15 @@ static struct hist_entry *add_dummy_hierarchy_entry(struct hists *hists, > > struct rb_node **p; > > struct rb_node *parent = NULL; > > struct hist_entry *he; > > - struct perf_hpp_fmt *fmt; > > bool leftmost = true; > > > > p = &root->rb_root.rb_node; > > while (*p != NULL) { > > - int64_t cmp = 0; > > + int64_t cmp; > > > > parent = *p; > > he = rb_entry(parent, struct hist_entry, rb_node_in); > > - > > - perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) { > > - cmp = fmt->collapse(fmt, he, pair); > > - if (cmp) > > - break; > > - } > > + cmp = hist_entry__collapse_hierarchy(he->hpp_list, he, pair); > > if (!cmp) > > goto out; > > > > @@ -2521,16 +2510,10 @@ static struct hist_entry *hists__find_hierarchy_entry(struct rb_root_cached *roo > > > > while (n) { > > struct hist_entry *iter; > > - struct perf_hpp_fmt *fmt; > > - int64_t cmp = 0; > > + int64_t cmp; > > > > iter = rb_entry(n, struct hist_entry, rb_node_in); > > - perf_hpp_list__for_each_sort_list(he->hpp_list, fmt) { > > - cmp = fmt->collapse(fmt, iter, he); > > - if (cmp) > > - break; > > - } > > - > > + cmp = hist_entry__collapse_hierarchy(he->hpp_list, iter, he); > > if (cmp < 0) > > n = n->rb_left; > > else if (cmp > 0) > > diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h > > index 1131056924d9c..46c8373e31465 100644 > > --- a/tools/perf/util/hist.h > > +++ b/tools/perf/util/hist.h > > @@ -342,8 +342,6 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al, > > struct perf_hpp; > > struct perf_hpp_fmt; > > > > -int64_t hist_entry__cmp(struct hist_entry *left, struct hist_entry *right); > > -int64_t hist_entry__collapse(struct hist_entry *left, struct hist_entry *right); > > int hist_entry__transaction_len(void); > > int hist_entry__sort_snprintf(struct hist_entry *he, char *bf, size_t size, > > struct hists *hists); > > @@ -452,6 +450,9 @@ struct perf_hpp { > > bool skip; > > }; > > > > +typedef int64_t (*perf_hpp_fmt_cmp_t)( > > + struct perf_hpp_fmt *, struct hist_entry *, struct hist_entry *); > > + > > struct perf_hpp_fmt { > > const char *name; > > int (*header)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp, > > @@ -463,12 +464,9 @@ struct perf_hpp_fmt { > > struct hist_entry *he); > > int (*entry)(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp, > > struct hist_entry *he); > > - int64_t (*cmp)(struct perf_hpp_fmt *fmt, > > - struct hist_entry *a, struct hist_entry *b); > > - int64_t (*collapse)(struct perf_hpp_fmt *fmt, > > - struct hist_entry *a, struct hist_entry *b); > > - int64_t (*sort)(struct perf_hpp_fmt *fmt, > > - struct hist_entry *a, struct hist_entry *b); > > + perf_hpp_fmt_cmp_t cmp; > > + perf_hpp_fmt_cmp_t collapse; > > + perf_hpp_fmt_cmp_t sort; > > bool (*equal)(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b); > > void (*free)(struct perf_hpp_fmt *fmt); > > > > -- > > 2.47.1.688.g23fc6f90ad-goog > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] perf hist: Fix bogus profiles when filters are enabled 2025-01-13 13:34 [PATCH v2 0/2] perf hist: Fix bogus profiles when filters are enabled Dmitry Vyukov 2025-01-13 13:34 ` [PATCH v2 1/2] perf hist: Deduplicate cmp/sort/collapse code Dmitry Vyukov @ 2025-01-13 13:34 ` Dmitry Vyukov 1 sibling, 0 replies; 5+ messages in thread From: Dmitry Vyukov @ 2025-01-13 13:34 UTC (permalink / raw) To: namhyung, irogers; +Cc: linux-perf-users, linux-kernel, Dmitry Vyukov When a filtered column is not present in the sort order, profiles become arbitrary broken. Filtered and non-filtered entries are collapsed together, and the filtered-by field ends up with a random value (either from a filtered or non-filtered entry). If we end up with filtered entry/value, then the whole collapsed entry will be filtered out and will be missing in the profile. If we end up with non-filtered entry/value, then the overhead value will be wrongly larger (include some subset of filtered out samples). This leads to very confusing profiles. The problem is hard to notice, and if noticed hard to understand. If the filter is for a single value, then it can be fixed by adding the corresponding field to the sort order (provided user understood the problem). But if the filter is for multiple values, it's impossible to fix b/c there is no concept of binary sorting based on filter predicate (we want to group all non-filtered values in one bucket, and all filtered values in another). Examples of affected commands: perf report --tid=123 perf report --sort overhead,symbol --comm=foo,bar Fix this by considering filtered status as the highest priority sort/collapse predicate. As a side effect this effectively adds a new feature of showing profile where several lines are combined based on arbitrary filtering predicate. For example, showing symbols from binaries foo and bar combined together, but not from other binaries; or showing combined overhead of several particular threads. Signed-off-by: Dmitry Vyukov <dvyukov@google.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Ian Rogers <irogers@google.com> Cc: linux-perf-users@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- tools/perf/util/hist.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index a4b8ec3805565..4d82882a020f6 100644 --- a/tools/perf/util/hist.c +++ b/tools/perf/util/hist.c @@ -1302,9 +1302,18 @@ hist_entry__cmp_impl(struct perf_hpp_list *hpp_list, struct hist_entry *left, { struct hists *hists = left->hists; struct perf_hpp_fmt *fmt; - int64_t cmp = 0; + int64_t cmp; perf_hpp_fmt_cmp_t fn; + /* + * Never collapse filtered and non-filtered entries. + * Note this is not the same as having an extra (invisible) fmt + * that corresponds to the filtered status. + */ + cmp = (int64_t)!!left->filtered - (int64_t)!!right->filtered; + if (cmp) + return cmp; + perf_hpp_list__for_each_sort_list(hpp_list, fmt) { if (ignore_dynamic && perf_hpp__is_dynamic_entry(fmt) && !perf_hpp__defined_dynamic_entry(fmt, hists)) -- 2.47.1.688.g23fc6f90ad-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-01-15 8:02 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-13 13:34 [PATCH v2 0/2] perf hist: Fix bogus profiles when filters are enabled Dmitry Vyukov 2025-01-13 13:34 ` [PATCH v2 1/2] perf hist: Deduplicate cmp/sort/collapse code Dmitry Vyukov 2025-01-15 1:07 ` Namhyung Kim 2025-01-15 8:02 ` Dmitry Vyukov 2025-01-13 13:34 ` [PATCH v2 2/2] perf hist: Fix bogus profiles when filters are enabled Dmitry Vyukov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.