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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e43b10b7-60b0-412a-b55f-96271764faa1@amd.com>
Date: Fri, 13 Dec 2024 09:57:50 -0600
From: "Moger, Babu" <bmoger@....com>
To: "Luck, Tony" <tony.luck@...el.com>, Babu Moger <babu.moger@....com>
Cc: corbet@....net, reinette.chatre@...el.com, tglx@...utronix.de,
 mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
 peternewman@...gle.com, fenghua.yu@...el.com, x86@...nel.org, hpa@...or.com,
 paulmck@...nel.org, akpm@...ux-foundation.org, thuth@...hat.com,
 rostedt@...dmis.org, xiongwei.song@...driver.com,
 pawan.kumar.gupta@...ux.intel.com, daniel.sneddon@...ux.intel.com,
 jpoimboe@...nel.org, perry.yuan@....com, kai.huang@...el.com,
 xiaoyao.li@...el.com, seanjc@...gle.com, xin3.li@...el.com,
 andrew.cooper3@...rix.com, ebiggers@...gle.com, mario.limonciello@....com,
 james.morse@....com, tan.shaopeng@...itsu.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, maciej.wieczor-retman@...el.com,
 eranian@...gle.com
Subject: Re: [PATCH v10 16/24] x86/resctrl: Add interface to the assign
 counter

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