RE: [PATCH V0 2/3] perf/x86/intel/bm.c: Add Intel Branch Monitoring support

From: Dey, Megha
Date: Fri Nov 10 2017 - 19:50:20 EST




>-----Original Message-----
>From: Jiri Olsa [mailto:jolsa@xxxxxxxxxx]
>Sent: Saturday, November 4, 2017 6:26 AM
>To: Megha Dey <megha.dey@xxxxxxxxxxxxxxx>
>Cc: x86@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
>doc@xxxxxxxxxxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx;
>hpa@xxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx;
>kstewart@xxxxxxxxxxxxxxxxxxx; Yu, Yu-cheng <yu-cheng.yu@xxxxxxxxx>;
>Brown, Len <len.brown@xxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
>peterz@xxxxxxxxxxxxx; acme@xxxxxxxxxx;
>alexander.shishkin@xxxxxxxxxxxxxxx; namhyung@xxxxxxxxxx;
>vikas.shivappa@xxxxxxxxxxxxxxx; pombredanne@xxxxxxxx;
>me@xxxxxxxxxxxx; bp@xxxxxxx; Andrejczuk, Grzegorz
><grzegorz.andrejczuk@xxxxxxxxx>; Luck, Tony <tony.luck@xxxxxxxxx>;
>corbet@xxxxxxx; Shankar, Ravi V <ravi.v.shankar@xxxxxxxxx>; Dey, Megha
><megha.dey@xxxxxxxxx>
>Subject: Re: [PATCH V0 2/3] perf/x86/intel/bm.c: Add Intel Branch
>Monitoring support
>
>On Fri, Nov 03, 2017 at 11:00:05AM -0700, Megha Dey wrote:
>
>SNIP
>
>> +static unsigned int bm_threshold = BM_MAX_THRESHOLD; static
>unsigned
>> +int bm_mispred_evt_cnt;
>> +
>> +/* Branch monitoring counter owners */ static struct perf_event
>> +*bm_counter_owner[2];
>
>SNIP
>
>> + * Find a hardware counter for the target task
>> + */
>> + for (i = 0; i < bm_num_counters; i++) {
>> + if ((bm_counter_owner[i] == NULL) ||
>> + (bm_counter_owner[i]->state ==
>PERF_EVENT_STATE_DEAD)) {
>> + counter_to_use = i;
>> + bm_counter_owner[i] = event;
>> + break;
>> + }
>> + }
>> +
>> + if (counter_to_use == -1)
>> + return -EBUSY;
>
>not sure I understand, your docs says: "There are 2 8-bit counters that each..
>"
>
>so there are 2 counters per CPU? if that's corrent, isn't this check too strict
>then? you could have more events configured running on other CPUs for
>another tasks
>
>given that we do task only events here, should bm_counter_owner be part
>of task, together with the limit..? I'm probably missing something..

Yes you are right. Initially, we had support for 2 events(from one or 2 tasks) to be monitored for the entire system. This indeed seems very limiting. In the next patchset, I will add support for 2 events per task (This is what the hardware can support).
>
>thanks,
>jirka