Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 0/5] arm_mpam: Introduce the Narrow-PARTID feature for MPAM driver

[Dave Martin <Dave.Martin@arm.com>]

On Fri, Dec 20, 2024 at 06:59:31PM +0800, Zeng Heng wrote:
> 
> 
> On 2024/12/13 0:17, Dave Martin wrote:
> > Hi,
> > 
> > On Sat, Dec 07, 2024 at 05:21:31PM +0800, Zeng Heng wrote:

[...]

> > > For a mixture of MSCs system, for MSCs that do not support narrow-partid,
> > > we use the PARTIDs exceeding the number of closids as reqPARTIDs for
> > > expanding the monitoring groups.
> > 
> > Mixed systems still seem not to be handled completely by this series?
> > 
> > You do cope with the scenario where some MSCs support Narrowing and
> > some do not, but there is still the problem of incompatible controls.
> > 
> > If a non-Narrowing MSC has controls that are not of the "partition
> > bitmap" type, then splitting a resctrl control group across multiple
> > PARTIDs is going to change the hardware regulation behaviour.  There
> > does not seem to be any way to work around this be programming
> > different control values (for example).  So, there may be over- or
> > under-allocation of resources compared with what the user requests in
> > resctrlfs.
> > 
> > So, I think there is still a need to check which controls are present,
> > and either disable the use of non-identity intPARTID<->reqPARTID
> > mappings if incompatible controls are present (or don't expose those
> > controls to resctrl).
> > 
> > (If you were not trying to address this issue yet then that is not a
> > problem for an RFC, but it is best to be clear about the
> > limitations...)
> > 
> 
> In the v3, the limitation you mentioned has not yet been handled.
> V3 has not yet determined how to define this limitation well. Additionally,
> for v3, I was still uncertain whether a large-scale
> refactoring was still necessary at that time.
> 
> In fact, I take this limitation very seriously, and I will try to define
> the limitation in next version, explaining it separately as a distinct
> patch.

Fair enough.  I just wanted to make sure that this issue is not missed
(since this is where a lot of the complexity comes from!)


[...]

> > This approach looks reasonable overall, and in this version the changes
> > do seem to be better localised in the mpam_resctrl.c glue code now.
> > 
> > I had also been working on a similar approach, so I have posted it for
> > comparison [1] -- though the two approaches are doing pretty much the
> > same thing, some details differ.
> > 
> > (Note, I have not addressed PARTID Narrowing at all yet; however,
> > I think more thought is needed for that.)
> > 
> 
> Yes, localize the narrow-partid feature within mpam_resctrl.c file is
> the biggest restructuring improvement in this version. Of course, thanks
> for your meticulous review and insightful comments.
> 
> In the meanwhile, I have roughly read through the patch set you sent
> out, especially patch 4 (arm_mpam: Introduce flexible CLOSID/RMID
> translation). If I have any comments worth discussing, I will try to
> share them.

I would appreciate that, thanks!

> 
> > 
> > Note: Are there bisection issues with some of the patches in the
> > series?  It looks like not all of the ID conversions are applied in the
> > same patch, so I'm wondering whether strange behaviour may be seen at
> > the intermediate commits.
> > 
> 
> The original intention of splitting the patch set was to facilitate review.
> I will merge the part of patch 5 into patch 2 and consider
> making the patch set be friendly for bisection issues.
> 
> 
> Best regards,
> Zeng Heng

Understood.

James may have a view on how important bisectability is, since this is
not upstream code -- but it is probably a good idea to maintain full
bisectability if at all possible.

Cheers
---Dave