On Thu, Jul 16, 2015 at 09:33:44PM +0100, kan.li...@intel.com wrote:
> From: Kan Liang <kan.li...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
