From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 19157C3DA42 for ; Wed, 10 Jul 2024 20:11:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=y2gLkXyrivD9RbOie2niuX/ZAaszbzqyjyBh6BbFgKg=; b=AE0Y6zZqWmv+G7HPnvRghPOc2W ITaoo3neHqXfEPGc4CnhLj6yDi8zxclfBllTIhHacWx/rr4NfCmqpzsXaEEqJsf8IlTp4DOQa5ydH 2rTLpkeJPelpCpAN5O/bkjOuSbcgvhQEGoIJ+tRrCyQxj7ZWhZN8+RAJgdAMTgZjP3mX/XlxTEYFg hop5KDiRs8BC2DRZXExv4mGyBCOJatOq9tj16XnFx21eMLFuQV8NAXQ8GYloBPIPAg0PoLbg10ATX k9Qc0rg316wukelaQOSWN9IwCXOkV3SQeTah5Us3zCZl0qm7Iy0Xr1Bt4qdNcPXrKUxrhgbV1x3vW 0MhR6R3g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sRdel-0000000BcO1-0eiz; Wed, 10 Jul 2024 20:11:15 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sRdeU-0000000BcJg-0kYI for linux-arm-kernel@lists.infradead.org; Wed, 10 Jul 2024 20:10:59 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 54D5E61A39; Wed, 10 Jul 2024 20:10:57 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id F2FC0C32781; Wed, 10 Jul 2024 20:10:55 +0000 (UTC) Date: Wed, 10 Jul 2024 21:10:53 +0100 From: Catalin Marinas To: Mark Brown Cc: Will Deacon , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Mark Rutland Subject: Re: [PATCH] arm64/fpsimd: Ensure we don't contend a SMCU from idling CPUs Message-ID: References: <20240618-arm64-sme-no-cpuidle-v1-1-1de872e1691f@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240618-arm64-sme-no-cpuidle-v1-1-1de872e1691f@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240710_131058_340858_38FCB48E X-CRM114-Status: GOOD ( 38.36 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org + Mark R, he commented on the other patch. On Tue, Jun 18, 2024 at 02:57:42PM +0100, Mark Brown wrote: > When we enter the kernel we currently don't update any of the floating > point register state until either something else uses floating point or we > get a CPU_PM_ENTER notification during suspend or cpuidle. This means that > for a system which has been configured with both suspend and cpuidle > disabled we will leave whatever floating point state was loaded in the > registers present while a CPU is idling. > > For SME this may cause an idling CPU to interfere with the operation of > other CPUs, SME may be implemented as a SMCU shared with between multiple > CPUs. Leaving ZA or especially streaming mode enabled may be taken by > the hardware as an indication that SME is active use by the CPU and cause > resources to be allocated to it at the expense of other CPUs. > > Add an arch_cpu_idle_enter() implementation which disables SME if it is > active when we idle the CPU, ensuring that we don't create any spurious > contention even if cpuidle is not enabled. I guess this approach is useful when the kernel does a light WFI rather than going all the way to firmware for a deep sleep state. In general, the shallower the sleep state is, the more CPU state is retained. From a power perspective, I wonder whether we should leave the decision to drop the SME state to a cpuidle driver. If SME is implemented as a shared unit, doing an SMSTOP could be a useful indication that other tasks can use it. Which situations should we consider for such idle scenario (we discussed them offline briefly)? I think: 1. Thread migration: a thread using SME is moved to a different CPU. Here SMSTOP makes sense because a new thread scheduled eventually will need a different SME state. 2. Thread page fault followed by waiting for I/O etc. and the kernel may switch to idle. Here it's probably less beneficial to do an SMSTOP. Any other cases? Blocking syscalls don't matter since we don't preserve the state between calls. The trade-off is for case (2) above and it depends on whether it happens sufficiently often to be noticeable. I wouldn't think so. > Signed-off-by: Mark Brown > --- > arch/arm64/include/asm/fpsimd.h | 2 ++ > arch/arm64/kernel/fpsimd.c | 14 ++++++++++++++ > arch/arm64/kernel/process.c | 5 +++++ > 3 files changed, 21 insertions(+) > > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h > index bc69ac368d73..e1453a1d261d 100644 > --- a/arch/arm64/include/asm/fpsimd.h > +++ b/arch/arm64/include/asm/fpsimd.h > @@ -82,6 +82,8 @@ extern void fpsimd_restore_current_state(void); > extern void fpsimd_update_current_state(struct user_fpsimd_state const *state); > extern void fpsimd_kvm_prepare(void); > > +extern void fpsimd_idle_enter(void); > + > struct cpu_fp_state { > struct user_fpsimd_state *st; > void *sve_state; > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 82e8a6017382..400eaad374a2 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -2126,6 +2126,20 @@ static void __init fpsimd_pm_init(void) > static inline void fpsimd_pm_init(void) { } > #endif /* CONFIG_CPU_PM */ > > +void fpsimd_idle_enter(void) > +{ > + /* > + * Leaving SME enabled may leave this core contending with > + * other cores if we have a SMCU, disable whenever we enter > + * idle to avoid this. Only do this if they're actually > + * enabled to avoid overhead in cases where we don't enter a > + * low enough power state to loose register state. > + */ > + if (system_supports_sme() && > + (read_sysreg_s(SYS_SVCR) & (SVCR_SM_MASK | SVCR_ZA_MASK))) > + fpsimd_save_and_flush_cpu_state(); > +} Do we always enter here via the idle thread? If we already had a thread switch we probably don't need to save the SME state again, only flush the state. > #ifdef CONFIG_HOTPLUG_CPU > static int fpsimd_cpu_dead(unsigned int cpu) > { > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 4ae31b7af6c3..fd616882cd49 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -68,6 +68,11 @@ EXPORT_SYMBOL(__stack_chk_guard); > void (*pm_power_off)(void); > EXPORT_SYMBOL_GPL(pm_power_off); > > +void arch_cpu_idle_enter(void) > +{ > + fpsimd_idle_enter(); > +} > + > #ifdef CONFIG_HOTPLUG_CPU > void __noreturn arch_cpu_idle_dead(void) > { -- Catalin