The default resource group and its files are created during kernel
init time. Upcoming changes will make some resctrl files optional
based on a mount parameter. If optional files are to be added to the
default group based on the mount option, then each new file needs to
be created separately and call kernfs_activate() again.

Create all files of the default resource group during resctrl
mount, destroyed during unmount, to avoid scattering resctrl
file addition across two separate code flows.

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl/internal.h |  2 +
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
 2 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index b09e7abd1299..44ad98f8c7af 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -611,5 +611,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
 void __init thread_throttle_mode_init(void);
 void __init mbm_config_rftype_init(const char *config);
 void rdt_staged_configs_clear(void);
+int rdtgroup_setup_root(struct rdt_fs_context *ctx);
+void rdtgroup_destroy_root(void);
 
 #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 80a4f76bc34b..98f9f880c30b 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2516,10 +2516,14 @@ static int rdt_get_tree(struct fs_context *fc)
 		goto out;
 	}
 
-	ret = rdt_enable_ctx(ctx);
+	ret = rdtgroup_setup_root(ctx);
 	if (ret)
 		goto out;
 
+	ret = rdt_enable_ctx(ctx);
+	if (ret)
+		goto out_root;
+
 	ret = schemata_list_create();
 	if (ret) {
 		schemata_list_destroy();
@@ -2528,6 +2532,12 @@ static int rdt_get_tree(struct fs_context *fc)
 
 	closid_init();
 
+	ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
+	if (ret)
+		goto out_schemata_free;
+
+	kernfs_activate(rdtgroup_default.kn);
+
 	ret = rdtgroup_create_info_dir(rdtgroup_default.kn);
 	if (ret < 0)
 		goto out_schemata_free;
@@ -2584,6 +2594,8 @@ static int rdt_get_tree(struct fs_context *fc)
 	schemata_list_destroy();
 out_ctx:
 	rdt_disable_ctx();
+out_root:
+	rdtgroup_destroy_root();
 out:
 	rdt_last_cmd_clear();
 	mutex_unlock(&rdtgroup_mutex);
@@ -2654,7 +2666,6 @@ static int rdt_init_fs_context(struct fs_context *fc)
 	if (!ctx)
 		return -ENOMEM;
 
-	ctx->kfc.root = rdt_root;
 	ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
 	fc->fs_private = &ctx->kfc;
 	fc->ops = &rdt_fs_context_ops;
@@ -2824,6 +2835,7 @@ static void rdt_kill_sb(struct super_block *sb)
 	rdt_pseudo_lock_release();
 	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
 	schemata_list_destroy();
+	rdtgroup_destroy_root();
 	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
 	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
 	static_branch_disable_cpuslocked(&rdt_enable_key);
@@ -3705,10 +3717,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
 	.show_options	= rdtgroup_show_options,
 };
 
-static int __init rdtgroup_setup_root(void)
+int rdtgroup_setup_root(struct rdt_fs_context *ctx)
 {
-	int ret;
-
 	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
 				      KERNFS_ROOT_CREATE_DEACTIVATED |
 				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
@@ -3716,6 +3726,20 @@ static int __init rdtgroup_setup_root(void)
 	if (IS_ERR(rdt_root))
 		return PTR_ERR(rdt_root);
 
+	ctx->kfc.root = rdt_root;
+	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
+
+	return 0;
+}
+
+void rdtgroup_destroy_root(void)
+{
+	kernfs_destroy_root(rdt_root);
+	rdtgroup_default.kn = NULL;
+}
+
+static void __init rdtgroup_setup_default(void)
+{
 	mutex_lock(&rdtgroup_mutex);
 
 	rdtgroup_default.closid = 0;
@@ -3725,19 +3749,7 @@ static int __init rdtgroup_setup_root(void)
 
 	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
 
-	ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
-	if (ret) {
-		kernfs_destroy_root(rdt_root);
-		goto out;
-	}
-
-	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
-	kernfs_activate(rdtgroup_default.kn);
-
-out:
 	mutex_unlock(&rdtgroup_mutex);
-
-	return ret;
 }
 
 static void domain_destroy_mon_state(struct rdt_domain *d)
@@ -3859,13 +3871,11 @@ int __init rdtgroup_init(void)
 	seq_buf_init(&last_cmd_status, last_cmd_status_buf,
 		     sizeof(last_cmd_status_buf));
 
-	ret = rdtgroup_setup_root();
-	if (ret)
-		return ret;
+	rdtgroup_setup_default();
 
 	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
 	if (ret)
-		goto cleanup_root;
+		return ret;
 
 	ret = register_filesystem(&rdt_fs_type);
 	if (ret)
@@ -3898,8 +3908,6 @@ int __init rdtgroup_init(void)
 
 cleanup_mountpoint:
 	sysfs_remove_mount_point(fs_kobj, "resctrl");
-cleanup_root:
-	kernfs_destroy_root(rdt_root);
 
 	return ret;
 }
@@ -3909,5 +3917,4 @@ void __exit rdtgroup_exit(void)
 	debugfs_remove_recursive(debugfs_resctrl);
 	unregister_filesystem(&rdt_fs_type);
 	sysfs_remove_mount_point(fs_kobj, "resctrl");
