[PATCH v2 1/2] powerpc/perf: Use stack siar instead of mfspr

[PATCH v2 1/2] powerpc/perf: Use stack siar instead of mfspr

[Kajol Jain]

Minor optimization in the 'perf_instruction_pointer' function code by
making use of stack siar instead of mfspr.

Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
into perf_read_regs")
Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 arch/powerpc/perf/core-book3s.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index bb0ee716de91..1b464aad29c4 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -2260,7 +2260,7 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
 		else
 			return regs->nip;
 	} else if (use_siar && siar_valid(regs))
-		return mfspr(SPRN_SIAR) + perf_ip_adjust(regs);
+		return siar + perf_ip_adjust(regs);
 	else if (use_siar)
 		return 0;		// no valid instruction pointer
 	else
-- 
2.26.2

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

[Kajol Jain]

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.
Patch resolves this issue by adding a ternary condition to return
regs->nip incase SIAR is 0.

Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
into perf_read_regs")
Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
---
 arch/powerpc/perf/core-book3s.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 1b464aad29c4..aeecaaf6810f 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -2260,7 +2260,7 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
 		else
 			return regs->nip;
 	} else if (use_siar && siar_valid(regs))
-		return siar + perf_ip_adjust(regs);
+		return siar ? siar + perf_ip_adjust(regs) : regs->nip;
 	else if (use_siar)
 		return 0;		// no valid instruction pointer
 	else
-- 
2.26.2

Re: [PATCH v2 1/2] powerpc/perf: Use stack siar instead of mfspr

[kajoljain]

On 8/13/21 1:54 PM, Kajol Jain wrote:
> Minor optimization in the 'perf_instruction_pointer' function code by
> making use of stack siar instead of mfspr.
> 
> Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
> into perf_read_regs")
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>

Please ignore this patch-set as I mentioned wrong version number. I will resend
this patch-set again with correct version. Sorry for the confusion.

Thanks,
Kajol Jain
> ---
>  arch/powerpc/perf/core-book3s.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index bb0ee716de91..1b464aad29c4 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -2260,7 +2260,7 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
>  		else
>  			return regs->nip;
>  	} else if (use_siar && siar_valid(regs))
> -		return mfspr(SPRN_SIAR) + perf_ip_adjust(regs);
> +		return siar + perf_ip_adjust(regs);
>  	else if (use_siar)
>  		return 0;		// no valid instruction pointer
>  	else
> 

Re: [PATCH v2 1/2] powerpc/perf: Use stack siar instead of mfspr

[Christophe Leroy]

Le 13/08/2021 à 10:29, kajoljain a écrit :
> 
> 
> On 8/13/21 1:54 PM, Kajol Jain wrote:
>> Minor optimization in the 'perf_instruction_pointer' function code by
>> making use of stack siar instead of mfspr.
>>
>> Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
>> into perf_read_regs")
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> 
> Please ignore this patch-set as I mentioned wrong version number. I will resend
> this patch-set again with correct version. Sorry for the confusion.

I fear you are creating even more confusion by sending a v1 after sending a v2 ...

Christophe

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

[Christophe Leroy]

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.
> Patch resolves this issue by adding a ternary condition to return
> regs->nip incase SIAR is 0.
> 
> Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
> into perf_read_regs")
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
>   arch/powerpc/perf/core-book3s.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 1b464aad29c4..aeecaaf6810f 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -2260,7 +2260,7 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
>   		else
>   			return regs->nip;
>   	} else if (use_siar && siar_valid(regs))
> -		return siar + perf_ip_adjust(regs);
> +		return siar ? siar + perf_ip_adjust(regs) : regs->nip;

Why bother about returning SIAR at all if regs->nip is ok ? Why not just return regs->nip all the time ?

>   	else if (use_siar)
>   		return 0;		// no valid instruction pointer
>   	else
> 

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

[Christophe Leroy]

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.
> 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 ?

> 
> Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
> into perf_read_regs")
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> ---
>   arch/powerpc/perf/core-book3s.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 1b464aad29c4..aeecaaf6810f 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -2260,7 +2260,7 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
>   		else
>   			return regs->nip;
>   	} else if (use_siar && siar_valid(regs))
> -		return siar + perf_ip_adjust(regs);
> +		return siar ? siar + perf_ip_adjust(regs) : regs->nip;
>   	else if (use_siar)
>   		return 0;		// no valid instruction pointer
>   	else
> 

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

