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

From: Reinette Chatre <reinette.chatre@intel.com>
To: <babu.moger@amd.com>, <corbet@lwn.net>, <tglx@linutronix.de>,
	<mingo@redhat.com>, <bp@alien8.de>
Cc: <fenghua.yu@intel.com>, <dave.hansen@linux.intel.com>,
	<x86@kernel.org>, <hpa@zytor.com>, <paulmck@kernel.org>,
	<akpm@linux-foundation.org>, <quic_neeraju@quicinc.com>,
	<rdunlap@infradead.org>, <damien.lemoal@opensource.wdc.com>,
	<songmuchun@bytedance.com>, <peterz@infradead.org>,
	<jpoimboe@kernel.org>, <pbonzini@redhat.com>,
	<chang.seok.bae@intel.com>, <pawan.kumar.gupta@linux.intel.com>,
	<jmattson@google.com>, <daniel.sneddon@linux.intel.com>,
	<sandipan.das@amd.com>, <tony.luck@intel.com>,
	<james.morse@arm.com>, <linux-doc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <bagasdotme@gmail.com>,
	<eranian@google.com>
Subject: Re: [PATCH v7 05/12] x86/resctrl: Detect and configure Slow Memory Bandwidth allocation
Date: Wed, 26 Oct 2022 13:23:04 -0700	[thread overview]
Message-ID: <e299ce70-ebca-a05f-a1f1-aed6386c379f@intel.com> (raw)
In-Reply-To: <ef734fc1-8a51-4c60-f73f-2d71fcc54b30@amd.com>

Hi Babu,

On 10/26/2022 12:07 PM, Moger, Babu wrote:
> On 10/25/22 18:43, Reinette Chatre wrote:
>> 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.
> 
> I am thinking the above check is required, It is updating the
> staged_config with default values. Right now, the default value for SMBA
> is same as MBA default value. So, I used this code to initialize.
> 
> Did I miss something?

As I described in the following comments my concern is related to all the
software controller code still executing for SMBA. Yes, in the above hunk
SMBA would need (some of) rdtgroup_init_mba() ... but note that it contains
software controller checks and in the above hunk its call is also followed
by another software controller check.

The software controller is just applicable to MBA and these checks have been
isolated to the MBA resource. Using it for SMBA that does not support
software controller at all is making the code harder to follow and sets this
code up for future mistakes. I think it would make the code easier to understand
if this is made very clear that software controller is not applicable to SMBA at
all instead of repurposing these flows.

>> 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?
> 
> There is no plan to support SMBA software controller. Yes, I think below
> checks are not required.

Thank you for clarifying. Having the code reflect that clearly would make everything
easier to understand and maintain.

Reinette