lkml.org 
[lkml]   [2023]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v1 00/14] Clean up libperf cpumap's empty function
    Em Wed, Dec 13, 2023 at 02:48:15PM +0200, Adrian Hunter escreveu:
    > On 12/12/23 19:59, Arnaldo Carvalho de Melo wrote:
    > > Em Tue, Nov 28, 2023 at 10:01:57PM -0800, Ian Rogers escreveu:
    > >> Rename and clean up the use of libperf CPU map functions particularly
    > >> focussing on perf_cpu_map__empty that may return true for maps
    > >> containing CPUs but also with an "any CPU"/dummy value.
    > >>
    > >> perf_cpu_map__nr is also troubling in that iterating an empty CPU map
    > >> will yield the "any CPU"/dummy value. Reduce the appearance of some
    > >> calls to this by using the perf_cpu_map__for_each_cpu macro.
    > >>
    > >> Ian Rogers (14):
    > >> libperf cpumap: Rename perf_cpu_map__dummy_new
    > >> libperf cpumap: Rename and prefer sysfs for perf_cpu_map__default_new
    > >> libperf cpumap: Rename perf_cpu_map__empty
    > >> libperf cpumap: Replace usage of perf_cpu_map__new(NULL)
    > >> libperf cpumap: Add for_each_cpu that skips the "any CPU" case
    > >
    > > Applied 1-6, with James Reviewed-by tags, would be good to have Adrian
    > > check the PT and BTS parts, testing the end result if he things its all
    > > ok.

    Ian,

    1-6 is in perf-tools-next now, can you please consider Adrian's
    suggestion to reduce patch size and rebase the remaining patches?

    - Arnaldo

    > Changing the same lines of code twice in the same patch set is not
    > really kernel style.
    >
    > Some of the churn could be reduced by applying and rebasing on the
    > patch below.
    >
    > Ideally the patches should be reordered so that the lines only
    > change once i.e.
    >
    > perf_cpu_map__empty -> <replacement>
    >
    > instead of
    >
    > perf_cpu_map__empty -> <rename> -> <replacement>
    >
    > If that is too much trouble, please accept my ack instead:
    >
    > Acked-by: Adrian Hunter <adrian.hunter@intel.com>



    >
    > From: Adrian Hunter <adrian.hunter@intel.com>
    >
    > Factor out perf_cpu_map__empty() use to reduce the occurrences and make
    > the code more readable.
    >
    > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
    > ---
    > tools/perf/arch/x86/util/intel-bts.c | 11 ++++++++---
    > tools/perf/arch/x86/util/intel-pt.c | 21 ++++++++++++---------
    > 2 files changed, 20 insertions(+), 12 deletions(-)
    >
    > diff --git a/tools/perf/arch/x86/util/intel-bts.c b/tools/perf/arch/x86/util/intel-bts.c
    > index d2c8cac11470..cebe994eb9db 100644
    > --- a/tools/perf/arch/x86/util/intel-bts.c
    > +++ b/tools/perf/arch/x86/util/intel-bts.c
    > @@ -59,6 +59,11 @@ intel_bts_info_priv_size(struct auxtrace_record *itr __maybe_unused,
    > return INTEL_BTS_AUXTRACE_PRIV_SIZE;
    > }
    >
    > +static bool intel_bts_per_cpu(struct evlist *evlist)
    > +{
    > + return !perf_cpu_map__empty(evlist->core.user_requested_cpus);
    > +}
    > +
    > static int intel_bts_info_fill(struct auxtrace_record *itr,
    > struct perf_session *session,
    > struct perf_record_auxtrace_info *auxtrace_info,
    > @@ -109,8 +114,8 @@ static int intel_bts_recording_options(struct auxtrace_record *itr,
    > struct intel_bts_recording *btsr =
    > container_of(itr, struct intel_bts_recording, itr);
    > struct perf_pmu *intel_bts_pmu = btsr->intel_bts_pmu;
    > + bool per_cpu_mmaps = intel_bts_per_cpu(evlist);
    > struct evsel *evsel, *intel_bts_evsel = NULL;
    > - const struct perf_cpu_map *cpus = evlist->core.user_requested_cpus;
    > bool privileged = perf_event_paranoid_check(-1);
    >
    > if (opts->auxtrace_sample_mode) {
    > @@ -143,7 +148,7 @@ static int intel_bts_recording_options(struct auxtrace_record *itr,
    > if (!opts->full_auxtrace)
    > return 0;
    >
    > - if (opts->full_auxtrace && !perf_cpu_map__empty(cpus)) {
    > + if (opts->full_auxtrace && per_cpu_mmaps) {
    > pr_err(INTEL_BTS_PMU_NAME " does not support per-cpu recording\n");
    > return -EINVAL;
    > }
    > @@ -224,7 +229,7 @@ static int intel_bts_recording_options(struct auxtrace_record *itr,
    > * In the case of per-cpu mmaps, we need the CPU on the
    > * AUX event.
    > */
    > - if (!perf_cpu_map__empty(cpus))
    > + if (per_cpu_mmaps)
    > evsel__set_sample_bit(intel_bts_evsel, CPU);
    > }
    >
    > diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c
    > index fa0c718b9e72..0ff9147c75da 100644
    > --- a/tools/perf/arch/x86/util/intel-pt.c
    > +++ b/tools/perf/arch/x86/util/intel-pt.c
    > @@ -312,6 +312,11 @@ static void intel_pt_tsc_ctc_ratio(u32 *n, u32 *d)
    > *d = eax;
    > }
    >
    > +static bool intel_pt_per_cpu(struct evlist *evlist)
    > +{
    > + return !perf_cpu_map__empty(evlist->core.user_requested_cpus);
    > +}
    > +
    > static int intel_pt_info_fill(struct auxtrace_record *itr,
    > struct perf_session *session,
    > struct perf_record_auxtrace_info *auxtrace_info,
    > @@ -322,7 +327,8 @@ static int intel_pt_info_fill(struct auxtrace_record *itr,
    > struct perf_pmu *intel_pt_pmu = ptr->intel_pt_pmu;
    > struct perf_event_mmap_page *pc;
    > struct perf_tsc_conversion tc = { .time_mult = 0, };
    > - bool cap_user_time_zero = false, per_cpu_mmaps;
    > + bool per_cpu_mmaps = intel_pt_per_cpu(session->evlist);
    > + bool cap_user_time_zero = false;
    > u64 tsc_bit, mtc_bit, mtc_freq_bits, cyc_bit, noretcomp_bit;
    > u32 tsc_ctc_ratio_n, tsc_ctc_ratio_d;
    > unsigned long max_non_turbo_ratio;
    > @@ -369,8 +375,6 @@ static int intel_pt_info_fill(struct auxtrace_record *itr,
    > ui__warning("Intel Processor Trace: TSC not available\n");
    > }
    >
    > - per_cpu_mmaps = !perf_cpu_map__empty(session->evlist->core.user_requested_cpus);
    > -
    > auxtrace_info->type = PERF_AUXTRACE_INTEL_PT;
    > auxtrace_info->priv[INTEL_PT_PMU_TYPE] = intel_pt_pmu->type;
    > auxtrace_info->priv[INTEL_PT_TIME_SHIFT] = tc.time_shift;
    > @@ -604,8 +608,8 @@ static int intel_pt_recording_options(struct auxtrace_record *itr,
    > struct perf_pmu *intel_pt_pmu = ptr->intel_pt_pmu;
    > bool have_timing_info, need_immediate = false;
    > struct evsel *evsel, *intel_pt_evsel = NULL;
    > - const struct perf_cpu_map *cpus = evlist->core.user_requested_cpus;
    > bool privileged = perf_event_paranoid_check(-1);
    > + bool per_cpu_mmaps = intel_pt_per_cpu(evlist);
    > u64 tsc_bit;
    > int err;
    >
    > @@ -774,8 +778,7 @@ static int intel_pt_recording_options(struct auxtrace_record *itr,
    > * Per-cpu recording needs sched_switch events to distinguish different
    > * threads.
    > */
    > - if (have_timing_info && !perf_cpu_map__empty(cpus) &&
    > - !record_opts__no_switch_events(opts)) {
    > + if (have_timing_info && per_cpu_mmaps && !record_opts__no_switch_events(opts)) {
    > if (perf_can_record_switch_events()) {
    > bool cpu_wide = !target__none(&opts->target) &&
    > !target__has_task(&opts->target);
    > @@ -832,7 +835,7 @@ static int intel_pt_recording_options(struct auxtrace_record *itr,
    > * In the case of per-cpu mmaps, we need the CPU on the
    > * AUX event.
    > */
    > - if (!perf_cpu_map__empty(cpus))
    > + if (per_cpu_mmaps)
    > evsel__set_sample_bit(intel_pt_evsel, CPU);
    > }
    >
    > @@ -858,7 +861,7 @@ static int intel_pt_recording_options(struct auxtrace_record *itr,
    > tracking_evsel->immediate = true;
    >
    > /* In per-cpu case, always need the time of mmap events etc */
    > - if (!perf_cpu_map__empty(cpus)) {
    > + if (per_cpu_mmaps) {
    > evsel__set_sample_bit(tracking_evsel, TIME);
    > /* And the CPU for switch events */
    > evsel__set_sample_bit(tracking_evsel, CPU);
    > @@ -870,7 +873,7 @@ static int intel_pt_recording_options(struct auxtrace_record *itr,
    > * Warn the user when we do not have enough information to decode i.e.
    > * per-cpu with no sched_switch (except workload-only).
    > */
    > - if (!ptr->have_sched_switch && !perf_cpu_map__empty(cpus) &&
    > + if (!ptr->have_sched_switch && per_cpu_mmaps &&
    > !target__none(&opts->target) &&
    > !intel_pt_evsel->core.attr.exclude_user)
    > ui__warning("Intel Processor Trace decoding will not be possible except for kernel tracing!\n");
    > --
    > 2.34.1
    >
    >
    >

    --

    - Arnaldo

    \
     
     \ /
      Last update: 2023-12-14 14:49    [W:2.553 / U:0.124 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site