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:25 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 int intel_bm_event_nmi_handler(unsigned int cmd, struct
>> +pt_regs *regs) {
>> +	struct perf_event *event;
>> +	union bm_detect_status stat;
>> +	struct perf_sample_data data;
>> +	int i;
>> +	unsigned long x;
>> +
>> +	rdmsrl(BR_DETECT_STATUS_MSR, stat.raw);
>> +
>> +	if (stat.event) {
>> +		wrmsrl(BR_DETECT_STATUS_MSR, 0);
>> +		apic_write(APIC_LVTPC, APIC_DM_NMI);
>> +		/*
>> +		 * Issue wake-up to corrresponding polling event
>> +		 */
>> +		x = stat.ctrl_hit;
>> +		for_each_set_bit(i, &x, bm_num_counters) {
>> +			event = bm_counter_owner[i];
>> +			perf_sample_data_init(&data, 0, event-
>>hw.last_period);
>> +			perf_event_overflow(event, &data, regs);
>
>hum, it's non sampling events only right? then you don't need any of the
>perf_sample_data stuff.. the perf_event_overflow call is basicaly nop

Yeah you are right. We were supporting sampling initially, forgot to get rid of this code.
Will change this in the next version.
>
>> +			local64_inc(&event->count);
>> +			atomic_set(&event->hw.bm_poll, POLLIN);
>> +			event->pending_wakeup = 1;
>> +			irq_work_queue(&event->pending);
>
>also this is for sampling events only
>
>seems like you only want to increment the event->count in here

We are currently working on a library similar to libperf which user space apps could make use of instead of perf command line monitoring. This code has been added so that the user can poll on when/if an interrupt is triggered and let the user know of its occurrence. If you think there is a better way of doing this, please let me know :)

>
>thanks,
>jirka
>
>> +		}
>> +		return NMI_HANDLED;
>> +	}
>> +	return NMI_DONE;
>> +}
>
>
>SNIP