Re: [PATCH 2/9] perf/x86: core_misc PMU disable and enable support

[Mark Rutland <mark.rutland@arm.com>]

On Thu, Jul 16, 2015 at 09:33:44PM +0100, kan.liang@intel.com wrote:
> From: Kan Liang <kan.liang@intel.com>
> 
> This patch implements core_misc PMU disable and enable functions.
> core_misc PMU counters are free running counters, so it's impossible to
> stop/start them.

Doesn't that effectively mean you can't group them? You'll get arbitrary
noise because counters will be incrementing as you read them.

[...]

> @@ -927,6 +933,10 @@ int p6_pmu_init(void);
>  
>  int knc_pmu_init(void);
>  
> +void intel_core_misc_pmu_enable(void);
> +
> +void intel_core_misc_pmu_disable(void);
> +
>  ssize_t events_sysfs_show(struct device *dev, struct device_attribute *attr,
>  			  char *page);
>  
> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> index b9826a9..651a86d 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -1586,6 +1586,8 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
>  	if (!x86_pmu.late_ack)
>  		apic_write(APIC_LVTPC, APIC_DM_NMI);
>  	__intel_pmu_disable_all();
> +	if (cpuc->core_misc_active_mask)
> +		intel_core_misc_pmu_disable();

Huh? Free running counters have nothing to do with the PMU interrupt;
there's nothing they can do to trigger it. This feels very hacky.

If this is necessary, surely it should live in __intel_pmu_disable_all?

[...]

> +void intel_core_misc_pmu_enable(void)
> +{
> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +	struct perf_event *event;
> +	u64 start;
> +	int bit;
> +
> +	for_each_set_bit(bit, cpuc->core_misc_active_mask,
> +			 X86_CORE_MISC_COUNTER_MAX) {
> +		event = cpuc->core_misc_events[bit];
> +		start = core_misc_pmu_read_counter(event);
> +		local64_set(&event->hw.prev_count, start);
> +	}
> +}
> +
> +void intel_core_misc_pmu_disable(void)
> +{
> +	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> +	int bit;
> +
> +	for_each_set_bit(bit, cpuc->core_misc_active_mask,
> +			 X86_CORE_MISC_COUNTER_MAX) {
> +		core_misc_pmu_event_update(cpuc->core_misc_events[bit]);
> +	}
> +}
> +
>  static void core_misc_pmu_event_del(struct perf_event *event, int mode)
>  {
>  	core_misc_pmu_event_stop(event, PERF_EF_UPDATE);
> @@ -863,6 +899,11 @@ static void __init core_misc_pmus_register(void)
>  			.capabilities	= PERF_PMU_CAP_NO_INTERRUPT,
>  		};
>  
> +		if (type->type == perf_intel_core_misc_thread) {
> +			type->pmu.pmu_disable = (void *) intel_core_misc_pmu_disable;
> +			type->pmu.pmu_enable = (void *) intel_core_misc_pmu_enable;

Why are you suprressing an entirely valid compiler warning here?

The signatures of intel_core_misc_pmu_{enable,disable} aren't right. Fix
them to take a struct pmu *.

Mark.