Messages in this thread | | | Subject | Re: [PATCH 4/5] perf arm-spe: Implement find_snapshot callback | From | German Gomez <> | Date | Fri, 15 Oct 2021 13:33:39 +0100 |
| |
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
| |