| Message ID | 1433763511-5270-3-git-send-email-khandual@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
> +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
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.
> >> + > >> /* 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?
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.
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; - cpuhw->bhrb_entries[u_index].predicted = ~pred; - - /* Get from address in next entry */ - val = read_bhrb(r_index++); - if (!val) - break; - addr = val & BHRB_EA; - if (val & BHRB_TARGET) { - /* Shouldn't have two targets in a - row.. Reset index and try again */ - r_index--; - addr = 0; - } - cpuhw->bhrb_entries[u_index].from = addr; - } else { - /* Branches to immediate field - (ie I or B form) */ - cpuhw->bhrb_entries[u_index].from = addr; - cpuhw->bhrb_entries[u_index].to = - power_pmu_bhrb_to(addr); - cpuhw->bhrb_entries[u_index].mispred = pred; - cpuhw->bhrb_entries[u_index].predicted = ~pred; + /* Shouldn't have two targets in a + row.. Reset index and try again */ + r_index--; + addr = 0; } - u_index++; - + update_branch_entry(cpuhw, u_index, addr, tmp, pred); + } else { + /* Branches to immediate field + (ie I or B form) */ + tmp = power_pmu_bhrb_to(addr); + update_branch_entry(cpuhw, u_index, addr, tmp, pred); } + u_index++; } cpuhw->bhrb_stack.nr = u_index; return;
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 <khandual@linux.vnet.ibm.com> --- arch/powerpc/perf/core-book3s.c | 107 ++++++++++++++++++++-------------------- 1 file changed, 54 insertions(+), 53 deletions(-)