| 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: <6b96076c-a8f2-2d55-d977-a5e91c2b0cba@linux.alibaba.com>
Date: Tue, 3 May 2022 15:59:26 +0800
From: Xin Hao <xhao@...ux.alibaba.com>
To: James Morse <james.morse@....com>, x86@...nel.org,
linux-kernel@...r.kernel.org
Cc: Fenghua Yu <fenghua.yu@...el.com>,
Reinette Chatre <reinette.chatre@...el.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
H Peter Anvin <hpa@...or.com>,
Babu Moger <Babu.Moger@....com>,
shameerali.kolothum.thodi@...wei.com,
D Scott Phillips OS <scott@...amperecomputing.com>,
lcherian@...vell.com, bobo.shaobowang@...wei.com,
tan.shaopeng@...itsu.com, Jamie Iles <quic_jiles@...cinc.com>,
Cristian Marussi <cristian.marussi@....com>,
xingxin.hx@...nanolis.org, baolin.wang@...ux.alibaba.com
Subject: Re: [PATCH v4 03/21] x86/resctrl: Add domain online callback for
resctrl work
On 4/17/22 12:06 AM, Xin Hao wrote:
>
>
> On 4/12/22 8:44 PM, James Morse wrote:
>> Because domains are exposed to user-space via resctrl, the filesystem
>> must update its state when CPU hotplug callbacks are triggered.
>>
>> Some of this work is common to any architecture that would support
>> resctrl, but the work is tied up with the architecture code to
>> allocate the memory.
>>
>> Move domain_setup_mon_state(), the monitor subdir creation call and the
>> mbm/limbo workers into a new resctrl_online_domain() call. These bits
>> are not specific to the architecture. Grouping them in one function
>> allows that code to be moved to /fs/ and re-used by another architecture.
>>
>> Reviewed-by: Jamie Iles<quic_jiles@...cinc.com>
>> Tested-by: Xin Hao<xhao@...ux.alibaba.com>
>> Reviewed-by: Shaopeng Tan<tan.shaopeng@...itsu.com>
>> Tested-by: Shaopeng Tan<tan.shaopeng@...itsu.com>
>> Tested-by: Cristian Marussi<cristian.marussi@....com>
>> Signed-off-by: James Morse<james.morse@....com>
>> ---
>> Changes since v2:
>> * Fixed kfree(d) rebase artefact.
>>
>> Changes since v1:
>> * Capitalisation
>> * Removed inline comment
>> * Added to the commit message
>> ---
>> arch/x86/kernel/cpu/resctrl/core.c | 57 ++++------------------
>> arch/x86/kernel/cpu/resctrl/internal.h | 2 -
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 65 ++++++++++++++++++++++++--
>> include/linux/resctrl.h | 1 +
>> 4 files changed, 69 insertions(+), 56 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index 2f87177f1f69..25f30148478b 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -443,42 +443,6 @@ static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d)
>> return 0;
>> }
>>
>> -static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d)
>> -{
>> - size_t tsize;
>> -
>> - if (is_llc_occupancy_enabled()) {
>> - d->rmid_busy_llc = bitmap_zalloc(r->num_rmid, GFP_KERNEL);
>> - if (!d->rmid_busy_llc)
>> - return -ENOMEM;
>> - INIT_DELAYED_WORK(&d->cqm_limbo, cqm_handle_limbo);
>> - }
>> - if (is_mbm_total_enabled()) {
>> - tsize = sizeof(*d->mbm_total);
>> - d->mbm_total = kcalloc(r->num_rmid, tsize, GFP_KERNEL);
>> - if (!d->mbm_total) {
>> - bitmap_free(d->rmid_busy_llc);
>> - return -ENOMEM;
>> - }
>> - }
>> - if (is_mbm_local_enabled()) {
>> - tsize = sizeof(*d->mbm_local);
>> - d->mbm_local = kcalloc(r->num_rmid, tsize, GFP_KERNEL);
>> - if (!d->mbm_local) {
>> - bitmap_free(d->rmid_busy_llc);
>> - kfree(d->mbm_total);
>> - return -ENOMEM;
>> - }
>> - }
>> -
>> - if (is_mbm_enabled()) {
>> - INIT_DELAYED_WORK(&d->mbm_over, mbm_handle_overflow);
>> - mbm_setup_overflow_handler(d, MBM_OVERFLOW_INTERVAL);
>> - }
>> -
>> - return 0;
>> -}
>> -
>> /*
>> * domain_add_cpu - Add a cpu to a resource's domain list.
>> *
>> @@ -498,6 +462,7 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
>> struct list_head *add_pos = NULL;
>> struct rdt_hw_domain *hw_dom;
>> struct rdt_domain *d;
>> + int err;
>>
>> d = rdt_find_domain(r, id, &add_pos);
>> if (IS_ERR(d)) {
>> @@ -527,21 +492,15 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
>> return;
>> }
>>
>> - if (r->mon_capable && domain_setup_mon_state(r, d)) {
>> - kfree(hw_dom->ctrl_val);
>> - kfree(hw_dom->mbps_val);
>> - kfree(hw_dom);
>> - return;
>> - }
>> -
>> list_add_tail(&d->list, add_pos);
>>
>> - /*
>> - * If resctrl is mounted, add
>> - * per domain monitor data directories.
>> - */
>> - if (static_branch_unlikely(&rdt_mon_enable_key))
>> - mkdir_mondata_subdir_allrdtgrp(r, d);
>> + err = resctrl_online_domain(r, d);
>> + if (err) {
>> + list_del(&d->list);
>> + kfree(hw_dom->ctrl_val);
>> + kfree(hw_dom->mbps_val);
>> + kfree(hw_dom);
>> + }
>> }
>>
>> static void domain_remove_cpu(int cpu, struct rdt_resource *r)
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 8828b5c1b6d2..be48a682dbdb 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -524,8 +524,6 @@ void mon_event_count(void *info);
>> int rdtgroup_mondata_show(struct seq_file *m, void *arg);
>> void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
>> unsigned int dom_id);
>> -void mkdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
>> - struct rdt_domain *d);
>> void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>> struct rdt_domain *d, struct rdtgroup *rdtgrp,
>> int evtid, int first);
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 651ff5288e85..53bdc07f9dac 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2565,16 +2565,13 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
>> * Add all subdirectories of mon_data for "ctrl_mon" groups
>> * and "monitor" groups with given domain id.
>> */
>> -void mkdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
>> - struct rdt_domain *d)
>> +static void mkdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
>> + struct rdt_domain *d)
>> {
>> struct kernfs_node *parent_kn;
>> struct rdtgroup *prgrp, *crgrp;
>> struct list_head *head;
>>
>> - if (!r->mon_capable)
>> - return;
>> -
>> list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
>> parent_kn = prgrp->mon.mon_data_kn;
>> mkdir_mondata_subdir(parent_kn, d, r, prgrp);
>> @@ -3236,6 +3233,64 @@ static int __init rdtgroup_setup_root(void)
>> return ret;
>> }
>>
>> +static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d)
>> +{
>> + size_t tsize;
>> +
>> + if (is_llc_occupancy_enabled()) {
>> + d->rmid_busy_llc = bitmap_zalloc(r->num_rmid, GFP_KERNEL);
>> + if (!d->rmid_busy_llc)
>> + return -ENOMEM;
>> + }
>> + if (is_mbm_total_enabled()) {
>> + tsize = sizeof(*d->mbm_total);
>> + d->mbm_total = kcalloc(r->num_rmid, tsize, GFP_KERNEL);
>> + if (!d->mbm_total) {
>> + bitmap_free(d->rmid_busy_llc);
>> + return -ENOMEM;
>> + }
>> + }
>> + if (is_mbm_local_enabled()) {
>> + tsize = sizeof(*d->mbm_local);
>> + d->mbm_local = kcalloc(r->num_rmid, tsize, GFP_KERNEL);
>> + if (!d->mbm_local) {
>> + bitmap_free(d->rmid_busy_llc);
>> + kfree(d->mbm_total);
>> + return -ENOMEM;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
>> +{
>> + int err;
>> +
>> + lockdep_assert_held(&rdtgroup_mutex);
>> +
>> + if (!r->mon_capable)
>> + return 0;
>
> This return value "0" may not quite correct, in code where it is
> called, if err = 0, i think if mon_capable is not supported, the
> hw_dom->mbps_val also need to be released? if I get this wrong，please
> let me know.
>
> + err = resctrl_online_domain(r, d);
> + if (err) {
> + list_del(&d->list);
> + kfree(hw_dom->ctrl_val);
> + kfree(hw_dom->mbps_val);
> + kfree(hw_dom);
> + }
>> +
>> + err = domain_setup_mon_state(r, d);
>> + if (err)
>> + return err;
>> +
>> + if (is_mbm_enabled()) {
>> + INIT_DELAYED_WORK(&d->mbm_over, mbm_handle_overflow);
>> + mbm_setup_overflow_handler(d, MBM_OVERFLOW_INTERVAL);
>> + }
>> +
>> + if (is_llc_occupancy_enabled())
>> + INIT_DELAYED_WORK(&d->cqm_limbo, cqm_handle_limbo);
>> +
>> + /* If resctrl is mounted, add per domain monitor data directories. */
>> + if (static_branch_unlikely(&rdt_mon_enable_key))
>> + mkdir_mondata_subdir_allrdtgrp(r, d);
>> +
>> + return 0;
>> +}
>> +
>> /*
>> * rdtgroup_init - rdtgroup initialization
>> *
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index 8180c539800d..d512455b4c3a 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -192,5 +192,6 @@ u32 resctrl_arch_get_num_closid(struct rdt_resource *r);
>> int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);
>> u32 resctrl_arch_get_config(struct rdt_resource *r, struct rdt_domain *d,
>> u32 closid, enum resctrl_conf_type type);
>> +int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d);
>>
>> #endif /* _RESCTRL_H */
> --
> Best Regards!
> Xin Hao
--
Best Regards!
Xin Hao
Powered by blists - more mailing lists