[RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 0/5] arm_mpam: Introduce the Narrow-PARTID feature for MPAM driver

[RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 0/5] arm_mpam: Introduce the Narrow-PARTID feature for MPAM driver

[Zeng Heng]

The patch set is applied for mpam/snapshot/v6.12-rc1 branch of
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git
repository.

The narrow-partid feature in MPAM allows for a more efficient use of
PARTIDs by enabling a many-to-one mapping of reqpartids (requested PARTIDs)
to intpartids (internal PARTIDs). This mapping reduces the number of unique
PARTIDs needed, thus allowing more tasks or processes to be monitored and
managed with the available resources.

For a mixture of MSCs system, for MSCs that do not support narrow-partid,
we use the PARTIDs exceeding the number of closids as reqPARTIDs for
expanding the monitoring groups.

In order to keep the existing resctrl API interface, the rmid contains both
req_idx and PMG information instead of PMG only under the MPAM driver. The
req_idx represents the req_idx-th sub-monitoring group under the control
group. The new rmid would be like:

    rmid = (req_idx << shift | pmg).

The new conversion relationship between closid/rmid and (req)PARTID/PMG is:

    (req)PARTID = (rmid.req_idx * n) + closid,
    PMG = rmid.pmg.

Each intPARTID has m reqPARTIDs, which are used to expand the number of
monitoring groups under the control group. Therefore, the number of
monitoring groups is no longer limited by the range of MPAM PMG, which
enhances the extensibility of the system's monitoring capabilities.

---
Compared with v1:
  - Rebase this patch set on latest MPAM driver of the v6.12-rc1 branch.

Compared with v2:
  - Refactor closid/rmid pair translation
  - Simplify the logic of synchronize configuration
  - Remove reqPARTID pool
---

Dave Martin (1):
  arm_mpam: Set INTERNAL as needed when setting MSC controls

Zeng Heng (4):
  arm_mpam: Introduce the definitions of intPARTID and reqPARTID
  arm_mpam: Read monitor value with new closid/rmid pair
  arm_mpam: Automatically synchronize the configuration of all
    sub-monitoring groups
  arm_mpam: Adapting the closid/rmid matching determination functions

 arch/arm64/include/asm/mpam.h               |   6 +-
 drivers/platform/arm64/mpam/mpam_devices.c  |  61 +++++++--
 drivers/platform/arm64/mpam/mpam_internal.h |   5 +
 drivers/platform/arm64/mpam/mpam_resctrl.c  | 142 +++++++++++++++-----
 4 files changed, 170 insertions(+), 44 deletions(-)

--
2.25.1

[RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 1/5] arm_mpam: Introduce the definitions of intPARTID and reqPARTID

[Zeng Heng]

The narrow-partid feature in MPAM allows for a more efficient use of
PARTIDs by enabling a many-to-one mapping of reqpartids (requested PARTIDs)
to intpartids (internal PARTIDs). This mapping reduces the number of unique
PARTIDs needed, thus allowing more tasks or processes to be monitored and
managed with the available resources.

Regarding intPARTID, MPAM uses it as the unit for control group
configuration delivery. MPAM will synchronize the delivered configuration
to all reqPARTIDs mapped to the same intPARTIDs. The number of intPARTIDs
is indicated by MPAMF_PARTID_NRW_IDR.INTPARTID_MAX if implemented, or
directly use the number of PARTID as intpartid_max if narrow-partid feature
is not supported.

reqPARTIDs can be used to expand the number of monitors, for each control
group is no longer simply restricted by the range of PMG. By mapping
between intPARTID and reqPARTID, the number of monitors would be greatly
expanded and more fine-grained monitoring under a control group will be
achieved.

As a MPAM driver applicable to general scenarios, it needs to be compatible
with systems not supporting narrow-partid and mixed MSCs (some MSCs support
narrow-partid and some do not) systems.

We determine the number of closids in the following manner:

  reqPARTID-np -- The number of reqPARTIDs supported by MSCs that support
                  narrow-partid.
  intPARTID-np -- The number of intPARTIDs supported by MSCs that support
                  narrow partid.
  PARTID-nnp   -- The number of PARTIDs supported by MSCs that do not
                  support narrow partid.

  n - Indicates the maximum number of control groups
  l - Represents the total number of reqpartids
  m - Indicates the number of reqpartids per control group

  n = min(intPARTID-np, PARTID-nnp)
  l = min(reqPARTID-np, PARTID-nnp)
  m = l // n

To illustrate how to determine n, l, and m through examples, we can assume
a specific example:

l3   - Supports the narrow-partid feature, supports 32 intPARTIDs, and
       supports 256 reqPARTIDs.
mata - Does not support the narrow PARTID feature, supports a range of 256
       PARTIDs.

Then,
n = min(intPARTID-l3, PARTID-mata) = min(32, 256) = 32
l = min(reqPARTID-l3, PARTID-mata) = min(256,256) = 256
m = 256 / 32 = 8

After initialization, the driver determines the 'n' parameter returned by
resctrl_arch_get_num_closid() and the 'l' parameter returned by
get_num_reqpartid().

Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
 drivers/platform/arm64/mpam/mpam_devices.c  | 12 +++++++++++-
 drivers/platform/arm64/mpam/mpam_internal.h |  2 ++
 drivers/platform/arm64/mpam/mpam_resctrl.c  |  9 +++++++--
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/arm64/mpam/mpam_devices.c b/drivers/platform/arm64/mpam/mpam_devices.c
index 9463045c0011..ca621bb132e9 100644
--- a/drivers/platform/arm64/mpam/mpam_devices.c
+++ b/drivers/platform/arm64/mpam/mpam_devices.c
@@ -67,6 +67,7 @@ static DEFINE_MUTEX(mpam_cpuhp_state_lock);
  * Generating traffic outside this range will result in screaming interrupts.
  */
 u16 mpam_partid_max;
+u16 mpam_intpartid_max;
 u8 mpam_pmg_max;
 static bool partid_max_init, partid_max_published;
 static DEFINE_SPINLOCK(partid_max_lock);
@@ -222,10 +223,16 @@ int mpam_register_requestor(u16 partid_max, u8 pmg_max)
 	spin_lock(&partid_max_lock);
 	if (!partid_max_init) {
 		mpam_partid_max = partid_max;
+		/*
+		 * Update mpam_intpartid_max here, in case the
+		 * system doesn't have narrow-partid feature.
+		 */
+		mpam_intpartid_max = partid_max;
 		mpam_pmg_max = pmg_max;
 		partid_max_init = true;
 	} else if (!partid_max_published) {
 		mpam_partid_max = min(mpam_partid_max, partid_max);
+		mpam_intpartid_max = min(mpam_intpartid_max, partid_max);
 		mpam_pmg_max = min(mpam_pmg_max, pmg_max);
 	} else {
 		/* New requestors can't lower the values */
@@ -984,7 +991,9 @@ static void mpam_ris_hw_probe(struct mpam_msc_ris *ris)
 		u16 partid_max = FIELD_GET(MPAMF_PARTID_NRW_IDR_INTPARTID_MAX, nrwidr);
 
 		mpam_set_feature(mpam_feat_partid_nrw, props);
-		msc->partid_max = min(msc->partid_max, partid_max);
+		msc->intpartid_max = min(msc->partid_max, partid_max);
+	} else {
+		msc->intpartid_max = msc->partid_max;
 	}
 
 	mpam_mon_sel_outer_unlock(msc);
@@ -1046,6 +1055,7 @@ static int mpam_msc_hw_probe(struct mpam_msc *msc)
 
 	spin_lock(&partid_max_lock);
 	mpam_partid_max = min(mpam_partid_max, msc->partid_max);
+	mpam_intpartid_max = min(mpam_intpartid_max, msc->intpartid_max);
 	mpam_pmg_max = min(mpam_pmg_max, msc->pmg_max);
 	spin_unlock(&partid_max_lock);
 
diff --git a/drivers/platform/arm64/mpam/mpam_internal.h b/drivers/platform/arm64/mpam/mpam_internal.h
index 5af6ed60272e..5fc9f09b6945 100644
--- a/drivers/platform/arm64/mpam/mpam_internal.h
+++ b/drivers/platform/arm64/mpam/mpam_internal.h
@@ -86,6 +86,7 @@ struct mpam_msc
 	bool			error_irq_requested;
 	bool			error_irq_hw_enabled;
 	u16			partid_max;
+	u16			intpartid_max;
 	u8			pmg_max;
 	unsigned long		ris_idxs[128 / BITS_PER_LONG];
 	u32			ris_max;
@@ -466,6 +467,7 @@ static inline void mpam_assert_srcu_read_lock_held(void)
 
 /* System wide partid/pmg values */
 extern u16 mpam_partid_max;
+extern u16 mpam_intpartid_max;
 extern u8 mpam_pmg_max;
 
 /* Scheduled work callback to enable mpam once all MSC have been probed */
diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c b/drivers/platform/arm64/mpam/mpam_resctrl.c
index 76ddbce0ea9c..ac3d228befcf 100644
--- a/drivers/platform/arm64/mpam/mpam_resctrl.c
+++ b/drivers/platform/arm64/mpam/mpam_resctrl.c
@@ -162,6 +162,11 @@ static bool mpam_resctrl_hide_cdp(enum resctrl_res_level rid)
  * only the system wide safe value is safe to use.
  */
 u32 resctrl_arch_get_num_closid(struct rdt_resource *ignored)
+{
+	return mpam_intpartid_max + 1;
+}
+
+static u32 get_num_reqpartid(void)
 {
 	return mpam_partid_max + 1;
 }
@@ -169,9 +174,9 @@ u32 resctrl_arch_get_num_closid(struct rdt_resource *ignored)
 u32 resctrl_arch_system_num_rmid_idx(void)
 {
 	u8 closid_shift = fls(mpam_pmg_max);
-	u32 num_partid = resctrl_arch_get_num_closid(NULL);
+	u32 num_reqpartid = get_num_reqpartid();
 
-	return num_partid << closid_shift;
+	return num_reqpartid << closid_shift;
 }
 
 u32 resctrl_arch_rmid_idx_encode(u32 closid, u32 rmid)
-- 
2.25.1

[RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 2/5] arm_mpam: Read monitor value with new closid/rmid pair

[Zeng Heng]

The MPAM driver statically assigns all reqPARTIDs to respective intPARTIDs.
For the new rmid allocation strategy, it will check if there is an
available rmid of any reqPARTID which belongs to the input closid, not just
the rmids belonging to the closid.

For a mixture of MSCs system, for MSCs that do not support narrow-partid,
we use the PARTIDs exceeding the number of closids as reqPARTIDs for
expanding the monitoring groups.

In order to keep the existing resctrl API interface, the rmid contains both
req_idx and PMG information instead of PMG only under the MPAM driver. The
req_idx represents the req_idx-th sub-monitoring group under the control
group. The new rmid would be like:

    rmid = (req_idx << shift | pmg).

