Received-SPF: pass (google.com: domain of linux-kernel+bounces-141419-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <e3304a27-0c2c-4981-ae4c-01540c180628@intel.com>
Date: Thu, 11 Apr 2024 10:41:03 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 24/31] x86/resctrl: Move get_config_index() to a header
To: Dave Martin <Dave.Martin@arm.com>
CC: James Morse <james.morse@arm.com>, <x86@kernel.org>,
	<linux-kernel@vger.kernel.org>, Fenghua Yu <fenghua.yu@intel.com>, "Thomas
 Gleixner" <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, "Borislav
 Petkov" <bp@alien8.de>, H Peter Anvin <hpa@zytor.com>, Babu Moger
	<Babu.Moger@amd.com>, <shameerali.kolothum.thodi@huawei.com>, "D Scott
 Phillips OS" <scott@os.amperecomputing.com>, <carl@os.amperecomputing.com>,
	<lcherian@marvell.com>, <bobo.shaobowang@huawei.com>,
	<tan.shaopeng@fujitsu.com>, <baolin.wang@linux.alibaba.com>, Jamie Iles
	<quic_jiles@quicinc.com>, Xin Hao <xhao@linux.alibaba.com>,
	<peternewman@google.com>, <dfustini@baylibre.com>, <amitsinght@marvell.com>,
	David Hildenbrand <david@redhat.com>, Rex Nie <rex.nie@jaguarmicro.com>
