Hi Reinette,
On 2/24/25 11:01, Reinette Chatre wrote:
> Hi James and Babu,
>
> On 2/24/25 7:49 AM, Moger, Babu wrote:
>> Hi James,
>>
>> On 2/21/25 12:06, James Morse wrote:
>>> Hi Babu,
>>>
>>> On 22/01/2025 20:20, Babu Moger wrote:
>
>>> This sequence has me confused:
>>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>>>> index 3d748fdbcb5f..a9a5dc626a1e 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>>>> @@ -1233,6 +1233,7 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>>>> r->mon.mbm_cntr_assignable = true;
>>>> cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
>>>> r->mon.num_mbm_cntrs = (ebx & GENMASK(15, 0)) + 1;
>>>
>>>> + hw_res->mbm_cntr_assign_enabled = true;
>>>
>>> Here the arch code sets ABMC to be enabled by default at boot.
>>>
>>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> index 6922173c4f8f..515969c5f64f 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> @@ -4302,9 +4302,13 @@ int resctrl_online_mon_domain(struct rdt_resource *r, struct rdt_mon_domain *d)
>>>>
>>>> void resctrl_online_cpu(unsigned int cpu)
>>>> {
>>>> + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>>>> +
>>>> mutex_lock(&rdtgroup_mutex);
>>>> /* The CPU is set in default rdtgroup after online. */
>>>> cpumask_set_cpu(cpu, &rdtgroup_default.cpu_mask);
>>>> + if (r->mon_capable && r->mon.mbm_cntr_assignable)
>>>> + resctrl_arch_mbm_cntr_assign_set_one(r);
>>>> mutex_unlock(&rdtgroup_mutex);
>>>> }
>>>
>>> But here, resctrl has to call back to the arch code to make sure the hardware is in the
>>> same state as hw_res->mbm_cntr_assign_enabled.
>
> Another scenario needing to be supported by this flow is when CPUs come online later ...
> after resctrl is mounted and potentially after the user modified the assignable counter
> mode.
If the user modifies the assignable counter mode. It is recorded in
mbm_cntr_assign_enabled already. When the new CPU comes online, the
hotplug handler(resctrl_arch_online_cpu) is will update the CPU to the new
mode after checking mbm_cntr_assign_enabled.
Are you talking about different case here? Please elaborate.
>
>>>
>>> Could this be done in resctrl_arch_online_cpu() instead? That way resctrl doesn't get CPUs
>>> in an inconsistent state that it has to fix up...
>
> Could you please elaborate the inconsistent state that would need to be fixed up?
>
>>>
>>
>> Sure. Here is the diff.
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
>> b/arch/x86/kernel/cpu/resctrl/core.c
>> index 22399f19810f..f48b298413bc 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -771,6 +771,12 @@ static int resctrl_arch_online_cpu(unsigned int cpu)
>> domain_add_cpu(cpu, r);
>> mutex_unlock(&domain_list_lock);
>>
>> + r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> + mutex_lock(&rdtgroup_mutex);
>> + if (r->mon_capable && r->mon.mbm_cntr_assignable)
>> + resctrl_arch_mbm_cntr_assign_set_one(r);
>> + mutex_unlock(&rdtgroup_mutex);
>> +
>> clear_closid_rmid(cpu);
>> resctrl_online_cpu(cpu);
>
> This would require every architecture to duplicate the above, no?
>
> Also, please note there is more appropriate domain_add_cpu_mon().
Yes. This may be better place to add this code. Will wait once James
clarifies on "inconsistent state".
--
Thanks
Babu Moger
