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

[Madhavan Srinivasan <maddy@linux.ibm.com>]

On 8/16/21 12:26 PM, Christophe Leroy wrote:
>
>
> Le 16/08/2021 à 08:44, kajoljain a écrit :
>>
>>
>> On 8/14/21 6:14 PM, Michael Ellerman wrote:
>>> Christophe Leroy <christophe.leroy@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 ?

pc having 0 is wrong (kernel does not execute at 0x0 or userspace).
yeah we should drop it.

Maddy
>
> Christophe