| 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: <26c3ef82-6c1f-5852-891a-7ba2e4cfe971@amd.com>
Date: Tue, 19 Oct 2021 18:19:03 -0500
From: Babu Moger <babu.moger@....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>,
shameerali.kolothum.thodi@...wei.com,
Jamie Iles <jamie@...iainc.com>,
D Scott Phillips OS <scott@...amperecomputing.com>,
lcherian@...vell.com, bobo.shaobowang@...wei.com,
tan.shaopeng@...itsu.com
Subject: Re: [PATCH v2 05/23] x86/resctrl: Add domain online callback for
resctrl work
On 10/1/21 11:02 AM, 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.
>
> Signed-off-by: James Morse <james.morse@....com>
> ---
> 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..f1fa54de8136 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(d);
> + }
> }
>
> 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 e243c7d15b81..19691f9ab061 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);
Looks like lockdep_assert_held was not there in this sequence.
Are you concerned about this lock not being held?
Thanks
Babu Moger
Powered by blists - more mailing lists