Received-SPF: pass (google.com: domain of linux-kernel+bounces-97798-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <66fdce3a-c7f6-4ef4-ab56-7c9ece0b00e2@nokia.com>
Date: Sat, 9 Mar 2024 08:45:35 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] arm: smp: Avoid false positive CPU hotplug Lockdep-RCU
 splat
To: Joel Fernandes <joel@joelfernandes.org>,
 Russell King <linux@armlinux.org.uk>, "Paul E. McKenney"
 <paulmck@kernel.org>, Josh Triplett <josh@joshtriplett.org>,
 Boqun Feng <boqun.feng@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>,
 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
 Lai Jiangshan <jiangshanlai@gmail.com>, Zqiang <qiang.zhang1211@gmail.com>,
 linux-arm-kernel@lists.infradead.org, rcu@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240307160951.3607374-1-stefan.wiehler@nokia.com>
 <bce79497-52c5-4241-aaf6-2a95dc459041@joelfernandes.org>
Content-Language: en-US
From: Stefan Wiehler <stefan.wiehler@nokia.com>
Organization: Nokia
In-Reply-To: <bce79497-52c5-4241-aaf6-2a95dc459041@joelfernandes.org>
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?dXhQWm1RYmVHQkcvRi9laTBsbStPZ1BrTUluQ0JsUm9hY3lDd3B0ZDZLNnhT?=
 =?utf-8?B?OFVaVUJ3eEV3YTJFVW0wS3J5ZkYrUGRYT1JtcHo4MElnSFFDbEVMZGVML2hz?=
 =?utf-8?B?S0l0Y0JJY3RDbFlMK2l1bzlwMnpmSGw4bnI1cGxhYkkwSXd1ZjdnL3VxRjdo?=
 =?utf-8?B?SW14K3p5cW9tUlZURmkxeHNTa29JWXRNb2FScFlxbTlyYWVpejBRSkR1VGRn?=
 =?utf-8?B?dHY0dmZENFVHdGNzNkc2MHpRT05HejNlRzhFSkl6QzNUNG5DMk84V0VtMnc2?=
 =?utf-8?B?NTFWK1lQMkxORzVNR2I2ZFlVN3E0eDRTbTl0djhRV3BkRVY4QlZSbGVXTHNH?=
 =?utf-8?B?cFFoY1dJdlNSNXVGZzlseXJhZytsdDRLQllLb1RITXkyRHNjbUJGR294Umg1?=
 =?utf-8?B?WGdvdHoxTU5CYU5CUmhsckNTTmVOZVVRdjZnNFVJelRXaHdLekMyamt0a1cy?=
 =?utf-8?B?Snk4algvdDFtY1pSNm1VVlZmOThkei8rYjl6eHhZQkR2dzE0WXNaTm95cldH?=
 =?utf-8?B?THhTMmh0ZmVCTUE0ZEFPUmZ5WWpjdTk5YzQwa0lFVUxURWhCVWQrRER1NGZ0?=
 =?utf-8?B?RFVHWmQ4RzBlZGtpOURnRG9yVVNjNHpmZllJamIzTVFLT2tkTjZhL3NzQW9Z?=
 =?utf-8?B?aHcvdnBGNWQrdEVRS0UzOFZSaWdzQ3pnazJDMXVGb0hlUG91aVhKVXNmVVQ3?=
 =?utf-8?B?YnlvWG9PKzk0aHUrZ3ZQZzdWVUx4VHE3U2dkTmZRV0tOWVhGRU80aFkra3Ra?=
 =?utf-8?B?TXFoVHk5anpYWWlwdUo1NGpHdVg3bXJ3cUdtcXRHRXo2eGU5QXdWZ2MzNHdR?=
 =?utf-8?B?OGU5a2h0QmdKaE1xMmxWSG5lVVFPalRSVXhPQThDRXVWK1BLVlBMcExJNnM1?=
 =?utf-8?B?bEhPMURud2N6MjgrdWw0MUZsWWlUT0VaZC82elk1TXpwRkRZUmw4ZVNFZVpm?=
 =?utf-8?B?VVRKU0ZqZUFCQi9VTTN3YlFyVFljNndNZmp1WTRhK3NnbzZTYTc5TXY0bkZu?=
 =?utf-8?B?cGF2cFI0STJ2L25Demc0aGxERWJiOWZ2cmp4WHZKa2pPcVNKWVEydE1Xd0pv?=
 =?utf-8?B?ZEI2eFNRbmRwa0pMajlLWUJPblNsUW5TeGNoM1UwZ09iTUE0QnY1NE9uSWxV?=
 =?utf-8?B?YWhjbXN5V0xORHVFL1BqZkxEbjZ2eXJ5ZDhISFdVOVJJZjIyR3NibnpRQ2hW?=
 =?utf-8?B?RldXVlNyRDBpTXFEUjJHNlNTSmorQ2NGemFxeXN1UWdZc1hud2EyN3lkbHpm?=
 =?utf-8?B?WVBOeDVPdUVlamdkcnlGUkhHNUlZY1N0YU43VUJCT3ZDQjZOQTJkSVN5bFps?=
 =?utf-8?B?MUFTdE52WkcvSXNFc0tUTS85MDlUMkxnYVl0bVhNTVZqRlpnaTdWaDVGbEFw?=
 =?utf-8?B?RlFqczhBWjRxZDd1VHVibHovek1GcS9BK3VtZUxyQnUyb1g1NU9NRmc5Y0xz?=
 =?utf-8?B?ck9pRllxSi92cHZaN1Y5d0Q4S1I5MW1nR2ZRcTRlYkFzS2R2L29RSVpJbm9w?=
 =?utf-8?B?bklLakQ2L3R2alMyMTUvampvTW05WVdJTEJFS1hSZjIxZkYvenhTRjU0bFdw?=
 =?utf-8?B?K2xielBkTjlzN3dTbFQ3WHRTWkI3d3QwQUJ4WDQremUxVEtKZFV2aEcvbDl5?=
 =?utf-8?B?MGt1UEFLVjIra2lEaXQwWlA3RnRhNFU5dUdJeFZqN3N4cEhacWhnUlJvRGxx?=
 =?utf-8?B?bUp2cGFxVll3WXY5Wk5pUnV1RlJHWjZuOUhZdCtienIwK3U3RCtuVmZjakpK?=
 =?utf-8?B?UVFWQTlvYkNjcWl5bjhlUkloWUdtS3MxS25RdmdNblU4MjhIVVZWNlNqQThV?=
 =?utf-8?B?MThrem02OVdVYzc0SWdEMFg1bnVJQXNCYzVjK096akUwUHQ1TFdRM0NkUkIy?=
 =?utf-8?B?TU0vbUx0eFZkbTBtaU82Tyt0ZWYyekJmVTZCSTlsUUF2OWhIZTBXeUY0NEJs?=
 =?utf-8?B?cDIxYlFLUXBIVzhEZHRwUllWVFZxUlVsQ3dvK3p5N1d2dlZWcFhqa3kzdm1h?=
 =?utf-8?B?NEowQVFiMHVZc08rZDFacUd1UzJzZ3E5YU1KdUNlL3oyMnJnUGRSSS8wNUhI?=
 =?utf-8?B?aGF3MThzREg4ZzRnVXlCVkY5cUZUQkM3cnE4RlZEUGhla0FHUTd3enRISWFs?=
 =?utf-8?B?RU9Pazh4TXlZWFhybnViTWhVdU12ek9CRXRPZm5hUmMwRmdraktaVlpCMEZ0?=
 =?utf-8?B?WVFBS0tFdkJLTVVZUDV1U2R6cThzTDg2WmtHTUQzNnh0ZU9jWURVTzVxdnNX?=
 =?utf-8?B?bmx3SXRlUm42UnVSRHpjaHpVbm13PT0=?=
