[PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[Babu Moger]

Introduce the interface to assign MBM events in mbm_cntr_assign mode.

Events can be enabled or disabled by writing to file
/sys/fs/resctrl/info/L3_MON/mbm_assign_control

Format is similar to the list format with addition of opcode for the
assignment operation.
 "<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"

Format for specific type of groups:

 * Default CTRL_MON group:
         "//<domain_id><opcode><flags>"

 * Non-default CTRL_MON group:
         "<CTRL_MON group>//<domain_id><opcode><flags>"

 * Child MON group of default CTRL_MON group:
         "/<MON group>/<domain_id><opcode><flags>"

 * Child MON group of non-default CTRL_MON group:
         "<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"

Domain_id '*' will apply the flags on all the domains.

Opcode can be one of the following:

 = Update the assignment to match the flags
 + Assign a new MBM event without impacting existing assignments.
 - Unassign a MBM event from currently assigned events.

Assignment flags can be one of the following:
 t  MBM total event
 l  MBM local event
 tl Both total and local MBM events
 _  None of the MBM events. Valid only with '=' opcode.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
    Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
    Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
    Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
    Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
    Removed ABMC reference in FS code.
    Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
    Not sure if we need to change the behaviour here. Processed them sequencially right now.
    Users have the liberty to pass the flags. Restricting it might be a problem later.

v6: Added support assign all if domain id is '*'
    Fixed the allocation of counter id if it not assigned already.

v5: Interface name changed from mbm_assign_control to mbm_control.
    Fixed opcode and flags combination.
    '=_" is valid.
    "-_" amd "+_" is not valid.
    Minor message update.
    Renamed the function with prefix - rdtgroup_.
    Corrected few documentation mistakes.
    Rebase related changes after SNC support.

v4: Added domain specific assignments. Fixed the opcode parsing.

v3: New patch.
    Addresses the feedback to provide the global assignment interface.
    https://lore.kernel.org/lkml/c73f444b-83a1-4e9a-95d3-54c5165ee782@intel.com/
---
 Documentation/arch/x86/resctrl.rst     |  94 +++++++++-
 arch/x86/kernel/cpu/resctrl/internal.h |  10 ++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 234 ++++++++++++++++++++++++-
 3 files changed, 336 insertions(+), 2 deletions(-)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index a72cb3a6b07a..e46ec63d920e 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -334,7 +334,7 @@ with the following files:
 	 t  MBM total event is assigned.
 	 l  MBM local event is assigned.
 	 tl Both total and local MBM events are assigned.
-	 _  None of the MBM events are assigned.
+	 _  None of the MBM events are assigned. Only works with opcode '=' for write.
 
 	Examples:
 	::
@@ -352,6 +352,98 @@ with the following files:
 	 There are four resctrl groups. All the groups have total and local MBM events
 	 assigned on domain 0 and 1.
 
+	Assignment state can be updated by writing to the interface.
+
+	Format is similar to the list format with addition of opcode for the
+	assignment operation.
+
+		"<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"
+
+	Format for each type of groups:
+
+        * Default CTRL_MON group:
+                "//<domain_id><opcode><flags>"
+
+        * Non-default CTRL_MON group:
+                "<CTRL_MON group>//<domain_id><opcode><flags>"
+
+        * Child MON group of default CTRL_MON group:
+                "/<MON group>/<domain_id><opcode><flags>"
+
+        * Child MON group of non-default CTRL_MON group:
+                "<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"
+
+	Domain_id '*' will apply the flags on all the domains.
+
+	Opcode can be one of the following:
+	::
+
+	 = Update the assignment to match the MBM event.
+	 + Assign a new MBM event without impacting existing assignments.
+	 - Unassign a MBM event from currently assigned events.
+
+	Examples:
+	::
+
+	  Initial group status:
+	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
+	  non_default_ctrl_mon_grp//0=tl;1=tl;
+	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl;
+	  //0=tl;1=tl;
+	  /child_default_mon_grp/0=tl;1=tl;
+
+	  To update the default group to assign only total MBM event on domain 0:
+	  # echo "//0=t" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
+
+	  Assignment status after the update:
+	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
+	  non_default_ctrl_mon_grp//0=tl;1=tl;
+	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl;
+	  //0=t;1=tl;
+	  /child_default_mon_grp/0=tl;1=tl;
+
+	  To update the MON group child_default_mon_grp to remove total MBM event on domain 1:
+	  # echo "/child_default_mon_grp/1-t" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
+
+	  Assignment status after the update:
+	  $ cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
+	  non_default_ctrl_mon_grp//0=tl;1=tl;
+	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl;
+	  //0=t;1=tl;
+	  /child_default_mon_grp/0=tl;1=l;
+
+	  To update the MON group non_default_ctrl_mon_grp/child_non_default_mon_grp to
+	  unassign both local and total MBM events on domain 1:
+	  # echo "non_default_ctrl_mon_grp/child_non_default_mon_grp/1=_" >
+			/sys/fs/resctrl/info/L3_MON/mbm_assign_control
+
+	  Assignment status after the update:
+	  non_default_ctrl_mon_grp//0=tl;1=tl;
+	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=_;
+	  //0=t;1=tl;
+	  /child_default_mon_grp/0=tl;1=l;
+
+	  To update the default group to add a local MBM event domain 0.
+	  # echo "//0+l" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
+
+	  Assignment status after the update:
+	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
+	  non_default_ctrl_mon_grp//0=tl;1=tl;
+	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=_;
+	  //0=tl;1=tl;
+	  /child_default_mon_grp/0=tl;1=l;
+
+	  To update the non default CTRL_MON group non_default_ctrl_mon_grp to unassign all
+	  the MBM events on all the domains.
+	  # echo "non_default_ctrl_mon_grp//*=_" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
+
+	  Assignment status after the update:
+	  #cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
+	  non_default_ctrl_mon_grp//0=_;1=_;
+	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=_;
+	  //0=tl;1=tl;
+	  /child_default_mon_grp/0=tl;1=l;
+
 "max_threshold_occupancy":
 		Read/write file provides the largest value (in
 		bytes) at which a previously used LLC_occupancy
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 3250561f0187..799f36eef2b6 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -67,6 +67,16 @@
 
 #define MON_CNTR_UNSET			U32_MAX
 
+/*
+ * Assignment flags for mbm_cntr_assign feature
+ */
+enum {
+	ASSIGN_NONE	= 0,
+	ASSIGN_TOTAL	= BIT(QOS_L3_MBM_TOTAL_EVENT_ID),
+	ASSIGN_LOCAL	= BIT(QOS_L3_MBM_LOCAL_EVENT_ID),
+	ASSIGN_INVALID,
+};
+
 /**
  * cpumask_any_housekeeping() - Choose any CPU in @mask, preferring those that
  *			        aren't marked nohz_full
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index ffa0ed98efbe..56ecdf7406ae 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1047,6 +1047,237 @@ static int rdtgroup_mbm_assign_control_show(struct kernfs_open_file *of,
 	return 0;
 }
 
+static int rdtgroup_str_to_mon_state(char *flag)
+{
+	int i, mon_state = ASSIGN_NONE;
+
+	for (i = 0; i < strlen(flag); i++) {
+		switch (*(flag + i)) {
+		case 't':
+			mon_state |= ASSIGN_TOTAL;
+			break;
+		case 'l':
+			mon_state |= ASSIGN_LOCAL;
+			break;
+		case '_':
+			mon_state = ASSIGN_NONE;
+			break;
+		default:
+			return ASSIGN_INVALID;
+		}
+	}
+
+	return mon_state;
+}
+
+static struct rdtgroup *rdtgroup_find_grp_by_name(enum rdt_group_type rtype,
+						  char *p_grp, char *c_grp)
+{
+	struct rdtgroup *rdtg, *crg;
+
+	if (rtype == RDTCTRL_GROUP && *p_grp == '\0') {
+		return &rdtgroup_default;
+	} else if (rtype == RDTCTRL_GROUP) {
+		list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list)
+			if (!strcmp(p_grp, rdtg->kn->name))
+				return rdtg;
+	} else if (rtype == RDTMON_GROUP) {
+		list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list) {
+			if (!strcmp(p_grp, rdtg->kn->name)) {
+				list_for_each_entry(crg, &rdtg->mon.crdtgrp_list,
+						    mon.crdtgrp_list) {
+					if (!strcmp(c_grp, crg->kn->name))
+						return crg;
+				}
+			}
+		}
+	}
+
+	return NULL;
+}
+
+static int rdtgroup_process_flags(struct rdt_resource *r,
+				  enum rdt_group_type rtype,
+				  char *p_grp, char *c_grp, char *tok)
+{
+	int op, mon_state, assign_state, unassign_state;
+	char *dom_str, *id_str, *op_str;
+	struct rdt_mon_domain *d;
+	struct rdtgroup *rdtgrp;
+	unsigned long dom_id;
+	int ret, found = 0;
+
+	rdtgrp = rdtgroup_find_grp_by_name(rtype, p_grp, c_grp);
+
+	if (!rdtgrp) {
+		rdt_last_cmd_puts("Not a valid resctrl group\n");
+		return -EINVAL;
+	}
+
+next:
+	if (!tok || tok[0] == '\0')
+		return 0;
+
+	/* Start processing the strings for each domain */
+	dom_str = strim(strsep(&tok, ";"));
+
+	op_str = strpbrk(dom_str, "=+-");
+
+	if (op_str) {
+		op = *op_str;
+	} else {
+		rdt_last_cmd_puts("Missing operation =, +, -, _ character\n");
+		return -EINVAL;
+	}
+
+	id_str = strsep(&dom_str, "=+-");
+
+	/* Check for domain id '*' which means all domains */
+	if (id_str && *id_str == '*') {
+		d = NULL;
+		goto check_state;
+	} else if (!id_str || kstrtoul(id_str, 10, &dom_id)) {
+		rdt_last_cmd_puts("Missing domain id\n");
+		return -EINVAL;
+	}
+
+	/* Verify if the dom_id is valid */
+	list_for_each_entry(d, &r->mon_domains, hdr.list) {
+		if (d->hdr.id == dom_id) {
+			found = 1;
+			break;
+		}
+	}
+
+	if (!found) {
+		rdt_last_cmd_printf("Invalid domain id %ld\n", dom_id);
+		return -EINVAL;
+	}
+
+check_state:
+	mon_state = rdtgroup_str_to_mon_state(dom_str);
+
+	if (mon_state == ASSIGN_INVALID) {
+		rdt_last_cmd_puts("Invalid assign flag\n");
+		goto out_fail;
+	}
+
+	assign_state = 0;
+	unassign_state = 0;
+
+	switch (op) {
+	case '+':
+		if (mon_state == ASSIGN_NONE) {
+			rdt_last_cmd_puts("Invalid assign opcode\n");
+			goto out_fail;
+		}
+		assign_state = mon_state;
+		break;
+	case '-':
+		if (mon_state == ASSIGN_NONE) {
+			rdt_last_cmd_puts("Invalid assign opcode\n");
+			goto out_fail;
+		}
+		unassign_state = mon_state;
+		break;
+	case '=':
+		assign_state = mon_state;
+		unassign_state = (ASSIGN_TOTAL | ASSIGN_LOCAL) & ~assign_state;
+		break;
+	default:
+		break;
+	}
+
+	if (assign_state & ASSIGN_TOTAL) {
+		ret = rdtgroup_assign_cntr(r, rdtgrp, d, QOS_L3_MBM_TOTAL_EVENT_ID);
+		if (ret)
+			goto out_fail;
+	}
+
+	if (assign_state & ASSIGN_LOCAL) {
+		ret = rdtgroup_assign_cntr(r, rdtgrp, d, QOS_L3_MBM_LOCAL_EVENT_ID);
+		if (ret)
+			goto out_fail;
+	}
+
+	if (unassign_state & ASSIGN_TOTAL) {
+		ret = rdtgroup_unassign_cntr(r, rdtgrp, d, QOS_L3_MBM_TOTAL_EVENT_ID);
+		if (ret)
+			goto out_fail;
+	}
+
+	if (unassign_state & ASSIGN_LOCAL) {
+		ret = rdtgroup_unassign_cntr(r, rdtgrp, d, QOS_L3_MBM_LOCAL_EVENT_ID);
+		if (ret)
+			goto out_fail;
+	}
+
+	goto next;
+
+out_fail:
+
+	return -EINVAL;
+}
+
+static ssize_t rdtgroup_mbm_assign_control_write(struct kernfs_open_file *of,
+						 char *buf, size_t nbytes, loff_t off)
+{
+	struct rdt_resource *r = of->kn->parent->priv;
+	char *token, *cmon_grp, *mon_grp;
+	enum rdt_group_type rtype;
+	int ret;
+
+	/* Valid input requires a trailing newline */
+	if (nbytes == 0 || buf[nbytes - 1] != '\n')
+		return -EINVAL;
+
+	buf[nbytes - 1] = '\0';
+
+	cpus_read_lock();
+	mutex_lock(&rdtgroup_mutex);
+
+	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
+		rdt_last_cmd_puts("mbm_cntr_assign mode is not enabled\n");
+		mutex_unlock(&rdtgroup_mutex);
+		cpus_read_unlock();
+		return -EINVAL;
+	}
+
+	rdt_last_cmd_clear();
+
+	while ((token = strsep(&buf, "\n")) != NULL) {
+		if (strstr(token, "/")) {
+			/*
+			 * The write command follows the following format:
+			 * “<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>”
+			 * Extract the CTRL_MON group.
+			 */
+			cmon_grp = strsep(&token, "/");
+
+			/*
+			 * Extract the MON_GROUP.
+			 * strsep returns empty string for contiguous delimiters.
+			 * Empty mon_grp here means it is a RDTCTRL_GROUP.
+			 */
+			mon_grp = strsep(&token, "/");
+
+			if (*mon_grp == '\0')
+				rtype = RDTCTRL_GROUP;
+			else
+				rtype = RDTMON_GROUP;
+
+			ret = rdtgroup_process_flags(r, rtype, cmon_grp, mon_grp, token);
+			if (ret)
+				break;
+		}
+	}
+
+	mutex_unlock(&rdtgroup_mutex);
+	cpus_read_unlock();
+
+	return ret ?: nbytes;
+}
+
 #ifdef CONFIG_PROC_CPU_RESCTRL
 
 /*
@@ -2315,9 +2546,10 @@ static struct rftype res_common_files[] = {
 	},
 	{
 		.name		= "mbm_assign_control",
-		.mode		= 0444,
+		.mode		= 0644,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdtgroup_mbm_assign_control_show,
+		.write		= rdtgroup_mbm_assign_control_write,
 	},
 	{
 		.name		= "cpus_list",
-- 
2.34.1

Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <68c8ef0592c653c5b99cd26d982966cd4a41cb31.1725488488.git.babu.moger@amd.com>
References: <68c8ef0592c653c5b99cd26d982966cd4a41cb31.1725488488.git.babu.moger@amd.com>
TO: Babu Moger <babu.moger@amd.com>

Hi Babu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.11-rc7 next-20240911]
[cannot apply to tip/x86/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Babu-Moger/x86-cpufeatures-Add-support-for-Assignable-Bandwidth-Monitoring-Counters-ABMC/20240905-062937
base:   linus/master
patch link:    https://lore.kernel.org/r/68c8ef0592c653c5b99cd26d982966cd4a41cb31.1725488488.git.babu.moger%40amd.com
patch subject: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups
:::::: branch date: 7 days ago
:::::: commit date: 7 days ago
config: i386-randconfig-141-20240911 (https://download.01.org/0day-ci/archive/20240911/202409112351.fbIMtRVP-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202409112351.fbIMtRVP-lkp@intel.com/

smatch warnings:
arch/x86/kernel/cpu/resctrl/rdtgroup.c:1278 rdtgroup_mbm_assign_control_write() error: uninitialized symbol 'ret'.

vim +/ret +1278 arch/x86/kernel/cpu/resctrl/rdtgroup.c

d9001030c01eb5 Babu Moger 2024-09-04  1221  
d9001030c01eb5 Babu Moger 2024-09-04  1222  static ssize_t rdtgroup_mbm_assign_control_write(struct kernfs_open_file *of,
d9001030c01eb5 Babu Moger 2024-09-04  1223  						 char *buf, size_t nbytes, loff_t off)
d9001030c01eb5 Babu Moger 2024-09-04  1224  {
d9001030c01eb5 Babu Moger 2024-09-04  1225  	struct rdt_resource *r = of->kn->parent->priv;
d9001030c01eb5 Babu Moger 2024-09-04  1226  	char *token, *cmon_grp, *mon_grp;
d9001030c01eb5 Babu Moger 2024-09-04  1227  	enum rdt_group_type rtype;
d9001030c01eb5 Babu Moger 2024-09-04  1228  	int ret;
d9001030c01eb5 Babu Moger 2024-09-04  1229  
d9001030c01eb5 Babu Moger 2024-09-04  1230  	/* Valid input requires a trailing newline */
d9001030c01eb5 Babu Moger 2024-09-04  1231  	if (nbytes == 0 || buf[nbytes - 1] != '\n')
d9001030c01eb5 Babu Moger 2024-09-04  1232  		return -EINVAL;
d9001030c01eb5 Babu Moger 2024-09-04  1233  
d9001030c01eb5 Babu Moger 2024-09-04  1234  	buf[nbytes - 1] = '\0';
d9001030c01eb5 Babu Moger 2024-09-04  1235  
d9001030c01eb5 Babu Moger 2024-09-04  1236  	cpus_read_lock();
d9001030c01eb5 Babu Moger 2024-09-04  1237  	mutex_lock(&rdtgroup_mutex);
d9001030c01eb5 Babu Moger 2024-09-04  1238  
d9001030c01eb5 Babu Moger 2024-09-04  1239  	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
d9001030c01eb5 Babu Moger 2024-09-04  1240  		rdt_last_cmd_puts("mbm_cntr_assign mode is not enabled\n");
d9001030c01eb5 Babu Moger 2024-09-04  1241  		mutex_unlock(&rdtgroup_mutex);
d9001030c01eb5 Babu Moger 2024-09-04  1242  		cpus_read_unlock();
d9001030c01eb5 Babu Moger 2024-09-04  1243  		return -EINVAL;
d9001030c01eb5 Babu Moger 2024-09-04  1244  	}
d9001030c01eb5 Babu Moger 2024-09-04  1245  
d9001030c01eb5 Babu Moger 2024-09-04  1246  	rdt_last_cmd_clear();
d9001030c01eb5 Babu Moger 2024-09-04  1247  
d9001030c01eb5 Babu Moger 2024-09-04  1248  	while ((token = strsep(&buf, "\n")) != NULL) {
d9001030c01eb5 Babu Moger 2024-09-04  1249  		if (strstr(token, "/")) {
d9001030c01eb5 Babu Moger 2024-09-04  1250  			/*
d9001030c01eb5 Babu Moger 2024-09-04  1251  			 * The write command follows the following format:
d9001030c01eb5 Babu Moger 2024-09-04  1252  			 * “<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>”
d9001030c01eb5 Babu Moger 2024-09-04  1253  			 * Extract the CTRL_MON group.
d9001030c01eb5 Babu Moger 2024-09-04  1254  			 */
d9001030c01eb5 Babu Moger 2024-09-04  1255  			cmon_grp = strsep(&token, "/");
d9001030c01eb5 Babu Moger 2024-09-04  1256  
d9001030c01eb5 Babu Moger 2024-09-04  1257  			/*
d9001030c01eb5 Babu Moger 2024-09-04  1258  			 * Extract the MON_GROUP.
d9001030c01eb5 Babu Moger 2024-09-04  1259  			 * strsep returns empty string for contiguous delimiters.
d9001030c01eb5 Babu Moger 2024-09-04  1260  			 * Empty mon_grp here means it is a RDTCTRL_GROUP.
d9001030c01eb5 Babu Moger 2024-09-04  1261  			 */
d9001030c01eb5 Babu Moger 2024-09-04  1262  			mon_grp = strsep(&token, "/");
d9001030c01eb5 Babu Moger 2024-09-04  1263  
d9001030c01eb5 Babu Moger 2024-09-04  1264  			if (*mon_grp == '\0')
d9001030c01eb5 Babu Moger 2024-09-04  1265  				rtype = RDTCTRL_GROUP;
d9001030c01eb5 Babu Moger 2024-09-04  1266  			else
d9001030c01eb5 Babu Moger 2024-09-04  1267  				rtype = RDTMON_GROUP;
d9001030c01eb5 Babu Moger 2024-09-04  1268  
d9001030c01eb5 Babu Moger 2024-09-04  1269  			ret = rdtgroup_process_flags(r, rtype, cmon_grp, mon_grp, token);
d9001030c01eb5 Babu Moger 2024-09-04  1270  			if (ret)
d9001030c01eb5 Babu Moger 2024-09-04  1271  				break;
d9001030c01eb5 Babu Moger 2024-09-04  1272  		}
d9001030c01eb5 Babu Moger 2024-09-04  1273  	}
d9001030c01eb5 Babu Moger 2024-09-04  1274  
d9001030c01eb5 Babu Moger 2024-09-04  1275  	mutex_unlock(&rdtgroup_mutex);
d9001030c01eb5 Babu Moger 2024-09-04  1276  	cpus_read_unlock();
d9001030c01eb5 Babu Moger 2024-09-04  1277  
d9001030c01eb5 Babu Moger 2024-09-04 @1278  	return ret ?: nbytes;
d9001030c01eb5 Babu Moger 2024-09-04  1279  }
d9001030c01eb5 Babu Moger 2024-09-04  1280  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[Reinette Chatre]

