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: <36af82d5-0d48-f899-9e95-1ec89be20581@intel.com>
Date:   Fri, 31 Mar 2023 16:27:51 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.9.1
Subject: Re: [PATCH v3 11/19] x86/resctrl: Allow arch to allocate memory
 needed in resctrl_arch_rmid_read()
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>
References: <20230320172620.18254-1-james.morse@arm.com>
 <20230320172620.18254-12-james.morse@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230320172620.18254-12-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?QW1JUFVlSnZQTDh5OFBsVHU5ZEFqakxncnZnZHZGQ1RXUkVQR25nMnltbzdB?=
 =?utf-8?B?bFcxNXVROXh1M2tuSFBTb3RhVlNFZ3VDLy9PR0hpZmx0NWd1cWtiaVRqQmtE?=
 =?utf-8?B?a0l0aE5DR1JJS0dIZkp3RUE0d0NRcmNZRnBFSWxPMXRVVUdUZUN1KzNZSDM5?=
 =?utf-8?B?OHZ6V1dSaHBsRnp3VzBYeTJ3N1FLUk1vcWtsNURNMnBWZ2ZMb1BNdjdlazUx?=
 =?utf-8?B?TFV3MGgzb2F6WUYrS3hXK0hZcDhMQi9nMWRJV1JNeFFLcG5mNG9TV2tacGpx?=
 =?utf-8?B?MERVTG1EbEZXRHNXajAxSWZaWk9SajZZWURkaE9UYjMzTytQRFduVW9jZmFw?=
 =?utf-8?B?TzJHM2R5OUZheUJWM3pXYXE4bGZlRmVabktRa1dMOFpkaHFMMHlNSzM0SGE0?=
 =?utf-8?B?WGU1b21oRjRPS0daaGVGam91SUQ4dFoxVDFIeFJvSG90VHQ0ejFaMTFwZmxH?=
 =?utf-8?B?VlFCM3ppMFFOZDg1SVA2dTliT2hDL1FWcFZlMkNDa0ZtTi9uUXpMcEdDYlUx?=
 =?utf-8?B?SlkycTlMYjF6UHJranZxU3JhbFJpc0VMbXlYR1BuUEhISVNHSFJoNXY1Y1Bo?=
 =?utf-8?B?bkU1emphMzV0ZGZIWW5Kek9FVjRSQ2VZajhLbEdwOHNSUXhIL2w1ajNFZWVm?=
 =?utf-8?B?dlhSNGVxdU9ZSVJoREtEWjdZMml6V3VZRERuNHg5VU5VSTRjQVo2Y1Yva3pW?=
 =?utf-8?B?bjBOQlRVR2cwNWo5eS9VVWpEeWZUZ2VOYjh3MUlFRzBMWEZkblArWktCdE1r?=
 =?utf-8?B?WXV2VEVCM2pEL2lSMm01WktBQVBveXJXUjFHQUhWRS9aRFVSUVppb3BGeURz?=
 =?utf-8?B?YURqUWdjU002YldlWjYwMEoyd1NPYlkxeUxRcXJpQUJmb1VZL2l2dlpIRlpD?=
 =?utf-8?B?WVFPaDhOazl4ZTRVVCtzcmdmbWR6WndOdlBYMG1OYXc4SFRNYnRxRFp0SVVs?=
 =?utf-8?B?N3M3VEQ2SVdGeXh1S00xbnlQTXpSOHhtUjNPTElGOVovZUpSOGd5Uk9DWmpx?=
 =?utf-8?B?MHJwNm1OTksrb2MwaXRNUVZTdDNOT05yNFdYeGdrc1UwcGtVZ2sycVFyUUVQ?=
 =?utf-8?B?MGdmT2Qwckp1dXEwSld4R1dxV1lzbitDdW9yUnhZYVY4czUxSVdpMXg1MGhk?=
 =?utf-8?B?L0VROThuSURDWldwaUNhOTM5TlZoWWYyeWFpNHM4M045VHpvdjNZNVVDNXR0?=
 =?utf-8?B?WTAwV2RTU2tkVXVpakFTczVscFpFNmVBc0Q0RG05cEJVejhmZ29WckZod0Zs?=
 =?utf-8?B?SXljRi9ibUFEL3BuYnN0WkRISVpnem9Tb2ZyZlRtWng3em5kUlEyL2l0clJ4?=
 =?utf-8?B?UXZFY3ZSTWdXcjBqVHo2NllQdTF6UEZFNk9KRm8weHE2dWpNTmEybjNGcTFp?=
 =?utf-8?B?TFRNdGlySmFvTytSZStNV2xETksvR0p3djg5c2ltcUkzY2VKU1FWL2xIeXRx?=
 =?utf-8?B?TXlkVkRWYzdtRTNvbVdxNmNJMzRBaWg3T1dqNEdDS1FmNGVQMkxOWEN5ZkFo?=
 =?utf-8?B?ZzRzd29KSFRzckxsaTlXZTVBbmFLbWhaRFdtVFcwTzRZOERXUHVFRzU5SjE5?=
 =?utf-8?B?WGJoSXhISXlwRDIzSEFFWUc1dlpZUFNYWElLTnk3SGxhbWQyOTlWUWNJUG80?=
 =?utf-8?B?TWpjNi9hNVVmbTBkcHd3UnhHTVVxSlFpb2YxUXN2T1RBRHNOdHRDSmRwcWpH?=
 =?utf-8?B?dS9rRERBcllGTjluV3oyTUVDZVpETmtzSHJKNEdHMS91SHV1TVVMLytYOVdv?=
 =?utf-8?B?eUFpS2c4WFNFdUtNaXQ2bjZBZVFVcUszUWl4MkE1NW01MzJoSzgrUmdKQWlo?=
 =?utf-8?B?WWZBV09VYkpoK2s2QjA0UW0vNGlLWm5uZmRBM0ZIQlNTZkt6TzJ2SXN1TnZu?=
 =?utf-8?B?ZmxVZmptZXM3UlcySE5pRTBSQzBJT3JlOG5FOW16aGNQRTJWK3BmeWR2bm02?=
 =?utf-8?B?SGozN3oxN3NlbXl0bHA1MEhVemhEcHBMZEpqM0haaTdFaUxMMlB1Qk9CZ0NC?=
 =?utf-8?B?N2FMTno0R3hsbUs1ZksyQnVjODJXNXFocDBNQkdpOEhiLy9vRGJhRnhKZFBx?=
 =?utf-8?B?cTRKTDI4YmNobmoya2ZnZXdPL05WRmZUM0FkWkowUzYyV0dxMUdBMEZIb2hh?=
 =?utf-8?B?eXhkajBzdjljSHplZmpNOFo2THU3ejhaSTNsVHJmbEQ3ZXdjaTArUTN1Rk5i?=
 =?utf-8?B?eXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9b2917cc-70cc-4db9-8da8-08db323f8d96
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Mar 2023 23:27:54.5108
 (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: 3hcySOubd/Kpv4tt+9SMUWCJnoVTtaufQMB/SFElohP7KKnSWrccocpy0GO4Y9FtrRp3xnm9CU3LjN8rxIDEKtU+mYSdyVDlYUisHOLaU00=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6426
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=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 autolearn=unavailable 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 3/20/2023 10:26 AM, James Morse wrote:
> Depending on the number of monitors available, Arm's MPAM may need to
> allocate a monitor prior to reading the counter value. Allocating a
> contended resource may involve sleeping.
> 
> All callers of resctrl_arch_rmid_read() read the counter on more than
> one domain. If the monitor is allocated globally, there is no need to

This does not seem accurate considering the __check_limbo() call that
is called for a single domain.

> allocate and free it for each call to resctrl_arch_rmid_read().
> 
> Add arch hooks for this allocation, which need calling before
> resctrl_arch_rmid_read(). The allocated monitor is passed to
> resctrl_arch_rmid_read(), then freed again afterwards. The helper
> can be called on any CPU, and can sleep.
> 
> Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/x86/include/asm/resctrl.h         | 11 +++++++
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 40 +++++++++++++++++++++++---
>  include/linux/resctrl.h                |  4 +--
>  4 files changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> index 752123b0ce40..1c87f1626456 100644
> --- a/arch/x86/include/asm/resctrl.h
> +++ b/arch/x86/include/asm/resctrl.h
> @@ -136,6 +136,17 @@ static inline u32 resctrl_arch_rmid_idx_encode(u32 ignored, u32 rmid)
>  	return rmid;
>  }
>  
> +/* x86 can always read an rmid, nothing needs allocating */
> +struct rdt_resource;
> +static inline int resctrl_arch_mon_ctx_alloc(struct rdt_resource *r, int evtid)
> +{
> +	might_sleep();
> +	return 0;
> +};
> +
> +static inline void resctrl_arch_mon_ctx_free(struct rdt_resource *r, int evtid,
> +					     int ctx) { };
> +
>  void resctrl_cpu_detect(struct cpuinfo_x86 *c);
>  
>  #else
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index a07557390895..7262b355e128 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -135,6 +135,7 @@ struct rmid_read {
>  	bool			first;
>  	int			err;
>  	u64			val;
> +	int			arch_mon_ctx;
>  };
>  
>  extern bool rdt_alloc_capable;
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index de72df06b37b..f38cd2f12285 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -15,6 +15,7 @@
>   * Software Developer Manual June 2016, volume 3, section 17.17.
>   */
>  
> +#include <linux/cpu.h>

