Re: [PATCH v5 10/16] fs/resctrl: Add support for hidden resource group files

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

Hi Reinette,

On 9/17/26 13:31, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/17/26 10:26 AM, Babu Moger wrote:
>> Hi Reinette,
>>
>>
>> On 9/16/26 00:33, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 8/26/26 12:32 PM, Babu Moger wrote:
>>>> @@ -2377,6 +2380,33 @@ static int rdtgroup_add_files(struct kernfs_node *kn, unsigned long fflags)
>>>>        return ret;
>>>>    }
>>>>    +/*
>>>> + * resctrl_hidden_files_set_visible() - Show or hide files marked hidden
>>>> + * @kn: resource group kernfs_node
>>>> + * @show: whether to show or hide
>>>> + *
>>>> + * Iterate res_common_files entries marked hidden and show or hide the
>>>> + * corresponding file under @kn.
>>>
>>> As a generic helper this is not appropriate since it just blindly makes *all*
>>> hidden files belonging to a resource group visible or invisible. This pretends to
>>> be a generic function but then assumes that only kernel mode related files
>>> are ever created with the hidden flag set because as the kernel mode changes
>>> all hidden files belonging to the named parent changes visibility.
>>>
>> Are you suggesting that I update the code comments to be more
>> specific about the group, or are you suggesting that the code itself
>> should be changed?
> 
> Both.  The "hidden" flag is *only* about whether a file should be visible
> at time of creation, whether it is by default "hidden". That is accomplished
> without this function.
> 
> Whether a file is visible or not after initial creation is feature specific.
> For example, as is done with resctrl_bmec_files_show(). There is no use
> case for "make all hidden files visible".
> 
> resctrl_hidden_files_set_visible() is a hack because it pretends to introduce
> a generic feature but the implementation only supports two specific files.
> Consider the scenario when somebody tries to add another resource
> group file that is by default hidden. Just setting the "hidden" flag will be
> a reasonable thing to do, but that unrelated file will have its visibility
> connected to the kernel mode feature!
> 
> Please replace resctrl_hidden_files_set_visible() with a kernel mode specific
> function that only controls visibility of this feature's specific files. That
> could even be in a separate patch.
> 
I will separate this patch.  Will rename it something like.

void resctrl_kmode_global_file_show(kn, "name", true/false);

Thanks
Babu