[Michael Ellerman]

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.
>> Patch resolves this issue by adding a ternary condition to return
>> regs->nip incase SIAR is 0.
>> 
>> Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
>> into perf_read_regs")
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>>   arch/powerpc/perf/core-book3s.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index 1b464aad29c4..aeecaaf6810f 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -2260,7 +2260,7 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
>>   		else
>>   			return regs->nip;
>>   	} else if (use_siar && siar_valid(regs))
>> -		return siar + perf_ip_adjust(regs);
>> +		return siar ? siar + perf_ip_adjust(regs) : regs->nip;
>
> Why bother about returning SIAR at all if regs->nip is ok ? Why not just return regs->nip all the time ?

Same answer as last time :)

https://lore.kernel.org/linuxppc-dev/87r1prxd9e.fsf@mpe.ellerman.id.au/

ie. SIAR can point into interrupts-off code, whereas regs->nip will
point to where we re-enabled interrupts.

cheers

Re: [PATCH v2 1/2] powerpc/perf: Use stack siar instead of mfspr

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 13/08/2021 à 10:29, kajoljain a écrit :
>> 
>> On 8/13/21 1:54 PM, Kajol Jain wrote:
>>> Minor optimization in the 'perf_instruction_pointer' function code by
>>> making use of stack siar instead of mfspr.
>>>
>>> Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
>>> into perf_read_regs")
>>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> 
>> Please ignore this patch-set as I mentioned wrong version number. I will resend
>> this patch-set again with correct version. Sorry for the confusion.
>
> I fear you are creating even more confusion by sending a v1 after sending a v2 ...

Yeah in future just reply to the v2 saying "oops I sent v2 instead of
v1" and leave it at that.

cheers

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

[Michael Ellerman]

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?

>> 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.

cheers

Re: [PATCH v2 1/2] powerpc/perf: Use stack siar instead of mfspr

[kajoljain]

On 8/14/21 6:00 PM, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 13/08/2021 à 10:29, kajoljain a écrit :
>>>
>>> On 8/13/21 1:54 PM, Kajol Jain wrote:
>>>> Minor optimization in the 'perf_instruction_pointer' function code by
>>>> making use of stack siar instead of mfspr.
>>>>
>>>> Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
>>>> into perf_read_regs")
>>>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>>>
>>> Please ignore this patch-set as I mentioned wrong version number. I will resend
>>> this patch-set again with correct version. Sorry for the confusion.
>>
>> I fear you are creating even more confusion by sending a v1 after sending a v2 ...
> 
> Yeah in future just reply to the v2 saying "oops I sent v2 instead of
> v1" and leave it at that.

Hi Christophe/Michael,
     Sure I will take care next time.

Thanks,
Kajol Jain

> 
> cheers
> 

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

[kajoljain]

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.

Thanks,
Kajol Jain
> 
> cheers
> 

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

[kajoljain]

On 8/13/21 3:04 PM, Christophe Leroy wrote:
> 
> 
> 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.
>> 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 ?

Hi Christophe,
    Yes its better to have common check. I will make these updates.

Thanks,
Kajol Jain

> 
>>
>> Fixes: 75382aa72f06 ("powerpc/perf: Move code to select SIAR or pt_regs
>> into perf_read_regs")
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>>   arch/powerpc/perf/core-book3s.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index 1b464aad29c4..aeecaaf6810f 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -2260,7 +2260,7 @@ unsigned long perf_instruction_pointer(struct pt_regs *regs)
>>           else
>>               return regs->nip;
>>       } else if (use_siar && siar_valid(regs))
>> -        return siar + perf_ip_adjust(regs);
>> +        return siar ? siar + perf_ip_adjust(regs) : regs->nip;
>>       else if (use_siar)
>>           return 0;        // no valid instruction pointer
>>       else
>>

Re: [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@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

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

[Madhavan Srinivasan]

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

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

[kajoljain]

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

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

[Michael Ellerman]

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

I think so.

That commit added siar_valid(), and updated record_and_restart() to only
record if siar_valid() returned true.

  -                        record = 1;
  +                        record = siar_valid(regs);

It then also changed perf_instruction_pointer():

  -        if (use_siar)
  +        if (use_siar && siar_valid(regs))
                   return mfspr(SPRN_SIAR) + perf_ip_adjust(regs);
  +        else if (use_siar)
  +                return 0;                // no valid instruction pointer
           else
                   return regs->nip;


The first change means we would never even call
perf_instruction_pointer() if siar_valid() is false, so we could never
hit the use_siar && !siar_valid() case.

But even so it's always preferable to use regs->nip than 0, even if nip
is somewhat skewed due to interrupts being disabled etc.

cheers