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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 17 Jul 2015 13:11:41 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	"kan.liang@...el.com" <kan.liang@...el.com>
Cc:	"a.p.zijlstra@...llo.nl" <a.p.zijlstra@...llo.nl>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"acme@...nel.org" <acme@...nel.org>,
	"eranian@...gle.com" <eranian@...gle.com>,
	"ak@...ux.intel.com" <ak@...ux.intel.com>,
	"adrian.hunter@...el.com" <adrian.hunter@...el.com>,
	"dsahern@...il.com" <dsahern@...il.com>,
	"jolsa@...nel.org" <jolsa@...nel.org>,
	"namhyung@...nel.org" <namhyung@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/9] perf/x86: core_misc PMU disable and enable support

On Thu, Jul 16, 2015 at 09:33:44PM +0100, kan.liang@...el.com wrote:
> From: Kan Liang <kan.liang@...el.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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/