-	kernfs_destroy_root(rdt_root);
 }
-- 
2.34.1

Hi, Babu,

On 8/21/23 16:30, Babu Moger wrote:
> The default resource group and its files are created during kernel
> init time. Upcoming changes will make some resctrl files optional
> based on a mount parameter. If optional files are to be added to the
> default group based on the mount option, then each new file needs to
> be created separately and call kernfs_activate() again.
> 
> Create all files of the default resource group during resctrl
> mount, destroyed during unmount, to avoid scattering resctrl
> file addition across two separate code flows.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>   arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>   2 files changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index b09e7abd1299..44ad98f8c7af 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -611,5 +611,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>   void __init thread_throttle_mode_init(void);
>   void __init mbm_config_rftype_init(const char *config);
>   void rdt_staged_configs_clear(void);
> +int rdtgroup_setup_root(struct rdt_fs_context *ctx);
> +void rdtgroup_destroy_root(void);

These two functions are called only in rdtgroup.c. Why are they exposed 
here?

>   
>   #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 80a4f76bc34b..98f9f880c30b 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2516,10 +2516,14 @@ static int rdt_get_tree(struct fs_context *fc)
>   		goto out;
>   	}
>   
> -	ret = rdt_enable_ctx(ctx);
> +	ret = rdtgroup_setup_root(ctx);
>   	if (ret)
>   		goto out;
>   
> +	ret = rdt_enable_ctx(ctx);
> +	if (ret)
> +		goto out_root;
> +
>   	ret = schemata_list_create();
>   	if (ret) {
>   		schemata_list_destroy();
> @@ -2528,6 +2532,12 @@ static int rdt_get_tree(struct fs_context *fc)
>   
>   	closid_init();
>   
> +	ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
> +	if (ret)
> +		goto out_schemata_free;
> +
> +	kernfs_activate(rdtgroup_default.kn);
> +
>   	ret = rdtgroup_create_info_dir(rdtgroup_default.kn);
>   	if (ret < 0)
>   		goto out_schemata_free;
> @@ -2584,6 +2594,8 @@ static int rdt_get_tree(struct fs_context *fc)
>   	schemata_list_destroy();
>   out_ctx:
>   	rdt_disable_ctx();
> +out_root:
> +	rdtgroup_destroy_root();
>   out:
>   	rdt_last_cmd_clear();
>   	mutex_unlock(&rdtgroup_mutex);
> @@ -2654,7 +2666,6 @@ static int rdt_init_fs_context(struct fs_context *fc)
>   	if (!ctx)
>   		return -ENOMEM;
>   
> -	ctx->kfc.root = rdt_root;
>   	ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
>   	fc->fs_private = &ctx->kfc;
>   	fc->ops = &rdt_fs_context_ops;
> @@ -2824,6 +2835,7 @@ static void rdt_kill_sb(struct super_block *sb)
>   	rdt_pseudo_lock_release();
>   	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
>   	schemata_list_destroy();
> +	rdtgroup_destroy_root();
>   	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
>   	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
>   	static_branch_disable_cpuslocked(&rdt_enable_key);
> @@ -3705,10 +3717,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>   	.show_options	= rdtgroup_show_options,
>   };
>   
> -static int __init rdtgroup_setup_root(void)
> +int rdtgroup_setup_root(struct rdt_fs_context *ctx)

Since rdtgroup_setup_root() is called only in this file, need to add 
"static".

>   {
> -	int ret;
> -
>   	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>   				      KERNFS_ROOT_CREATE_DEACTIVATED |
>   				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
> @@ -3716,6 +3726,20 @@ static int __init rdtgroup_setup_root(void)
>   	if (IS_ERR(rdt_root))
>   		return PTR_ERR(rdt_root);
>   
> +	ctx->kfc.root = rdt_root;
> +	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
> +
> +	return 0;
> +}
> +
> +void rdtgroup_destroy_root(void)

Ditto.

> +{
> +	kernfs_destroy_root(rdt_root);
> +	rdtgroup_default.kn = NULL;
> +}
> +
> +static void __init rdtgroup_setup_default(void)
> +{
>   	mutex_lock(&rdtgroup_mutex);
>   
>   	rdtgroup_default.closid = 0;
> @@ -3725,19 +3749,7 @@ static int __init rdtgroup_setup_root(void)
>   
>   	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
>   
> -	ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
> -	if (ret) {
> -		kernfs_destroy_root(rdt_root);
> -		goto out;
> -	}
> -
> -	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
> -	kernfs_activate(rdtgroup_default.kn);
> -
> -out:
>   	mutex_unlock(&rdtgroup_mutex);
> -
> -	return ret;
>   }
>   
>   static void domain_destroy_mon_state(struct rdt_domain *d)
> @@ -3859,13 +3871,11 @@ int __init rdtgroup_init(void)
>   	seq_buf_init(&last_cmd_status, last_cmd_status_buf,
>   		     sizeof(last_cmd_status_buf));
>   
> -	ret = rdtgroup_setup_root();
> -	if (ret)
> -		return ret;
> +	rdtgroup_setup_default();
>   
>   	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
>   	if (ret)
> -		goto cleanup_root;
> +		return ret;
>   
>   	ret = register_filesystem(&rdt_fs_type);
>   	if (ret)
> @@ -3898,8 +3908,6 @@ int __init rdtgroup_init(void)
>   
>   cleanup_mountpoint:
>   	sysfs_remove_mount_point(fs_kobj, "resctrl");
> -cleanup_root:
> -	kernfs_destroy_root(rdt_root);
>   
>   	return ret;
>   }
> @@ -3909,5 +3917,4 @@ void __exit rdtgroup_exit(void)
>   	debugfs_remove_recursive(debugfs_resctrl);
>   	unregister_filesystem(&rdt_fs_type);
>   	sysfs_remove_mount_point(fs_kobj, "resctrl");
> -	kernfs_destroy_root(rdt_root);
>   }

