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 <redacted>
---
arch/powerpc/perf/core-book3s.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 <redacted>
---
arch/powerpc/perf/core-book3s.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 <redacted>
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
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 <redacted>
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
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 <redacted>
---
arch/powerpc/perf/core-book3s.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-08-14 12:25:58
Christophe Leroy [off-list ref] writes:
Le 13/08/2021 à 10:24, Kajol Jain a écrit :
quoted
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 <redacted>
---
arch/powerpc/perf/core-book3s.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-08-14 12:31:22
Christophe Leroy [off-list ref] writes:
Le 13/08/2021 à 10:29, kajoljain a écrit :
quoted
On 8/13/21 1:54 PM, Kajol Jain wrote:
quoted
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 <redacted>
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
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-08-14 12:44:51
Christophe Leroy [off-list ref] writes:
Le 13/08/2021 à 10:24, Kajol Jain a écrit :
quoted
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?
quoted
Patch resolves this issue by adding a ternary condition to return
regs->nip incase SIAR is 0.
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
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 <redacted>
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
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.
quoted
quoted
Patch resolves this issue by adding a ternary condition to return
regs->nip incase SIAR is 0.
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
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.
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.
quoted
quoted
quoted
Patch resolves this issue by adding a ternary condition to return
regs->nip incase SIAR is 0.
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.
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.
quoted
quoted
quoted
Patch resolves this issue by adding a ternary condition to return
regs->nip incase SIAR is 0.
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.
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.
quoted
quoted
quoted
Patch resolves this issue by adding a ternary condition to return
regs->nip incase SIAR is 0.
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.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-08-17 12:50:05
Christophe Leroy [off-list ref] writes:
Le 16/08/2021 à 08:44, kajoljain a écrit :
quoted
On 8/14/21 6:14 PM, Michael Ellerman wrote:
...
quoted
quoted
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.
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