| 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
| ||
|
Date: Fri, 1 Mar 2019 11:59:40 -0500
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Jiri Olsa <jolsa@...hat.com>, Peter Zijlstra <peterz@...radead.org>
Cc: Vince Weaver <vincent.weaver@...ne.edu>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...nel.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
lkml <linux-kernel@...r.kernel.org>,
Andi Kleen <ak@...ux.intel.com>,
Stephane Eranian <eranian@...gle.com>
Subject: Re: [RFC] perf/x86/rapl: Getting zero on energy-cores event
On 3/1/2019 6:42 AM, Jiri Olsa wrote:
> hi,
> I'm getting zero counts for energy-cores event on broadwell-x
> server (model 0x4f)
>
> I checked intel_rapl powercap driver and it won't export the
> counter if it rdmsr returns zero on it
>
> the SDM also says the rdmsr returns zero for some models
>
> I made changes on perf rapl pmu below to remove sysfs events
> if their rdmsr returns zero just to ilustrate the case
>
> I think there's probably better fix, but I'm not sure if
> there's a reason for zero counters to be exposed..?
+Stephane, who is the author.
I'm OK to hide the zero counters.
>
> thoughts? thanks,
> jirka
>
>
> ---
> diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c
> index 94dc564146ca..effb9a9d2368 100644
> --- a/arch/x86/events/intel/rapl.c
> +++ b/arch/x86/events/intel/rapl.c
> @@ -54,6 +54,7 @@
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/perf_event.h>
> +#include <linux/nospec.h>
> #include <asm/cpu_device_id.h>
> #include <asm/intel-family.h>
> #include "../perf_event.h"
> @@ -346,10 +347,18 @@ static void rapl_pmu_event_del(struct perf_event *event, int flags)
> rapl_pmu_event_stop(event, PERF_EF_UPDATE);
> }
>
> +static unsigned int event_msr[NR_RAPL_DOMAINS] = {
> + MSR_PP0_ENERGY_STATUS,
> + MSR_PKG_ENERGY_STATUS,
> + MSR_DRAM_ENERGY_STATUS,
> + MSR_PP1_ENERGY_STATUS,
> + MSR_PLATFORM_ENERGY_STATUS,
> +};
> +
> static int rapl_pmu_event_init(struct perf_event *event)
> {
> u64 cfg = event->attr.config & RAPL_EVENT_MASK;
> - int bit, msr, ret = 0;
> + int bit, ret = 0;
> struct rapl_pmu *pmu;
>
> /* only look at RAPL events */
> @@ -365,33 +374,12 @@ static int rapl_pmu_event_init(struct perf_event *event)
>
> event->event_caps |= PERF_EV_CAP_READ_ACTIVE_PKG;
>
> - /*
> - * check event is known (determines counter)
> - */
> - switch (cfg) {
> - case INTEL_RAPL_PP0:
> - bit = RAPL_IDX_PP0_NRG_STAT;
> - msr = MSR_PP0_ENERGY_STATUS;
> - break;
> - case INTEL_RAPL_PKG:
> - bit = RAPL_IDX_PKG_NRG_STAT;
> - msr = MSR_PKG_ENERGY_STATUS;
> - break;
> - case INTEL_RAPL_RAM:
> - bit = RAPL_IDX_RAM_NRG_STAT;
> - msr = MSR_DRAM_ENERGY_STATUS;
> - break;
> - case INTEL_RAPL_PP1:
> - bit = RAPL_IDX_PP1_NRG_STAT;
> - msr = MSR_PP1_ENERGY_STATUS;
> - break;
> - case INTEL_RAPL_PSYS:
> - bit = RAPL_IDX_PSYS_NRG_STAT;
> - msr = MSR_PLATFORM_ENERGY_STATUS;
> - break;
> - default:
> + if (!cfg || cfg >= NR_RAPL_DOMAINS)
> return -EINVAL;
> - }
> +
> + cfg = array_index_nospec(cfg, NR_RAPL_DOMAINS);
> + bit = cfg - 1;
> +
> /* check event supported */
> if (!(rapl_cntr_mask & (1 << bit)))
> return -EINVAL;
> @@ -406,7 +394,7 @@ static int rapl_pmu_event_init(struct perf_event *event)
> return -EINVAL;
> event->cpu = pmu->cpu;
> event->pmu_private = pmu;
> - event->hw.event_base = msr;
> + event->hw.event_base = event_msr[bit];
> event->hw.config = cfg;
> event->hw.idx = bit;
>
> @@ -435,11 +423,27 @@ static struct attribute_group rapl_pmu_attr_group = {
> .attrs = rapl_pmu_attrs,
> };
>
> -RAPL_EVENT_ATTR_STR(energy-cores, rapl_cores, "event=0x01");
> -RAPL_EVENT_ATTR_STR(energy-pkg , rapl_pkg, "event=0x02");
> -RAPL_EVENT_ATTR_STR(energy-ram , rapl_ram, "event=0x03");
> -RAPL_EVENT_ATTR_STR(energy-gpu , rapl_gpu, "event=0x04");
> -RAPL_EVENT_ATTR_STR(energy-psys, rapl_psys, "event=0x05");
> +static ssize_t
> +rapl_event_sysfs_show(struct device *dev, struct device_attribute *attr,
> + char *page)
> +{
> + struct perf_pmu_events_attr *pmu_attr =
> + container_of(attr, struct perf_pmu_events_attr, attr);
> +
> + return sprintf(page, "event=%llu\n", pmu_attr->id);
> +}
> +
> +#define RAPL_EVENT_ATTR(_name, v, _id) \
> +static struct perf_pmu_events_attr event_attr_##v = { \
> + .attr = __ATTR(_name, 0444, rapl_event_sysfs_show, NULL), \
> + .id = RAPL_IDX_##_id##_NRG_STAT, \
> +};
> +
> +RAPL_EVENT_ATTR(energy-cores, rapl_cores, PP0);
> +RAPL_EVENT_ATTR(energy-pkg , rapl_pkg, PKG);
> +RAPL_EVENT_ATTR(energy-ram , rapl_ram, RAM);
> +RAPL_EVENT_ATTR(energy-gpu , rapl_gpu, PP1);
> +RAPL_EVENT_ATTR(energy-psys , rapl_psys, PSYS);
>
> RAPL_EVENT_ATTR_STR(energy-cores.unit, rapl_cores_unit, "Joules");
> RAPL_EVENT_ATTR_STR(energy-pkg.unit , rapl_pkg_unit, "Joules");
> @@ -780,6 +784,59 @@ static const struct x86_cpu_id rapl_cpu_match[] __initconst = {
>
> MODULE_DEVICE_TABLE(x86cpu, rapl_cpu_match);
>
> +static void __init remove_name(struct attribute **attrs, const char *name)
> +{
> + struct device_attribute *attr;
> + int i, j;
> +
> + for (i = 0; attrs[i]; i++) {
> + attr = (struct device_attribute *) attrs[i];
> +
> + if (strncmp(name, attr->attr.name, strlen(name)))
> + continue;
> +
> + for (j = i; attrs[j]; j++)
> + attrs[j] = attrs[j + 1];
> +
> + /* Check the shifted attr. */
> + i--;
> + }
> +}
> +
> +static void __init check_events(struct attribute **attrs)
> +{
> + struct perf_pmu_events_attr *pmu_attr;
> + struct device_attribute *attr;
> + int i, j;
> +
> + for (i = 0; attrs[i]; i++) {
> + u64 val = 0;
> + u64 bit;
> +
> + attr = (struct device_attribute *) attrs[i];
> + pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr);
> +
> + if (pmu_attr->event_str)
> + continue;
> +
> + bit = pmu_attr->id;
> +
> + if (WARN_ON_ONCE(bit >= NR_RAPL_DOMAINS))
> + continue;
> +
> + if (!rdmsrl_safe(event_msr[bit], &val) && val)
> + continue;
> +
> + remove_name(&attrs[i + 1], attr->attr.name);
In perf cstate, we check and insert the available events for sysfs attrs
I think we should use the same way here. It's better to factor out a
common function for both cstate and rapl.
Peter,
The cstate, rapl and uncore are similar. I think it should be the right
direction to abstract several common functions for them.
Here, the sysfs attrs is an example.
The common topology related functions I proposed is another example.
https://lore.kernel.org/lkml/9631a92f-5b24-26a6-e160-4e4c0b4697c1@linux.intel.com/
Thanks,
Kan
Powered by blists - more mailing lists