lkml.org 
[lkml]   [2023]   [Aug]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use for perf list
From
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

\
 
 \ /
  Last update: 2023-08-15 11:00    [W:0.043 / U:2.272 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site