lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8df24fe8-4d90-4105-acf0-e4f2667c42c9@linux.intel.com>
Date: Thu, 3 Oct 2024 19:29:06 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Namhyung Kim <namhyung@...nel.org>, Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Adrian Hunter <adrian.hunter@...el.com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Dapeng Mi <dapeng1.mi@...ux.intel.com>, linux-perf-users@...r.kernel.org,
 linux-kernel@...r.kernel.org, Yongwei Ma <yongwei.ma@...el.com>,
 Dapeng Mi <dapeng1.mi@...el.com>
Subject: Re: [Patch v5 0/6] Bug fixes on topdown events reordering



On 2024-10-03 6:13 p.m., Namhyung Kim wrote:
>> Dapeng's comment should cover replace the comment /* Followed by
>> topdown events. */ but there are other things amiss. I'm thinking of
>> something like: "slots,cycles,{instructions,topdown-be-bound}" the
>> topdown-be-bound should get sorted and grouped with slots, but cycles
>> and instructions have no reason to be reordered, so do we end up with
>> slots, instructions and topdown-be-bound being grouped with cycles
>> sitting ungrouped in the middle of the evlist? I believe there are
>> assumptions that grouped evsels are adjacent in the evlist, not least
>> in:
>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2106
>> Does cycles instructions end up being broken out of a group in this
>> case? Which feels like the case the code was trying to avoid.
> I got this:
> 
>   $ sudo ./perf record -a -e "slots,cycles,{instructions,topdown-be-bound}" true
>   Error:
>   The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (topdown-be-bound).
>   "dmesg | grep -i perf" may provide additional information.

To be honest, I think the "slots,cycles,{instructions,topdown-be-bound}"
is a meaningless case. Why a user wants to group instructions and
topdown events, but leave the slots out of the group?
There could be hundreds of different combinations caused by the perf
metrics mess. I don't think the re-ordering code should/can fix all of them.

For the case which the re-ordering cannot cover (like above), an error
out is acceptable. So the end user can update their command to a more
meaningful format, either {slots,cycles,instructions,topdown-be-bound}
or {slots,topdown-be-bound},cycles,instructions still works.

I think what the patch set really fixed is the failure of sample read
with perf metrics. Without the patch set, it never works no matter how
you change the order of the events.
A better ordering is just a nice to have feature. If perf cannot
provides a perfect re-ordering, I think an error out is also OK.

Thanks,
Kan

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