| Message ID | 20210429150100.282180-3-leo.yan@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf arm-spe: Correct recording configurations | expand |
On 29/04/2021 18:00, Leo Yan wrote: > The dummy event is mainly used for mmap, the TIME sample is only needed > for per-cpu case so that the perf tool can rely on the correct timing > for parsing symbols. And the CPU sample is useless for mmap. > > This patch enables TIME sample for per-cpu mmap and doesn't enable CPU > sample. For later extension (e.g. support multiple AUX events), it sets > the dummy event when the condition "opts->full_auxtrace" is true. > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > --- > tools/perf/arch/arm64/util/arm-spe.c | 30 ++++++++++++++++------------ > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c > index 902e73a64184..f6eec0900604 100644 > --- a/tools/perf/arch/arm64/util/arm-spe.c > +++ b/tools/perf/arch/arm64/util/arm-spe.c > @@ -70,7 +70,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr, > struct evsel *evsel, *arm_spe_evsel = NULL; > struct perf_cpu_map *cpus = evlist->core.cpus; > bool privileged = perf_event_paranoid_check(-1); > - struct evsel *tracking_evsel; > int err; > > sper->evlist = evlist; > @@ -126,18 +125,23 @@ static int arm_spe_recording_options(struct auxtrace_record *itr, > evsel__set_sample_bit(arm_spe_evsel, CPU); > > /* Add dummy event to keep tracking */ > - err = parse_events(evlist, "dummy:u", NULL); > - if (err) > - return err; > - > - tracking_evsel = evlist__last(evlist); > - evlist__set_tracking_event(evlist, tracking_evsel); > - > - tracking_evsel->core.attr.freq = 0; > - tracking_evsel->core.attr.sample_period = 1; > - evsel__set_sample_bit(tracking_evsel, TIME); > - evsel__set_sample_bit(tracking_evsel, CPU); > - evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK); > + if (opts->full_auxtrace) { > + struct evsel *tracking_evsel; Hi Leo, I know the "if (opts->full_auxtrace)" pattern is copied from other auxtrace files, but I don't think it does anything because there is this at the top of the function: if (!opts->full_auxtrace) return 0; The same applies for other usages of "full_auxtrace" in the same function. They are all always true. I'm also not sure if it's ever defined what full_auxtrace means. James > + > + err = parse_events(evlist, "dummy:u", NULL); > + if (err) > + return err; > + > + tracking_evsel = evlist__last(evlist); > + evlist__set_tracking_event(evlist, tracking_evsel); > + > + tracking_evsel->core.attr.freq = 0; > + tracking_evsel->core.attr.sample_period = 1; > + > + /* In per-cpu case, always need the time of mmap events etc */ > + if (!perf_cpu_map__empty(cpus)) > + evsel__set_sample_bit(tracking_evsel, TIME); > + } > > return 0; > } >
On 12/05/2021 17:39, James Clark wrote: > > > On 29/04/2021 18:00, Leo Yan wrote: >> The dummy event is mainly used for mmap, the TIME sample is only needed [...] >> - tracking_evsel->core.attr.freq = 0; >> - tracking_evsel->core.attr.sample_period = 1; >> - evsel__set_sample_bit(tracking_evsel, TIME); >> - evsel__set_sample_bit(tracking_evsel, CPU); >> - evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK); >> + if (opts->full_auxtrace) { >> + struct evsel *tracking_evsel; > > Hi Leo, > > I know the "if (opts->full_auxtrace)" pattern is copied from other auxtrace > files, but I don't think it does anything because there is this at the top > of the function: > > if (!opts->full_auxtrace) > return 0; > > The same applies for other usages of "full_auxtrace" in the same function. > They are all always true. I'm also not sure if it's ever defined what > full_auxtrace means. > > James Apart from this issue above, I've tested the full patchset with various combinations of --per-thread, -a and --timestamp and don't see any issues like missing command names or decode issues. (Apart from -a where Command is reported as '-1', but this issue is present before and after this patchset so is unrelated.) I think it makes sense to unify the behaviour to make it more like Coresight and others so this is a good change. Reviewed-by: James Clark <james.clark@arm.com> Tested-by: James Clark <james.clark@arm.com> > >> + >> + err = parse_events(evlist, "dummy:u", NULL); >> + if (err) >> + return err; >> + >> + tracking_evsel = evlist__last(evlist); >> + evlist__set_tracking_event(evlist, tracking_evsel); >> + >> + tracking_evsel->core.attr.freq = 0; >> + tracking_evsel->core.attr.sample_period = 1; >> + >> + /* In per-cpu case, always need the time of mmap events etc */ >> + if (!perf_cpu_map__empty(cpus)) >> + evsel__set_sample_bit(tracking_evsel, TIME); >> + } >> >> return 0; >> } >>
On Wed, May 12, 2021 at 05:39:56PM +0300, James Clark wrote: > > > On 29/04/2021 18:00, Leo Yan wrote: > > The dummy event is mainly used for mmap, the TIME sample is only needed > > for per-cpu case so that the perf tool can rely on the correct timing > > for parsing symbols. And the CPU sample is useless for mmap. > > > > This patch enables TIME sample for per-cpu mmap and doesn't enable CPU > > sample. For later extension (e.g. support multiple AUX events), it sets > > the dummy event when the condition "opts->full_auxtrace" is true. > > > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > > --- > > tools/perf/arch/arm64/util/arm-spe.c | 30 ++++++++++++++++------------ > > 1 file changed, 17 insertions(+), 13 deletions(-) > > > > diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c > > index 902e73a64184..f6eec0900604 100644 > > --- a/tools/perf/arch/arm64/util/arm-spe.c > > +++ b/tools/perf/arch/arm64/util/arm-spe.c > > @@ -70,7 +70,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr, > > struct evsel *evsel, *arm_spe_evsel = NULL; > > struct perf_cpu_map *cpus = evlist->core.cpus; > > bool privileged = perf_event_paranoid_check(-1); > > - struct evsel *tracking_evsel; > > int err; > > > > sper->evlist = evlist; > > @@ -126,18 +125,23 @@ static int arm_spe_recording_options(struct auxtrace_record *itr, > > evsel__set_sample_bit(arm_spe_evsel, CPU); > > > > /* Add dummy event to keep tracking */ > > - err = parse_events(evlist, "dummy:u", NULL); > > - if (err) > > - return err; > > - > > - tracking_evsel = evlist__last(evlist); > > - evlist__set_tracking_event(evlist, tracking_evsel); > > - > > - tracking_evsel->core.attr.freq = 0; > > - tracking_evsel->core.attr.sample_period = 1; > > - evsel__set_sample_bit(tracking_evsel, TIME); > > - evsel__set_sample_bit(tracking_evsel, CPU); > > - evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK); > > + if (opts->full_auxtrace) { > > + struct evsel *tracking_evsel; > > Hi Leo, > > I know the "if (opts->full_auxtrace)" pattern is copied from other auxtrace > files, but I don't think it does anything because there is this at the top > of the function: > > if (!opts->full_auxtrace) > return 0; > > The same applies for other usages of "full_auxtrace" in the same function. > They are all always true. I'm also not sure if it's ever defined what > full_auxtrace means. Good pointing. TBH, I also stuggled for handling "opts->full_auxtrace" when wrote the patch; IIUC, "opts->full_auxtrace" is also used in builtin-record.c to indicate if the recording contains AUX tracing. Will follow your suggestion to respin the patch (and refine the code) to remove the redundant condition checking for "opts->full_auxtrace". Thanks, Leo
Hi James, On Wed, May 12, 2021 at 06:17:03PM +0300, James Clark wrote: [...] > Apart from this issue above, I've tested the full patchset with various combinations > of --per-thread, -a and --timestamp and don't see any issues like missing command names > or decode issues. (Apart from -a where Command is reported as '-1', but this issue is > present before and after this patchset so is unrelated.) Good finding! Will take a look for the failure issue with option '-a'. > I think it makes sense to unify the behaviour to make it more like Coresight and others > so this is a good change. > > Reviewed-by: James Clark <james.clark@arm.com> > Tested-by: James Clark <james.clark@arm.com> Thanks a lot for the testing, Leo
Em Wed, May 12, 2021 at 11:23:30PM +0800, Leo Yan escreveu: > On Wed, May 12, 2021 at 05:39:56PM +0300, James Clark wrote: > > > > > > On 29/04/2021 18:00, Leo Yan wrote: > > > The dummy event is mainly used for mmap, the TIME sample is only needed > > > for per-cpu case so that the perf tool can rely on the correct timing > > > for parsing symbols. And the CPU sample is useless for mmap. > > > > > > This patch enables TIME sample for per-cpu mmap and doesn't enable CPU > > > sample. For later extension (e.g. support multiple AUX events), it sets > > > the dummy event when the condition "opts->full_auxtrace" is true. > > > > > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > > > --- > > > tools/perf/arch/arm64/util/arm-spe.c | 30 ++++++++++++++++------------ > > > 1 file changed, 17 insertions(+), 13 deletions(-) > > > > > > diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c > > > index 902e73a64184..f6eec0900604 100644 > > > --- a/tools/perf/arch/arm64/util/arm-spe.c > > > +++ b/tools/perf/arch/arm64/util/arm-spe.c > > > @@ -70,7 +70,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr, > > > struct evsel *evsel, *arm_spe_evsel = NULL; > > > struct perf_cpu_map *cpus = evlist->core.cpus; > > > bool privileged = perf_event_paranoid_check(-1); > > > - struct evsel *tracking_evsel; > > > int err; > > > > > > sper->evlist = evlist; > > > @@ -126,18 +125,23 @@ static int arm_spe_recording_options(struct auxtrace_record *itr, > > > evsel__set_sample_bit(arm_spe_evsel, CPU); > > > > > > /* Add dummy event to keep tracking */ > > > - err = parse_events(evlist, "dummy:u", NULL); > > > - if (err) > > > - return err; > > > - > > > - tracking_evsel = evlist__last(evlist); > > > - evlist__set_tracking_event(evlist, tracking_evsel); > > > - > > > - tracking_evsel->core.attr.freq = 0; > > > - tracking_evsel->core.attr.sample_period = 1; > > > - evsel__set_sample_bit(tracking_evsel, TIME); > > > - evsel__set_sample_bit(tracking_evsel, CPU); > > > - evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK); > > > + if (opts->full_auxtrace) { > > > + struct evsel *tracking_evsel; > > > > Hi Leo, > > > > I know the "if (opts->full_auxtrace)" pattern is copied from other auxtrace > > files, but I don't think it does anything because there is this at the top > > of the function: > > > > if (!opts->full_auxtrace) > > return 0; > > > > The same applies for other usages of "full_auxtrace" in the same function. > > They are all always true. I'm also not sure if it's ever defined what > > full_auxtrace means. > > Good pointing. TBH, I also stuggled for handling "opts->full_auxtrace" > when wrote the patch; IIUC, "opts->full_auxtrace" is also used in > builtin-record.c to indicate if the recording contains AUX tracing. > > Will follow your suggestion to respin the patch (and refine the code) > to remove the redundant condition checking for "opts->full_auxtrace". Ok, so please collect his Tested-by and Reviewed-by and I'll wait for v2 then, - Arnaldo
On Tue, May 18, 2021 at 09:54:02AM -0300, Arnaldo Carvalho de Melo wrote: [...] > > Will follow your suggestion to respin the patch (and refine the code) > > to remove the redundant condition checking for "opts->full_auxtrace". > > Ok, so please collect his Tested-by and Reviewed-by and I'll wait for v2 > then, Sure, will send the patches in tomorrow, sorry for some delay. And thanks for reminding!
diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c index 902e73a64184..f6eec0900604 100644 --- a/tools/perf/arch/arm64/util/arm-spe.c +++ b/tools/perf/arch/arm64/util/arm-spe.c @@ -70,7 +70,6 @@ static int arm_spe_recording_options(struct auxtrace_record *itr, struct evsel *evsel, *arm_spe_evsel = NULL; struct perf_cpu_map *cpus = evlist->core.cpus; bool privileged = perf_event_paranoid_check(-1); - struct evsel *tracking_evsel; int err; sper->evlist = evlist; @@ -126,18 +125,23 @@ static int arm_spe_recording_options(struct auxtrace_record *itr, evsel__set_sample_bit(arm_spe_evsel, CPU); /* Add dummy event to keep tracking */ - err = parse_events(evlist, "dummy:u", NULL); - if (err) - return err; - - tracking_evsel = evlist__last(evlist); - evlist__set_tracking_event(evlist, tracking_evsel); - - tracking_evsel->core.attr.freq = 0; - tracking_evsel->core.attr.sample_period = 1; - evsel__set_sample_bit(tracking_evsel, TIME); - evsel__set_sample_bit(tracking_evsel, CPU); - evsel__reset_sample_bit(tracking_evsel, BRANCH_STACK); + if (opts->full_auxtrace) { + struct evsel *tracking_evsel; + + err = parse_events(evlist, "dummy:u", NULL); + if (err) + return err; + + tracking_evsel = evlist__last(evlist); + evlist__set_tracking_event(evlist, tracking_evsel); + + tracking_evsel->core.attr.freq = 0; + tracking_evsel->core.attr.sample_period = 1; + + /* In per-cpu case, always need the time of mmap events etc */ + if (!perf_cpu_map__empty(cpus)) + evsel__set_sample_bit(tracking_evsel, TIME); + } return 0; }
The dummy event is mainly used for mmap, the TIME sample is only needed for per-cpu case so that the perf tool can rely on the correct timing for parsing symbols. And the CPU sample is useless for mmap. This patch enables TIME sample for per-cpu mmap and doesn't enable CPU sample. For later extension (e.g. support multiple AUX events), it sets the dummy event when the condition "opts->full_auxtrace" is true. Signed-off-by: Leo Yan <leo.yan@linaro.org> --- tools/perf/arch/arm64/util/arm-spe.c | 30 ++++++++++++++++------------ 1 file changed, 17 insertions(+), 13 deletions(-)