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

["Luck, Tony" <tony.luck@intel.com>]

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.

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

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

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