Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <435fd495-9c3d-3c53-5c6e-441308ab18d3@amd.com>
Date:   Thu, 5 Oct 2023 16:33:00 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH v6 14/24] x86/resctrl: Allow resctrl_arch_rmid_read() to
 sleep
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>,
        Reinette Chatre <reinette.chatre@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, amitsinght@marvell.com
References: <20230914172138.11977-1-james.morse@arm.com>
 <20230914172138.11977-15-james.morse@arm.com>
From:   "Moger, Babu" <bmoger@amd.com>
In-Reply-To: <20230914172138.11977-15-james.morse@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?REkxaVorYzV0VHdZS3pvR0owOHdIRmZidHhrVEJlQUxVQXNraDE4TGdxK2Qr?=
 =?utf-8?B?WVJXelBNa3RoeEtla3VBbEhLR0E2VUwwVWxFWU92NjgvSGhITnU2T2pGTkhL?=
 =?utf-8?B?QU5ORnRjeEtCY05tLzdLYWY0NzE0RGNJdWV4cURkUU5sekF6Tit6eTFNR1RI?=
 =?utf-8?B?bHVHRkIybFE1SFY5RDh5dVVZREJKempDa3pyaXhKZkZTQWQ0M0IrQnl1WVF2?=
 =?utf-8?B?dnFZY0s0dGJEcDltM0RGNE4yaXl3RDJWOFV2YWhMQ1VPaXhOQmxlTGRUL0Ny?=
 =?utf-8?B?VnhVR3Z1aG9jOWE5R1JJa05RaEk4UnVobzc2MW5MNUs4aytvakRGeHFMajll?=
 =?utf-8?B?Zk1sYkNpbHY2QVJCSzE1bVNWb0c0MVNlY096alAxSUpQczVOTG5NZlhHSkx3?=
 =?utf-8?B?cDlDWEt4VDhVeURMb3FPdVVpRkNmeDhGc1IwaEY4Nmc0RldYWmExT3VCa054?=
 =?utf-8?B?dk1zeXlEMmdaRE8xS0N6K3pEU043MVZGYXhmTm91bE5ycWUvdjlReUIzMmRV?=
 =?utf-8?B?RDFHczRsWDlLRmNRb0NpOFo3VXNoYS81M0IxNDFYOS9oRGR0RndyMkQ3akJP?=
 =?utf-8?B?UHNibVQvK0krMGcyaHhrQnNaaFNxQ3FYTWRTRFZwNmZjMlpwVTh0N2FZaUhK?=
 =?utf-8?B?d0FyRWx1Y2NMYjExaHZjdk5YUGFrRlg4UVZyUkRlRzBkQ2gvREgyMWZSVk5M?=
 =?utf-8?B?ZHRjVWxvblFFM3JCbCtXYm1JbU9VRU5nMzlyWXc5UWErbWR2ZkpSUjFzZUt2?=
 =?utf-8?B?ekxqSTNZMEtXVW9DTjlqODQ4a3l4Q3U5Rk51cGRIcFNMYUFpcmdPRnM3MitC?=
 =?utf-8?B?eVBRSWhHWTJBQVVDSUNDY0VXb20rL1VwalpNSWtxZ2U5REhSaTRKY0RidTVx?=
 =?utf-8?B?M2xEcVp6c3lGZ1R5OEJZYXpLUHZteDdFM002WlJ4NFF5b1Y4MkhlVFVEVmJ4?=
 =?utf-8?B?d05STlJYaXB0aXVzOEcxUlFzQWMveFNvVHJwWFBEUDZyRHdCelRPSUlNRE9F?=
 =?utf-8?B?Z0tObVZtaTFuR0o4Sm50WGpnNWljbGFCdEIwYUxBQ2Z2V2dEK1VlYzJaUExH?=
 =?utf-8?B?T240UDZaMnYxTVF3dncwVmxqbGR4OHY3c2xzT1FJMkhqdUxPOFBxV2IzMEF5?=
 =?utf-8?B?dnoyS2MyWHlVSGdhWjcrQ01EZGJKb0NqQWRTY1FhQWxoajFyTFp6WlZUSGVU?=
 =?utf-8?B?bDdRUjUwcVd1R1pkV1Fyc2ZMMWFxdUZYOXJSWHBCTlhLYXJVRTBBK1VtR2lR?=
 =?utf-8?B?WjN0LzJEalE4NUU0U0NkcXZzVURINGU5ekJ3VEw2ZnNoMFVuV1RISkJKSGRz?=
 =?utf-8?B?ZHVBeEx2LzZ0dG9DNStzOFFRVklDODg4YmlCSmRwenFKa3orUlZxUVA5dlVi?=
 =?utf-8?B?Y1QxaThQNmJSYU42UDBsS29SRFNuRDZ3OTBTV1RUMkx2RGx2d2E4a1BSUlhG?=
 =?utf-8?B?Qm8rb3hVNld3VDBRY294TTFiTUY3M0pDa0hZSUZrOVJqSnZRK2pxLzdrVi96?=
 =?utf-8?B?ZXk0VTh0WGJJTTZZVlpEZExSRUx4bW5mdW9JRTRzU2xGYWVZR2xZcHc2cm1G?=
 =?utf-8?B?WHJTMnIvMGFpVHd6MWcvU1JDbGJibnQ0MG4vYVFndEg4aXNBUzRkUFZTOFk3?=
 =?utf-8?B?QU5peENNTk5IcVorRzVMTXlFSEp6d2xFcmUwWXpYZS9KSHgvV3FUeGkwckZx?=
 =?utf-8?B?N0d3VjIvTzM5aUtyS2lwbjc0RFFBOFgwcElwTzFsa3l4ajNvdzRKY095azhI?=
 =?utf-8?B?eUZTYUhiY3NKcVRPaHgrRGlsZzhGNGhzWUUwNGdlZkZvdllmU25QdEplVUsz?=
 =?utf-8?B?c1oxR0E5dHRVZzRIcGl5ZG16OHBPZGNNZXFWQ05KZEpHaFVRTlBoTnFCQ0N6?=
 =?utf-8?B?TWZ4UnYrVzZnTlB2ekFUNWUxYUlSR2V4L1kwQm9LdE52R1dRdXdOR1JsRkRS?=
 =?utf-8?B?QTEvTEFwWno2cUUveWcxU0cvQTc1eENzRklybzlXbTRHOHYxNFRLMHJncFZ0?=
 =?utf-8?B?dHZwWUlVZUlDaGlINHNUYVU1b0xUckdRbS9CbGtJQWkzMEsvOUhubndkdGRN?=
 =?utf-8?B?YjFIcFBzM3Naa0dORjcxY0RzZWM5OTBJWkhPOXZrclNXVHVFL3RxTHl1amVE?=
 =?utf-8?Q?DcRg=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 52eda0e9-0a00-4a66-fbe3-08dbc5eaa89d
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Oct 2023 21:33:04.6642
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: J6FeHmjvngilDjvy5lnntwGlWlF8AIp69MPAkIsrrqSYXhRBDV7/mkdp7IrDQU3a
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW6PR12MB8735
X-Spam-Status: No, score=-4.1 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Thu, 05 Oct 2023 14:33:18 -0700 (PDT)

