On Wed, 10 Sep 2025 20:42:54 +0000
James Morse <james.morse@xxxxxxx> wrote:
> To make a decision about whether to expose an mpam class as
> a resctrl resource we need to know its overall supported
> features and properties.
>
> Once we've probed all the resources, we can walk the tree
> and produce overall values by merging the bitmaps. This
> eliminates features that are only supported by some MSC
> that make up a component or class.
>
> If bitmap properties are mismatched within a component we
> cannot support the mismatched feature.
>
> Care has to be taken as vMSC may hold mismatched RIS.
>
> Signed-off-by: James Morse <james.morse@xxxxxxx>
> Reviewed-by: Ben Horgan <ben.horgan@xxxxxxx>
A trivial things inline.
Reviewed-by: Jonathan Cameron <jonathan.cameron@xxxxxxxxxx>
> +/*
> + * Combine two props fields.
> + * If this is for controls that alias the same resource, it is safe to just
> + * copy the values over. If two aliasing controls implement the same scheme
> + * a safe value must be picked.
> + * For non-aliasing controls, these control different resources, and the
> + * resulting safe value must be compatible with both. When merging values in
> + * the tree, all the aliasing resources must be handled first.
> + * On mismatch, parent is modified.
> + */
> +static void __props_mismatch(struct mpam_props *parent,
> + struct mpam_props *child, bool alias)
> +{
> + if (CAN_MERGE_FEAT(parent, child, mpam_feat_cpor_part, alias)) {
> + parent->cpbm_wd = child->cpbm_wd;
> + } else if (MISMATCHED_FEAT(parent, child, mpam_feat_cpor_part,
> + cpbm_wd, alias)) {
> + pr_debug("%s cleared cpor_part\n", __func__);
> + mpam_clear_feature(mpam_feat_cpor_part, &parent->features);
> + parent->cpbm_wd = 0;
> + }
> +
> + if (CAN_MERGE_FEAT(parent, child, mpam_feat_mbw_part, alias)) {
> + parent->mbw_pbm_bits = child->mbw_pbm_bits;
> + } else if (MISMATCHED_FEAT(parent, child, mpam_feat_mbw_part,
> + mbw_pbm_bits, alias)) {
> + pr_debug("%s cleared mbw_part\n", __func__);
> + mpam_clear_feature(mpam_feat_mbw_part, &parent->features);
> + parent->mbw_pbm_bits = 0;
> + }
> +
> + /* bwa_wd is a count of bits, fewer bits means less precision */
> + if (alias && !mpam_has_bwa_wd_feature(parent) && mpam_has_bwa_wd_feature(child)) {
Seems like an overly long line given other local wrapping.
> + parent->bwa_wd = child->bwa_wd;
> + } else if (MISMATCHED_HELPER(parent, child, mpam_has_bwa_wd_feature,
> + bwa_wd, alias)) {
> + pr_debug("%s took the min bwa_wd\n", __func__);
> + parent->bwa_wd = min(parent->bwa_wd, child->bwa_wd);
> + }
> +
> + /* For num properties, take the minimum */
> + if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_csu, alias)) {
> + parent->num_csu_mon = child->num_csu_mon;
> + } else if (MISMATCHED_FEAT(parent, child, mpam_feat_msmon_csu,
> + num_csu_mon, alias)) {
> + pr_debug("%s took the min num_csu_mon\n", __func__);
> + parent->num_csu_mon = min(parent->num_csu_mon, child->num_csu_mon);
> + }
> +
> + if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_mbwu, alias)) {
> + parent->num_mbwu_mon = child->num_mbwu_mon;
> + } else if (MISMATCHED_FEAT(parent, child, mpam_feat_msmon_mbwu,
> + num_mbwu_mon, alias)) {
> + pr_debug("%s took the min num_mbwu_mon\n", __func__);
> + parent->num_mbwu_mon = min(parent->num_mbwu_mon, child->num_mbwu_mon);
> + }
> +
> +/*
> + * If a vmsc doesn't match class feature/configuration, do the right thing(tm).
> + * For 'num' properties we can just take the minimum.
> + * For properties where the mismatched unused bits would make a difference, we
> + * nobble the class feature, as we can't configure all the resources.
> + * e.g. The L3 cache is composed of two resources with 13 and 17 portion
> + * bitmaps respectively.
> + */
> +static void
> +__class_props_mismatch(struct mpam_class *class, struct mpam_vmsc *vmsc)
I'm not really sure what the __ prefix denotes here.
> +{
> + struct mpam_props *cprops = &class->props;
> + struct mpam_props *vprops = &vmsc->props;
> +
> + lockdep_assert_held(&mpam_list_lock); /* we modify class */
> +
> + pr_debug("%s: Merging features for class:0x%lx &= vmsc:0x%lx\n",
> + dev_name(&vmsc->msc->pdev->dev),
> + (long)cprops->features, (long)vprops->features);
According to https://docs.kernel.org/core-api/printk-formats.html
should be fine using %x for u16 values. So why dance through a cast to long?
> +
> + /* Take the safe value for any common features */
> + __props_mismatch(cprops, vprops, false);
> +}
> +
> +static void
> +__vmsc_props_mismatch(struct mpam_vmsc *vmsc, struct mpam_msc_ris *ris)
> +{
> + struct mpam_props *rprops = &ris->props;
> + struct mpam_props *vprops = &vmsc->props;
> +
> + lockdep_assert_held(&mpam_list_lock); /* we modify vmsc */
> +
> + pr_debug("%s: Merging features for vmsc:0x%lx |= ris:0x%lx\n",
> + dev_name(&vmsc->msc->pdev->dev),
> + (long)vprops->features, (long)rprops->features);
Same as above comment on casts being unnecessary.
> +
> + /*
> + * Merge mismatched features - Copy any features that aren't common,
> + * but take the safe value for any common features.
> + */
> + __props_mismatch(vprops, rprops, true);
> +}
> +
> +/*
> + * Copy the first component's first vMSC's properties and features to the
> + * class. __class_props_mismatch() will remove conflicts.
> + * It is not possible to have a class with no components, or a component with
> + * no resources. The vMSC properties have already been built.
If it's not possible do we need the defensive _or_null and error checks?
> + */
> +static void mpam_enable_init_class_features(struct mpam_class *class)
> +{
> + struct mpam_vmsc *vmsc;
> + struct mpam_component *comp;
> +
> + comp = list_first_entry_or_null(&class->components,
> + struct mpam_component, class_list);
> + if (WARN_ON(!comp))
> + return;
> +
> + vmsc = list_first_entry_or_null(&comp->vmsc,
> + struct mpam_vmsc, comp_list);
> + if (WARN_ON(!vmsc))
> + return;
> +
> + class->props = vmsc->props;
> +}
> +/*
> + * Merge all the common resource features into class.
> + * vmsc features are bitwise-or'd together, this must be done first.
I'm not sure what 'this' is here - I think it's a missing plural that has
me confused. Perhaps 'these must be done first.'
> + * Next the class features are the bitwise-and of all the vmsc features.
> + * Other features are the min/max as appropriate.
> + *
> + * To avoid walking the whole tree twice, the class->nrdy_usec property is
> + * updated when working with the vmsc as it is a max(), and doesn't need
> + * initialising first.
Perhaps state that this comment is about what happens in each call of
mpam_enable_merge_vmsc_features() Or move the comment to that function.
> + */
> +static void mpam_enable_merge_features(struct list_head *all_classes_list)
> +{
> + struct mpam_class *class;
> + struct mpam_component *comp;
> +
> + lockdep_assert_held(&mpam_list_lock);
> +
> + list_for_each_entry(class, all_classes_list, classes_list) {
> + list_for_each_entry(comp, &class->components, class_list)
> + mpam_enable_merge_vmsc_features(comp);
> +
> + mpam_enable_init_class_features(class);
> +
> + list_for_each_entry(comp, &class->components, class_list)
> + mpam_enable_merge_class_features(comp);
> + }
> +}
