Messages in this thread | | | Date | Thu, 9 Mar 2017 16:00:48 -0800 (PST) | From | Shivappa Vikas <> | Subject | Re: [PATCH 1/5] x86/intel_rdt: Update control registers only when user really modifies it |
| |
On Wed, 1 Mar 2017, Thomas Gleixner wrote:
> On Fri, 17 Feb 2017, Vikas Shivappa wrote: > > x86/intel_rdt: Update control registers only when user really modifies it > > This hardly is a precise short summary. > >> When a schemata is updated, the values for all the domains and all >> resources are entered. However, the values for each of them may not >> change in many use cases as the user is only updating values for a >> subset of resources and domains. The resource control values are updated >> via QOS_MSRs which are per package. Change the update to QOS_MSRs to >> happen only when the control value on the particular domain is updated. >> Hence not sending IPIs on all domains when user updates the control >> vals. > > Can you please structure your changelogs in a way which makes them > readable? The above is one big confusing lump. I asked you before to > provide changelogs which are properly structured: > > 1) Context > 2) Problem > 3) Solution
Will fix all the change logs -
> > and the sections to be precise and clear and not clobbered with completely > useless implementation details. > > So a proper changelog for this would be: > > x86/intel_rdt: Avoid update of unchanged control registers > > Schemata files can only be updated as a whole, even if only a single > value for a specific domain/resource changes. > > The current implementation updates all control registers unconditionally > even if the values have not been changed by the schemata update. This > results in pointless IPIs and MSR writes. > > Add a check whether the control register value actually changed and only > update the affected CPUs. > > Can you spot the difference? > >> index f369cb8..14ba504 100644 >> --- a/arch/x86/kernel/cpu/intel_rdt_schemata.c >> +++ b/arch/x86/kernel/cpu/intel_rdt_schemata.c >> @@ -114,9 +114,16 @@ static int update_domains(struct rdt_resource *r, int closid) >> msr_param.high = msr_param.low + 1; >> msr_param.res = r; >> >> + /* >> + * Only update the domains that user has changed. >> + * There by avoiding unnecessary IPIs. > > s/There by/Thereby/ > > But the above is wrong anyway because you split it into two sentences and > obfuscate the reasoning. > > * To avoid unnecessary IPIs update only domains, which have been > * changed by the schemata write. > > That makes it clear that we do it in order to avoid the IPIs. The above > could be misinterpreted as having the side effect of avoiding the IPIs. >
Will fix the comment
Thanks, Vikas
> Thanks, > > tglx >
| |