linux-kernel - Re: [PATCH v7 05/12] x86/resctrl: Detect and configure Slow Memory Bandwidth allocation

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: <c999a7f8-b083-72a6-ff07-201369339c92@intel.com>
Date:   Tue, 25 Oct 2022 16:43:50 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Babu Moger <babu.moger@....com>, <corbet@....net>,
        <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>
CC:     <fenghua.yu@...el.com>, <dave.hansen@...ux.intel.com>,
        <x86@...nel.org>, <hpa@...or.com>, <paulmck@...nel.org>,
        <akpm@...ux-foundation.org>, <quic_neeraju@...cinc.com>,
        <rdunlap@...radead.org>, <damien.lemoal@...nsource.wdc.com>,
        <songmuchun@...edance.com>, <peterz@...radead.org>,
        <jpoimboe@...nel.org>, <pbonzini@...hat.com>,
        <chang.seok.bae@...el.com>, <pawan.kumar.gupta@...ux.intel.com>,
        <jmattson@...gle.com>, <daniel.sneddon@...ux.intel.com>,
        <sandipan.das@....com>, <tony.luck@...el.com>,
        <james.morse@....com>, <linux-doc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <bagasdotme@...il.com>,
        <eranian@...gle.com>
Subject: Re: [PATCH v7 05/12] x86/resctrl: Detect and configure Slow Memory
 Bandwidth allocation

Hi Babu,

Nitpick in Subject ... "allocation" -> "Allocation"?

On 10/17/2022 3:26 PM, Babu Moger wrote:

...

> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
>  
>  	list_for_each_entry(s, &resctrl_schema_all, list) {
>  		r = s->res;
> -		if (r->rid == RDT_RESOURCE_MBA) {
> +		if (r->rid == RDT_RESOURCE_MBA ||
> +		    r->rid == RDT_RESOURCE_SMBA) {
>  			rdtgroup_init_mba(r, rdtgrp->closid);
>  			if (is_mba_sc(r))
>  				continue;

The above hunk and the ones that follow are unexpected.

Note that the software controller, when resctrl is mounted with "mba_MBps", is 
only supported by RDT_RESOURCE_MBA. At this time this really is hard coded all
over the place, for example:

static int set_mba_sc(bool mba_sc)
{
	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
	...

}

Since SMBA hardcodes "delay_linear = false" I do not expect it to support the software
controller ... but these hunks appear to treat SMBA as though it does. It is the "MBA software
controller", not "SMBA software controller". Why does it check above if the MBA software
controller is enabled on SMBA?			

> @@ -3287,7 +3289,8 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d)
>  {
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> -	if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA)
> +	if (supports_mba_mbps() &&
> +	    (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA))
>  		mba_sc_domain_destroy(r, d);
>  
>  	if (!r->mon_capable)
> @@ -3354,8 +3357,9 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
>  
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> -	if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA)
> -		/* RDT_RESOURCE_MBA is never mon_capable */
> +	if (supports_mba_mbps() &&
> +	    (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_MBA))
> +		/* RDT_RESOURCE_MBA (or SMBA) is never mon_capable */

What does this change do? Did you mean to add a r->rid == RDT_RESOURCE_SMBA check?

>  		return mba_sc_domain_allocate(r, d);
>  
>  	if (!r->mon_capable)
> 
> 

Why are the MBA software controller resources allocated/destroyed for a SMBA resource? If
you want to support the software controller for SMBA then there are a lot of other changes
missing.

Reinette