All resctrl monitor events are associated with the L3 resource, but
this is about to change.

To prepare for additional types of monitoring domains, move open coded L3
resource monitoring domain initialization from domain_add_cpu_mon() into
a new helper l3_mon_domain_setup() called by domain_add_cpu_mon().

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/resctrl/core.c | 57 +++++++++++++++++-------------
 1 file changed, 32 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 8be2619db2e7..055df4d406d0 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -496,37 +496,13 @@ static void domain_add_cpu_ctrl(int cpu, struct rdt_resource *r)
 	}
 }
 
-static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
+static void l3_mon_domain_setup(int cpu, int id, struct rdt_resource *r, struct list_head *add_pos)
 {
-	int id = get_domain_id_from_scope(cpu, r->mon_scope);
-	struct list_head *add_pos = NULL;
 	struct rdt_hw_mon_domain *hw_dom;
-	struct rdt_domain_hdr *hdr;
 	struct rdt_mon_domain *d;
 	struct cacheinfo *ci;
 	int err;
 
-	lockdep_assert_held(&domain_list_lock);
-
-	if (id < 0) {
-		pr_warn_once("Can't find monitor domain id for CPU:%d scope:%d for resource %s\n",
-			     cpu, r->mon_scope, r->name);
-		return;
-	}
-
-	hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
-	if (hdr) {
-		if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, r->rid))
-			return;
-		d = container_of(hdr, struct rdt_mon_domain, hdr);
-
-		cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
-		/* Update the mbm_assign_mode state for the CPU if supported */
-		if (r->mon.mbm_cntr_assignable)
-			resctrl_arch_mbm_cntr_assign_set_one(r);
-		return;
-	}
-
 	hw_dom = kzalloc_node(sizeof(*hw_dom), GFP_KERNEL, cpu_to_node(cpu));
 	if (!hw_dom)
 		return;
@@ -565,6 +541,37 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
 	}
 }
 
+static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
+{
+	int id = get_domain_id_from_scope(cpu, r->mon_scope);
+	struct list_head *add_pos = NULL;
+	struct rdt_domain_hdr *hdr;
+
+	lockdep_assert_held(&domain_list_lock);
+
+	if (id < 0) {
+		pr_warn_once("Can't find monitor domain id for CPU:%d scope:%d for resource %s\n",
+			     cpu, r->mon_scope, r->name);
+		return;
+	}
+
+	hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
+	if (hdr) {
+		cpumask_set_cpu(cpu, &hdr->cpu_mask);
+
+		return;
+	}
+
+	switch (r->rid) {
+	case RDT_RESOURCE_L3:
+		l3_mon_domain_setup(cpu, id, r, add_pos);
+		break;
+	default:
+		pr_warn_once("Unknown resource rid=%d\n", r->rid);
+		break;
+	}
+}
+
 static void domain_add_cpu(int cpu, struct rdt_resource *r)
 {
 	if (r->alloc_capable)
-- 
2.51.0

Hi Tony,

On 9/12/25 3:10 PM, Tony Luck wrote:
> All resctrl monitor events are associated with the L3 resource, but
> this is about to change.

Please see Boris's feedback about changelogs in [1]. To address that,
please rework the changelogs to not have so much copy&pasted context 
in patches. 

> 
> To prepare for additional types of monitoring domains, move open coded L3
> resource monitoring domain initialization from domain_add_cpu_mon() into
> a new helper l3_mon_domain_setup() called by domain_add_cpu_mon().
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/kernel/cpu/resctrl/core.c | 57 +++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 8be2619db2e7..055df4d406d0 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -496,37 +496,13 @@ static void domain_add_cpu_ctrl(int cpu, struct rdt_resource *r)
>  	}
>  }
>  
> -static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> +static void l3_mon_domain_setup(int cpu, int id, struct rdt_resource *r, struct list_head *add_pos)
>  {
> -	int id = get_domain_id_from_scope(cpu, r->mon_scope);
> -	struct list_head *add_pos = NULL;
>  	struct rdt_hw_mon_domain *hw_dom;
> -	struct rdt_domain_hdr *hdr;
>  	struct rdt_mon_domain *d;
>  	struct cacheinfo *ci;
>  	int err;
>  
> -	lockdep_assert_held(&domain_list_lock);
> -
> -	if (id < 0) {
> -		pr_warn_once("Can't find monitor domain id for CPU:%d scope:%d for resource %s\n",
> -			     cpu, r->mon_scope, r->name);
> -		return;
> -	}
> -
> -	hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
> -	if (hdr) {
> -		if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, r->rid))
> -			return;
> -		d = container_of(hdr, struct rdt_mon_domain, hdr);
> -
> -		cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
> -		/* Update the mbm_assign_mode state for the CPU if supported */
> -		if (r->mon.mbm_cntr_assignable)
> -			resctrl_arch_mbm_cntr_assign_set_one(r);

