Re: [PATCH v10 16/24] x86/resctrl: Add interface to the assign counter

["Moger, Babu" <bmoger@amd.com>]

Hi Tony,

On 12/12/2024 5:37 PM, Luck, Tony wrote:
> On Thu, Dec 12, 2024 at 02:15:19PM -0600, Babu Moger wrote:
>> +/*
>> + * Assign a hardware counter to event @evtid of group @rdtgrp.
>> + * Counter will be assigned to all the domains if rdt_mon_domain is NULL
>> + * else the counter will be assigned to specific domain.
>> + */
>> +int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>> +			       struct rdt_mon_domain *d, enum resctrl_event_id evtid)
>> +{
>> +	int cntr_id, ret = 0;
>> +
>> +	if (!d) {
>> +		list_for_each_entry(d, &r->mon_domains, hdr.list) {
>> +			if (mbm_cntr_assigned(r, d, rdtgrp, evtid))
>> +				continue;
>> +
>> +			cntr_id = mbm_cntr_alloc(r, d, rdtgrp, evtid);
>> +			if (cntr_id <  0) {
>> +				rdt_last_cmd_puts("Domain Out of MBM assignable counters\n");
> 
> Message could be more helpful by including the domain number.

Yes. We can do that. I will to use rdt_last_cmd_printf().

> 
>> +				continue;
> 
> Not sure whether continuing is the right thing to do here. Sure the
> other domains may have available counters, but now you may have a
> confused status where some requested operations succeeded and others
> failed.
> 
>> +			}
>> +
>> +			ret = resctrl_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
>> +						  rdtgrp->closid, cntr_id, true);
>> +			if (ret)
>> +				goto out_done_assign;
>> +		}
>> +	} else {
>> +		if (mbm_cntr_assigned(r, d, rdtgrp, evtid))
>> +			goto out_done_assign;
>> +
>> +		cntr_id = mbm_cntr_alloc(r, d, rdtgrp, evtid);
>> +		if (cntr_id <  0) {
>> +			rdt_last_cmd_puts("Domain Out of MBM assignable counters\n");
> 
> Ditto helpful to include domain number.

Sure.

> 
>> +			goto out_done_assign;
> 
> When you run out of counters here, you still return 0 from this
> function. This means that updating via write to the "mbm_assign_control"
> file may return success, even though the operation failed.
> 
> E.g. with no counters available:
> 
> # cat available_mbm_cntrs
> 0=0;1=0
> 
> Try to set a monitor domain to record local bandwidth:
> 
> # echo 'c1/m94/0=l;1=_;' > mbm_assign_control
> # echo $?
> 0
> 
> Looks like it worked!
> 
> But it didn't.
> 
> # cat ../last_cmd_status
> Domain Out of MBM assignable counters
> 
> rdtgroup_assign_cntr_event() does say that it didn't if you think to
> check here.

Yes. Agree.

It is right thing to continue assignment if one of the domain is out of 
counters. In that case how about we save the error(say error_domain) and 
continue. And finally return success if both ret and error_domain are zeros.

    return ret ?  ret : error_domain:


> 
>> +		}
>> +
>> +		ret = resctrl_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
>> +					  rdtgrp->closid, cntr_id, true);
>> +	}
>> +
>> +out_done_assign:
>> +	if (ret)
>> +		mbm_cntr_free(r, d, rdtgrp, evtid);
>> +
>> +	return ret;
>> +}
> 
> -Tony
> 


thanks
Babu