diff mbox series

[v6,6/8] x86/resctrl: Move default control group creation during mount

Message ID 168980893158.1619861.8945141367681744527.stgit@bmoger-ubuntu
State New
Headers
Series x86/resctrl: Miscellaneous resctrl features |

Commit Message

Moger, Babu July 19, 2023, 11:22 p.m. UTC 
  The resctrl default control group is created during kernel init time. If
the new files are to be added to the default group during the mount based
on the mount option, then each file needs to be created separately and
call kernfs_activate.

This can avoided if all the files are created during the mount and
destroyed during the umount. Move the root and default group creation
in rdt_get_tree and removal in rdt_kill_sb.

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

Comments

Reinette Chatre Aug. 4, 2023, 8:42 p.m. UTC | #1 
Hi Babu,

On 7/19/2023 4:22 PM, Babu Moger wrote:
> The resctrl default control group is created during kernel init time. If
> the new files are to be added to the default group during the mount based
> on the mount option, then each file needs to be created separately and
> call kernfs_activate.
> 
> This can avoided if all the files are created during the mount and
> destroyed during the umount. Move the root and default group creation
> in rdt_get_tree and removal in rdt_kill_sb.

Please use () to indicate function names.

> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> @@ -3704,10 +3720,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(void)
>  {
> -	int ret;
> -
>  	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>  				      KERNFS_ROOT_CREATE_DEACTIVATED |
>  				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
> @@ -3715,28 +3729,17 @@ static int __init rdtgroup_setup_root(void)
>  	if (IS_ERR(rdt_root))
>  		return PTR_ERR(rdt_root);
>  
> -	mutex_lock(&rdtgroup_mutex);
> -
>  	rdtgroup_default.closid = 0;
>  	rdtgroup_default.mon.rmid = 0;
>  	rdtgroup_default.type = RDTCTRL_GROUP;
> +	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
>  	INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list);
>  
>  	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;
> +	return 0;
>  }

I am missing something here. Why is it now needed to re-initialize
and add default resource group on every mount of resctrl? I expected
only the kernfs related changes to move.

Reinette
Moger, Babu Aug. 8, 2023, 4:22 p.m. UTC | #2 
Hi Reinette,

On 8/4/23 15:42, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/19/2023 4:22 PM, Babu Moger wrote:
>> The resctrl default control group is created during kernel init time. If
>> the new files are to be added to the default group during the mount based
>> on the mount option, then each file needs to be created separately and
>> call kernfs_activate.
>>
>> This can avoided if all the files are created during the mount and
>> destroyed during the umount. Move the root and default group creation
>> in rdt_get_tree and removal in rdt_kill_sb.
> 
> Please use () to indicate function names.

Sure.

> 
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> ...
> 
>> @@ -3704,10 +3720,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(void)
>>  {
>> -	int ret;
>> -
>>  	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>>  				      KERNFS_ROOT_CREATE_DEACTIVATED |
>>  				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
>> @@ -3715,28 +3729,17 @@ static int __init rdtgroup_setup_root(void)
>>  	if (IS_ERR(rdt_root))
>>  		return PTR_ERR(rdt_root);
>>  
>> -	mutex_lock(&rdtgroup_mutex);
>> -
>>  	rdtgroup_default.closid = 0;
>>  	rdtgroup_default.mon.rmid = 0;
>>  	rdtgroup_default.type = RDTCTRL_GROUP;
>> +	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
>>  	INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list);
>>  
>>  	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;
>> +	return 0;
>>  }
> 
> I am missing something here. Why is it now needed to re-initialize
> and add default resource group on every mount of resctrl? I expected
> only the kernfs related changes to move.

Yes. We can do that. I think I started with the previous version and went
that route. We dont have to change the default group initialization.
Will send new version soon.
Thanks
Babu Moger
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 783c3e36633c..885b656d1088 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -601,5 +601,6 @@  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(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 9a7204f71d2d..e0570bce76a2 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2512,10 +2512,16 @@  static int rdt_get_tree(struct fs_context *fc)
 		goto out;
 	}
 
-	ret = rdt_enable_ctx(ctx);
+	ret = rdtgroup_setup_root();
 	if (ret)
 		goto out;
 
+	ctx->kfc.root = rdt_root;
+
+	ret = rdt_enable_ctx(ctx);
+	if (ret)
+		goto out_root;
+
 	ret = schemata_list_create();
 	if (ret) {
 		schemata_list_destroy();
@@ -2524,6 +2530,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;
@@ -2582,6 +2594,9 @@  static int rdt_get_tree(struct fs_context *fc)
 	if (ctx->enable_mba_mbps)
 		set_mba_sc(false);
 	cdp_disable_all();
+out_root:
+	list_del(&rdtgroup_default.rdtgroup_list);
+	kernfs_destroy_root(rdt_root);
 out:
 	rdt_last_cmd_clear();
 	mutex_unlock(&rdtgroup_mutex);
@@ -2652,7 +2667,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;
@@ -2821,7 +2835,9 @@  static void rdt_kill_sb(struct super_block *sb)
 	cdp_disable_all();
 	rmdir_all_sub();
 	rdt_pseudo_lock_release();
-	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
+	/* Remove the default group and cleanup the root */
+	list_del(&rdtgroup_default.rdtgroup_list);
+	kernfs_destroy_root(rdt_root);
 	schemata_list_destroy();
 	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
 	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
@@ -3704,10 +3720,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(void)
 {
-	int ret;
-
 	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
 				      KERNFS_ROOT_CREATE_DEACTIVATED |
 				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
@@ -3715,28 +3729,17 @@  static int __init rdtgroup_setup_root(void)
 	if (IS_ERR(rdt_root))
 		return PTR_ERR(rdt_root);
 
-	mutex_lock(&rdtgroup_mutex);
-
 	rdtgroup_default.closid = 0;
 	rdtgroup_default.mon.rmid = 0;
 	rdtgroup_default.type = RDTCTRL_GROUP;
+	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
 	INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list);
 
 	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;
+	return 0;
 }
 
 static void domain_destroy_mon_state(struct rdt_domain *d)
@@ -3858,13 +3861,9 @@  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;
-
 	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
 	if (ret)
-		goto cleanup_root;
+		return ret;
 
 	ret = register_filesystem(&rdt_fs_type);
 	if (ret)
@@ -3897,9 +3896,6 @@  int __init rdtgroup_init(void)
 
 cleanup_mountpoint:
 	sysfs_remove_mount_point(fs_kobj, "resctrl");
-cleanup_root:
-	kernfs_destroy_root(rdt_root);
-
 	return ret;
 }
 
@@ -3908,5 +3904,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);
 }