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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id A67CEE77199 for ; Mon, 6 Jan 2025 13:05:25 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 25EBD6B008C; Mon, 6 Jan 2025 08:05:25 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 20E066B0092; Mon, 6 Jan 2025 08:05:25 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0AF036B0093; Mon, 6 Jan 2025 08:05:25 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id E014D6B008C for ; Mon, 6 Jan 2025 08:05:24 -0500 (EST) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 78363814F4 for ; Mon, 6 Jan 2025 13:05:24 +0000 (UTC) X-FDA: 82977048168.24.462FE39 Received: from mail-ed1-f49.google.com (mail-ed1-f49.google.com [209.85.208.49]) by imf14.hostedemail.com (Postfix) with ESMTP id 68863100021 for ; Mon, 6 Jan 2025 13:05:22 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=b3kMBm2m; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf14.hostedemail.com: domain of jannh@google.com designates 209.85.208.49 as permitted sender) smtp.mailfrom=jannh@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736168722; a=rsa-sha256; cv=none; b=aJHk++d06+BQN35KjRx6K+5QcPKpUYgfhJ4PWCP5Bc8dY58xxbmutTfbK+HfgSTo5VwyRw mOobNH7nvFU0KDT+gClQlT5HRDsCfpSjkNrO93L8u3klVaErd6346ufnzMR8isuROfSPIU rFNPVagZ/R7EmkHfn4I4+6pc5UhxK8o= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=b3kMBm2m; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf14.hostedemail.com: domain of jannh@google.com designates 209.85.208.49 as permitted sender) smtp.mailfrom=jannh@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1736168722; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=GApFg0VCzIIFzCNKW7Q5q7CNyuJPqlur9R5LeNMN3uQ=; b=3KJO08uKK/+59Usfwm4zR5ISdRMx6ShlCawxlSco5OXRwrT7ciE4RDfcRiKF1pRHtqMSL+ 1IUdtDSLE19lJAhsw3TLN7CzCDosVXek6tHtUdkrg4wh7qFiEmyaFtOikVQ5R+Lv+U46Of eS39uWpmRN+k65L/DKWoJPraOCmwyCY= Received: by mail-ed1-f49.google.com with SMTP id 4fb4d7f45d1cf-5d3e638e1b4so9309a12.1 for ; Mon, 06 Jan 2025 05:05:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1736168721; x=1736773521; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=GApFg0VCzIIFzCNKW7Q5q7CNyuJPqlur9R5LeNMN3uQ=; b=b3kMBm2m6/Ef6swLwDdCS/ygnFo8kkS6P2p11Wz3RXOMZLuaLzzFIvJ/Cp/pkYRWjy gn2xRiVr//wMTGN00T5NR42Iwc8DFVYSbJl1TRrcpUVJ1ypioOYfohyY3rDCxg5wH9F+ /v55kZtg1XRlJ4qdS3awnvTK/23laY5o3r3L7R3dfReWfE4kPOfvGBM9mI/kgAGwXsya PeDQ0f7L2rgj2iSc/ebR/E9C9uZswrFn9FxAN+88kf2JvkXryXSX2Y5tYJJglAEphoGv 32ncM637GxsZPjXWVDxds46nsukTVR+rHBOtAkh6QgQ3Apgp7fPIHvzzhsTIKMfCJIxj VIMw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736168721; x=1736773521; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=GApFg0VCzIIFzCNKW7Q5q7CNyuJPqlur9R5LeNMN3uQ=; b=FyMdZtuJqZWyq/Y30ro/FSxDfTcsJIk0G3ljgFf7CBnhY4C81MpE18RFDV5EcCXJmM xyArRQDajZLEIiDzeKwGe3EFWOCCtoLO8jFKgEDDeHQlXNKiQg0WVeQrEEFc2qEWG2Z6 m7t2jrMydfZVLQOxWRVyuo0tgoIFKVVl+tNHSbFLzKVBFeES1NFItyF2xYhWIhvmikgl Eyh8lyxK1cwczd+UOGAz7r0svjEKKgD2ALkrWY3qN7AVCl06WSJY+C8KNNPULm9aFoai jH/Bi0tphGI5kHm3rFVP7Ob5tPXOptUbVDwsFWnX/yBIAoEe2WRwWmRJZvJ3fK0UMj/M YPUA== X-Forwarded-Encrypted: i=1; AJvYcCW+30dAUH1z4w9IKwU0alO9Xw9L+l47NwnFMG+omtycXHB/FoZUfRFiEF9jSn/CNPVg/9yEkImEKQ==@kvack.org X-Gm-Message-State: AOJu0YyvyLu6eng9fGlOa/9jKJlOlc0vUzsJkO3aWOF/SbKFaoIP4yzN Mbmqj2momW16yopxe+0iavpDIJ+p+X0ZRgWAye1PHLRibTTeQTT7xr9DXcvgTu5EALziFw8WTzK SWGHOoos0nnnyKEdl52cePIrPsPmKUQvkipZp X-Gm-Gg: ASbGncu0dHw4TrmTdEeiGhwrjfwB4Ou0KUgUFgc/UgMP8jV+GNihLmBEb6CsBuzU4T5 zWYeD4CuGqs6eg0bLttMOxpmqiorpGJPN5SKLAmNGOqQXf3XFXcvxAUtxFk2ERLGEBA== X-Google-Smtp-Source: AGHT+IHV0qk7Cf+hgP7s9eyPAKn7YTwZgD2qPEMy8sBmKyTB+jYyzSA09RtjoDJ1dBrhpwDyiU+UkwSC/pCPIeRCCHY= X-Received: by 2002:a50:8e08:0:b0:5d0:d7ca:7bf4 with SMTP id 4fb4d7f45d1cf-5d950668ec3mr46743a12.0.1736168720319; Mon, 06 Jan 2025 05:05:20 -0800 (PST) MIME-Version: 1.0 References: <20241230175550.4046587-1-riel@surriel.com> <20241230175550.4046587-10-riel@surriel.com> <96d1b007b3b88c43feac58d2646d675b94daf1fb.camel@surriel.com> In-Reply-To: <96d1b007b3b88c43feac58d2646d675b94daf1fb.camel@surriel.com> From: Jann Horn Date: Mon, 6 Jan 2025 14:04:44 +0100 X-Gm-Features: AbW1kvbjWoM1C8I8u1jG3SOdqNEemFXW9MvQdCvebQKSctQ3R8reEL2YgdyqVNk Message-ID: Subject: Re: [PATCH 09/12] x86/mm: enable broadcast TLB invalidation for multi-threaded processes To: Rik van Riel Cc: x86@kernel.org, linux-kernel@vger.kernel.org, kernel-team@meta.com, dave.hansen@linux.intel.com, luto@kernel.org, peterz@infradead.org, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de, hpa@zytor.com, akpm@linux-foundation.org, nadav.amit@gmail.com, zhengqi.arch@bytedance.com, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 68863100021 X-Stat-Signature: rf1z8getgototjra4tcin19bbe5xodci X-Rspam-User: X-HE-Tag: 1736168722-918704 X-HE-Meta: U2FsdGVkX18GCETdb1O1wz3Dfnd6akwmvl/VkGPHtwjmKZbZYgwUMKdFvzhDFj/TqPRPMPv06incCumb5fCJWKc8SLDHPsYkYVAKztHJc6F9kqt1l28QNVud1NtXO+mnNaLrAI0fLtAPDrYKkBkGTX778QCd0wsLsK+7j65yHR9OuEe4FIAtiD8R47rHShSBsSQRSkMt9DeDEEjQXn+qLPmcDtUh95gRuyK5xyhCejb9Kl/PyBbuEQTkpGKe0Lp7BSNvb1PxGsdRAFaVVlY5yA74X8GkuZPE2ZL4e8ikuMLLG3DVOFtqpN+nvhQxMoUJcGBTmUWn0VIxef+ld9JBeHMT/SRD6PaFD6G2AYd8YFJuQh6gtQTaMui8Ee73pOuYqg3/Ma0lk51ZYBB5dR7KSD/nQuECaLtFSOKgBVZD5svHFcjbWqU+9GExpy7L9vN7zpapKrsdZP/EXbrpgBevwQdigYxlyBdNSH1tsO4VGUX6zRMtTDzJhecDnF6OhHm0kMcIVUQqcofUUpx81uLyEksVifbGlzHg6qjRKsZIfuzE/9AWAyo3k9+Z4FZvHQoYaNyVRl3o1PGTThRb7Cd4mLNRAEmritf9xxWAQwJ7b1MfsBHWXJEqXKdwXazH3625Es7WIwFloGp3Krkz6rFDmbHP9SN+21ihAkwEQuAKpm+DK3883fIooPDwnbN0bkQc6lt2AldJAvEfmaEUWru42gY1d2yZS0Q5/zFOz7eKq0beC/TAMzsGY84oxpMVouYqOPt4P/mBZ0QECGlHYoj1uSeEGoP7Qz/7Dv5RqWbdC2Mt1VNtzzXpOEnvMMei83AKSGkbhhAmCke+mnNqGne4OWShwgTkdz2ZkrjJN5o8ZIBJb7m9nZXTY1iAV+mmRmvh/5zQCgJc6jsAwtx7pyX3YgpZIwVbJPu0CpDoJAFKUhQkVmsJ3JtAbWCE3J98PW/iNK4VOIilaKWPgiBKjK9 /ryXDavX IZtxdsNJ+XMbNXEvr31HGshN903F1SFR/JGtyqw2Vx3ZrwKaYh5zRuHSiRgyfuKv109rlp172upxm+2XlgtYq7LxoV81XzzFROjR4fCFo6xans1RAuzJmIS1kpkiE+ip6Pxij3MP4gfz1CczUJvsq6jf1oIBXA2DwelmsrBopyK658VdQBLQYgOmh5UIhWklHC6uQ14LSALbJShjsWXGU0h4ysSyjL2HRvIEVLy9K8RHWIozGnlEXLrqJ1J+eJ/vDgNdqd4ZxU6AAKt0ZNbZsgbVtmYMD89kF9DG9F3L/9prHemSvLDRlRC0Q/nCSsqRGslBxNfjAmEg5enA= X-Bogosity: Ham, tests=bogofilter, spamicity=0.004140, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Sat, Jan 4, 2025 at 3:55=E2=80=AFAM Rik van Riel wrot= e: > On Fri, 2025-01-03 at 18:36 +0100, Jann Horn wrote: > > Maybe change how mm->context.asid_transition works such that it is > > immediately set on mm creation and cleared when the transition is > > done, so that you don't have to touch it here? > > > If we want to document the ordering, won't it be better > to keep both assignments close to each other (with WRITE_ONCE), > so the code stays easier to understand for future maintenance? You have a point there. I was thinking that if asid_transition is set on mm creation, we don't have to think about the ordering properties as hard; but I guess you're right that it would be more clean/future-proof to do the writes together here. > > > + return; > > > + > > > + for_each_cpu(cpu, mm_cpumask(mm)) { > > > + if (READ_ONCE(per_cpu(cpu_tlbstate.loaded_mm, cpu)) > > > !=3D mm) > > > + continue; > > > > switch_mm_irqs_off() picks an ASID and writes CR3 before writing > > loaded_mm: > > "/* Make sure we write CR3 before loaded_mm. */" > > > > Can we race with a concurrent switch_mm_irqs_off() on the other CPU > > such that the other CPU has already switched CR3 to our MM using the > > old ASID, but has not yet written loaded_mm, such that we skip it > > here? And then we'll think we finished the ASID transition, and the > > next time we do a flush, we'll wrongly omit the flush for that other > > CPU even though it's still using the old ASID? > > That is a very good question. > > I suppose we need to check against LOADED_MM_SWITCHING > too, and possibly wait to see what mm shows up on that > CPU before proceeding? > > Maybe as simple as this? > > for_each_cpu(cpu, mm_cpumask(mm)) { > while (READ_ONCE(per_cpu(cpu_tlbstate.loaded_mm, cpu) > =3D=3D LOADED_MM_SWITCHING) > cpu_relax(); > > if (READ_ONCE(per_cpu(cpu_tlbstate.loaded_mm, cpu)) !=3D > mm) > continue; > > /* > * If at least one CPU is not using the broadcast ASID > yet, > * send a TLB flush IPI. The IPI should cause > stragglers > * to transition soon. > */ > if (per_cpu(cpu_tlbstate.loaded_mm_asid, cpu) !=3D > bc_asid) { > flush_tlb_multi(mm_cpumask(info->mm), info); > return; > } > } > > Then the only change needed to switch_mm_irqs_off > would be to move the LOADED_MM_SWITCHING line to > before choose_new_asid, to fully close the window. > > Am I overlooking anything here? I think that might require having a full memory barrier in switch_mm_irqs_off to ensure that the write of LOADED_MM_SWITCHING can't be reordered after reads in choose_new_asid(). Which wouldn't be very nice; we probably should avoid adding heavy barriers to the task switch path... Hmm, but I think luckily the cpumask_set_cpu() already implies a relaxed RMW atomic, which I think on X86 is actually the same as a sequentially consistent atomic, so as long as you put the LOADED_MM_SWITCHING line before that, it might do the job? Maybe with an smp_mb__after_atomic() and/or an explainer comment. (smp_mb__after_atomic() is a no-op on x86, so maybe just a comment is the right way. Documentation/memory-barriers.txt says smp_mb__after_atomic() can be used together with atomic RMW bitop functions.) > > > + > > > + /* > > > + * If at least one CPU is not using the broadcast > > > ASID yet, > > > + * send a TLB flush IPI. The IPI should cause > > > stragglers > > > + * to transition soon. > > > + */ > > > + if (per_cpu(cpu_tlbstate.loaded_mm_asid, cpu) !=3D > > > bc_asid) { > > > > READ_ONCE()? Also, I think this needs a comment explaining that this > > can race with concurrent MM switches such that we wrongly think that > > there's a straggler (because we're not reading the loaded_mm and the > > loaded_mm_asid as one atomic combination). > > I'll add the READ_ONCE. > > Will the race still exist if we wait on > LOADED_MM_SWITCHING as proposed above? I think so, since between reading the loaded_mm and reading the loaded_mm_asid, the remote CPU might go through an entire task switch. Like: 1. We read the loaded_mm, and see that the remote CPU is currently running in our mm_struct. 2. The remote CPU does a task switch to another process with a different mm_struct. 3. We read the loaded_mm_asid, and see an ASID that does not match our broadcast ASID (because the loaded ASID is not for our mm_struct).