Hi Babu, On Thu, May 2, 2024 at 9:25 AM Moger, Babu <babu.moger@xxxxxxx> wrote: > On 5/1/24 12:48, Peter Newman wrote: > > The FS layer is informed by the arch layer (through rdt_resource > > fields) how many assignable monitors are available and whether a > > monitor is assigned to an entire group or a single event in a group. > > Also, the FS layer can assume that monitors are indexed contiguously, > > allowing it to host the data structures managing FS-level view of > > monitor usage. > > > > I used the following resctrl_arch-interfaces to propagate assignments > > to the implementation: > > > > void resctrl_arch_assign_monitor(struct rdt_domain *d, u32 mon_id, u32 > > closid, u32 rmid, int evtid); > > Sure. I can add these in next version. > > Few comments.. > > AMD does not need closid for assignment. I assume ARM requires closid. Correct, MPAM needs a CLOSID+RMID (PARTID+PMG) to identify a monitoring group. The CLOSID parameter is ignored on x86. > > What is mon_id here? On ABMC, the value is programmed into L3_QOS_ABMC_CFG.CtrID > > > void resctrl_arch_unassign_monitor(struct rdt_domain *d, u32 mon_id); > > We need rmid and evtid for unassign interface here. >From my reading of the ABMC specification, it does not look necessary to program BwSrc or BwType when changing L3_QOS_ABMC_CFG.CtrEn to 0 for a particular CtrID. This interface only disables a counter, so it should not need to know about how it was previously used when assign is able to reassign, as assign will always reset the arch_mbm data. I do not see any harm in the arch_mbm data being stale while the counter is unassigned, because the data is not accessed when reading the hardware counter fails. In general, resctrl_arch_rmid_read() cannot return any information if the hardware counter is not readable at the time it is called. > > > > > > I chose to allow reassigning an assigned monitor without calling > > unassign first. This is important when monitors are unassigned and > > assigned in a single write to mbm_assign_control, as it allows all > > updates to be performed in a single round of parallel IPIs to the > > domains. > > Yes. It is not required to call unassign before assign. Hardware(AMD) > supports it. > > But, we only have 32 counters. We need to know which counter we are going > to use for assignment. If all the counters already assigned, then we can't > figure out the counter id without calling unassigm first. Using the random > counter will overwrite the already assigned counter. I made the caller of resctrl_arch_assign_monitor() responsible for selecting which monitor to assign. As long as the user orders the unassign operations before the assign operations in a write to mbm_assign_control, the FS code will be able to find an available monitor ID. > > I chose to make this a mount option to simplify the management of the > > monitor tracking data structures. They are simply allocated at mount > > time and deallocated and unmount. > > Initially I added it as an mount option. > Based on our earlier discussion, we decided to use the assign feature by > default if hardware supports it. Users don't have to worry about the details. > > > > I called the option "mon_assign": The mount option parser calls > > resctrl_arch_mon_assign_enable() to determine whether the > > implementation supports assignment in some form. If it returns an > > error, the mount fails. When successful, the assignable monitor count > > is made non-zero in the appropriate rdt_resource, triggering the > > behavior change in the FS layer. > > > > I'm still not sure if it's a good idea to enable monitor assignment by > > default. This would be a major disruption in the MBM usage model > > triggered by moving software between AMD CPU models. I thought the > > Why will it be a disruption? Why do you think mount option will solve the > problem? As always, there will be option to go back to legacy mode. right? > > > safest option was to disallow creating more monitoring groups than > > monitors unless the option is selected. Given that nobody else > > Current code allows to create more groups, but it will report "Monitor > assignment failed" when it runs out of monitors. Ok that should be fine then. However, I don't think it's necessary to support dynamically changing the usage model of monitoring groups without remounting. I believe it makes it more difficult for the FS code to generically manage monitor assignment. -Peter
