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

[Michael Ellerman <mpe@ellerman.id.au>]

On Tue, 2013-12-24 at 09:20 +0530, Anshuman Khandual wrote:
> On 12/24/2013 08:59 AM, Michael Ellerman wrote:
> > 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_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.

> I believe conditional branch to CTR register and the below conditional branch
> with static hint are excluded when processed with BHRB PMU based filter IFM3,
> Here the SW implemented filter try to match those exclusions, so that a user
> should not see any difference in results whether the filter is processed
> either in PMU or in SW.

OK. That's what I meant by "we might have discussed this".

So you need to make it very clear in the code that we are implementing the IFM3
semantics, with a comment. Otherwise it's not obviously clear why those
semantics make sense.

And we need to make extra sure we implement the same semantics as IFM3, which I
don't think you do at the moment.

The description for IFM3 is:

   Do not record:
    * b and bl instructions, 
    * bc and bcl instructions for which the BO field indicates “Branch always.”
   
   For bclr, bclrl, bctr, bctrl, bctar, and bctarl instructions for which
   the BO field indicates “Branch always,” record only one entry
   containing the Branch target address.

So I don't think your SW filter implements that part correctly. You are
discarding all branches with "branch always" set.


   Do not record:
    * Branch instructions for which BO[0]=1, 

This is what excludes branches to CTR. But, it's only branches to CTR that
don't also depend on CR[BI] - we need to make that clear in the code.

    * Branch instructions for which the “a” bit in the BO field is set to 1.

So that's the is_bo_crbi_hint() check and rejection, but it's not related to
CR[BI] at all.

There's a note about CR[BI]:

    Do not record instructions that do not depend on the value of CR[BI].

But I think you've misinterpreted that. 

    Do not record instructions that do not depend on the value of CR[BI].

    Do     record instructions that        depend on the value of CR[BI].


In fact the only branches that don't depend on CR[BI] are "branch always"
branches, and branches with BO[0]=1, both of which were handled above.

cheers