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

[Reinette Chatre]

Hi Babu,

On 5/14/2020 1:06 PM, Babu Moger wrote:
> 
> 
>> -----Original Message-----
>> From: Reinette Chatre <reinette.chatre@xxxxxxxxx>
>> Sent: Thursday, May 14, 2020 1:11 PM
>> To: Moger, Babu <Babu.Moger@xxxxxxx>; tglx@xxxxxxxxxxxxx;
>> fenghua.yu@xxxxxxxxx; bp@xxxxxxxxx; tony.luck@xxxxxxxxx
>> Cc: kuo-lang.tseng@xxxxxxxxx; ravi.v.shankar@xxxxxxxxx; mingo@xxxxxxxxxx;
>> hpa@xxxxxxxxx; x86@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH V3 1/4] x86/resctrl: Enable user to view and select thread
>> throttling mode
>>
>> 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.
> 
> Oh. Ok. I was using linux master.  Never mind..

My apologies, I neglected to include the repo to which these patches
apply into the cover letter. Will make sure to do it for next version.


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

...

>>
>>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdevelop
>> er.amd.com%2Fwp-
>> content%2Fresources%2F56375.pdf&amp;data=02%7C01%7Cbabu.moger%40a
>> md.com%7C5e7d546489d54002870808d7f8322934%7C3dd8961fe4884e608e11
>> a82d994e183d%7C0%7C0%7C637250766898923893&amp;sdata=hQ6usFmXiUX
>> p%2BZCrM2qdVn2Z3Ggx1c2g4nVwrEqrzG4%3D&amp;reserved=0 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.
> 
> Here is the text from spec.
> "Platform QOS features are implemented on a logical processor basis.
> Therefore, multiple hardware threads of a single physical CPU core may
> have independent resource monitoring and enforcement configurations."
> 
> So, bandwidth allocation is already at thread level. But AMD does not use
> memory delay throttle model to control the allocation like Intel does. So,
> I would say you can initialize it as UNDEFINED not to cause any confusion.

Will do. The next version intends to follow your guidance and moves to
treating this platform feature as a property of the memory bandwidth
resource using the "arch_.." prefix that James also follows and is
intended to be architecture independent.

Thank you very much for your valuable feedback. I look forward to hear
what you think about the next version.

Reinette