Re: [PATCH V4 1/4] x86/resctrl: Enable user to view and select thread throttling mode

[Reinette Chatre]

Hi Babu,

On 5/18/2020 10:28 AM, Babu Moger wrote:
> Hi Reinette,
>  Looks good most part. Few minor comments.

Thank you very much for taking a look.

> 
>> -----Original Message-----
>> From: Reinette Chatre <reinette.chatre@xxxxxxxxx>
>> Sent: Saturday, May 16, 2020 1:29 PM
>> To: tglx@xxxxxxxxxxxxx; fenghua.yu@xxxxxxxxx; bp@xxxxxxxxx;
>> tony.luck@xxxxxxxxx
>> Cc: kuo-lang.tseng@xxxxxxxxx; ravi.v.shankar@xxxxxxxxx; mingo@xxxxxxxxxx;
>> Moger, Babu <Babu.Moger@xxxxxxx>; hpa@xxxxxxxxx; x86@xxxxxxxxxx;
>> linux-kernel@xxxxxxxxxxxxxxx; Reinette Chatre <reinette.chatre@xxxxxxxxx>
>> Subject: [PATCH V4 1/4] x86/resctrl: Enable user to view and select thread
>> throttling mode
>>

...

>> +static void mba_cfg_reconfigure_throttle_mode(struct rdt_resource *r)
>> +{
>> +	if (!r->alloc_capable)
>> +		return;
>> +
>> +	if (r == &rdt_resources_all[RDT_RESOURCE_MBA] &&
>> +	    r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX)
>> +		wrmsrl(MSR_MBA_CFG, mba_cfg_msr);
>> +}
> 
> How about this? It is kind of consistent with other checks that are done.
> 
> If (r->alloc_capable && (r == &rdt_resources_all[RDT_RESOURCE_MBA]) &&
>     (r->membw.arch_throttle_mode == THREAD_THROTTLE_MIN_MAX))
>              wrmsrl(MSR_MBA_CFG, mba_cfg_msr);
> 

Sure. Will do (with fewer parentheses).

>> +
>> +/*
>> + * Model-specific test to determine if platform where memory bandwidth
>> + * control is applied to a core can be configured to apply either the
>> + * maximum or minimum of the per-thread delay values.
>> + * By default, platforms where memory bandwidth control is applied to a
>> + * core will select the maximum delay value of the per-thread CLOS.
>> + *
>> + * NOTE: delay value programmed to hardware is inverse of bandwidth
>> + * percentage configured via user interface.
>> + */
>> +static bool mba_cfg_supports_min_max_intel(void)
>> +{
>> +	switch (boot_cpu_data.x86_model) {
>> +	case INTEL_FAM6_ATOM_TREMONT_D:
>> +	case INTEL_FAM6_ICELAKE_X:
>> +	case INTEL_FAM6_ICELAKE_D:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +
>> +	return false;
> 
> Is this last return required?  I don't think so.  We will never go here.
> 

Indeed. Thank you for catching this.

Reinette