Hi James,

On 9/14/2023 12:21 PM, James Morse wrote:
> MPAM's cache occupancy counters can take a little while to settle once
> the monitor has been configured. The maximum settling time is described
> to the driver via a firmware table. The value could be large enough
> that it makes sense to sleep. To avoid exposing this to resctrl, it
> should be hidden behind MPAM's resctrl_arch_rmid_read().
>
> resctrl_arch_rmid_read() may be called via IPI meaning it is unable
> to sleep. In this case resctrl_arch_rmid_read() should return an error
> if it needs to sleep. This will only affect MPAM platforms where
> the cache occupancy counter isn't available immediately, nohz_full is
> in use, and there are there are no housekeeping CPUs in the necessary

:%s/there are there are/there are/

Thanks

Babu

> domain.
>
> There are three callers of resctrl_arch_rmid_read():
> __mon_event_count() and __check_limbo() are both called from a
> non-migrateable context. mon_event_read() invokes __mon_event_count()
> using smp_call_on_cpu(), which adds work to the target CPUs workqueue.
> rdtgroup_mutex() is held, meaning this cannot race with the resctrl
> cpuhp callback. __check_limbo() is invoked via schedule_delayed_work_on()
> also adds work to a per-cpu workqueue.
>
> The remaining call is add_rmid_to_limbo() which is called in response
> to a user-space syscall that frees an RMID. This opportunistically
> reads the LLC occupancy counter on the current domain to see if the
> RMID is over the dirty threshold. This has to disable preemption to
> avoid reading the wrong domain's value. Disabling pre-emption here
> prevents resctrl_arch_rmid_read() from sleeping.
>
> add_rmid_to_limbo() walks each domain, but only reads the counter
> on one domain. If the system has more than one domain, the RMID will
> always be added to the limbo list. If the RMIDs usage was not over the
> threshold, it will be removed from the list when __check_limbo() runs.
> Make this the default behaviour. Free RMIDs are always added to the
> limbo list for each domain.
>
> The user visible effect of this is that a clean RMID is not available
> for re-allocation immediately after 'rmdir()' completes, this behaviour
> was never portable as it never happened on a machine with multiple
> domains.
>
> Removing this path allows resctrl_arch_rmid_read() to sleep if its called
> with interrupts unmasked. Document this is the expected behaviour, and
> add a might_sleep() annotation to catch changes that won't work on arm64.
>
> Reviewed-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
> Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
> Tested-By: Peter Newman <peternewman@google.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> The previous version allowed resctrl_arch_rmid_read() to be called on the
> wrong CPUs, but now that this needs to take nohz_full and housekeeping into
> account, its too complex.
>
> Changes since v3:
>   * Removed error handling for smp_call_function_any(), this can't race
>     with the cpuhp callbacks as both hold rdtgroup_mutex.
>   * Switched to the alternative of removing the counter read, this simplifies
>     things dramatically.
>
> Changes since v4:
>   * Messed with capitalisation.
>   * Removed some dead code now that entry->busy will never be zero in
>     add_rmid_to_limbo().
>   * Rephrased the comment above resctrl_arch_rmid_read_context_check().
> ---
>   arch/x86/kernel/cpu/resctrl/monitor.c | 25 +++++--------------------
>   include/linux/resctrl.h               | 18 +++++++++++++++++-
>   2 files changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 7749e6569a4a..05d949ec94f1 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -278,6 +278,8 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
>   	u64 msr_val, chunks;
>   	int ret;
>   
> +	resctrl_arch_rmid_read_context_check();
> +
>   	if (!cpumask_test_cpu(smp_processor_id(), &d->cpu_mask))
>   		return -EINVAL;
>   
> @@ -454,8 +456,6 @@ static void add_rmid_to_limbo(struct rmid_entry *entry)
>   {
>   	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>   	struct rdt_domain *d;
> -	int cpu, err;
> -	u64 val = 0;
>   	u32 idx;
>   
>   	lockdep_assert_held(&rdtgroup_mutex);
> @@ -463,17 +463,7 @@ static void add_rmid_to_limbo(struct rmid_entry *entry)
>   	idx = resctrl_arch_rmid_idx_encode(entry->closid, entry->rmid);
>   
>   	entry->busy = 0;
> -	cpu = get_cpu();
>   	list_for_each_entry(d, &r->domains, list) {
> -		if (cpumask_test_cpu(cpu, &d->cpu_mask)) {
> -			err = resctrl_arch_rmid_read(r, d, entry->closid,
> -						     entry->rmid,
> -						     QOS_L3_OCCUP_EVENT_ID,
> -						     &val);
> -			if (err || val <= resctrl_rmid_realloc_threshold)
> -				continue;
> -		}
> -
>   		/*
>   		 * For the first limbo RMID in the domain,
>   		 * setup up the limbo worker.
> @@ -483,15 +473,10 @@ static void add_rmid_to_limbo(struct rmid_entry *entry)
>   		set_bit(idx, d->rmid_busy_llc);
>   		entry->busy++;
>   	}
> -	put_cpu();
>   
> -	if (entry->busy) {
> -		rmid_limbo_count++;
> -		if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
> -			closid_num_dirty_rmid[entry->closid]++;
> -	} else {
> -		list_add_tail(&entry->list, &rmid_free_lru);
> -	}
> +	rmid_limbo_count++;
> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
> +		closid_num_dirty_rmid[entry->closid]++;
>   }
>   
>   void free_rmid(u32 closid, u32 rmid)
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 660752406174..f7311102e94c 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -236,7 +236,12 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d);
>    * @eventid:		eventid to read, e.g. L3 occupancy.
>    * @val:		result of the counter read in bytes.
>    *
> - * Call from process context on a CPU that belongs to domain @d.
> + * Some architectures need to sleep when first programming some of the counters.
> + * (specifically: arm64's MPAM cache occupancy counters can return 'not ready'
> + *  for a short period of time). Call from a non-migrateable process context on
> + * a CPU that belongs to domain @d. e.g. use smp_call_on_cpu() or
> + * schedule_work_on(). This function can be called with interrupts masked,
> + * e.g. using smp_call_function_any(), but may consistently return an error.
>    *
>    * Return:
>    * 0 on success, or -EIO, -EINVAL etc on error.
> @@ -245,6 +250,17 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
>   			   u32 closid, u32 rmid, enum resctrl_event_id eventid,
>   			   u64 *val);
>   
> +/**
> + * resctrl_arch_rmid_read_context_check()  - warn about invalid contexts
> + *
> + * When built with CONFIG_DEBUG_ATOMIC_SLEEP generate a warning when
> + * resctrl_arch_rmid_read() is called with preemption disabled.
> + */
> +static inline void resctrl_arch_rmid_read_context_check(void)
> +{
> +	if (!irqs_disabled())
> +		might_sleep();
> +}
>   
>   /**
>    * resctrl_arch_reset_rmid() - Reset any private state associated with rmid