Thanks.

-Fenghua

Hi Fenghua,

On 9/1/2023 6:21 PM, Fenghua Yu wrote:
> Hi, Babu,
>
> On 8/21/23 16:30, Babu Moger wrote:
>> The default resource group and its files are created during kernel
>> init time. Upcoming changes will make some resctrl files optional
>> based on a mount parameter. If optional files are to be added to the
>> default group based on the mount option, then each new file needs to
>> be created separately and call kernfs_activate() again.
>>
>> Create all files of the default resource group during resctrl
>> mount, destroyed during unmount, to avoid scattering resctrl
>> file addition across two separate code flows.
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>   arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>   2 files changed, 33 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h 
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index b09e7abd1299..44ad98f8c7af 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -611,5 +611,7 @@ void rdt_domain_reconfigure_cdp(struct 
>> rdt_resource *r);
>>   void __init thread_throttle_mode_init(void);
>>   void __init mbm_config_rftype_init(const char *config);
>>   void rdt_staged_configs_clear(void);
>> +int rdtgroup_setup_root(struct rdt_fs_context *ctx);
>> +void rdtgroup_destroy_root(void);
>
> These two functions are called only in rdtgroup.c. Why are they 
> exposed here?
Yes. Removed it now.
>
>>     #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c 
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 80a4f76bc34b..98f9f880c30b 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2516,10 +2516,14 @@ static int rdt_get_tree(struct fs_context *fc)
>>           goto out;
>>       }
>>   -    ret = rdt_enable_ctx(ctx);
>> +    ret = rdtgroup_setup_root(ctx);
>>       if (ret)
>>           goto out;
>>   +    ret = rdt_enable_ctx(ctx);
>> +    if (ret)
>> +        goto out_root;
>> +
>>       ret = schemata_list_create();
>>       if (ret) {
>>           schemata_list_destroy();
>> @@ -2528,6 +2532,12 @@ static int rdt_get_tree(struct fs_context *fc)
>>         closid_init();
>>   +    ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
>> +    if (ret)
>> +        goto out_schemata_free;
>> +
>> +    kernfs_activate(rdtgroup_default.kn);
>> +
>>       ret = rdtgroup_create_info_dir(rdtgroup_default.kn);
>>       if (ret < 0)
>>           goto out_schemata_free;
>> @@ -2584,6 +2594,8 @@ static int rdt_get_tree(struct fs_context *fc)
>>       schemata_list_destroy();
>>   out_ctx:
>>       rdt_disable_ctx();
>> +out_root:
>> +    rdtgroup_destroy_root();
>>   out:
>>       rdt_last_cmd_clear();
>>       mutex_unlock(&rdtgroup_mutex);
>> @@ -2654,7 +2666,6 @@ static int rdt_init_fs_context(struct 
>> fs_context *fc)
>>       if (!ctx)
>>           return -ENOMEM;
>>   -    ctx->kfc.root = rdt_root;
>>       ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
>>       fc->fs_private = &ctx->kfc;
>>       fc->ops = &rdt_fs_context_ops;
>> @@ -2824,6 +2835,7 @@ static void rdt_kill_sb(struct super_block *sb)
>>       rdt_pseudo_lock_release();
>>       rdtgroup_default.mode = RDT_MODE_SHAREABLE;
>>       schemata_list_destroy();
>> +    rdtgroup_destroy_root();
>> static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
>>       static_branch_disable_cpuslocked(&rdt_mon_enable_key);
>>       static_branch_disable_cpuslocked(&rdt_enable_key);
>> @@ -3705,10 +3717,8 @@ static struct kernfs_syscall_ops 
>> rdtgroup_kf_syscall_ops = {
>>       .show_options    = rdtgroup_show_options,
>>   };
>>   -static int __init rdtgroup_setup_root(void)
>> +int rdtgroup_setup_root(struct rdt_fs_context *ctx)
>
> Since rdtgroup_setup_root() is called only in this file, need to add 
> "static".

Yes. Taken care of it now.


>
>>   {
>> -    int ret;
>> -
>>       rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>>                         KERNFS_ROOT_CREATE_DEACTIVATED |
>>                         KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
>> @@ -3716,6 +3726,20 @@ static int __init rdtgroup_setup_root(void)
>>       if (IS_ERR(rdt_root))
>>           return PTR_ERR(rdt_root);
>>   +    ctx->kfc.root = rdt_root;
>> +    rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
>> +
>> +    return 0;
>> +}
>> +
>> +void rdtgroup_destroy_root(void)
>
> Ditto.

Thanks

Babu

Hi Fenghua,

On 9/1/2023 4:21 PM, Fenghua Yu wrote:
> On 8/21/23 16:30, Babu Moger wrote:
>> The default resource group and its files are created during kernel
>> init time. Upcoming changes will make some resctrl files optional
>> based on a mount parameter. If optional files are to be added to the
>> default group based on the mount option, then each new file needs to
>> be created separately and call kernfs_activate() again.
>>
>> Create all files of the default resource group during resctrl
>> mount, destroyed during unmount, to avoid scattering resctrl
>> file addition across two separate code flows.
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>   arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>   2 files changed, 33 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index b09e7abd1299..44ad98f8c7af 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -611,5 +611,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>   void __init thread_throttle_mode_init(void);
>>   void __init mbm_config_rftype_init(const char *config);
>>   void rdt_staged_configs_clear(void);
>> +int rdtgroup_setup_root(struct rdt_fs_context *ctx);
>> +void rdtgroup_destroy_root(void);
> 
> These two functions are called only in rdtgroup.c. Why are they exposed here?
> 

Could you please take a look at the email thread [1] that
discusses this? We reached a compromise but would appreciate
if you have any guidance on the right solution.

Reinette

[1] https://lore.kernel.org/lkml/972db626-1d74-679d-72f2-3e122f95c314@intel.com/

Hi, Reinette,

On 9/1/23 16:36, Reinette Chatre wrote:
> Hi Fenghua,
> 
> On 9/1/2023 4:21 PM, Fenghua Yu wrote:
>> On 8/21/23 16:30, Babu Moger wrote:
>>> The default resource group and its files are created during kernel
>>> init time. Upcoming changes will make some resctrl files optional
>>> based on a mount parameter. If optional files are to be added to the
>>> default group based on the mount option, then each new file needs to
>>> be created separately and call kernfs_activate() again.
>>>
>>> Create all files of the default resource group during resctrl
>>> mount, destroyed during unmount, to avoid scattering resctrl
>>> file addition across two separate code flows.
>>>
>>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>> ---
>>>    arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>>    arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>>    2 files changed, 33 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index b09e7abd1299..44ad98f8c7af 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -611,5 +611,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>>    void __init thread_throttle_mode_init(void);
>>>    void __init mbm_config_rftype_init(const char *config);
>>>    void rdt_staged_configs_clear(void);
>>> +int rdtgroup_setup_root(struct rdt_fs_context *ctx);
>>> +void rdtgroup_destroy_root(void);
>>
>> These two functions are called only in rdtgroup.c. Why are they exposed here?
>>
> 
> Could you please take a look at the email thread [1] that
> discusses this? We reached a compromise but would appreciate
> if you have any guidance on the right solution.

Yes, putting the static declarations earlier in rdtgroup.c is right AFAICT.

> 
> Reinette
> 
> [1] https://lore.kernel.org/lkml/972db626-1d74-679d-72f2-3e122f95c314@intel.com/
> 

Thanks.

-Fenghua

On 9/1/2023 4:46 PM, Fenghua Yu wrote:
> 
> Yes, putting the static declarations earlier in rdtgroup.c is right AFAICT.
> 

Thank you very much Fenghua.

Reinette

Hi Babu,

On 8/21/2023 4:30 PM, Babu Moger wrote:
> The default resource group and its files are created during kernel
> init time. Upcoming changes will make some resctrl files optional
> based on a mount parameter. If optional files are to be added to the
> default group based on the mount option, then each new file needs to
> be created separately and call kernfs_activate() again.
> 
> Create all files of the default resource group during resctrl
> mount, destroyed during unmount, to avoid scattering resctrl
> file addition across two separate code flows.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>  2 files changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index b09e7abd1299..44ad98f8c7af 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -611,5 +611,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>  void __init thread_throttle_mode_init(void);
>  void __init mbm_config_rftype_init(const char *config);
>  void rdt_staged_configs_clear(void);
> +int rdtgroup_setup_root(struct rdt_fs_context *ctx);
> +void rdtgroup_destroy_root(void);
>  

From what I can tell these functions are only used in rdtgroup.c.
Can this export be avoided by just moving these functions within
rdtgroup.c and making them static?

>  #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 80a4f76bc34b..98f9f880c30b 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2516,10 +2516,14 @@ static int rdt_get_tree(struct fs_context *fc)
>  		goto out;
>  	}
>  
> -	ret = rdt_enable_ctx(ctx);
> +	ret = rdtgroup_setup_root(ctx);
>  	if (ret)
>  		goto out;
>  
> +	ret = rdt_enable_ctx(ctx);
> +	if (ret)
> +		goto out_root;
> +
>  	ret = schemata_list_create();
>  	if (ret) {
>  		schemata_list_destroy();
> @@ -2528,6 +2532,12 @@ static int rdt_get_tree(struct fs_context *fc)
>  
>  	closid_init();
>  
> +	ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
> +	if (ret)
> +		goto out_schemata_free;
> +
> +	kernfs_activate(rdtgroup_default.kn);
> +
>  	ret = rdtgroup_create_info_dir(rdtgroup_default.kn);
>  	if (ret < 0)
>  		goto out_schemata_free;
> @@ -2584,6 +2594,8 @@ static int rdt_get_tree(struct fs_context *fc)
>  	schemata_list_destroy();
>  out_ctx:
>  	rdt_disable_ctx();
> +out_root:
> +	rdtgroup_destroy_root();
>  out:
>  	rdt_last_cmd_clear();
>  	mutex_unlock(&rdtgroup_mutex);
> @@ -2654,7 +2666,6 @@ static int rdt_init_fs_context(struct fs_context *fc)
>  	if (!ctx)
>  		return -ENOMEM;
>  
> -	ctx->kfc.root = rdt_root;
>  	ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
>  	fc->fs_private = &ctx->kfc;
>  	fc->ops = &rdt_fs_context_ops;
> @@ -2824,6 +2835,7 @@ static void rdt_kill_sb(struct super_block *sb)
>  	rdt_pseudo_lock_release();
>  	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
>  	schemata_list_destroy();
> +	rdtgroup_destroy_root();
>  	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
>  	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
>  	static_branch_disable_cpuslocked(&rdt_enable_key);
> @@ -3705,10 +3717,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>  	.show_options	= rdtgroup_show_options,
>  };
>  
> -static int __init rdtgroup_setup_root(void)
> +int rdtgroup_setup_root(struct rdt_fs_context *ctx)
>  {
> -	int ret;
> -
>  	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>  				      KERNFS_ROOT_CREATE_DEACTIVATED |
>  				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
> @@ -3716,6 +3726,20 @@ static int __init rdtgroup_setup_root(void)
>  	if (IS_ERR(rdt_root))
>  		return PTR_ERR(rdt_root);
>  
> +	ctx->kfc.root = rdt_root;
> +	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
> +
> +	return 0;
> +}
> +
> +void rdtgroup_destroy_root(void)
> +{
> +	kernfs_destroy_root(rdt_root);
> +	rdtgroup_default.kn = NULL;
> +}
> +
> +static void __init rdtgroup_setup_default(void)
> +{
>  	mutex_lock(&rdtgroup_mutex);
>  
>  	rdtgroup_default.closid = 0;
> @@ -3725,19 +3749,7 @@ static int __init rdtgroup_setup_root(void)
>  
>  	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
>  
> -	ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
> -	if (ret) {
> -		kernfs_destroy_root(rdt_root);
> -		goto out;
> -	}
> -
> -	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
> -	kernfs_activate(rdtgroup_default.kn);
> -
> -out:
>  	mutex_unlock(&rdtgroup_mutex);
> -
> -	return ret;
>  }
>  
>  static void domain_destroy_mon_state(struct rdt_domain *d)
> @@ -3859,13 +3871,11 @@ int __init rdtgroup_init(void)
>  	seq_buf_init(&last_cmd_status, last_cmd_status_buf,
>  		     sizeof(last_cmd_status_buf));
>  
> -	ret = rdtgroup_setup_root();
> -	if (ret)
> -		return ret;
> +	rdtgroup_setup_default();
>  
>  	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
>  	if (ret)
> -		goto cleanup_root;
> +		return ret;
>  
>  	ret = register_filesystem(&rdt_fs_type);
>  	if (ret)
> @@ -3898,8 +3908,6 @@ int __init rdtgroup_init(void)
>  
>  cleanup_mountpoint:
>  	sysfs_remove_mount_point(fs_kobj, "resctrl");
> -cleanup_root:
> -	kernfs_destroy_root(rdt_root);
>  
>  	return ret;
>  }
> @@ -3909,5 +3917,4 @@ void __exit rdtgroup_exit(void)
>  	debugfs_remove_recursive(debugfs_resctrl);
>  	unregister_filesystem(&rdt_fs_type);
>  	sysfs_remove_mount_point(fs_kobj, "resctrl");
> -	kernfs_destroy_root(rdt_root);
>  }


