| Message ID | 20240117141405.3063506-14-amitsinght@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: MPAM: add support for priority partitioning control | expand |
Hi Amit, On Wed, Jan 17, 2024 at 6:15 AM Amit Singh Tomar <amitsinght@marvell.com> wrote: > > +/* Club different resource properties under a class that resctrl uses, > + * for instance, L3 cache that supports both CPOR, and DSPRI need to have > + * knowledge of both cpbm_wd and dspri_wd. This is needed when two controls > + * are enumerated under differnt RIS Index. > + */ > +static void mpam_enable_club_class_features(struct mpam_class *class, > + struct mpam_msc_ris *ris) It looks like "club" is used as a synonym to "class" here to evade the bigger issue that mpam_classes are not defined correctly as DSPRI resources should not be in the same mpam_class as the L3 CPOR and CSU features. This hardware makes it clear that the definition of mpam_class as all resources in a (level x {memory,cache}) needs to be revised. -Peter
Hi Peter, Thanks for having a look. -----Original Message----- From: Peter Newman <peternewman@google.com> Sent: Wednesday, January 17, 2024 11:33 PM To: Amit Singh Tomar <amitsinght@marvell.com> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; fenghua.yu@intel.com; reinette.chatre@intel.com; james.morse@arm.com; George Cherian <gcherian@marvell.com>; robh@kernel.org; dfustini@baylibre.com; jonathan.cameron@huawei.com Subject: [EXT] Re: [PATCH v1 13/14] arm_mpam: Handle resource instances mapped to different controls External Email ---------------------------------------------------------------------- Hi Amit, On Wed, Jan 17, 2024 at 6:15 AM Amit Singh Tomar <amitsinght@marvell.com> wrote: > > +/* Club different resource properties under a class that resctrl > +uses, > + * for instance, L3 cache that supports both CPOR, and DSPRI need to > +have > + * knowledge of both cpbm_wd and dspri_wd. This is needed when two > +controls > + * are enumerated under differnt RIS Index. > + */ > +static void mpam_enable_club_class_features(struct mpam_class *class, > + struct mpam_msc_ris *ris) It looks like "club" is used as a synonym to "class" here to evade the bigger issue that mpam_classes are not defined correctly as DSPRI resources should not be in the same mpam_class as the L3 CPOR and CSU features. This hardware makes it clear that the definition of mpam_class as all resources in a (level x {memory,cache}) needs to be revised. On Marvell platform, DSPRI control register (MPAMCFG_PRI_NS), and Identification Register (MPAMF_PRI_IDR_NS) are implemented within the LLC MPAM block (the address range contains control and identification registers for CPOR, and DSPRI), and therefore we treat DSPRI as one of the L3 resource. However, suppose we approach it as totally different standalone resource type (PRI) other than Cache storage resource type (CPOR, and CCAP), and define a new MPAM class type for it, there is no standard way to discover this new resource type (PRI) from ACPI tables. I'm concerned about accessing DSPRI related registers, if we are going to tide it to new MPAM class (as we discover whole L3 MPAM block using firmware tables, and tide it's resources to L3 MPAM class). Thanks -Amit
[CC Tony Luck] Hi Amit, On Fri, Jan 19, 2024 at 5:01 AM Amit Singh Tomar <amitsinght@marvell.com> wrote: > > Hi Peter, > > Thanks for having a look. > > -----Original Message----- > From: Peter Newman <peternewman@google.com> > Sent: Wednesday, January 17, 2024 11:33 PM > To: Amit Singh Tomar <amitsinght@marvell.com> > Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; fenghua.yu@intel.com; reinette.chatre@intel.com; james.morse@arm.com; George Cherian <gcherian@marvell.com>; robh@kernel.org; dfustini@baylibre.com; jonathan.cameron@huawei.com > Subject: [EXT] Re: [PATCH v1 13/14] arm_mpam: Handle resource instances mapped to different controls > > External Email > > ---------------------------------------------------------------------- > Hi Amit, > > On Wed, Jan 17, 2024 at 6:15 AM Amit Singh Tomar <amitsinght@marvell.com> wrote: > > > > > +/* Club different resource properties under a class that resctrl > > +uses, > > + * for instance, L3 cache that supports both CPOR, and DSPRI need to > > +have > > + * knowledge of both cpbm_wd and dspri_wd. This is needed when two > > +controls > > + * are enumerated under differnt RIS Index. > > + */ > > +static void mpam_enable_club_class_features(struct mpam_class *class, > > + struct mpam_msc_ris *ris) > > It looks like "club" is used as a synonym to "class" here to evade the bigger issue that mpam_classes are not defined correctly as DSPRI resources should not be in the same mpam_class as the L3 CPOR and CSU features. > > This hardware makes it clear that the definition of mpam_class as all resources in a (level x {memory,cache}) needs to be revised. > > On Marvell platform, DSPRI control register (MPAMCFG_PRI_NS), and Identification Register (MPAMF_PRI_IDR_NS) are implemented within the LLC MPAM block (the address range contains control and identification registers for CPOR, and DSPRI), and therefore we treat DSPRI as one of the L3 resource. However, suppose we approach it as totally different standalone resource type (PRI) other than Cache storage resource type (CPOR, and CCAP), and define a new MPAM class type for it, there is no standard way to discover this new resource type (PRI) from ACPI tables. > > I'm concerned about accessing DSPRI related registers, if we are going to tide it to new MPAM class (as we discover whole L3 MPAM block using firmware tables, and tide it's resources to L3 MPAM class). This is becoming more of a discussion of MPAM (and resctrl) in general, so I hope James can participate. Also I should point out that when discussing MPAM, "resource" refers to a non-RIS MSC or a single RIS-index on a RIS-enabled MSC, while the "mpam_class" structure in the code is the counterpart to what RDT (and resctrl) call a resource. From my reading of the code, the consequence of (RIS) resources being in the same mpam_class is that they can be programmed uniformly through a single schema line in the schemata file, so __resource_props_mismatch() goes to work on eliminating any resources (and extra control granularity) which are not programmed exactly the same way. This function seems more geared toward big.LITTLE systems where the cache controls on one cluster are dissimilar from those on the peer cluster and would need to be normalized (James, can you confirm?). But in this situation, it seems like a better idea to present a separate schema for one cluster's controls from the others. For example, "L2P" and "L2E", with non-overlapping domains. In the case of an MSC implementing RIS, the controls are independent by definition, so I can't see why the work done in __resource_props_mismatch() would be applicable. -Peter
Hi Peter, >> >> It looks like "club" is used as a synonym to "class" here to evade the bigger issue that mpam_classes are not defined correctly as DSPRI resources should not be in the same mpam_class as the L3 CPOR and CSU features. >> >> This hardware makes it clear that the definition of mpam_class as all resources in a (level x {memory,cache}) needs to be revised. >> >> On Marvell platform, DSPRI control register (MPAMCFG_PRI_NS), and Identification Register (MPAMF_PRI_IDR_NS) are implemented within the LLC MPAM block (the address range contains control and identification registers for CPOR, and DSPRI), and therefore we treat DSPRI as one of the L3 resource. However, suppose we approach it as totally different standalone resource type (PRI) other than Cache storage resource type (CPOR, and CCAP), and define a new MPAM class type for it, there is no standard way to discover this new resource type (PRI) from ACPI tables. >> >> I'm concerned about accessing DSPRI related registers, if we are going to tide it to new MPAM class (as we discover whole L3 MPAM block using firmware tables, and tide it's resources to L3 MPAM class). > > This is becoming more of a discussion of MPAM (and resctrl) in > general, so I hope James can participate. Also I should point out that > when discussing MPAM, "resource" refers to a non-RIS MSC or a single > RIS-index on a RIS-enabled MSC, while the "mpam_class" structure in > the code is the counterpart to what RDT (and resctrl) call a resource. > > From my reading of the code, the consequence of (RIS) resources being > in the same mpam_class is that they can be programmed uniformly > through a single schema line in the schemata file, so > __resource_props_mismatch() goes to work on eliminating any resources > (and extra control granularity) which are not programmed exactly the > same way. This function seems more geared toward big.LITTLE systems > where the cache controls on one cluster are dissimilar from those on > the peer cluster and would need to be normalized (James, can you > confirm?). But in this situation, it seems like a better idea to > present a separate schema for one cluster's controls from the others. > For example, "L2P" and "L2E", with non-overlapping domains. > > In the case of an MSC implementing RIS, the controls are independent > by definition, This greatly relies on the implementation. For an MSC, identical control may be situated at two separate indexes, each with different attributes. For instance, let's take an example of MARVELL implementation, There are three different resources at index 0,1,2. These are enumerated in TAD_CMN_MPAM_RIS_E: 0: MSC 1: LTG 2: DTG LLC MSC, resource at index 1, and 2 possesses cache portion partitioning feature, i.e., If MPAMCFG_PART_SEL_NS[RIS] is set to 1 (LTG) or to 2 (DTG), then MPAMF_IDR_NS[HAS_CPOR_PART] is set to 1. LTG resource has 16 portion bitmap, and DTG has 18 portion bitmap (mapped to same CPOR control), and only one can be configured. so I can't see why the work done in > __resource_props_mismatch() would be applicable. > IMHO, __resource_props_mismatch would be needed for such cases. Thanks -Amit
diff --git a/drivers/platform/mpam/mpam_devices.c b/drivers/platform/mpam/mpam_devices.c index 388d57281fd8..df7ac2c8bcb3 100644 --- a/drivers/platform/mpam/mpam_devices.c +++ b/drivers/platform/mpam/mpam_devices.c @@ -1788,11 +1788,15 @@ __resource_props_mismatch(struct mpam_msc_ris *ris, struct mpam_class *class) lockdep_assert_held(&mpam_list_lock); /* we modify class */ - /* Clear missing features */ - cprops->features &= rprops->features; + /* Clear missing features, we don't want to clear features, + * when different controls are implemented at different RIS + * index. + */ + if (class->props.features == ris->props.features) + cprops->features &= rprops->features; /* Clear incompatible features */ - if (cprops->cpbm_wd != rprops->cpbm_wd) + if (cprops->cpbm_wd != rprops->cpbm_wd && rprops->cpbm_wd) mpam_clear_feature(mpam_feat_cpor_part, &cprops->features); if (cprops->mbw_pbm_bits != rprops->mbw_pbm_bits) mpam_clear_feature(mpam_feat_mbw_part, &cprops->features); @@ -1802,14 +1806,14 @@ __resource_props_mismatch(struct mpam_msc_ris *ris, struct mpam_class *class) cprops->bwa_wd = min(cprops->bwa_wd, rprops->bwa_wd); /* For num properties, take the minimum */ - if (cprops->num_csu_mon != rprops->num_csu_mon) + if (cprops->num_csu_mon != rprops->num_csu_mon && rprops->num_csu_mon) cprops->num_csu_mon = min(cprops->num_csu_mon, rprops->num_csu_mon); if (cprops->num_mbwu_mon != rprops->num_mbwu_mon) cprops->num_mbwu_mon = min(cprops->num_mbwu_mon, rprops->num_mbwu_mon); if (cprops->intpri_wd != rprops->intpri_wd) cprops->intpri_wd = min(cprops->intpri_wd, rprops->intpri_wd); - if (cprops->dspri_wd != rprops->dspri_wd) + if (cprops->dspri_wd != rprops->dspri_wd && rprops->dspri_wd) cprops->dspri_wd = min(cprops->dspri_wd, rprops->dspri_wd); /* {int,ds}pri may not have differing 0-low behaviour */ @@ -1845,6 +1849,20 @@ static void mpam_enable_init_class_features(struct mpam_class *class) class->props = ris->props; } +/* Club different resource properties under a class that resctrl uses, + * for instance, L3 cache that supports both CPOR, and DSPRI need to have + * knowledge of both cpbm_wd and dspri_wd. This is needed when two controls + * are enumerated under differnt RIS Index. + */ +static void mpam_enable_club_class_features(struct mpam_class *class, + struct mpam_msc_ris *ris) +{ + class->props.features |= ris->props.features; + class->props.cpbm_wd |= ris->props.cpbm_wd; + class->props.dspri_wd |= ris->props.dspri_wd; + class->props.num_csu_mon |= ris->props.num_csu_mon; +} + /* Merge all the common resource features into class. */ static void mpam_enable_merge_features(void) { @@ -1860,6 +1878,14 @@ static void mpam_enable_merge_features(void) list_for_each_entry(comp, &class->components, class_list) { list_for_each_entry(ris, &comp->ris, comp_list) { __resource_props_mismatch(ris, class); + /* + * There can be multiple resources under a class which is + * mapped to different controls, For instance L3 cache + * can have both CPOR and DSPRI implemented with different + * RIS. + */ + if (class->props.features != ris->props.features) + mpam_enable_club_class_features(class, ris); class->nrdy_usec = max(class->nrdy_usec, ris->msc->nrdy_usec);
At the moment, configuring multiple resource instances (mapped to same control) under a resource class is not supported. For instance, on MARVELL SoC MPAMF_IDR_NS[RIS_MAX] (under LLC MSC) is 0x2, and there are three different resource at index 0,1,2. These are enumerated in TAD_CMN_MPAM_RIS_E: 0: MSC 1: LTG 2: DTG LLC MSC resource at index 0 has the Priority partitioning features. If MPAMCFG_PART_SEL_NS[RIS] is set to 0 (MSC), then MPAMF_IDR_NS[HAS_PRI_PART] is set to 1, leaving HAS_CPOR_PART bit to 0. CPOR and PRI_PART are mutually exclusive resources as far configuration is concerned. With this change, multiple resource instances that maps to different control, say LTG for CPOR, and MSC for PRI_PART is handled properly. Signed-off-by: Amit Singh Tomar <amitsinght@marvell.com> --- Changes since RFC: * Trimmed down the commit message. * Tried to handle the corner case as suggested by Jonathan by calling the __resource_props_mismatch even when different controls are enumerated at different RIS index. --- drivers/platform/mpam/mpam_devices.c | 36 ++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 5 deletions(-)