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

[Moger, Babu]

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