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

[Reinette Chatre]

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.

Reinette