X-OriginatorOrg: nokia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 35f590dc-ac5a-4e83-ff87-08dc400ce90d
X-MS-Exchange-CrossTenant-AuthSource: PAWPR07MB9688.eurprd07.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Mar 2024 07:45:37.4633
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5d471751-9675-428d-917b-70f44f9630b0
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: W7ZpP+NTMbC+6bHqRSTVMjE0aGYoZ3EypEAG9Eh3Rdk30iXsBL2HrAZHitNqHgvvOl0Zb0CynwtoEvD4imTNQMggjOR3lJ0wiREVuIwQD3Y=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: GV1PR07MB8997

> I agree with the problem but disagree with the patch because it feels like a
> terrible workaround.
> 
> Can we just use arch_spin_lock() for the cpu_asid_lock? This might require
> acquiring the raw_lock within the raw_spinlock_t, but there is precedent:
> 
> arch/powerpc/kvm/book3s_hv_rm_mmu.c:245:
> arch_spin_lock(&kvm->mmu_lock.rlock.raw_lock);
> 
> IMO, lockdep tracking of this lock is not necessary or possible considering the
> hotplug situation.
> 
> Or is there a reason you need lockdep working for the cpu_asid_lock?

I was not aware of this possibility to bypass lockdep tracing, but this seems
to work and indeed looks like less of a workaround:

diff --git a/arch/arm/mm/context.c b/arch/arm/mm/context.c
index 4204ffa2d104..4fc2c559f1b6 100644
--- a/arch/arm/mm/context.c
+++ b/arch/arm/mm/context.c
@@ -254,7 +254,8 @@ void check_and_switch_context(struct mm_struct *mm, struct task_struct *tsk)
            && atomic64_xchg(&per_cpu(active_asids, cpu), asid))
                goto switch_mm_fastpath;

-       raw_spin_lock_irqsave(&cpu_asid_lock, flags);
+       local_irq_save(flags);
+       arch_spin_lock(&cpu_asid_lock.raw_lock);
        /* Check that our ASID belongs to the current generation. */
        asid = atomic64_read(&mm->context.id);
        if ((asid ^ atomic64_read(&asid_generation)) >> ASID_BITS) {
@@ -269,7 +270,8 @@ void check_and_switch_context(struct mm_struct *mm, struct task_struct *tsk)

        atomic64_set(&per_cpu(active_asids, cpu), asid);
        cpumask_set_cpu(cpu, mm_cpumask(mm));
-       raw_spin_unlock_irqrestore(&cpu_asid_lock, flags);
+       arch_spin_unlock(&cpu_asid_lock.raw_lock);
+       local_irq_restore(flags);

 switch_mm_fastpath:
        cpu_switch_mm(mm->pgd, mm);

@Russell, what do you think?