Re: [PATCH V4 08/10] powerpc, perf: Enable SW filtering in branchstack sampling framework

From: Michael Ellerman
Date: Mon Dec 23 2013 - 22:30:01 EST


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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/