diff mbox series

[v10,09/10] x86/resctrl: Add support for the files for MON groups only

Message ID 20230915224227.1336967-10-babu.moger@amd.com
State New
Headers
Series x86/resctrl: Miscellaneous resctrl features |

Commit Message

Moger, Babu Sept. 15, 2023, 10:42 p.m. UTC 
  There are 3 types resctrl root files.
1. RFTYPE_BASE : Files common for both MON and CTRL groups
2. RFTYPE_BASE | RFTYPE_CTRL : Files only for CTRL groups
3. RFTYPE_BASE | RFTYPE_MON : Files only for MON groups

Files only for the MON groups are not supported now. Add the
support to create these files.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Fenghua Yu Sept. 25, 2023, 9:20 p.m. UTC | #1 
On 9/15/23 15:42, Babu Moger wrote:
> There are 3 types resctrl root files.
> 1. RFTYPE_BASE : Files common for both MON and CTRL groups
> 2. RFTYPE_BASE | RFTYPE_CTRL : Files only for CTRL groups
> 3. RFTYPE_BASE | RFTYPE_MON : Files only for MON groups
> 
> Files only for the MON groups are not supported now. Add the
> support to create these files.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>

Thanks.

-Fenghua
Reinette Chatre Sept. 27, 2023, 6:34 p.m. UTC | #2 
Hi Babu,

On 9/15/2023 3:42 PM, Babu Moger wrote:
> There are 3 types resctrl root files.

Considering patch #4, should this be "base"?

> 1. RFTYPE_BASE : Files common for both MON and CTRL groups
> 2. RFTYPE_BASE | RFTYPE_CTRL : Files only for CTRL groups

If (1) is accurate then I cannot see how (2) can be accurate.

> 3. RFTYPE_BASE | RFTYPE_MON : Files only for MON groups

Same here.

> 
> Files only for the MON groups are not supported now. Add the
> support to create these files.

This is not accurate. Files "only for the MON groups" are
actually the only monitor files that *are* supported. The
problem being fixed here is that monitor files are
not supported for CTRL_MON groups.

I made an attempt at rewriting this commit message but I am
doing it quite quickly so please do improve it:

	Files unique to monitoring groups have the
	RFTYPE_MON flag. When a new monitoring group is created
	the resctrl files with flags RFTYPE_BASE (files common
	to all resource groups) and RFTYPE_MON (files unique
	to monitoring groups) are created to support interacting with
	the new monitoring group.

	A resource group can support both monitoring and control,
	also termed a CTRL_MON resource group. CTRL_MON groups should
	get both monitoring and control resctrl files but that
	is not the case. Only the RFTYPE_BASE and RFTYPE_CTRL files
	are created for	CTRL_MON groups. This is not a problem
	because there are no monitoring specific files with the
	RFTYPE_MON flag associated with resource groups. 

	A later patch introduces the first RFTYPE_MON file for
	resource groups so fix resctrl file creation for CTRL_MON
	groups to ensure that monitoring files,	those with the
	RFTYPE_MON flag, are also created.
 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

I only have feedback about the changelog. The patch looks good to me.

Reinette
Moger, Babu Sept. 27, 2023, 9:34 p.m. UTC | #3 
Hi Reinette,

On 9/27/2023 1:34 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 9/15/2023 3:42 PM, Babu Moger wrote:
>> There are 3 types resctrl root files.
> Considering patch #4, should this be "base"?
Sure.
>
>> 1. RFTYPE_BASE : Files common for both MON and CTRL groups
>> 2. RFTYPE_BASE | RFTYPE_CTRL : Files only for CTRL groups
> If (1) is accurate then I cannot see how (2) can be accurate.

How about ?

2. RFTYPE_BASE | RFTYPE_CTRL : Files for the CTRL groups

>
>> 3. RFTYPE_BASE | RFTYPE_MON : Files only for MON groups
> Same here.

How bout?

3. RFTYPE_BASE | RFTYPE_MON : Files for the MON groups

