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

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

[PATCH v7 00/24] x86/resctrl : Support AMD Assignable Bandwidth Monitoring Counters (ABMC)

[Babu Moger]

This series adds the support for Assignable Bandwidth Monitoring Counters
(ABMC). It is also called QoS RMID Pinning feature

Series is written such that it is easier to support other assignable
features supported from different vendors.

The feature details are documented in the  APM listed below [1].
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
Monitoring (ABMC). The documentation is available at
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537

The patches are based on top of commit
a85536e1bce7 (tip/master) Merge branch into tip/master: 'x86/timers'

# Introduction

Users can create as many monitor groups as RMIDs supported by the hardware.
However, bandwidth monitoring feature on AMD system only guarantees that
RMIDs currently assigned to a processor will be tracked by hardware.
The counters of any other RMIDs which are no longer being tracked will be
reset to zero. The MBM event counters return "Unavailable" for the RMIDs
that are not tracked by hardware. So, there can be only limited number of
groups that can give guaranteed monitoring numbers. With ever changing
configurations there is no way to definitely know which of these groups
are being tracked for certain point of time. Users do not have the option
to monitor a group or set of groups for certain period of time without
worrying about RMID being reset in between.
    
The ABMC feature provides an option to the user to assign a hardware
counter to an RMID and monitor the bandwidth as long as it is assigned.
The assigned RMID will be tracked by the hardware until the user unassigns
it manually. There is no need to worry about counters being reset during
this period. Additionally, the user can specify a bitmask identifying the
specific bandwidth types from the given source to track with the counter.

Without ABMC enabled, monitoring will work in current mode without
assignment option.

# Linux Implementation

Create a generic interface aimed to support user space assignment
of scarce counters used for monitoring. First usage of interface
is by ABMC with option to expand usage to "soft-ABMC" and MPAM
counters in future.

Feature adds following interface files:

/sys/fs/resctrl/info/L3_MON/mbm_assign_mode: Reports the list of assignable
monitoring features supported. The enclosed brackets indicate which
feature is enabled.

/sys/fs/resctrl/info/L3_MON/num_mbm_cntrs: Reports the number of monitoring
counters available for assignment.

/sys/fs/resctrl/info/L3_MON/mbm_assign_control: Reports the resctrl group and monitor
status of each group. Assignment state can be updated by writing to the
interface.

# Examples

a. Check if ABMC support is available
	#mount -t resctrl resctrl /sys/fs/resctrl/

	#cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
	[mbm_cntr_assign]
	default

	ABMC feature is detected and it is enabled.

b. Check how many ABMC counters are available. 

	#cat /sys/fs/resctrl/info/L3_MON/num_mbm_cntrs 
	32

c. Create few resctrl groups.

	# mkdir /sys/fs/resctrl/mon_groups/child_default_mon_grp
	# mkdir /sys/fs/resctrl/non_default_ctrl_mon_grp
	# mkdir /sys/fs/resctrl/non_default_ctrl_mon_grp/mon_groups/child_non_default_mon_grp


d. This series adds a new interface file /sys/fs/resctrl/info/L3_MON/mbm_assign_control
   to list and modify the group's monitoring states. File provides single place
   to list monitoring states of all the resctrl groups. It makes it easier for
   user space to to learn about the used counters without needing to traverse
   all the groups thus reducing the number of file system calls.

	The list follows the following format:

	"<CTRL_MON group>/<MON group>/<domain_id>=<flags>"

	Format for specific type of groups:

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

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

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

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

       Flags can be one of the following:

        t  MBM total event is enabled.
        l  MBM local event is enabled.
        tl Both total and local MBM events are enabled.
        _  None of the MBM events are enabled

	Examples:

	# 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;
	
	There are four groups and all the groups have local and total
	event enabled on domain 0 and 1.

e. Update the group assignment states using the interface file /sys/fs/resctrl/info/L3_MON/mbm_assign_control.

 	The write format is similar to the above list format with addition
	of opcode for the assignment operation.
    	“<CTRL_MON group>/<MON group>/<domain_id><opcode><flags>”

	
	* 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>"
	
	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.

	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. Only works with '=' opcode.
	
	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 enable only total 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 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
	remove both local and total 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 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;


