Hi Leo,
Would you be ok with the current patch the way it is? In case it's of any help, I'm sharing the testing steps that James and I went through when testing this internally, if you want to add to it
- Test that only a portion of the buffer is saved until there is a wraparound
$ ./perf record -vvv -e arm_spe/period=148576/u -S -- taskset --cpu-list 0 stress --cpu 1 & while true; do sleep 0.2; killall -s USR2 perf; done
- Test snapshot mode in CPU mode
$ sudo ./perf record -vvv -C 0 -e arm_spe/period=148576/u -S -- taskset --cpu-list 0 stress --cpu 1 &
- Test that auxtrace buffers correspond to an aux record - Test snapshot default sizes in sudo and user modes - Test small snapshot size
$ ./perf record -vvv -e arm_spe/period=148576/u -S1000 -m16,16 -- taskset --cpu-list 0 stress --cpu 1 &
If there are any concerns with the patches, please let me know and I will try to address them.
Thanks, German
On 13/10/2021 08:51, Will Deacon wrote:
On Wed, Oct 13, 2021 at 08:39:16AM +0800, Leo Yan wrote:
On Mon, Oct 11, 2021 at 04:55:37PM +0100, German Gomez wrote:
On 06/10/2021 10:51, Leo Yan wrote:
On Wed, Oct 06, 2021 at 10:35:20AM +0100, German Gomez wrote:
[...]
So simply say, I think the head pointer monotonically increasing is the right thing to do in Arm SPE driver.
I will talk to James about how we can proceed on this.
Thanks!
I took this offline with James and, though it looks possible to patch the SPE driver to have a monotonically increasing head pointer in order to simplify the handling in the perf tool, it could be a breaking change for users of the perf_event_open syscall that currently rely on the way it works now.
Here I cannot create the connection between AUX head pointer and the breakage of calling perf_event_open().
Could you elaborate what's the reason the monotonical increasing head pointer will lead to the breakage for perf_event_open()?
It's a user-visible change in behaviour, isn't it? Therefore we risk breaking applications that rely on the current behaviour if we change it unconditionally.
Given that the driver has always worked like this and it doesn't sound like it's the end of the world to deal with it in userspace (after all, it's aligned with intel-pt), then I don't think we should change it.
Will
Hi German,
On Fri, Oct 15, 2021 at 01:33:39PM +0100, German Gomez wrote:
Hi Leo,
Would you be ok with the current patch the way it is?
Sorry for my failure to catch up the discussion.
As you and Will have mentioned in other emails that it will lead to breakage if we change to monotonical increasing head, I read the code and realized the difficulty to use monotonical increasing head in Arm SPE driver. So let's use the way as this patch set is.
In case it's of any help, I'm sharing the testing steps that James and I went through when testing this internally, if you want to add to it
- Test that only a portion of the buffer is saved until there is a wraparound
$ ./perf record -vvv -e arm_spe/period=148576/u -S -- taskset --cpu-list 0 stress --cpu 1 & while true; do sleep 0.2; killall -s USR2 perf; done
- Test snapshot mode in CPU mode
$ sudo ./perf record -vvv -C 0 -e arm_spe/period=148576/u -S -- taskset --cpu-list 0 stress --cpu 1 &
- Test that auxtrace buffers correspond to an aux record
- Test snapshot default sizes in sudo and user modes
- Test small snapshot size
$ ./perf record -vvv -e arm_spe/period=148576/u -S1000 -m16,16 -- taskset --cpu-list 0 stress --cpu 1 &
If there are any concerns with the patches, please let me know and I will try to address them.
Thanks for sharing the testing cases. Could give me a bit more time for the test at my side? And please expect I might give some comments if I think it's necessary.
Thanks, Leo
On 15/10/2021 15:16, Leo Yan wrote:
Hi German,
On Fri, Oct 15, 2021 at 01:33:39PM +0100, German Gomez wrote:
[...]
Thanks for sharing the testing cases. Could give me a bit more time for the test at my side? And please expect I might give some comments if I think it's necessary.
Thanks, Leo
Absolutely. Please take the time you need.
Many thanks, German
Hi German, Will,
On Fri, Oct 15, 2021 at 01:33:39PM +0100, German Gomez wrote:
[...]
$ ./perf record -vvv -e arm_spe/period=148576/u -S1000 -m16,16 -- taskset --cpu-list 0 stress --cpu 1 &
When testing Arm SPE snapshot mode with the command (it's quite similiar with up command but not exactly same):
# ./perf --debug verbose=3 record -e arm_spe/period=148576/u -C 0 -S1000 -m16,16 \ -- taskset --cpu-list 0 stress --cpu 1 & # kill -USR2 [pid_num]
... then I wait for long time and didn't stop the perf program, then I observed the output file contains many redundant events PERF_RECORD_AUX. E.g. in the shared perf data file [1], you could use below commands to see tons of the events PERF_RECORD_AUX which I only send only one USR2 signal for taking snapshot:
# perf report -D -i perf.data --stdio | grep -E 'RECORD_AUX' | wc -l 2245787
# perf report -D -i perf.data --stdio | grep -E 'SPE' . ... ARM SPE data: size 0x3e8 bytes Binary file (standard input) matches
I looked into the Arm SPE driver and found it doesn't really support free run mode for AUX ring buffer when the driver runs in snapshot mode, the pair functions perf_aux_output_end() and perf_aux_output_begin() are invoked when every time handle the interrupt. The detailed flow is:
arm_spe_pmu_irq_handler() `> arm_spe_pmu_buf_get_fault_act() `> arm_spe_perf_aux_output_end() `> set SPE registers `> perf_aux_output_end() `> arm_spe_perf_aux_output_begin() `> perf_aux_output_begin() `> set SPE registers
Seems to me, a possible solution is to add an extra parameter 'int in_interrupt' for functions arm_spe_perf_aux_output_end() and arm_spe_perf_aux_output_begin(), if this parameter is passed as 1 in the interrupt handling, these two functions should skip invoking perf_aux_output_end() and perf_aux_output_begin() so can avoid the redundant perf event PERF_RECORD_AUX.
arm_spe_pmu_irq_handler() `> arm_spe_pmu_buf_get_fault_act() `> arm_spe_perf_aux_output_end(..., in_interrupt=1) `> set SPE registers `> arm_spe_perf_aux_output_begin(..., in_interrupt=1) `> set SPE registers
P.s. I think Intel-PT has supported free run mode for snapshot mode, so it should not generate interrupt in this mode. Thus Intel-PT can avoid this issue, please see the code [2].
Thanks, Leo
[1] https://people.linaro.org/~leo.yan/spe/snapshot_test/perf.data [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch...
Hi Leo,
Yeah I agree the redundant AUX events are adding unnecessary bloat to the perf.data file... We actually cam across this when doing one of the test cases. Sorry for not reporting it!
Could we patch the driver in a separate patch set? Or do you think this is critical for the purposes of this one?
Thanks, German
On 17/10/2021 07:13, Leo Yan wrote:
Hi German, Will,
On Fri, Oct 15, 2021 at 01:33:39PM +0100, German Gomez wrote:
[...]
$ ./perf record -vvv -e arm_spe/period=148576/u -S1000 -m16,16 -- taskset --cpu-list 0 stress --cpu 1 &
When testing Arm SPE snapshot mode with the command (it's quite similiar with up command but not exactly same):
# ./perf --debug verbose=3 record -e arm_spe/period=148576/u -C 0 -S1000 -m16,16 \ -- taskset --cpu-list 0 stress --cpu 1 & # kill -USR2 [pid_num]
... then I wait for long time and didn't stop the perf program, then I observed the output file contains many redundant events PERF_RECORD_AUX. E.g. in the shared perf data file [1], you could use below commands to see tons of the events PERF_RECORD_AUX which I only send only one USR2 signal for taking snapshot:
# perf report -D -i perf.data --stdio | grep -E 'RECORD_AUX' | wc -l 2245787
# perf report -D -i perf.data --stdio | grep -E 'SPE' . ... ARM SPE data: size 0x3e8 bytes Binary file (standard input) matches
I looked into the Arm SPE driver and found it doesn't really support free run mode for AUX ring buffer when the driver runs in snapshot mode, the pair functions perf_aux_output_end() and perf_aux_output_begin() are invoked when every time handle the interrupt. The detailed flow is:
arm_spe_pmu_irq_handler() `> arm_spe_pmu_buf_get_fault_act() `> arm_spe_perf_aux_output_end() `> set SPE registers `> perf_aux_output_end() `> arm_spe_perf_aux_output_begin() `> perf_aux_output_begin() `> set SPE registers
Seems to me, a possible solution is to add an extra parameter 'int in_interrupt' for functions arm_spe_perf_aux_output_end() and arm_spe_perf_aux_output_begin(), if this parameter is passed as 1 in the interrupt handling, these two functions should skip invoking perf_aux_output_end() and perf_aux_output_begin() so can avoid the redundant perf event PERF_RECORD_AUX.
arm_spe_pmu_irq_handler() `> arm_spe_pmu_buf_get_fault_act() `> arm_spe_perf_aux_output_end(..., in_interrupt=1) `> set SPE registers `> arm_spe_perf_aux_output_begin(..., in_interrupt=1) `> set SPE registers
P.s. I think Intel-PT has supported free run mode for snapshot mode, so it should not generate interrupt in this mode. Thus Intel-PT can avoid this issue, please see the code [2].
Thanks, Leo
[1] https://people.linaro.org/~leo.yan/spe/snapshot_test/perf.data [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch...
Hi German,
On Tue, Oct 19, 2021 at 10:23:01AM +0100, German Gomez wrote:
Hi Leo,
Yeah I agree the redundant AUX events are adding unnecessary bloat to the perf.data file... We actually cam across this when doing one of the test cases. Sorry for not reporting it!
No worries.
Could we patch the driver in a separate patch set? Or do you think this is critical for the purposes of this one?
Yeah, we can take low priority for the redundant AUX events issue.
Please take a look for the issue mentioned in another email for recording trace data with wrong size. I think the issue for wrong snapshot trace size should have a fixing in Arm SPE driver, and the fixing need to be verified with the perf patches. After that I am fine for merging the perf patches (and you could upstream kernel driver patches separately). How about you think?
Thanks, Leo
Hi Leo,
On 17/10/2021 07:13, Leo Yan wrote:
[...]
I looked into the Arm SPE driver and found it doesn't really support free run mode for AUX ring buffer when the driver runs in snapshot mode, the pair functions perf_aux_output_end() and perf_aux_output_begin() are invoked when every time handle the interrupt. The detailed flow is:
arm_spe_pmu_irq_handler() `> arm_spe_pmu_buf_get_fault_act() `> arm_spe_perf_aux_output_end() `> set SPE registers `> perf_aux_output_end() `> arm_spe_perf_aux_output_begin() `> perf_aux_output_begin() `> set SPE registers
Seems to me, a possible solution is to add an extra parameter 'int in_interrupt' for functions arm_spe_perf_aux_output_end() and arm_spe_perf_aux_output_begin(), if this parameter is passed as 1 in the interrupt handling, these two functions should skip invoking perf_aux_output_end() and perf_aux_output_begin() so can avoid the redundant perf event PERF_RECORD_AUX.
arm_spe_pmu_irq_handler() `> arm_spe_pmu_buf_get_fault_act() `> arm_spe_perf_aux_output_end(..., in_interrupt=1) `> set SPE registers `> arm_spe_perf_aux_output_begin(..., in_interrupt=1) `> set SPE registers
I brought the issue of the redundant AUX events to the team, and we know of at least one tool in Arm relying on these events in snapshot mode. So we think that changing this behavior of the driver might not be easy to do right now.
P.s. I think Intel-PT has supported free run mode for snapshot mode, so it should not generate interrupt in this mode. Thus Intel-PT can avoid this issue, please see the code [2].
Thanks, Leo
[1] https://people.linaro.org/~leo.yan/spe/snapshot_test/perf.data [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch...
Thanks, German
-
German Gomez -
Leo Yan