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.cha...@intel.com>
>> Sent: Saturday, May 16, 2020 1:29 PM
>> To: t...@linutronix.de; fenghua...@intel.com; b...@alien8.de;
>> tony.l...@intel.com
>> Cc: kuo-lang.ts...@intel.com; ravi.v.shan...@intel.com; mi...@redhat.com;
>> Moger, Babu <babu.mo...@amd.com>; h...@zytor.com; x...@kernel.org;
>> linux-kernel@vger.kernel.org; Reinette Chatre <reinette.cha...@intel.com>
>> 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