f. Read the event mbm_total_bytes and mbm_local_bytes of the default group.
   There is no change in reading the events with ABMC. If the event is unassigned
   when reading, then the read will come back as "Unassigned".
	
	# cat /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes
	779247936
	# cat /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local_bytes 
	765207488
	
g. Check the bandwidth configuration for the group. Note that bandwidth
   configuration has a domain scope. Total event defaults to 0x7F (to
   count all the events) and local event defaults to 0x15 (to count all
   the local numa events). The event bitmap decoding is available at
   https://www.kernel.org/doc/Documentation/x86/resctrl.rst
   in section "mbm_total_bytes_config", "mbm_local_bytes_config":
	
	#cat /sys/fs/resctrl/info/L3_MON/mbm_total_bytes_config 
	0=0x7f;1=0x7f
	
	#cat /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config 
	0=0x15;1=0x15
	
h. Change the bandwidth source for domain 0 for the total event to count only reads.
   Note that this change effects total events on the domain 0.
	
	#echo 0=0x33 > /sys/fs/resctrl/info/L3_MON/mbm_total_bytes_config 
	#cat /sys/fs/resctrl/info/L3_MON/mbm_total_bytes_config 
	0=0x33;1=0x7F
	
i. Now read the total event again. The first read will come back with "Unavailable"
   status. The subsequent read of mbm_total_bytes will display only the read events.
	
	#cat /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes
	Unavailable
	#cat /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes
	314101

j. Users will have the option to go back to 'default' mbm_assign_mode if required.
   This can be done using the following command. Note that switching the
   mbm_assign_mode will reset all the MBM counters of all resctrl groups.

	# echo "default" > /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
	# cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
	mbm_cntr_assign
	[default]

	
k. Unmount the resctrl
	 
	#umount /sys/fs/resctrl/
---
v7:
   Major changes are related to FS and arch codes separation.
   Changed few interface names based on feedback.
   Here are the summary and each patch contains changes specific the patch.

   Removed WARN_ON for num_mbm_cntrs. Decided to dynamically allocate the bitmap.
   WARN_ON is not required anymore.
 
   Renamed the function resctrl_arch_get_abmc_enabled() to resctrl_arch_mbm_cntr_assign_enabled().

   Merged resctrl_arch_mbm_cntr_assign_disable, resctrl_arch_mbm_cntr_assign_disable
   and renamed to resctrl_arch_mbm_cntr_assign_set(). Passed the struct rdt_resource
   to these functions.

   Removed resctrl_arch_reset_rmid_all() from arch code. This will be done from FS the caller.

   Updated the descriptions/commit log in resctrl.rst to generic text. Removed ABMC references.
   Renamed mbm_mode to mbm_assign_mode.
   Renamed mbm_control to  mbm_assign_control.
   Introduced mutex lock in rdtgroup_mbm_mode_show().
 
   The 'legacy' mode is called 'default' mode. 

   Removed the static allocation and now allocating bitmap mbm_cntr_free_map dynamically.

   Merged rdtgroup_assign_cntr(), rdtgroup_alloc_cntr() into one.
   Merged rdtgroup_unassign_cntr(), rdtgroup_free_cntr() into one.
   
  Added struct rdt_resource to the interface functions resctrl_arch_assign_cntr ()
  and resctrl_arch_unassign_cntr().
  Rename rdtgroup_abmc_cfg() to resctrl_abmc_config_one_amd().
   
  Added a new patch to fix counter assignment on event config changes.

  Removed the references of ABMC from user interfaces.

  Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
  Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.

  Thomas Gleixner asked us to update  https://gitlab.com/x86-cpuid.org/x86-cpuid-db. 
  It needs internal approval. We are working on it.