The rest of this patch looks good to me.

Reinette

Hi Reinette,

On 8/29/23 15:11, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/21/2023 4:30 PM, Babu Moger wrote:
>> The default resource group and its files are created during kernel
>> init time. Upcoming changes will make some resctrl files optional
>> based on a mount parameter. If optional files are to be added to the
>> default group based on the mount option, then each new file needs to
>> be created separately and call kernfs_activate() again.
>>
>> Create all files of the default resource group during resctrl
>> mount, destroyed during unmount, to avoid scattering resctrl
>> file addition across two separate code flows.
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>  2 files changed, 33 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index b09e7abd1299..44ad98f8c7af 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -611,5 +611,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>  void __init thread_throttle_mode_init(void);
>>  void __init mbm_config_rftype_init(const char *config);
>>  void rdt_staged_configs_clear(void);
>> +int rdtgroup_setup_root(struct rdt_fs_context *ctx);
>> +void rdtgroup_destroy_root(void);
>>  
> 
> From what I can tell these functions are only used in rdtgroup.c.
> Can this export be avoided by just moving these functions within
> rdtgroup.c and making them static?

Yes. It is used only in rdtgroup.c. We can make this static by adding the
prototypes of these function in the beginning of rdtgroup.c file to avoid
implicit declaration compiler errors.

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 189c51c479d3..a97118b6f9f0 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -54,6 +54,9 @@ static struct kernfs_node *kn_mondata;
 static struct seq_buf last_cmd_status;
 static char last_cmd_status_buf[512];

