Messages in this thread | | | From | Ian Rogers <> | Date | Wed, 14 Jun 2023 19:04:23 -0700 | Subject | Re: [PATCH] perf top & record: Fix segfault when default cycles event is not supported |
| |
On Wed, Jun 14, 2023 at 6:55 PM Yang Jihong <yangjihong1@huawei.com> wrote: > > Hello, > > On 2023/6/15 6:03, Ian Rogers wrote: > > On Wed, Jun 14, 2023 at 9:18 AM Ian Rogers <irogers@google.com> wrote: > >> > >> On Wed, Jun 14, 2023 at 8:18 AM Yang Jihong <yangjihong1@huawei.com> wrote: > >>> > >>> The perf-record and perf-top call parse_event() to add a cycles event to > >>> an empty evlist. For the system that does not support hardware cycles > >>> event, such as QEMU, the evlist is empty due to the following code process: > >>> > >>> parse_event(evlist, "cycles:P" or ""cycles:Pu") > >>> parse_events(evlist, "cycles:P") > >>> __parse_events > >>> ... > >>> ret = parse_events__scanner(str, &parse_state); > >>> // ret = 0 > >>> ... > >>> ret2 = parse_events__sort_events_and_fix_groups() > >>> if (ret2 < 0) > >>> return ret; > >>> // The cycles event is not supported, here ret2 = -EINVAL, > >>> // Here return 0. > >>> ... > >>> evlist__splice_list_tail(evlist) > >>> // The code here does not execute to, so the evlist is still empty. > >>> > >>> A null pointer occurs when the content in the evlist is accessed later. > >>> > >>> Before: > >>> > >>> # perf list hw > >>> > >>> List of pre-defined events (to be used in -e or -M): > >>> > >>> # perf record true > >>> libperf: Miscounted nr_mmaps 0 vs 1 > >>> WARNING: No sample_id_all support, falling back to unordered processing > >>> perf: Segmentation fault > >>> Obtained 1 stack frames. > >>> [0xc5beff] > >>> Segmentation fault > >>> > >>> Solution: > >>> If cycles event is not supported, try to fall back to cpu-clock event. > >>> > >>> After: > >>> # perf record true > >>> [ perf record: Woken up 1 times to write data ] > >>> [ perf record: Captured and wrote 0.006 MB perf.data ] > >>> # > >>> > >>> Fixes: 7b100989b4f6 ("perf evlist: Remove __evlist__add_default") > >>> Signed-off-by: Yang Jihong <yangjihong1@huawei.com> > >> > >> Thanks, useful addition. The cpu-clock fall back wasn't present before > >> 7b100989b4f6 so is the fixes tag correct? > > > > Hmm... it should be coming from evsel__fallback: > > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/evsel.c?h=tmp.perf-tools-next#n2840 > > so we shouldn't duplicate that logic. The question is why we're not > > doing the fallback. > > > > Yes, it's a bit of the same logic as evsel__fallback, or we can call > evlist__add_default() as before, simply create an evsel of hardware > cycles and add it directly to evlist. > > Please confirm whether this solution is feasible. If it is feasible, I > will send a v2 version.
The previous evlist__add_default logic didn't handle wildcard PMUs for cycles, hence wanting to reuse the parse events logic. The error is that the logic now isn't doing the fallback properly. I think an evlist__add_cycles which uses evsel__fallback makes sense matching the previous logic. I'd be happy if you took a look. I'll write a patch so that the perf_pmus list of core PMUs is never empty.
Thanks, Ian
> Thanks, > Yang
| |