Messages in this thread | | | Subject | Re: [PATCH V4 1/4] x86/resctrl: Enable user to view and select thread throttling mode | From | Reinette Chatre <> | Date | Mon, 18 May 2020 15:46:02 -0700 |
| |
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@intel.com> >> Sent: Saturday, May 16, 2020 1:29 PM >> To: tglx@linutronix.de; fenghua.yu@intel.com; bp@alien8.de; >> tony.luck@intel.com >> Cc: kuo-lang.tseng@intel.com; ravi.v.shankar@intel.com; mingo@redhat.com; >> Moger, Babu <Babu.Moger@amd.com>; hpa@zytor.com; x86@kernel.org; >> linux-kernel@vger.kernel.org; Reinette Chatre <reinette.chatre@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
| |