Re: [PATCH v2 08/29] arm_mpam: Add the class and component structures for firmware described ris

[James Morse <james.morse@arm.com>]

Hi Fenghua,

On 03/10/2025 17:54, Fenghua Yu wrote:
> On 9/10/25 13:42, James Morse 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.

>> 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

>> +#define add_to_garbage(x)                \
>> +do {                            \
>> +    __typeof__(x) _x = (x);                \
>> +    _x->garbage.to_free = _x;            \
>> +    llist_add(&_x->garbage.llist, &mpam_garbage);    \
>> +} while (0)
>> +static void mpam_free_garbage(void)
>> +{
>> +    struct mpam_garbage *iter, *tmp;
>> +    struct llist_node *to_free = llist_del_all(&mpam_garbage);
>> +
> 
> Should this be protected by mpam_list_lock and check if the lock is held?
> 
> +    lockdep_assert_held(&mpam_list_lock);
> 
> Multiple threads may add and free garbage in parallel. Please see later free_garbage() is
> not protected by any lock.

Indeed - because its using llist instead. That is safe for concurrent use as you can only
consume the whole list in one go with a cmpxchg(val, NULL). In the event of a race, one
gets to own the llist and walk throught it - the other sees an empty list.


> 
>> +    if (!to_free)
>> +        return;
>> +
>> +    synchronize_srcu(&mpam_srcu);
>> +
>> +    llist_for_each_entry_safe(iter, tmp, to_free, llist) {
>> +        if (iter->pdev)
>> +            devm_kfree(&iter->pdev->dev, iter->to_free);
>> +        else
>> +            kfree(iter->to_free);
>> +    }
>> +}

>> +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;
>> +
> 
> Should setting msc->ris_idxs bit be moved to the end of this function after all error
> handling paths? The reason is this bit is better to be 0 (or recovered) if any error
> happens. It's hard to recover it to 0 for each error handling. The easiest way is to set
> it at the end of the function.

This is an up front test for firmware tables that describe one RIS twice.
No error recovery is needed as this is all this bitfield is used for.


>> +    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);
>> +        return PTR_ERR(comp);
>> +    }
>> +
>> +    vmsc = mpam_vmsc_get(comp, msc);
>> +    if (IS_ERR(vmsc)) {
>> +        if (list_empty(&comp->vmsc))
>> +            mpam_comp_destroy(comp);
>> +        return PTR_ERR(vmsc);
>> +    }
>> +
>> +    err = mpam_ris_get_affinity(msc, &ris->affinity, type, class, comp);
>> +    if (err) {
>> +        if (list_empty(&vmsc->ris))
>> +            mpam_vmsc_destroy(vmsc);
>> +        return err;
>> +    }
>> +
>> +    ris->ris_idx = ris_idx;
>> +    INIT_LIST_HEAD_RCU(&ris->vmsc_list);
> 
> vmsc_list will be used but not initialized. Missing INIT_LIST_HEAD_RCU(&ris->msc_list) here?

Fixed,


>> +    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);
>> +

> Setting the msc->ris_idxs here is better to avoid to clear it in each error handling path.

But misses the error it is supposed to catch...


>> +    return 0;
>> +}
>> +
>> @@ -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);
>>       mutex_unlock(&mpam_list_lock);
>> +
>> +    mpam_free_garbage();
> 
> Should mpam_free_garbage() be protected by mpam_list_lock? It may race with adding
> garbage. I can see other adding and freeing garbage are protected by mpam_list_lock but
> not this one.

No - it uses llist and is part of the deferred freeing, it should not need any locks.


Thanks,

James