[2/7] perf tools: Add util function for overriding user set config values

  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(-)
  

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
>