Re: [PATCH v2 02/24] x86/resctrl: Split struct rdt_domain

[Reinette Chatre <reinette.chatre@intel.com>]

Hi James,

On 4/8/2021 10:20 AM, James Morse wrote:
> On 31/03/2021 22:36, Reinette Chatre wrote:
>> On 3/12/2021 9:58 AM, James Morse wrote:
>>> resctrl is the defacto Linux ABI for SoC resource partitioning features.
>>> To support it on another architecture, it needs to be abstracted from
>>> the features provided by Intel RDT and AMD PQoS, and moved to /fs/.
>>>
>>> Split struct rdt_domain up too. Move everything that that is particular
>>
>> s/that that/that/
>>
>>> to resctrl into a new header file. resctrl code paths touching a 'hw'
>>> struct indicates where an abstraction is needed.
>>
>> Similar to previous patch it would help to explain how this split was chosen. For example,
>> why are the CPUs to which a resource is associated not considered a hardware property?
> 
> Similarly, because the meaning of those CPUs doesn't differ or change between architectures.

Got it. This seems to be a repeat of the discussion about patch 1. 
Please note that the description of rdt_hw_domain in this patch reads 
"hw attributes of a group of CPUs that share a resource". This can be 
understood to mean that the struct contains attributes discovered from 
hardware.


> I've expanded the middle paragraph in the commit message to explain why the arch specific
> things are arch specific:
> | Continue by splitting struct rdt_domain, into an architecture private
> | 'hw' struct, which contains the common resctrl structure that would be
> | used by any architecture.
> |
> | The hardware values in ctrl_val and mbps_val need to be accessed via
> | helpers to allow another architecture to convert these into a different
> | format if necessary.
> |
> | After this split, filesystem code code paths touching a 'hw' struct
> | indicates where an abstraction is needed.
> 
> and similarly changed the kernel doc comment.

Thank you. I assume this includes changing the "hw attributes of a group 
of CPUs that share a resource" I mention above.

> 
> Let me know if you prefer some other struct name.
> 

I am ok with current naming. Other folks may have better ideas.

Thank you

Reinette