possible regression in perf

possible regression in perf

[trinity pointard]

Hi,

I think I found a regression in perf related to the control fd.
At its introduction (bee328cb71eb0b38ab128d7c475209d973a13f92), it
looks like enabling or disabling counters would print the current
counters. However when `enable/disable <counter>` was introduced
(991ae4eb36911fe9a99bef1c22b9578ceec3896a), it changed what
`evlist__ctlfd_process()` would return on a successful enable/disable,
which made the call to `process_interval()` in `process_evlist()`
essentially dead code.

I was in the process of patching perf so that `snapshot` would print
current counters (in which case I would send enable, sleep a bit, and
send disable+snapshot, to do my measurements). But given what I saw, I
think just sending enable and disable was enough at one point.

Is this a known issue? Would a patch which makes
`evlist__ctlfd_enable()` return 1 on success be an acceptable fix?
It's somewhat more verbose than what I wanted initially (printing both
on enable and disable, printing on enable isn't useful to me), but
that's reasonably easy to post-process (or enable could be made to not
print current counters).

Regards,
trinity-1686a/trinity Pointard

P.S.: this is my first time posting to LKML, please tell me if I did
anything wrong :-)

Re: possible regression in perf

[Jiri Olsa]

On Mon, Dec 11, 2023 at 07:00:51PM +0100, trinity pointard wrote:
> Hi,
> 
> I think I found a regression in perf related to the control fd.
> At its introduction (bee328cb71eb0b38ab128d7c475209d973a13f92), it
> looks like enabling or disabling counters would print the current
> counters. However when `enable/disable <counter>` was introduced
> (991ae4eb36911fe9a99bef1c22b9578ceec3896a), it changed what
> `evlist__ctlfd_process()` would return on a successful enable/disable,
> which made the call to `process_interval()` in `process_evlist()`
> essentially dead code.
> 
> I was in the process of patching perf so that `snapshot` would print
> current counters (in which case I would send enable, sleep a bit, and
> send disable+snapshot, to do my measurements). But given what I saw, I
> think just sending enable and disable was enough at one point.
> 
> Is this a known issue? Would a patch which makes
> `evlist__ctlfd_enable()` return 1 on success be an acceptable fix?
> It's somewhat more verbose than what I wanted initially (printing both
> on enable and disable, printing on enable isn't useful to me), but
> that's reasonably easy to post-process (or enable could be made to not
> print current counters).
> 
> Regards,
> trinity-1686a/trinity Pointard
> 
> P.S.: this is my first time posting to LKML, please tell me if I did
> anything wrong :-)

hi,
thanks for the report, could you please send proposed change as patch
and steps to reproduce the issue?

jirka

[PATCH] fix perf-stat regression printing stat on enable/disable

[trinity Pointard]

It's possible to reproduce the issue with this script:
```
ctl_fifo=perf_ctl.fifo
mkfifo ${ctl_fifo}
exec {ctl_fd}<>${ctl_fifo}
perf stat -I 10000 --control fd:${ctl_fd} sleep 3 &
sleep 1 && echo 'disable' >&${ctl_fd}
sleep 1 && echo 'enable' >&${ctl_fd}
exec {ctl_fd}>&-
unlink ${ctl_fifo}
wait
```

Testing with 27e9769aad3c435993a2e0cd91f5d868294145d0 (perf stat:
Introduce --control fd:ctl-fd[,ack-fd] options), it prints stats 3
times. On disable, on enable (stats are mostly zeroes though), and on
end of execution.
Testing with a39b6ac3781d46ba18193c9dbb2110f31e9bffe9 (v6.7-rc5), it
prints only one time, at the end of execution.

My idea of a fix is to modify evlist__ctlfd_enable such that it and
evlist__ctlfd_process return a positive value on success, restoring
its previous behaviour.

Signed-off-by: trinity Pointard <trinity@deuxfleurs.fr>
---
 tools/perf/util/evlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index e36da58522ef..ed7e2dec365d 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -2111,7 +2111,7 @@ static int evlist__ctlfd_enable(struct evlist *evlist, char *cmd_data, bool enab
 		}
 	}
 
-	return 0;
+	return 1;
 }
 
 static int evlist__ctlfd_list(struct evlist *evlist, char *cmd_data)
-- 
2.43.0