[PATCH] powerpc/perf: Record counter overflow always if SAMPLE_IP is unset

[Athira Rajeev]

> On 04-Feb-2021, at 8:25 AM, Michael Ellerman <mpe at ellerman.id.au> wrote:
> 
> Athira Rajeev <atrajeev at 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 <atrajeev at 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