Received-SPF: pass (google.com: domain of linux-kernel+bounces-166807-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <cover.1711674410.git.babu.moger@amd.com> <CALPaoCjZ3oLdKymJjASt0aqtd0GGOme7LavvYOtPYTb_rA-mYQ@mail.gmail.com>
 <b35dc4e9-7e8b-42ed-9a51-ae50d521cf4b@amd.com>
In-Reply-To: <b35dc4e9-7e8b-42ed-9a51-ae50d521cf4b@amd.com>
From: Peter Newman <peternewman@google.com>
Date: Thu, 2 May 2024 10:50:15 -0700
Message-ID: <CALPaoChxYoJx8eR48EkSKf-hu2p2myQJLZEhj_Pq6O4R15-=5A@mail.gmail.com>
Subject: Re: [RFC PATCH v3 00/17] x86/resctrl : Support AMD Assignable
 Bandwidth Monitoring Counters (ABMC)
To: babu.moger@amd.com
Cc: corbet@lwn.net, fenghua.yu@intel.com, reinette.chatre@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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Babu,

On Thu, May 2, 2024 at 9:25=E2=80=AFAM Moger, Babu <babu.moger@amd.com> wro=
te:
> On 5/1/24 12:48, Peter Newman wrote:
> > The FS layer is informed by the arch layer (through rdt_resource
> > fields) how many assignable monitors are available and whether a
> > monitor is assigned to an entire group or a single event in a group.
> > Also, the FS layer can assume that monitors are indexed contiguously,
> > allowing it to host the data structures managing FS-level view of
> > monitor usage.
> >
> > I used the following resctrl_arch-interfaces to propagate assignments
> > to the implementation:
> >
> > void resctrl_arch_assign_monitor(struct rdt_domain *d, u32 mon_id, u32
> > closid, u32 rmid, int evtid);
>
> Sure. I can add these in next version.
>
> Few comments..
>
> AMD does not need closid for assignment. I assume ARM requires closid.

Correct, MPAM needs a CLOSID+RMID (PARTID+PMG) to identify a
monitoring group. The CLOSID parameter is ignored on x86.

>
> What is mon_id here?

On ABMC, the value is programmed into L3_QOS_ABMC_CFG.CtrID


>
> > void resctrl_arch_unassign_monitor(struct rdt_domain *d, u32 mon_id);
>
> We need rmid and evtid for unassign interface here.

From my reading of the ABMC specification, it does not look necessary
to program BwSrc or BwType when changing L3_QOS_ABMC_CFG.CtrEn to 0
for a particular CtrID. This interface only disables a counter, so it
should not need to know about how it was previously used when assign
is able to reassign, as assign will always reset the arch_mbm data.

I do not see any harm in the arch_mbm data being stale while the
counter is unassigned, because the data is not accessed when reading
the hardware counter fails. In general, resctrl_arch_rmid_read()
cannot return any information if the hardware counter is not readable
at the time it is called.

>
>
> >
> > I chose to allow reassigning an assigned monitor without calling
> > unassign first. This is important when monitors are unassigned and
> > assigned in a single write to mbm_assign_control, as it allows all
> > updates to be performed in a single round of parallel IPIs to the
> > domains.
>
> Yes. It is not required to call unassign before assign. Hardware(AMD)
> supports it.
>
> But, we only have 32 counters. We need to know which counter we are going
> to use for assignment. If all the counters already assigned, then we can'=
t
> figure out the counter id without calling unassigm first. Using the rando=
m
> counter will overwrite the already assigned counter.

I made the caller of resctrl_arch_assign_monitor() responsible for
selecting which monitor to assign. As long as the user orders the
unassign operations before the assign operations in a write to
mbm_assign_control, the FS code will be able to find an available
monitor ID.


> > I chose to make this a mount option to simplify the management of the
> > monitor tracking data structures. They are simply allocated at mount
> > time and deallocated and unmount.
>
> Initially I added it as an mount option.
> Based on our earlier discussion, we decided to use the assign feature by
> default if hardware supports it. Users don't have to worry about the deta=
ils.
> >
> > I called the option "mon_assign": The mount option parser calls
> > resctrl_arch_mon_assign_enable() to determine whether the
> > implementation supports assignment in some form. If it returns an
> > error, the mount fails. When successful, the assignable monitor count
> > is made non-zero in the appropriate rdt_resource, triggering the
> > behavior change in the FS layer.
> >
> > I'm still not sure if it's a good idea to enable monitor assignment by
> > default. This would be a major disruption in the MBM usage model
> > triggered by moving software between AMD CPU models. I thought the
>
> Why will it be a disruption? Why do you think mount option will solve the
> problem? As always, there will be option to go back to legacy mode. right=
?
>
> > safest option was to disallow creating more monitoring groups than
> > monitors unless the option is selected. Given that nobody else
>
> Current code allows to create more groups, but it will report "Monitor
> assignment failed" when it runs out of monitors.

Ok that should be fine then.

However, I don't think it's necessary to support dynamically changing
the usage model of monitoring groups without remounting. I believe it
makes it more difficult for the FS code to generically manage monitor
assignment.

-Peter