lkml.org 
[lkml]   [2017]   [Mar]   [1]   [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 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

    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.

    Thanks,

    tglx

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