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 EA8EAC0218D for ; Mon, 27 Jan 2025 01:31:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 21246280114; Sun, 26 Jan 2025 20:31:41 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 1C38A280112; Sun, 26 Jan 2025 20:31:41 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 062A3280114; Sun, 26 Jan 2025 20:31:40 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id DD38F280112 for ; Sun, 26 Jan 2025 20:31:40 -0500 (EST) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 45EABC1188 for ; Mon, 27 Jan 2025 01:31:40 +0000 (UTC) X-FDA: 83051504760.06.EE5E6C4 Received: from mail-pl1-f171.google.com (mail-pl1-f171.google.com [209.85.214.171]) by imf26.hostedemail.com (Postfix) with ESMTP id 66900140002 for ; Mon, 27 Jan 2025 01:31:38 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=NJFUkHSg; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf26.hostedemail.com: domain of hughd@google.com designates 209.85.214.171 as permitted sender) smtp.mailfrom=hughd@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1737941498; a=rsa-sha256; cv=none; b=D+V8DyoFhxIGF6n5yX7gu/fgS2HfQ2pzuQ0GV6tFcTBZAUb2TgSNeymMUFopjzwYbzzs9+ BEcCqupeAS9oEK442aMWCuIeax6xSV/I5yyBZCbzSg/3lKdRxNjeTRpRgVb2tpOS43GLIF 2QWHaNxCAcFuVPHOgEQF834NF8XZJ6c= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=NJFUkHSg; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf26.hostedemail.com: domain of hughd@google.com designates 209.85.214.171 as permitted sender) smtp.mailfrom=hughd@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1737941498; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=POceRtMWVWLLwpVXZyFlwaFV2xC0SiBcMp9OIByqfZ8=; b=H/VpTqw0ffkD6kwJWiVYghTNfzej3lwBB/UhCnf8ZquNvbiqe8QmMh+odxRq2C5soDc0lE qgfmEfcHibpZefuwgZB2wkLfNr+LjVj9YSRFFqIevhps8tOB+X+ztjUp6KFgg9lhDQMMng b92F9NUBaYC8HI1AJQ3Mt+yKoMjrZm0= Received: by mail-pl1-f171.google.com with SMTP id d9443c01a7336-2166651f752so75197235ad.3 for ; Sun, 26 Jan 2025 17:31:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1737941497; x=1738546297; darn=kvack.org; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:from:to:cc:subject:date:message-id:reply-to; bh=POceRtMWVWLLwpVXZyFlwaFV2xC0SiBcMp9OIByqfZ8=; b=NJFUkHSgwi79mUiF0fsNPyDuutvM0EISREmlln1EhH/f5qWeJU3QdMVOjInkmSmdum 9E2A/AL3IjeJ26mokppKX7eGIfYg99boqaAsjFGO+510k2EfxqWKGFoo3GD8G8m3LneP Fkjl3yGF8I+F3aSzDXzlewoBqyYnC6txkQQIzWxE+pdANbgW6/Ts8jNsRDF71S9w6dyc 4Jr9MyMr3Gy8LRs1L3EpCXYltLEv1ycdyf+yxgMPCYiLGUzgY1czkAMyNXFswMFHG1d8 xCEcAgTwlUBUfsy5epEEjuIW4tLAhqwHZiPx7frMr/NDuZ/LlFf2v0OVfsHlmxbwE8Am 5jdw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737941497; x=1738546297; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=POceRtMWVWLLwpVXZyFlwaFV2xC0SiBcMp9OIByqfZ8=; b=hUoCcwVxappFqbuG6RMvj+vlhAniJg1VnC/LwF142uw922TtIB1CoJEdaTvmSJMOpU yyNv6M5wuShu2aeUCdkPFgNc+MzKjQWu8Gb4Mb8tOzUJWT2phPxiFGHQIqnQKbKU4w4r zUxzedr4t42KUXFij5FLkLdGl2ZyGg8nYReAHaZvawgmRn1Iiw3Xj/pbQXxtnaY5SPuT GD0l9ICe0uqL4iYlzzW76vRKJ3KdJK+MG2AQ81cCpzZHJLmjZlyI31FVfoqEF9nVhEpb DF66S+ZAMU5BeQ0ObrRZq9ryGglBalrWWWT5Mq7Cp3Yzg88fUhyOSeXIAKwTiqfV5F+w VQeg== X-Forwarded-Encrypted: i=1; AJvYcCX7z6oQr3qWBURrcAljUwBK4mSYn00kiqm/t7m8Dfj6X4DdyWWo8ojBpoG7djczKhB0jgZldWMIPg==@kvack.org X-Gm-Message-State: AOJu0Yw+mO40Ix0zq5cPHK9l0NNLTLa8RV4OsjK0Xy/i+lFMWTWOsfXc z7JNPQpSEDZ2AAqQli/6/Du0Ey2rcI87EIk4bGpY/2XMaV9k/sAc9zHlJdPU6w== X-Gm-Gg: ASbGncvmaHqtu34ITLqywKyrao/VuIudpMgVPiTN5HOXac7Oq2/Tr7AHyysalv4Hzz8 Q/Eq/DAVdJ1l7B7KkUMJVvfxcJfBQFQzNoFeBTmUk7GSe7YOHOVaCGsHf3moPSFsWlScowjhXQ/ 7/us+wLvm4CiyIg2vLFTZVGx5AW6Lm6pqYWA/03DxD7Zw1+vXetJsR0S3WNANdzJ61LH4eKqN3/ ozokRjpz7/8pzm4ZvaF4MnrtvmVGo7PtMHwQgX7qcZiawfT58HIKX0DK0F5ZUHhqv/glR8QPjBi B9skabyd//4Ec1Me0Gui5sYZmwXkYRlPthB92Seoqs7NrMIKsYzH/iVdB0L9BaLR X-Google-Smtp-Source: AGHT+IF3nXHvDnjkTs9Lalq7+vB5ht9+80y0rjngMtsBffGQq/s/lUsmT7Sh1KnuAANnuh5JXv4mvw== X-Received: by 2002:a05:6a20:7f96:b0:1d9:c78f:4207 with SMTP id adf61e73a8af0-1eb2147d3f9mr58967025637.11.1737941496837; Sun, 26 Jan 2025 17:31:36 -0800 (PST) Received: from darker.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-ac496cb2106sm5282233a12.67.2025.01.26.17.31.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 26 Jan 2025 17:31:35 -0800 (PST) Date: Sun, 26 Jan 2025 17:31:25 -0800 (PST) From: Hugh Dickins To: Roman Gushchin cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Hugh Dickins , Andrew Morton , Jann Horn , Peter Zijlstra , Will Deacon , "Aneesh Kumar K.V" , Nick Piggin , linux-arch@vger.kernel.org Subject: Re: [PATCH v3] mmu_gather: move tlb flush for VM_PFNMAP/VM_MIXEDMAP vmas into free_pgtables() In-Reply-To: <32128611-60d5-147c-7e82-7b1dfbe8236b@google.com> Message-ID: <1e6330a1-c671-c8d0-7eab-e6b9fc7a9d2a@google.com> References: <20250123164358.2384447-1-roman.gushchin@linux.dev> <32128611-60d5-147c-7e82-7b1dfbe8236b@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Rspam-User: X-Rspamd-Queue-Id: 66900140002 X-Rspamd-Server: rspam10 X-Stat-Signature: xd36ft9gw5oonorn37nj7kd7oo9wmsy6 X-HE-Tag: 1737941498-898944 X-HE-Meta: U2FsdGVkX1/BFAQQhMsgT/301B87DntTXPsvAs7L8xO6UMuQmQs6RC4zngf5Q+gNBQsoZbm3Dg1P+WD/JUKSf84auS3+flJgoT7pdLfM1khl3PL5qjlTOwc22iaPr6C5ZPkh8yIuUSBuolXDhuLyJwPMehTdk6FOgSl81hUBCyNbP9VA8x9wrgTyWi7DJZI0PTeuY1odbrfqW/D8G59xIkUAJ+qpF0fXqLOAPlTmmamt8G0W2y8WaLTMZ2D/6ekjcG/71+YGMiimNvdsMHiXms+fJhIqT+Wm6kRMfwF2Nr54e+4fveElrNLrztDejqVIEEK6/LrUETcAke/F6cSuaKa5DG0e0alxK9I/Rv9y1V7yqwyVYE9QzIZ3Nos25ZkBOGQThpUseCtDWEwVtYF6FLwPiT4PbJM8UdUSUR+086nNu/BfJAGxhKF6moAKP4QVrN6CKdcb5bilQxNMfnWA6TEkXUjP+FtT+8GVv2MIcHb+JmunvBaJwDAEVbHp+2Fq+6Xf6Y7uBs4/bmga1N5nYf8ysUftQWjLhyXjUE6NJDgQPjtPCYjv1gIdaQgylWBnxJRiKy5NO/FktEv2Nn3KYLp+Ae5SUkuqV52YWXjMVLl0tE7vFsfO7GF8e5KkrHmcP7JO0lyWpeGt4IXVELhk6Xrjky6CtIFG3bzW1a6NyToCw7pby1nqYnZthx6lO7LkkQ2ZqYMNDw989HSrgb1xmQsDh3DkJncXBNfySYaIYTZNWDAywtwJ0T43Ocy+eeuRcOIdDqNosdA5SB3fNDN5Ul6aDg5+Tjd9+kQfskxCozG0+nZqbhvK7Ide2BpfOZR38Zdv+pW3i7zxMgH4lNlBGZdZ97iFAaOiPZmftf94DxHoy2wyFsL+Zcao4P/cv0QPoCaM6RCTfPeqH908NTr/v6Epd773o+MWVbRJASuOVaJQrxruGhsoGiqjJwFRlwx8BK+UOmBBe5bo/Z3Yzp6 zj7yozmi UPsiEvxKqkRFVHbmS8v8dCb4OX+WG7VfDamDx0JAm+4y8LSDwP+9o8iZCHKTBXuXNP+lej7S3W6pNbfUdwF4ZuQZowH7Ya37eyXE9fFzM1k3/BKlL8lf8FO76uYiZDB+dp4ox0k6geRHLQNk93p122CQi8N2J1dro+i8N9vWBl+9iMG+utrE0cJb7lRFFiZ27X8kZ8iy3VStXbBuct9InN9cMyGty0zvGaFq54wl+1rmGdgzztdDjxOTWYGXHquc2tNPZNLVb8cKgYcrWNGiVl+urIrTNs5fRdsjdpR9pdXq8j0Rm7THRZzX3Y8/lSrghKhBH6ActCVDWpIGdMshvBCdlag+oqvputewslO4RvQjr4jy8ZsWFURCwGkODQP7m6AiZ/P//f0HCU43uKs5MzrFuwDFT2RQhOKRi9dCaciURbNCb4P1vYyA0WOSMQ2WBAqPgsI7zjhxMnZAxYJVxhDW+IvxO2ZNisG6WOMp8UjgBu6vKki/ndiUGihOmrTh5sLKeYEvVufLvjS2Sohkdo0AMeexk2PAHNigMVGZUM4dOfD99sGWx3bO9uQvZ8RF/z77zemXm3jRY9K2a8dot8o3iHzFpWIqjeZrOWcHJVJtA9Wu4tED0GfPivQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, 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 Thu, 23 Jan 2025, Hugh Dickins wrote: > On Thu, 23 Jan 2025, Roman Gushchin wrote: > > > Commit b67fbebd4cf9 ("mmu_gather: Force tlb-flush VM_PFNMAP vmas") > > added a forced tlbflush to tlb_vma_end(), which is required to avoid a > > race between munmap() and unmap_mapping_range(). However it added some > > overhead to other paths where tlb_vma_end() is used, but vmas are not > > removed, e.g. madvise(MADV_DONTNEED). > > > > Fix this by moving the tlb flush out of tlb_end_vma() into > > free_pgtables(), somewhat similar to the stable version of the > > original commit: e.g. stable commit 895428ee124a ("mm: Force TLB flush > > for PFNMAP mappings before unlink_file_vma()"). > > > > Note, that if tlb->fullmm is set, no flush is required, as the whole > > mm is about to be destroyed. > > > > --- > > As Liam said, thanks. > > > > > v3: > > - added initialization of vma_pfn in __tlb_reset_range() (by Hugh D.) > > > > v2: > > - moved vma_pfn flag handling into tlb.h (by Peter Z.) > > - added comments (by Peter Z.) > > - fixed the vma_pfn flag setting (by Hugh D.) > > > > Suggested-by: Jann Horn > > Signed-off-by: Roman Gushchin > > Cc: Peter Zijlstra > > Cc: Will Deacon > > Cc: "Aneesh Kumar K.V" > > Cc: Andrew Morton > > Cc: Nick Piggin > > Cc: Hugh Dickins > > Cc: linux-arch@vger.kernel.org > > Cc: linux-mm@kvack.org > > --- > > include/asm-generic/tlb.h | 49 ++++++++++++++++++++++++++++----------- > > mm/memory.c | 7 ++++++ > > 2 files changed, 42 insertions(+), 14 deletions(-) > I had quite a wobble on Friday, couldn't be sure of anything at all. But I've now spent longer, quietly thinking about this (v3) patch, and the various races; and with Jann's help, I do feel much more confident about it all today. > The code looks right to me now, but not the comments (I usually > prefer no comment to a wrong or difficult to get right comment). Yes, the code does look right to me now. And although I can still quibble about the comments, I'd better not waste your time with that. Let me say Acked-by: Hugh Dickins while recognizing that this may not be the patch which goes into the tree, since Peter has other ideas on the naming and wording. > > Except when I try to write a simple enough correct comment, > I find the code has to be changed, and that then suggests > further changes... Sigh. > > (We could also go down a path of saying that all of the vma_pfn stuff > would be better under #fidef CONFIG_MMU_GATHER_MERGE_VMAS; but I > think we shall only confuse ourselves that way - it shouldn't be > enough to matter, so long as it does not add any extra TLB flushes.) > > > > > diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h > > index 709830274b75..cdc95b69b91d 100644 > > --- a/include/asm-generic/tlb.h > > +++ b/include/asm-generic/tlb.h > > @@ -380,8 +380,16 @@ static inline void __tlb_reset_range(struct mmu_gather *tlb) > > tlb->cleared_pmds = 0; > > tlb->cleared_puds = 0; > > tlb->cleared_p4ds = 0; > > + > > + /* > > + * vma_pfn can only be set in tlb_start_vma(), so let's > > + * initialize it here. Also a tlb flush issued by > > + * tlb_flush_mmu_pfnmap() will cancel the vma_pfn state, > > + * so that unnecessary subsequent flushes are avoided. > > No, that misses the point (or misses half of the point): the > tlb_flush_mmu_pfnmap() itself won't need to flush if for other reasons > we've done a TLB flush since the last VM_PFNMAP|VM_MIXEDMAP vma. > > What I want to write is: > * vma_pfn can only be set in tlb_start_vma(), so initialize it here. > * And then any call to tlb_flush_mmu_tlbonly() will reset it again, > * so that unnecessary subsequent flushes are avoided. > > But once I look at tlb_flush_mmu_tlbonly(), I'm reminded that actually > it does nothing, if none of cleared_ptes etc. is set: so would not reset > vma_pfn in that case; which is okay-ish, but makes writing the comment hard. > > So maybe tlb_flush_mmu_tlbonly() should do an explicit "tlb->vma_pfn = 0" > before returning early; but that then raises the question of whether it > would not be better just to initialize vma_pfn to 0 in __tlb_gather_mmu(), > not touch it in __tlb_reset_range(), but reset it to 0 at the start of > tlb_flush_mmu_tlbonly(). > > But it also makes me realize that tlb_flush_mmu_tlbonly() avoiding > __tlb_reset_range() when nothing was cleared, is not all that good: > because if flushing a small range is better than flushing a large range, > then it would be good to reset the range even when nothing was cleared > (though it looks stupid to reset all the fields just found 0 already). My paragraph (about the existing code, independent of your patch) looks nonsense to me now: if there was nothing to be cleared, then the range would not have been updated, so would not benefit from being reset. It's still true that there would sometimes be an optimization in setting "tlb->vma_pfn = 0" in every tlb_flush_mmu_tlbonly(); but it's merely an optimization, for an unusual case, which you may find demands yet more thought than it deserves; my guess is that you will prefer not to add that change, and that's fine by me. So, if you did respin and change the comment (but I don't insist), maybe: * vma_pfn can only be set in tlb_start_vma(), so initialize it here. * And then it will be reset again after any call to tlb_flush(), * so that unnecessary subsequent flushes are avoided. Hugh