Re: [PATCH v7 37/49] x86/resctrl: Expand the width of dom_id by replacing mon_data_bits

["Luck, Tony" <tony.luck@intel.com>]

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