On Wed, 10 Sep 2025 20:42:48 +0000
James Morse <james.morse@xxxxxxx> wrote:
> An MSC is a container of resources, each identified by their RIS index.
> Some RIS are described by firmware to provide their position in the system.
> Others are discovered when the driver probes the hardware.
>
> To configure a resource it needs to be found by its class, e.g. 'L2'.
> There are two kinds of grouping, a class is a set of components, which
> are visible to user-space as there are likely to be multiple instances
> of the L2 cache. (e.g. one per cluster or package)
>
> Add support for creating and destroying structures to allow a hierarchy
> of resources to be created.
>
> CC: Ben Horgan <ben.horgan@xxxxxxx>
> Signed-off-by: James Morse <james.morse@xxxxxxx>
Various minor things inline.
Biggest is I think maybe just moving to explicit reference counts
rather than using the empty list for that might end up easier to
read. Mostly because everyone knows what a kref_put() is about.
Obviously a bit pointless in practice, but I prefer not to think too
hard.
> ---
> Changes since v1:
> * Fixed a comp/vmsc typo.
> * Removed duplicate description from the commit message.
> * Moved parenthesis in the add_to_garbage() macro.
> * Check for out of range ris_idx when creating ris.
> * Removed GFP as probe_lock is no longer a spin lock.
> * Removed alloc flag as ended up searching the lists itself.
> * Added a comment about affinity masks not overlapping.
>
> Changes since RFC:
> * removed a pr_err() debug message that crept in.
> ---
> drivers/resctrl/mpam_devices.c | 406 +++++++++++++++++++++++++++++++-
> drivers/resctrl/mpam_internal.h | 90 +++++++
> include/linux/arm_mpam.h | 8 +-
> 3 files changed, 493 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
> index efc4738e3b4d..c7f4981b3545 100644
> --- a/drivers/resctrl/mpam_devices.c
> +++ b/drivers/resctrl/mpam_devices.c
> @@ -18,7 +18,6 @@
> #include <linux/printk.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> -#include <linux/srcu.h>
Why does this include no longer make sense?
> #include <linux/types.h>
>
> #include "mpam_internal.h"
> @@ -31,7 +30,7 @@
> static DEFINE_MUTEX(mpam_list_lock);
> static LIST_HEAD(mpam_all_msc);
>
> -static struct srcu_struct mpam_srcu;
> +struct srcu_struct mpam_srcu;
...
> +/* List of all objects that can be free()d after synchronise_srcu() */
> +static LLIST_HEAD(mpam_garbage);
> +
> +#define init_garbage(x) init_llist_node(&(x)->garbage.llist)
Whilst this obviously works, I'd rather pass garbage to init_garbage
instead of the containing structure (where type varies)
> +
> +static struct mpam_vmsc *
> +mpam_vmsc_alloc(struct mpam_component *comp, struct mpam_msc *msc)
> +{
> + struct mpam_vmsc *vmsc;
> +
> + lockdep_assert_held(&mpam_list_lock);
> +
> + vmsc = kzalloc(sizeof(*vmsc), GFP_KERNEL);
> + if (!vmsc)
> + return ERR_PTR(-ENOMEM);
> + init_garbage(vmsc);
> +
> + INIT_LIST_HEAD_RCU(&vmsc->ris);
> + INIT_LIST_HEAD_RCU(&vmsc->comp_list);
> + vmsc->comp = comp;
> + vmsc->msc = msc;
> +
> + list_add_rcu(&vmsc->comp_list, &comp->vmsc);
> +
> + return vmsc;
> +}
> +static struct mpam_component *
> +mpam_component_get(struct mpam_class *class, int id)
> +{
> + struct mpam_component *comp;
> +
> + lockdep_assert_held(&mpam_list_lock);
> +
> + list_for_each_entry(comp, &class->components, class_list) {
> + if (comp->comp_id == id)
> + return comp;
> + }
> +
> + return mpam_component_alloc(class, id);
> +}
> +static struct mpam_class *
> +mpam_class_get(u8 level_idx, enum mpam_class_types type)
> +{
> + bool found = false;
> + struct mpam_class *class;
> +
> + lockdep_assert_held(&mpam_list_lock);
> +
> + list_for_each_entry(class, &mpam_classes, classes_list) {
> + if (class->type == type && class->level == level_idx) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (found)
> + return class;
Maybe this gets more complex later, but if it doesn't, just return class where you set
found above. Matches with pattern used in mpam_component_get() above.
> +
> + return mpam_class_alloc(level_idx, type);
> +}
> +static void mpam_ris_destroy(struct mpam_msc_ris *ris)
> +{
> + struct mpam_vmsc *vmsc = ris->vmsc;
> + struct mpam_msc *msc = vmsc->msc;
> + struct platform_device *pdev = msc->pdev;
> + struct mpam_component *comp = vmsc->comp;
> + struct mpam_class *class = comp->class;
> +
> + lockdep_assert_held(&mpam_list_lock);
> +
> + /*
> + * It is assumed affinities don't overlap. If they do the class becomes
> + * unusable immediately.
> + */
> + cpumask_andnot(&comp->affinity, &comp->affinity, &ris->affinity);
> + cpumask_andnot(&class->affinity, &class->affinity, &ris->affinity);
> + clear_bit(ris->ris_idx, &msc->ris_idxs);
> + list_del_rcu(&ris->vmsc_list);
> + list_del_rcu(&ris->msc_list);
> + add_to_garbage(ris);
> + ris->garbage.pdev = pdev;
> +
> + if (list_empty(&vmsc->ris))
See below. I think it is worth using an explicit reference count even
though that will only reach zero when the list is empty.
> + mpam_vmsc_destroy(vmsc);
> +}
> +static int mpam_ris_create_locked(struct mpam_msc *msc, u8 ris_idx,
> + enum mpam_class_types type, u8 class_id,
> + int component_id)
> +{
> + int err;
> + struct mpam_vmsc *vmsc;
> + struct mpam_msc_ris *ris;
> + struct mpam_class *class;
> + struct mpam_component *comp;
> +
> + lockdep_assert_held(&mpam_list_lock);
> +
> + if (ris_idx > MPAM_MSC_MAX_NUM_RIS)
> + return -EINVAL;
> +
> + if (test_and_set_bit(ris_idx, &msc->ris_idxs))
> + return -EBUSY;
> +
> + ris = devm_kzalloc(&msc->pdev->dev, sizeof(*ris), GFP_KERNEL);
> + if (!ris)
> + return -ENOMEM;
> + init_garbage(ris);
> +
> + class = mpam_class_get(class_id, type);
> + if (IS_ERR(class))
> + return PTR_ERR(class);
> +
> + comp = mpam_component_get(class, component_id);
> + if (IS_ERR(comp)) {
> + if (list_empty(&class->components))
> + mpam_class_destroy(class);
Maybe just reference count the classes with a kref and do a put here?
> + return PTR_ERR(comp);
> + }
> +
> + vmsc = mpam_vmsc_get(comp, msc);
> + if (IS_ERR(vmsc)) {
> + if (list_empty(&comp->vmsc))
> + mpam_comp_destroy(comp);
Similar to classes I wonder if simple reference counting is cleaner.
> + return PTR_ERR(vmsc);
> + }
> +
> + err = mpam_ris_get_affinity(msc, &ris->affinity, type, class, comp);
> + if (err) {
> + if (list_empty(&vmsc->ris))
and here as well.
> + mpam_vmsc_destroy(vmsc);
> + return err;
> + }
> +
> + ris->ris_idx = ris_idx;
> + INIT_LIST_HEAD_RCU(&ris->vmsc_list);
> + ris->vmsc = vmsc;
> +
> + cpumask_or(&comp->affinity, &comp->affinity, &ris->affinity);
> + cpumask_or(&class->affinity, &class->affinity, &ris->affinity);
> + list_add_rcu(&ris->vmsc_list, &vmsc->ris);
> +
> + return 0;
> +}
> /*
> * An MSC can control traffic from a set of CPUs, but may only be accessible
> * from a (hopefully wider) set of CPUs. The common reason for this is power
> @@ -74,10 +469,10 @@ static void mpam_msc_drv_remove(struct platform_device *pdev)
> return;
>
> mutex_lock(&mpam_list_lock);
> - platform_set_drvdata(pdev, NULL);
> - list_del_rcu(&msc->all_msc_list);
> - synchronize_srcu(&mpam_srcu);
> + mpam_msc_destroy(msc);
I'd suggest introducing mpam_msc_destroy() in the earlier patch. Doesn't make a huge
difference but 2 out of 3 things removed here would be untouched if you do that.
> mutex_unlock(&mpam_list_lock);
> +
> + mpam_free_garbage();
> }
