linux-kernel - Re: [PATCH v7 37/49] x86/resctrl: Expand the width of dom_id by replacing mon_data

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] [day] [month] [year] [list]

Message-ID: <Z-H-VesKknnUMmpb@agluck-desk3>
Date: Mon, 24 Mar 2025 17:52:37 -0700
From: "Luck, Tony" <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: James Morse <james.morse@....com>, x86@...nel.org,
	linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	H Peter Anvin <hpa@...or.com>, Babu Moger <Babu.Moger@....com>,
	shameerali.kolothum.thodi@...wei.com,
	D Scott Phillips OS <scott@...amperecomputing.com>,
	carl@...amperecomputing.com, lcherian@...vell.com,
	bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
	baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
	Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
	dfustini@...libre.com, amitsinght@...vell.com,
	David Hildenbrand <david@...hat.com>,
	Rex Nie <rex.nie@...uarmicro.com>,
	Dave Martin <dave.martin@....com>, Koba Ko <kobak@...dia.com>,
	Shanker Donthineni <sdonthineni@...dia.com>, fenghuay@...dia.com
Subject: Re: [PATCH v7 37/49] x86/resctrl: Expand the width of dom_id by
 replacing mon_data_bits

On Thu, Mar 13, 2025 at 08:25:08AM -0700, Reinette Chatre wrote:
> Hi James,
> 
> On 3/12/25 11:04 AM, James Morse wrote:
> > On 07/03/2025 05:03, Reinette Chatre wrote:
> >> On 2/28/25 11:59 AM, James Morse wrote:
> 
> ...
> 
> >> With all of the above I do not think this will work on an SNC enabled
> >> system ... to confirm this I tried it out and it is not possible to mount
> >> resctrl on an SNC enabled system and the WARN_ON_ONCE() this patch adds to
> >> mon_add_all_files() is hit.
> > 
> > I hadn't realised the mon_sub directories for SNC weren't all directly under mon_data.
> > Searching from mon_data will need the parent name too. What I've come up with is:
> > -------%<-------
> > 	snc_mode = r->mon_scope == RESCTRL_L3_NODE;
> > 	if (!snc_mode) {
> > 		sprintf(name, "mon_%s_%02d", r->name, d->hdr.id);
> > 		kn_target_dir = kernfs_find_and_get(kn_mondata, name);
> > 	} else {
> > 		sprintf(name, "mon_%s_%02d", r->name, d->ci->id);
> > 		kn_target_dir = kernfs_find_and_get(kn_mondata, name);
> > 
> > 		if (snc_mode && !do_sum) {
> 
> snc_mode should always be true here?
> 
> > 			sprintf(name, "mon_sub_%s_%02d", r->name, d->hdr.id);
> > 			kernfs_put(kn_target_dir);
> 
> I think this needs some extra guardrails. If kn_target_dir is NULL here
> it looks like that the kernfs_put() above will be fine, but from what I can tell
> the kernfs_find_and_get() below will not be.
> 
> > 			kn_target_dir = kernfs_find_and_get(kn_target_dir, name);
> > 		}
> > 	}
> > 	kernfs_put(kn_target_dir);
> > 	if (!kn_target_dir)
> > 		return NULL;
> > -------%<-------
> > 
> 
> This looks good to me. In original patch a NULL kn within mon_get_default_kn_priv()
> was used as prompt to create the private data. It is thus not obvious to me from this
> snippet what is being returned "to", but I do not think that was your point of sharing
> this snippet. 

Is this all overly complex trying to re-use the "priv" fields from
the default resctrl group?  Would it be easier to just keep a list
of each combinations of region id, domain id, sum, and event id that have
already been allocated and re-use existing ones, or add to the list
for new ones. Scanning this list may be less overhead that all the
sprintf() and kernfs_find_and_get() searches.


Something like:


static LIST_HEAD(kn_priv_list);

static struct mon_data *mon_get_kn_priv(struct rdt_resource *r,
					struct rdt_mon_domain *d,
					struct mon_evt *mevt,
					struct rdtgroup *rdtgrp,
					bool do_sum)
{
	struct mon_data *priv;

	list_for_each_entry(priv, &kn_priv_list, list) {
		if (priv->rid == r->rid && priv->domid == (do_sum ? d->ci->id : d->hdr.id) &&
		    priv->sum == do_sum && priv->evtid == mevt->evtid)
			return priv;
		}
	}

	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
	if (!priv)
		return NULL;

	priv->rid = r->rid;
	priv->domid = do_sum ? d->ci->id : d->hdr.id;
	priv->sum = do_sum;
	priv->evtid = mevt->evtid;
	list_add_tail(&priv->list, &kn_priv_list);

	return priv;
}

Maybe just ignore the "domain goes offline case" there's not much memory
tied up in these structures (and the domain may come back).

Just free the whole list when resctrl is unmounted.

static void mon_put_kn_priv(void)
{
	struct mon_data *priv, *tmp;

	list_for_each_entry_safe(priv, tmp, &kn_priv_list, list)
		kfree(priv);
}

[Lightly tested the "get" path. Haven't tried the "put" code.]



-Tony