Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <13187438-94f4-d6d0-81c2-cdedc0d0243f@intel.com>
Date:   Thu, 15 Jun 2023 15:03:23 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.11.2
Subject: Re: [PATCH v4 02/24] x86/resctrl: Access per-rmid structures by index
Content-Language: en-US
To:     James Morse <james.morse@arm.com>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     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>,
        <xingxin.hx@openanolis.org>, <baolin.wang@linux.alibaba.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Xin Hao <xhao@linux.alibaba.com>, <peternewman@google.com>,
        <dfustini@baylibre.com>
References: <20230525180209.19497-1-james.morse@arm.com>
 <20230525180209.19497-3-james.morse@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230525180209.19497-3-james.morse@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eTJIamhwczVoOWVxUDl2NDZyVlAxYVN2d0tBeCt4RW1ubEcwMXp5ZzRNVWFK?=
 =?utf-8?B?L3dxUXJLSWNJZHI5eTB3OWQ3ckRlcUNIdWZ0QnJTakpIM0dBcjJMU1RtcGh5?=
 =?utf-8?B?dWhrRnRja1RVNGFjeTNydzdYbnpxQWt2aXJPVnFiV2g3T0FCSlhUTVVMdUVp?=
 =?utf-8?B?cFI1N1VDdldsQ2lsaDN3Y0xlU2I0TXN0alFEUXE2L1lHSVJCOHZDelNoc3ow?=
 =?utf-8?B?Sm5ySjRGTXZSM2I4aWdoVEZBK2tuUWx1MDdKWEFlMSs0eVpXam50MlBac0lu?=
 =?utf-8?B?NDZzbEw5QktKUWRFR0p1R2Ntd0IxRXlwdndQNWI0SXhEWC9IeDZyM0c0aWto?=
 =?utf-8?B?a21hRlVyYktQNjNSc1VyRmZlOEFIRTBMRFI5dnJleWhYYTl0akdPck41b01z?=
 =?utf-8?B?endPUkhsZHZaWHo2dUoxaTBzazZPM2pKcEpiQmcrQTVXWkt6VmVCUDR6Zk9k?=
 =?utf-8?B?elVMVDVrYlVuMk5Xd01LNFA2TWkxRVhwdzF2aXZBZTNIbkRFQzFpUFNlOEZr?=
 =?utf-8?B?ZGRGR3Z2VTBmNWVtMzFGaHRPZGpDeFVDMUxyZWZDZGtFSUFBU3VMVjZZODFj?=
 =?utf-8?B?aFdoOUkyYndOT1VxbmJkZG5xeVBXS1BkaS92TTY0dFpNRHRGVUVSWWJLU2tE?=
 =?utf-8?B?czkzMHRpMEtZbzNoL3dCOENvLzJFQ0ZMbnNHRjFBNEVMaVhXdCtPWE10TUtE?=
 =?utf-8?B?eGF2aDNXa0E1TER0MlFEUFR6aElZcUREM0V6OWJWZ1J0VkRUd2hqTlRkMTRS?=
 =?utf-8?B?TUlmWkJpOFZBQmNtb2g3dXA1K08zSm5ic0ZZbGZrc2FldU5ZODN3QVU0VDJs?=
 =?utf-8?B?czFtMlBKQ0EzbytvbktraGYrN3lqZk1Ha2p6cjlUL1dncCtDbkZKd1NWdTh4?=
 =?utf-8?B?ZUdzbGtPTHN3cS9NTVNHbFBjaTc1TURKUm5tNHYwcVpUSzZPWTZWWkZVNGw2?=
 =?utf-8?B?T1pCWnpEOUc2dWlXeVBnajJnSTVaam5uQUpVL2VXUHdzaEt5bVgxZC92LzFL?=
 =?utf-8?B?VGpHcWVLN0w0bUUrK1p6OHhhRWplbVc2SmZleFRhS2U4K1ZNaW5ObTZYS3Rm?=
 =?utf-8?B?ZHU4L2xzdjlYVWtVVFBFeG5OQ1k3eG5oSURkUENNM1dGTnpQSWp1N0RkUmZy?=
 =?utf-8?B?SitqRUxUR0Q4TFdBbGQ2S1NlcHE4N3U2TGRXaVhHQnQwNzc1anh4U3JSZFRQ?=
 =?utf-8?B?LzBIaUR3SU14VkZNYTNyN0szSERMeTRZdmRQRUIxR2tjQjQxR0JsMENBdmRE?=
 =?utf-8?B?S0lvUkEyL0pQR3dLRkF5ak5WTmFHcnZFV0c3N2h5VWdYSG5zTDgzK2pBenJl?=
 =?utf-8?B?UkVSYkFiSHBVeFJIc2FWOS9aVEtlbkNFMUtDeHBxTTdVdmJ6c0lxNGdrZ1Mx?=
 =?utf-8?B?TC9hVnhERXRNTWtad0NMejVXcjF3T0huaG96cWN6bVdUa3lmcEo0aE0zd0NQ?=
 =?utf-8?B?KzkxRUVXL2hrdjJUYlJFT3ZxUTU4Y3dWTUZmampkUmludkdIMFdYUE1jV3Ni?=
 =?utf-8?B?N011aExHNzY1ZEh6bXNtRHI2OG51S2Z6aDFmN2ZpZ0dxWEllS016M05mNFll?=
 =?utf-8?B?aXcrMXpsdStXRnBxM09HMTZsdFhZaTZkUVhTdXMvV25jTTJUcFQ5ZFR4VTNp?=
 =?utf-8?B?QVUwejU1SEVoaHdkVk5yaUIwSTkzSlUvYjByalEwMUhWbEZuUmNqUkF1VXdL?=
 =?utf-8?B?T1FJWGYraFoxK0MzbjlrZ2pCbVJ0S2t5QXVYRTZjTmVmemhhOTlMdFRwLzdq?=
 =?utf-8?B?bU5oNGNrZ2pTMEYrSmVTemRQRjdMajBiWDIxa3FPeUZWQVJEbVh0RjRtS0M4?=
 =?utf-8?B?UllmY3FnQlppOTVMS3JTMTZaR1lzS3dPTElhNU9GQUYrZmUzZkhuVHlWQTRr?=
 =?utf-8?B?YlZYbGFvY3BNdGcycUFaTDIzOTdsejhEQkN1Tk9ROTlQNGhlWUZISUsyYXN0?=
 =?utf-8?B?bzgzZUtZT09BaTlpNC9MY093dlcwckpaZndwdm92elJ3MktyZXZmaURXN1k4?=
 =?utf-8?B?d3Nva3JOWVZMbFV0SDFac0VKQ29ibHM2Q2VxYk5uSXU2N1FmZ1VPMFBTUlFx?=
 =?utf-8?B?M2RmWC9DL0VubEJyZC9hall2YTcrNm1zQUcxNFpaWVV0Y1pFS014Q2xKcjhp?=
 =?utf-8?B?TDVoMEk5MG0rTWVoSk05Z1dlbkNhV0QrTkJyK0JMSlNjdk1aaVBYZmozR0Fu?=
 =?utf-8?B?VHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 12566648-111f-4496-32e9-08db6dec5785
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Jun 2023 22:03:25.2592
 (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: uvKo+kpK+KvDrSv1Q6roWi2lM+iHXaWUVf5mauOaz6JCn+KKKoNzMnddfxhq+8b789NVNAMv2xM5Cuo6bOStFcqGYhKwuokEWmJIs5Mohfg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4781
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi James,

On 5/25/2023 11:01 AM, James Morse wrote:
> Because of the differences between Intel RDT/AMD QoS and Arm's MPAM
> monitors, RMID values on arm64 are not unique unless the CLOSID is

I find the above a bit confusing ... the theme seems to be "RMID values
on arm64 are not unique because they are different from Intel".
Compare to: "One of the differences between Intel RDT/AMD QoS and
Arm's MPAM monitors is that RMID values on arm64 are not unique unless
the CLOSID is also included."

> also included. Bitmaps like rmid_busy_llc need to be sized by the
> number of unique entries for this resource.
> 
> Add helpers to encode/decode the CLOSID and RMID to an index. The
> domain's rmid_busy_llc and rmid_ptrs[] are then sized by index,
> as are the domain mbm_local and mbm_total arrays.

You can use "[]" to indicate an array.

> On x86, the index is always just the RMID, so all these structures
> remain the same size.

I do not consider this accurate considering that the previous
patch increased the size of each element to support this change.

> The index gives resctrl a unique value it can use to store monitor
> values, and allows MPAM to decode the CLOSID when reading the hardware
> counters.
> 
> Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> Changes since v1:
>  * Added X86_BAD_CLOSID macro to make it clear what this value means
>  * Added second WARN_ON() for closid checking, and made both _ONCE()
> 
> Changes since v2:
>  * Added RESCTRL_RESERVED_CLOSID
>  * Removed a newline
>  * Repharsed some comments
>  * Renamed a variable 'ignore'd
>  * Moved X86_RESCTRL_BAD_CLOSID to a previous patch
> 
> Changes since v3:
>  * Changed a variable name
>  * Fixed various typos
> ---
>  arch/x86/include/asm/resctrl.h         | 17 ++++++
>  arch/x86/kernel/cpu/resctrl/core.c     |  2 +-
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 84 +++++++++++++++++---------
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |  7 ++-
>  include/linux/resctrl.h                |  3 +
>  6 files changed, 83 insertions(+), 31 deletions(-)
> 

...

> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 86574adedd64..bcc25f5339c0 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -142,12 +142,29 @@ static inline u64 get_corrected_mbm_count(u32 rmid, unsigned long val)
>  	return val;
>  }
>  
> -static inline struct rmid_entry *__rmid_entry(u32 closid, u32 rmid)
> +/*
> + * x86 and arm64 differ in their handling of monitoring.
> + * x86's RMID are an independent number, there is only one source of traffic
> + * with an RMID value of '1'.
> + * arm64's PMG extend the PARTID/CLOSID space, there are multiple sources of
> + * traffic with a PMG value of '1', one for each CLOSID, meaning the RMID
> + * value is no longer unique.
> + * To account for this, resctrl uses an index. On x86 this is just the RMID,
> + * on arm64 it encodes the CLOSID and RMID. This gives a unique number.
> + *
> + * The domain's rmid_busy_llc and rmid_ptrs are sized by index. The arch code

rmid_ptrs[]

> + * must accept an attempt to read every index.
> + */
> +static inline struct rmid_entry *__rmid_entry(u32 idx)
>  {
>  	struct rmid_entry *entry;
> +	u32 closid, rmid;
>  
> -	entry = &rmid_ptrs[rmid];
> -	WARN_ON(entry->rmid != rmid);
> +	entry = &rmid_ptrs[idx];
> +	resctrl_arch_rmid_idx_decode(idx, &closid, &rmid);
> +
> +	WARN_ON_ONCE(entry->closid != closid);
> +	WARN_ON_ONCE(entry->rmid != rmid);
>  
>  	return entry;
>  }