+static int rdtgroup_setup_root(struct rdt_fs_context *ctx);
+static void rdtgroup_destroy_root(void);
+
 struct dentry *debugfs_resctrl;

 void rdt_last_cmd_clear(void)


> 
>>  #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 80a4f76bc34b..98f9f880c30b 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2516,10 +2516,14 @@ static int rdt_get_tree(struct fs_context *fc)
>>  		goto out;
>>  	}
>>  
>> -	ret = rdt_enable_ctx(ctx);
>> +	ret = rdtgroup_setup_root(ctx);
>>  	if (ret)
>>  		goto out;
>>  
>> +	ret = rdt_enable_ctx(ctx);
>> +	if (ret)
>> +		goto out_root;
>> +
>>  	ret = schemata_list_create();
>>  	if (ret) {
>>  		schemata_list_destroy();
>> @@ -2528,6 +2532,12 @@ static int rdt_get_tree(struct fs_context *fc)
>>  
>>  	closid_init();
>>  
>> +	ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
>> +	if (ret)
>> +		goto out_schemata_free;
>> +
>> +	kernfs_activate(rdtgroup_default.kn);
>> +
>>  	ret = rdtgroup_create_info_dir(rdtgroup_default.kn);
>>  	if (ret < 0)
>>  		goto out_schemata_free;
>> @@ -2584,6 +2594,8 @@ static int rdt_get_tree(struct fs_context *fc)
>>  	schemata_list_destroy();
>>  out_ctx:
>>  	rdt_disable_ctx();
>> +out_root:
>> +	rdtgroup_destroy_root();
>>  out:
>>  	rdt_last_cmd_clear();
>>  	mutex_unlock(&rdtgroup_mutex);
>> @@ -2654,7 +2666,6 @@ static int rdt_init_fs_context(struct fs_context *fc)
>>  	if (!ctx)
>>  		return -ENOMEM;
>>  
>> -	ctx->kfc.root = rdt_root;
>>  	ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
>>  	fc->fs_private = &ctx->kfc;
>>  	fc->ops = &rdt_fs_context_ops;
>> @@ -2824,6 +2835,7 @@ static void rdt_kill_sb(struct super_block *sb)
>>  	rdt_pseudo_lock_release();
>>  	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
>>  	schemata_list_destroy();
>> +	rdtgroup_destroy_root();
>>  	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
>>  	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
>>  	static_branch_disable_cpuslocked(&rdt_enable_key);
>> @@ -3705,10 +3717,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>>  	.show_options	= rdtgroup_show_options,
>>  };
>>  
>> -static int __init rdtgroup_setup_root(void)
>> +int rdtgroup_setup_root(struct rdt_fs_context *ctx)
>>  {
>> -	int ret;
>> -
>>  	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>>  				      KERNFS_ROOT_CREATE_DEACTIVATED |
>>  				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
>> @@ -3716,6 +3726,20 @@ static int __init rdtgroup_setup_root(void)
>>  	if (IS_ERR(rdt_root))
>>  		return PTR_ERR(rdt_root);
>>  
>> +	ctx->kfc.root = rdt_root;
>> +	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
>> +
>> +	return 0;
>> +}
>> +
>> +void rdtgroup_destroy_root(void)
>> +{
>> +	kernfs_destroy_root(rdt_root);
>> +	rdtgroup_default.kn = NULL;
>> +}
>> +
>> +static void __init rdtgroup_setup_default(void)
>> +{
>>  	mutex_lock(&rdtgroup_mutex);
>>  
>>  	rdtgroup_default.closid = 0;
>> @@ -3725,19 +3749,7 @@ static int __init rdtgroup_setup_root(void)
>>  
>>  	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
>>  
>> -	ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
>> -	if (ret) {
>> -		kernfs_destroy_root(rdt_root);
>> -		goto out;
>> -	}
>> -
>> -	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
>> -	kernfs_activate(rdtgroup_default.kn);
>> -
>> -out:
>>  	mutex_unlock(&rdtgroup_mutex);
>> -
>> -	return ret;
>>  }
>>  
>>  static void domain_destroy_mon_state(struct rdt_domain *d)
>> @@ -3859,13 +3871,11 @@ int __init rdtgroup_init(void)
>>  	seq_buf_init(&last_cmd_status, last_cmd_status_buf,
>>  		     sizeof(last_cmd_status_buf));
>>  
>> -	ret = rdtgroup_setup_root();
>> -	if (ret)
>> -		return ret;
>> +	rdtgroup_setup_default();
>>  
>>  	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
>>  	if (ret)
>> -		goto cleanup_root;
>> +		return ret;
>>  
>>  	ret = register_filesystem(&rdt_fs_type);
>>  	if (ret)
>> @@ -3898,8 +3908,6 @@ int __init rdtgroup_init(void)
>>  
>>  cleanup_mountpoint:
>>  	sysfs_remove_mount_point(fs_kobj, "resctrl");
>> -cleanup_root:
>> -	kernfs_destroy_root(rdt_root);
>>  
>>  	return ret;
>>  }
>> @@ -3909,5 +3917,4 @@ void __exit rdtgroup_exit(void)
>>  	debugfs_remove_recursive(debugfs_resctrl);
>>  	unregister_filesystem(&rdt_fs_type);
>>  	sysfs_remove_mount_point(fs_kobj, "resctrl");
>> -	kernfs_destroy_root(rdt_root);
>>  }
> 
> 
> The rest of this patch looks good to me.-- 
Thanks
Babu Moger

