| 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
| ||
|
Message-ID: <6eac25a0c523144980b7f606132d49221906e911.camel@intel.com>
Date: Fri, 12 Jul 2024 06:15:16 +0000
From: "Zhang, Rui" <rui.zhang@...el.com>
To: "alexander.shishkin@...ux.intel.com" <alexander.shishkin@...ux.intel.com>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "Hunter, Adrian"
<adrian.hunter@...el.com>, "mingo@...hat.com" <mingo@...hat.com>,
"irogers@...gle.com" <irogers@...gle.com>, "tglx@...utronix.de"
<tglx@...utronix.de>, "gustavoars@...nel.org" <gustavoars@...nel.org>,
"kan.liang@...ux.intel.com" <kan.liang@...ux.intel.com>, "kees@...nel.org"
<kees@...nel.org>, "mark.rutland@....com" <mark.rutland@....com>,
"peterz@...radead.org" <peterz@...radead.org>, "Dhananjay.Ugwekar@....com"
<Dhananjay.Ugwekar@....com>, "bp@...en8.de" <bp@...en8.de>, "acme@...nel.org"
<acme@...nel.org>, "oleksandr@...alenko.name" <oleksandr@...alenko.name>,
"jolsa@...nel.org" <jolsa@...nel.org>, "x86@...nel.org" <x86@...nel.org>,
"namhyung@...nel.org" <namhyung@...nel.org>
CC: "ravi.bangoria@....com" <ravi.bangoria@....com>, "kprateek.nayak@....com"
<kprateek.nayak@....com>, "gautham.shenoy@....com" <gautham.shenoy@....com>,
"linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>,
"sandipan.das@....com" <sandipan.das@....com>, "ananth.narayan@....com"
<ananth.narayan@....com>, "linux-pm@...r.kernel.org"
<linux-pm@...r.kernel.org>
Subject: Re: [PATCH v4 10/11] perf/x86/rapl: Add per-core energy counter
support for AMD CPUs
>
> @@ -352,9 +384,13 @@ static int rapl_pmu_event_init(struct perf_event
> *event)
> u64 cfg = event->attr.config & RAPL_EVENT_MASK;
> int bit, ret = 0;
> struct rapl_pmu *rapl_pmu;
> + struct rapl_pmus *curr_rapl_pmus;
>
> /* only look at RAPL events */
> - if (event->attr.type != rapl_pmus_pkg->pmu.type)
> + if (event->attr.type == rapl_pmus_pkg->pmu.type ||
> + (rapl_pmus_core && event->attr.type ==
> rapl_pmus_core->pmu.type))
> + curr_rapl_pmus = container_of(event->pmu, struct
> rapl_pmus, pmu);
> + else
> return -ENOENT;
>
> /* check only supported bits are set */
> @@ -364,7 +400,8 @@ static int rapl_pmu_event_init(struct perf_event
> *event)
> if (event->cpu < 0)
> return -EINVAL;
>
> - event->event_caps |= PERF_EV_CAP_READ_ACTIVE_PKG;
> + if (curr_rapl_pmus == rapl_pmus_pkg)
> + event->event_caps |= PERF_EV_CAP_READ_ACTIVE_PKG;
>
> if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
> return -EINVAL;
this sanity check becomes bogus for per_core event.
> @@ -373,7 +410,8 @@ static int rapl_pmu_event_init(struct perf_event
> *event)
> bit = cfg - 1;
>
> /* check event supported */
> - if (!(rapl_pkg_cntr_mask & (1 << bit)))
> + if (!(rapl_pkg_cntr_mask & (1 << bit)) &&
> + !(rapl_core_cntr_mask & (1 << bit)))
> return -EINVAL;
what if bit > 1 for a per_core event?
>
> /* unsupported modes and filters */
> @@ -381,12 +419,18 @@ static int rapl_pmu_event_init(struct
> perf_event *event)
> return -EINVAL;
>
> /* must be done before validate_group */
> - rapl_pmu = cpu_to_rapl_pmu(event->cpu);
> + if (curr_rapl_pmus == rapl_pmus_core) {
> + rapl_pmu = curr_rapl_pmus-
> >rapl_pmu[topology_logical_core_id(event->cpu)];
> + event->hw.event_base = rapl_model-
> >rapl_core_msrs[bit].msr;
> + } else {
> + rapl_pmu = curr_rapl_pmus-
> >rapl_pmu[get_rapl_pmu_idx(event->cpu)];
> + event->hw.event_base = rapl_model-
> >rapl_pkg_msrs[bit].msr;
> + }
> +
To avoid the above issues and check for (curr_rapl_pmus ==
rapl_pmus_core) all over the places, I'd suggest we do the
per_core/per_pkg sanity checks and handlings altogether, say something
like
if (event->attr.type == rapl_pmus_pkg->pmu.type) {
all sanity checks
rapl_pmu = ...
event->hw.event_base = ...
} else if (rapl_pmus_core && event->attr.type ==
rapl_pmus_core->pmu.type) {
all sanity checks
rapl_pmu = ...
event->hw.event_base = ...
} else {
return --ENOENT;
}
[...]
> static int rapl_cpu_offline(unsigned int cpu)
> {
> - return __rapl_cpu_offline(rapl_pmus_pkg,
> get_rapl_pmu_idx(cpu),
> + int ret = __rapl_cpu_offline(rapl_pmus_pkg,
> get_rapl_pmu_idx(cpu),
> get_rapl_pmu_cpumask(cpu), cpu);
extra space after '='?
[...]
> +
> + if (ret == 0 && rapl_model->core_events)
> + ret = __rapl_cpu_offline(rapl_pmus_core,
> topology_logical_core_id(cpu),
> + topology_sibling_cpumask(cpu),
> cpu);
> +
> + return ret;
> }
>
> static int __rapl_cpu_online(struct rapl_pmus *rapl_pmus, unsigned
> int rapl_pmu_idx,
> @@ -629,8 +725,14 @@ static int __rapl_cpu_online(struct rapl_pmus
> *rapl_pmus, unsigned int rapl_pmu_
>
> static int rapl_cpu_online(unsigned int cpu)
> {
> - return __rapl_cpu_online(rapl_pmus_pkg,
> get_rapl_pmu_idx(cpu),
> + int ret = __rapl_cpu_online(rapl_pmus_pkg,
> get_rapl_pmu_idx(cpu),
> get_rapl_pmu_cpumask(cpu), cpu);
extra space after '='?
[...]
> +
> + if (rapl_core_cntr_mask & (1 << PERF_RAPL_PER_CORE))
> + pr_info("hw unit of domain %s 2^-%d Joules\n",
> + rapl_core_domain_names[PERF_RAPL_PER_CORE],
> rapl_core_hw_unit);
> }
Are we expecting to have more than one Domain for per_core power PMU?
if no, we don't need introduce
+enum perf_rapl_core_events {
+ PERF_RAPL_PER_CORE = 0, /* per-core */
+
+ PERF_RAPL_CORE_EVENTS_MAX,
+ NR_RAPL_CORE_DOMAINS = PERF_RAPL_CORE_EVENTS_MAX,
+};
+
and check for NR_RAPL_CORE_DOMAINS all over the place.
Or else, we should use a loop here to advertise all possible per_core
domains. Either is okay with me but the code needs to be consistent.
>
> static void cleanup_rapl_pmus(struct rapl_pmus *rapl_pmus)
> @@ -712,14 +820,16 @@ static const struct attribute_group
> *rapl_attr_update[] = {
> NULL,
> };
>
> -static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr)
> +static const struct attribute_group *rapl_per_core_attr_update[] = {
> + &rapl_events_per_core_group,
> +};
> +
> +static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr,
> int nr_rapl_pmu,
> + const struct attribute_group
> **rapl_attr_groups,
> + const struct attribute_group
> **rapl_attr_update)
> {
> - int nr_rapl_pmu = topology_max_packages();
> struct rapl_pmus *rapl_pmus;
>
> - if (!rapl_pmu_is_pkg_scope())
> - nr_rapl_pmu *= topology_max_dies_per_package();
> -
> rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu,
> nr_rapl_pmu), GFP_KERNEL);
> if (!rapl_pmus)
> return -ENOMEM;
> @@ -809,8 +919,10 @@ static struct rapl_model model_spr = {
>
> static struct rapl_model model_amd_hygon = {
> .pkg_events = BIT(PERF_RAPL_PKG),
> + .core_events = BIT(PERF_RAPL_PER_CORE),
> .msr_power_unit = MSR_AMD_RAPL_POWER_UNIT,
> .rapl_pkg_msrs = amd_rapl_pkg_msrs,
> + .rapl_core_msrs = amd_rapl_core_msrs,
> };
>
> static const struct x86_cpu_id rapl_model_match[] __initconst = {
> @@ -867,6 +979,11 @@ static int __init rapl_pmu_init(void)
> {
> const struct x86_cpu_id *id;
> int ret;
> + int nr_rapl_pmu = topology_max_packages() *
> topology_max_dies_per_package();
> + int nr_cores = topology_max_packages() *
> topology_num_cores_per_package();
> +
I thought we agreed to use one variable for all three cases.
thanks,
rui
Powered by blists - more mailing lists