* [PATCH] powerpc, perf: Fix processing conditions for invalid BHRB entries
@ 2013-05-06 9:06 Anshuman Khandual
2013-05-06 11:11 ` Michael Neuling
0 siblings, 1 reply; 4+ messages in thread
From: Anshuman Khandual @ 2013-05-06 9:06 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel; +Cc: mikey, khandual
Fixing some conditions during BHRB entry processing.
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
arch/powerpc/perf/core-book3s.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 09db68d..1de2756 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -1481,25 +1481,25 @@ void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
target = val & BHRB_TARGET;
/* Probable Missed entry: Not applicable for POWER8 */
- if ((addr == 0) && (target == 0) && (pred == 1)) {
+ if ((addr == 0) && (!target) && pred) {
r_index++;
continue;
}
/* Real Missed entry: Power8 based missed entry */
- if ((addr == 0) && (target == 1) && (pred == 1)) {
+ if ((addr == 0) && target && pred) {
r_index++;
continue;
}
/* Reserved condition: Not a valid entry */
- if ((addr == 0) && (target == 1) && (pred == 0)) {
+ if ((addr == 0) && target && (!pred)) {
r_index++;
continue;
}
/* Is a target address */
- if (val & BHRB_TARGET) {
+ if (target) {
/* First address cannot be a target address */
if (r_index == 0) {
r_index++;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc, perf: Fix processing conditions for invalid BHRB entries
2013-05-06 9:06 [PATCH] powerpc, perf: Fix processing conditions for invalid BHRB entries Anshuman Khandual
@ 2013-05-06 11:11 ` Michael Neuling
2013-05-06 12:25 ` Anshuman Khandual
0 siblings, 1 reply; 4+ messages in thread
From: Michael Neuling @ 2013-05-06 11:11 UTC (permalink / raw)
To: Anshuman Khandual; +Cc: linuxppc-dev, linux-kernel
Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:
> Fixing some conditions during BHRB entry processing.
I think we can simplify this a lot more... something like the below.
Also, this marks the "to" address as all 1s, which is better poison
value since it's possible to branch to/from 0x0.
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index c627843..d410d65 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -1463,65 +1463,45 @@ void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
{
u64 val;
u64 addr;
- int r_index, u_index, target, pred;
+ int r_index, u_index, pred;
r_index = 0;
u_index = 0;
while (r_index < ppmu->bhrb_nr) {
/* Assembly read function */
- val = read_bhrb(r_index);
+ val = read_bhrb(r_index++);
/* Terminal marker: End of valid BHRB entries */
- if (val == 0) {
+ if (!val) {
break;
} else {
/* BHRB field break up */
addr = val & BHRB_EA;
pred = val & BHRB_PREDICTION;
- target = val & BHRB_TARGET;
- /* Probable Missed entry: Not applicable for POWER8 */
- if ((addr == 0) && (target == 0) && (pred == 1)) {
- r_index++;
+ if (!addr)
+ /* invalid entry */
continue;
- }
-
- /* Real Missed entry: Power8 based missed entry */
- if ((addr == 0) && (target == 1) && (pred == 1)) {
- r_index++;
- continue;
- }
- /* Reserved condition: Not a valid entry */
- if ((addr == 0) && (target == 1) && (pred == 0)) {
- r_index++;
- continue;
- }
-
- /* Is a target address */
if (val & BHRB_TARGET) {
/* First address cannot be a target address */
- if (r_index == 0) {
- r_index++;
+ if (r_index == 0)
continue;
- }
/* Update target address for the previous entry */
cpuhw->bhrb_entries[u_index - 1].to = addr;
cpuhw->bhrb_entries[u_index - 1].mispred = pred;
cpuhw->bhrb_entries[u_index - 1].predicted = ~pred;
-
- /* Dont increment u_index */
- r_index++;
} else {
/* Update address, flags for current entry */
cpuhw->bhrb_entries[u_index].from = addr;
+ cpuhw->bhrb_entries[u_index].to =
+ 0xffffffffffffffff;
cpuhw->bhrb_entries[u_index].mispred = pred;
cpuhw->bhrb_entries[u_index].predicted = ~pred;
/* Successfully popullated one entry */
u_index++;
- r_index++;
}
}
}
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc, perf: Fix processing conditions for invalid BHRB entries
2013-05-06 11:11 ` Michael Neuling
@ 2013-05-06 12:25 ` Anshuman Khandual
2013-05-06 21:22 ` Michael Neuling
0 siblings, 1 reply; 4+ messages in thread
From: Anshuman Khandual @ 2013-05-06 12:25 UTC (permalink / raw)
To: Michael Neuling; +Cc: linuxppc-dev, linux-kernel
On 05/06/2013 04:41 PM, Michael Neuling wrote:
> Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:
>
>> Fixing some conditions during BHRB entry processing.
>
> I think we can simplify this a lot more... something like the below.
>
I feel that the conditional handling of the invalid BHRB entries should be
present which would help us during the debug and also could be used for more
granular branch classification or error handling later on.
> Also, this marks the "to" address as all 1s, which is better poison
> value since it's possible to branch to/from 0x0.
>
Agreed.
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index c627843..d410d65 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -1463,65 +1463,45 @@ void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
> {
> u64 val;
> u64 addr;
> - int r_index, u_index, target, pred;
> + int r_index, u_index, pred;
>
> r_index = 0;
> u_index = 0;
> while (r_index < ppmu->bhrb_nr) {
> /* Assembly read function */
> - val = read_bhrb(r_index);
> + val = read_bhrb(r_index++);
>
> /* Terminal marker: End of valid BHRB entries */
> - if (val == 0) {
> + if (!val) {
> break;
> } else {
> /* BHRB field break up */
> addr = val & BHRB_EA;
> pred = val & BHRB_PREDICTION;
> - target = val & BHRB_TARGET;
>
> - /* Probable Missed entry: Not applicable for POWER8 */
> - if ((addr == 0) && (target == 0) && (pred == 1)) {
> - r_index++;
> + if (!addr)
> + /* invalid entry */
> continue;
> - }
> -
> - /* Real Missed entry: Power8 based missed entry */
> - if ((addr == 0) && (target == 1) && (pred == 1)) {
> - r_index++;
> - continue;
> - }
>
> - /* Reserved condition: Not a valid entry */
> - if ((addr == 0) && (target == 1) && (pred == 0)) {
> - r_index++;
> - continue;
> - }
> -
> - /* Is a target address */
> if (val & BHRB_TARGET) {
> /* First address cannot be a target address */
> - if (r_index == 0) {
> - r_index++;
> + if (r_index == 0)
> continue;
> - }
>
> /* Update target address for the previous entry */
> cpuhw->bhrb_entries[u_index - 1].to = addr;
> cpuhw->bhrb_entries[u_index - 1].mispred = pred;
> cpuhw->bhrb_entries[u_index - 1].predicted = ~pred;
> -
> - /* Dont increment u_index */
> - r_index++;
> } else {
> /* Update address, flags for current entry */
> cpuhw->bhrb_entries[u_index].from = addr;
> + cpuhw->bhrb_entries[u_index].to =
> + 0xffffffffffffffff;
> cpuhw->bhrb_entries[u_index].mispred = pred;
> cpuhw->bhrb_entries[u_index].predicted = ~pred;
>
> /* Successfully popullated one entry */
> u_index++;
> - r_index++;
> }
> }
> }
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc, perf: Fix processing conditions for invalid BHRB entries
2013-05-06 12:25 ` Anshuman Khandual
@ 2013-05-06 21:22 ` Michael Neuling
0 siblings, 0 replies; 4+ messages in thread
From: Michael Neuling @ 2013-05-06 21:22 UTC (permalink / raw)
To: Anshuman Khandual; +Cc: linuxppc-dev, linux-kernel
Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:
> On 05/06/2013 04:41 PM, Michael Neuling wrote:
> > Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:
> >
> >> Fixing some conditions during BHRB entry processing.
> >
> > I think we can simplify this a lot more... something like the below.
> >
>
> I feel that the conditional handling of the invalid BHRB entries should be
> present which would help us during the debug and also could be used for more
> granular branch classification or error handling later on.
If we need that, then we can add it later on. You'd still need to
modify the code to do any debug, branch classification or error handling
as the code is now. It's just an unnecessary place holder.
The code is unnecessarily verbose now. This patch removes about 20
lines of code and does the same thing.
Mikey
>
> > Also, this marks the "to" address as all 1s, which is better poison
> > value since it's possible to branch to/from 0x0.
> >
>
> Agreed.
>
>
> > diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> > index c627843..d410d65 100644
> > --- a/arch/powerpc/perf/core-book3s.c
> > +++ b/arch/powerpc/perf/core-book3s.c
> > @@ -1463,65 +1463,45 @@ void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
> > {
> > u64 val;
> > u64 addr;
> > - int r_index, u_index, target, pred;
> > + int r_index, u_index, pred;
> >
> > r_index = 0;
> > u_index = 0;
> > while (r_index < ppmu->bhrb_nr) {
> > /* Assembly read function */
> > - val = read_bhrb(r_index);
> > + val = read_bhrb(r_index++);
> >
> > /* Terminal marker: End of valid BHRB entries */
> > - if (val == 0) {
> > + if (!val) {
> > break;
> > } else {
> > /* BHRB field break up */
> > addr = val & BHRB_EA;
> > pred = val & BHRB_PREDICTION;
> > - target = val & BHRB_TARGET;
> >
> > - /* Probable Missed entry: Not applicable for POWER8 */
> > - if ((addr == 0) && (target == 0) && (pred == 1)) {
> > - r_index++;
> > + if (!addr)
> > + /* invalid entry */
> > continue;
> > - }
> > -
> > - /* Real Missed entry: Power8 based missed entry */
> > - if ((addr == 0) && (target == 1) && (pred == 1)) {
> > - r_index++;
> > - continue;
> > - }
> >
> > - /* Reserved condition: Not a valid entry */
> > - if ((addr == 0) && (target == 1) && (pred == 0)) {
> > - r_index++;
> > - continue;
> > - }
> > -
> > - /* Is a target address */
> > if (val & BHRB_TARGET) {
> > /* First address cannot be a target address */
> > - if (r_index == 0) {
> > - r_index++;
> > + if (r_index == 0)
> > continue;
> > - }
> >
> > /* Update target address for the previous entry */
> > cpuhw->bhrb_entries[u_index - 1].to = addr;
> > cpuhw->bhrb_entries[u_index - 1].mispred = pred;
> > cpuhw->bhrb_entries[u_index - 1].predicted = ~pred;
> > -
> > - /* Dont increment u_index */
> > - r_index++;
> > } else {
> > /* Update address, flags for current entry */
> > cpuhw->bhrb_entries[u_index].from = addr;
> > + cpuhw->bhrb_entries[u_index].to =
> > + 0xffffffffffffffff;
> > cpuhw->bhrb_entries[u_index].mispred = pred;
> > cpuhw->bhrb_entries[u_index].predicted = ~pred;
> >
> > /* Successfully popullated one entry */
> > u_index++;
> > - r_index++;
> > }
> > }
> > }
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-05-06 21:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-06 9:06 [PATCH] powerpc, perf: Fix processing conditions for invalid BHRB entries Anshuman Khandual
2013-05-06 11:11 ` Michael Neuling
2013-05-06 12:25 ` Anshuman Khandual
2013-05-06 21:22 ` Michael Neuling
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).