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

[Dey, Megha]

>-----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 void intel_bm_event_update(struct perf_event *event) {
>> +	union bm_detect_status cur_stat;
>> +
>> +	rdmsrl(BR_DETECT_STATUS_MSR, cur_stat.raw);
>> +	local64_set(&event->hw.prev_count, (uint64_t)cur_stat.raw); }
>> +
>> +static void intel_bm_event_stop(struct perf_event *event, int mode) {
>> +	wrmsrl(BR_DETECT_COUNTER_CONFIG_BASE + event->id,
>> +	       (event->hw.bm_counter_conf & ~1));
>> +
>> +	intel_bm_event_update(event);
>> +}
>> +
>> +static void intel_bm_event_del(struct perf_event *event, int flags) {
>> +	intel_bm_event_stop(event, flags);
>> +}
>> +
>> +static void intel_bm_event_read(struct perf_event *event) { }
>
>should you call intel_bm_event_update in here? so the read syscall gets
>updated data in case the case the event is active

We do not update the event->count in the intel_bm_event_update function. We are basically saving the status register contents when a task is being scheduled out. So it has nothing to do with the read syscall. Having said that, I will probably put what stop() does in del() and get rid of the stop() function.
>
>jirka