...

> @@ -377,14 +399,16 @@ static void add_rmid_to_limbo(struct rmid_entry *entry)
>  
>  void free_rmid(u32 closid, u32 rmid)
>  {
> +	u32 idx = resctrl_arch_rmid_idx_encode(closid, rmid);
>  	struct rmid_entry *entry;
>  
> -	if (!rmid)
> -		return;
> -
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> -	entry = __rmid_entry(closid, rmid);
> +	/* do not allow the default rmid to be free'd */
> +	if (!idx)
> +		return;
> +

The interface seem to become blurry here. There are new
architecture specific encode/decode callbacks while at the same
time there are a few requirements:
- closid 0 and rmid 0 are reserved
- closid 0 and rmid 0 must map to index 0 (the architecture
callbacks thus do not have must freedom here ... they must
return index 0 for closid 0 and rmid 0, no?).

It does seem a bit strange that the one layer provides values (0,0)
to other layer while requiring a specific answer (0).

What if RESCTRL_RESERVED_RMID is also introduced and before encoding
the CLOSID and RMID the core first checks if it is a reserved entry
being freed and exit early if this is the case?


> +	entry = __rmid_entry(idx);
>  
>  	if (is_llc_occupancy_enabled())
>  		add_rmid_to_limbo(entry);

...

> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 7d80bae05f59..ff7452f644e4 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -6,6 +6,9 @@
>  #include <linux/list.h>
>  #include <linux/pid.h>
>  
> +/* CLOSID value used by the default control group */
> +#define RESCTRL_RESERVED_CLOSID		0
> +

#define RESCTRL_RESERVED_RMID		0 ?

>  #ifdef CONFIG_PROC_CPU_RESCTRL
>  
>  int proc_resctrl_show(struct seq_file *m,


Reinette