Hi Babu,

On 9/4/24 3:21 PM, Babu Moger wrote:
> Introduce the interface to assign MBM events in mbm_cntr_assign mode.
> 
> Events can be enabled or disabled by writing to file
> /sys/fs/resctrl/info/L3_MON/mbm_assign_control
> 
> Format is similar to the list format with addition of opcode for the
> assignment operation.
>  "<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"
> 
> Format for specific type of groups:
> 
>  * Default CTRL_MON group:
>          "//<domain_id><opcode><flags>"
> 
>  * Non-default CTRL_MON group:
>          "<CTRL_MON group>//<domain_id><opcode><flags>"
> 
>  * Child MON group of default CTRL_MON group:
>          "/<MON group>/<domain_id><opcode><flags>"
> 
>  * Child MON group of non-default CTRL_MON group:
>          "<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"
> 
> Domain_id '*' will apply the flags on all the domains.
> 
> Opcode can be one of the following:
> 
>  = Update the assignment to match the flags
>  + Assign a new MBM event without impacting existing assignments.
>  - Unassign a MBM event from currently assigned events.
> 
> Assignment flags can be one of the following:
>  t  MBM total event
>  l  MBM local event
>  tl Both total and local MBM events
>  _  None of the MBM events. Valid only with '=' opcode.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
>     Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
>     Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
>     Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
>     Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
>     Removed ABMC reference in FS code.
>     Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
>     Not sure if we need to change the behaviour here. Processed them sequencially right now.
>     Users have the liberty to pass the flags. Restricting it might be a problem later.

