Messages in this thread | | | Date | Tue, 15 Aug 2023 09:56:28 +0100 | Subject | Re: [PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use for perf list | From | John Garry <> |
| |
On 14/08/2023 17:09, Ian Rogers wrote: > On Mon, Aug 14, 2023 at 8:57 AM Ian Rogers<irogers@google.com> wrote: >> On Fri, Aug 11, 2023 at 8:51 AM John Garry<john.g.garry@oracle.com> wrote: >>> On 10/08/2023 22:49, Ian Rogers wrote: >>>> When there are multiple PMUs that differ only by suffix, by default >>>> just list the first one and skip all others. As the PMUs are sorted, >>>> the scan routine checks that the PMU names match and the numbers are >>>> consecutive. If "-v" is passed to "perf list" then list all PMUs. >>> I really think that this should be merged with the next change. I don't >>> like the intermediate step of by default only printing the first PMU. >> Ack. I'll leave it as 3 patches and then leave it to Arnaldo squash as >> quite often he wants more patches.
Why are more patches desirable? I do like the approach of taking a bite at a time, but we should also maintain ability to easily bisect and keep logical steps as one.
>> >>>> Signed-off-by: Ian Rogers<irogers@google.com> >>>> --- >>>> tools/perf/builtin-list.c | 8 +++++ >>>> tools/perf/util/pmus.c | 54 ++++++++++++++++++++++++++++++++-- >>>> tools/perf/util/print-events.h | 1 + >>>> 3 files changed, 61 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c >>>> index 7fec2cca759f..8fe4ddf02c14 100644 >>>> --- a/tools/perf/builtin-list.c >>>> +++ b/tools/perf/builtin-list.c >>>> @@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group, >>>> strbuf_release(&buf); >>>> } >>>> >>>> +static bool default_skip_duplicate_pmus(void *ps) >>>> +{ >>>> + struct print_state *print_state = ps; >>>> + >>>> + return !print_state->long_desc; >>>> +} >>>> + >>>> int cmd_list(int argc, const char **argv) >>>> { >>>> int i, ret = 0; >>>> @@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv) >>>> .print_end = default_print_end, >>>> .print_event = default_print_event, >>>> .print_metric = default_print_metric, >>>> + .skip_duplicate_pmus = default_skip_duplicate_pmus, >>>> }; >>>> const char *cputype = NULL; >>>> const char *unit_name = NULL; >>>> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c >>>> index 3581710667b0..5073843aca19 100644 >>>> --- a/tools/perf/util/pmus.c >>>> +++ b/tools/perf/util/pmus.c >>>> @@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu) >>>> return NULL; >>>> } >>>> >>>> +static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu) >>>> +{ >>>> + bool use_core_pmus = !pmu || pmu->is_core; >>>> + int last_pmu_name_len = 0; >>>> + unsigned long last_pmu_num = 0; >>>> + const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : ""; >>>> + >>>> + if (!pmu) { >>>> + pmu_read_sysfs(/*core_only=*/false); >>>> + pmu = list_prepare_entry(pmu, &core_pmus, list); >>>> + } else >>>> + last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num); >>>> + >>>> + if (use_core_pmus) { >>>> + list_for_each_entry_continue(pmu, &core_pmus, list) { >>>> + unsigned long pmu_num = 0; >>>> + int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num); >>>> + >>>> + if (last_pmu_name_len == pmu_name_len && >>>> + (last_pmu_num + 1 == pmu_num) && >>>> + !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) { >>>> + last_pmu_num++; >>>> + continue; >>>> + } >>>> + return pmu; >>>> + } >>>> + pmu = NULL; >>> you assign pmu NULL >>> >>>> + pmu = list_prepare_entry(pmu, &other_pmus, list); >>> and then re-assign it. If list_prepare_entry() needs first arg = NULL, >>> then can just use NULL explicitly? >> Done. > So because of the macro magic in list_prepare_entry you can't > explicitly pass NULL here as doing so yields: > tools/include/linux/kernel.h:36:33: error: request for member ‘list’ > in something not a structure or union > 36 | const typeof(((type *)0)->member) * __mptr = (ptr); \ > | ^~
ok, fine, so maybe add a comment as the code looks odd..
Thanks, John
| |