linux-kernel - Re: [PATCH v6 21/22] x86/resctrl: Introduce interface to list monitor states of all the groups

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: <2a65c646-ccd4-4e5e-bd1a-f56e69b1532c@arm.com>
Date: Fri, 16 Aug 2024 17:28:37 +0100
From: James Morse <james.morse@....com>
To: Babu Moger <babu.moger@....com>
Cc: x86@...nel.org, hpa@...or.com, paulmck@...nel.org, rdunlap@...radead.org,
 tj@...nel.org, peterz@...radead.org, yanjiewtw@...il.com,
 kim.phillips@....com, lukas.bulwahn@...il.com, seanjc@...gle.com,
 jmattson@...gle.com, leitao@...ian.org, jpoimboe@...nel.org,
 rick.p.edgecombe@...el.com, kirill.shutemov@...ux.intel.com,
 jithu.joseph@...el.com, kai.huang@...el.com, kan.liang@...ux.intel.com,
 daniel.sneddon@...ux.intel.com, pbonzini@...hat.com, sandipan.das@....com,
 ilpo.jarvinen@...ux.intel.com, peternewman@...gle.com,
 maciej.wieczor-retman@...el.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, eranian@...gle.com,
 dave.hansen@...ux.intel.com, tglx@...utronix.de, corbet@....net,
 fenghua.yu@...el.com, reinette.chatre@...el.com, mingo@...hat.com,
 bp@...en8.de
Subject: Re: [PATCH v6 21/22] x86/resctrl: Introduce interface to list monitor
 states of all the groups

Hi Babu,

On 06/08/2024 23:00, Babu Moger wrote:
> Provide the interface to list the monitor states of all the resctrl
> groups in ABMC mode.
> 
> Example:
> $cat /sys/fs/resctrl/info/L3_MON/mbm_control
> 
> List follows the following format:
> 
> "<CTRL_MON group>/<MON group>/<domain_id>=<flags>"
> 
> Format for specific type of groups:
> 
> - Default CTRL_MON group:
>   "//<domain_id>=<flags>"
> 
> - Non-default CTRL_MON group:
>   "<CTRL_MON group>//<domain_id>=<flags>"
> 
> - Child MON group of default CTRL_MON group:
>   "/<MON group>/<domain_id>=<flags>"
> 
> - Child MON group of non-default CTRL_MON group:
>   "<CTRL_MON group>/<MON group>/<domain_id>=<flags>"
> 
> Flags can be one of the following:
> t  MBM total event is enabled
> l  MBM local event is enabled
> tl Both total and local MBM events are enabled
> _  None of the MBM events are enabled


> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index d15fd1bde5f4..d7aadca5e4ab 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -965,6 +965,75 @@ static int rdtgroup_num_mbm_cntrs_show(struct kernfs_open_file *of,
>  	return 0;
>  }
>  
> +static char *rdtgroup_mon_state_to_str(struct rdtgroup *rdtgrp,
> +				       struct rdt_mon_domain *d, char *str)
> +{
> +	char *tmp = str;
> +	int index;
> +
> +	/*
> +	 * Query the monitor state for the domain.
> +	 * Index 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID
> +	 * Index 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
> +	 */
> +	index = mon_event_config_index_get(QOS_L3_MBM_TOTAL_EVENT_ID);
> +	if (rdtgrp->mon.cntr_id[index] != MON_CNTR_UNSET &&
> +	    test_bit(rdtgrp->mon.cntr_id[index], d->mbm_cntr_map))
> +		*tmp++ = 't';
> +
> +	index = mon_event_config_index_get(QOS_L3_MBM_LOCAL_EVENT_ID);
> +	if (rdtgrp->mon.cntr_id[index] != MON_CNTR_UNSET &&
> +	    test_bit(rdtgrp->mon.cntr_id[index], d->mbm_cntr_map))
> +		*tmp++ = 'l';
> +
> +	if (tmp == str)
> +		*tmp++ = '_';
> +
> +	*tmp = '\0';
> +	return str;
> +}
> +
> +static int rdtgroup_mbm_control_show(struct kernfs_open_file *of,
> +				     struct seq_file *s, void *v)
> +{
> +	struct rdt_resource *r = of->kn->parent->priv;

> +	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);

This is filesystem code, once it moves to /fs/ you can't grab an architecture specific
struct like this. (suggestion below).


> +	struct rdt_mon_domain *dom;
> +	struct rdtgroup *rdtg;
> +	char str[10];

Shouldn't new commands that might fail start with rdt_last_cmd_clear()?


> +	if (!hw_res->mbm_cntr_assign_enabled) {

I think this should be wrapped up as:
| resctrl_arch_mbm_cntr_assign_test(r)

as this flag is private to the architecture.


> +		rdt_last_cmd_puts("ABMC feature is not enabled\n");

lockdep barks that you need to hold rdtgroup_mutex when calling rdt_last_cmd_puts() -
otherwise this can run in parallel with another syscall.


> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&rdtgroup_mutex);
> +
> +	list_for_each_entry(rdtg, &rdt_all_groups, rdtgroup_list) {
> +		struct rdtgroup *crg;
> +
> +		seq_printf(s, "%s//", rdtg->kn->name);
> +
> +		list_for_each_entry(dom, &r->mon_domains, hdr.list)
> +			seq_printf(s, "%d=%s;", dom->hdr.id,
> +				   rdtgroup_mon_state_to_str(rdtg, dom, str));
> +		seq_putc(s, '\n');
> +
> +		list_for_each_entry(crg, &rdtg->mon.crdtgrp_list,
> +				    mon.crdtgrp_list) {
> +			seq_printf(s, "%s/%s/", rdtg->kn->name, crg->kn->name);
> +
> +			list_for_each_entry(dom, &r->mon_domains, hdr.list)
> +				seq_printf(s, "%d=%s;", dom->hdr.id,
> +					   rdtgroup_mon_state_to_str(crg, dom, str));
> +			seq_putc(s, '\n');
> +		}
> +	}
> +
> +	mutex_unlock(&rdtgroup_mutex);
> +	return 0;
> +}


Thanks,

James