linux-kernel - Re: [PATCH v1 03/20] x86/resctrl: Add domain online callback for resctrl work

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fc092644-0f5d-b0d1-d14d-765bd42b9cf8@intel.com>
Date:   Wed, 1 Sep 2021 14:19:45 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     James Morse <james.morse@....com>, <x86@...nel.org>,
        <linux-kernel@...r.kernel.org>
CC:     Fenghua Yu <fenghua.yu@...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>,
        Jamie Iles <jamie@...iainc.com>,
        "D Scott Phillips OS" <scott@...amperecomputing.com>,
        <lcherian@...vell.com>, <bobo.shaobowang@...wei.com>
Subject: Re: [PATCH v1 03/20] x86/resctrl: Add domain online callback for
 resctrl work

Hi James,

On 7/29/2021 3:35 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.

Could you please replace "cpu" with "CPU" throughout the series in 
changelogs and comments?

> 
> 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.

(I read the above as problem statement.)

> 
> Move domain_setup_mon_state(), the monitor subdir creation call and the
> mbm/limbo workers into a new resctrl_online_domain() call.

(I read the above as what the code does.)

Could you please add description on what the patch does to address the 
problem you describe?

...

> +}
> +
> +int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
> +{
> +	int err;
> +
> +	lockdep_assert_held(&rdtgroup_mutex); // the arch code took this for us
> +

Please do not use trailing comments.

> +	if (!r->mon_capable)
> +		return 0;
> +
> +	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);
> +

You also seem to address an issue where this work was not properly 
cleaned up on the error paths of the replaced domain_setup_mon_state(). 
Thank you.

> +	/* If resctrl is mounted, add per domain monitor data directories. */
> +	if (static_branch_unlikely(&rdt_enable_key))

Should this be rdt_mon_enable_key instead?

Reinette