diff mbox series

[v15-RFC,1/8] x86/resctrl: Split the RDT_RESOURCE_L3 resource

Message ID 20240130222034.37181-2-tony.luck@intel.com
State New
Headers
Series Add support for Sub-NUMA cluster (SNC) systems |

Commit Message

Luck, Tony Jan. 30, 2024, 10:20 p.m. UTC 
  The RDT_RESOURCE_L3 is unique in that it is used for both monitoring
an control functions. This made sense while both uses had the same
scope. But systems with Sub-NUMA clustering enabled do not follow this
pattern.

Create a new resource: RDT_RESOURCE_L3_MON ready to take over the
monitoring functions.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/resctrl/internal.h |  1 +
 arch/x86/kernel/cpu/resctrl/core.c     | 10 ++++++++++
 2 files changed, 11 insertions(+)

Comments

Moger, Babu Feb. 9, 2024, 3:28 p.m. UTC | #1 
Hi Tony,

On 1/30/24 16:20, Tony Luck wrote:
> The RDT_RESOURCE_L3 is unique in that it is used for both monitoring
> an control functions. This made sense while both uses had the same

an/and

> scope. But systems with Sub-NUMA clustering enabled do not follow this
> pattern.
> 
> Create a new resource: RDT_RESOURCE_L3_MON ready to take over the
> monitoring functions.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>  arch/x86/kernel/cpu/resctrl/core.c     | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 52e7e7deee10..c6051bc70e96 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -429,6 +429,7 @@ DECLARE_STATIC_KEY_FALSE(rdt_alloc_enable_key);
>  extern struct dentry *debugfs_resctrl;
>  
>  enum resctrl_res_level {
> +	RDT_RESOURCE_L3_MON,
>  	RDT_RESOURCE_L3,

How about?
RDT_RESOURCE_L3,
RDT_RESOURCE_L3_MON,

>  	RDT_RESOURCE_L2,
>  	RDT_RESOURCE_MBA,
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index aa9810a64258..c50f55d7790e 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -60,6 +60,16 @@ mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m,
>  #define domain_init(id) LIST_HEAD_INIT(rdt_resources_all[id].r_resctrl.domains)
>  
>  struct rdt_hw_resource rdt_resources_all[] = {
> +	[RDT_RESOURCE_L3_MON] =
> +	{
> +		.r_resctrl = {
> +			.rid			= RDT_RESOURCE_L3_MON,
> +			.name			= "L3",

L3_MON ?

> +			.cache_level		= 3,
> +			.domains		= domain_init(RDT_RESOURCE_L3_MON),
> +			.fflags			= RFTYPE_RES_CACHE,
> +		},
> +	},
>  	[RDT_RESOURCE_L3] =
>  	{
>  		.r_resctrl = {
Luck, Tony Feb. 9, 2024, 6:44 p.m. UTC | #2 
On Fri, Feb 09, 2024 at 09:28:16AM -0600, Moger, Babu wrote:
> >  enum resctrl_res_level {
> > +	RDT_RESOURCE_L3_MON,
> >  	RDT_RESOURCE_L3,
> 
> How about?
> RDT_RESOURCE_L3,
> RDT_RESOURCE_L3_MON,

Does the order matter? I put the L3_MON one first because historically
cache occupancy was the first resctrl tool. But if you have a better
argument for the order, then I can change it.

> >  	RDT_RESOURCE_L2,
> >  	RDT_RESOURCE_MBA,
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index aa9810a64258..c50f55d7790e 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -60,6 +60,16 @@ mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m,
> >  #define domain_init(id) LIST_HEAD_INIT(rdt_resources_all[id].r_resctrl.domains)
> >  
> >  struct rdt_hw_resource rdt_resources_all[] = {
> > +	[RDT_RESOURCE_L3_MON] =
> > +	{
> > +		.r_resctrl = {
> > +			.rid			= RDT_RESOURCE_L3_MON,
> > +			.name			= "L3",
> 
> L3_MON ?

That was my first choice too. But I found:

$ ls /sys/fs/resctrl/info
L3  L3_MON_MON  last_cmd_status  MB

This would be easy to fix ... just change this code to not append
an extra "_MON" to the directory name:

        for_each_mon_capable_rdt_resource(r) {
                fflags = r->fflags | RFTYPE_MON_INFO;
                sprintf(name, "%s_MON", r->name);
                ret = rdtgroup_mkdir_info_resdir(r, name, fflags);
                if (ret)
                        goto out_destroy;
        }

But I also saw this:
$ ls /sys/fs/resctrl/mon_data/
mon_L3_MON_00  mon_L3_MON_01

which didn't seem to have an easy fix. So I took the easy route and left
the ".name" field as "L3_MON".

-Tony
Moger, Babu Feb. 9, 2024, 7:40 p.m. UTC | #3 
On 2/9/24 12:44, Tony Luck wrote:
> On Fri, Feb 09, 2024 at 09:28:16AM -0600, Moger, Babu wrote:
>>>  enum resctrl_res_level {
>>> +	RDT_RESOURCE_L3_MON,
>>>  	RDT_RESOURCE_L3,
>>
>> How about?
>> RDT_RESOURCE_L3,
>> RDT_RESOURCE_L3_MON,
> 
> Does the order matter? I put the L3_MON one first because historically
> cache occupancy was the first resctrl tool. But if you have a better
> argument for the order, then I can change it.

That is fine. No need to change.

> 
>>>  	RDT_RESOURCE_L2,
>>>  	RDT_RESOURCE_MBA,
>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>> index aa9810a64258..c50f55d7790e 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>> @@ -60,6 +60,16 @@ mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m,
>>>  #define domain_init(id) LIST_HEAD_INIT(rdt_resources_all[id].r_resctrl.domains)
>>>  
>>>  struct rdt_hw_resource rdt_resources_all[] = {
>>> +	[RDT_RESOURCE_L3_MON] =
>>> +	{
>>> +		.r_resctrl = {
>>> +			.rid			= RDT_RESOURCE_L3_MON,
>>> +			.name			= "L3",
>>
>> L3_MON ?
> 
> That was my first choice too. But I found:
> 
> $ ls /sys/fs/resctrl/info
> L3  L3_MON_MON  last_cmd_status  MB
> 
> This would be easy to fix ... just change this code to not append
> an extra "_MON" to the directory name:
> 
>         for_each_mon_capable_rdt_resource(r) {
>                 fflags = r->fflags | RFTYPE_MON_INFO;
>                 sprintf(name, "%s_MON", r->name);
>                 ret = rdtgroup_mkdir_info_resdir(r, name, fflags);
>                 if (ret)
>                         goto out_destroy;
>         }
> 
> But I also saw this:
> $ ls /sys/fs/resctrl/mon_data/
> mon_L3_MON_00  mon_L3_MON_01
> 
> which didn't seem to have an easy fix. So I took the easy route and left
> the ".name" field as "L3_MON".
> 

Ok. Sounds good.
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 52e7e7deee10..c6051bc70e96 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -429,6 +429,7 @@  DECLARE_STATIC_KEY_FALSE(rdt_alloc_enable_key);
 extern struct dentry *debugfs_resctrl;
 
 enum resctrl_res_level {
+	RDT_RESOURCE_L3_MON,
 	RDT_RESOURCE_L3,
 	RDT_RESOURCE_L2,
 	RDT_RESOURCE_MBA,
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index aa9810a64258..c50f55d7790e 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -60,6 +60,16 @@  mba_wrmsr_amd(struct rdt_domain *d, struct msr_param *m,
 #define domain_init(id) LIST_HEAD_INIT(rdt_resources_all[id].r_resctrl.domains)
 
 struct rdt_hw_resource rdt_resources_all[] = {
+	[RDT_RESOURCE_L3_MON] =
+	{
+		.r_resctrl = {
+			.rid			= RDT_RESOURCE_L3_MON,
+			.name			= "L3",
+			.cache_level		= 3,
+			.domains		= domain_init(RDT_RESOURCE_L3_MON),
+			.fflags			= RFTYPE_RES_CACHE,
+		},
+	},
 	[RDT_RESOURCE_L3] =
 	{
 		.r_resctrl = {