| 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: <e43c36d3-4cf4-431b-bd6b-3e7f072179c4@amd.com>
Date: Mon, 7 Oct 2024 14:08:13 +0530
From: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
To: "Gautham R. Shenoy" <gautham.shenoy@....com>
Cc: peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
namhyung@...nel.org, mark.rutland@....com,
alexander.shishkin@...ux.intel.com, jolsa@...nel.org, irogers@...gle.com,
adrian.hunter@...el.com, kan.liang@...ux.intel.com, tglx@...utronix.de,
bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
rui.zhang@...el.com, oleksandr@...alenko.name, eranian@...gle.com,
ravi.bangoria@....com, linux-perf-users@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 9/9] perf/x86/rapl: Add per-core energy counter support
for AMD CPUs
Hello Gautham,
On 10/7/2024 12:11 PM, Gautham R. Shenoy wrote:
> On Fri, Sep 13, 2024 at 03:48:01PM +0000, Dhananjay Ugwekar wrote:
>> Add a new "power_per_core" PMU and "energy-per-core" event for
>> monitoring energy consumption by each core. The existing energy-cores
>> event aggregates the energy consumption at the package level.
>> This new event aligns with the AMD's per_core energy counters.
[Snip]
>>
>> @@ -156,10 +170,14 @@ static struct rapl_model *rapl_model;
>> * Helper function to get the correct topology id according to the
>> * RAPL PMU scope.
>> */
>> -static inline unsigned int get_rapl_pmu_idx(int cpu)
>> +static inline unsigned int get_rapl_pmu_idx(int cpu, int scope)
>> {
>> - return rapl_pkg_pmu_is_pkg_scope() ? topology_logical_package_id(cpu) :
>> - topology_logical_die_id(cpu);
>> + if (scope == PERF_PMU_SCOPE_PKG)
>> + return topology_logical_package_id(cpu);
>> + else if (scope == PERF_PMU_SCOPE_DIE)
>
> You don't need the "else if" since you are returning if there is a
> match for the earlier if condition.
Right, will modify accordingly
>
>> + return topology_logical_die_id(cpu);
>> + else
> ^^^^^
> Please check if the scope is SCOPE_CORE here. Again, you don't need the
> else condition.
Yes, will add the if check and remove the else
>
>
>> + return topology_logical_core_id(cpu);
>
>
>
>> }
>>
>> static inline u64 rapl_read_counter(struct perf_event *event)
[Snip]
>> @@ -337,12 +356,13 @@ static void rapl_pmu_event_del(struct perf_event *event, int flags)
>> static int rapl_pmu_event_init(struct perf_event *event)
>> {
>> u64 cfg = event->attr.config & RAPL_EVENT_MASK;
>> - int bit, rapl_pmu_idx, ret = 0;
>> + int bit, rapl_pmus_scope, rapl_pmu_idx, ret = 0;
>> struct rapl_pmu *rapl_pmu;
>> + struct rapl_pmus *rapl_pmus;
>>
>> - /* only look at RAPL events */
>> - if (event->attr.type != rapl_pmus_pkg->pmu.type)
>> - return -ENOENT;
>
> Don't we need the check to only look at RAPL events of pkg or core ?
> Or is that covered by a check below ?
I moved this check to the PMU specific if blocks (highlighted below)
>
>
>
>> + /* unsupported modes and filters */
>> + if (event->attr.sample_period) /* no sampling */
>> + return -EINVAL;
>>
>> /* check only supported bits are set */
>> if (event->attr.config & ~RAPL_EVENT_MASK)
>> @@ -351,31 +371,49 @@ static int rapl_pmu_event_init(struct perf_event *event)
>> if (event->cpu < 0)
>> return -EINVAL;
>>
>> - if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
>> + rapl_pmus = container_of(event->pmu, struct rapl_pmus, pmu);
>> + if (!rapl_pmus)
>> return -EINVAL;
>> -
>> - cfg = array_index_nospec((long)cfg, NR_RAPL_PKG_DOMAINS + 1);
>> - bit = cfg - 1;
>> -
>> - /* check event supported */
>> - if (!(rapl_pmus_pkg->cntr_mask & (1 << bit)))
>> + rapl_pmus_scope = rapl_pmus->pmu.scope;
>> +
>> + if (rapl_pmus_scope == PERF_PMU_SCOPE_PKG || rapl_pmus_scope == PERF_PMU_SCOPE_DIE) {
>> + /* only look at RAPL package events */
>> + if (event->attr.type != rapl_pmus_pkg->pmu.type)
>> + return -ENOENT; ^^^^ here and
>> +
>> + cfg = array_index_nospec((long)cfg, NR_RAPL_PKG_DOMAINS + 1);
>> + if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
>> + return -EINVAL;
>> +
>> + bit = cfg - 1;
>> + event->hw.event_base = rapl_model->rapl_pkg_msrs[bit].msr;
>> + } else if (rapl_pmus_scope == PERF_PMU_SCOPE_CORE) {
>> + /* only look at RAPL per-core events */
>> + if (event->attr.type != rapl_pmus_core->pmu.type)
>> + return -ENOENT;
^^^^ here
>> +
>> + cfg = array_index_nospec((long)cfg, NR_RAPL_CORE_DOMAINS + 1);
>> + if (!cfg || cfg >= NR_RAPL_PKG_DOMAINS + 1)
>> + return -EINVAL;
>> +
>> + bit = cfg - 1;
>> + event->hw.event_base = rapl_model->rapl_core_msrs[bit].msr;
>> + } else
>> return -EINVAL;
>>
>> - /* unsupported modes and filters */
>> - if (event->attr.sample_period) /* no sampling */
>> + /* check event supported */
>> + if (!(rapl_pmus->cntr_mask & (1 << bit)))
>> return -EINVAL;
>>
>> - rapl_pmu_idx = get_rapl_pmu_idx(event->cpu);
>> - if (rapl_pmu_idx >= rapl_pmus_pkg->nr_rapl_pmu)
>> + rapl_pmu_idx = get_rapl_pmu_idx(event->cpu, rapl_pmus_scope);
>> + if (rapl_pmu_idx >= rapl_pmus->nr_rapl_pmu)
>> return -EINVAL;
>> -
>> /* must be done before validate_group */
>> - rapl_pmu = rapl_pmus_pkg->rapl_pmu[rapl_pmu_idx];
>> + rapl_pmu = rapl_pmus->rapl_pmu[rapl_pmu_idx];
>> if (!rapl_pmu)
>> return -EINVAL;
>>
>> event->pmu_private = rapl_pmu;
>> - event->hw.event_base = rapl_model->rapl_pkg_msrs[bit].msr;
>> event->hw.config = cfg;
>> event->hw.idx = bit;
>>
[Snip]
>> @@ -644,15 +720,19 @@ static void __init init_rapl_pmu(struct rapl_pmus *rapl_pmus)
>> cpus_read_unlock();
>> }
>>
>> -static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr, int rapl_pmu_scope)
>> +static int __init init_rapl_pmus(struct rapl_pmus **rapl_pmus_ptr, int rapl_pmu_scope,
>> + const struct attribute_group **rapl_attr_groups,
>> + const struct attribute_group **rapl_attr_update)
>> {
>> int nr_rapl_pmu;
>> struct rapl_pmus *rapl_pmus;
>>
>> if (rapl_pmu_scope == PERF_PMU_SCOPE_PKG)
>> nr_rapl_pmu = topology_max_packages();
>> - else
>> + else if (rapl_pmu_scope == PERF_PMU_SCOPE_DIE)
>> nr_rapl_pmu = topology_max_packages() * topology_max_dies_per_package();
>> + else
>> + nr_rapl_pmu = topology_max_packages() * topology_num_cores_per_package();
>
> Here please check if the rapl_pmu_scope is PERF_PMU_SCOPE_CORE instead
> of assuming it to be the case if the scope is neither SCOPE_PKG nor
> SCOPE_DIE. If it is neither of these three, then return an error.
Actually, I thought there is only one caller of this function (rapl_pmu_init),
which only passes one of these 3 values, so I thought maybe the error check isnt
needed. What do you think?
Thanks,
Dhananjay
>
>
> --
> Thanks and Regards
> gautham.
>
>>
>> rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu, nr_rapl_pmu), GFP_KERNEL);
>> if (!rapl_pmus)
Powered by blists - more mailing lists