>
>> Files only for the MON groups are not supported now. Add the
>> support to create these files.
> This is not accurate. Files "only for the MON groups" are
> actually the only monitor files that *are* supported. The
> problem being fixed here is that monitor files are
> not supported for CTRL_MON groups.
>
> I made an attempt at rewriting this commit message but I am
> doing it quite quickly so please do improve it:
>
> 	Files unique to monitoring groups have the
> 	RFTYPE_MON flag. When a new monitoring group is created
> 	the resctrl files with flags RFTYPE_BASE (files common
> 	to all resource groups) and RFTYPE_MON (files unique
> 	to monitoring groups) are created to support interacting with
> 	the new monitoring group.
>
> 	A resource group can support both monitoring and control,
> 	also termed a CTRL_MON resource group. CTRL_MON groups should
> 	get both monitoring and control resctrl files but that
> 	is not the case. Only the RFTYPE_BASE and RFTYPE_CTRL files
> 	are created for	CTRL_MON groups. This is not a problem
> 	because there are no monitoring specific files with the
> 	RFTYPE_MON flag associated with resource groups.
>
> 	A later patch introduces the first RFTYPE_MON file for
> 	resource groups so fix resctrl file creation for CTRL_MON
> 	groups to ensure that monitoring files,	those with the
> 	RFTYPE_MON flag, are also created.

Looks good with slight change(only last para. Rest looks good). Just 
removed the "fix".

A later patch introduces the first RFTYPE_MON file for
resource groups. Add the changes to create the files for CTRL_MON
groups to ensure that monitoring files,	those with the RFTYPE_MON flag,
are also created.


>   
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> I only have feedback about the changelog. The patch looks good to me.

Thanks

Babu
Reinette Chatre Sept. 27, 2023, 10:02 p.m. UTC | #4 
Hi Babu,

On 9/27/2023 2:34 PM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 9/27/2023 1:34 PM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 9/15/2023 3:42 PM, Babu Moger wrote:
>>> There are 3 types resctrl root files.
>> Considering patch #4, should this be "base"?
> Sure.
>>
>>> 1. RFTYPE_BASE : Files common for both MON and CTRL groups
>>> 2. RFTYPE_BASE | RFTYPE_CTRL : Files only for CTRL groups
>> If (1) is accurate then I cannot see how (2) can be accurate.
> 
> How about ?
> 
> 2. RFTYPE_BASE | RFTYPE_CTRL : Files for the CTRL groups

Yes, this is accurate. Even so, this snippet does not seem
necessary considering the changelog rewrite below.

> 
>>
>>> 3. RFTYPE_BASE | RFTYPE_MON : Files only for MON groups
>> Same here.
> 
> How bout?
> 
> 3. RFTYPE_BASE | RFTYPE_MON : Files for the MON groups

Same comment.

> 
>>
>>> Files only for the MON groups are not supported now. Add the
>>> support to create these files.
>> This is not accurate. Files "only for the MON groups" are
>> actually the only monitor files that *are* supported. The
>> problem being fixed here is that monitor files are
>> not supported for CTRL_MON groups.
>>
>> I made an attempt at rewriting this commit message but I am
>> doing it quite quickly so please do improve it:
>>
>>     Files unique to monitoring groups have the
>>     RFTYPE_MON flag. When a new monitoring group is created
>>     the resctrl files with flags RFTYPE_BASE (files common
>>     to all resource groups) and RFTYPE_MON (files unique
>>     to monitoring groups) are created to support interacting with
>>     the new monitoring group.
>>
>>     A resource group can support both monitoring and control,
>>     also termed a CTRL_MON resource group. CTRL_MON groups should
>>     get both monitoring and control resctrl files but that
>>     is not the case. Only the RFTYPE_BASE and RFTYPE_CTRL files
>>     are created for    CTRL_MON groups. This is not a problem
>>     because there are no monitoring specific files with the
>>     RFTYPE_MON flag associated with resource groups.
>>
>>     A later patch introduces the first RFTYPE_MON file for
>>     resource groups so fix resctrl file creation for CTRL_MON
>>     groups to ensure that monitoring files,    those with the
>>     RFTYPE_MON flag, are also created.
> 
> Looks good with slight change(only last para. Rest looks good). Just removed the "fix".
> 
> A later patch introduces the first RFTYPE_MON file for
> resource groups. Add the changes to create the files for CTRL_MON
> groups to ensure that monitoring files,    those with the RFTYPE_MON flag,
> are also created.
> 