Why is this needed?

>  #include <linux/module.h>
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
> @@ -271,7 +272,7 @@ static void smp_call_rmid_read(void *_arg)
>  
>  int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
>  			   u32 closid, u32 rmid, enum resctrl_event_id eventid,
> -			   u64 *val)
> +			   u64 *val, int ignored)
>  {
>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>  	struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
> @@ -317,9 +318,14 @@ void __check_limbo(struct rdt_domain *d, bool force_free)
>  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
>  	struct rmid_entry *entry;
>  	u32 idx, cur_idx = 1;
> +	int arch_mon_ctx;
>  	bool rmid_dirty;
>  	u64 val = 0;
>  
> +	arch_mon_ctx = resctrl_arch_mon_ctx_alloc(r, QOS_L3_OCCUP_EVENT_ID);
> +	if (arch_mon_ctx < 0)
> +		return;
> +

The vision for this is not clear to me. When I read that context needs to be allocated
I expect it to return a pointer to some new context, not an int. What would the
"context" consist of?


...

> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index ff7452f644e4..03e4f41cd336 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -233,6 +233,7 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d);
>   * @rmid:		rmid of the counter to read.
>   * @eventid:		eventid to read, e.g. L3 occupancy.
>   * @val:		result of the counter read in bytes.
> + * @arch_mon_ctx:	An allocated context from resctrl_arch_mon_ctx_alloc().
>   *

Could this description be expanded to indicate what this context is used for?

>   * Call from process context on a CPU that belongs to domain @d.
>   *


Reinette