Messages in this thread | | | Subject | Re: [PATCH v2 01/24] x86/resctrl: Split struct rdt_resource | From | Reinette Chatre <> | Date | Wed, 31 Mar 2021 14:35:52 -0700 |
| |
Hi James,
A significant time has passed since the first version and with that a lot of my context lost.
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/. > > Start by splitting struct rdt_resource, (the name is kept to keep the noise > down), and add some type-trickery to keep the foreach helpers working.
Could you please replace "add some type-trickery" with a description of the changes(tricks?) referred to? Comments in the code would be helpful also ... helping to avoid frowning at what at first glance seems like an out-of-bounds access.
> > Move everything that is particular to resctrl into a new header > file, keeping the x86 hardware accessors where they are. resctrl code > paths touching a 'hw' struct indicates where an abstraction is needed.
This establishes the significance of this patch. Here the rdt_resource struct is split up and it is this split that guides the subsequent abstraction. Considering this I find that this description does not explain the resulting split sufficiently.
Specifically, after reading the above summary I expect fs information in rdt_resource and hw information in rdt_hw_resource but that does not seem to be the case. For example, num_rmid is a property obtained from hardware but is found in rdt_resource while other hardware properties initialized at the same time are found in rdt_hw_resource. It is interesting to look at when the hardware is discovered (for example, functions like cache_alloc_hsw_probe(), __get_mem_config_intel(), __rdt_get_mem_config_amd(), rdt_get_cache_alloc_cfg()). Note how some of the discovered values end up in rdt_resource and some in rdt_hw_resource. I was expecting these properties discovered from hardware to be in rdt_hw_resource.
It is also not clear to me how these structures are intended to be used for related hardware properties. For example, rdt_resource keeps the properties alloc_capable/alloc_enabled/mon_capable/mon_enabled - but in this series companion properties of cdp_capable/cdp_enabled are introduced and placed in rdt_hw_resource. That seems contradicting to me.
Since this change is so foundational it would be very helpful if the resulting split could be explained in more detail.
Thank you
Reinette
| |