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 <[email protected]> >> Sent: Saturday, May 16, 2020 1:29 PM >> To: [email protected]; [email protected]; [email protected]; >> [email protected] >> Cc: [email protected]; [email protected]; [email protected]; >> Moger, Babu <[email protected]>; [email protected]; [email protected]; >> [email protected]; Reinette Chatre <[email protected]> >> 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
