Re: [PATCH v4 5/7] fs/resctrl: Continue counter allocation after failure

[Reinette Chatre]

Hi Ben,

On 3/26/26 10:25 AM, Ben Horgan wrote:
> In mbm_event mode, with mbm_assign_on_mkdir set to 1, when a user creates a
> new CTRL_MON or MON group resctrl will attempt to allocate counters for

"will attempt" -> "attempts"

> each of the supported mbm events on each resctrl domain. As counters are

"mbm" -> "MBM"

> limited, these allocations may fail. If an mbm_total_event counter

This switches from high level description to example, back to high level description.

> allocation fails then the mbm_total_event counter allocations for the
> remaining domains are skipped and then the mbm_local_event counter
> allocations are made. These failures don't cause the group allocation to

"are made -> "are attempted"

> fail but the user should still be aware of them. A message for each
> attempted allocation is reported in last_cmd_status but in order to fully
> interpret that information the user needs to know what was skipped.  This
> is knowable as the domain list is sorted but it is undesirable to rely on
> such implementation details.

User can always get most accurate counter assignment state from
mbm_L3_assignments. There is no need for any guessing or needing to know
implementation details.

> 
> Writes to mbm_L3_assignments using the wildcard format, <event>:*=e, will
> also skip counter allocation after any counter allocation failure.  Leading
> once again to counters that are allocated but have no corresponding message
> in last_cmd_status to indicate that.

User should not rely on last_cmd_status for state and we should not move
towards making it part of ABI.

> 
> When a new group is created always attempt to allocate all the counters
> requested. Similarly, when a a wildcard assign operation is written to

"a a wildcard" 

> mbm_L3_assignments, attempt to allocate all counters requested by that
> particular operation.
> 
> For mbm_L3_assignments, continue to return an error on counter allocation
> failure and for a write specifying multiple assign operations continue to
> abort after the first failing assign operation.

I support the change to attempt counter creation in all domains but I am
concerned about the motivation here - the goal should not be to document
all failed domains in last_cmd_status and pointing user to it to learn which
allocations failed. Instead user should use mbm_L3_assignments for most
accurate state.

Consider a changelog like below that just focuses on problem being solved
(but please correct me if you find I am missing the point):

	In mbm_event mode, with mbm_assign_on_mkdir set to 1, when a user
	creates a new CTRL_MON or MON group resctrl attempts to allocate
	counters for each of the supported MBM events on each resctrl
	domain. As counters are limited, such allocation may fail and
	when it does counter allocations for the remaining domains are
	skipped even if the domains have available counters.                                                                       
                                                                                
	Since a counter allocation failure may result in counter allocation
	skipped on other domains the user needs to view the resource group's
	mbm_L3_assignments files to get an accurate view of counter assignment
	in a new resource group and then manually create counters in the skipped
	domains	with available counters.                                             
                                                                                
	Writes to mbm_L3_assignments using the wildcard format, <event>:*=e,            
	also skip counter allocation in other domains after a counter allocation      
	failure.                                                                        
                                                                                
	When handling a request to create counters in all domains it is unnecessary
	for a counter allocation in one domain to prevent counter allocation in
	other domains. Always attempt to allocate all the counters requested.                                      


> 
> Signed-off-by: Ben Horgan <ben.horgan@xxxxxxx>
> ---
>  fs/resctrl/monitor.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 6afa2af26ff7..3f33fff8eb7f 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -1209,9 +1209,10 @@ static int rdtgroup_alloc_assign_cntr(struct rdt_resource *r, struct rdt_l3_mon_
>   * NULL; otherwise, assign the counter to the specified domain @d.
>   *
>   * If all counters in a domain are already in use, rdtgroup_alloc_assign_cntr()
> - * will fail. The assignment process will abort at the first failure encountered
> - * during domain traversal, which may result in the event being only partially
> - * assigned.
> + * will fail. When attempting to assign counters to all domains, carry on trying
> + * to assign counters after a failure since only some domains may have counters
> + * and the goal is to assign counters where possible. If any counter assignment
> + * fails, return the error from the last failing assignment.
>   *
>   * Return:
>   * 0 on success, < 0 on failure.
> @@ -1224,9 +1225,11 @@ static int rdtgroup_assign_cntr_event(struct rdt_l3_mon_domain *d, struct rdtgro
>  
>  	if (!d) {
>  		list_for_each_entry(d, &r->mon_domains, hdr.list) {
> -			ret = rdtgroup_alloc_assign_cntr(r, d, rdtgrp, mevt);
> -			if (ret)
> -				return ret;
> +			int err;
> +
> +			err = rdtgroup_alloc_assign_cntr(r, d, rdtgrp, mevt);
> +			if (err)
> +				ret = err;
>  		}
>  	} else {
>  		ret = rdtgroup_alloc_assign_cntr(r, d, rdtgrp, mevt);

The change looks good to me.

Reinette