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: <47c64fa8-30e2-47b9-a479-90ea7f8cbbb9@amd.com>
Date: Fri, 20 Dec 2024 14:34:01 -0600
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
 Babu Moger <babu.moger@....com>, corbet@....net, tglx@...utronix.de,
 mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
 tony.luck@...el.com, peternewman@...gle.com
Cc: 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,
 sandipan.das@....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 Reinette,


On 12/19/2024 5:22 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 12/12/24 12:15 PM, Babu Moger wrote:
>> The mbm_cntr_assign mode offers several counters that can be assigned
>> to an RMID, event pair and monitor the bandwidth as long as it is
>> assigned.
>>
>> Counters are managed at the domain level. Introduce the interface to
>> allocate/free/assign the counters.
> 
> Changelog of previous patch also claimed to "Provide the interface to assign the
> counter ids to RMID." Please let changelogs describe the change more accurately.
> 
> (This still does not provide a user interface so what is meant by interface is
> unclear)

How about this?

The mbm_cntr_assign mode offers several counters that can be assigned
to an RMID, event pair and monitor the bandwidth as long as it is
assigned.

Counters are managed at the domain level. Introduce the functionality to 
allocate/free/assign the counters.

If the user requests assignments across all domains, assignments will 
abort on first failure. The error will be logged in 
/sys/fs/resctrl/info/last_cmd_status.


> 
> 
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 849bcfe4ea5b..70d2577fc377 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -704,5 +704,8 @@ unsigned int mon_event_config_index_get(u32 evtid);
>>   int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>   			     enum resctrl_event_id evtid, u32 rmid, u32 closid,
>>   			     u32 cntr_id, bool assign);
>> -
>> +int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>> +			       struct rdt_mon_domain *d, enum resctrl_event_id evtid);
> 
> Could you please be consistent in the ordering of parameters?
> 
> int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d,
> 			       struct rdtgroup *rdtgrp, enum resctrl_event_id evtid);

Sure.

> 
>> +struct mbm_state *get_mbm_state(struct rdt_mon_domain *d, u32 closid,
>> +				u32 rmid, enum resctrl_event_id evtid);
>>   #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index f857af361af1..8823cd97ff1f 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -575,8 +575,8 @@ void free_rmid(u32 closid, u32 rmid)
>>   		list_add_tail(&entry->list, &rmid_free_lru);
>>   }
>>   
>> -static struct mbm_state *get_mbm_state(struct rdt_mon_domain *d, u32 closid,
>> -				       u32 rmid, enum resctrl_event_id evtid)
>> +struct mbm_state *get_mbm_state(struct rdt_mon_domain *d, u32 closid,
>> +				u32 rmid, enum resctrl_event_id evtid)
>>   {
>>   	u32 idx = resctrl_arch_rmid_idx_encode(closid, rmid);
>>   
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index e895d2415f22..1c8694a68cf4 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1927,6 +1927,116 @@ int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>>   	return 0;
>>   }
>>   
>> +/*
>> + * Configure the counter for the event, RMID pair for the domain.
> 
> This description can be more helpful ... it essentially just re-writes function
> header.

I can drop this. There is not much to explain here. Code seems easy to 
follow.

> 
>> + */
>> +static int resctrl_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>> +			       enum resctrl_event_id evtid, u32 rmid, u32 closid,
>> +			       u32 cntr_id, bool assign)
>> +{
>> +	struct mbm_state *m;
>> +	int ret;
>> +
>> +	ret = resctrl_arch_config_cntr(r, d, evtid, rmid, closid, cntr_id, assign);
>> +	if (ret)
>> +		return ret;
>> +
>> +	m = get_mbm_state(d, closid, rmid, evtid);
>> +	if (m)
>> +		memset(m, 0, sizeof(struct mbm_state));
>> +
>> +	return ret;
>> +}
>> +
>> +static bool mbm_cntr_assigned(struct rdt_resource *r, struct rdt_mon_domain *d,
>> +			      struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
>> +{
>> +	int cntr_id;
>> +
>> +	for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
>> +		if (d->cntr_cfg[cntr_id].rdtgrp == rdtgrp &&
>> +		    d->cntr_cfg[cntr_id].evtid == evtid)
>> +			return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +static int mbm_cntr_alloc(struct rdt_resource *r, struct rdt_mon_domain *d,
>> +			  struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
>> +{
>> +	int cntr_id;
>> +
>> +	for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
>> +		if (!d->cntr_cfg[cntr_id].rdtgrp) {
>> +			d->cntr_cfg[cntr_id].rdtgrp = rdtgrp;
>> +			d->cntr_cfg[cntr_id].evtid = evtid;
>> +			return cntr_id;
>> +		}
>> +	}
>> +
>> +	return -EINVAL;
> 
> This can be -ENOSPC

Sure.
> 
>> +}
>> +
>> +static void mbm_cntr_free(struct rdt_resource *r, struct rdt_mon_domain *d,
>> +			  struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
>> +{
>> +	int cntr_id;
>> +
>> +	for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
>> +		if (d->cntr_cfg[cntr_id].rdtgrp == rdtgrp &&
>> +		    d->cntr_cfg[cntr_id].evtid == evtid)
>> +			memset(&d->cntr_cfg[cntr_id], 0, sizeof(struct mbm_cntr_cfg));
>> +	}
>> +}
> 
>>>From what I can tell the counter ID is always available when the counter is freed so
> it can just be freed directly without looping over array?

Yes. With immediate return on every individual failure, we can do this 
easily.

> 
>> +
>> +/*
>> + * Assign a hardware counter to event @evtid of group @rdtgrp.
>> + * Counter will be assigned to all the domains if rdt_mon_domain is NULL
> 
> (to be consistent) "if rdt_mon_domain is NULL" -> "if @d is NULL"

Sure.

> 
>> + * else the counter will be assigned to specific domain.
> 
> "will be assigned to specific domain" -> "will be assigned to @d"

Sure.
> 
> Reinette
> 
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