Re: [RESEND PATCH v4 10/15] fs/resctrl: Reset the kernel-mode binding when an rdtgroup is removed

[Babu Moger <babu.moger@amd.com>]

Hi Reinette,

On 8/10/26 22:29, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/7/26 2:50 PM, Babu Moger wrote:
>> Resctrl keeps track of the rdtgroup backing the active global kernel-mode
>> policy, including the group's CPU mask used to program kernel-mode
>> associations.
>>
>> When that rdtgroup is deleted, resctrl must update both its internal state
>> and the hardware configuration associated with the group's kmode_cpu_mask.
> 
> Instead of updating the hardware configuration itself resctrl fs will request
> the underlying architecture to do so.

Yes. Sure.

> 
>> Introduce rdtgroup_config_kmode_reset() to disable the active kernel-mode
>> association for the bound group's CPU mask and to clear its kernel-mode
>> state. Invoke this helper from rdtgroup_kmode_detach(), and reset
>> resctrl_kcfg to INHERIT_CTRL_AND_MON. Ensure rdtgroup_kmode_detach() is
>> called during group removal and filesystem teardown, before the bound
>> rdtgroup is freed.
> 
> (Same comment as before - please do not describe the code details and write in
> imperative tone. Please check all changelogs.)

Ack.

> 
> I think it will be helpful to add reminder why "reset" call needs to
> include all the existing assignments. This could be part of resctrl fs "contract"
> with architecture on when and how it calls resctrl_arch_configure_kmode().

Sure.>
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v4: Re-wrote the changelog.
>>      Added the call free_all_child_rdtgrp() and rmdir_all_sub()
>>      Simplified the code comments.
>>
>> v3: New patch to handle the kernel_mode clean up.
>> ---
>>   fs/resctrl/rdtgroup.c | 62 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 62 insertions(+)
>>
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 0d5c94169d03..21659fd75850 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -1127,6 +1127,61 @@ static int resctrl_kernel_mode_show(struct kernfs_open_file *of,
>>   	return ret;
>>   }
>>   
>> +/**
>> + * rdtgroup_config_kmode_reset() - Tear down the kernel-mode binding on @rdtgrp
>> + * @rdtgrp:	Resctrl group whose kernel-mode binding is being released.
>> + *		May be %NULL when no group is currently bound, in which case
>> + *		this is a no-op.
>> + * @kmode:	Kernel-mode policy currently active on @rdtgrp.
>> + *
>> + * Reset the kernel-mode binding on the CPUs in @rdtgrp's @kmode_cpu_mask.
>> + */
>> +static void rdtgroup_config_kmode_reset(struct rdtgroup *rdtgrp,
> 
> So far the word "active" has been used a lot when referring to the "active"
> kernel mode. To complement this I think it will make this work easier to
> understand if this is renamed to rdtgroup_kmode_deactivate() and to match
> it rdtgroup_config_kmode() could be renamed to rdtgroup_kmode_activate()?

Sure.

> 
>> +					enum resctrl_kernel_mode kmode)
>> +{
>> +	bool assign_mon = false;
>> +
>> +	if (!rdtgrp)
>> +		return;
>> +
>> +	if (kmode == INHERIT_CTRL_AND_MON)
>> +		goto out_clear;
> 
> When kmode is INHERIT_CTRL_AND_MON then there should not be a resource group
> assigned so the work done at "out_clear" is not necessary?

Correct. Not required.

> 
>> +
>> +	if (kmode == GLOBAL_ASSIGN_CTRL_ASSIGN_MON_PER_CPU)
>> +		assign_mon = true;
> 
> Could use the same assignment as resctrl_kmode_online_cpu() and kmode_cpus_write() ?

Sure.

Thanks
Babu