lkml.org 
[lkml]   [2017]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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
    >

    \
     
     \ /
      Last update: 2017-03-10 01:00    [W:5.689 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site