Re: [PATCH v10 02/28] x86/resctrl: Move L3 initialization into new helper function

[Reinette Chatre <reinette.chatre@intel.com>]

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