v6:
  We still need to finalize few interface details on mbm_assign_mode and mbm_assign_control
  in case of ABMC and Soft-ABMC. We can continue the discussion with this series.

  Added support for domain-id '*' to update all the domains at once.
  Fixed assign interface to allocate the counter if counter is
  not assigned.   
  Fixed unassign interface to free the counter if the counter is not
  assigned in any of the domains.

  Renamed abmc_capable to mbm_cntr_assignable.

  Renamed abmc_enabled to mbm_cntr_assign_enabled.
  Used msr_set_bit and msr_clear_bit for msr updates.
  Renamed resctrl_arch_abmc_enable() to resctrl_arch_mbm_cntr_assign_enable().
  Renamed resctrl_arch_abmc_disable() to resctrl_arch_mbm_cntr_assign_disable().

  Changed the display name from num_cntrs to num_mbm_cntrs.

  Removed the variable mbm_cntrs_free_map_len. This is not required.
  Removed the call mbm_cntrs_init() in arch code. This needs to be done at higher level.
  Used DECLARE_BITMAP to initialize mbm_cntrs_free_map.
  Removed unused config value definitions.

  Introduced mbm_cntr_map to track counters at domain level. With this
  we dont need to send MSR read to read the counter configuration.

  Separated all the counter id management to upper level in FS code.

  Added checks to detect "Unassigned" before reading the RMID.

  More details in each patch.

v5:
  Rebase changes (because of SNC support)

  Interface changes.
   /sys/fs/resctrl/mbm_assign to /sys/fs/resctrl/mbm_assign_mode.
   /sys/fs/resctrl/mbm_assign_control to /sys/fs/resctrl/mbm_assign_control.

  Added few arch specific routines.
  resctrl_arch_get_abmc_enabled.
  resctrl_arch_abmc_enable.
  resctrl_arch_abmc_disable.

  Few renames
   num_cntrs_free_map -> mbm_cntrs_free_map
   num_cntrs_init -> mbm_cntrs_init
   arch_domain_mbm_evt_config -> resctrl_arch_mbm_evt_config

  Introduced resctrl_arch_event_config_get and
    resctrl_arch_event_config_set() to update event configuration.

  Removed mon_state field mongroup. Added MON_CNTR_UNSET to initialize counters.

  Renamed ctr_id to cntr_id for the hardware counter.
 
  Report "Unassigned" in case the user attempts to read the events without assigning the counter.
  
  ABMC is enabled during the boot up. Can be enabled or disabled later.

  Fixed opcode and flags combination.
    '=_" is valid.
    "-_" amd "+_" is not valid.

 Added all the comments as far as I know. If I missed something, it is not intentional.

v4: 
  Main change is domain specific event assignment.
  Kept the ABMC feature as a default.
  Dynamcic switching between ABMC and mbm_legacy is still allowed.
  We are still not clear about mount option.
  Moved the monitoring related data in resctrl_mon structure from rdt_resource.
  Fixed the display of legacy and ABMC mode.
  Used bimap APIs when possible.
  Removed event configuration read from MSRs. We can use the
  internal saved data.(patch 12)
  Added more comments about L3_QOS_ABMC_CFG MSR.
  Added IPIs to read the assignment status for each domain (patch 18 and 19)
  More details in each patch.

v3:
   This series adds the support for global assignment mode discussed in
   the thread. https://lore.kernel.org/lkml/20231201005720.235639-1-babu.moger@amd.com/
   Removed the individual assignment mode and included the global assignment interface.
   Added following interface files.
   a. /sys/fs/resctrl/info/L3_MON/mbm_assign
      Used for displaying the current assignment mode and switch between
      ABMC and legacy mode.
   b. /sys/fs/resctrl/info/L3_MON/mbm_assign_control
      Used for lising the groups assignment mode and modify the assignment states.
   c. Most of the changes are related to the new interface.
   d. Addressed the comments from Reinette, James and Peter.
   e. Hope I have addressed most of the major feedbacks discussed. If I missed
      something then it is not intentional. Please feel free to comment.
   f. Sending this as an RFC as per Reinette's comment. So, this is still open
      for discussion.