"Add the changes to" is not necessary. With this and removing the "fix" it
can be summarized to:
	A later patch introduces the first monitoring specific (RFTYPE_MON)
	file for resource groups. Ensure that files with the RFTYPE_MON
	flag are created for CTRL_MON groups.

What do you think?

Reinette
Ilpo Järvinen Sept. 29, 2023, 3:59 p.m. UTC | #5 
On Fri, 15 Sep 2023, Babu Moger wrote:

> There are 3 types resctrl root files.
> 1. RFTYPE_BASE : Files common for both MON and CTRL groups
> 2. RFTYPE_BASE | RFTYPE_CTRL : Files only for CTRL groups
> 3. RFTYPE_BASE | RFTYPE_MON : Files only for MON groups
> 
> Files only for the MON groups are not supported now. Add the
> support to create these files.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 953b082cec8a..55d1b90f460e 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2533,6 +2533,7 @@ static void schemata_list_destroy(void)
>  static int rdt_get_tree(struct fs_context *fc)
>  {
>  	struct rdt_fs_context *ctx = rdt_fc2context(fc);
> +	unsigned long flags = RFTYPE_CTRL_BASE;
>  	struct rdt_domain *dom;
>  	struct rdt_resource *r;
>  	int ret;
> @@ -2563,7 +2564,10 @@ static int rdt_get_tree(struct fs_context *fc)
>  
>  	closid_init();
>  
> -	ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
> +	if (rdt_mon_capable)
> +		flags |= RFTYPE_MON;
> +
> +	ret = rdtgroup_add_files(rdtgroup_default.kn, flags);
>  	if (ret)
>  		goto out_schemata_free;
>  
> @@ -3253,8 +3257,8 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>  			     enum rdt_group_type rtype, struct rdtgroup **r)
>  {
>  	struct rdtgroup *prdtgrp, *rdtgrp;
> +	unsigned long files = 0;
>  	struct kernfs_node *kn;
> -	uint files = 0;
>  	int ret;
>  
>  	prdtgrp = rdtgroup_kn_lock_live(parent_kn);
> @@ -3306,10 +3310,13 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>  		goto out_destroy;
>  	}
>  
> -	if (rtype == RDTCTRL_GROUP)
> +	if (rtype == RDTCTRL_GROUP) {
>  		files = RFTYPE_BASE | RFTYPE_CTRL;
> -	else
> +		if (rdt_mon_capable)
> +			files |= RFTYPE_MON;
> +	} else {
>  		files = RFTYPE_BASE | RFTYPE_MON;
> +	}
>  
>  	ret = rdtgroup_add_files(kn, files);
>  	if (ret) {
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Moger, Babu Oct. 2, 2023, 5:26 p.m. UTC | #6 
Hi Reinette,
Forgot to respond to this.

On 9/27/23 17:02, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/27/2023 2:34 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 9/27/2023 1:34 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 9/15/2023 3:42 PM, Babu Moger wrote:
>>>> There are 3 types resctrl root files.
>>> Considering patch #4, should this be "base"?
>> Sure.
>>>
>>>> 1. RFTYPE_BASE : Files common for both MON and CTRL groups
>>>> 2. RFTYPE_BASE | RFTYPE_CTRL : Files only for CTRL groups
>>> If (1) is accurate then I cannot see how (2) can be accurate.
>>
>> How about ?
>>
>> 2. RFTYPE_BASE | RFTYPE_CTRL : Files for the CTRL groups
> 
> Yes, this is accurate. Even so, this snippet does not seem
> necessary considering the changelog rewrite below.

Sure. Removed this whole snippet.

> 
>>
>>>
>>>> 3. RFTYPE_BASE | RFTYPE_MON : Files only for MON groups
>>> Same here.
>>
>> How bout?
>>
>> 3. RFTYPE_BASE | RFTYPE_MON : Files for the MON groups
> 
> Same comment.
> 
>>
>>>
>>>> Files only for the MON groups are not supported now. Add the
>>>> support to create these files.
>>> This is not accurate. Files "only for the MON groups" are
>>> actually the only monitor files that *are* supported. The
>>> problem being fixed here is that monitor files are
>>> not supported for CTRL_MON groups.
>>>
>>> I made an attempt at rewriting this commit message but I am
>>> doing it quite quickly so please do improve it:
>>>
>>>     Files unique to monitoring groups have the
>>>     RFTYPE_MON flag. When a new monitoring group is created
>>>     the resctrl files with flags RFTYPE_BASE (files common
>>>     to all resource groups) and RFTYPE_MON (files unique
>>>     to monitoring groups) are created to support interacting with
>>>     the new monitoring group.
>>>
>>>     A resource group can support both monitoring and control,
>>>     also termed a CTRL_MON resource group. CTRL_MON groups should
>>>     get both monitoring and control resctrl files but that
>>>     is not the case. Only the RFTYPE_BASE and RFTYPE_CTRL files
>>>     are created for    CTRL_MON groups. This is not a problem
>>>     because there are no monitoring specific files with the
>>>     RFTYPE_MON flag associated with resource groups.
>>>
>>>     A later patch introduces the first RFTYPE_MON file for
>>>     resource groups so fix resctrl file creation for CTRL_MON
>>>     groups to ensure that monitoring files,    those with the
>>>     RFTYPE_MON flag, are also created.
>>
>> Looks good with slight change(only last para. Rest looks good). Just removed the "fix".
>>
>> A later patch introduces the first RFTYPE_MON file for
>> resource groups. Add the changes to create the files for CTRL_MON
>> groups to ensure that monitoring files,    those with the RFTYPE_MON flag,
>> are also created.
>>
> 
> "Add the changes to" is not necessary. With this and removing the "fix" it
> can be summarized to:
> 	A later patch introduces the first monitoring specific (RFTYPE_MON)
> 	file for resource groups. Ensure that files with the RFTYPE_MON
> 	flag are created for CTRL_MON groups.
> 
> What do you think?
> 
Yes. Looks good.
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 953b082cec8a..55d1b90f460e 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2533,6 +2533,7 @@  static void schemata_list_destroy(void)
 static int rdt_get_tree(struct fs_context *fc)
 {
 	struct rdt_fs_context *ctx = rdt_fc2context(fc);
+	unsigned long flags = RFTYPE_CTRL_BASE;
 	struct rdt_domain *dom;
 	struct rdt_resource *r;
 	int ret;
@@ -2563,7 +2564,10 @@  static int rdt_get_tree(struct fs_context *fc)
 
 	closid_init();
 
-	ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
+	if (rdt_mon_capable)
+		flags |= RFTYPE_MON;
+
+	ret = rdtgroup_add_files(rdtgroup_default.kn, flags);
 	if (ret)
 		goto out_schemata_free;
 
@@ -3253,8 +3257,8 @@  static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 			     enum rdt_group_type rtype, struct rdtgroup **r)
 {
 	struct rdtgroup *prdtgrp, *rdtgrp;
+	unsigned long files = 0;
 	struct kernfs_node *kn;
-	uint files = 0;
 	int ret;
 
 	prdtgrp = rdtgroup_kn_lock_live(parent_kn);
@@ -3306,10 +3310,13 @@  static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 		goto out_destroy;
 	}
 
-	if (rtype == RDTCTRL_GROUP)
+	if (rtype == RDTCTRL_GROUP) {
 		files = RFTYPE_BASE | RFTYPE_CTRL;
-	else
+		if (rdt_mon_capable)
+			files |= RFTYPE_MON;
+	} else {
 		files = RFTYPE_BASE | RFTYPE_MON;
+	}
 
 	ret = rdtgroup_add_files(kn, files);
 	if (ret) {