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

[Luck, Tony]

On Mon, Mar 24, 2025 at 05:52:37PM -0700, Luck, Tony wrote:
> 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.

James,

I played around with the simplification some more and tested on both
normal and SNC systems. Below is a patch against:
 
git://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git mpam/move_to_fs/v7

Note that this is *after* the move to fs/resctrl as I was too chicken
to try applying this before the move and then re-run the script to
move things. But if you take this suggestion, just mash it into your
"Expand the width of dom_id by replacing mon_data_bits" patch.
Maybe give me Co-developed-by credit, but that's not important.

Note that your expansion of mon_data is going to be very useful going
forward.  I want to add extra information to struct mon_data:

1) Flag to note that an event counter can be read from any CPU, not
just the ones in the domain specified by the mon_data/mon_L3_XX/*
file.

2) Type field to specify how to display the value of each counter
(since I want floating point instead of integer for the energy
counters).

-Tony