Could you please give an example of what problem may be encountered later? An assignment
like "domain=_lt" seems like a contradiction to me since user space essentially asks
for "None of the MBM events" as well as "MBM total event" and "MBM local event".


...

> @@ -352,6 +352,98 @@ with the following files:
>  	 There are four resctrl groups. All the groups have total and local MBM events
>  	 assigned on domain 0 and 1.
>  
> +	Assignment state can be updated by writing to the interface.
> +
> +	Format is similar to the list format with addition of opcode for the
> +	assignment operation.
> +
> +		"<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"
> +
> +	Format for each type of groups:
> +
> +        * Default CTRL_MON group:
> +                "//<domain_id><opcode><flags>"
> +
> +        * Non-default CTRL_MON group:
> +                "<CTRL_MON group>//<domain_id><opcode><flags>"
> +
> +        * Child MON group of default CTRL_MON group:
> +                "/<MON group>/<domain_id><opcode><flags>"
> +
> +        * Child MON group of non-default CTRL_MON group:
> +                "<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"
> +
> +	Domain_id '*' will apply the flags on all the domains.
> +
> +	Opcode can be one of the following:
> +	::
> +
> +	 = Update the assignment to match the MBM event.
> +	 + Assign a new MBM event without impacting existing assignments.
> +	 - Unassign a MBM event from currently assigned events.
> +
> +	Examples:
> +	::
> +
> +	  Initial group status:
> +	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
> +	  non_default_ctrl_mon_grp//0=tl;1=tl;
> +	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl;
> +	  //0=tl;1=tl;
> +	  /child_default_mon_grp/0=tl;1=tl;
> +

