Messages in this thread |  | | Subject | Re: [PATCH V4 08/10] powerpc, perf: Enable SW filtering in branch stack sampling framework | From | Michael Ellerman <> | Date | Tue, 24 Dec 2013 14:29:50 +1100 |
| |
On Fri, 2013-12-20 at 16:31 +0530, Anshuman Khandual wrote: > On 12/09/2013 11:51 AM, Michael Ellerman wrote: > > On Wed, 2013-04-12 at 10:32:40 UTC, Anshuman Khandual wrote: > >> + > >> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_IND_CALL) { > >> + /* XL-form instruction */ > >> + if (instr_is_branch_xlform(*addr)) { > >> + > >> + /* LR should be set */ > >> + if (is_branch_link_set(*addr)) { > >> + /* > >> + * Conditional and unconditional > >> + * branch to CTR. > >> + */ > >> + if (is_xlform_ctr(*addr)) > >> + result = true; > >> + > >> + /* > >> + * Conditional and unconditional > >> + * branch to LR. > >> + */ > >> + if (is_xlform_lr(*addr)) > >> + result = true; > >> + > >> + /* > >> + * Conditional and unconditional > >> + * branch to TAR. > >> + */ > >> + if (is_xlform_tar(*addr)) > >> + result = true; > > > > What other kind of XL-Form branch is there? > > I am not sure. Do you know of any ?
That was my point. There are no other types, so you can just do:
if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_IND_CALL) if (instr_is_branch_xlform(*addr) && is_branch_link_set(*addr)) return true;
> >> + if (bhrb_sw_filter & PERF_SAMPLE_BRANCH_COND) { > >> + > >> + /* I-form instruction - excluded */ > >> + if (instr_is_branch_iform(*addr)) > >> + goto out; > >> + > >> + /* B-form or XL-form instruction */ > >> + if (instr_is_branch_bform(*addr) || instr_is_branch_xlform(*addr)) { > >> + > >> + /* Not branch always */ > >> + if (!is_bo_always(*addr)) { > >> + > >> + /* Conditional branch to CTR register */ > >> + if (is_bo_ctr(*addr)) > >> + goto out; > > > > We might have discussed this but why not? > > Did not get that, discuss what ?
Why are we saying a conditional branch to the CTR is not a conditional branch?
It is conditional, so I think it should be included.
> >> + > >> + /* CR[BI] conditional branch with static hint */ > > > > A conditional branch with a static hint is still a conditional branch? > > No its not.
Yes it is?
In fact they could be very interesting branches. Because the compiler or programmer has statically hinted them, if the hint is wrong they may be a major source of branch midpredicts.
> >> + if (is_bo_crbi_off(*addr) || is_bo_crbi_on(*addr)) { > >> + if (is_bo_crbi_hint(*addr)) > >> + goto out; > >> + } > >> + > >> + result = true; > >> + } > >> + } > >> + } > >> +out: > >> + return result; > >> +} > >> + } else { > >> + /* > >> + * Userspace address needs to be > >> + * copied first before analysis. > >> + */ > >> + pagefault_disable(); > >> + ret = __get_user_inatomic(instr, (unsigned int __user *)addr); > > > > I suspect you borrowed this incantation from the callchain code. Unlike that > > code you don't fallback to reading the page tables directly. > > > > I'd rather see the accessor in the callchain code made generic and have you > > call it here. > > You have mentioned to take care of this issue yourself.
Yes I will.
cheers
|  |