| Message ID | 20230424134748.228137-3-james.clark@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf: cs-etm: Fixes around timestamped and timeless decoding | expand |
On 24/04/23 16:47, James Clark wrote: > There is some duplicated code to only override config values if they > haven't already been set by the user so make a util function for this. > > Signed-off-by: James Clark <james.clark@arm.com> One minor comment, nevertheless: Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > tools/perf/arch/arm64/util/arm-spe.c | 26 ++----------------------- > tools/perf/arch/x86/util/intel-pt.c | 22 ++------------------- > tools/perf/util/evsel.c | 29 ++++++++++++++++++++++++++++ > tools/perf/util/evsel.h | 3 +++ > 4 files changed, 36 insertions(+), 44 deletions(-) > > diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c > index ef497a29e814..3b1676ff03f9 100644 > --- a/tools/perf/arch/arm64/util/arm-spe.c > +++ b/tools/perf/arch/arm64/util/arm-spe.c > @@ -36,29 +36,6 @@ struct arm_spe_recording { > bool *wrapped; > }; > > -static void arm_spe_set_timestamp(struct auxtrace_record *itr, > - struct evsel *evsel) > -{ > - struct arm_spe_recording *ptr; > - struct perf_pmu *arm_spe_pmu; > - struct evsel_config_term *term = evsel__get_config_term(evsel, CFG_CHG); > - u64 user_bits = 0, bit; > - > - ptr = container_of(itr, struct arm_spe_recording, itr); > - arm_spe_pmu = ptr->arm_spe_pmu; > - > - if (term) > - user_bits = term->val.cfg_chg; > - > - bit = perf_pmu__format_bits(&arm_spe_pmu->format, "ts_enable"); > - > - /* Skip if user has set it */ > - if (bit & user_bits) > - return; > - > - evsel->core.attr.config |= bit; > -} > - > static size_t > arm_spe_info_priv_size(struct auxtrace_record *itr __maybe_unused, > struct evlist *evlist __maybe_unused) > @@ -238,7 +215,8 @@ static int arm_spe_recording_options(struct auxtrace_record *itr, > */ > if (!perf_cpu_map__empty(cpus)) { > evsel__set_sample_bit(arm_spe_evsel, CPU); > - arm_spe_set_timestamp(itr, arm_spe_evsel); > + evsel__set_config_if_unset(arm_spe_pmu, arm_spe_evsel, > + "ts_enable", 1); > } > > /* > diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c > index 2cff11de9d8a..17336da08b58 100644 > --- a/tools/perf/arch/x86/util/intel-pt.c > +++ b/tools/perf/arch/x86/util/intel-pt.c > @@ -576,25 +576,6 @@ static int intel_pt_validate_config(struct perf_pmu *intel_pt_pmu, > return err; > } > > -static void intel_pt_config_sample_mode(struct perf_pmu *intel_pt_pmu, > - struct evsel *evsel) > -{ > - u64 user_bits = 0, bits; > - struct evsel_config_term *term = evsel__get_config_term(evsel, CFG_CHG); > - > - if (term) > - user_bits = term->val.cfg_chg; > - > - bits = perf_pmu__format_bits(&intel_pt_pmu->format, "psb_period"); > - > - /* Did user change psb_period */ > - if (bits & user_bits) > - return; > - > - /* Set psb_period to 0 */ > - evsel->core.attr.config &= ~bits; > -} > - > static void intel_pt_min_max_sample_sz(struct evlist *evlist, > size_t *min_sz, size_t *max_sz) > { > @@ -686,7 +667,8 @@ static int intel_pt_recording_options(struct auxtrace_record *itr, > return 0; > > if (opts->auxtrace_sample_mode) > - intel_pt_config_sample_mode(intel_pt_pmu, intel_pt_evsel); > + evsel__set_config_if_unset(intel_pt_pmu, intel_pt_evsel, > + "psb_period", 0); > > err = intel_pt_validate_config(intel_pt_pmu, intel_pt_evsel); > if (err) > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index a85a987128aa..cdf1445136ad 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -3216,3 +3216,32 @@ void evsel__remove_from_group(struct evsel *evsel, struct evsel *leader) > leader->core.nr_members--; > } > } > + > +/* > + * Set @config_name to @val as long as the user hasn't already set or cleared it > + * by passing a config term on the command line. > + * > + * @val is the value to put into the bits specified by @config_name rather than > + * the bit pattern. It is shifted into position by this function, so to set > + * something to true, pass 1 for val rather than a pre shifted value. > + */ > +#define field_prep(_mask, _val) (((_val) << (ffsll(_mask) - 1)) & (_mask)) I notice there is already tools/include/linux/bitfield.h so may FIELD_PREP from there could be used? > +void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel, > + const char *config_name, u64 val) > +{ > + u64 user_bits = 0, bits; > + struct evsel_config_term *term = evsel__get_config_term(evsel, CFG_CHG); > + > + if (term) > + user_bits = term->val.cfg_chg; > + > + bits = perf_pmu__format_bits(&pmu->format, config_name); > + > + /* Do nothing if the user changed the value */ > + if (bits & user_bits) > + return; > + > + /* Otherwise replace it */ > + evsel->core.attr.config &= ~bits; > + evsel->core.attr.config |= field_prep(bits, val); > +} > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h > index 68072ec655ce..4120f5ff673d 100644 > --- a/tools/perf/util/evsel.h > +++ b/tools/perf/util/evsel.h > @@ -529,4 +529,7 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel); > ((((src) >> (pos)) & ((1ull << (size)) - 1)) << (63 - ((pos) + (size) - 1))) > > u64 evsel__bitfield_swap_branch_flags(u64 value); > +void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel, > + const char *config_name, u64 val); > + > #endif /* __PERF_EVSEL_H */
On 24/04/2023 16:36, Adrian Hunter wrote: > On 24/04/23 16:47, James Clark wrote: >> There is some duplicated code to only override config values if they >> haven't already been set by the user so make a util function for this. >> >> Signed-off-by: James Clark <james.clark@arm.com> > > One minor comment, nevertheless: > > Acked-by: Adrian Hunter <adrian.hunter@intel.com> > Thanks for the review >> --- >> tools/perf/arch/arm64/util/arm-spe.c | 26 ++----------------------- >> tools/perf/arch/x86/util/intel-pt.c | 22 ++------------------- >> tools/perf/util/evsel.c | 29 ++++++++++++++++++++++++++++ >> tools/perf/util/evsel.h | 3 +++ >> 4 files changed, 36 insertions(+), 44 deletions(-) >> [...] >> } >> } >> + >> +/* >> + * Set @config_name to @val as long as the user hasn't already set or cleared it >> + * by passing a config term on the command line. >> + * >> + * @val is the value to put into the bits specified by @config_name rather than >> + * the bit pattern. It is shifted into position by this function, so to set >> + * something to true, pass 1 for val rather than a pre shifted value. >> + */ >> +#define field_prep(_mask, _val) (((_val) << (ffsll(_mask) - 1)) & (_mask)) > > I notice there is already tools/include/linux/bitfield.h > so may FIELD_PREP from there could be used? I tried that first, but it seems like quite a lot of effort went into it to make it work on const only values so it doesn't work here. There is this [1] change to make a non const one but it doesn't look like it went anywhere: [1]: https://patchwork.kernel.org/project/linux-omap/patch/3a54a6703879d10f08cf0275a2a69297ebd2b1d4.1637592133.git.geert+renesas@glider.be/#24610749
Em Mon, Apr 24, 2023 at 06:36:14PM +0300, Adrian Hunter escreveu: > On 24/04/23 16:47, James Clark wrote: > > There is some duplicated code to only override config values if they > > haven't already been set by the user so make a util function for this. > > > > Signed-off-by: James Clark <james.clark@arm.com> > > One minor comment, nevertheless: > > Acked-by: Adrian Hunter <adrian.hunter@intel.com> I just moved to evsel__set_config_if_unset() to util/pmu.c, next to some other evsel__ functions to not break the python.so binding, before I was getting: [acme@quaco perf-tools-next]$ perf test -v python Couldn't bump rlimit(MEMLOCK), failures may take place when creating BPF maps, etc 19: 'import perf' in python : --- start --- test child forked, pid 500086 python usage test: "echo "import sys ; sys.path.append('/tmp/build/perf-tools-next/python'); import perf" | '/usr/bin/python3' " Traceback (most recent call last): File "<stdin>", line 1, in <module> ImportError: /tmp/build/perf-tools-next/python/perf.cpython-311-x86_64-linux-gnu.so: undefined symbol: perf_pmu__format_bits test child finished with -1 ---- end ---- 'import perf' in python: FAILED! [acme@quaco perf-tools-next]$ Please run 'perf test' and 'make -C tools/perf build-test' prior to sending pull requests, Thanks, applied. - Arnaldo
On 24/04/2023 18:45, Arnaldo Carvalho de Melo wrote: > Em Mon, Apr 24, 2023 at 06:36:14PM +0300, Adrian Hunter escreveu: >> On 24/04/23 16:47, James Clark wrote: >>> There is some duplicated code to only override config values if they >>> haven't already been set by the user so make a util function for this. >>> >>> Signed-off-by: James Clark <james.clark@arm.com> >> >> One minor comment, nevertheless: >> >> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > > I just moved to evsel__set_config_if_unset() to util/pmu.c, next to > some other evsel__ functions to not break the python.so binding, before > I was getting: > > [acme@quaco perf-tools-next]$ perf test -v python > Couldn't bump rlimit(MEMLOCK), failures may take place when creating BPF maps, etc > 19: 'import perf' in python : > --- start --- > test child forked, pid 500086 > python usage test: "echo "import sys ; sys.path.append('/tmp/build/perf-tools-next/python'); import perf" | '/usr/bin/python3' " > Traceback (most recent call last): > File "<stdin>", line 1, in <module> > ImportError: /tmp/build/perf-tools-next/python/perf.cpython-311-x86_64-linux-gnu.so: undefined symbol: perf_pmu__format_bits > test child finished with -1 > ---- end ---- > 'import perf' in python: FAILED! > [acme@quaco perf-tools-next]$ > > Please run 'perf test' and 'make -C tools/perf build-test' prior to > sending pull requests, > > Thanks, applied. > Ah sorry! I ran it from an in source build and got the python import error so I ignored that test. I see the new error if I run it from tools/perf instead. Interestingly with an out of source build it doesn't matter which cwd you run the Python test from because $OUTPUT is an absolute path. Normally I do an out of source build, but the Coresight tests don't currently work with that. Which I will submit another fix for... I don't know if it's worth getting rid of that edge by making sure PYTHONPATH is always absolute even for in source builds or if it will break something else like a make install? It's because of this line: -DPYTHONPATH="BUILD_STR($(OUTPUT)python)" Will make sure that they all pass next time. I also sent a fix for the build-test target on my platform. > - Arnaldo >
diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c index ef497a29e814..3b1676ff03f9 100644 --- a/tools/perf/arch/arm64/util/arm-spe.c +++ b/tools/perf/arch/arm64/util/arm-spe.c @@ -36,29 +36,6 @@ struct arm_spe_recording { bool *wrapped; }; -static void arm_spe_set_timestamp(struct auxtrace_record *itr, - struct evsel *evsel) -{ - struct arm_spe_recording *ptr; - struct perf_pmu *arm_spe_pmu; - struct evsel_config_term *term = evsel__get_config_term(evsel, CFG_CHG); - u64 user_bits = 0, bit; - - ptr = container_of(itr, struct arm_spe_recording, itr); - arm_spe_pmu = ptr->arm_spe_pmu; - - if (term) - user_bits = term->val.cfg_chg; - - bit = perf_pmu__format_bits(&arm_spe_pmu->format, "ts_enable"); - - /* Skip if user has set it */ - if (bit & user_bits) - return; - - evsel->core.attr.config |= bit; -} - static size_t arm_spe_info_priv_size(struct auxtrace_record *itr __maybe_unused, struct evlist *evlist __maybe_unused) @@ -238,7 +215,8 @@ static int arm_spe_recording_options(struct auxtrace_record *itr, */ if (!perf_cpu_map__empty(cpus)) { evsel__set_sample_bit(arm_spe_evsel, CPU); - arm_spe_set_timestamp(itr, arm_spe_evsel); + evsel__set_config_if_unset(arm_spe_pmu, arm_spe_evsel, + "ts_enable", 1); } /* diff --git a/tools/perf/arch/x86/util/intel-pt.c b/tools/perf/arch/x86/util/intel-pt.c index 2cff11de9d8a..17336da08b58 100644 --- a/tools/perf/arch/x86/util/intel-pt.c +++ b/tools/perf/arch/x86/util/intel-pt.c @@ -576,25 +576,6 @@ static int intel_pt_validate_config(struct perf_pmu *intel_pt_pmu, return err; } -static void intel_pt_config_sample_mode(struct perf_pmu *intel_pt_pmu, - struct evsel *evsel) -{ - u64 user_bits = 0, bits; - struct evsel_config_term *term = evsel__get_config_term(evsel, CFG_CHG); - - if (term) - user_bits = term->val.cfg_chg; - - bits = perf_pmu__format_bits(&intel_pt_pmu->format, "psb_period"); - - /* Did user change psb_period */ - if (bits & user_bits) - return; - - /* Set psb_period to 0 */ - evsel->core.attr.config &= ~bits; -} - static void intel_pt_min_max_sample_sz(struct evlist *evlist, size_t *min_sz, size_t *max_sz) { @@ -686,7 +667,8 @@ static int intel_pt_recording_options(struct auxtrace_record *itr, return 0; if (opts->auxtrace_sample_mode) - intel_pt_config_sample_mode(intel_pt_pmu, intel_pt_evsel); + evsel__set_config_if_unset(intel_pt_pmu, intel_pt_evsel, + "psb_period", 0); err = intel_pt_validate_config(intel_pt_pmu, intel_pt_evsel); if (err) diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c index a85a987128aa..cdf1445136ad 100644 --- a/tools/perf/util/evsel.c +++ b/tools/perf/util/evsel.c @@ -3216,3 +3216,32 @@ void evsel__remove_from_group(struct evsel *evsel, struct evsel *leader) leader->core.nr_members--; } } + +/* + * Set @config_name to @val as long as the user hasn't already set or cleared it + * by passing a config term on the command line. + * + * @val is the value to put into the bits specified by @config_name rather than + * the bit pattern. It is shifted into position by this function, so to set + * something to true, pass 1 for val rather than a pre shifted value. + */ +#define field_prep(_mask, _val) (((_val) << (ffsll(_mask) - 1)) & (_mask)) +void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel, + const char *config_name, u64 val) +{ + u64 user_bits = 0, bits; + struct evsel_config_term *term = evsel__get_config_term(evsel, CFG_CHG); + + if (term) + user_bits = term->val.cfg_chg; + + bits = perf_pmu__format_bits(&pmu->format, config_name); + + /* Do nothing if the user changed the value */ + if (bits & user_bits) + return; + + /* Otherwise replace it */ + evsel->core.attr.config &= ~bits; + evsel->core.attr.config |= field_prep(bits, val); +} diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h index 68072ec655ce..4120f5ff673d 100644 --- a/tools/perf/util/evsel.h +++ b/tools/perf/util/evsel.h @@ -529,4 +529,7 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel); ((((src) >> (pos)) & ((1ull << (size)) - 1)) << (63 - ((pos) + (size) - 1))) u64 evsel__bitfield_swap_branch_flags(u64 value); +void evsel__set_config_if_unset(struct perf_pmu *pmu, struct evsel *evsel, + const char *config_name, u64 val); + #endif /* __PERF_EVSEL_H */
There is some duplicated code to only override config values if they haven't already been set by the user so make a util function for this. Signed-off-by: James Clark <james.clark@arm.com> --- tools/perf/arch/arm64/util/arm-spe.c | 26 ++----------------------- tools/perf/arch/x86/util/intel-pt.c | 22 ++------------------- tools/perf/util/evsel.c | 29 ++++++++++++++++++++++++++++ tools/perf/util/evsel.h | 3 +++ 4 files changed, 36 insertions(+), 44 deletions(-)