Re: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread throttling mode

[Reinette Chatre]

Hi Babu,

Thank you very much for taking a look.

On 5/14/2020 9:45 AM, Babu Moger wrote:
> Hi Reinnette,
> 
> The patches did not apply on my tree. I got the latest tree today. You

Which tree did you use as baseline? These patches should apply cleanly
on top of the other resctrl patches already queued for inclusion into
v5.8 as found on the x86/cache branch of the tip repo.

> might want to check again.
> Hunk #1 FAILED at 29.
> 1 out of 7 hunks FAILED -- saving rejects to file
> arch/x86/kernel/cpu/resctrl/rdtgroup.c.rej
> 
> 
>> -----Original Message-----
>> From: Reinette Chatre <reinette.chatre@xxxxxxxxx>
>> Sent: Wednesday, May 6, 2020 6:50 PM
>> To: tglx@xxxxxxxxxxxxx; fenghua.yu@xxxxxxxxx; bp@xxxxxxxxx;
>> tony.luck@xxxxxxxxx
>> Cc: kuo-lang.tseng@xxxxxxxxx; ravi.v.shankar@xxxxxxxxx; mingo@xxxxxxxxxx;
>> Moger, Babu <Babu.Moger@xxxxxxx>; hpa@xxxxxxxxx; x86@xxxxxxxxxx;
>> linux-kernel@xxxxxxxxxxxxxxx; Reinette Chatre <reinette.chatre@xxxxxxxxx>
>> Subject: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread
>> throttling mode
>>

...

>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
>> b/arch/x86/kernel/cpu/resctrl/core.c
>> index 12f967c6b603..1bc686777069 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -250,6 +250,30 @@ static inline bool rdt_get_mb_table(struct rdt_resource
>> *r)
>>  	return false;
>>  }
>>
>> +/*
>> + * Model-specific test to determine if platform where memory bandwidth
>> + * control is applied to a core can be configured to apply either the
>> + * maximum or minimum of the per-thread delay values.
>> + * By default, platforms where memory bandwidth control is applied to a
>> + * core will select the maximum delay value of the per-thread CLOS.
>> + *
>> + * NOTE: delay value programmed to hardware is inverse of bandwidth
>> + * percentage configured via user interface.
>> + */
>> +bool mba_cfg_supports_min_max_intel(void)
>> +{
>> +	switch (boot_cpu_data.x86_model) {
>> +	case INTEL_FAM6_ATOM_TREMONT_D:
>> +	case INTEL_FAM6_ICELAKE_X:
>> +	case INTEL_FAM6_ICELAKE_D:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +
>> +	return false;
>> +}
> 
> I see that you are calling this function multiple times. Why don't you

It is called:
- once during initialization
- once per package initialization (when first CPU on a package comes online)
- once per read from or write to the new "thread_throttle_mode" file

> make it as a property in rdt_resource. Set it only once during the
> init(may be in get_mem_config_intel). Then you can use it wherever
> required. This also probably help James to make everything architecture
> independent. What do you think?

If I understand correctly this would require understanding how each
architecture behaves in this regard to ensure the property within
rdt_resource is initialized correctly. While it could just be set within
get_mem_config_intel as you suggest, this property would be present in
AMD's resource and thus need a value ... could you please provide
guidance what this property should be on AMD? I looked at the bandwidth
enforcement section of
https://developer.amd.com/wp-content/resources/56375.pdf but it is not
obvious to me where bandwidth is actually enforced and how this
enforcement is affected when threads and/or cores are running tasks with
different CLOS that have different bandwidth limits assigned.

With AMD's properties understood then the new "thread_throttle_mode"
file could be visible on all platforms, not just Intel, and display
accurate values for all and be architecture independent.

Alternatively, the new property could have values: UNDEFINED, MIN, MAX,
or PER_THREAD ... with AMD having UNDEFINED and the
"thread_throttle_mode" continues to be visible on Intel platforms only.

I would appreciate your thoughts on this.

> I assume that this property is probably not part of CPUID.

Correct. This specific property is model specific, but also transient in
that it is replaced by X86_FEATURE_PER_THREAD_MBA (that is part of
CPUID) in future platforms.

Reinette