The mapping relationships between each group's closid/rmid and the
respective MSCs' intPARTID/reqPARTID/PARTID are illustrated:

n - Indicates the total number of intPARTIDs
m - Indicates the number of reqPARTIDs per intPARTID

P - Partition group (control group)
M - Monitoring group

Group closid rmid.req_idx (req)PARTID      MSCs with narrow-partid  MSCs without narrow-partid
P1    0      -            0                   intPARTID_1              PARTID_1
M1_1  0      0            0                       ├── reqPARTID_1_1       ├── PARTID_1_1
M1_2  0      1            0+n                     ├── reqPARTID_1_2       ├── PARTID_1_2
M1_3  0      2            0+n*2                   ├── reqPARTID_1_3       ├── PARTID_1_3
 ...                                              ├── ...                 ├── ...
M1_m  0      (m-1)        0+n*(m-1)               └── reqPARTID_1_m       └── PARTID_1_m

P2    1      -            1                   intPARTID_2              PARTID_2
M2_1  1      0            1                       ├── reqPARTID_2_1       ├── PARTID_2_1
M2_2  1      1            1+n                     ├── reqPARTID_2_2       ├── PARTID_2_2
M2_3  1      2            1+n*2                   ├── reqPARTID_2_3       ├── PARTID_2_3
 ...                                              ├── ...                 ├── ...
M2_m  1      (m-1)        1+n*(m-1)               └── reqPARTID_2_m       └── PARTID_2_m

Pn    (n-1)  -            (n-1)               intPARTID_n              PARTID_n
Mn_1  (n-1)  0            (n-1)                   ├── reqPARTID_n_1       ├── PARTID_n_1
Mn_2  (n-1)  1            (n-1)+n                 ├── reqPARTID_n_2       ├── PARTID_n_2
Mn_3  (n-1)  2            (n-1)+n*2               ├── reqPARTID_n_3       ├── PARTID_n_3
 ...                                              ├── ...                 ├── ...
Mn_m  (n-1)  (m-1)        (n-1)+n*(m-1) = n*m-1   └── reqPARTID_n_m       └── PARTID_n_m

Based on the example provided, the conversion relationship between
closid/rmid and (req)PARTID/PMG is:

    (req)PARTID = (rmid.req_idx * n) + closid,
    PMG = rmid.pmg.

When the resctrl layer uses the new closid/rmid pair to read or reset the
monitoring values, these new conversion functions(closid_rmid2reqpartid()
and rmid2pmg()) would be utilized to gain the new (req)PARTID/PMG pair.

Since the rmid no longer contains only PMG information, it includes both
*req_idx* and *pmg*. Therefore, the conversion between rmid_idx and
closid/rmid needs to be adapted accordingly too.

Each control group has m (req)PARTIDs, which are used to expand the number
of monitoring groups under the control group. Therefore, the number of
monitoring groups is no longer limited by the range of MPAM's PMG, which
enhances the extensibility of the system's monitoring capabilities.

Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
 drivers/platform/arm64/mpam/mpam_resctrl.c | 64 ++++++++++++++++++----
 1 file changed, 52 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c b/drivers/platform/arm64/mpam/mpam_resctrl.c
index ac3d228befcf..965ff9fd45d3 100644
--- a/drivers/platform/arm64/mpam/mpam_resctrl.c
+++ b/drivers/platform/arm64/mpam/mpam_resctrl.c
@@ -171,6 +171,11 @@ static u32 get_num_reqpartid(void)
 	return mpam_partid_max + 1;
 }
 
+static u32 get_num_reqpartid_per_closid(void)
+{
+	return get_num_reqpartid() / resctrl_arch_get_num_closid(NULL);
+}
+
 u32 resctrl_arch_system_num_rmid_idx(void)
 {
 	u8 closid_shift = fls(mpam_pmg_max);
@@ -179,24 +184,59 @@ u32 resctrl_arch_system_num_rmid_idx(void)
 	return num_reqpartid << closid_shift;
 }
 
+/*
+ * Under MPAM driver, the rmid contains two pieces of information: one is
+ * req_idx, and the other is pmg. Therefore,
+ *     closid_shift = req_shift + pmg_shift.
+ */
 u32 resctrl_arch_rmid_idx_encode(u32 closid, u32 rmid)
 {
-	u8 closid_shift = fls(mpam_pmg_max);
+	u32 rmid_mask;
+	u8 closid_shift;
+	u8 pmg_shift = fls(mpam_pmg_max);
+	u8 req_shift = fls(get_num_reqpartid_per_closid() - 1);
+
+	closid_shift = req_shift + pmg_shift;
+	rmid_mask = ~(~0 << closid_shift);
 
 	BUG_ON(closid_shift > 8);
 
-	return (closid << closid_shift) | rmid;
+	return (closid << closid_shift) | (rmid & rmid_mask);
 }
 
 void resctrl_arch_rmid_idx_decode(u32 idx, u32 *closid, u32 *rmid)
 {
-	u8 closid_shift = fls(mpam_pmg_max);
-	u32 pmg_mask = ~(~0 << closid_shift);
+	u32 rmid_mask;
+	u8 closid_shift;
+	u8 pmg_shift = fls(mpam_pmg_max);
+	u8 req_shift = fls(get_num_reqpartid_per_closid() - 1);
+
+	closid_shift = req_shift + pmg_shift;
+	rmid_mask = ~(~0 << closid_shift);
 
 	BUG_ON(closid_shift > 8);
 
-	*closid = idx >> closid_shift;
-	*rmid = idx & pmg_mask;
+	if (closid)
+		*closid = idx >> closid_shift;
+	if (rmid)
+		*rmid = idx & rmid_mask;
+}
+
+static u32 closid_rmid2reqpartid(u32 closid, u32 rmid)
+{
+	u8 pmg_shift = fls(mpam_pmg_max);
+	u32 req_idx = (rmid >> pmg_shift);
+	u8 intpartid_shift = fls(mpam_intpartid_max);
+
+	return (req_idx << intpartid_shift) | closid;
+}
+
+static u32 rmid2pmg(u32 rmid)
+{
+	u8 pmg_shift = fls(mpam_pmg_max);
+	u32 pmg_mask = ~(~0 << pmg_shift);
+
+	return rmid & pmg_mask;
 }
 
 void resctrl_arch_sched_in(struct task_struct *tsk)
