Re: [PATCH v3 07/26] x86/resctrl: Move L3 initialization out of domain_add_cpu_mon()

Reinette Chatre
2025-04-22T18:19:09Z

Hi Tony,

On 4/21/25 1:01 PM, Luck, Tony wrote:
> On Fri, Apr 18, 2025 at 02:51:01PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 4/7/25 4:40 PM, Tony Luck wrote:
>>> To prepare for additional types of monitoring domains, move all the
>>> L3 specific initialization into a helper function.
>>>
>>> Signed-off-by: Tony Luck <tony.luck@intel.com>
>>> ---
>>> arch/x86/kernel/cpu/resctrl/core.c | 57 ++++++++++++++++++------------
>>> 1 file changed, 35 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>> index d82a4a2db699..703423b0be0e 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>> @@ -493,33 +493,12 @@ static void domain_add_cpu_ctrl(int cpu, struct rdt_resource *r)
>>> }
>>> }
>>>
>>> -static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>>> +static void setup_l3_mon_domain(int cpu, int id, struct rdt_resource *r, struct list_head *add_pos)
>>> {
>>> - int id = get_domain_id_from_scope(cpu, r->mon_scope);
>>> - struct list_head *add_pos = NULL;
>>> struct rdt_hw_mon_domain *hw_dom;
>>> - struct rdt_domain_hdr *hdr;
>>> struct rdt_mon_domain *d;
>>> int err;
>>>
>>> - lockdep_assert_held(&domain_list_lock);
>>> -
>>> - if (id < 0) {
>>> - pr_warn_once("Can't find monitor domain id for CPU:%d scope:%d for resource %s\n",
>>> - cpu, r->mon_scope, r->name);
>>> - return;
>>> - }
>>> -
>>> - hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
>>> - if (hdr) {
>>> - if (WARN_ON_ONCE(hdr->type != DOMTYPE(r->rid, DOMTYPE_MON)))
>>> - return;
>>> - d = container_of(hdr, struct rdt_mon_domain, hdr);
>>> -
>>> - cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
>>> - return;
>>> - }
>>> -
>>
>> Many functions called by this new "setup_l3_mon_domain()" are specific to L3 resource but
>> since L3 resource has so far been the only one supported the naming has been generic. Now that
>> this function is made resource specific I think it will help make the code clear if the
>> L3 specific functions called by it are also renamed. For example, mon_domain_free() can
>> be renamed to free_l3_mon_domain() to match the "setup_l3_mon_domain()" introduced here. Also
>> arch_mon_domain_online() -> arch_l3_mon_domain_online().
>
> What about "struct rdt_mon_domain"? It is now specific to L3. Should I
> change that to rdt_l3_mon_domain" as well (60 lines affected)?
>
> Ditto for rdt_hw_mon_domain (but only 12 lines for this one).

Thank you for considering this. My vote would be "yes" for both. I think it will help
understand and maintain the code if naming helps to make obvious what data/code applies to.
Monitoring has been synonymous with L3 monitoring for so long that there may be many
instances of this implicit assumption.

Reinette