linux-kernel - Re: [RFC] x86, perf: Add an aperfmperf driver

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:	Wed, 29 Apr 2015 18:17:05 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Len Brown <len.brown@...el.com>,
	Paul Mackerras <paulus@...ba.org>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Ingo Molnar <mingo@...hat.com>
Subject: Re: [RFC] x86, perf: Add an aperfmperf driver

On Wed, Apr 29, 2015 at 11:50 AM, Andy Lutomirski <luto@...capital.net> wrote:
> On Apr 29, 2015 2:09 AM, "Peter Zijlstra" <peterz@...radead.org> wrote:
>>
>> On Tue, Apr 28, 2015 at 02:25:37PM -0700, Andy Lutomirski wrote:
>> > diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
>> > index 80091ae54c2b..fadc822efc90 100644
>> > --- a/arch/x86/kernel/cpu/Makefile
>> > +++ b/arch/x86/kernel/cpu/Makefile
>> > @@ -45,6 +45,8 @@ obj-$(CONFIG_PERF_EVENTS_INTEL_UNCORE)      += perf_event_intel_uncore.o \
>> >                                          perf_event_intel_uncore_snb.o \
>> >                                          perf_event_intel_uncore_snbep.o \
>> >                                          perf_event_intel_uncore_nhmex.o
>> > +obj-$(CONFIG_CPU_SUP_INTEL)          += perf_event_aperf_mperf.o
>> > +obj-$(CONFIG_CPU_SUP_AMD)            += perf_event_aperf_mperf.o
>>
>> Does this actually work? I would expect it to go complain about having
>> to build it twice if you have both set.
>
> No, but only because I spelled the filename wrong while regenerating
> the patch.  Oops!
>
>>
>> > diff --git a/arch/x86/kernel/cpu/perf_event_aperfmperf.c b/arch/x86/kernel/cpu/perf_event_aperfmperf.c
>> > new file mode 100644
>> > index 000000000000..6e6d113bd9ce
>> > --- /dev/null
>> > +++ b/arch/x86/kernel/cpu/perf_event_aperfmperf.c
>> > @@ -0,0 +1,119 @@
>> > +#include <linux/perf_event.h>
>> > +
>> > +#define APERFMPERF_EVENT_APERF 0
>> > +#define APERFMPERF_EVENT_MPERF 1
>> > +
>>
>> > +static int aperfmperf_event_init(struct perf_event *event)
>> > +{
>> > +     if (event->attr.type != event->pmu->type)
>> > +             return -ENOENT;
>> > +
>> > +     if (event->attr.config != APERFMPERF_EVENT_APERF &&
>> > +         event->attr.config != APERFMPERF_EVENT_MPERF)
>> > +             return -ENOENT;
>>
>> Once we pass the type test we know its 'our' event, and we can go return
>> fatal errors. No other PMU will pick this up.
>>
>> This could therefore turn into an -EINVAL.
>>
>> > +
>> > +     if (event->attr.config1 != 0)
>> > +             return -ENOENT;
>>
>> Idem.
>>
>> > +     /* no sampling */
>> > +     if (event->hw.sample_period)
>> > +             return -EINVAL;
>>
>> You could have set pmu::capabilities =
>> PERF_PMU_CAP_NO_INTERRUPT which would also have killed that dead.
>
>
> That checks attr.sample_period.  I'm a bit confused about the
> relationship between event->hw and event->attr.  Do I not need to
> check hw.sample_period?
>
>>
>> > +     /* unsupported modes and filters */
>> > +     if (event->attr.exclude_user   ||
>> > +         event->attr.exclude_kernel ||
>> > +         event->attr.exclude_hv     ||
>> > +         event->attr.exclude_idle   ||
>> > +         event->attr.exclude_host   ||
>> > +         event->attr.exclude_guest  ||
>> > +         event->attr.freq           ||
>> > +         event->attr.sample_period) /* no sampling */
>> > +             return -EINVAL;
>> > +
>> > +     event->hw.idx = -1;
>> > +     event->hw.event_base = (event->attr.config == APERFMPERF_EVENT_APERF ?
>> > +                             MSR_IA32_APERF : MSR_IA32_MPERF);
>> > +
>> > +     return 0;
>> > +}
>>
>> The rest looks about right. Very simple thing indeed ;-)
>
> Before I submit v2, do you think this is actually worth doing?  I can
> see it being useful for answering questions like "did this workload
> end up running at full speed".
>

To clarify, this is partially redundant with "cpu-cycles" and
"ref-cycles".  That being said, these are simpler, actually documented
as being appropriate for measuring cpu performance states, and don't
have any scheduling constraints.

Also, is perf stat able to count while idle?  perf stat -a -e
cpu-cycles sleep 1 reports very small numbers.

> --Andy



-- 
Andy Lutomirski
AMA Capital Management, LLC
--
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/