@@ -397,7 +437,7 @@ int resctrl_arch_rmid_read(struct rdt_resource	*r, struct rdt_mon_domain *d,
 		cfg.mon = resctrl_arch_rmid_idx_encode(closid, rmid);
 
 	cfg.match_pmg = true;
-	cfg.pmg = rmid;
+	cfg.pmg = rmid2pmg(rmid);
 	cfg.opts = resctrl_evt_config_to_mpam(dom->mbm_local_evt_cfg);
 
 	if (irqs_disabled()) {
@@ -405,7 +445,7 @@ int resctrl_arch_rmid_read(struct rdt_resource	*r, struct rdt_mon_domain *d,
 		err = -EIO;
 	} else {
 		if (cdp_enabled) {
-			cfg.partid = closid << 1;
+			cfg.partid = closid_rmid2reqpartid(closid, rmid) << 1;
 			err = mpam_msmon_read(dom->comp, &cfg, type, val);
 			if (err)
 				return err;
@@ -415,7 +455,7 @@ int resctrl_arch_rmid_read(struct rdt_resource	*r, struct rdt_mon_domain *d,
 			if (!err)
 				*val += cdp_val;
 		} else {
-			cfg.partid = closid;
+			cfg.partid = closid_rmid2reqpartid(closid, rmid);
 			err = mpam_msmon_read(dom->comp, &cfg, type, val);
 		}
 	}
@@ -434,18 +474,18 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
 
 	cfg.mon = resctrl_arch_rmid_idx_encode(closid, rmid);
 	cfg.match_pmg = true;
-	cfg.pmg = rmid;
+	cfg.pmg = rmid2pmg(rmid);
 
 	dom = container_of(d, struct mpam_resctrl_dom, resctrl_mon_dom);
 
 	if (cdp_enabled) {
-		cfg.partid = closid << 1;
+		cfg.partid = closid_rmid2reqpartid(closid, rmid) << 1;
 		mpam_msmon_reset_mbwu(dom->comp, &cfg);
 
 		cfg.partid += 1;
 		mpam_msmon_reset_mbwu(dom->comp, &cfg);
 	} else {
-		cfg.partid = closid;
+		cfg.partid = closid_rmid2reqpartid(closid, rmid);
 		mpam_msmon_reset_mbwu(dom->comp, &cfg);
 	}
 }
-- 
2.25.1

[RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 3/5] arm_mpam: Set INTERNAL as needed when setting MSC controls

[Zeng Heng]

From: Dave Martin <Dave.Martin@arm.com>

Currently, when an MSC implements PARTID narrowing, MPAMCFG_PART_SEL is
left set to the reqPARTID while programming resource controls in
an MSC.

The MPAM architecture does not guarantee that any particular resource
controls will be updated correctly in this scenario.  Instead,
MPAMCFG_PART_SEL must be written with the corresponding intPARTID and
with the INTERNAL bit set before attempting to program resource controls.
Only the PARTID->intPARTID mappings can be written without the INTERNAL
bit set in MPAMCFG_PART_SEL.

Fix it, by rewriting MPAMCFG_PART_SEL appropriately after setting the
intPARTID mapping.

The MPAMCFG_INTPARTID_INTERNAL flag is not currently accepted as
input to the __mpam_part_sel() helper.  In the interest of keeping
the code clean, break this helper up.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: James Morse <james.morse@arm.com>
---
 drivers/platform/arm64/mpam/mpam_devices.c | 26 +++++++++++++++++-----
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/arm64/mpam/mpam_devices.c b/drivers/platform/arm64/mpam/mpam_devices.c
index ca621bb132e9..781c9146718d 100644
--- a/drivers/platform/arm64/mpam/mpam_devices.c
+++ b/drivers/platform/arm64/mpam/mpam_devices.c
@@ -205,15 +205,27 @@ static u64 mpam_msc_read_esr(struct mpam_msc *msc)
 	return (esr_high << 32) | esr_low;
 }
 
+static void __mpam_part_sel_raw(u32 partsel, struct mpam_msc *msc)
+{
+	lockdep_assert_held(&msc->part_sel_lock);
+	mpam_write_partsel_reg(msc, PART_SEL, partsel);
+}
+
 static void __mpam_part_sel(u8 ris_idx, u16 partid, struct mpam_msc *msc)
 {
-	u32 partsel;
+	u32 partsel = FIELD_PREP(MPAMCFG_PART_SEL_RIS, ris_idx) |
+		      FIELD_PREP(MPAMCFG_PART_SEL_PARTID_SEL, partid);
 
-	lockdep_assert_held(&msc->part_sel_lock);
+	__mpam_part_sel_raw(partsel, msc);
+}
 
-	partsel = FIELD_PREP(MPAMCFG_PART_SEL_RIS, ris_idx) |
-		  FIELD_PREP(MPAMCFG_PART_SEL_PARTID_SEL, partid);
-	mpam_write_partsel_reg(msc, PART_SEL, partsel);
+static void __mpam_intpart_sel(u8 ris_idx, u16 intpartid, struct mpam_msc *msc)
+{
+	u32 partsel = FIELD_PREP(MPAMCFG_PART_SEL_RIS, ris_idx) |
+		      FIELD_PREP(MPAMCFG_PART_SEL_PARTID_SEL, intpartid) |
+		      MPAMCFG_PART_SEL_INTERNAL;
+
+	__mpam_part_sel_raw(partsel, msc);
 }
 
 int mpam_register_requestor(u16 partid_max, u8 pmg_max)
@@ -1540,9 +1552,11 @@ static void mpam_reprogram_ris_partid(struct mpam_msc_ris *ris, u16 partid,
 	mutex_lock(&msc->part_sel_lock);
 	__mpam_part_sel(ris->ris_idx, partid, msc);
 
-	if (mpam_has_feature(mpam_feat_partid_nrw, rprops))
+	if (mpam_has_feature(mpam_feat_partid_nrw, rprops)) {
 		mpam_write_partsel_reg(msc, INTPARTID,
 				       MPAMCFG_INTPARTID_INTERNAL | partid);
+		__mpam_intpart_sel(ris->ris_idx, partid, msc);
+	}
 
 	if (mpam_has_feature(mpam_feat_cpor_part, rprops)) {
 		if (mpam_has_feature(mpam_feat_cpor_part, cfg))
-- 
2.25.1

[RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 4/5] arm_mpam: Automatically synchronize the configuration of all sub-monitoring groups

[Zeng Heng]

After the system expands the narrow-partid feature and statically assigns
all (req)PARTIDs to each control group, the following scenarios require
configuration synchronization operations:

  1. MSCs that support narrow-partid need to establish a mapping between
     reqPARTID and intPARTID after creating a new monitoring group.
  2. MSCs that do not support narrow-partid need to synchronize the
     configuration of sub-monitoring groups after users update the control
     group configuration.

In __write_config(), we synchronize a control group's configuration to each
sub-monitoring group.

Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
 drivers/platform/arm64/mpam/mpam_devices.c  | 25 ++++++++++++++++++---
 drivers/platform/arm64/mpam/mpam_internal.h |  3 +++
 drivers/platform/arm64/mpam/mpam_resctrl.c  | 10 ++++++++-
 3 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/arm64/mpam/mpam_devices.c b/drivers/platform/arm64/mpam/mpam_devices.c
index 781c9146718d..91c5849f76e3 100644
--- a/drivers/platform/arm64/mpam/mpam_devices.c
+++ b/drivers/platform/arm64/mpam/mpam_devices.c
@@ -1544,6 +1544,7 @@ static void mpam_reprogram_ris_partid(struct mpam_msc_ris *ris, u16 partid,
 	u32 pri_val = 0;
 	u16 cmax = MPAMCFG_CMAX_CMAX;
 	u16 bwa_fract = MPAMCFG_MBW_MAX_MAX;
+	u16 intpartid = req2intpartid(partid);
 	struct mpam_msc *msc = ris->vmsc->msc;
 	struct mpam_props *rprops = &ris->props;
 	u16 dspri = GENMASK(rprops->dspri_wd, 0);
@@ -1554,8 +1555,14 @@ static void mpam_reprogram_ris_partid(struct mpam_msc_ris *ris, u16 partid,
 
 	if (mpam_has_feature(mpam_feat_partid_nrw, rprops)) {
 		mpam_write_partsel_reg(msc, INTPARTID,
-				       MPAMCFG_INTPARTID_INTERNAL | partid);
-		__mpam_intpart_sel(ris->ris_idx, partid, msc);
+				       MPAMCFG_INTPARTID_INTERNAL |
+				       intpartid);
+
+		/* Already finish mapping reqPARTID to intPARTID */
+		if (partid != intpartid)
+			goto out;
+
+		__mpam_intpart_sel(ris->ris_idx, intpartid, msc);
 	}
 
 	if (mpam_has_feature(mpam_feat_cpor_part, rprops)) {
@@ -1615,6 +1622,7 @@ static void mpam_reprogram_ris_partid(struct mpam_msc_ris *ris, u16 partid,
 
 	mpam_quirk_post_config_change(ris, partid, cfg);
 
+out:
 	mutex_unlock(&msc->part_sel_lock);
 }
 
@@ -3072,9 +3080,20 @@ struct mpam_write_config_arg {
 
 static int __write_config(void *arg)
 {
+	int closid_num = resctrl_arch_get_num_closid(NULL);
 	struct mpam_write_config_arg *c = arg;
+	u32 reqpartid, req_idx;
+
+	WARN_ON(c->partid >= closid_num);
 
-	mpam_reprogram_ris_partid(c->ris, c->partid, &c->comp->cfg[c->partid]);
+	/* Synchronize the configuration to each sub-monitoring group. */
+	for (req_idx = 0; req_idx < get_num_reqpartid_per_closid();
+	     req_idx++) {
+		reqpartid = req_idx * closid_num + c->partid;
+
+		mpam_reprogram_ris_partid(c->ris, reqpartid,
+					 &c->comp->cfg[c->partid]);
+	}
 
 	return 0;
 }
diff --git a/drivers/platform/arm64/mpam/mpam_internal.h b/drivers/platform/arm64/mpam/mpam_internal.h
index 5fc9f09b6945..c02365338b21 100644
--- a/drivers/platform/arm64/mpam/mpam_internal.h
+++ b/drivers/platform/arm64/mpam/mpam_internal.h
@@ -773,4 +773,7 @@ static inline void mpam_resctrl_teardown_class(struct mpam_class *class) { }
  */
 #define MSMON_CAPT_EVNT_NOW    BIT(0)
 
+u32 get_num_reqpartid_per_closid(void);
+u32 req2intpartid(u32 reqpartid);
+
 #endif /* MPAM_INTERNAL_H */
diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c b/drivers/platform/arm64/mpam/mpam_resctrl.c
index 965ff9fd45d3..47e3a51b0942 100644
--- a/drivers/platform/arm64/mpam/mpam_resctrl.c
+++ b/drivers/platform/arm64/mpam/mpam_resctrl.c
@@ -171,11 +171,19 @@ static u32 get_num_reqpartid(void)
 	return mpam_partid_max + 1;
 }
 
-static u32 get_num_reqpartid_per_closid(void)
+u32 get_num_reqpartid_per_closid(void)
 {
 	return get_num_reqpartid() / resctrl_arch_get_num_closid(NULL);
 }
 
+u32 req2intpartid(u32 reqpartid)
+{
+	u8 intpartid_shift = fls(mpam_intpartid_max);
+	u32 intpartid_mask = ~(~0 << intpartid_shift);
+
+	return reqpartid & intpartid_mask;
+}
+
 u32 resctrl_arch_system_num_rmid_idx(void)
 {
 	u8 closid_shift = fls(mpam_pmg_max);
-- 
2.25.1

[RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 5/5] arm_mpam: Adapting the closid/rmid matching determination functions

[Zeng Heng]

According to the previous patches, add the inverse functions for the
closid/rmid conversion functions to serve as the conversion functions for
reqpartid/pmg. And adapt the matching determination functions
resctrl_arch_match_closid() and resctrl_arch_match_rmid() by the inverse
functions.

For the same reason, when updating the (req)PARTID/PMG pair for a task,
the new conversion functions also are used for adaptation.

Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
 arch/arm64/include/asm/mpam.h              |  6 ++-
 drivers/platform/arm64/mpam/mpam_resctrl.c | 63 +++++++++++++++-------
 2 files changed, 47 insertions(+), 22 deletions(-)

diff --git a/arch/arm64/include/asm/mpam.h b/arch/arm64/include/asm/mpam.h
index e5f385767174..9fc095530671 100644
--- a/arch/arm64/include/asm/mpam.h
+++ b/arch/arm64/include/asm/mpam.h
@@ -93,6 +93,8 @@ static inline u64 mpam_get_regval(struct task_struct *tsk)
 #endif
 }
 
+u32 rmid2pmg(u32 rmid);
+
 static inline void resctrl_arch_set_rmid(struct task_struct *tsk, u32 rmid)
 {
 #ifdef CONFIG_ARM64_MPAM
@@ -100,8 +102,8 @@ static inline void resctrl_arch_set_rmid(struct task_struct *tsk, u32 rmid)
 
 	regval &= ~MPAM1_EL1_PMG_D;
 	regval &= ~MPAM1_EL1_PMG_I;
-	regval |= FIELD_PREP(MPAM1_EL1_PMG_D, rmid);
-	regval |= FIELD_PREP(MPAM1_EL1_PMG_I, rmid);
+	regval |= FIELD_PREP(MPAM1_EL1_PMG_D, rmid2pmg(rmid));
+	regval |= FIELD_PREP(MPAM1_EL1_PMG_I, rmid2pmg(rmid));
 
 	WRITE_ONCE(task_thread_info(tsk)->mpam_partid_pmg, regval);
 #endif
diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c b/drivers/platform/arm64/mpam/mpam_resctrl.c
index 47e3a51b0942..cb5da3de907a 100644
--- a/drivers/platform/arm64/mpam/mpam_resctrl.c
+++ b/drivers/platform/arm64/mpam/mpam_resctrl.c
@@ -239,7 +239,7 @@ static u32 closid_rmid2reqpartid(u32 closid, u32 rmid)
 	return (req_idx << intpartid_shift) | closid;
 }
 
-static u32 rmid2pmg(u32 rmid)
+u32 rmid2pmg(u32 rmid)
 {
 	u8 pmg_shift = fls(mpam_pmg_max);
 	u32 pmg_mask = ~(~0 << pmg_shift);
@@ -247,6 +247,20 @@ static u32 rmid2pmg(u32 rmid)
 	return rmid & pmg_mask;
 }
 
+static u32 reqpartid2closid(u32 reqpartid)
+{
+	return req2intpartid(reqpartid);
+}
+
+static u32 reqpartid_pmg2rmid(u32 reqpartid, u32 pmg)
+{
+	u8 pmg_shift = fls(mpam_pmg_max);
+	u8 intpartid_shift = fls(mpam_intpartid_max);
+	u32 req_idx = (reqpartid >> intpartid_shift);
+
+	return (req_idx << pmg_shift) | pmg;
+}
+
 void resctrl_arch_sched_in(struct task_struct *tsk)
 {
 	lockdep_assert_preemption_disabled();
@@ -256,20 +270,24 @@ void resctrl_arch_sched_in(struct task_struct *tsk)
 
 void resctrl_arch_set_cpu_default_closid_rmid(int cpu, u32 closid, u32 rmid)
 {
-	BUG_ON(closid > U16_MAX);
-	BUG_ON(rmid > U8_MAX);
+	u32 reqpartid = closid_rmid2reqpartid(closid, rmid);
+	u32 pmg = rmid2pmg(rmid);
+	u32 partid_d, partid_i;
+
+	BUG_ON(reqpartid > U16_MAX);
+	BUG_ON(pmg > U8_MAX);
 
 	if (!cdp_enabled) {
-		mpam_set_cpu_defaults(cpu, closid, closid, rmid, rmid);
+		mpam_set_cpu_defaults(cpu, reqpartid, reqpartid, pmg, pmg);
 	} else {
 		/*
 		 * When CDP is enabled, resctrl halves the closid range and we
 		 * use odd/even partid for one closid.
 		 */
-		u32 partid_d = resctrl_get_config_index(closid, CDP_DATA);
-		u32 partid_i = resctrl_get_config_index(closid, CDP_CODE);
+		partid_d = resctrl_get_config_index(reqpartid, CDP_DATA);
+		partid_i = resctrl_get_config_index(reqpartid, CDP_CODE);
 
-		mpam_set_cpu_defaults(cpu, partid_d, partid_i, rmid, rmid);
+		mpam_set_cpu_defaults(cpu, partid_d, partid_i, pmg, pmg);
 	}
 }
 
@@ -289,41 +307,46 @@ void resctrl_arch_sync_cpu_closid_rmid(void *info)
 
 void resctrl_arch_set_closid_rmid(struct task_struct *tsk, u32 closid, u32 rmid)
 {
-	BUG_ON(closid > U16_MAX);
-	BUG_ON(rmid > U8_MAX);
+	u32 reqpartid = closid_rmid2reqpartid(closid, rmid);
+	u32 pmg = rmid2pmg(rmid);
+	u32 partid_d, partid_i;
+
+	BUG_ON(reqpartid > U16_MAX);
+	BUG_ON(pmg > U8_MAX);
 
 	if (!cdp_enabled) {
-		mpam_set_task_partid_pmg(tsk, closid, closid, rmid, rmid);
+		mpam_set_task_partid_pmg(tsk, reqpartid, reqpartid, pmg, pmg);
 	} else {
-		u32 partid_d = resctrl_get_config_index(closid, CDP_DATA);
-		u32 partid_i = resctrl_get_config_index(closid, CDP_CODE);
+		partid_d = resctrl_get_config_index(reqpartid, CDP_DATA);
+		partid_i = resctrl_get_config_index(reqpartid, CDP_CODE);
 
-		mpam_set_task_partid_pmg(tsk, partid_d, partid_i, rmid, rmid);
+		mpam_set_task_partid_pmg(tsk, partid_d, partid_i, pmg, pmg);
 	}
 }
 
 bool resctrl_arch_match_closid(struct task_struct *tsk, u32 closid)
 {
 	u64 regval = mpam_get_regval(tsk);
-	u32 tsk_closid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
+	u32 tsk_partid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
 
 	if (cdp_enabled)
-		tsk_closid >>= 1;
+		tsk_partid >>= 1;
 
-	return tsk_closid == closid;
+	return reqpartid2closid(tsk_partid) == closid;
 }
 
 /* The task's pmg is not unique, the partid must be considered too */
 bool resctrl_arch_match_rmid(struct task_struct *tsk, u32 closid, u32 rmid)
 {
 	u64 regval = mpam_get_regval(tsk);
-	u32 tsk_closid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
-	u32 tsk_rmid = FIELD_GET(MPAM1_EL1_PMG_D, regval);
+	u32 tsk_pmg = FIELD_GET(MPAM1_EL1_PMG_D, regval);
+	u32 tsk_partid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
 
 	if (cdp_enabled)
-		tsk_closid >>= 1;
+		tsk_partid >>= 1;
 
-	return (tsk_closid == closid) && (tsk_rmid == rmid);
+	return (reqpartid2closid(tsk_partid) == closid) &&
+	       (reqpartid_pmg2rmid(tsk_partid, tsk_pmg) == rmid);
 }
 
 struct rdt_resource *resctrl_arch_get_resource(enum resctrl_res_level l)
-- 
2.25.1

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 0/5] arm_mpam: Introduce the Narrow-PARTID feature for MPAM driver

[Dave Martin]

Hi,

On Sat, Dec 07, 2024 at 05:21:31PM +0800, Zeng Heng wrote:
> The patch set is applied for mpam/snapshot/v6.12-rc1 branch of
> https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git
> repository.
> 
> The narrow-partid feature in MPAM allows for a more efficient use of
> PARTIDs by enabling a many-to-one mapping of reqpartids (requested PARTIDs)
> to intpartids (internal PARTIDs). This mapping reduces the number of unique
> PARTIDs needed, thus allowing more tasks or processes to be monitored and
> managed with the available resources.
> 
> For a mixture of MSCs system, for MSCs that do not support narrow-partid,
> we use the PARTIDs exceeding the number of closids as reqPARTIDs for
> expanding the monitoring groups.

Mixed systems still seem not to be handled completely by this series?

You do cope with the scenario where some MSCs support Narrowing and
some do not, but there is still the problem of incompatible controls.

If a non-Narrowing MSC has controls that are not of the "partition
bitmap" type, then splitting a resctrl control group across multiple
PARTIDs is going to change the hardware regulation behaviour.  There
does not seem to be any way to work around this be programming
different control values (for example).  So, there may be over- or
under-allocation of resources compared with what the user requests in
resctrlfs.

So, I think there is still a need to check which controls are present,
and either disable the use of non-identity intPARTID<->reqPARTID
mappings if incompatible controls are present (or don't expose those
controls to resctrl).

(If you were not trying to address this issue yet then that is not a
problem for an RFC, but it is best to be clear about the
limitations...)

> In order to keep the existing resctrl API interface, the rmid contains both
> req_idx and PMG information instead of PMG only under the MPAM driver. The
> req_idx represents the req_idx-th sub-monitoring group under the control
> group. The new rmid would be like:
> 
>     rmid = (req_idx << shift | pmg).
> 
> The new conversion relationship between closid/rmid and (req)PARTID/PMG is:
> 
>     (req)PARTID = (rmid.req_idx * n) + closid,
>     PMG = rmid.pmg.
> 
> Each intPARTID has m reqPARTIDs, which are used to expand the number of
> monitoring groups under the control group. Therefore, the number of
> monitoring groups is no longer limited by the range of MPAM PMG, which
> enhances the extensibility of the system's monitoring capabilities.
> 
> ---
> Compared with v1:
>   - Rebase this patch set on latest MPAM driver of the v6.12-rc1 branch.
> 
> Compared with v2:
>   - Refactor closid/rmid pair translation
>   - Simplify the logic of synchronize configuration
>   - Remove reqPARTID pool
> ---

This approach looks reasonable overall, and in this version the changes
do seem to be better localised in the mpam_resctrl.c glue code now.

I had also been working on a similar approach, so I have posted it for
comparison [1] -- though the two approaches are doing pretty much the
same thing, some details differ.

(Note, I have not addressed PARTID Narrowing at all yet; however,
I think more thought is needed for that.)


Note: Are there bisection issues with some of the patches in the
series?  It looks like not all of the ID conversions are applied in the
same patch, so I'm wondering whether strange behaviour may be seen at
the intermediate commits.

Cheers
---Dave


[1] [RFC PATCH 0/6] Introduce flexible CLOSID/RMID translation
https://lore.kernel.org/lkml/20241212154000.330467-1-Dave.Martin@arm.com/

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 2/5] arm_mpam: Read monitor value with new closid/rmid pair

[Dave Martin]

Hi,

On Sat, Dec 07, 2024 at 05:21:33PM +0800, Zeng Heng wrote:
> The MPAM driver statically assigns all reqPARTIDs to respective intPARTIDs.
> For the new rmid allocation strategy, it will check if there is an
> available rmid of any reqPARTID which belongs to the input closid, not just
> the rmids belonging to the closid.
> 
> For a mixture of MSCs system, for MSCs that do not support narrow-partid,
> we use the PARTIDs exceeding the number of closids as reqPARTIDs for
> expanding the monitoring groups.
> 
> In order to keep the existing resctrl API interface, the rmid contains both
> req_idx and PMG information instead of PMG only under the MPAM driver. The
> req_idx represents the req_idx-th sub-monitoring group under the control
> group. The new rmid would be like:
> 
>     rmid = (req_idx << shift | pmg).
> 
> The mapping relationships between each group's closid/rmid and the
> respective MSCs' intPARTID/reqPARTID/PARTID are illustrated:
> 
> n - Indicates the total number of intPARTIDs
> m - Indicates the number of reqPARTIDs per intPARTID
> 
> P - Partition group (control group)
> M - Monitoring group
> 
> Group closid rmid.req_idx (req)PARTID      MSCs with narrow-partid  MSCs without narrow-partid
> P1    0      -            0                   intPARTID_1              PARTID_1
> M1_1  0      0            0                       ├── reqPARTID_1_1       ├── PARTID_1_1
> M1_2  0      1            0+n                     ├── reqPARTID_1_2       ├── PARTID_1_2
> M1_3  0      2            0+n*2                   ├── reqPARTID_1_3       ├── PARTID_1_3
>  ...                                              ├── ...                 ├── ...
> M1_m  0      (m-1)        0+n*(m-1)               └── reqPARTID_1_m       └── PARTID_1_m
> 
> P2    1      -            1                   intPARTID_2              PARTID_2
> M2_1  1      0            1                       ├── reqPARTID_2_1       ├── PARTID_2_1
> M2_2  1      1            1+n                     ├── reqPARTID_2_2       ├── PARTID_2_2
> M2_3  1      2            1+n*2                   ├── reqPARTID_2_3       ├── PARTID_2_3
>  ...                                              ├── ...                 ├── ...
> M2_m  1      (m-1)        1+n*(m-1)               └── reqPARTID_2_m       └── PARTID_2_m
> 
> Pn    (n-1)  -            (n-1)               intPARTID_n              PARTID_n
> Mn_1  (n-1)  0            (n-1)                   ├── reqPARTID_n_1       ├── PARTID_n_1
> Mn_2  (n-1)  1            (n-1)+n                 ├── reqPARTID_n_2       ├── PARTID_n_2
> Mn_3  (n-1)  2            (n-1)+n*2               ├── reqPARTID_n_3       ├── PARTID_n_3
>  ...                                              ├── ...                 ├── ...
> Mn_m  (n-1)  (m-1)        (n-1)+n*(m-1) = n*m-1   └── reqPARTID_n_m       └── PARTID_n_m
> 
> Based on the example provided, the conversion relationship between
> closid/rmid and (req)PARTID/PMG is:
> 
>     (req)PARTID = (rmid.req_idx * n) + closid,
>     PMG = rmid.pmg.

It seemed more natural to me for the PARTIDs assigned to a particular
CLOSID to be consecutively numbered (see [1]), though it works either
way.

Otherwise, the approach makes sense.

[...]

Cheers
---Dave


[1] [RFC PATCH 4/6] arm_mpam: Introduce flexible CLOSID/RMID translation
https://lore.kernel.org/lkml/20241212154000.330467-5-Dave.Martin@arm.com/

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 4/5] arm_mpam: Automatically synchronize the configuration of all sub-monitoring groups

[Dave Martin]

Hi,

On Sat, Dec 07, 2024 at 05:21:35PM +0800, Zeng Heng wrote:
> After the system expands the narrow-partid feature and statically assigns
> all (req)PARTIDs to each control group, the following scenarios require
> configuration synchronization operations:
> 
>   1. MSCs that support narrow-partid need to establish a mapping between
>      reqPARTID and intPARTID after creating a new monitoring group.
>   2. MSCs that do not support narrow-partid need to synchronize the
>      configuration of sub-monitoring groups after users update the control
>      group configuration.
> 
> In __write_config(), we synchronize a control group's configuration to each
> sub-monitoring group.
> 
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> ---
>  drivers/platform/arm64/mpam/mpam_devices.c  | 25 ++++++++++++++++++---
>  drivers/platform/arm64/mpam/mpam_internal.h |  3 +++
>  drivers/platform/arm64/mpam/mpam_resctrl.c  | 10 ++++++++-
>  3 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/arm64/mpam/mpam_devices.c b/drivers/platform/arm64/mpam/mpam_devices.c
> index 781c9146718d..91c5849f76e3 100644
> --- a/drivers/platform/arm64/mpam/mpam_devices.c
> +++ b/drivers/platform/arm64/mpam/mpam_devices.c
> @@ -1544,6 +1544,7 @@ static void mpam_reprogram_ris_partid(struct mpam_msc_ris *ris, u16 partid,
>  	u32 pri_val = 0;
>  	u16 cmax = MPAMCFG_CMAX_CMAX;
>  	u16 bwa_fract = MPAMCFG_MBW_MAX_MAX;
> +	u16 intpartid = req2intpartid(partid);
>  	struct mpam_msc *msc = ris->vmsc->msc;
>  	struct mpam_props *rprops = &ris->props;
>  	u16 dspri = GENMASK(rprops->dspri_wd, 0);
> @@ -1554,8 +1555,14 @@ static void mpam_reprogram_ris_partid(struct mpam_msc_ris *ris, u16 partid,
>  
>  	if (mpam_has_feature(mpam_feat_partid_nrw, rprops)) {
>  		mpam_write_partsel_reg(msc, INTPARTID,
> -				       MPAMCFG_INTPARTID_INTERNAL | partid);
> -		__mpam_intpart_sel(ris->ris_idx, partid, msc);
> +				       MPAMCFG_INTPARTID_INTERNAL |
> +				       intpartid);
> +
> +		/* Already finish mapping reqPARTID to intPARTID */
> +		if (partid != intpartid)
> +			goto out;
> +
> +		__mpam_intpart_sel(ris->ris_idx, intpartid, msc);
>  	}
>  
>  	if (mpam_has_feature(mpam_feat_cpor_part, rprops)) {

[...]

> @@ -3072,9 +3080,20 @@ struct mpam_write_config_arg {
>  
>  static int __write_config(void *arg)
>  {
> +	int closid_num = resctrl_arch_get_num_closid(NULL);
>  	struct mpam_write_config_arg *c = arg;
> +	u32 reqpartid, req_idx;
> +
> +	WARN_ON(c->partid >= closid_num);
>  
> -	mpam_reprogram_ris_partid(c->ris, c->partid, &c->comp->cfg[c->partid]);
> +	/* Synchronize the configuration to each sub-monitoring group. */
> +	for (req_idx = 0; req_idx < get_num_reqpartid_per_closid();
> +	     req_idx++) {
> +		reqpartid = req_idx * closid_num + c->partid;
> +
> +		mpam_reprogram_ris_partid(c->ris, reqpartid,
> +					 &c->comp->cfg[c->partid]);
> +	}
>  
>  	return 0;
>  }

I haven't decided whether this iteration belongs here or in
mpam_resctrl.c.

Your approach looks like it should work; I do it in
resctrl_arch_update_one() instead [1], but I think the approaches are
pretty much equivalent -- but let me know if you have any thoughts on
it.

[...]

Cheers
---Dave


[1] [RFC PATCH 4/6] arm_mpam: Introduce flexible CLOSID/RMID translation
https://lore.kernel.org/lkml/20241212154000.330467-5-Dave.Martin@arm.com/

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 5/5] arm_mpam: Adapting the closid/rmid matching determination functions

[Dave Martin]

Hi,

On Sat, Dec 07, 2024 at 05:21:36PM +0800, Zeng Heng wrote:
> According to the previous patches, add the inverse functions for the
> closid/rmid conversion functions to serve as the conversion functions for
> reqpartid/pmg. And adapt the matching determination functions
> resctrl_arch_match_closid() and resctrl_arch_match_rmid() by the inverse
> functions.
> 
> For the same reason, when updating the (req)PARTID/PMG pair for a task,
> the new conversion functions also are used for adaptation.
> 
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> ---
>  arch/arm64/include/asm/mpam.h              |  6 ++-
>  drivers/platform/arm64/mpam/mpam_resctrl.c | 63 +++++++++++++++-------
>  2 files changed, 47 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/mpam.h b/arch/arm64/include/asm/mpam.h
> index e5f385767174..9fc095530671 100644
> --- a/arch/arm64/include/asm/mpam.h
> +++ b/arch/arm64/include/asm/mpam.h
> @@ -93,6 +93,8 @@ static inline u64 mpam_get_regval(struct task_struct *tsk)
>  #endif
>  }
>  
> +u32 rmid2pmg(u32 rmid);
> +
>  static inline void resctrl_arch_set_rmid(struct task_struct *tsk, u32 rmid)
>  {
>  #ifdef CONFIG_ARM64_MPAM
> @@ -100,8 +102,8 @@ static inline void resctrl_arch_set_rmid(struct task_struct *tsk, u32 rmid)
>  
>  	regval &= ~MPAM1_EL1_PMG_D;
>  	regval &= ~MPAM1_EL1_PMG_I;
> -	regval |= FIELD_PREP(MPAM1_EL1_PMG_D, rmid);
> -	regval |= FIELD_PREP(MPAM1_EL1_PMG_I, rmid);
> +	regval |= FIELD_PREP(MPAM1_EL1_PMG_D, rmid2pmg(rmid));
> +	regval |= FIELD_PREP(MPAM1_EL1_PMG_I, rmid2pmg(rmid));

Note, this function does not seem to be used; I added a patch in my
series [1] to get rid of it instead of converting it.

>  
>  	WRITE_ONCE(task_thread_info(tsk)->mpam_partid_pmg, regval);
>  #endif
> diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c b/drivers/platform/arm64/mpam/mpam_resctrl.c

[...]

>  void resctrl_arch_set_cpu_default_closid_rmid(int cpu, u32 closid, u32 rmid)
>  {
> -	BUG_ON(closid > U16_MAX);
> -	BUG_ON(rmid > U8_MAX);
> +	u32 reqpartid = closid_rmid2reqpartid(closid, rmid);
> +	u32 pmg = rmid2pmg(rmid);
> +	u32 partid_d, partid_i;
> +
> +	BUG_ON(reqpartid > U16_MAX);
> +	BUG_ON(pmg > U8_MAX);
>  
>  	if (!cdp_enabled) {
> -		mpam_set_cpu_defaults(cpu, closid, closid, rmid, rmid);
> +		mpam_set_cpu_defaults(cpu, reqpartid, reqpartid, pmg, pmg);
>  	} else {
>  		/*
>  		 * When CDP is enabled, resctrl halves the closid range and we
>  		 * use odd/even partid for one closid.
>  		 */
> -		u32 partid_d = resctrl_get_config_index(closid, CDP_DATA);
> -		u32 partid_i = resctrl_get_config_index(closid, CDP_CODE);
> +		partid_d = resctrl_get_config_index(reqpartid, CDP_DATA);
> +		partid_i = resctrl_get_config_index(reqpartid, CDP_CODE);
>  
> -		mpam_set_cpu_defaults(cpu, partid_d, partid_i, rmid, rmid);
> +		mpam_set_cpu_defaults(cpu, partid_d, partid_i, pmg, pmg);

Prior to this patch, will the PARTID and/or PMG programmed for a
control group be different from the PARTID and/or PMG used to program
the MSCs?

If so, those changes probably need to be in the same patch.

[...]

> @@ -289,41 +307,46 @@ void resctrl_arch_sync_cpu_closid_rmid(void *info)

[...]

>  /* The task's pmg is not unique, the partid must be considered too */
>  bool resctrl_arch_match_rmid(struct task_struct *tsk, u32 closid, u32 rmid)
>  {
>  	u64 regval = mpam_get_regval(tsk);
> -	u32 tsk_closid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
> -	u32 tsk_rmid = FIELD_GET(MPAM1_EL1_PMG_D, regval);
> +	u32 tsk_pmg = FIELD_GET(MPAM1_EL1_PMG_D, regval);
> +	u32 tsk_partid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
>  
>  	if (cdp_enabled)
> -		tsk_closid >>= 1;
> +		tsk_partid >>= 1;
>  
> -	return (tsk_closid == closid) && (tsk_rmid == rmid);
> +	return (reqpartid2closid(tsk_partid) == closid) &&
> +	       (reqpartid_pmg2rmid(tsk_partid, tsk_pmg) == rmid);

Do we actually need the reverse mappings here?

It doesn't really matter which ID namespace is used for the
comparison, so in my version of this I converted the passed-in closid
and rmid to PARTID / PMG form and then compared those with tsk's
values.

(But if I've missed some subtlety here, please let me know!)

[...]

Cheers
---Dave


[1] [RFC PATCH 3/6] arm_mpam: Delete unused function resctrl_arch_set_rmid()
https://lore.kernel.org/lkml/20241212154000.330467-4-Dave.Martin@arm.com/

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 2/5] arm_mpam: Read monitor value with new closid/rmid pair

[Zeng Heng]

On 2024/12/13 0:18, Dave Martin wrote:
> Hi,
> 
> On Sat, Dec 07, 2024 at 05:21:33PM +0800, Zeng Heng wrote:
>> The MPAM driver statically assigns all reqPARTIDs to respective intPARTIDs.
>> For the new rmid allocation strategy, it will check if there is an
>> available rmid of any reqPARTID which belongs to the input closid, not just
>> the rmids belonging to the closid.
>>
>> For a mixture of MSCs system, for MSCs that do not support narrow-partid,
>> we use the PARTIDs exceeding the number of closids as reqPARTIDs for
>> expanding the monitoring groups.
>>
>> In order to keep the existing resctrl API interface, the rmid contains both
>> req_idx and PMG information instead of PMG only under the MPAM driver. The
>> req_idx represents the req_idx-th sub-monitoring group under the control
>> group. The new rmid would be like:
>>
>>      rmid = (req_idx << shift | pmg).
>>
>> The mapping relationships between each group's closid/rmid and the
>> respective MSCs' intPARTID/reqPARTID/PARTID are illustrated:
>>
>> n - Indicates the total number of intPARTIDs
>> m - Indicates the number of reqPARTIDs per intPARTID
>>
>> P - Partition group (control group)
>> M - Monitoring group
>>
>> Group closid rmid.req_idx (req)PARTID      MSCs with narrow-partid  MSCs without narrow-partid
>> P1    0      -            0                   intPARTID_1              PARTID_1
>> M1_1  0      0            0                       ├── reqPARTID_1_1       ├── PARTID_1_1
>> M1_2  0      1            0+n                     ├── reqPARTID_1_2       ├── PARTID_1_2
>> M1_3  0      2            0+n*2                   ├── reqPARTID_1_3       ├── PARTID_1_3
>>   ...                                              ├── ...                 ├── ...
>> M1_m  0      (m-1)        0+n*(m-1)               └── reqPARTID_1_m       └── PARTID_1_m
>>
>> P2    1      -            1                   intPARTID_2              PARTID_2
>> M2_1  1      0            1                       ├── reqPARTID_2_1       ├── PARTID_2_1
>> M2_2  1      1            1+n                     ├── reqPARTID_2_2       ├── PARTID_2_2
>> M2_3  1      2            1+n*2                   ├── reqPARTID_2_3       ├── PARTID_2_3
>>   ...                                              ├── ...                 ├── ...
>> M2_m  1      (m-1)        1+n*(m-1)               └── reqPARTID_2_m       └── PARTID_2_m
>>
>> Pn    (n-1)  -            (n-1)               intPARTID_n              PARTID_n
>> Mn_1  (n-1)  0            (n-1)                   ├── reqPARTID_n_1       ├── PARTID_n_1
>> Mn_2  (n-1)  1            (n-1)+n                 ├── reqPARTID_n_2       ├── PARTID_n_2
>> Mn_3  (n-1)  2            (n-1)+n*2               ├── reqPARTID_n_3       ├── PARTID_n_3
>>   ...                                              ├── ...                 ├── ...
>> Mn_m  (n-1)  (m-1)        (n-1)+n*(m-1) = n*m-1   └── reqPARTID_n_m       └── PARTID_n_m
>>
>> Based on the example provided, the conversion relationship between
>> closid/rmid and (req)PARTID/PMG is:
>>
>>      (req)PARTID = (rmid.req_idx * n) + closid,
>>      PMG = rmid.pmg.
> 
> It seemed more natural to me for the PARTIDs assigned to a particular
> CLOSID to be consecutively numbered (see [1]), though it works either
> way.
> 
> Otherwise, the approach makes sense.

Yes, I agree with your point and it would be included in the next 
version soon.


Best Regards,
Zeng Heng

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 5/5] arm_mpam: Adapting the closid/rmid matching determination functions

[Zeng Heng]

On 2024/12/13 0:19, Dave Martin wrote:
> Hi,
> 
> On Sat, Dec 07, 2024 at 05:21:36PM +0800, Zeng Heng wrote:
>> According to the previous patches, add the inverse functions for the
>> closid/rmid conversion functions to serve as the conversion functions for
>> reqpartid/pmg. And adapt the matching determination functions
>> resctrl_arch_match_closid() and resctrl_arch_match_rmid() by the inverse
>> functions.
>>
>> For the same reason, when updating the (req)PARTID/PMG pair for a task,
>> the new conversion functions also are used for adaptation.
>>
>> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
>> ---
>>   arch/arm64/include/asm/mpam.h              |  6 ++-
>>   drivers/platform/arm64/mpam/mpam_resctrl.c | 63 +++++++++++++++-------
>>   2 files changed, 47 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/mpam.h b/arch/arm64/include/asm/mpam.h
>> index e5f385767174..9fc095530671 100644
>> --- a/arch/arm64/include/asm/mpam.h
>> +++ b/arch/arm64/include/asm/mpam.h
>> @@ -93,6 +93,8 @@ static inline u64 mpam_get_regval(struct task_struct *tsk)
>>   #endif
>>   }
>>   
>> +u32 rmid2pmg(u32 rmid);
>> +
>>   static inline void resctrl_arch_set_rmid(struct task_struct *tsk, u32 rmid)
>>   {
>>   #ifdef CONFIG_ARM64_MPAM
>> @@ -100,8 +102,8 @@ static inline void resctrl_arch_set_rmid(struct task_struct *tsk, u32 rmid)
>>   
>>   	regval &= ~MPAM1_EL1_PMG_D;
>>   	regval &= ~MPAM1_EL1_PMG_I;
>> -	regval |= FIELD_PREP(MPAM1_EL1_PMG_D, rmid);
>> -	regval |= FIELD_PREP(MPAM1_EL1_PMG_I, rmid);
>> +	regval |= FIELD_PREP(MPAM1_EL1_PMG_D, rmid2pmg(rmid));
>> +	regval |= FIELD_PREP(MPAM1_EL1_PMG_I, rmid2pmg(rmid));
> 
> Note, this function does not seem to be used; I added a patch in my
> series [1] to get rid of it instead of converting it.
> 

At that time, I was still hesitating whether to directly delete that 
function. Yes, I will skip the adaptation of this function.

>>   
>>   	WRITE_ONCE(task_thread_info(tsk)->mpam_partid_pmg, regval);
>>   #endif
>> diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c b/drivers/platform/arm64/mpam/mpam_resctrl.c
> 
> [...]
> 
>>   void resctrl_arch_set_cpu_default_closid_rmid(int cpu, u32 closid, u32 rmid)
>>   {
>> -	BUG_ON(closid > U16_MAX);
>> -	BUG_ON(rmid > U8_MAX);
>> +	u32 reqpartid = closid_rmid2reqpartid(closid, rmid);
>> +	u32 pmg = rmid2pmg(rmid);
>> +	u32 partid_d, partid_i;
>> +
>> +	BUG_ON(reqpartid > U16_MAX);
>> +	BUG_ON(pmg > U8_MAX);
>>   
>>   	if (!cdp_enabled) {
>> -		mpam_set_cpu_defaults(cpu, closid, closid, rmid, rmid);
>> +		mpam_set_cpu_defaults(cpu, reqpartid, reqpartid, pmg, pmg);
>>   	} else {
>>   		/*
>>   		 * When CDP is enabled, resctrl halves the closid range and we
>>   		 * use odd/even partid for one closid.
>>   		 */
>> -		u32 partid_d = resctrl_get_config_index(closid, CDP_DATA);
>> -		u32 partid_i = resctrl_get_config_index(closid, CDP_CODE);
>> +		partid_d = resctrl_get_config_index(reqpartid, CDP_DATA);
>> +		partid_i = resctrl_get_config_index(reqpartid, CDP_CODE);
>>   
>> -		mpam_set_cpu_defaults(cpu, partid_d, partid_i, rmid, rmid);
>> +		mpam_set_cpu_defaults(cpu, partid_d, partid_i, pmg, pmg);
> 
> Prior to this patch, will the PARTID and/or PMG programmed for a
> control group be different from the PARTID and/or PMG used to program
> the MSCs?
> 
> If so, those changes probably need to be in the same patch.
> 

Thanks to point it out. It's reasonable to put setting PARTID/PMG 
operations together with the closid/rmid pair remapping.

> 
>> @@ -289,41 +307,46 @@ void resctrl_arch_sync_cpu_closid_rmid(void *info)
> 
> [...]
> 
>>   /* The task's pmg is not unique, the partid must be considered too */
>>   bool resctrl_arch_match_rmid(struct task_struct *tsk, u32 closid, u32 rmid)
>>   {
>>   	u64 regval = mpam_get_regval(tsk);
>> -	u32 tsk_closid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
>> -	u32 tsk_rmid = FIELD_GET(MPAM1_EL1_PMG_D, regval);
>> +	u32 tsk_pmg = FIELD_GET(MPAM1_EL1_PMG_D, regval);
>> +	u32 tsk_partid = FIELD_GET(MPAM1_EL1_PARTID_D, regval);
>>   
>>   	if (cdp_enabled)
>> -		tsk_closid >>= 1;
>> +		tsk_partid >>= 1;
>>   
>> -	return (tsk_closid == closid) && (tsk_rmid == rmid);
>> +	return (reqpartid2closid(tsk_partid) == closid) &&
>> +	       (reqpartid_pmg2rmid(tsk_partid, tsk_pmg) == rmid);
> 
> Do we actually need the reverse mappings here?
> 
> It doesn't really matter which ID namespace is used for the
> comparison, so in my version of this I converted the passed-in closid
> and rmid to PARTID / PMG form and then compared those with tsk's
> values.
> 
> (But if I've missed some subtlety here, please let me know!)
> 

Yes, we actually don't need the reverse mappings, and reuse the
closid_rmid2reqpartid() and rmid2pmg() to finish the comparison.

Best regards,
Zeng Heng

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 4/5] arm_mpam: Automatically synchronize the configuration of all sub-monitoring groups

[Zeng Heng]

On 2024/12/13 0:18, Dave Martin wrote:
> Hi,
> 
>> @@ -3072,9 +3080,20 @@ struct mpam_write_config_arg {
>>   
>>   static int __write_config(void *arg)
>>   {
>> +	int closid_num = resctrl_arch_get_num_closid(NULL);
>>   	struct mpam_write_config_arg *c = arg;
>> +	u32 reqpartid, req_idx;
>> +
>> +	WARN_ON(c->partid >= closid_num);
>>   
>> -	mpam_reprogram_ris_partid(c->ris, c->partid, &c->comp->cfg[c->partid]);
>> +	/* Synchronize the configuration to each sub-monitoring group. */
>> +	for (req_idx = 0; req_idx < get_num_reqpartid_per_closid();
>> +	     req_idx++) {
>> +		reqpartid = req_idx * closid_num + c->partid;
>> +
>> +		mpam_reprogram_ris_partid(c->ris, reqpartid,
>> +					 &c->comp->cfg[c->partid]);
>> +	}
>>   
>>   	return 0;
>>   }
> 
> I haven't decided whether this iteration belongs here or in
> mpam_resctrl.c.
> 
> Your approach looks like it should work; I do it in
> resctrl_arch_update_one() instead [1], but I think the approaches are
> pretty much equivalent -- but let me know if you have any thoughts on
> it.
> 

Yes, the actual functions of these two locations are essentially the
same. However, at the __write_config position, we can reduce the
repeated judgments of cfg[partid] in mpam_update_config() and also
decrease the times of smp_call remote invocations.

What about your option towards it?

Best regards,
Zeng Heng

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 0/5] arm_mpam: Introduce the Narrow-PARTID feature for MPAM driver

[Zeng Heng]

On 2024/12/13 0:17, Dave Martin wrote:
> Hi,
> 
> On Sat, Dec 07, 2024 at 05:21:31PM +0800, Zeng Heng wrote:
>> The patch set is applied for mpam/snapshot/v6.12-rc1 branch of
>> https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git
>> repository.
>>
>> The narrow-partid feature in MPAM allows for a more efficient use of
>> PARTIDs by enabling a many-to-one mapping of reqpartids (requested PARTIDs)
>> to intpartids (internal PARTIDs). This mapping reduces the number of unique
>> PARTIDs needed, thus allowing more tasks or processes to be monitored and
>> managed with the available resources.
>>
>> For a mixture of MSCs system, for MSCs that do not support narrow-partid,
>> we use the PARTIDs exceeding the number of closids as reqPARTIDs for
>> expanding the monitoring groups.
> 
> Mixed systems still seem not to be handled completely by this series?
> 
> You do cope with the scenario where some MSCs support Narrowing and
> some do not, but there is still the problem of incompatible controls.
> 
> If a non-Narrowing MSC has controls that are not of the "partition
> bitmap" type, then splitting a resctrl control group across multiple
> PARTIDs is going to change the hardware regulation behaviour.  There
> does not seem to be any way to work around this be programming
> different control values (for example).  So, there may be over- or
> under-allocation of resources compared with what the user requests in
> resctrlfs.
> 
> So, I think there is still a need to check which controls are present,
> and either disable the use of non-identity intPARTID<->reqPARTID
> mappings if incompatible controls are present (or don't expose those
> controls to resctrl).
> 
> (If you were not trying to address this issue yet then that is not a
> problem for an RFC, but it is best to be clear about the
> limitations...)
> 

In the v3, the limitation you mentioned has not yet been handled.
V3 has not yet determined how to define this limitation well. 
Additionally, for v3, I was still uncertain whether a large-scale
refactoring was still necessary at that time.

In fact, I take this limitation very seriously, and I will try to define
the limitation in next version, explaining it separately as a distinct
patch.


>> In order to keep the existing resctrl API interface, the rmid contains both
>> req_idx and PMG information instead of PMG only under the MPAM driver. The
>> req_idx represents the req_idx-th sub-monitoring group under the control
>> group. The new rmid would be like:
>>
>>      rmid = (req_idx << shift | pmg).
>>
>> The new conversion relationship between closid/rmid and (req)PARTID/PMG is:
>>
>>      (req)PARTID = (rmid.req_idx * n) + closid,
>>      PMG = rmid.pmg.
>>
>> Each intPARTID has m reqPARTIDs, which are used to expand the number of
>> monitoring groups under the control group. Therefore, the number of
>> monitoring groups is no longer limited by the range of MPAM PMG, which
>> enhances the extensibility of the system's monitoring capabilities.
>>
>> ---
>> Compared with v1:
>>    - Rebase this patch set on latest MPAM driver of the v6.12-rc1 branch.
>>
>> Compared with v2:
>>    - Refactor closid/rmid pair translation
>>    - Simplify the logic of synchronize configuration
>>    - Remove reqPARTID pool
>> ---
> 
> This approach looks reasonable overall, and in this version the changes
> do seem to be better localised in the mpam_resctrl.c glue code now.
> 
> I had also been working on a similar approach, so I have posted it for
> comparison [1] -- though the two approaches are doing pretty much the
> same thing, some details differ.
> 
> (Note, I have not addressed PARTID Narrowing at all yet; however,
> I think more thought is needed for that.)
> 

Yes, localize the narrow-partid feature within mpam_resctrl.c file is
the biggest restructuring improvement in this version. Of course, thanks
for your meticulous review and insightful comments.

In the meanwhile, I have roughly read through the patch set you sent
out, especially patch 4 (arm_mpam: Introduce flexible CLOSID/RMID
translation). If I have any comments worth discussing, I will try to
share them.

> 
> Note: Are there bisection issues with some of the patches in the
> series?  It looks like not all of the ID conversions are applied in the
> same patch, so I'm wondering whether strange behaviour may be seen at
> the intermediate commits.
> 

The original intention of splitting the patch set was to facilitate 
review. I will merge the part of patch 5 into patch 2 and consider
making the patch set be friendly for bisection issues.


Best regards,
Zeng Heng

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 4/5] arm_mpam: Automatically synchronize the configuration of all sub-monitoring groups

[Dave Martin]

Hi,

On Fri, Dec 20, 2024 at 05:36:23PM +0800, Zeng Heng wrote:
> 
> 
> On 2024/12/13 0:18, Dave Martin wrote:
> > Hi,
> > 
> > > @@ -3072,9 +3080,20 @@ struct mpam_write_config_arg {
> > >   static int __write_config(void *arg)
> > >   {
> > > +	int closid_num = resctrl_arch_get_num_closid(NULL);
> > >   	struct mpam_write_config_arg *c = arg;
> > > +	u32 reqpartid, req_idx;
> > > +
> > > +	WARN_ON(c->partid >= closid_num);
> > > -	mpam_reprogram_ris_partid(c->ris, c->partid, &c->comp->cfg[c->partid]);
> > > +	/* Synchronize the configuration to each sub-monitoring group. */
> > > +	for (req_idx = 0; req_idx < get_num_reqpartid_per_closid();
> > > +	     req_idx++) {
> > > +		reqpartid = req_idx * closid_num + c->partid;
> > > +
> > > +		mpam_reprogram_ris_partid(c->ris, reqpartid,
> > > +					 &c->comp->cfg[c->partid]);
> > > +	}
> > >   	return 0;
> > >   }
> > 
> > I haven't decided whether this iteration belongs here or in
> > mpam_resctrl.c.
> > 
> > Your approach looks like it should work; I do it in
> > resctrl_arch_update_one() instead [1], but I think the approaches are
> > pretty much equivalent -- but let me know if you have any thoughts on
> > it.
> > 
> 
> Yes, the actual functions of these two locations are essentially the
> same. However, at the __write_config position, we can reduce the
> repeated judgments of cfg[partid] in mpam_update_config() and also
> decrease the times of smp_call remote invocations.
> 
> What about your option towards it?

I think either can be done.  I was aiming to keep things as simple as
possible for now, and contain all the mapping logic in mpam_resctrl.c.

I think that with my version of the code, changing the
mpam_apply_config() interface to accept a PARTID range instead of a
single PARTID might be a natural way to do this.

This probably does make sense, in order to avoid excessive SMP cross-
calling; I will have a go and see whether this works.


(Note, there is likely to be redundant cross-calling already, which is
one reason why I did not pay close attention to this issue.  If we
could batch updates separately per group of CPUs then we could reduce
the number of cross-calls, though care would be needed if CPUs can be
hotplugged while processing a batch of updates.  I think that a change
to the resctrl core interface might be necessary if we want the arch
code to be able to queue and schedule updates in this way; resctrl
currently assumes that each update is applied immediately.)

Cheers
---Dave

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 0/5] arm_mpam: Introduce the Narrow-PARTID feature for MPAM driver

[Dave Martin]

On Fri, Dec 20, 2024 at 06:59:31PM +0800, Zeng Heng wrote:
> 
> 
> On 2024/12/13 0:17, Dave Martin wrote:
> > Hi,
> > 
> > On Sat, Dec 07, 2024 at 05:21:31PM +0800, Zeng Heng wrote:

[...]

> > > For a mixture of MSCs system, for MSCs that do not support narrow-partid,
> > > we use the PARTIDs exceeding the number of closids as reqPARTIDs for
> > > expanding the monitoring groups.
> > 
> > Mixed systems still seem not to be handled completely by this series?
> > 
> > You do cope with the scenario where some MSCs support Narrowing and
> > some do not, but there is still the problem of incompatible controls.
> > 
> > If a non-Narrowing MSC has controls that are not of the "partition
> > bitmap" type, then splitting a resctrl control group across multiple
> > PARTIDs is going to change the hardware regulation behaviour.  There
> > does not seem to be any way to work around this be programming
> > different control values (for example).  So, there may be over- or
> > under-allocation of resources compared with what the user requests in
> > resctrlfs.
> > 
> > So, I think there is still a need to check which controls are present,
> > and either disable the use of non-identity intPARTID<->reqPARTID
> > mappings if incompatible controls are present (or don't expose those
> > controls to resctrl).
> > 
> > (If you were not trying to address this issue yet then that is not a
> > problem for an RFC, but it is best to be clear about the
> > limitations...)
> > 
> 
> In the v3, the limitation you mentioned has not yet been handled.
> V3 has not yet determined how to define this limitation well. Additionally,
> for v3, I was still uncertain whether a large-scale
> refactoring was still necessary at that time.
> 
> In fact, I take this limitation very seriously, and I will try to define
> the limitation in next version, explaining it separately as a distinct
> patch.

Fair enough.  I just wanted to make sure that this issue is not missed
(since this is where a lot of the complexity comes from!)


[...]

> > This approach looks reasonable overall, and in this version the changes
> > do seem to be better localised in the mpam_resctrl.c glue code now.
> > 
> > I had also been working on a similar approach, so I have posted it for
> > comparison [1] -- though the two approaches are doing pretty much the
> > same thing, some details differ.
> > 
> > (Note, I have not addressed PARTID Narrowing at all yet; however,
> > I think more thought is needed for that.)
> > 
> 
> Yes, localize the narrow-partid feature within mpam_resctrl.c file is
> the biggest restructuring improvement in this version. Of course, thanks
> for your meticulous review and insightful comments.
> 
> In the meanwhile, I have roughly read through the patch set you sent
> out, especially patch 4 (arm_mpam: Introduce flexible CLOSID/RMID
> translation). If I have any comments worth discussing, I will try to
> share them.

I would appreciate that, thanks!

> 
> > 
> > Note: Are there bisection issues with some of the patches in the
> > series?  It looks like not all of the ID conversions are applied in the
> > same patch, so I'm wondering whether strange behaviour may be seen at
> > the intermediate commits.
> > 
> 
> The original intention of splitting the patch set was to facilitate review.
> I will merge the part of patch 5 into patch 2 and consider
> making the patch set be friendly for bisection issues.
> 
> 
> Best regards,
> Zeng Heng

Understood.

James may have a view on how important bisectability is, since this is
not upstream code -- but it is probably a good idea to maintain full
bisectability if at all possible.

Cheers
---Dave

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 2/5] arm_mpam: Read monitor value with new closid/rmid pair

[Zeng Heng]

On 2024/12/13 0:18, Dave Martin wrote:
> Hi,
> 
> On Sat, Dec 07, 2024 at 05:21:33PM +0800, Zeng Heng wrote:
>> The MPAM driver statically assigns all reqPARTIDs to respective intPARTIDs.
>> For the new rmid allocation strategy, it will check if there is an
>> available rmid of any reqPARTID which belongs to the input closid, not just
>> the rmids belonging to the closid.
>>
>> For a mixture of MSCs system, for MSCs that do not support narrow-partid,
>> we use the PARTIDs exceeding the number of closids as reqPARTIDs for
>> expanding the monitoring groups.
>>
>> In order to keep the existing resctrl API interface, the rmid contains both
>> req_idx and PMG information instead of PMG only under the MPAM driver. The
>> req_idx represents the req_idx-th sub-monitoring group under the control
>> group. The new rmid would be like:
>>
>>      rmid = (req_idx << shift | pmg).
>>
>> The mapping relationships between each group's closid/rmid and the
>> respective MSCs' intPARTID/reqPARTID/PARTID are illustrated:
>>
>> n - Indicates the total number of intPARTIDs
>> m - Indicates the number of reqPARTIDs per intPARTID
>>
>> P - Partition group (control group)
>> M - Monitoring group
>>
>> Group closid rmid.req_idx (req)PARTID      MSCs with narrow-partid  MSCs without narrow-partid
>> P1    0      -            0                   intPARTID_1              PARTID_1
>> M1_1  0      0            0                       ├── reqPARTID_1_1       ├── PARTID_1_1
>> M1_2  0      1            0+n                     ├── reqPARTID_1_2       ├── PARTID_1_2
>> M1_3  0      2            0+n*2                   ├── reqPARTID_1_3       ├── PARTID_1_3
>>   ...                                              ├── ...                 ├── ...
>> M1_m  0      (m-1)        0+n*(m-1)               └── reqPARTID_1_m       └── PARTID_1_m
>>
>> P2    1      -            1                   intPARTID_2              PARTID_2
>> M2_1  1      0            1                       ├── reqPARTID_2_1       ├── PARTID_2_1
>> M2_2  1      1            1+n                     ├── reqPARTID_2_2       ├── PARTID_2_2
>> M2_3  1      2            1+n*2                   ├── reqPARTID_2_3       ├── PARTID_2_3
>>   ...                                              ├── ...                 ├── ...
>> M2_m  1      (m-1)        1+n*(m-1)               └── reqPARTID_2_m       └── PARTID_2_m
>>
>> Pn    (n-1)  -            (n-1)               intPARTID_n              PARTID_n
>> Mn_1  (n-1)  0            (n-1)                   ├── reqPARTID_n_1       ├── PARTID_n_1
>> Mn_2  (n-1)  1            (n-1)+n                 ├── reqPARTID_n_2       ├── PARTID_n_2
>> Mn_3  (n-1)  2            (n-1)+n*2               ├── reqPARTID_n_3       ├── PARTID_n_3
>>   ...                                              ├── ...                 ├── ...
>> Mn_m  (n-1)  (m-1)        (n-1)+n*(m-1) = n*m-1   └── reqPARTID_n_m       └── PARTID_n_m
>>
>> Based on the example provided, the conversion relationship between
>> closid/rmid and (req)PARTID/PMG is:
>>
>>      (req)PARTID = (rmid.req_idx * n) + closid,
>>      PMG = rmid.pmg.
> 
> It seemed more natural to me for the PARTIDs assigned to a particular
> CLOSID to be consecutively numbered (see [1]), though it works either
> way.
> 
> Otherwise, the approach makes sense.
> 


After attempting to change the mapping method in practice, I found that
there are some following advantages of the current method which keeps
intPARTIDs are mapped to the first n IDs:

1. Because closid is exactly equal to intPARTID, and the conversion
relationship between closid and intPARTID remains unchanged under the
current method (still only call the resctrl_get_config_index() for
conversion), maintaining the original semantics during the MPAM
configuration updating;

2. Since there is no need to create a new transformation (like
closid2intpartid()) between closid and intPARTID, this can reduce the
work of function adaptations, such as in resctrl_arch_update_one(),
resctrl_arch_get_config(), and so on, which doesn't need any extra
adaptions and keeps things as simple as possible.

Looking forward to your comments.


Greeting for new year,
Zeng Heng

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 2/5] arm_mpam: Read monitor value with new closid/rmid pair

[Dave Martin]

On Fri, Jan 03, 2025 at 02:55:17PM +0800, Zeng Heng wrote:
> 
> 
> On 2024/12/13 0:18, Dave Martin wrote:
> > Hi,
> > 
> > On Sat, Dec 07, 2024 at 05:21:33PM +0800, Zeng Heng wrote:

[...]

> > > Based on the example provided, the conversion relationship between
> > > closid/rmid and (req)PARTID/PMG is:
> > > 
> > >      (req)PARTID = (rmid.req_idx * n) + closid,
> > >      PMG = rmid.pmg.
> > 
> > It seemed more natural to me for the PARTIDs assigned to a particular
> > CLOSID to be consecutively numbered (see [1]), though it works either
> > way.
> > 
> > Otherwise, the approach makes sense.
> > 
> 
> 
> After attempting to change the mapping method in practice, I found that
> there are some following advantages of the current method which keeps
> intPARTIDs are mapped to the first n IDs:

Thanks for having a go.

> 1. Because closid is exactly equal to intPARTID, and the conversion
> relationship between closid and intPARTID remains unchanged under the
> current method (still only call the resctrl_get_config_index() for
> conversion), maintaining the original semantics during the MPAM
> configuration updating;

You are right about this, but I think this is just moving complexity
around rather than eliminating it?

I've tried various approaches, and there there always seems to be one
ugly step somewhere; either something in mpam_devices.c that feels like
it should be in mpam_resctrl.c, or something in mpam_resctrl.c that
feels like it should be in mpam_devices.c.

> 2. Since there is no need to create a new transformation (like
> closid2intpartid()) between closid and intPARTID, this can reduce the
> work of function adaptations, such as in resctrl_arch_update_one(),
> resctrl_arch_get_config(), and so on, which doesn't need any extra
> adaptions and keeps things as simple as possible.
> 
> Looking forward to your comments.
> 
> 
> Greeting for new year,
> Zeng Heng
> 

Happy New Year to you too!

What you say is true, but I think the runtime cost of the conversions
is going to be trivial compared with the cost of the actual MSC
programming.

For context: I'm hoping to factor the code so that the conversion is as
cleanly separated out as possible, so that it would be straightforward
to move to an arbitrary mapping in the future if it is possible to
agree changes in the core resctrl interface so that the PARTID/PMG
allocations can be dynamic.  If we do that, the conversion would
probably become a simple table lookup.

This factoring seems more important than which precise mapping we
choose right now.

But in the interests of improving PARTID Narrowing support sooner,
I think that going straight to dynamic allocation is not the best
approach -- so my idea is to prepare for that on the MPAM driver side,
but not prioritise developing a dynamic approach until after the
resctrlfs refactoring and the MPAM driver are merged upstream.

Does that make sense?

Cheers
---Dave

Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 2/5] arm_mpam: Read monitor value with new closid/rmid pair

[Zeng Heng]

On 2025/1/3 23:31, Dave Martin wrote:
> On Fri, Jan 03, 2025 at 02:55:17PM +0800, Zeng Heng wrote:
>>
>>
>> On 2024/12/13 0:18, Dave Martin wrote:
>>> Hi,
>>>
>>> On Sat, Dec 07, 2024 at 05:21:33PM +0800, Zeng Heng wrote:
> 
> [...]
> 
>>>> Based on the example provided, the conversion relationship between
>>>> closid/rmid and (req)PARTID/PMG is:
>>>>
>>>>       (req)PARTID = (rmid.req_idx * n) + closid,
>>>>       PMG = rmid.pmg.
>>>
>>> It seemed more natural to me for the PARTIDs assigned to a particular
>>> CLOSID to be consecutively numbered (see [1]), though it works either
>>> way.
>>>
>>> Otherwise, the approach makes sense.
>>>
>>
>>
>> After attempting to change the mapping method in practice, I found that
>> there are some following advantages of the current method which keeps
>> intPARTIDs are mapped to the first n IDs:
> 
> Thanks for having a go.
> 
>> 1. Because closid is exactly equal to intPARTID, and the conversion
>> relationship between closid and intPARTID remains unchanged under the
>> current method (still only call the resctrl_get_config_index() for
>> conversion), maintaining the original semantics during the MPAM
>> configuration updating;
> 
> You are right about this, but I think this is just moving complexity
> around rather than eliminating it?
> 
> I've tried various approaches, and there there always seems to be one
> ugly step somewhere; either something in mpam_devices.c that feels like
> it should be in mpam_resctrl.c, or something in mpam_resctrl.c that
> feels like it should be in mpam_devices.c.
> 
>> 2. Since there is no need to create a new transformation (like
>> closid2intpartid()) between closid and intPARTID, this can reduce the
>> work of function adaptations, such as in resctrl_arch_update_one(),
>> resctrl_arch_get_config(), and so on, which doesn't need any extra
>> adaptions and keeps things as simple as possible.
>>
>> Looking forward to your comments.
>>
>>
>> Greeting for new year,
>> Zeng Heng
>>
> 
> Happy New Year to you too!
> 
> What you say is true, but I think the runtime cost of the conversions
> is going to be trivial compared with the cost of the actual MSC
> programming.
> 
> For context: I'm hoping to factor the code so that the conversion is as
> cleanly separated out as possible, so that it would be straightforward
> to move to an arbitrary mapping in the future if it is possible to
> agree changes in the core resctrl interface so that the PARTID/PMG
> allocations can be dynamic.  If we do that, the conversion would
> probably become a simple table lookup.
> 
> This factoring seems more important than which precise mapping we
> choose right now.
> 
> But in the interests of improving PARTID Narrowing support sooner,
> I think that going straight to dynamic allocation is not the best
> approach -- so my idea is to prepare for that on the MPAM driver side,
> but not prioritise developing a dynamic approach until after the
> resctrlfs refactoring and the MPAM driver are merged upstream.
> 
> Does that make sense?
> 

One of the main reasons for only being able to use static allocation is
the constraint of the alloc_rmid(). Unless we switch to using another
interface (such as resctrl_arch_alloc_rmid()) for rmid allocation, doing
so would definitely break the existing public interface of resctrl.

Consequently, this would increase the difficulty of merging the MPAM
driver into the mainline. (Of course, even after merging into the
upstream, expectations for MPAM to change the common interface still
remain cautious.)

But for how to support dynamic allocation in the future, I have the
following preliminary views and possible directions for evolution:

1. The mapping between closid and intpartid will continue to be
maintained as it is, supporting the NP feature, which should not lead to
changes in the definition of closid, because essentially NP is just an
enhanced feature for monitoring;

2. rmid will no longer only contain pmg information. rmid would be
composed of rmid.req_idx (or directly rmid.reqpartid) and rmid.pmg.

Once the mapping between closid/rmid is determined, the allocation
method will also be determined, and the dynamic allocation process can
be like table lookup.

Currently, I am prioritizing updating the issues you discussed
previously on the basis of v3 and releasing it as v4 for further review.

Both v3 and v4 include the above ideas, and we can continue discussions 
in any direction based on v4.

Thank you in advance.


Best regards,
Zeng Heng