Message-ID: <acf5c82f-1d33-4cd4-ff50-5f7c37f49a1e@huawei.com>
Date: Fri, 20 Dec 2024 18:59:31 +0800
From: Zeng Heng <zengheng4@...wei.com>
To: Dave Martin <Dave.Martin@....com>
CC: <james.morse@....com>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <jonathan.cameron@...wei.com>,
	<xiexiuqi@...wei.com>, "Wangshaobo (bobo)" <bobo.shaobowang@...wei.com>
Subject: Re: [RFC PATCH mpam mpam/snapshot/v6.12-rc1 v3 0/5] arm_mpam:
 Introduce the Narrow-PARTID feature for MPAM driver



On 2024/12/13 0:17, Dave Martin wrote:
> Hi,
> 
> On Sat, Dec 07, 2024 at 05:21:31PM +0800, Zeng Heng wrote:
>> The patch set is applied for mpam/snapshot/v6.12-rc1 branch of
>> https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git
>> repository.
>>
>> The narrow-partid feature in MPAM allows for a more efficient use of
>> PARTIDs by enabling a many-to-one mapping of reqpartids (requested PARTIDs)
>> to intpartids (internal PARTIDs). This mapping reduces the number of unique
>> PARTIDs needed, thus allowing more tasks or processes to be monitored and
>> managed with the available resources.
>>
>> 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.


>> In order to keep the existing resctrl API interface, the rmid contains both
>> req_idx and PMG information instead of PMG only under the MPAM driver. The
>> req_idx represents the req_idx-th sub-monitoring group under the control
>> group. The new rmid would be like:
>>
>>      rmid = (req_idx << shift | pmg).
>>
>> The new conversion relationship between closid/rmid and (req)PARTID/PMG is:
>>
>>      (req)PARTID = (rmid.req_idx * n) + closid,
>>      PMG = rmid.pmg.
>>
>> Each intPARTID has m reqPARTIDs, which are used to expand the number of
>> monitoring groups under the control group. Therefore, the number of
>> monitoring groups is no longer limited by the range of MPAM PMG, which
>> enhances the extensibility of the system's monitoring capabilities.
>>
>> ---
>> Compared with v1:
>>    - Rebase this patch set on latest MPAM driver of the v6.12-rc1 branch.
>>
>> Compared with v2:
>>    - Refactor closid/rmid pair translation
>>    - Simplify the logic of synchronize configuration
>>    - Remove reqPARTID pool
>> ---
> 
> 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.

> 
> 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

Powered by blists - more mailing lists