| Message ID | 20241022155920.17511-9-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [V14,01/11] perf/x86/intel/pt: Fix buffer full but size is 0 case | expand |
Hi Adrian, On Tue, Oct 22, 2024 at 06:59:14PM +0300, Adrian Hunter wrote: > > Display "feature is not supported" error message if aux_start_paused, > aux_pause or aux_resume result in a perf_event_open() error. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > Acked-by: Ian Rogers <irogers@google.com> > Reviewed-by: Andi Kleen <ak@linux.intel.com> > --- > > > Changes in V13: > Add error message also in EOPNOTSUPP case (Leo) > > > tools/perf/util/evsel.c | 12 ++++++++++++ > tools/perf/util/evsel.h | 1 + > 2 files changed, 13 insertions(+) > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index 95593b55d9a7..88b31a005ac6 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -2102,6 +2102,12 @@ bool evsel__detect_missing_features(struct evsel *evsel) > perf_missing_features.inherit_sample_read = true; > pr_debug2("Using PERF_SAMPLE_READ / :S modifier is not compatible with inherit, falling back to no-inherit.\n"); > return true; > + } else if (!perf_missing_features.aux_pause_resume && > + (evsel->core.attr.aux_pause || evsel->core.attr.aux_resume || > + evsel->core.attr.aux_start_paused)) { > + perf_missing_features.aux_pause_resume = true; > + pr_debug2_peo("Kernel has no aux_pause/aux_resume support, bailing out\n"); > + return false; This patch fails to apply on the latest perf-tools-next branch due to conflict: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git branch: perf-tools-next You might need to rebase it on the latest code base. Thanks, Leo > } else if (!perf_missing_features.branch_counters && > (evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_COUNTERS)) { > perf_missing_features.branch_counters = true; > @@ -3279,6 +3285,10 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target, > return scnprintf(msg, size, > "%s: PMU Hardware doesn't support 'aux_output' feature", > evsel__name(evsel)); > + if (evsel->core.attr.aux_action) > + return scnprintf(msg, size, > + "%s: PMU Hardware doesn't support 'aux_action' feature", > + evsel__name(evsel)); > if (evsel->core.attr.sample_period != 0) > return scnprintf(msg, size, > "%s: PMU Hardware doesn't support sampling/overflow-interrupts. Try 'perf stat'", > @@ -3309,6 +3319,8 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target, > return scnprintf(msg, size, "clockid feature not supported."); > if (perf_missing_features.clockid_wrong) > return scnprintf(msg, size, "wrong clockid (%d).", clockid); > + if (perf_missing_features.aux_pause_resume) > + return scnprintf(msg, size, "The 'aux_pause / aux_resume' feature is not supported, update the kernel."); > if (perf_missing_features.aux_output) > return scnprintf(msg, size, "The 'aux_output' feature is not supported, update the kernel."); > if (!target__has_cpu(target)) > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > index 9fcaf417b277..87135012217d 100644 > --- a/tools/perf/util/evsel.h > +++ b/tools/perf/util/evsel.h > @@ -205,6 +205,7 @@ struct perf_missing_features { > bool weight_struct; > bool read_lost; > bool branch_counters; > + bool aux_pause_resume; > bool inherit_sample_read; > }; > > -- > 2.43.0 > >
Hello, On Fri, Nov 08, 2024 at 03:41:52PM +0000, Leo Yan wrote: > Hi Adrian, > > On Tue, Oct 22, 2024 at 06:59:14PM +0300, Adrian Hunter wrote: > > > > Display "feature is not supported" error message if aux_start_paused, > > aux_pause or aux_resume result in a perf_event_open() error. > > > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > > Acked-by: Ian Rogers <irogers@google.com> > > Reviewed-by: Andi Kleen <ak@linux.intel.com> > > --- > > > > > > Changes in V13: > > Add error message also in EOPNOTSUPP case (Leo) > > > > > > tools/perf/util/evsel.c | 12 ++++++++++++ > > tools/perf/util/evsel.h | 1 + > > 2 files changed, 13 insertions(+) > > > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > > index 95593b55d9a7..88b31a005ac6 100644 > > --- a/tools/perf/util/evsel.c > > +++ b/tools/perf/util/evsel.c > > @@ -2102,6 +2102,12 @@ bool evsel__detect_missing_features(struct evsel *evsel) > > perf_missing_features.inherit_sample_read = true; > > pr_debug2("Using PERF_SAMPLE_READ / :S modifier is not compatible with inherit, falling back to no-inherit.\n"); > > return true; > > + } else if (!perf_missing_features.aux_pause_resume && > > + (evsel->core.attr.aux_pause || evsel->core.attr.aux_resume || > > + evsel->core.attr.aux_start_paused)) { > > + perf_missing_features.aux_pause_resume = true; > > + pr_debug2_peo("Kernel has no aux_pause/aux_resume support, bailing out\n"); > > + return false; > > This patch fails to apply on the latest perf-tools-next branch due to > conflict: > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git > branch: perf-tools-next > > You might need to rebase it on the latest code base. Yep, please do for the tooling patches. Thanks, Namhyung
On 8/11/24 19:39, Namhyung Kim wrote: > Hello, > > On Fri, Nov 08, 2024 at 03:41:52PM +0000, Leo Yan wrote: >> Hi Adrian, >> >> On Tue, Oct 22, 2024 at 06:59:14PM +0300, Adrian Hunter wrote: >>> >>> Display "feature is not supported" error message if aux_start_paused, >>> aux_pause or aux_resume result in a perf_event_open() error. >>> >>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>> Acked-by: Ian Rogers <irogers@google.com> >>> Reviewed-by: Andi Kleen <ak@linux.intel.com> >>> --- >>> >>> >>> Changes in V13: >>> Add error message also in EOPNOTSUPP case (Leo) >>> >>> >>> tools/perf/util/evsel.c | 12 ++++++++++++ >>> tools/perf/util/evsel.h | 1 + >>> 2 files changed, 13 insertions(+) >>> >>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c >>> index 95593b55d9a7..88b31a005ac6 100644 >>> --- a/tools/perf/util/evsel.c >>> +++ b/tools/perf/util/evsel.c >>> @@ -2102,6 +2102,12 @@ bool evsel__detect_missing_features(struct evsel *evsel) >>> perf_missing_features.inherit_sample_read = true; >>> pr_debug2("Using PERF_SAMPLE_READ / :S modifier is not compatible with inherit, falling back to no-inherit.\n"); >>> return true; >>> + } else if (!perf_missing_features.aux_pause_resume && >>> + (evsel->core.attr.aux_pause || evsel->core.attr.aux_resume || >>> + evsel->core.attr.aux_start_paused)) { >>> + perf_missing_features.aux_pause_resume = true; >>> + pr_debug2_peo("Kernel has no aux_pause/aux_resume support, bailing out\n"); >>> + return false; >> >> This patch fails to apply on the latest perf-tools-next branch due to >> conflict: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git >> branch: perf-tools-next >> >> You might need to rebase it on the latest code base. > > Yep, please do for the tooling patches. I'd noticed that, but there is more work to sort it out. The probing simply won't work when there are dependencies on other events, or other attr members. But there is no point in trying to "fallback" in that case either. It is just a failure, but what *is* still needed is a sensible error message. The other patches, including the ones that come after, still apply by the way, or they did the other day, so they could be applied anyway.
On Fri, Nov 08, 2024 at 08:01:02PM +0200, Adrian Hunter wrote: > On 8/11/24 19:39, Namhyung Kim wrote: > > Hello, > > > > On Fri, Nov 08, 2024 at 03:41:52PM +0000, Leo Yan wrote: > >> Hi Adrian, > >> > >> On Tue, Oct 22, 2024 at 06:59:14PM +0300, Adrian Hunter wrote: > >>> > >>> Display "feature is not supported" error message if aux_start_paused, > >>> aux_pause or aux_resume result in a perf_event_open() error. > >>> > >>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > >>> Acked-by: Ian Rogers <irogers@google.com> > >>> Reviewed-by: Andi Kleen <ak@linux.intel.com> > >>> --- > >>> > >>> > >>> Changes in V13: > >>> Add error message also in EOPNOTSUPP case (Leo) > >>> > >>> > >>> tools/perf/util/evsel.c | 12 ++++++++++++ > >>> tools/perf/util/evsel.h | 1 + > >>> 2 files changed, 13 insertions(+) > >>> > >>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > >>> index 95593b55d9a7..88b31a005ac6 100644 > >>> --- a/tools/perf/util/evsel.c > >>> +++ b/tools/perf/util/evsel.c > >>> @@ -2102,6 +2102,12 @@ bool evsel__detect_missing_features(struct evsel *evsel) > >>> perf_missing_features.inherit_sample_read = true; > >>> pr_debug2("Using PERF_SAMPLE_READ / :S modifier is not compatible with inherit, falling back to no-inherit.\n"); > >>> return true; > >>> + } else if (!perf_missing_features.aux_pause_resume && > >>> + (evsel->core.attr.aux_pause || evsel->core.attr.aux_resume || > >>> + evsel->core.attr.aux_start_paused)) { > >>> + perf_missing_features.aux_pause_resume = true; > >>> + pr_debug2_peo("Kernel has no aux_pause/aux_resume support, bailing out\n"); > >>> + return false; > >> > >> This patch fails to apply on the latest perf-tools-next branch due to > >> conflict: > >> > >> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git > >> branch: perf-tools-next > >> > >> You might need to rebase it on the latest code base. > > > > Yep, please do for the tooling patches. > > I'd noticed that, but there is more work to sort it out. > The probing simply won't work when there are dependencies > on other events, or other attr members. But there is no > point in trying to "fallback" in that case either. It is > just a failure, but what *is* still needed is a sensible > error message. Do you need to probe with two or more events? I guess we can extend evsel__detect_missing_pmu_features to check more complicated setups. For now, it only checks exclude_guest attribute. > > The other patches, including the ones that come after, > still apply by the way, or they did the other day, so > they could be applied anyway. Do you want me to process the series without this patch? I think this is needed to support old kernels and I'd like to have it together. Would be hard for you to update this change in the current form and work on improving it later? Thanks, Namhyung
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index 95593b55d9a7..88b31a005ac6 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -2102,6 +2102,12 @@ bool evsel__detect_missing_features(struct evsel *evsel) perf_missing_features.inherit_sample_read = true; pr_debug2("Using PERF_SAMPLE_READ / :S modifier is not compatible with inherit, falling back to no-inherit.\n"); return true; + } else if (!perf_missing_features.aux_pause_resume && + (evsel->core.attr.aux_pause || evsel->core.attr.aux_resume || + evsel->core.attr.aux_start_paused)) { + perf_missing_features.aux_pause_resume = true; + pr_debug2_peo("Kernel has no aux_pause/aux_resume support, bailing out\n"); + return false; } else if (!perf_missing_features.branch_counters && (evsel->core.attr.branch_sample_type & PERF_SAMPLE_BRANCH_COUNTERS)) { perf_missing_features.branch_counters = true; @@ -3279,6 +3285,10 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target, return scnprintf(msg, size, "%s: PMU Hardware doesn't support 'aux_output' feature", evsel__name(evsel)); + if (evsel->core.attr.aux_action) + return scnprintf(msg, size, + "%s: PMU Hardware doesn't support 'aux_action' feature", + evsel__name(evsel)); if (evsel->core.attr.sample_period != 0) return scnprintf(msg, size, "%s: PMU Hardware doesn't support sampling/overflow-interrupts. Try 'perf stat'", @@ -3309,6 +3319,8 @@ int evsel__open_strerror(struct evsel *evsel, struct target *target, return scnprintf(msg, size, "clockid feature not supported."); if (perf_missing_features.clockid_wrong) return scnprintf(msg, size, "wrong clockid (%d).", clockid); + if (perf_missing_features.aux_pause_resume) + return scnprintf(msg, size, "The 'aux_pause / aux_resume' feature is not supported, update the kernel."); if (perf_missing_features.aux_output) return scnprintf(msg, size, "The 'aux_output' feature is not supported, update the kernel."); if (!target__has_cpu(target)) diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h index 9fcaf417b277..87135012217d 100644 --- a/tools/perf/util/evsel.h +++ b/tools/perf/util/evsel.h @@ -205,6 +205,7 @@ struct perf_missing_features { bool weight_struct; bool read_lost; bool branch_counters; + bool aux_pause_resume; bool inherit_sample_read; };