Hi Babu,

On 8/30/2023 12:50 PM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 8/29/23 15:11, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>> The default resource group and its files are created during kernel
>>> init time. Upcoming changes will make some resctrl files optional
>>> based on a mount parameter. If optional files are to be added to the
>>> default group based on the mount option, then each new file needs to
>>> be created separately and call kernfs_activate() again.
>>>
>>> Create all files of the default resource group during resctrl
>>> mount, destroyed during unmount, to avoid scattering resctrl
>>> file addition across two separate code flows.
>>>
>>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>> ---
>>>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>>  2 files changed, 33 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index b09e7abd1299..44ad98f8c7af 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -611,5 +611,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>>  void __init thread_throttle_mode_init(void);
>>>  void __init mbm_config_rftype_init(const char *config);
>>>  void rdt_staged_configs_clear(void);
>>> +int rdtgroup_setup_root(struct rdt_fs_context *ctx);
>>> +void rdtgroup_destroy_root(void);
>>>  
>>
>> From what I can tell these functions are only used in rdtgroup.c.
>> Can this export be avoided by just moving these functions within
>> rdtgroup.c and making them static?
> 
> Yes. It is used only in rdtgroup.c. We can make this static by adding the
> prototypes of these function in the beginning of rdtgroup.c file to avoid
> implicit declaration compiler errors.

