Re: [PATCH v8 16/21] x86/resctrl: Always initialise rid field in rdt_resources_all[]

Luck, Tony
2025-04-15T19:08:20Z

On Fri, Apr 11, 2025 at 04:42:24PM +0000, James Morse wrote:
> x86 has an array, rdt_resources_all[], of all possible resources.
> The for-each-resource walkers depend on the rid field of all
> resources being initialised.
>
> If the array ever grows due to another architecture adding a resource
> type that is not defined on x86, the for-each-resources walkers will
> loop forever.

This feels a bit weird. Having rdt_resources_all[] be a "swiss cheese"
array full of holes where other architectures defined events that aren't
supported by x86.

But it does work, so it can go in like this. But someday I may revisit
some experimental patches I did a while back that:
1) Split the rdt_resource structure into separate "ctrl" and "mon"
pieces.
2) Replaced this array with a pair of lists, one each for enabled
ctrl and mon resources.
3) Changed the resource walkers to use list_for_each*() macros.

>
> Initialise all the rid values in resctrl_arch_late_init() before
> any for-each-resource walker can be called.
>
> Signed-off-by: James Morse <james.morse@arm.com>
>
> ---
> Changes since v7:
> * Split out of a previous patch due to a botched merged conflict.
> ---
> arch/x86/kernel/cpu/resctrl/core.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 58d7c6accdf2..ce684da600bc 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -60,7 +60,6 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
> [RDT_RESOURCE_L3] =
> {
> .r_resctrl = {
> - .rid = RDT_RESOURCE_L3,
> .name = "L3",
> .ctrl_scope = RESCTRL_L3_CACHE,
> .mon_scope = RESCTRL_L3_CACHE,
> @@ -74,7 +73,6 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
> [RDT_RESOURCE_L2] =
> {
> .r_resctrl = {
> - .rid = RDT_RESOURCE_L2,
> .name = "L2",
> .ctrl_scope = RESCTRL_L2_CACHE,
> .ctrl_domains = ctrl_domain_init(RDT_RESOURCE_L2),
> @@ -86,7 +84,6 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
> [RDT_RESOURCE_MBA] =
> {
> .r_resctrl = {
> - .rid = RDT_RESOURCE_MBA,
> .name = "MB",
> .ctrl_scope = RESCTRL_L3_CACHE,
> .ctrl_domains = ctrl_domain_init(RDT_RESOURCE_MBA),
> @@ -96,7 +93,6 @@ struct rdt_hw_resource rdt_resources_all[RDT_NUM_RESOURCES] = {
> [RDT_RESOURCE_SMBA] =
> {
> .r_resctrl = {
> - .rid = RDT_RESOURCE_SMBA,
> .name = "SMBA",
> .ctrl_scope = RESCTRL_L3_CACHE,
> .ctrl_domains = ctrl_domain_init(RDT_RESOURCE_SMBA),
> @@ -996,7 +992,11 @@ void resctrl_cpu_detect(struct cpuinfo_x86 *c)
> static int __init resctrl_arch_late_init(void)
> {
> struct rdt_resource *r;
> - int state, ret;
> + int state, ret, i;
> +
> + /* Initialise all rid values for_each_rdt_resource() */
> + for (i = 0; i < RDT_NUM_RESOURCES; i++)
> + rdt_resources_all[i].r_resctrl.rid = i;
>
> /*
> * Initialize functions(or definitions) that are different
> --
> 2.20.1

-Tony