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

[kajoljain <kjain@linux.ibm.com>]

On 8/17/21 11:07 AM, Madhavan Srinivasan wrote:
> 
> 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.

Hi Madhavan,
    Sure I will add another patch to drop return 0 condition.

Thanks,
Kajol jain
> 
> Maddy
>>
>> Christophe