References: <20240321165106.31602-1-james.morse@arm.com>
 <20240321165106.31602-25-james.morse@arm.com>
 <bfc39b79-b3bf-4187-ba63-56db1ed6d565@intel.com>
 <Zhfy3goUX8kWM5Hn@e133380.arm.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <Zhfy3goUX8kWM5Hn@e133380.arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MTZjWlltem1IbW9hMEFad3hOWmdYYVo3c2hPMlZEQ3hZN2szcVhHdkMxVkRX?=
 =?utf-8?B?Nk90RGt6NWFXOWVOc2NGOG1VdVhEdGNIb2l4UGgwblhCRkFiZHJRb0wrN0dP?=
 =?utf-8?B?Z3dhQmVLYnhQQzhrWERIa0FCb0pQa0lRV0FrTStUOERJSDBLTHZkaW9ZRFZs?=
 =?utf-8?B?YzYrNU1DMzc5ZnlpUG9EWEtTd2dSemFNc3hWajhJVUlTcFZiaisvV2ZtT0Ju?=
 =?utf-8?B?aE9oU0VKTEdIVzhUVTJiUGd2b0d4d1JHcTVhK0ZvODBoQk52YURIZ0FKcnlW?=
 =?utf-8?B?OGdZTG1kcEIwVmFJdVRkYk50dWEvTjBzb09kV3hWL3JQMjYzNzNpUWJUa25V?=
 =?utf-8?B?NTNHLzErWFE2aWVRcmRnYTBoeVRqNzBvM1U5Q1pEU2ZjdHpIQzZpZVVNbVB1?=
 =?utf-8?B?M3lkR1lrSkJxWm1mbkd3ZUFOc0NLRDE2SUN3UzlJa0VZTGFWTGc5ZXV1UFp1?=
 =?utf-8?B?K1JaTTBDeUFSdW9XZytZWUJDQlhVNjF1NzQwZnNVc0JNUTFGcTF6bER3Y05L?=
 =?utf-8?B?UGUzbEZKSEZmVmptYXg3eXB3SFVmVURFQ1NPNll5MU9JSmQzV2N4NnpFVXZa?=
 =?utf-8?B?VjJCL1kzUENWSWlueGFMYUlOczQyNkwzUVU3d2dMbVhyOGFaRWE3K1lNSTJt?=
 =?utf-8?B?SDdQSjFGV0tVT3JyVGFoK2hCNjB2eXczWU96L0lVL0pUb05ZV1c0c2xlMGp6?=
 =?utf-8?B?SU9lcWtPcmFGWWVMWDgzOCtWUXFMTE95ZTZYMEhKTHVML28wR04zL0ZOZFRm?=
 =?utf-8?B?YmlrVm52dUVPV2ZUYzdkUVJSTUo3NW55c0RDQkZGMWFHUHdiNTVmUDE3TEth?=
 =?utf-8?B?eHJoSFRKQ0ZTZXFpazhiN1lNTDNnQVphUzVaWnlTdDdrbVgzY3c0VXVUVmJz?=
 =?utf-8?B?R1VGZnRFNnc3dmpKU3pkRStWaXRCdi8rS0RhaVU4NGJPRlgwOVBTbStPVmVm?=
 =?utf-8?B?Y3FpSzdxWjBXdnluTFl3MTRlTStpRGVadVNMNnc2Qm9QU2RTMFRsc2xYRE9H?=
 =?utf-8?B?cFdtTXRuTEZmZVJlakQ5TkZMTWU1bjhySk9aSUVZeXpacjhCbk5ZSExYWUUx?=
 =?utf-8?B?U25wMGRzOTlrQ1E2MUNDWnk4UURUdzVNV25GMzJiZitZRlVRamdsYnNsb1Zj?=
 =?utf-8?B?ajNEZ20wYWE0VmNUT0NJRUlncmMreDE5bUpsdFVJd1ZFaC9rQXRkbnpMZGF0?=
 =?utf-8?B?WjFrNnZHUkdjK24xaXBWSEJyZVhPeGpRMzJYdUQwVlhyTkUreTkwQzkrNVE3?=
 =?utf-8?B?Zzl4Y2tmRFFkV2lEaGpkSGMzeXdLeFlmY1VWUHB0Q2pmQmt4RkhHTXEwWTZn?=
 =?utf-8?B?cGFLTkNRZVAyMytKeXd3MFVaS1l5U1NLU1lOMFE3Q2l1czl1T3NEM3NYN01D?=
 =?utf-8?B?a0xFTkY4eUdzTmRpMlk3QlcrblpzSU5pZkt2VnBSdzh0ZUVDMkRTTkFZcnBj?=
 =?utf-8?B?TGMzeTZiZmJldVpIZFFEM3dXN2FuQ0FzYmlFTXF6UGllQ3grMFlLRDB3bkZU?=
 =?utf-8?B?ck9yNHJRc0ZLTmJ2bThvRVNyY0FFVWF6K1BkTkdCbUw5UnhtVE92ZE5wbkRF?=
 =?utf-8?B?QWRiTDMvQnVWVE5TWXc2Y1JEQ1VSRVBWUkh2czRHejdzZ2ROb1lDQmJhSU9I?=
 =?utf-8?B?amV1djVlc0tIay9UaWpsbDgycWI4Z2s0QWlzK25WYTZZVnhXUTFRWEpKT0lX?=
 =?utf-8?B?NEtsVXc2RklOazAyTlA4YzJaWHZzek9nb1pkNmdjMTFlaDNrVzd2by9rMHp2?=
 =?utf-8?B?cWdKNlVDTTdiMmxNTXVCOWduUG9vSGxJbmJGVWwzZmZpbGQwdnhMRjlPVTBV?=
 =?utf-8?B?bEtGUlJZOEVnRFdpeHhCcDVLaDBmTmc1Nmg4V1hBeEVPNjFnOTRzdVNYOFcr?=
 =?utf-8?B?R3k4VE4xUk9IU2UwZGE5eUprblQ5ZHRSUXVucmVqdHZBd3kvSEhiZTJha0Vj?=
 =?utf-8?B?TE5QRE5BWEdib1lkQ3Q4WGhuenJwQVVvemFjemJoMnZ6NmJ6UVk1enZTaUhC?=
 =?utf-8?B?eWNwMGt4S0U0UW9ocEJOZnpnMjFyenlKbkpHalgvKzBtaUFmbUZxYVhNVXFu?=
 =?utf-8?B?RUd4MnFyTlhOZnVnQjVESGlmZWtRdE1mTjdTVUJpM0dJTURwLzB2Zm1wOUM4?=
 =?utf-8?B?UDVaQjREb2RCbWQ3dktNVlp5c05XN0JzNnB0Ukl0ZmVTWXNhMTg1dGxvZHYz?=
 =?utf-8?B?d2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 96c60a39-c8ed-4f40-4f71-08dc5a4e92fd
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Apr 2024 17:41:10.1241
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: v+0+CpdgMrEoxXe2IZGruWXwpGrluNOc8ERFJ2fXM2beV2a9ddu/Y6J5C9m9kEaDIvR7ZTzT/1yN8lEs7jwdKUdkNAb/2rXFIMWlXuaKt8I=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7823
X-OriginatorOrg: intel.com