v2:
   a. Major change is the way ABMC is enabled. Earlier, user needed to remount
      with -o abmc to enable ABMC feature. Removed that option now.
      Now users can enable ABMC by "$echo 1 to /sys/fs/resctrl/info/L3_MON/mbm_assign_enable".
     
   b. Added new word 21 to x86/cpufeatures.h.

   c. Display unsupported if user attempts to read the events when ABMC is enabled
      and event is not assigned.

   d. Display monitor_state as "Unsupported" when ABMC is disabled.
  
   e. Text updates and rebase to latest tip tree (as of Jan 18).
 
   f. This series is still work in progress. I am yet to hear from ARM developers. 

v6:
  https://lore.kernel.org/lkml/cover.1722981659.git.babu.moger@amd.com/

v5:
  https://lore.kernel.org/lkml/cover.1720043311.git.babu.moger@amd.com/

v4:
  https://lore.kernel.org/lkml/cover.1716552602.git.babu.moger@amd.com/

v3:
 https://lore.kernel.org/lkml/cover.1711674410.git.babu.moger@amd.com/  

v2:
  https://lore.kernel.org/lkml/20231201005720.235639-1-babu.moger@amd.com/

v1 :
   https://lore.kernel.org/lkml/20231201005720.235639-1-babu.moger@amd.com/


Babu Moger (24):
  x86/cpufeatures: Add support for Assignable Bandwidth Monitoring
    Counters (ABMC)
  x86/resctrl: Add ABMC feature in the command line options
  x86/resctrl: Consolidate monitoring related data from rdt_resource
  x86/resctrl: Detect Assignable Bandwidth Monitoring feature details
  x86/resctrl: Introduce resctrl_file_fflags_init() to initialize fflags
  x86/resctrl: Add support to enable/disable AMD ABMC feature
  x86/resctrl: Introduce the interface to display monitor mode
  x86/resctrl: Introduce interface to display number of monitoring
    counters
  x86/resctrl: Introduce bitmap mbm_cntr_free_map to track assignable
    counters
  x86/resctrl: Introduce mbm_total_cfg and mbm_local_cfg in struct
    rdt_hw_mon_domain
  x86/resctrl: Remove MSR reading of event configuration value
  x86/resctrl: Introduce mbm_cntr_map to track counters at domain
  x86/resctrl: Add data structures and definitions for ABMC assignment
  x86/resctrl: Introduce cntr_id in mongroup for assignments
  x86/resctrl: Implement resctrl_arch_assign_cntr to assign a counter
    with ABMC
  x86/resctrl: Add the interface to assign/update counter assignment
  x86/resctrl: Add the interface to unassign a MBM counter
  x86/resctrl: Auto Assign/unassign counters when mbm_cntr_assign is
    enabled
  x86/resctrl: Report "Unassigned" for MBM events in mbm_cntr_assign
    mode
  x86/resctrl: Introduce the interface to switch between monitor modes
  x86/resctrl: Configure mbm_cntr_assign mode if supported
  x86/resctrl: Update assignments on event configuration changes
  x86/resctrl: Introduce interface to list assignment states of all the
    groups
  x86/resctrl: Introduce interface to modify assignment states of the
    groups

 .../admin-guide/kernel-parameters.txt         |   2 +-
 Documentation/arch/x86/resctrl.rst            | 198 ++++
 arch/x86/include/asm/cpufeatures.h            |   1 +
 arch/x86/include/asm/msr-index.h              |   2 +
 arch/x86/kernel/cpu/cpuid-deps.c              |   3 +
 arch/x86/kernel/cpu/resctrl/core.c            |  19 +-
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c     |  13 +-
 arch/x86/kernel/cpu/resctrl/internal.h        |  77 +-
 arch/x86/kernel/cpu/resctrl/monitor.c         |  90 +-
 arch/x86/kernel/cpu/resctrl/rdtgroup.c        | 875 ++++++++++++++++--
 arch/x86/kernel/cpu/scattered.c               |   1 +
 include/linux/resctrl.h                       |  31 +-
 12 files changed, 1227 insertions(+), 85 deletions(-)

-- 
2.34.1

[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

[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