Re: perf tools: software regression in d0a0a511493d269514fcbd852481cdca32c95350

["Adrián Herrera Arcila" <adrian.herrera@arm.com>]

Thank you Leo.

I think the source code fragment in perf-record is more readable 
(logical) than in perf-stat, so replicating it would be better, also for 
consistency. I have prepared a patch for that which I will send for review.

perf-stat differs from perf-record in the presence of BPF counters; 
these are enabled inside enable_counters immediately, independent of the 
delay; I respected that behaviour, but if it is not a problem to delay 
their enablement as well, the code would be slightly simplified. I have 
added Song Liu (author of BPF counters), in case of any comments on this.

Note Thomas issue arised when using "-a"; without it, events are enabled 
on exec of the workload/command, so the instruction he mentioned would 
be captured. The code structure in perf-record, and in the patch I 
prepared, relies implicitly on enable_on_exec; I tried to clarify it in 
the comments.

Kind regards,
Adrián.

On 14/07/2022 13:46, Leo Yan wrote:
> Hi Adri�n,
>
> On Wed, Jul 13, 2022 at 08:30:37AM +0100, Adri�n Herrera Arcila wrote:
>
> [...]
>
>> I think there are two behaviours that are valuable to maintain to enable two
>> uses: (1) if no delay is specified, the program should start after counters
>> are enabled, to prevent the unexpected behaviour that Thomas fixed with this
>> change; (2) if delay is specified, the program should start before the delay
>> is engaged.
>>
>> Two possible solutions are: (1) though conditionals; if delay > 0, start the
>> workload, then call enable_counters, which will usleep for the delay; if no
>> delay, then behaviour as it is in v5.18; (2) replace blocking usleep with
>> asynchronous callback, if that exists in Linux, that enables counters after
>> the delay.
> If we look at other perf sub tools (like 'perf record'), they uses
> your mentioned solution (1) to handle the delay and enabling counters,
> based on the condition checking for 'stat_config.initial_delay', 'perf
> record' enables counters at different time point.
>
> Alternatively, we can start workload based on the condition checking
> for 'stat_config.initial_delay', so we can get a below patch, this patch
> should can fix the issue, but the change is a bit urgly for me, so
> welcome a better fixing; if no, I will send a patch in my tomorrow
> morning.
>
> Thanks,
> Leo
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index d2ecd4d29624..046686a7c8d6 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -970,10 +970,15 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>   	 * Enable counters and exec the command:
>   	 */
>   	if (forks) {
> +		if (stat_config.initial_delay > 0)
> +			evlist__start_workload(evsel_list);
> +
>   		err = enable_counters();
>   		if (err)
>   			return -1;
> -		evlist__start_workload(evsel_list);
> +
> +		if (stat_config.initial_delay <= 0)
> +			evlist__start_workload(evsel_list);
>   
>   		t0 = rdclock();
>   		clock_gettime(CLOCK_MONOTONIC, &ref_time);