Similar to previous patch, looking at this generated doc does not seem to reflect
what is intended. Above and below are all formatted as code, the descriptions as
well as the actual "code".

> +	  To update the default group to assign only total MBM event on domain 0:
> +	  # echo "//0=t" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
> +
> +	  Assignment status after the update:
> +	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
> +	  non_default_ctrl_mon_grp//0=tl;1=tl;
> +	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl;
> +	  //0=t;1=tl;
> +	  /child_default_mon_grp/0=tl;1=tl;
> +
> +	  To update the MON group child_default_mon_grp to remove total MBM event on domain 1:
> +	  # echo "/child_default_mon_grp/1-t" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
> +
> +	  Assignment status after the update:
> +	  $ cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
> +	  non_default_ctrl_mon_grp//0=tl;1=tl;
> +	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl;
> +	  //0=t;1=tl;
> +	  /child_default_mon_grp/0=tl;1=l;
> +
> +	  To update the MON group non_default_ctrl_mon_grp/child_non_default_mon_grp to
> +	  unassign both local and total MBM events on domain 1:
> +	  # echo "non_default_ctrl_mon_grp/child_non_default_mon_grp/1=_" >
> +			/sys/fs/resctrl/info/L3_MON/mbm_assign_control
> +
> +	  Assignment status after the update:
> +	  non_default_ctrl_mon_grp//0=tl;1=tl;
> +	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=_;
> +	  //0=t;1=tl;
> +	  /child_default_mon_grp/0=tl;1=l;
> +
> +	  To update the default group to add a local MBM event domain 0.
> +	  # echo "//0+l" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
> +
> +	  Assignment status after the update:
> +	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
> +	  non_default_ctrl_mon_grp//0=tl;1=tl;
> +	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=_;
> +	  //0=tl;1=tl;
> +	  /child_default_mon_grp/0=tl;1=l;
> +
> +	  To update the non default CTRL_MON group non_default_ctrl_mon_grp to unassign all
> +	  the MBM events on all the domains.
> +	  # echo "non_default_ctrl_mon_grp//*=_" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
> +
> +	  Assignment status after the update:
> +	  #cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
> +	  non_default_ctrl_mon_grp//0=_;1=_;
> +	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=_;
> +	  //0=tl;1=tl;
> +	  /child_default_mon_grp/0=tl;1=l;
> +
>  "max_threshold_occupancy":
>  		Read/write file provides the largest value (in
>  		bytes) at which a previously used LLC_occupancy

...

