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
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
>