Re: [PATCH V8 03/10] powerpc, perf: Re organize BHRB processing
On 06/11/2015 09:02 AM, Daniel Axtens wrote:
+
/* Processing BHRB entries */
static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
{
- u64 val;
- u64 addr;
+ u64 val, addr, tmp;
Please don't use 'tmp' here. As far as I can tell, you use this variable
to compute the 'to' address. The name should reflect that.
Agreed but then it will be a new preparatory patch at the beginning
of this patch series.
I don't think I understand what you're saying here. Why do you need a
new patch? As I understand it, you've introduced 'tmp' in this patch;
couldn't you just rename it to, for example, to_addr, instead of tmp in
this patch?
Sorry for the confusion, I meant separate patch for the other
two changes I had agreed to (i.e changing the name and type of
'pred' variable) as suggested in the previous mail not for this
one. Will change 'tmp' into 'to_addr' in this patch itself.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V8 03/10] powerpc, perf: Re organize BHRB processing
On 06/10/2015 10:06 AM, Daniel Axtens wrote:
+void update_branch_entry(struct cpu_hw_events *cpuhw,
+int index, u64 from, u64 to, int pred)
+{
+cpuhw-bhrb_entries[index].from = from;
+cpuhw-bhrb_entries[index].to = to;
+cpuhw-bhrb_entries[index].mispred = pred;
+cpuhw-bhrb_entries[index].predicted = ~pred;
+}
I realise you're copying existing code, but:
- could you please rename pred? If we assign .mispred to pred
and .predicted to ~pred, we should pick a different name for pred.
Agreed.
- I'm really uncomfortable with the bitwise inverting a signed integer.
Can you explain what is going on here? Looking at
include/uapi/linux/perf_event.h, this seems to be a single bit flag:
shouldn't this then be a logical flip rather than a bitwise one?
(Furthermore, looking at that header, why is pred an int at all? Why not
a bool?)
Agreed.
+
/* Processing BHRB entries */
static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
{
-u64 val;
-u64 addr;
+u64 val, addr, tmp;
Please don't use 'tmp' here. As far as I can tell, you use this variable
to compute the 'to' address. The name should reflect that.
Agreed but then it will be a new preparatory patch at the beginning
of this patch series.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V8 03/10] powerpc, perf: Re organize BHRB processing
+
/* Processing BHRB entries */
static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
{
- u64 val;
- u64 addr;
+ u64 val, addr, tmp;
Please don't use 'tmp' here. As far as I can tell, you use this variable
to compute the 'to' address. The name should reflect that.
Agreed but then it will be a new preparatory patch at the beginning
of this patch series.
I don't think I understand what you're saying here. Why do you need a
new patch? As I understand it, you've introduced 'tmp' in this patch;
couldn't you just rename it to, for example, to_addr, instead of tmp in
this patch?
--
Regards,
Daniel
signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V8 03/10] powerpc, perf: Re organize BHRB processing
+void update_branch_entry(struct cpu_hw_events *cpuhw,
+ int index, u64 from, u64 to, int pred)
+{
+ cpuhw-bhrb_entries[index].from = from;
+ cpuhw-bhrb_entries[index].to = to;
+ cpuhw-bhrb_entries[index].mispred = pred;
+ cpuhw-bhrb_entries[index].predicted = ~pred;
+}
I realise you're copying existing code, but:
- could you please rename pred? If we assign .mispred to pred
and .predicted to ~pred, we should pick a different name for pred.
- I'm really uncomfortable with the bitwise inverting a signed integer.
Can you explain what is going on here? Looking at
include/uapi/linux/perf_event.h, this seems to be a single bit flag:
shouldn't this then be a logical flip rather than a bitwise one?
(Furthermore, looking at that header, why is pred an int at all? Why not
a bool?)
+
/* Processing BHRB entries */
static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
{
- u64 val;
- u64 addr;
+ u64 val, addr, tmp;
Please don't use 'tmp' here. As far as I can tell, you use this variable
to compute the 'to' address. The name should reflect that.
Regards,
Daniel
signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH V8 03/10] powerpc, perf: Re organize BHRB processing
This patch cleans up some existing indentation problem in code and
re organizes the BHRB processing code with an helper function named
'update_branch_entry' making it more readable. This patch does not
change any functionality.
Signed-off-by: Anshuman Khandual khand...@linux.vnet.ibm.com
---
arch/powerpc/perf/core-book3s.c | 107
1 file changed, 54 insertions(+), 53 deletions(-)
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index ae61629..d10d2c1 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -412,11 +412,19 @@ static __u64 power_pmu_bhrb_to(u64 addr)
return target - (unsigned long)instr + addr;
}
+void update_branch_entry(struct cpu_hw_events *cpuhw,
+ int index, u64 from, u64 to, int pred)
+{
+ cpuhw-bhrb_entries[index].from = from;
+ cpuhw-bhrb_entries[index].to = to;
+ cpuhw-bhrb_entries[index].mispred = pred;
+ cpuhw-bhrb_entries[index].predicted = ~pred;
+}
+
/* Processing BHRB entries */
static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
{
- u64 val;
- u64 addr;
+ u64 val, addr, tmp;
int r_index, u_index, pred;
r_index = 0;
@@ -427,63 +435,56 @@ static void power_pmu_bhrb_read(struct cpu_hw_events
*cpuhw)
if (!val)
/* Terminal marker: End of valid BHRB entries */
break;
- else {
- addr = val BHRB_EA;
- pred = val BHRB_PREDICTION;
- if (!addr)
- /* invalid entry */
- continue;
+ addr = val BHRB_EA;
+ pred = val BHRB_PREDICTION;
+
+ if (!addr)
+ /* invalid entry */
+ continue;
- /* Branches are read most recent first (ie. mfbhrb 0 is
-* the most recent branch).
-* There are two types of valid entries:
-* 1) a target entry which is the to address of a
-*computed goto like a blr,bctr,btar. The next
-*entry read from the bhrb will be branch
-*corresponding to this target (ie. the actual
-*blr/bctr/btar instruction).
-* 2) a from address which is an actual branch. If a
-*target entry proceeds this, then this is the
-*matching branch for that target. If this is not
-*following a target entry, then this is a branch
-*where the target is given as an immediate field
-*in the instruction (ie. an i or b form branch).
-*In this case we need to read the instruction from
-*memory to determine the target/to address.
+ /* Branches are read most recent first (ie. mfbhrb 0 is
+* the most recent branch).
+* There are two types of valid entries:
+* 1) a target entry which is the to address of a
+*computed goto like a blr,bctr,btar. The next
+*entry read from the bhrb will be branch
+*corresponding to this target (ie. the actual
+*blr/bctr/btar instruction).
+* 2) a from address which is an actual branch. If a
+*target entry proceeds this, then this is the
+*matching branch for that target. If this is not
+*following a target entry, then this is a branch
+*where the target is given as an immediate field
+*in the instruction (ie. an i or b form branch).
+*In this case we need to read the instruction from
+*memory to determine the target/to address.
+*/
+ if (val BHRB_TARGET) {
+ /* Target branches use two entries
+* (ie. computed gotos/XL form)
*/
+ tmp = addr;
+
+ /* Get from address in next entry */
+ val = read_bhrb(r_index++);
+ if (!val)
+ break;
+ addr = val BHRB_EA;
if (val BHRB_TARGET) {
- /* Target branches use two entries
-* (ie. computed gotos/XL form)
-*/
- cpuhw-bhrb_entries[u_index].to = addr;
- cpuhw-bhrb_entries[u_index].mispred = pred;
-
