Received-SPF: pass (google.com: domain of linux-kernel+bounces-69282-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231201005720.235639-1-babu.moger@amd.com> <cover.1705688538.git.babu.moger@amd.com>
 <7c26af23-fa1e-4e01-8088-8fbd9be3d6f3@intel.com> <431d6ac4-53cb-2f73-3cda-22616df2f96a@amd.com>
 <4bb63a78-0d0d-47bc-ad65-558af8bc5519@intel.com> <51c60991-eb10-40e8-b3ab-676b92b0c662@amd.com>
In-Reply-To: <51c60991-eb10-40e8-b3ab-676b92b0c662@amd.com>
From: Peter Newman <peternewman@google.com>
Date: Fri, 16 Feb 2024 12:18:30 -0800
Message-ID: <CALPaoChhKJiMAueFtgCTc7ffO++S5DJCySmxqf9ZDmhR9RQapw@mail.gmail.com>
Subject: Re: [PATCH v2 00/17] x86/resctrl : Support AMD Assignable Bandwidth
 Monitoring Counters (ABMC)
To: babu.moger@amd.com
Cc: Reinette Chatre <reinette.chatre@intel.com>, corbet@lwn.net, fenghua.yu@intel.com, 
	tglx@linutronix.de, mingo@redhat.com, bp@alien8.de, 
	dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com, 
	paulmck@kernel.org, rdunlap@infradead.org, tj@kernel.org, 
	peterz@infradead.org, yanjiewtw@gmail.com, kim.phillips@amd.com, 
	lukas.bulwahn@gmail.com, seanjc@google.com, jmattson@google.com, 
	leitao@debian.org, jpoimboe@kernel.org, rick.p.edgecombe@intel.com, 
	kirill.shutemov@linux.intel.com, jithu.joseph@intel.com, kai.huang@intel.com, 
	kan.liang@linux.intel.com, daniel.sneddon@linux.intel.com, 
	pbonzini@redhat.com, sandipan.das@amd.com, ilpo.jarvinen@linux.intel.com, 
	maciej.wieczor-retman@intel.com, linux-doc@vger.kernel.org, 
	linux-kernel@vger.kernel.org, eranian@google.com, 
	"james.morse@arm.com" <james.morse@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Babu,

On Thu, Feb 8, 2024 at 9:29=E2=80=AFAM Moger, Babu <babu.moger@amd.com> wro=
te:
> On 2/5/24 16:38, Reinette Chatre wrote:
> > This could be improved beyond a binary "enable"/"disable" interface to =
user space.
> > For example, the hardware can discover which "mbm counter assign" relat=
ed feature
> > (I'm counting the "soft RMID" here as one of the "mbm counter assign" r=
elated
> > features) is supported on the platform and it can be presented to the u=
ser like:
> >
> > # cat /sys/fs/resctrl/info/L3_MON/mbm_assign
> > [feature_1] feature_2 feature_3
>
> How about this?
> # cat /sys/fs/resctrl/info/L3_MON/mbm_assign
>  ABMC:Capable
>  SOFT-RMID:Capable
>
> To enable ABMC
> # echo ABMC:enable > /sys/fs/resctrl/info/L3_MON/mbm_assign
>
> When ABMC is enabled:
> # cat /sys/fs/resctrl/info/L3_MON/mbm_assign
>  ABMC:Enable
>  SOFT-RMID:Capable

There would be no need to use soft RMIDs on a system that supports
ABMC, so I can't think of a reason why the underlying implementation
would matter to our users. The user should only have to request the
interface where monitors must be assigned manually. The mount would
succeed if the system has a way to support the interface.


> > You have made it clear on several occasions that you do not intend to s=
upport
> > domain level assignment. That may be ok but the interface you create sh=
ould
> > not prevent future support of domain level assignment.
> >
> > If my point is not clear, could you please share how this interface is =
able to
> > support domain level assignment in the future?
> >
> > I am starting to think that we need a file similar to the schemata file
> > for group and domain level monitor configurations.
>
> Something like this?
>
> By default
> #cat /sys/fs/resctrl/monitor_state
> default:0=3Dtotal=3Dassign,local=3Dassign;1=3Dtotal=3Dassign,local=3Dassi=
gn
>
> With ABMC,
> #cat /sys/fs/resctrl/monitor_state
> ABMC:0=3Dtotal=3Dunassign,local=3Dunassign;1=3Dtotal=3Dunassign,local=3Du=
nassign

The benefit from all the string parsing in this interface is only
halving the number of monitor_state sysfs writes we'd need compared to
creating a separate file for mbm_local and mbm_total. Given that our
use case is to assign the 32 assignable counters to read the bandwidth
of ~256 monitoring groups, this isn't a substantial gain to help us. I
think you should just focus on providing the necessary control
granularity without trying to consolidate writes in this interface. I
will propose an additional interface below to optimize our use case.

Whether mbm_total and mbm_local are combined in the group directories
or not, I don't see why you wouldn't just repeat the same file
interface in the domain directories for a user needing finer-grained
controls.


> >> Peter, James,
> >>
> >> Please comment on what you want achieve in "assignment" based on the f=
eatures you are working on.

I prototyped and tested the following additional interface for the
large-scale, batch use case that we're primarily concerned about:

info/L3_MON/mbm_{local,total}_bytes_assigned

Writing a whitespace-delimited list of mongroup directory paths does
the following:
1. unassign all monitors for the given counter
2. assigns a monitor to each mongroup referenced in the write
3. batches per-domain register updates resulting from the assignments
into a single IPI for each domain

This interface allows us to do less sysfs writes and IPIs on systems
with more assignable monitoring resources, rather than doing more.

The reference to a mongroup when reading/writing the above node is the
resctrl-root-relative path to the monitoring group. There is probably
a more concise way to refer to the groups, but my prototype used
kernfs_walk_and_get() to locate each rdtgroup struct.

I would also like to add that in the software-ABMC prototype I made,
because it's based on assignment of a small number of RMIDs,
assignment results in all counters being assigned at once. On
implementations where per-counter assignments aren't possible,
assignment through such a resource would be allowed to assign more
resources than explicitly requested.

This would allow an implementation only capable of global assignment
to assign resources to all groups when a non-empty string is written
to the proposed file nodes, and all resources to be unassigned when an
empty string is written. Reading back from the file nodes would tell
the user how much was actually assigned.

Thanks!
-Peter