[PATCH v2 2/2] powerpc/perf: Return regs->nip as instruction pointer value when SIAR is 0

[Christophe Leroy]

Le 16/08/2021 à 08:44, kajoljain a écrit :
> 
> 
> On 8/14/21 6:14 PM, Michael Ellerman wrote:
>> Christophe Leroy <christophe.leroy at csgroup.eu> writes:
>>> Le 13/08/2021 à 10:24, Kajol Jain a écrit :
>>>> Incase of random sampling, there can be scenarios where SIAR is not
>>>> latching sample address and results in 0 value. Since current code
>>>> directly returning the siar value, we could see multiple instruction
>>>> pointer values as 0 in perf report.
>>
>> Can you please give more detail on that? What scenarios? On what CPUs?
>>
> 
> Hi Michael,
>      Sure I will update these details in my next patch-set.
> 
>>>> Patch resolves this issue by adding a ternary condition to return
>>>> regs->nip incase SIAR is 0.
>>>
>>> Your description seems rather similar to
>>> https://github.com/linuxppc/linux/commit/2ca13a4cc56c920a6c9fc8ee45d02bccacd7f46c
>>>
>>> Does it mean that the problem occurs on more than the power10 DD1 ?
>>>
>>> In that case, can the solution be common instead of doing something for power10 DD1 and something
>>> for others ?
>>
>> Agreed.
>>
>> This change would seem to make that P10 DD1 logic superfluous.
>>
>> Also we already have a fallback to regs->nip in the else case of the if,
>> so we should just use that rather than adding a ternary condition.
>>
>> eg.
>>
>> 	if (use_siar && siar_valid(regs) && siar)
>> 		return siar + perf_ip_adjust(regs);
>> 	else if (use_siar)
>> 		return 0;		// no valid instruction pointer
>> 	else
>> 		return regs->nip;
>>
>>
>> I'm also not sure why we have that return 0 case, I can't think of why
>> we'd ever want to do that rather than using nip. So maybe we should do
>> another patch to drop that case.
> 
> Yeah make sense. I will remove return 0 case in my next version.
> 

This was added by commit 
https://github.com/linuxppc/linux/commit/e6878835ac4794f25385522d29c634b7bbb7cca9

Are we sure it was an error to add it and it can be removed ?

Christophe