Re: [PATCH v5 13/16] fs/resctrl: Deactivate the kernel mode association when a group is removed

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

Hi Reinette,

On 9/16/26 00:36, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/26/26 12:32 PM, Babu Moger wrote:
>> +/**
>> + * rdtgroup_kmode_detach() - Detach @rdtgrp from a kernel mode association
>> + * @rdtgrp: Resctrl group being removed or torn down.
>> + *
>> + * If @rdtgrp backs the active kernel mode association, disable the
>> + * hardware association programmed for the group's kmode_cpu_mask,
>> + * clear the associated kernel mode state, and restore
>> + * RESCTRL_INHERIT_USER as the active mode.
> 
> I am not going to keep pointing these out but please check entire series
> and remove all these verbatim code descriptions from function comments.

Ack.

> 
>> + */
>> +static void rdtgroup_kmode_detach(struct rdtgroup *rdtgrp)
> 
> Similar to previous comments this only seems relevant to the
> global per-CPU kernel mode so function can be made specific to it.

ok.

> 
>> +{
>> +	if (!rdtgrp || !rdtgrp->kmode)
>> +		return;
>> +
>> +	if (resctrl_kcfg.active.k_rdtgrp != rdtgrp) {
> 
> Related to comment in previous patch this double check of rdtgrp->kmode
> and resctrl_kcfg.active.k_rdtgrp seems to support that only one is needed?

Yes. We can remove rdtgrp->kmode field.

> 
>> +		pr_warn("resctrl: kernel mode group not valid\n");
> 
> Please just fix this file's pr_fmt(fmt) instead.


Hmm... It looks like we have a pr_fmt(fmt) inconsistency across multiple 
files.

Some files use:

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt


while others use:

#define pr_fmt(fmt) "resctrl: " fmt


My understanding is that these will produce different log prefixes. For 
example, KBUILD_MODNAME will typically use the object file name, whereas 
"resctrl" provides a consistent subsystem-wide prefix.

Do you think we should standardize all files to use the same prefix? If 
so, I believe that would be better handled as a separate cleanup patch 
rather than being mixed into this change.

Thanks,
Babu