Re: [PATCH v13 23/27] x86/resctrl: Introduce mbm_L3_assignments to list assignments in a group

["Moger, Babu" <bmoger@amd.com>]

Hi Reinette,

On 5/22/2025 11:47 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/15/25 3:52 PM, Babu Moger wrote:
>> Introduce the interface to display the assignment states for each group
>> when mbm_cntr_assign mode is enabled.
>>
>> The list is displayed in the following format:
>> <Event configuration>:<Domain id>=<Assignment type>
> 
> Should this just be <Event>? The information is just the event name, not
> its configuration that will be in the "event_filter" file.

Yes. Sure.

> 
>>
>> Event configuration: A valid event configuration listed in the
>> /sys/fs/resctrl/info/L3_MON/counter_configs directory.
>>
>> Domain ID: A valid domain ID number.
>>
>> The assignment type can be one of the following:
>>
>> _ : No event configuration assigned
>>
>> e : Event configuration assigned in exclusive mode
>>

This needs to change as well based on comments below(resctrl.rst).
This is note for me.

>> Example:
>> $cd /sys/fs/resctrl
>> $cat mbm_L3_assignments
>> mbm_total_bytes:0=e;1=e
>> mbm_local_bytes:0=e;1=e
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v13: Changelog update.
>>       Few changes in mbm_L3_assignments_show() after moving the event config to evt_list.
>>       Resolved conflicts caused by the recent FS/ARCH code restructure.
>>       The rdtgroup.c/monitor.c files have been split between the FS and ARCH directories.
>>
>> v12: New patch:
>>       Assignment interface moved inside the group based the discussion
>>       https://lore.kernel.org/lkml/CALPaoCiii0vXOF06mfV=kVLBzhfNo0SFqt4kQGwGSGVUqvr2Dg@mail.gmail.com/#t
>> ---
>>   Documentation/filesystems/resctrl.rst | 28 +++++++++++++++
>>   fs/resctrl/monitor.c                  |  1 +
>>   fs/resctrl/rdtgroup.c                 | 52 +++++++++++++++++++++++++++
>>   3 files changed, 81 insertions(+)
>>
>> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
>> index 356f1f918a86..2350c1f21f4e 100644
>> --- a/Documentation/filesystems/resctrl.rst
>> +++ b/Documentation/filesystems/resctrl.rst
>> @@ -504,6 +504,34 @@ When the "mba_MBps" mount option is used all CTRL_MON groups will also contain:
>>   	/sys/fs/resctrl/info/L3_MON/mon_features changes the input
>>   	event.
>>   
>> +"mbm_L3_assignments":
>> +	This interface file is created when the mbm_cntr_assign mode is supported
> 
> "This interface file is created when" -> "Exists when mbm_cntr_assign mode is supported"?

Sure,

> 
>> +	and shows the assignment status for each group.
> 
> This doc is in the portion documenting files in monitor groups. So rather:
> "the assignment status for each group" -> "the counter assignment status for the MON group"?

ok.

> 
>> +
>> +	The assignment list is displayed in the following format:
>> +
>> +	<Event configuration>:<Domain id>=<Assignment type>
> 
> <Event configuration> -> <Event>
> 
Ok.

>> +
>> +	Event configuration: A valid event configuration listed in the
> 
> "A valid event in the /sys/fs/resctrl/info/L3_MON/event_configs directory"

Sure.

> 
>> +	/sys/fs/resctrl/info/L3_MON/counter_configs directory.
>> +
>> +	Domain ID: A valid domain ID number.
> 
> "A valid domain ID"
> 
>> +
>> +	Assignment types:
>> +
>> +	_ : No event configuration assigned
> 
> hmmm ... since the line has event as first field, would this not reflect the
> counter? That is "No counter assigned"

Sure.

> 
>> +
>> +	e : Event configuration assigned in exclusive mode
> 
> "Counter assigned exclusively"? (with exclusive defined somewhere)

