| 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: <a0397a78-1e75-4bb3-bdc7-d28791c448d4@amd.com>
Date: Tue, 11 Jun 2024 14:03:20 +0530
From: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
To: "Zhang, Rui" <rui.zhang@...el.com>,
"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>, "bp@...en8.de"
<bp@...en8.de>, "acme@...nel.org" <acme@...nel.org>,
"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 2/6] perf/x86/rapl: Rename rapl_pmu variables
Hello Rui,
On 6/11/2024 11:13 AM, Zhang, Rui wrote:
> On Mon, 2024-06-10 at 10:07 +0000, Dhananjay Ugwekar wrote:
>> Rename struct rapl_pmu variables from "pmu" to "rapl_pmu", to
>> avoid any confusion between the variables of two different
>> structs pmu and rapl_pmu.
>> As rapl_pmu also contains a pointer to
>> struct pmu, which leads to situations in code like pmu->pmu,
>> which is needlessly confusing. Above scenario is replaced with
>> much more readable rapl_pmu->pmu with this change.
>>
>> Also rename "pmus" member in rapl_pmus struct, for same reason.
>>
>
> As you are adding a new per_core pmu, can we just rename the current
> rapl_pmu to something like rapl_pkg_pmus (as I mentioned in the
> previous email, we can consider the current RAPL MSRs as package scope
> on Intel platforms as well), and name the new one as rapl_core_pmus?
Sure this makes sense, will modify the original "rapl_pmus" variable name,
but please note the renaming that I refer to in this patch is only
limited to the local "struct rapl_pmu" variables used inside functions,
not the global variable name you mention.
Regards,
Dhananjay
>
> IMO, "rapl_pmus" + "rapl_pmus_per_core" is still confusing.
>
> thanks,
> rui
>
>> No functional change.
>>
>> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@....com>
>> ---
>> arch/x86/events/rapl.c | 104 ++++++++++++++++++++-------------------
>> --
>> 1 file changed, 52 insertions(+), 52 deletions(-)
>>
>> diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
>> index 73be25e1f4b4..b4e2073a178e 100644
>> --- a/arch/x86/events/rapl.c
>> +++ b/arch/x86/events/rapl.c
>> @@ -120,7 +120,7 @@ struct rapl_pmu {
>> struct rapl_pmus {
>> struct pmu pmu;
>> unsigned int nr_rapl_pmu;
>> - struct rapl_pmu *pmus[] __counted_by(nr_rapl_pmu);
>> + struct rapl_pmu *rapl_pmu[]
>> __counted_by(nr_rapl_pmu);
>> };
>>
>> enum rapl_unit_quirk {
>> @@ -164,7 +164,7 @@ static inline struct rapl_pmu
>> *cpu_to_rapl_pmu(unsigned int cpu)
>> * The unsigned check also catches the '-1' return value for
>> non
>> * existent mappings in the topology map.
>> */
>> - return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ? rapl_pmus-
>>> pmus[rapl_pmu_idx] : NULL;
>> + return rapl_pmu_idx < rapl_pmus->nr_rapl_pmu ? rapl_pmus-
>>> rapl_pmu[rapl_pmu_idx] : NULL;
>> }
>>
>> static inline u64 rapl_read_counter(struct perf_event *event)
>> @@ -228,34 +228,34 @@ static void rapl_start_hrtimer(struct rapl_pmu
>> *pmu)
>>
>> static enum hrtimer_restart rapl_hrtimer_handle(struct hrtimer
>> *hrtimer)
>> {
>> - struct rapl_pmu *pmu = container_of(hrtimer, struct rapl_pmu,
>> hrtimer);
>> + struct rapl_pmu *rapl_pmu = container_of(hrtimer, struct
>> rapl_pmu, hrtimer);
>> struct perf_event *event;
>> unsigned long flags;
>>
>> - if (!pmu->n_active)
>> + if (!rapl_pmu->n_active)
>> return HRTIMER_NORESTART;
>>
>> - raw_spin_lock_irqsave(&pmu->lock, flags);
>> + raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
>>
>> - list_for_each_entry(event, &pmu->active_list, active_entry)
>> + list_for_each_entry(event, &rapl_pmu->active_list,
>> active_entry)
>> rapl_event_update(event);
>>
>> - raw_spin_unlock_irqrestore(&pmu->lock, flags);
>> + raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
>>
>> - hrtimer_forward_now(hrtimer, pmu->timer_interval);
>> + hrtimer_forward_now(hrtimer, rapl_pmu->timer_interval);
>>
>> return HRTIMER_RESTART;
>> }
>>
>> -static void rapl_hrtimer_init(struct rapl_pmu *pmu)
>> +static void rapl_hrtimer_init(struct rapl_pmu *rapl_pmu)
>> {
>> - struct hrtimer *hr = &pmu->hrtimer;
>> + struct hrtimer *hr = &rapl_pmu->hrtimer;
>>
>> hrtimer_init(hr, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>> hr->function = rapl_hrtimer_handle;
>> }
>>
>> -static void __rapl_pmu_event_start(struct rapl_pmu *pmu,
>> +static void __rapl_pmu_event_start(struct rapl_pmu *rapl_pmu,
>> struct perf_event *event)
>> {
>> if (WARN_ON_ONCE(!(event->hw.state & PERF_HES_STOPPED)))
>> @@ -263,39 +263,39 @@ static void __rapl_pmu_event_start(struct
>> rapl_pmu *pmu,
>>
>> event->hw.state = 0;
>>
>> - list_add_tail(&event->active_entry, &pmu->active_list);
>> + list_add_tail(&event->active_entry, &rapl_pmu->active_list);
>>
>> local64_set(&event->hw.prev_count, rapl_read_counter(event));
>>
>> - pmu->n_active++;
>> - if (pmu->n_active == 1)
>> - rapl_start_hrtimer(pmu);
>> + rapl_pmu->n_active++;
>> + if (rapl_pmu->n_active == 1)
>> + rapl_start_hrtimer(rapl_pmu);
>> }
>>
>> static void rapl_pmu_event_start(struct perf_event *event, int mode)
>> {
>> - struct rapl_pmu *pmu = event->pmu_private;
>> + struct rapl_pmu *rapl_pmu = event->pmu_private;
>> unsigned long flags;
>>
>> - raw_spin_lock_irqsave(&pmu->lock, flags);
>> - __rapl_pmu_event_start(pmu, event);
>> - raw_spin_unlock_irqrestore(&pmu->lock, flags);
>> + raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
>> + __rapl_pmu_event_start(rapl_pmu, event);
>> + raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
>> }
>>
>> static void rapl_pmu_event_stop(struct perf_event *event, int mode)
>> {
>> - struct rapl_pmu *pmu = event->pmu_private;
>> + struct rapl_pmu *rapl_pmu = event->pmu_private;
>> struct hw_perf_event *hwc = &event->hw;
>> unsigned long flags;
>>
>> - raw_spin_lock_irqsave(&pmu->lock, flags);
>> + raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
>>
>> /* mark event as deactivated and stopped */
>> if (!(hwc->state & PERF_HES_STOPPED)) {
>> - WARN_ON_ONCE(pmu->n_active <= 0);
>> - pmu->n_active--;
>> - if (pmu->n_active == 0)
>> - hrtimer_cancel(&pmu->hrtimer);
>> + WARN_ON_ONCE(rapl_pmu->n_active <= 0);
>> + rapl_pmu->n_active--;
>> + if (rapl_pmu->n_active == 0)
>> + hrtimer_cancel(&rapl_pmu->hrtimer);
>>
>> list_del(&event->active_entry);
>>
>> @@ -313,23 +313,23 @@ static void rapl_pmu_event_stop(struct
>> perf_event *event, int mode)
>> hwc->state |= PERF_HES_UPTODATE;
>> }
>>
>> - raw_spin_unlock_irqrestore(&pmu->lock, flags);
>> + raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
>> }
>>
>> static int rapl_pmu_event_add(struct perf_event *event, int mode)
>> {
>> - struct rapl_pmu *pmu = event->pmu_private;
>> + struct rapl_pmu *rapl_pmu = event->pmu_private;
>> struct hw_perf_event *hwc = &event->hw;
>> unsigned long flags;
>>
>> - raw_spin_lock_irqsave(&pmu->lock, flags);
>> + raw_spin_lock_irqsave(&rapl_pmu->lock, flags);
>>
>> hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
>>
>> if (mode & PERF_EF_START)
>> - __rapl_pmu_event_start(pmu, event);
>> + __rapl_pmu_event_start(rapl_pmu, event);
>>
>> - raw_spin_unlock_irqrestore(&pmu->lock, flags);
>> + raw_spin_unlock_irqrestore(&rapl_pmu->lock, flags);
>>
>> return 0;
>> }
>> @@ -343,7 +343,7 @@ 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 *pmu;
>> + struct rapl_pmu *rapl_pmu;
>>
>> /* only look at RAPL events */
>> if (event->attr.type != rapl_pmus->pmu.type)
>> @@ -373,11 +373,11 @@ static int rapl_pmu_event_init(struct
>> perf_event *event)
>> return -EINVAL;
>>
>> /* must be done before validate_group */
>> - pmu = cpu_to_rapl_pmu(event->cpu);
>> - if (!pmu)
>> + rapl_pmu = cpu_to_rapl_pmu(event->cpu);
>> + if (!rapl_pmu)
>> return -EINVAL;
>> - event->cpu = pmu->cpu;
>> - event->pmu_private = pmu;
>> + event->cpu = rapl_pmu->cpu;
>> + event->pmu_private = rapl_pmu;
>> event->hw.event_base = rapl_msrs[bit].msr;
>> event->hw.config = cfg;
>> event->hw.idx = bit;
>> @@ -560,22 +560,22 @@ static struct perf_msr amd_rapl_msrs[] = {
>> static int rapl_cpu_offline(unsigned int cpu)
>> {
>> const struct cpumask *rapl_pmu_cpumask =
>> get_rapl_pmu_cpumask(cpu);
>> - struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
>> + struct rapl_pmu *rapl_pmu = cpu_to_rapl_pmu(cpu);
>> int target;
>>
>> /* Check if exiting cpu is used for collecting rapl events */
>> if (!cpumask_test_and_clear_cpu(cpu, &rapl_cpu_mask))
>> return 0;
>>
>> - pmu->cpu = -1;
>> + rapl_pmu->cpu = -1;
>> /* Find a new cpu to collect rapl events */
>> target = cpumask_any_but(rapl_pmu_cpumask, cpu);
>>
>> /* Migrate rapl events to the new target */
>> if (target < nr_cpu_ids) {
>> cpumask_set_cpu(target, &rapl_cpu_mask);
>> - pmu->cpu = target;
>> - perf_pmu_migrate_context(pmu->pmu, cpu, target);
>> + rapl_pmu->cpu = target;
>> + perf_pmu_migrate_context(rapl_pmu->pmu, cpu, target);
>> }
>> return 0;
>> }
>> @@ -584,21 +584,21 @@ static int rapl_cpu_online(unsigned int cpu)
>> {
>> unsigned int rapl_pmu_idx = get_rapl_pmu_idx(cpu);
>> const struct cpumask *rapl_pmu_cpumask =
>> get_rapl_pmu_cpumask(cpu);
>> - struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
>> + struct rapl_pmu *rapl_pmu = cpu_to_rapl_pmu(cpu);
>> int target;
>>
>> - if (!pmu) {
>> - pmu = kzalloc_node(sizeof(*pmu), GFP_KERNEL,
>> cpu_to_node(cpu));
>> - if (!pmu)
>> + if (!rapl_pmu) {
>> + rapl_pmu = kzalloc_node(sizeof(*rapl_pmu),
>> GFP_KERNEL, cpu_to_node(cpu));
>> + if (!rapl_pmu)
>> return -ENOMEM;
>>
>> - raw_spin_lock_init(&pmu->lock);
>> - INIT_LIST_HEAD(&pmu->active_list);
>> - pmu->pmu = &rapl_pmus->pmu;
>> - pmu->timer_interval = ms_to_ktime(rapl_timer_ms);
>> - rapl_hrtimer_init(pmu);
>> + raw_spin_lock_init(&rapl_pmu->lock);
>> + INIT_LIST_HEAD(&rapl_pmu->active_list);
>> + rapl_pmu->pmu = &rapl_pmus->pmu;
>> + rapl_pmu->timer_interval =
>> ms_to_ktime(rapl_timer_ms);
>> + rapl_hrtimer_init(rapl_pmu);
>>
>> - rapl_pmus->pmus[rapl_pmu_idx] = pmu;
>> + rapl_pmus->rapl_pmu[rapl_pmu_idx] = rapl_pmu;
>> }
>>
>> /*
>> @@ -610,7 +610,7 @@ static int rapl_cpu_online(unsigned int cpu)
>> return 0;
>>
>> cpumask_set_cpu(cpu, &rapl_cpu_mask);
>> - pmu->cpu = cpu;
>> + rapl_pmu->cpu = cpu;
>> return 0;
>> }
>>
>> @@ -679,7 +679,7 @@ static void cleanup_rapl_pmus(void)
>> int i;
>>
>> for (i = 0; i < rapl_pmus->nr_rapl_pmu; i++)
>> - kfree(rapl_pmus->pmus[i]);
>> + kfree(rapl_pmus->rapl_pmu[i]);
>> kfree(rapl_pmus);
>> }
>>
>> @@ -699,7 +699,7 @@ static int __init init_rapl_pmus(void)
>> if (rapl_pmu_is_pkg_scope())
>> nr_rapl_pmu = topology_max_packages();
>>
>> - rapl_pmus = kzalloc(struct_size(rapl_pmus, pmus,
>> nr_rapl_pmu), GFP_KERNEL);
>> + rapl_pmus = kzalloc(struct_size(rapl_pmus, rapl_pmu,
>> nr_rapl_pmu), GFP_KERNEL);
>> if (!rapl_pmus)
>> return -ENOMEM;
>>
>
Powered by blists - more mailing lists