Hi Dave,

On 4/11/2024 7:25 AM, Dave Martin wrote:
> On Mon, Apr 08, 2024 at 08:25:26PM -0700, Reinette Chatre wrote:
>> Hi James,
>>
>> On 3/21/2024 9:50 AM, James Morse wrote:
>>> get_config_index() is used by the architecture specific code to map a
>>> CLOSID+type pair to an index in the configuration arrays.
>>>
>>> MPAM needs to do this too to preserve the ABI to user-space, there is
>>> no reason to do it differently.
>>>
>>> Move the helper to a header file.
>>>
>>> Signed-off-by: James Morse <james.morse@arm.com>
>>> ---
>>>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 19 +++----------------
>>>  include/linux/resctrl.h                   | 15 +++++++++++++++
>>>  2 files changed, 18 insertions(+), 16 deletions(-)
> 
> [...]
> 
>>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>>> index 3de5bc63ace0..73c111963433 100644
>>> --- a/include/linux/resctrl.h
>>> +++ b/include/linux/resctrl.h
>>> @@ -258,6 +258,21 @@ bool resctrl_arch_is_evt_configurable(enum resctrl_event_id evt);
>>>  void resctrl_arch_mon_event_config_write(void *info);
>>>  void resctrl_arch_mon_event_config_read(void *info);
>>>  
>>> +/* For use by arch code to remap resctrl's smaller CDP CLOSID range */
>>> +static inline u32 resctrl_get_config_index(u32 closid,
>>> +					   enum resctrl_conf_type type)
>>> +{
>>> +	switch (type) {
>>> +	default:
>>> +	case CDP_NONE:
>>> +		return closid;
>>> +	case CDP_CODE:
>>> +			return (closid * 2) + 1;
>>> +	case CDP_DATA:
>>> +			return (closid * 2);
>>> +	}
>>> +}
>>
>> (please check the tabs)
> 
> Noted.  I also see that redundant parentheses seem spuriously added
> compared with the original version of this moved code.  I can make a
> note to drop them if you prefer.
> 
>> This change is unexpected to me. Could you please elaborate how
>> MPAM's variant of CDP works?
>>
>> Thank you very much.
>>
>> Reinette
> 
> Note: I haven't discussed this specifically with James, so the following
> is my best guess at the rationale...  With that in mind:
> 
> For MPAM, CDP isn't a special mode; instead, the PARTIDs for
> instructions and data are always configured independently in the CPU.
> If resctrl is not configured for CDP, we simply program the same PARTID
> value both for instructions and data on task switch.
> 
> For a given resctrl control group we could pick two random unrelated
> PARTIDs, but there seems to be no advantage in doing that since resctrl
> enables cdp globally or not, and we would require more effort to
> translate resctrl closids to PARTIDs if we didn't pair the IDs up
> systematically.
> 
> (See [1], [2] in James' snapshot, which illustrate how he proposes
> to do it.)
> 
> 
> So, we may as well stick with the same scheme already established for
> x86: nothing forces us to do that, but it looks simpler than the
> alternatives.  I think that's the idea, anyway.
> 
> Then, if the same scheme is used by multiple arches (and 100% of the
> arches currently known to resctrl), it probably makes sense to share the
> definition of the mapping at least as a default for arches that don't
> have their own different ways of doing it.
> 
> Does this make sense?

It does, thank you very much.

> 
> I can recommend adding some of this rationale to the commit message
> if it helps (and assuming I'm right!)

Sounds good, thank you.

Reinette