Sure.

> 
>> +
>> +	Example:
>> +	To list the assignment states for the default group.
> 
> "the counter assignment states"?

ok

> 
>> +	::
>> +
>> +	  # cd /sys/fs/resctrl
>> +	  # cat mbm_L3_assignments
>> +	    mbm_total_bytes:0=e;1=e
>> +	    mbm_local_bytes:0=e;1=e
>> +
>>   Resource allocation rules
>>   -------------------------
>>   
>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index 5f6c4b662f3b..b982540ce4e3 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -935,6 +935,7 @@ int resctrl_mon_resource_init(void)
>>   		resctrl_file_fflags_init("event_filter", RFTYPE_ASSIGN_CONFIG);
>>   		resctrl_file_fflags_init("mbm_assign_on_mkdir", RFTYPE_MON_INFO |
>>   					 RFTYPE_RES_CACHE);
>> +		resctrl_file_fflags_init("mbm_L3_assignments", RFTYPE_MON_BASE);
>>   	}
>>   
>>   	return 0;
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 931ea355f159..8d970b99bbbd 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -2080,6 +2080,52 @@ static ssize_t resctrl_mbm_assign_on_mkdir_write(struct kernfs_open_file *of,
>>   	return ret ?: nbytes;
>>   }
>>   
>> +static int mbm_L3_assignments_show(struct kernfs_open_file *of, struct seq_file *s, void *v)
>> +{
>> +	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>> +	struct rdt_mon_domain *d;
>> +	struct rdtgroup *rdtgrp;
>> +	struct mon_evt *mevt;
>> +	int ret = 0;
>> +	bool sep;
>> +
>> +	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>> +	if (!rdtgrp)
>> +		return -ENOENT;
> 
> Missing a rdtgroup_kn_unlock()?

Yes. Will add it.

> 
>> +
>> +	rdt_last_cmd_clear();
>> +	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
>> +		rdt_last_cmd_puts("mbm_cntr_assign mode not enabled\n");
>> +		ret = -ENOENT;
>> +		goto assign_out;
> 
> grep goto fs/resctrl/rdtgroup.c

Will change it to "out_assign".

> 
>> +	}
>> +
>> +	list_for_each_entry(mevt, &r->mon.evt_list, list) {
> 
> can use for_each_mbm_event() and then below will not be needed?

Yes.

> 
>> +		if (mevt->mbm_mode != MBM_MODE_ASSIGN)
>> +			continue;
>> +
>> +		sep = false;
>> +		seq_printf(s, "%s:", mevt->name);
>> +		list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> +			if (sep)
>> +				seq_putc(s, ';');
>> +
>> +			if (mbm_cntr_get(r, d, rdtgrp, mevt->evtid) >= 0)
>> +				seq_printf(s, "%d=e", d->hdr.id);
>> +			else
>> +				seq_printf(s, "%d=_", d->hdr.id);
>> +
>> +			sep = true;
>> +		}
>> +		seq_putc(s, '\n');
>> +	}
>> +
>> +assign_out:
>> +	rdtgroup_kn_unlock(of->kn);
>> +
>> +	return ret;
>> +}
>> +
>>   /* rdtgroup information files for one cache resource. */
>>   static struct rftype res_common_files[] = {
>>   	{
>> @@ -2218,6 +2264,12 @@ static struct rftype res_common_files[] = {
>>   		.seq_show	= event_filter_show,
>>   		.write		= event_filter_write,
>>   	},
>> +	{
>> +		.name		= "mbm_L3_assignments",
>> +		.mode		= 0444,
>> +		.kf_ops		= &rdtgroup_kf_single_ops,
>> +		.seq_show	= mbm_L3_assignments_show,
>> +	},
>>   	{
>>   		.name		= "mbm_assign_mode",
>>   		.mode		= 0444,
> 
> Reinette
> 

Thanks
Babu