Re: [RFC PATCH v2 04/10] arm_mpam: resctrl: Split control and monitoring domain setup into helpers

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Shaopeng,

Thank you very much for taking a look.

On 11/1/26 9:41 PM, Shaopeng Tan (Fujitsu) wrote:

...

>> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
>> index dc442851fd7e..246560247a28 100644
>> --- a/drivers/resctrl/mpam_resctrl.c
>> +++ b/drivers/resctrl/mpam_resctrl.c
>> @@ -1615,13 +1615,77 @@ static struct mpam_component *find_component(struct mpam_class *class, int cpu)
>>          return NULL;
>>  }
>>  
>> +static int mpam_resctrl_setup_ctrl_domain(unsigned int cpu,
>> +                                         struct rdt_resource *r,
>> +                                         struct mpam_component *comp,
>> +                                         struct mpam_resctrl_dom *dom)
>> +{
>> +       struct rdt_ctrl_domain *ctrl_d = &dom->resctrl_ctrl_dom;
>> +       int err;
>> +
>> +       dom->ctrl_comp = comp;
>> +
>> +       mpam_resctrl_domain_hdr_init(cpu, comp, r->rid, &ctrl_d->hdr);
>> +       ctrl_d->hdr.type = RESCTRL_CTRL_DOMAIN;
>> +       err = resctrl_online_ctrl_domain(r, ctrl_d);
>> +       if (err)
>> +               return err;
>> +
>> +       mpam_resctrl_domain_insert(&r->ctrl_domains, &ctrl_d->hdr);
>> +
>> +       return 0;
>> +}
>> +
>> +static int mpam_resctrl_setup_mon_domain(unsigned int cpu,
>> +                                        struct rdt_resource *r,
>> +                                        struct mpam_resctrl_dom *dom)
>> +{
>> +       struct rdt_l3_mon_domain *mon_d = &dom->resctrl_mon_dom;
>> +       struct mpam_component *any_mon_comp = NULL;
>> +       enum resctrl_event_id eventid;
>> +       struct mpam_resctrl_mon *mon;
>> +       int err;
>> +
>> +       /*
>> +        * Even if the monitor domain is backed by a different
>> +        * component, the L3 component IDs need to be used... only
>> +        * there may be no ctrl_comp for the L3.
>> +        * Search each event's class list for a component with
>> +        * overlapping CPUs and set up the dom->mon_comp array.
>> +        */
>> +
>> +       for_each_mpam_resctrl_mon(mon, eventid) {
>> +               struct mpam_component *mon_comp;
>> +
>> +               if (!mon->class)
>> +                       continue;       // dummy resource
>> +
>> +               mon_comp = find_component(mon->class, cpu);
>> +               dom->mon_comp[eventid] = mon_comp;
>> +               if (mon_comp)
>> +                       any_mon_comp = mon_comp;
>> +       }
>> +       if (!any_mon_comp) {
>> +               WARN_ON_ONCE(0);
>> +               return -EFAULT;
>> +       }
>> +
>> +       mpam_resctrl_domain_hdr_init(cpu, any_mon_comp, r->rid, &mon_d->hdr);
>> +       mon_d->hdr.type = RESCTRL_MON_DOMAIN;
>> +       err = resctrl_online_mon_domain(r, &mon_d->hdr);
>> +       if (err)
>> +               return err;
>> +
>> +       mpam_resctrl_domain_insert(&r->mon_domains, &mon_d->hdr);
>> +
>> +       return 0;
>> +}
>> +
> 
> Currently, it returns success even when the domain fails to be inserted. Is there a specific reason for this design?
> I believe it would be safer if mpam_resctrl_domain_insert() returned an int to indicate success or failure. 
> 
> For example:
> 	static int mpam_resctrl_domain_insert(struct list_head *list,
> 					       struct rdt_domain_hdr *new)

Could mpam_resctrl_domain_insert() ever fail? A domain is only created
after the lookup found none for this CPU. For a control domain the lookup is by
the component's CPU affinity while the insert checks the domain id, but the id
is the component's id and component affinities within a class do not overlap, so
absence of one implies absence of the other. For a monitoring domain both are
the L3 cache id.

For mpam_resctrl_domain_insert() to return a failure an insert would have found
an existing domain which means a domain appeared _after_ it was found to be absent.
That would be a driver bug rather than a runtime error. The WARN_ON_ONCE() reports
it. Handling it would add unwind code for a state that cannot occur.

This is how I understand the flow and believe this is why the existing code
does not report it. 

Reinette