Message-ID: <2214f589-03d5-4037-8997-bbf78077a101@intel.com>
Date: Thu, 5 Dec 2024 20:14:46 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: Peter Newman <peternewman@...gle.com>
CC: Fenghua Yu <fenghua.yu@...el.com>, James Morse <james.morse@....com>,
	Stephane Eranian <eranian@...gle.com>, Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
	<dave.hansen@...ux.intel.com>, <x86@...nel.org>, "H. Peter Anvin"
	<hpa@...or.com>, Peter Zijlstra <peterz@...radead.org>, Juri Lelli
	<juri.lelli@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>, Steven Rostedt
	<rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, Mel Gorman
	<mgorman@...e.de>, Valentin Schneider <vschneid@...hat.com>, Uros Bizjak
	<ubizjak@...il.com>, Mike Rapoport <rppt@...nel.org>, "Kirill A. Shutemov"
	<kirill.shutemov@...ux.intel.com>, Rick Edgecombe
	<rick.p.edgecombe@...el.com>, Xin Li <xin3.li@...el.com>, Babu Moger
	<babu.moger@....com>, Shaopeng Tan <tan.shaopeng@...itsu.com>, "Maciej
 Wieczor-Retman" <maciej.wieczor-retman@...el.com>, Jens Axboe
	<axboe@...nel.dk>, Christian Brauner <brauner@...nel.org>, Oleg Nesterov
	<oleg@...hat.com>, Andrew Morton <akpm@...ux-foundation.org>, Tycho Andersen
	<tandersen@...flix.com>, Nicholas Piggin <npiggin@...il.com>, Beau Belgrave
	<beaub@...ux.microsoft.com>, "Matthew Wilcox (Oracle)" <willy@...radead.org>,
	<linux-kernel@...r.kernel.org>, Tony Luck <tony.luck@...el.com>
Subject: Re: [PATCH v1 3/6] x86/resctrl: Disallow mongroup rename on MPAM

+Tony

Hi Peter,

On 12/5/24 7:03 AM, Peter Newman wrote:
> On Sat, Apr 6, 2024 at 12:10 AM Peter Newman <peternewman@...gle.com> wrote:
>> On Thu, Apr 4, 2024 at 4:11 PM Reinette Chatre
>> <reinette.chatre@...el.com> wrote:
>>>
>>> Hi Peter,
>>>
>>> On 3/25/2024 10:27 AM, Peter Newman wrote:
>>>> Moving a monitoring group to a different parent control assumes that the
>>>> monitors will not be impacted. This is not the case on MPAM where the
>>>> PMG is an extension of the PARTID.
>>>>
>>>> Detect this situation by requiring the change in CLOSID not to affect
>>>> the result of resctrl_arch_rmid_idx_encode(), otherwise return
>>>> -EOPNOTSUPP.
>>>>
>>>
>>> Thanks for catching this. This seems out of place in this series. It sounds
>>> more like an independent fix that should go in separately.
>>
>> I asserted in a comment in a patch later in the series that the
>> mongroup parent pointer never changes on MPAM, then decided to follow
>> up on whether it was actually true, so it's only here because this
>> series depends on it. I'll post it again separately with the fix you
>> requested below.
> 
> I'm preparing to finally re-post this patch, but another related
> question came up...
> 
> It turns out the check added to mongroup rename in this patch is an
> important property that the user needs in order to correctly interpret
> the value returned by info/L3_MON/num_rmids.
> 
> I had told some Google users to attempt to move a monitoring group to
> a new parent to determine whether the monitoring groups are
> independent of their parent ctrl_mon groups. This approach proved
> unwieldy when used on a system where the maximum number of allowed
> groups has already been created. (In their problem case, a
> newly-created process wanted to determine this property independently
> of the older process which had originally mounted resctrl.)

Could you please elaborate why users need the information that monitoring
groups are independent of their parent ctrl_mon group?
I understood from [2] that Arm can be expected to support moving monitor
groups so I am concerned about userspace building some assumptions like
"if the monitoring groups are dependent on their parent ctrl_mon groups then
monitor groups cannot be moved".

> 
> Also, this information does not require an active rdtgroup pointer or
> CLOSID/RMID. The following will also work:
> 
>  resctrl_arch_rmid_idx_encode(0, 0) != resctrl_arch_rmid_idx_encode(1, 0);
> 
> I propose adding a new info file returning the result of this
> expression to be placed beside num_rmids. I would name it
> "mon_id_includes_control_id", as it implies that the hardware
> logically interprets the monitoring ID as concatenated with its parent
> control ID. This tells the user whether num_rmids defines the number
> of monitoring groups (and ctrl_mon groups) which can be created system
> wide or whether it's one more than the number of monitoring groups
> which can be created below every ctrl_mon group.

Is the goal purely to guide interpretation of "info/L3_MON/num_rmids"?

The "mon_id_includes_control_id" seems unique to a use case and if I understand
correctly needs additional interpretation from user space to reach the actual
data of interest, which I understand to be the number of monitor groups that
can be created.

A while ago James proposed [1] adding a new "num_groups" inside each "mon_groups"
directory, on x86 it will be the same as num_rmids, on Arm it will be the maximum PMG bits.
At a high level(*) I think something like this may be an intuitive way to address this
issue. What do you think?

(*) If considering this I do think it may be more intuitive to have the file
be at the top level of the control group and be named "num_mon_groups" (or something better)
to support the idea that it includes the CTRL_MON group self and not that all monitor groups
are within the mon_groups directory.

Also please keep in mind that during the discussions about moving monitoring groups
there were some ideas of needing to provide additional information to user space about
whether counters are reset on a monitor group move. I think that if we are starting
to look at these random properties as files in resctrl (like "mon_id_includes_control_id"
and maybe "counter_reset_on_monitor_group_move") then we should consider some alternatives
to present these flags to prevent resctrl's info directory from turning into a mess.

> Without this information, I find it difficult to create test cases
> with a maximal number of groups in a portable fashion.
> 
> Also, this would be a good opportunity to generalize "num_rmids" to
> "num_mon_ids" and add a symlink between the two for backwards
> compatibility.

I am hesitant to add "num_mon_ids" to the top level since it creates impression of
being a generic file but is difficult for me come up with a value that can be
interpreted accurately by user space (without knowing the underlying architecture, which
we want to avoid). I think we can perhaps focus on alternatives, like new per monitor
group files, that provide accurate information to user space and through that
let "num_rmids" become obsolete. 

Reinette

[1] https://lore.kernel.org/all/cbe665c2-fe83-e446-1696-7115c0f9fd76@arm.com/
[2] https://lore.kernel.org/all/f80299a4-7eaf-46a0-89e6-b9f5385f183c@arm.com/

Powered by blists - more mailing lists