linux-kernel - Re: [RFC PATCH 2/3 v2] perf: Implement Nehalem uncore pmu

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:	Sun, 21 Nov 2010 13:46:21 +0100
From:	"Andi Kleen" <andi@...stfloor.org>
To:	"Lin Ming" <ming.m.lin@...el.com>
Cc:	"Peter Zijlstra" <a.p.zijlstra@...llo.nl>,
	"Ingo Molnar" <mingo@...e.hu>,
	"Stephane Eranian" <eranian@...gle.com>,
	"Andi Kleen" <andi@...stfloor.org>,
	"lkml" <linux-kernel@...r.kernel.org>,
	"Frederic Weisbecker" <fweisbec@...il.com>,
	"Arjan van de Ven" <arjan@...radead.org>
Subject: Re: [RFC PATCH 2/3 v2] perf: Implement Nehalem uncore pmu


>
> 2. Uncore pmu NMI handling
>
> All the 4 cores are programmed to receive uncore counter overflow
> interrupt. The NMI handler(running on 1 of the 4 cores) handle all
> counters enabled by all 4 cores.

Really for uncore monitoring there is no need to use an NMI handler.
You can't profile a core anyways, so you can just delay the reporting
a little bit. It may simplify the code to not use one here
and just use an ordinary handler.

In general since there is already much trouble with overloaded
NMI events avoiding new NMIs is a good idea.



> +
> +static struct node_hw_events *uncore_events[MAX_NUMNODES];

Don't declare static arrays with MAX_NUMNODES, that number can be
very large and cause unnecessary bloat. Better use per CPU data or similar
(e.g. with  alloc_percpu)

> +	/*
> +	 * The hw event starts counting from this event offset,
> +	 * mark it to be able to extra future deltas:
> +	 */
> +	local64_set(&hwc->prev_count, (u64)-left);

Your use of local* seems dubious. That is only valid if it's really
all on the same CPU. Is that really true?

> +static int uncore_pmu_add(struct perf_event *event, int flags)
> +{
> +	int node = numa_node_id();

this should be still package id

> +	/* Check CPUID signatures: 06_1AH, 06_1EH, 06_1FH */
> +	model = eax.split.model | (eax.split.ext_model << 4);
> +	if (eax.split.family != 6 || (model != 0x1A && model != 0x1E && model !=
> 0x1F))
> +		return;

You can just get that from boot_cpu_data, no need to call cpuid

> +#include <linux/perf_event.h>
> +#include <linux/capability.h>
> +#include <linux/notifier.h>
> +#include <linux/hardirq.h>
> +#include <linux/kprobes.h>
> +#include <linux/module.h>
> +#include <linux/kdebug.h>
> +#include <linux/sched.h>
> +#include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/highmem.h>
> +#include <linux/cpu.h>
> +#include <linux/bitops.h>

Do you really need all these includes?


-Andi
--
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/