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