Rebase error? Note the assignable counter initialization done on CPU online ...

> -		return;
> -	}
> -
>  	hw_dom = kzalloc_node(sizeof(*hw_dom), GFP_KERNEL, cpu_to_node(cpu));
>  	if (!hw_dom)
>  		return;
> @@ -565,6 +541,37 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>  	}
>  }
>  
> +static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> +{
> +	int id = get_domain_id_from_scope(cpu, r->mon_scope);
> +	struct list_head *add_pos = NULL;
> +	struct rdt_domain_hdr *hdr;
> +
> +	lockdep_assert_held(&domain_list_lock);
> +
> +	if (id < 0) {
> +		pr_warn_once("Can't find monitor domain id for CPU:%d scope:%d for resource %s\n",
> +			     cpu, r->mon_scope, r->name);
> +		return;
> +	}
> +
> +	hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
> +	if (hdr) {
> +		cpumask_set_cpu(cpu, &hdr->cpu_mask);
> +

... assignable counter initialization no longer done on CPU online.

Looking closer the l3_mon_domain_setup() also now contains assignable counter
initialization that is gated by a RDT_RESOURCE_L3 check. Considering the flow
I think it may thus be simpler and consistent to not return here
but instead have the additional initialization done in resource specific
areas below. 

> +		return;
> +	}
> +
> +	switch (r->rid) {
> +	case RDT_RESOURCE_L3:

Something like:
		if (hdr) {
			/* do resource specific CPU initialization here */
			return;
		}

> +		l3_mon_domain_setup(cpu, id, r, add_pos);
> +		break;
> +	default:
> +		pr_warn_once("Unknown resource rid=%d\n", r->rid);
> +		break;
> +	}
> +}
> +
>  static void domain_add_cpu(int cpu, struct rdt_resource *r)
>  {
>  	if (r->alloc_capable)

Reinette

On Thu, Sep 18, 2025 at 02:49:05PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 9/12/25 3:10 PM, Tony Luck wrote:
> > All resctrl monitor events are associated with the L3 resource, but
> > this is about to change.
> 
> Please see Boris's feedback about changelogs in [1]. To address that,
> please rework the changelogs to not have so much copy&pasted context 
> in patches. 

I understand that Boris doesn't want to see large amounts of text copied
from the cover letter into each patch. But there is still a need to meet the
"Describe your problem" requirement for a good commit message.

Several of the prepatory patches in this series make changes to expand the
capabilities of fs/resctrl to handle monitor events from resources other
than RDT_RESOURCE_L3.

Is a single short sentence repeated in several patches "too much"?

> 
> > 
> > To prepare for additional types of monitoring domains, move open coded L3
> > resource monitoring domain initialization from domain_add_cpu_mon() into
> > a new helper l3_mon_domain_setup() called by domain_add_cpu_mon().
> > 
> > Signed-off-by: Tony Luck <tony.luck@intel.com>
> > ---
> >  arch/x86/kernel/cpu/resctrl/core.c | 57 +++++++++++++++++-------------
> >  1 file changed, 32 insertions(+), 25 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index 8be2619db2e7..055df4d406d0 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -496,37 +496,13 @@ static void domain_add_cpu_ctrl(int cpu, struct rdt_resource *r)
> >  	}
> >  }
> >  
> > -static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> > +static void l3_mon_domain_setup(int cpu, int id, struct rdt_resource *r, struct list_head *add_pos)
> >  {
> > -	int id = get_domain_id_from_scope(cpu, r->mon_scope);
> > -	struct list_head *add_pos = NULL;
> >  	struct rdt_hw_mon_domain *hw_dom;
> > -	struct rdt_domain_hdr *hdr;
> >  	struct rdt_mon_domain *d;
> >  	struct cacheinfo *ci;
> >  	int err;
> >  
> > -	lockdep_assert_held(&domain_list_lock);
> > -
> > -	if (id < 0) {
> > -		pr_warn_once("Can't find monitor domain id for CPU:%d scope:%d for resource %s\n",
> > -			     cpu, r->mon_scope, r->name);
> > -		return;
> > -	}
> > -
> > -	hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
> > -	if (hdr) {
> > -		if (!domain_header_is_valid(hdr, RESCTRL_MON_DOMAIN, r->rid))
> > -			return;
> > -		d = container_of(hdr, struct rdt_mon_domain, hdr);
> > -
> > -		cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
> > -		/* Update the mbm_assign_mode state for the CPU if supported */
> > -		if (r->mon.mbm_cntr_assignable)
> > -			resctrl_arch_mbm_cntr_assign_set_one(r);
> 
> Rebase error? Note the assignable counter initialization done on CPU online ...

Right. This code was lost in the rebase.
> 
> > -		return;
> > -	}
> > -
> >  	hw_dom = kzalloc_node(sizeof(*hw_dom), GFP_KERNEL, cpu_to_node(cpu));
> >  	if (!hw_dom)
> >  		return;
> > @@ -565,6 +541,37 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> >  	}
> >  }
> >  
> > +static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
> > +{
> > +	int id = get_domain_id_from_scope(cpu, r->mon_scope);
> > +	struct list_head *add_pos = NULL;
> > +	struct rdt_domain_hdr *hdr;
> > +
> > +	lockdep_assert_held(&domain_list_lock);
> > +
> > +	if (id < 0) {
> > +		pr_warn_once("Can't find monitor domain id for CPU:%d scope:%d for resource %s\n",
> > +			     cpu, r->mon_scope, r->name);
> > +		return;
> > +	}
> > +
> > +	hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
> > +	if (hdr) {
> > +		cpumask_set_cpu(cpu, &hdr->cpu_mask);
> > +
> 
> ... assignable counter initialization no longer done on CPU online.
> 
> Looking closer the l3_mon_domain_setup() also now contains assignable counter
> initialization that is gated by a RDT_RESOURCE_L3 check. Considering the flow
> I think it may thus be simpler and consistent to not return here
> but instead have the additional initialization done in resource specific
> areas below. 

Yes.
> 
> > +		return;
> > +	}
> > +
> > +	switch (r->rid) {
> > +	case RDT_RESOURCE_L3:
> 
> Something like:
> 		if (hdr) {
> 			/* do resource specific CPU initialization here */
> 			return;
> 		}

In this specific case the resource specific operation needs to happen
on every CPU (it updates the per-CPU MSR_IA32_L3_QOS_EXT_CFG). So I
think the code fragment needs to be:

	switch (r->rid) {
	case RDT_RESOURCE_L3:
		/* Update the mbm_assign_mode state for the CPU if supported */
		if (r->mon.mbm_cntr_assignable)
			resctrl_arch_mbm_cntr_assign_set_one(r);
		if (!hdr)
			l3_mon_domain_setup(cpu, id, r, add_pos);
		break;

> 
> > +		l3_mon_domain_setup(cpu, id, r, add_pos);
> > +		break;
> > +	default:
> > +		pr_warn_once("Unknown resource rid=%d\n", r->rid);
> > +		break;
> > +	}
> > +}
> > +
> >  static void domain_add_cpu(int cpu, struct rdt_resource *r)
> >  {
> >  	if (r->alloc_capable)
> 
> Reinette

-Tony

On Fri, Sep 19, 2025 at 11:09:54AM -0700, Luck, Tony wrote:
> I understand that Boris doesn't want to see large amounts of text copied
> from the cover letter into each patch. But there is still a need to meet the
> "Describe your problem" requirement for a good commit message.
> 
> Several of the prepatory patches in this series make changes to expand the
> capabilities of fs/resctrl to handle monitor events from resources other
> than RDT_RESOURCE_L3.
> 
> Is a single short sentence repeated in several patches "too much"?

It probably isn't too much but having boilerplate code in commit messages
feels to the reader - at least to me - like: lemme jump over the boilerplate
code in order to go to the meat of the change. And then the meat of the change
is basically explaining the diff. And then I'm left scratching my head, why is
this even in my mbox.

I dunno, maybe it is just me but I end up doing that pretty often. :-\

For example, this particular commit I'd write somewhere along those lines:

"Carve out the resource monitoring domain init code into a separate helper in
order to be able to initialize new types of monitoring domains besides the
usual L3 ones."

Or so.

Short'n'sweet.

You don't explain what the code does as that should be visible from the diff
itself. You're explaining the why: "hey, I need this code into a separate
helper in order to do bla, so I'm carving it out."

That's it.

IOW, you don't necessarily have to go through the common steps of describing
the problem first, especially when you're doing preparatory, refactoring
patches.

Reading the next patch, the commit message is perfectly fine even without:

"All monitoring events are associated with the L3 resource."

Patch 4 doesn't have a boilerplate sentence and it is perfectly fine as it is.

Patch 5 you can start like this:

"Up until now, all monitoring events were associated with the L3 resource and
it made sense to use the L3 specific "struct rdt_mon_domain *" arguments to
functions manipulating domains...."

And even if you have the gist of the boilerplate, it reads better because it
is not simply slapped there mechanically.

This is not needed:

"Update kerneldoc for mon_data::sum to note that it is only used for
RDT_RESOURCE_L3."

because it is in the diff. Unless there's a reason for it to be called out in
the commit message...? And now I'm wondering why is this called out...

See what I mean?

Patch 6 I'd write like this:

"Use a generic struct rdt_domain_hdr representing a generic domain header in
struct rmid_read in order to support other telemetry events' domains besides
an L3 one. Adjust the code interacting with it to the new struct layout."

Something like that.

IOW, this is basically explaining the intention and *why* the patch is there.
No need for "for the calls from mon_event_read() via smp_call*() to
__mon_event_count() and resctrl_arch_rmid_read() and
resctrl_arch_cntr_read()." as that is functionality details. If one is really
interested, one can apply the patch and stare at it but the *why* is a lot
more important.

Dunno, this is ofc subjective but IMNSVHO, writing things this way makes it
a lot easier to review and understand the why. The what one can figure out.

And *if* one can't figure out the what because there's something non-trivial,
then by all means, call that out in the commit message.

And so on...

I hope I'm making some sense here.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Hi Tony,

On 9/19/25 11:09 AM, Luck, Tony wrote:
> On Thu, Sep 18, 2025 at 02:49:05PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 9/12/25 3:10 PM, Tony Luck wrote:
>>> All resctrl monitor events are associated with the L3 resource, but
>>> this is about to change.
>>
>> Please see Boris's feedback about changelogs in [1]. To address that,
>> please rework the changelogs to not have so much copy&pasted context 
>> in patches. 
> 
> I understand that Boris doesn't want to see large amounts of text copied
> from the cover letter into each patch. But there is still a need to meet the
> "Describe your problem" requirement for a good commit message.
> 
> Several of the prepatory patches in this series make changes to expand the
> capabilities of fs/resctrl to handle monitor events from resources other
> than RDT_RESOURCE_L3.
> 
> Is a single short sentence repeated in several patches "too much"?

You are correct that "repetitive" is subjective and "too much" is not
deterministic. Yet, we received a clear message that repetitive boilerplate is
really annoying. To me, five of the first seven patches starting with the same sentence
is close if not firmly in the repetitive boilerplate category. I've already highlighted
that that sentence can just be dropped from patch #3 and looking ahead, also from
patch #7, without losing context. Those are simple changes and I believe the
changelogs can be further reworked to improve the context to be unique per patch.

On a high level: we received valuable information about expectations from
changelogs. While I agree this is subjective, I am not interested in probes to
determine what the tolerance for repetition is but instead I intend to just take
the feedback to heart and work towards avoiding repetition.

>>> To prepare for additional types of monitoring domains, move open coded L3
>>> resource monitoring domain initialization from domain_add_cpu_mon() into
>>> a new helper l3_mon_domain_setup() called by domain_add_cpu_mon().
>>>
>>> Signed-off-by: Tony Luck <tony.luck@intel.com>
>>> ---

...
.
>>
>>> +		return;
>>> +	}
>>> +
>>> +	switch (r->rid) {
>>> +	case RDT_RESOURCE_L3:
>>
>> Something like:
>> 		if (hdr) {
>> 			/* do resource specific CPU initialization here */
>> 			return;
>> 		}
> 
> In this specific case the resource specific operation needs to happen
> on every CPU (it updates the per-CPU MSR_IA32_L3_QOS_EXT_CFG). So I
> think the code fragment needs to be:
> 
> 	switch (r->rid) {
> 	case RDT_RESOURCE_L3:
> 		/* Update the mbm_assign_mode state for the CPU if supported */
> 		if (r->mon.mbm_cntr_assignable)
> 			resctrl_arch_mbm_cntr_assign_set_one(r);
> 		if (!hdr)
> 			l3_mon_domain_setup(cpu, id, r, add_pos);
> 		break;
> 

ok. I think this will work for this specific initialization. I do not think this is
appropriate as a general pattern since the per-CPU initialization may be called with
and without a domain structure. 

>>
>>> +		l3_mon_domain_setup(cpu, id, r, add_pos);
>>> +		break;
>>> +	default:
>>> +		pr_warn_once("Unknown resource rid=%d\n", r->rid);
>>> +		break;
>>> +	}
>>> +}
>>> +

Reinette