Date: Wed, 1 Mar 2017 14:31:35 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Vikas Shivappa <vikas.shivappa@linux.intel.com>
cc: vikas.shivappa@intel.com, linux-kernel@vger.kernel.org, x86@kernel.org,
        hpa@zytor.com, mingo@kernel.org, peterz@infradead.org,
        ravi.v.shankar@intel.com, tony.luck@intel.com, fenghua.yu@intel.com,
        andi.kleen@intel.com
Subject: Re: [PATCH 1/5] x86/intel_rdt: Update control registers only when
 user really modifies it
In-Reply-To: <1487360328-6768-2-git-send-email-vikas.shivappa@linux.intel.com>
Message-ID: <alpine.DEB.2.20.1703011412090.4005@nanos>
References: <1487360328-6768-1-git-send-email-vikas.shivappa@linux.intel.com> <1487360328-6768-2-git-send-email-vikas.shivappa@linux.intel.com>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2335
Lines: 67

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