Re: [PATCH 11/21] perf/x86/intel/lbr: Support LBR_CTL

[Peter Zijlstra]

On Fri, Jun 19, 2020 at 03:15:09PM -0400, Liang, Kan wrote:
> 
> 
> On 6/19/2020 2:40 PM, Peter Zijlstra wrote:
> > On Fri, Jun 19, 2020 at 07:03:59AM -0700, kan.liang@xxxxxxxxxxxxxxx wrote:
> > > -	if (x86_pmu.extra_regs || x86_pmu.lbr_sel_map) {
> > > +	if (x86_pmu.extra_regs || x86_pmu.lbr_sel_map || x86_pmu.lbr_ctl_map) {
> > 
> > > +	union {
> > > +		u64		lbr_sel_mask;		   /* LBR_SELECT valid bits */
> > > +		u64		lbr_ctl_mask;		   /* LBR_CTL valid bits */
> > > +	};
> > 
> > This makes absolutely no sense. There is hoping the compiler realizes
> > how stupid that is and fixes it for you, but shees.
> > 
> 
> The lbr_ctl_map and the lbr_ctl_mask are two different things.
> 
> The lbr_ctl_map stores the mapping from PERF_SAMPLE_BRANCH_* to the
> corresponding filtering bits in LBR_CTL MSR. It is used to replace the old
> lbr_sel_map. The mapping information in the old lbr_sel_map is hard coded,
> and has a const type. But for arch LBR, the LBR filtering capabilities are
> enumerated from CPUID. We should not hard code the mapping. So I add a new
> variable lbr_ctl_map.
> 
>  	const int	*lbr_sel_map;		   /* lbr_select mappings */
> +	int		*lbr_ctl_map;		   /* LBR_CTL mappings */
> 
> 
> I think we cannot reuse the old lbr_sel_map for the lbr_ctl_map.

Of course you can, you just did it, they're the exact same variable, you
just got confused with all the naming nonsense. You then got further
confused and ended up writing code that checked if a variable was not 0
twice, just to make sure.