> +static int rdtgroup_process_flags(struct rdt_resource *r,
> +				  enum rdt_group_type rtype,
> +				  char *p_grp, char *c_grp, char *tok)
> +{
> +	int op, mon_state, assign_state, unassign_state;
> +	char *dom_str, *id_str, *op_str;
> +	struct rdt_mon_domain *d;
> +	struct rdtgroup *rdtgrp;
> +	unsigned long dom_id;
> +	int ret, found = 0;
> +
> +	rdtgrp = rdtgroup_find_grp_by_name(rtype, p_grp, c_grp);
> +
> +	if (!rdtgrp) {
> +		rdt_last_cmd_puts("Not a valid resctrl group\n");
> +		return -EINVAL;
> +	}
> +
> +next:
> +	if (!tok || tok[0] == '\0')
> +		return 0;
> +
> +	/* Start processing the strings for each domain */
> +	dom_str = strim(strsep(&tok, ";"));
> +
> +	op_str = strpbrk(dom_str, "=+-");
> +
> +	if (op_str) {
> +		op = *op_str;
> +	} else {
> +		rdt_last_cmd_puts("Missing operation =, +, -, _ character\n");

"_" is not an operation.

> +		return -EINVAL;
> +	}
> +
> +	id_str = strsep(&dom_str, "=+-");
> +
> +	/* Check for domain id '*' which means all domains */
> +	if (id_str && *id_str == '*') {
> +		d = NULL;
> +		goto check_state;
> +	} else if (!id_str || kstrtoul(id_str, 10, &dom_id)) {
> +		rdt_last_cmd_puts("Missing domain id\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Verify if the dom_id is valid */
> +	list_for_each_entry(d, &r->mon_domains, hdr.list) {
> +		if (d->hdr.id == dom_id) {
> +			found = 1;
> +			break;
> +		}
> +	}
> +
> +	if (!found) {
> +		rdt_last_cmd_printf("Invalid domain id %ld\n", dom_id);
> +		return -EINVAL;
> +	}
> +
> +check_state:
> +	mon_state = rdtgroup_str_to_mon_state(dom_str);
> +
> +	if (mon_state == ASSIGN_INVALID) {
> +		rdt_last_cmd_puts("Invalid assign flag\n");
> +		goto out_fail;
> +	}
> +
> +	assign_state = 0;
> +	unassign_state = 0;
> +
> +	switch (op) {
> +	case '+':
> +		if (mon_state == ASSIGN_NONE) {
> +			rdt_last_cmd_puts("Invalid assign opcode\n");
> +			goto out_fail;
> +		}
> +		assign_state = mon_state;
> +		break;
> +	case '-':
> +		if (mon_state == ASSIGN_NONE) {
> +			rdt_last_cmd_puts("Invalid assign opcode\n");
> +			goto out_fail;
> +		}
> +		unassign_state = mon_state;
> +		break;
> +	case '=':
> +		assign_state = mon_state;
> +		unassign_state = (ASSIGN_TOTAL | ASSIGN_LOCAL) & ~assign_state;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (assign_state & ASSIGN_TOTAL) {
> +		ret = rdtgroup_assign_cntr(r, rdtgrp, d, QOS_L3_MBM_TOTAL_EVENT_ID);

hmmm ... wasn't unassign going to happen first? That would potentially make counters
available to help subsequent assign succeed.

> +		if (ret)
> +			goto out_fail;
> +	}
> +
> +	if (assign_state & ASSIGN_LOCAL) {
> +		ret = rdtgroup_assign_cntr(r, rdtgrp, d, QOS_L3_MBM_LOCAL_EVENT_ID);
> +		if (ret)
> +			goto out_fail;
> +	}
> +
> +	if (unassign_state & ASSIGN_TOTAL) {
> +		ret = rdtgroup_unassign_cntr(r, rdtgrp, d, QOS_L3_MBM_TOTAL_EVENT_ID);
> +		if (ret)
> +			goto out_fail;
> +	}
> +
> +	if (unassign_state & ASSIGN_LOCAL) {
> +		ret = rdtgroup_unassign_cntr(r, rdtgrp, d, QOS_L3_MBM_LOCAL_EVENT_ID);
> +		if (ret)
> +			goto out_fail;
> +	}
> +
> +	goto next;
> +
> +out_fail:
> +
> +	return -EINVAL;
> +}
> +

Reinette

Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[Moger, Babu]

Hi Reinette,


On 9/19/2024 12:59 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/4/24 3:21 PM, Babu Moger wrote:
>> Introduce the interface to assign MBM events in mbm_cntr_assign mode.
>>
>> Events can be enabled or disabled by writing to file
>> /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>>
>> Format is similar to the list format with addition of opcode for the
>> assignment operation.
>>   "<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"
>>
>> Format for specific type of groups:
>>
>>   * Default CTRL_MON group:
>>           "//<domain_id><opcode><flags>"
>>
>>   * Non-default CTRL_MON group:
>>           "<CTRL_MON group>//<domain_id><opcode><flags>"
>>
>>   * Child MON group of default CTRL_MON group:
>>           "/<MON group>/<domain_id><opcode><flags>"
>>
>>   * Child MON group of non-default CTRL_MON group:
>>           "<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"
>>
>> Domain_id '*' will apply the flags on all the domains.
>>
>> Opcode can be one of the following:
>>
>>   = Update the assignment to match the flags
>>   + Assign a new MBM event without impacting existing assignments.
>>   - Unassign a MBM event from currently assigned events.
>>
>> Assignment flags can be one of the following:
>>   t  MBM total event
>>   l  MBM local event
>>   tl Both total and local MBM events
>>   _  None of the MBM events. Valid only with '=' opcode.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
>>      Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
>>      Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
>>      Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
>>      Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
>>      Removed ABMC reference in FS code.
>>      Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
>>      Not sure if we need to change the behaviour here. Processed them sequencially right now.
>>      Users have the liberty to pass the flags. Restricting it might be a problem later.
> 
> Could you please give an example of what problem may be encountered later? An assignment
> like "domain=_lt" seems like a contradiction to me since user space essentially asks
> for "None of the MBM events" as well as "MBM total event" and "MBM local event".

I agree it is contradiction. But user is the one who decides to do that. 
I think we should allow it. Also, there is some value to it as well.

"domain=_lt" This will also reset the counters if the total and local 
events are assigned earlier this action.


> 
> 
> ...
> 
>> @@ -352,6 +352,98 @@ with the following files:
>>   	 There are four resctrl groups. All the groups have total and local MBM events
>>   	 assigned on domain 0 and 1.
>>   
>> +	Assignment state can be updated by writing to the interface.
>> +
>> +	Format is similar to the list format with addition of opcode for the
>> +	assignment operation.
>> +
>> +		"<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"
>> +
>> +	Format for each type of groups:
>> +
>> +        * Default CTRL_MON group:
>> +                "//<domain_id><opcode><flags>"
>> +
>> +        * Non-default CTRL_MON group:
>> +                "<CTRL_MON group>//<domain_id><opcode><flags>"
>> +
>> +        * Child MON group of default CTRL_MON group:
>> +                "/<MON group>/<domain_id><opcode><flags>"
>> +
>> +        * Child MON group of non-default CTRL_MON group:
>> +                "<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>"
>> +
>> +	Domain_id '*' will apply the flags on all the domains.
>> +
>> +	Opcode can be one of the following:
>> +	::
>> +
>> +	 = Update the assignment to match the MBM event.
>> +	 + Assign a new MBM event without impacting existing assignments.
>> +	 - Unassign a MBM event from currently assigned events.
>> +
>> +	Examples:
>> +	::
>> +
>> +	  Initial group status:
>> +	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +	  non_default_ctrl_mon_grp//0=tl;1=tl;
>> +	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl;
>> +	  //0=tl;1=tl;
>> +	  /child_default_mon_grp/0=tl;1=tl;
>> +
> 
> Similar to previous patch, looking at this generated doc does not seem to reflect
> what is intended. Above and below are all formatted as code, the descriptions as
> well as the actual "code".

Sure. Will check again.

> 
>> +	  To update the default group to assign only total MBM event on domain 0:
>> +	  # echo "//0=t" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +
>> +	  Assignment status after the update:
>> +	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +	  non_default_ctrl_mon_grp//0=tl;1=tl;
>> +	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl;
>> +	  //0=t;1=tl;
>> +	  /child_default_mon_grp/0=tl;1=tl;
>> +
>> +	  To update the MON group child_default_mon_grp to remove total MBM event on domain 1:
>> +	  # echo "/child_default_mon_grp/1-t" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +
>> +	  Assignment status after the update:
>> +	  $ cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +	  non_default_ctrl_mon_grp//0=tl;1=tl;
>> +	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=tl;
>> +	  //0=t;1=tl;
>> +	  /child_default_mon_grp/0=tl;1=l;
>> +
>> +	  To update the MON group non_default_ctrl_mon_grp/child_non_default_mon_grp to
>> +	  unassign both local and total MBM events on domain 1:
>> +	  # echo "non_default_ctrl_mon_grp/child_non_default_mon_grp/1=_" >
>> +			/sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +
>> +	  Assignment status after the update:
>> +	  non_default_ctrl_mon_grp//0=tl;1=tl;
>> +	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=_;
>> +	  //0=t;1=tl;
>> +	  /child_default_mon_grp/0=tl;1=l;
>> +
>> +	  To update the default group to add a local MBM event domain 0.
>> +	  # echo "//0+l" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +
>> +	  Assignment status after the update:
>> +	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +	  non_default_ctrl_mon_grp//0=tl;1=tl;
>> +	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=_;
>> +	  //0=tl;1=tl;
>> +	  /child_default_mon_grp/0=tl;1=l;
>> +
>> +	  To update the non default CTRL_MON group non_default_ctrl_mon_grp to unassign all
>> +	  the MBM events on all the domains.
>> +	  # echo "non_default_ctrl_mon_grp//*=_" > /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +
>> +	  Assignment status after the update:
>> +	  #cat /sys/fs/resctrl/info/L3_MON/mbm_assign_control
>> +	  non_default_ctrl_mon_grp//0=_;1=_;
>> +	  non_default_ctrl_mon_grp/child_non_default_mon_grp/0=tl;1=_;
>> +	  //0=tl;1=tl;
>> +	  /child_default_mon_grp/0=tl;1=l;
>> +
>>   "max_threshold_occupancy":
>>   		Read/write file provides the largest value (in
>>   		bytes) at which a previously used LLC_occupancy
> 
> ...
> 
>> +static int rdtgroup_process_flags(struct rdt_resource *r,
>> +				  enum rdt_group_type rtype,
>> +				  char *p_grp, char *c_grp, char *tok)
>> +{
>> +	int op, mon_state, assign_state, unassign_state;
>> +	char *dom_str, *id_str, *op_str;
>> +	struct rdt_mon_domain *d;
>> +	struct rdtgroup *rdtgrp;
>> +	unsigned long dom_id;
>> +	int ret, found = 0;
>> +
>> +	rdtgrp = rdtgroup_find_grp_by_name(rtype, p_grp, c_grp);
>> +
>> +	if (!rdtgrp) {
>> +		rdt_last_cmd_puts("Not a valid resctrl group\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +next:
>> +	if (!tok || tok[0] == '\0')
>> +		return 0;
>> +
>> +	/* Start processing the strings for each domain */
>> +	dom_str = strim(strsep(&tok, ";"));
>> +
>> +	op_str = strpbrk(dom_str, "=+-");
>> +
>> +	if (op_str) {
>> +		op = *op_str;
>> +	} else {
>> +		rdt_last_cmd_puts("Missing operation =, +, -, _ character\n");
> 
> "_" is not an operation.

Sure. Will remove  this charactor.

> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	id_str = strsep(&dom_str, "=+-");
>> +
>> +	/* Check for domain id '*' which means all domains */
>> +	if (id_str && *id_str == '*') {
>> +		d = NULL;
>> +		goto check_state;
>> +	} else if (!id_str || kstrtoul(id_str, 10, &dom_id)) {
>> +		rdt_last_cmd_puts("Missing domain id\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Verify if the dom_id is valid */
>> +	list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> +		if (d->hdr.id == dom_id) {
>> +			found = 1;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!found) {
>> +		rdt_last_cmd_printf("Invalid domain id %ld\n", dom_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +check_state:
>> +	mon_state = rdtgroup_str_to_mon_state(dom_str);
>> +
>> +	if (mon_state == ASSIGN_INVALID) {
>> +		rdt_last_cmd_puts("Invalid assign flag\n");
>> +		goto out_fail;
>> +	}
>> +
>> +	assign_state = 0;
>> +	unassign_state = 0;
>> +
>> +	switch (op) {
>> +	case '+':
>> +		if (mon_state == ASSIGN_NONE) {
>> +			rdt_last_cmd_puts("Invalid assign opcode\n");
>> +			goto out_fail;
>> +		}
>> +		assign_state = mon_state;
>> +		break;
>> +	case '-':
>> +		if (mon_state == ASSIGN_NONE) {
>> +			rdt_last_cmd_puts("Invalid assign opcode\n");
>> +			goto out_fail;
>> +		}
>> +		unassign_state = mon_state;
>> +		break;
>> +	case '=':
>> +		assign_state = mon_state;
>> +		unassign_state = (ASSIGN_TOTAL | ASSIGN_LOCAL) & ~assign_state;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	if (assign_state & ASSIGN_TOTAL) {
>> +		ret = rdtgroup_assign_cntr(r, rdtgrp, d, QOS_L3_MBM_TOTAL_EVENT_ID);
> 
> hmmm ... wasn't unassign going to happen first? That would potentially make counters
> available to help subsequent assign succeed.

Good point. I will change the order.

> 
>> +		if (ret)
>> +			goto out_fail;
>> +	}
>> +
>> +	if (assign_state & ASSIGN_LOCAL) {
>> +		ret = rdtgroup_assign_cntr(r, rdtgrp, d, QOS_L3_MBM_LOCAL_EVENT_ID);
>> +		if (ret)
>> +			goto out_fail;
>> +	}
>> +
>> +	if (unassign_state & ASSIGN_TOTAL) {
>> +		ret = rdtgroup_unassign_cntr(r, rdtgrp, d, QOS_L3_MBM_TOTAL_EVENT_ID);
>> +		if (ret)
>> +			goto out_fail;
>> +	}
>> +
>> +	if (unassign_state & ASSIGN_LOCAL) {
>> +		ret = rdtgroup_unassign_cntr(r, rdtgrp, d, QOS_L3_MBM_LOCAL_EVENT_ID);
>> +		if (ret)
>> +			goto out_fail;
>> +	}
>> +
>> +	goto next;
>> +
>> +out_fail:
>> +
>> +	return -EINVAL;
>> +}
>> +
> 
> Reinette
> 
> 

Thanks
-- 
- Babu Moger

Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[Reinette Chatre]

Hi Babu,

On 9/27/24 10:47 AM, Moger, Babu wrote:
> On 9/19/2024 12:59 PM, Reinette Chatre wrote:
>> On 9/4/24 3:21 PM, Babu Moger wrote:

>>> v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
>>>      Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
>>>      Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
>>>      Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
>>>      Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
>>>      Removed ABMC reference in FS code.
>>>      Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
>>>      Not sure if we need to change the behaviour here. Processed them sequencially right now.
>>>      Users have the liberty to pass the flags. Restricting it might be a problem later.
>>
>> Could you please give an example of what problem may be encountered later? An assignment
>> like "domain=_lt" seems like a contradiction to me since user space essentially asks
>> for "None of the MBM events" as well as "MBM total event" and "MBM local event".
> 
> I agree it is contradiction. But user is the one who decides to do that. I think we should allow it. Also, there is some value to it as well.
> 
> "domain=_lt" This will also reset the counters if the total and local events are assigned earlier this action.

The last sentence is not clear to me. Could you please elaborate what
you mean with "are assigned earlier this action"?

Reinette

Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[Moger, Babu]

Hi Reinette,

On 10/2/2024 1:19 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/27/24 10:47 AM, Moger, Babu wrote:
>> On 9/19/2024 12:59 PM, Reinette Chatre wrote:
>>> On 9/4/24 3:21 PM, Babu Moger wrote:
> 
>>>> v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
>>>>       Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
>>>>       Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
>>>>       Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
>>>>       Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
>>>>       Removed ABMC reference in FS code.
>>>>       Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
>>>>       Not sure if we need to change the behaviour here. Processed them sequencially right now.
>>>>       Users have the liberty to pass the flags. Restricting it might be a problem later.
>>>
>>> Could you please give an example of what problem may be encountered later? An assignment
>>> like "domain=_lt" seems like a contradiction to me since user space essentially asks
>>> for "None of the MBM events" as well as "MBM total event" and "MBM local event".
>>
>> I agree it is contradiction. But user is the one who decides to do that. I think we should allow it. Also, there is some value to it as well.
>>
>> "domain=_lt" This will also reset the counters if the total and local events are assigned earlier this action.
> 
> The last sentence is not clear to me. Could you please elaborate what
> you mean with "are assigned earlier this action"?
>

I think I confused you here. "domain=_lt" is equivalent to "domain=lt". 
  My reasoning is handling all the combination in the code adds code 
complexity and leave it the user what he wants to do.
- Babu Moger

Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[Reinette Chatre]

Hi Babu,

On 10/3/24 6:11 PM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 10/2/2024 1:19 PM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 9/27/24 10:47 AM, Moger, Babu wrote:
>>> On 9/19/2024 12:59 PM, Reinette Chatre wrote:
>>>> On 9/4/24 3:21 PM, Babu Moger wrote:
>>
>>>>> v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
>>>>>       Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
>>>>>       Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
>>>>>       Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
>>>>>       Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
>>>>>       Removed ABMC reference in FS code.
>>>>>       Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
>>>>>       Not sure if we need to change the behaviour here. Processed them sequencially right now.
>>>>>       Users have the liberty to pass the flags. Restricting it might be a problem later.
>>>>
>>>> Could you please give an example of what problem may be encountered later? An assignment
>>>> like "domain=_lt" seems like a contradiction to me since user space essentially asks
>>>> for "None of the MBM events" as well as "MBM total event" and "MBM local event".
>>>
>>> I agree it is contradiction. But user is the one who decides to do that. I think we should allow it. Also, there is some value to it as well.
>>>
>>> "domain=_lt" This will also reset the counters if the total and local events are assigned earlier this action.
>>
>> The last sentence is not clear to me. Could you please elaborate what
>> you mean with "are assigned earlier this action"?
>>
> 
> I think I confused you here. "domain=_lt" is equivalent to "domain=lt".  My reasoning is handling all the combination in the code adds code complexity and leave it the user what he wants to do.

hmmm ... and how about "domain=lt_"? Do you think this should also be equivalent to
"domain=lt" or perhaps an expectation that counters should be assigned to the two events
and then immediately unassigned?

Giving user such flexibility may be interpreted as the assignment seen as acting
sequentially through the flags provided. Ideally the interface should behave in
a predictable way if the goal is to provide flexibility to the user. 

Reinette

Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[Moger, Babu]

Hi Reinette,

On 10/3/2024 9:17 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/3/24 6:11 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 10/2/2024 1:19 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 9/27/24 10:47 AM, Moger, Babu wrote:
>>>> On 9/19/2024 12:59 PM, Reinette Chatre wrote:
>>>>> On 9/4/24 3:21 PM, Babu Moger wrote:
>>>
>>>>>> v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
>>>>>>        Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
>>>>>>        Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
>>>>>>        Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
>>>>>>        Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
>>>>>>        Removed ABMC reference in FS code.
>>>>>>        Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
>>>>>>        Not sure if we need to change the behaviour here. Processed them sequencially right now.
>>>>>>        Users have the liberty to pass the flags. Restricting it might be a problem later.
>>>>>
>>>>> Could you please give an example of what problem may be encountered later? An assignment
>>>>> like "domain=_lt" seems like a contradiction to me since user space essentially asks
>>>>> for "None of the MBM events" as well as "MBM total event" and "MBM local event".
>>>>
>>>> I agree it is contradiction. But user is the one who decides to do that. I think we should allow it. Also, there is some value to it as well.
>>>>
>>>> "domain=_lt" This will also reset the counters if the total and local events are assigned earlier this action.
>>>
>>> The last sentence is not clear to me. Could you please elaborate what
>>> you mean with "are assigned earlier this action"?
>>>
>>
>> I think I confused you here. "domain=_lt" is equivalent to "domain=lt".  My reasoning is handling all the combination in the code adds code complexity and leave it the user what he wants to do.
> 
> hmmm ... and how about "domain=lt_"? Do you think this should also be equivalent to
> "domain=lt" or perhaps an expectation that counters should be assigned to the two events
> and then immediately unassigned?

Yes. "domain=lt_" should be "domain=lt".

> 
> Giving user such flexibility may be interpreted as the assignment seen as acting
> sequentially through the flags provided. Ideally the interface should behave in
> a predictable way if the goal is to provide flexibility to the user.
> 

My only concern is adding the check now and reverting it back later.
Basically process the flags sequentially and don't differentiate between 
the flags. I feel it fits the predictable behavior. No?

-- 
- Babu Moger

Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[Reinette Chatre]

Hi Babu,

On 10/4/24 9:38 AM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 10/3/2024 9:17 PM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 10/3/24 6:11 PM, Moger, Babu wrote:
>>> Hi Reinette,
>>>
>>> On 10/2/2024 1:19 PM, Reinette Chatre wrote:
>>>> Hi Babu,
>>>>
>>>> On 9/27/24 10:47 AM, Moger, Babu wrote:
>>>>> On 9/19/2024 12:59 PM, Reinette Chatre wrote:
>>>>>> On 9/4/24 3:21 PM, Babu Moger wrote:
>>>>
>>>>>>> v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
>>>>>>>        Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
>>>>>>>        Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
>>>>>>>        Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
>>>>>>>        Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
>>>>>>>        Removed ABMC reference in FS code.
>>>>>>>        Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
>>>>>>>        Not sure if we need to change the behaviour here. Processed them sequencially right now.
>>>>>>>        Users have the liberty to pass the flags. Restricting it might be a problem later.
>>>>>>
>>>>>> Could you please give an example of what problem may be encountered later? An assignment
>>>>>> like "domain=_lt" seems like a contradiction to me since user space essentially asks
>>>>>> for "None of the MBM events" as well as "MBM total event" and "MBM local event".
>>>>>
>>>>> I agree it is contradiction. But user is the one who decides to do that. I think we should allow it. Also, there is some value to it as well.
>>>>>
>>>>> "domain=_lt" This will also reset the counters if the total and local events are assigned earlier this action.
>>>>
>>>> The last sentence is not clear to me. Could you please elaborate what
>>>> you mean with "are assigned earlier this action"?
>>>>
>>>
>>> I think I confused you here. "domain=_lt" is equivalent to "domain=lt".  My reasoning is handling all the combination in the code adds code complexity and leave it the user what he wants to do.
>>
>> hmmm ... and how about "domain=lt_"? Do you think this should also be equivalent to
>> "domain=lt" or perhaps an expectation that counters should be assigned to the two events
>> and then immediately unassigned?
> 
> Yes. "domain=lt_" should be "domain=lt".
> 
>>
>> Giving user such flexibility may be interpreted as the assignment seen as acting
>> sequentially through the flags provided. Ideally the interface should behave in
>> a predictable way if the goal is to provide flexibility to the user.
>>
> 
> My only concern is adding the check now and reverting it back later.
> Basically process the flags sequentially and don't differentiate between the flags. I feel it fits the predictable behavior. No?

This is the point I was trying to make. If flags are processed sequentially then it would be
predictable behavior and if that is documented expectation then that should be ok. The problem
that I want to highlight is that if predictable sequential processing is the goal then
"domain=_lt" cannot be interpreted the same as "domain="lt_". When flags in "domain=lt_"
are processed sequentially then final state should be "domain=_", no?

If sequential processing is done then "domain=_lt" means "unassign all counters followed
by assign of counter to local MBM monitoring, followed by assign of counter to total MBM
monitoring". Similarly, "domain=lt_" means "assign a counter to local MBM monitoring, then
assign a counter to total MBM monitoring, then unassign all counters".

If this sequential processing is the goal then the implementation would still need to be
adapted. Consider, for example, "domain=lt" ... with sequential processing the user
indicates/expects that "local MBM monitoring" has priority if there is only one counter
available, but the current implementation does not process it sequentially and would end up
assigning counter to "total MBM monitoring" first.

Reinette
 

Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[Moger, Babu]

Hi Reinette,

On 10/4/2024 11:52 AM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/4/24 9:38 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 10/3/2024 9:17 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 10/3/24 6:11 PM, Moger, Babu wrote:
>>>> Hi Reinette,
>>>>
>>>> On 10/2/2024 1:19 PM, Reinette Chatre wrote:
>>>>> Hi Babu,
>>>>>
>>>>> On 9/27/24 10:47 AM, Moger, Babu wrote:
>>>>>> On 9/19/2024 12:59 PM, Reinette Chatre wrote:
>>>>>>> On 9/4/24 3:21 PM, Babu Moger wrote:
>>>>>
>>>>>>>> v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
>>>>>>>>         Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
>>>>>>>>         Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
>>>>>>>>         Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
>>>>>>>>         Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
>>>>>>>>         Removed ABMC reference in FS code.
>>>>>>>>         Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
>>>>>>>>         Not sure if we need to change the behaviour here. Processed them sequencially right now.
>>>>>>>>         Users have the liberty to pass the flags. Restricting it might be a problem later.
>>>>>>>
>>>>>>> Could you please give an example of what problem may be encountered later? An assignment
>>>>>>> like "domain=_lt" seems like a contradiction to me since user space essentially asks
>>>>>>> for "None of the MBM events" as well as "MBM total event" and "MBM local event".
>>>>>>
>>>>>> I agree it is contradiction. But user is the one who decides to do that. I think we should allow it. Also, there is some value to it as well.
>>>>>>
>>>>>> "domain=_lt" This will also reset the counters if the total and local events are assigned earlier this action.
>>>>>
>>>>> The last sentence is not clear to me. Could you please elaborate what
>>>>> you mean with "are assigned earlier this action"?
>>>>>
>>>>
>>>> I think I confused you here. "domain=_lt" is equivalent to "domain=lt".  My reasoning is handling all the combination in the code adds code complexity and leave it the user what he wants to do.
>>>
>>> hmmm ... and how about "domain=lt_"? Do you think this should also be equivalent to
>>> "domain=lt" or perhaps an expectation that counters should be assigned to the two events
>>> and then immediately unassigned?
>>
>> Yes. "domain=lt_" should be "domain=lt".
>>
>>>
>>> Giving user such flexibility may be interpreted as the assignment seen as acting
>>> sequentially through the flags provided. Ideally the interface should behave in
>>> a predictable way if the goal is to provide flexibility to the user.
>>>
>>
>> My only concern is adding the check now and reverting it back later.
>> Basically process the flags sequentially and don't differentiate between the flags. I feel it fits the predictable behavior. No?
> 
> This is the point I was trying to make. If flags are processed sequentially then it would be
> predictable behavior and if that is documented expectation then that should be ok. The problem
> that I want to highlight is that if predictable sequential processing is the goal then
> "domain=_lt" cannot be interpreted the same as "domain="lt_". When flags in "domain=lt_"
> are processed sequentially then final state should be "domain=_", no?

Yes. that is correct.
> 
> If sequential processing is done then "domain=_lt" means "unassign all counters followed
> by assign of counter to local MBM monitoring, followed by assign of counter to total MBM
> monitoring". Similarly, "domain=lt_" means "assign a counter to local MBM monitoring, then
> assign a counter to total MBM monitoring, then unassign all counters".

Yes. That is correct.
> 
> If this sequential processing is the goal then the implementation would still need to be
> adapted. Consider, for example, "domain=lt" ... with sequential processing the user
> indicates/expects that "local MBM monitoring" has priority if there is only one counter
> available, but the current implementation does not process it sequentially and would end up
> assigning counter to "total MBM monitoring" first.

Sure. Lets accommodate the sequential processing. Process the  flags in 
the order it is provided. I need to make few changes to 
rdtgroup_process_flags() to address it. Hopefully, it can be done 
without much complexity. Thanks
-- 
- Babu Moger

Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[Reinette Chatre]

Hi Babu,

On 10/4/24 12:36 PM, Moger, Babu wrote:
> On 10/4/2024 11:52 AM, Reinette Chatre wrote:
>> On 10/4/24 9:38 AM, Moger, Babu wrote:
>>> On 10/3/2024 9:17 PM, Reinette Chatre wrote:
>>>> On 10/3/24 6:11 PM, Moger, Babu wrote:
>>>>> On 10/2/2024 1:19 PM, Reinette Chatre wrote:
>>>>>> On 9/27/24 10:47 AM, Moger, Babu wrote:
>>>>>>> On 9/19/2024 12:59 PM, Reinette Chatre wrote:
>>>>>>>> On 9/4/24 3:21 PM, Babu Moger wrote:
>>>>>>
>>>>>>>>> v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
>>>>>>>>>         Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
>>>>>>>>>         Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
>>>>>>>>>         Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
>>>>>>>>>         Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
>>>>>>>>>         Removed ABMC reference in FS code.
>>>>>>>>>         Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
>>>>>>>>>         Not sure if we need to change the behaviour here. Processed them sequencially right now.
>>>>>>>>>         Users have the liberty to pass the flags. Restricting it might be a problem later.
>>>>>>>>
>>>>>>>> Could you please give an example of what problem may be encountered later? An assignment
>>>>>>>> like "domain=_lt" seems like a contradiction to me since user space essentially asks
>>>>>>>> for "None of the MBM events" as well as "MBM total event" and "MBM local event".
>>>>>>>
>>>>>>> I agree it is contradiction. But user is the one who decides to do that. I think we should allow it. Also, there is some value to it as well.
>>>>>>>
>>>>>>> "domain=_lt" This will also reset the counters if the total and local events are assigned earlier this action.
>>>>>>
>>>>>> The last sentence is not clear to me. Could you please elaborate what
>>>>>> you mean with "are assigned earlier this action"?
>>>>>>
>>>>>
>>>>> I think I confused you here. "domain=_lt" is equivalent to "domain=lt".  My reasoning is handling all the combination in the code adds code complexity and leave it the user what he wants to do.
>>>>
>>>> hmmm ... and how about "domain=lt_"? Do you think this should also be equivalent to
>>>> "domain=lt" or perhaps an expectation that counters should be assigned to the two events
>>>> and then immediately unassigned?
>>>
>>> Yes. "domain=lt_" should be "domain=lt".
>>>
>>>>
>>>> Giving user such flexibility may be interpreted as the assignment seen as acting
>>>> sequentially through the flags provided. Ideally the interface should behave in
>>>> a predictable way if the goal is to provide flexibility to the user.
>>>>
>>>
>>> My only concern is adding the check now and reverting it back later.
>>> Basically process the flags sequentially and don't differentiate between the flags. I feel it fits the predictable behavior. No?
>>
>> This is the point I was trying to make. If flags are processed sequentially then it would be
>> predictable behavior and if that is documented expectation then that should be ok. The problem
>> that I want to highlight is that if predictable sequential processing is the goal then
>> "domain=_lt" cannot be interpreted the same as "domain="lt_". When flags in "domain=lt_"
>> are processed sequentially then final state should be "domain=_", no?
> 
> Yes. that is correct.
>>
>> If sequential processing is done then "domain=_lt" means "unassign all counters followed
>> by assign of counter to local MBM monitoring, followed by assign of counter to total MBM
>> monitoring". Similarly, "domain=lt_" means "assign a counter to local MBM monitoring, then
>> assign a counter to total MBM monitoring, then unassign all counters".
> 
> Yes. That is correct.
>>
>> If this sequential processing is the goal then the implementation would still need to be
>> adapted. Consider, for example, "domain=lt" ... with sequential processing the user
>> indicates/expects that "local MBM monitoring" has priority if there is only one counter
>> available, but the current implementation does not process it sequentially and would end up
>> assigning counter to "total MBM monitoring" first.
> 
> Sure. Lets accommodate the sequential processing. Process the  flags
> in the order it is provided. I need to make few changes to
> rdtgroup_process_flags() to address it. Hopefully, it can be done
> without much complexity. Thanks

I doubt that the implementation would be complex but it may take some effort for it
to be efficient ... taking actions that involve changing kernel and HW state for each
flag as it is encountered vs. parsing all flags and changing kernel and HW state once.

The risk is that a simple request like "domain=lt" may take twice as long when
doing sequential processing. When users provide flags like "domain=_lt" to take advantage
of sequential processing then there may be an argument like "user gets what is being asked
for" when things are slower, but I am not sure the same can be true for a user
that just wants to run "domain=lt".

To me it seems simpler to require that "_" always appears by itself and that
any flags set by the user using "=" are combined during parsing so that they can be
acted on in a single flow. If indeed users want to do something sequentially
like "unassign all flags and then assign local MBM" then instead of "domain=_l"
I think "domain=_;domain=l" could be used?

Reinette

Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[Moger, Babu]

Hi Reinette,

On 10/4/2024 4:09 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/4/24 12:36 PM, Moger, Babu wrote:
>> On 10/4/2024 11:52 AM, Reinette Chatre wrote:
>>> On 10/4/24 9:38 AM, Moger, Babu wrote:
>>>> On 10/3/2024 9:17 PM, Reinette Chatre wrote:
>>>>> On 10/3/24 6:11 PM, Moger, Babu wrote:
>>>>>> On 10/2/2024 1:19 PM, Reinette Chatre wrote:
>>>>>>> On 9/27/24 10:47 AM, Moger, Babu wrote:
>>>>>>>> On 9/19/2024 12:59 PM, Reinette Chatre wrote:
>>>>>>>>> On 9/4/24 3:21 PM, Babu Moger wrote:
>>>>>>>
>>>>>>>>>> v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
>>>>>>>>>>          Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
>>>>>>>>>>          Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
>>>>>>>>>>          Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
>>>>>>>>>>          Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
>>>>>>>>>>          Removed ABMC reference in FS code.
>>>>>>>>>>          Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
>>>>>>>>>>          Not sure if we need to change the behaviour here. Processed them sequencially right now.
>>>>>>>>>>          Users have the liberty to pass the flags. Restricting it might be a problem later.
>>>>>>>>>
>>>>>>>>> Could you please give an example of what problem may be encountered later? An assignment
>>>>>>>>> like "domain=_lt" seems like a contradiction to me since user space essentially asks
>>>>>>>>> for "None of the MBM events" as well as "MBM total event" and "MBM local event".
>>>>>>>>
>>>>>>>> I agree it is contradiction. But user is the one who decides to do that. I think we should allow it. Also, there is some value to it as well.
>>>>>>>>
>>>>>>>> "domain=_lt" This will also reset the counters if the total and local events are assigned earlier this action.
>>>>>>>
>>>>>>> The last sentence is not clear to me. Could you please elaborate what
>>>>>>> you mean with "are assigned earlier this action"?
>>>>>>>
>>>>>>
>>>>>> I think I confused you here. "domain=_lt" is equivalent to "domain=lt".  My reasoning is handling all the combination in the code adds code complexity and leave it the user what he wants to do.
>>>>>
>>>>> hmmm ... and how about "domain=lt_"? Do you think this should also be equivalent to
>>>>> "domain=lt" or perhaps an expectation that counters should be assigned to the two events
>>>>> and then immediately unassigned?
>>>>
>>>> Yes. "domain=lt_" should be "domain=lt".
>>>>
>>>>>
>>>>> Giving user such flexibility may be interpreted as the assignment seen as acting
>>>>> sequentially through the flags provided. Ideally the interface should behave in
>>>>> a predictable way if the goal is to provide flexibility to the user.
>>>>>
>>>>
>>>> My only concern is adding the check now and reverting it back later.
>>>> Basically process the flags sequentially and don't differentiate between the flags. I feel it fits the predictable behavior. No?
>>>
>>> This is the point I was trying to make. If flags are processed sequentially then it would be
>>> predictable behavior and if that is documented expectation then that should be ok. The problem
>>> that I want to highlight is that if predictable sequential processing is the goal then
>>> "domain=_lt" cannot be interpreted the same as "domain="lt_". When flags in "domain=lt_"
>>> are processed sequentially then final state should be "domain=_", no?
>>
>> Yes. that is correct.
>>>
>>> If sequential processing is done then "domain=_lt" means "unassign all counters followed
>>> by assign of counter to local MBM monitoring, followed by assign of counter to total MBM
>>> monitoring". Similarly, "domain=lt_" means "assign a counter to local MBM monitoring, then
>>> assign a counter to total MBM monitoring, then unassign all counters".
>>
>> Yes. That is correct.
>>>
>>> If this sequential processing is the goal then the implementation would still need to be
>>> adapted. Consider, for example, "domain=lt" ... with sequential processing the user
>>> indicates/expects that "local MBM monitoring" has priority if there is only one counter
>>> available, but the current implementation does not process it sequentially and would end up
>>> assigning counter to "total MBM monitoring" first.
>>
>> Sure. Lets accommodate the sequential processing. Process the  flags
>> in the order it is provided. I need to make few changes to
>> rdtgroup_process_flags() to address it. Hopefully, it can be done
>> without much complexity. Thanks
> 
> I doubt that the implementation would be complex but it may take some effort for it
> to be efficient ... taking actions that involve changing kernel and HW state for each
> flag as it is encountered vs. parsing all flags and changing kernel and HW state once.
> 
> The risk is that a simple request like "domain=lt" may take twice as long when
> doing sequential processing. When users provide flags like "domain=_lt" to take advantage
> of sequential processing then there may be an argument like "user gets what is being asked
> for" when things are slower, but I am not sure the same can be true for a user
> that just wants to run "domain=lt".
> 
> To me it seems simpler to require that "_" always appears by itself and that

Ok. Lets go with this approach treating "_" as a special and cannot be 
combined with other flags. Seems simple to implement.

> any flags set by the user using "=" are combined during parsing so that they can be
> acted on in a single flow. If indeed users want to do something sequentially
> like "unassign all flags and then assign local MBM" then instead of "domain=_l"
> I think "domain=_;domain=l" could be used?

Yes. It can be done.
thanks
Babu Moger