Why not just place the functions earlier in rdtgroup.c so that they are
located before all callers? 

Reinette

Hi Reinette,

On 8/30/23 15:00, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/30/2023 12:50 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 8/29/23 15:11, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>>> The default resource group and its files are created during kernel
>>>> init time. Upcoming changes will make some resctrl files optional
>>>> based on a mount parameter. If optional files are to be added to the
>>>> default group based on the mount option, then each new file needs to
>>>> be created separately and call kernfs_activate() again.
>>>>
>>>> Create all files of the default resource group during resctrl
>>>> mount, destroyed during unmount, to avoid scattering resctrl
>>>> file addition across two separate code flows.
>>>>
>>>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> ---
>>>>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>>>  2 files changed, 33 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> index b09e7abd1299..44ad98f8c7af 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -611,5 +611,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>>>  void __init thread_throttle_mode_init(void);
>>>>  void __init mbm_config_rftype_init(const char *config);
>>>>  void rdt_staged_configs_clear(void);
>>>> +int rdtgroup_setup_root(struct rdt_fs_context *ctx);
>>>> +void rdtgroup_destroy_root(void);
>>>>  
>>>
>>> From what I can tell these functions are only used in rdtgroup.c.
>>> Can this export be avoided by just moving these functions within
>>> rdtgroup.c and making them static?
>>
>> Yes. It is used only in rdtgroup.c. We can make this static by adding the
>> prototypes of these function in the beginning of rdtgroup.c file to avoid
>> implicit declaration compiler errors.
> 
> Why not just place the functions earlier in rdtgroup.c so that they are
> located before all callers? 

Couple of problems with that.
1.  rdtgroup_setup_root needs the the definition of
rdtgroup_kf_syscall_ops which is defined later in the file.

Static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
         .mkdir          = rdtgroup_mkdir,
         .rmdir          = rdtgroup_rmdir,
         .rename         = rdtgroup_rename,
         .show_options   = rdtgroup_show_options,
};

2. rdtgroup_setup_root is called in rdt_get_tree which is defined earlier
in the file.

So, this needs re-arrange of all these functions. That is reason I made
these functions global. Thought it may be too much a change for this purpose.
-- 
Thanks
Babu Moger

Hi Babu,

