> On 04-Feb-2021, at 8:25 AM, Michael Ellerman <m...@ellerman.id.au> wrote:
>
> Athira Rajeev <atraj...@linux.vnet.ibm.com> writes:
>> While sampling for marked events, currently we record the sample only
>> if the SIAR valid bit of Sampled Instruction Event Register (SIER) is
>> set. SIAR_VALID bit is used for fetching the instruction address from
>> Sampled Instruction Address Register(SIAR). But there are some usecases,
>> where the user is interested only in the PMU stats at each counter
>> overflow and the exact IP of the overflow event is not required.
>> Dropping SIAR invalid samples will fail to record some of the counter
>> overflows in such cases.
>>
>> Example of such usecase is dumping the PMU stats (event counts)
>> after some regular amount of instructions/events from the userspace
>> (ex: via ptrace). Here counter overflow is indicated to userspace via
>> signal handler, and captured by monitoring and enabling I/O
>> signaling on the event file descriptor. In these cases, we expect to
>> get sample/overflow indication after each specified sample_period.
>>
>> Perf event attribute will not have PERF_SAMPLE_IP set in the
>> sample_type if exact IP of the overflow event is not requested. So
>> while profiling if SAMPLE_IP is not set, just record the counter overflow
>> irrespective of SIAR_VALID check.
>>
>> Signed-off-by: Athira Rajeev <atraj...@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/perf/core-book3s.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/perf/core-book3s.c
>> b/arch/powerpc/perf/core-book3s.c
>> index 28206b1fe172..bb4828a05e4d 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -2166,10 +2166,16 @@ static void record_and_restart(struct perf_event
>> *event, unsigned long val,
>> * address even when freeze on supervisor state (kernel) is set in
>> * MMCR2. Check attr.exclude_kernel and address to drop the sample in
>> * these cases.
>> + *
>> + * If address is not requested in the sample
>> + * via PERF_SAMPLE_IP, just record that sample
>> + * irrespective of SIAR valid check.
>> */
>> - if (event->attr.exclude_kernel && record)
>> - if (is_kernel_addr(mfspr(SPRN_SIAR)))
>> + if (event->attr.exclude_kernel && record) {
>> + if (is_kernel_addr(mfspr(SPRN_SIAR)) &&
>> (event->attr.sample_type & PERF_SAMPLE_IP))
>> record = 0;
>> + } else if (!record && !(event->attr.sample_type & PERF_SAMPLE_IP))
>> + record = 1;
>
> This seems wrong, you're assuming that record was set previously to
> = siar_valid(), but it may be that record is still 0 from the
> initialisation and we weren't going to record.
>
> Don't we need something more like this?
Hi Michael,
Thanks for checking the patch and sharing the suggestion.
Yes, the below change looks good and tested with my scenario.
I will send a V2 with new change.
Thanks
Athira
>
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 9fd06010e8b6..e4e8a017d339 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -2136,7 +2136,12 @@ static void record_and_restart(struct perf_event
> *event, unsigned long val,
> left += period;
> if (left <= 0)
> left = period;
> - record = siar_valid(regs);
> +
> + if (event->attr.sample_type & PERF_SAMPLE_IP)
> + record = siar_valid(regs);
> + else
> + record = 1;
> +
> event->hw.last_period = event->hw.sample_period;
> }
> if (left < 0x80000000LL)
> @@ -2154,9 +2159,10 @@ static void record_and_restart(struct perf_event
> *event, unsigned long val,
> * MMCR2. Check attr.exclude_kernel and address to drop the sample in
> * these cases.
> */
> - if (event->attr.exclude_kernel && record)
> - if (is_kernel_addr(mfspr(SPRN_SIAR)))
> - record = 0;
> + if (event->attr.exclude_kernel &&
> + (event->attr.sample_type & PERF_SAMPLE_IP) &&
> + is_kernel_addr(mfspr(SPRN_SIAR)))
> + record = 0;
>
> /*
> * Finally record data if requested.
>
>
>
> cheers