On 8/30/2023 2:18 PM, Moger, Babu wrote:
> On 8/30/23 15:00, Reinette Chatre wrote:
>> On 8/30/2023 12:50 PM, Moger, Babu wrote:
>>> On 8/29/23 15:11, Reinette Chatre wrote:
>>>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>>>> The default resource group and its files are created during kernel
>>>>> init time. Upcoming changes will make some resctrl files optional
>>>>> based on a mount parameter. If optional files are to be added to the
>>>>> default group based on the mount option, then each new file needs to
>>>>> be created separately and call kernfs_activate() again.
>>>>>
>>>>> Create all files of the default resource group during resctrl
>>>>> mount, destroyed during unmount, to avoid scattering resctrl
>>>>> file addition across two separate code flows.
>>>>>
>>>>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>>> ---
>>>>>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>>>>  2 files changed, 33 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>> index b09e7abd1299..44ad98f8c7af 100644
>>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>> @@ -611,5 +611,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>>>>  void __init thread_throttle_mode_init(void);
>>>>>  void __init mbm_config_rftype_init(const char *config);
>>>>>  void rdt_staged_configs_clear(void);
>>>>> +int rdtgroup_setup_root(struct rdt_fs_context *ctx);
>>>>> +void rdtgroup_destroy_root(void);
>>>>>  
>>>>
>>>> From what I can tell these functions are only used in rdtgroup.c.
>>>> Can this export be avoided by just moving these functions within
>>>> rdtgroup.c and making them static?
>>>
>>> Yes. It is used only in rdtgroup.c. We can make this static by adding the
>>> prototypes of these function in the beginning of rdtgroup.c file to avoid
>>> implicit declaration compiler errors.
>>
>> Why not just place the functions earlier in rdtgroup.c so that they are
>> located before all callers? 
> 
> Couple of problems with that.
> 1.  rdtgroup_setup_root needs the the definition of
> rdtgroup_kf_syscall_ops which is defined later in the file.
> 
> Static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>          .mkdir          = rdtgroup_mkdir,
>          .rmdir          = rdtgroup_rmdir,
>          .rename         = rdtgroup_rename,
>          .show_options   = rdtgroup_show_options,
> };
> 
> 2. rdtgroup_setup_root is called in rdt_get_tree which is defined earlier
> in the file.
> 
> So, this needs re-arrange of all these functions. That is reason I made
> these functions global. Thought it may be too much a change for this purpose.

I see, yes, to accomplish this would trigger a lot of churn and also seem
to cascade into other dependencies needing to be taken into account.
As you suggested the static declaration can be added to the top of rdtgroup.c
as proposal for the next stage.

Reinette

Hi Reinette,

On 8/30/23 17:05, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/30/2023 2:18 PM, Moger, Babu wrote:
>> On 8/30/23 15:00, Reinette Chatre wrote:
>>> On 8/30/2023 12:50 PM, Moger, Babu wrote:
>>>> On 8/29/23 15:11, Reinette Chatre wrote:
>>>>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>>>>> The default resource group and its files are created during kernel
>>>>>> init time. Upcoming changes will make some resctrl files optional
>>>>>> based on a mount parameter. If optional files are to be added to the
>>>>>> default group based on the mount option, then each new file needs to
>>>>>> be created separately and call kernfs_activate() again.
>>>>>>
>>>>>> Create all files of the default resource group during resctrl
>>>>>> mount, destroyed during unmount, to avoid scattering resctrl
>>>>>> file addition across two separate code flows.
>>>>>>
>>>>>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>>>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>>>> ---
>>>>>>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>>>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>>>>>  2 files changed, 33 insertions(+), 24 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>>> index b09e7abd1299..44ad98f8c7af 100644
>>>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>>> @@ -611,5 +611,7 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>>>>>>  void __init thread_throttle_mode_init(void);
>>>>>>  void __init mbm_config_rftype_init(const char *config);
>>>>>>  void rdt_staged_configs_clear(void);
>>>>>> +int rdtgroup_setup_root(struct rdt_fs_context *ctx);
>>>>>> +void rdtgroup_destroy_root(void);
>>>>>>  
>>>>>
>>>>> From what I can tell these functions are only used in rdtgroup.c.
>>>>> Can this export be avoided by just moving these functions within
>>>>> rdtgroup.c and making them static?
>>>>
>>>> Yes. It is used only in rdtgroup.c. We can make this static by adding the
>>>> prototypes of these function in the beginning of rdtgroup.c file to avoid
>>>> implicit declaration compiler errors.
>>>
>>> Why not just place the functions earlier in rdtgroup.c so that they are
>>> located before all callers? 
>>
>> Couple of problems with that.
>> 1.  rdtgroup_setup_root needs the the definition of
>> rdtgroup_kf_syscall_ops which is defined later in the file.
>>
>> Static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>>          .mkdir          = rdtgroup_mkdir,
>>          .rmdir          = rdtgroup_rmdir,
>>          .rename         = rdtgroup_rename,
>>          .show_options   = rdtgroup_show_options,
>> };
>>
>> 2. rdtgroup_setup_root is called in rdt_get_tree which is defined earlier
>> in the file.
>>
>> So, this needs re-arrange of all these functions. That is reason I made
>> these functions global. Thought it may be too much a change for this purpose.
> 
> I see, yes, to accomplish this would trigger a lot of churn and also seem
> to cascade into other dependencies needing to be taken into account.
> As you suggested the static declaration can be added to the top of rdtgroup.c
> as proposal for the next stage.
> 

Sure.
Thanks
Babu Moger