From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f171.google.com (mail-pl1-f171.google.com [209.85.214.171]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 56AA51D356F for ; Thu, 23 Jan 2025 22:26:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.171 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737671216; cv=none; b=kGtSlsvIDV3Li4jFXyIEF3rUIBqBKIbBHWRqWf2SjV4ARUqY7MkGfMFQUPkhS7k01TPw0gsRO83XdbpD5mWb3+B4LnZornwDujQOJkUWgohmLdRfOor6q4MYRwXefCXbdrK9JBKxKWqJPjZHXPYDrSCXDAvpH4350yikvYfqmd0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737671216; c=relaxed/simple; bh=YxkDzrDcoFpsXuNxNlivsvDgnLQCtMSMvJ79SrwZ/Uc=; h=Date:From:To:cc:Subject:In-Reply-To:Message-ID:References: MIME-Version:Content-Type; b=C12kI//eav0WaVrRI6uR6shCkIeKQpskeaizza0LvuFQosIJd9tVZPqwAi2D43W9mos2mNLFcuiXtLdELDG4azT9Hmif8i6GMvfVAulc3FGBjw+S8bI6P9/kL15ZhLUdbvr1gdlg/F/kg2n2xGGbRzZu1w1hel8mNdeDT5v+9ds= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=XaNBbxxY; arc=none smtp.client-ip=209.85.214.171 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="XaNBbxxY" Received: by mail-pl1-f171.google.com with SMTP id d9443c01a7336-218c8aca5f1so33487405ad.0 for ; Thu, 23 Jan 2025 14:26:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1737671213; x=1738276013; darn=vger.kernel.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=tft7nm1JR9P/DYe+6ctmV/lRjjdKS3eqeij/WSGrhTY=; b=XaNBbxxYC1Q3YhD80DROYj9L/VZTOzHrHXRRC51d5vrA6T4EZpNLk5YiedShkqajoQ BwLZ3GE8GMJL/AjDeie3tbwL7JuFblEhS7lAN4qmi4lqe5lvPlgKDKeUPp+FGLGuCaJz Hp+4huXkaMJ7nlmOTnYAU+1YlnbMTAfCASNBKGw54BDleEOni/waYi7Zs78kns0cgjlj Qt+XDvd25zlH8rE3dcnIR03wXvHvXZPVqt8digAkD1yBbiX1VYdi5PjPsxQ3fNUliLc+ Iw0w3J2r+ZJJLVrtIxh67ZiQnJ7UCpu9Fs7yBOD2FudwS6gkeMjyU5yVmF3U81ytZzZd Viww== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737671213; x=1738276013; 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=tft7nm1JR9P/DYe+6ctmV/lRjjdKS3eqeij/WSGrhTY=; b=exB7eES6GL/4TRAsCks7XMhSzW7dqxGE9UKL7PkHUsdnIRUU3JESAIy613BtZbuZuY PRKI/gC18BDclL+oLKQfZtQhN7NhOMqy/MsR/Ik5gA09AWYHmF3Jv8FaFahvrXqH8aPh +Z6atziPyH+g1dMPYabsn43cXVPXXAckKK97ohM2uo+Y9QohoPfwbxYiyDLgtZg6Z8Bs ZtpZbOZxHbjVrZ2ff9aTT0ulE1BjcWWj8dWXUI/yxSvfstv5hGhd0WvzaLsaZIPwHPzY k2PDySjWk1jrietW48PNfOJOYkPo5vtvIE2SqQhNEHIqSltnPXEdTpzY+8ZTGXBYZbVM MNGw== X-Gm-Message-State: AOJu0YyFm+ilALEHcpf2toU6BmfRR3izWbyzzHJlvFzbYvIXs+cQznpK Iz7/R6W4AwbxC1wa7qTDEF3hmtQruujGM42cHLE7jXE99jMhxFJCuIRa8HAobg== X-Gm-Gg: ASbGncsgouWqCQquTU4+2ZoKZEVH700SlK3gJAF59OLXwltHOqrmnsNe1mS/tkb4L4F SOmu9xfnO5XDJbNMh1y297kzb59dhy3clpE7j+dTwk+5kgIFXB6SLP0Bp/JuDRiE+VxyHRivUyy 2YzdUIuT/8kD8JkOUzfPEtYCmDI5dJYe7+VXgq7xlFweKOkCR2Ja8mfecfH9F+s6z9YO3Z51eJE InPuqz7CM15l+U/Fq/DDdB0STa6mO1pgO5O1lEjvG5qIf5VkOOr8/5/lVPPHJqgw0N1PLQBnoZk eK6MkOLM5bbUt3cBS76ea0QuQNvLVY7effCvi7UWO5xloxHIwKWrJVY1F4beQm1G3/SSUxPHYj4 = X-Google-Smtp-Source: AGHT+IFdr/JY0zYSzFQXqKrxePDHm7GTPzdct2mHp7qiWEvNNUEv0l0Rhkvb8+8S4Eb6YB26iHiPTg== X-Received: by 2002:a05:6a20:7f96:b0:1d9:c78f:4207 with SMTP id adf61e73a8af0-1eb2147d3f9mr41098399637.11.1737671213391; Thu, 23 Jan 2025 14:26:53 -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 d2e1a72fcca58-72f8a6d22e4sm471624b3a.83.2025.01.23.14.26.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Jan 2025 14:26:52 -0800 (PST) Date: Thu, 23 Jan 2025 14:26:42 -0800 (PST) From: Hugh Dickins To: Roman Gushchin cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Andrew Morton , Jann Horn , Peter Zijlstra , Will Deacon , "Aneesh Kumar K.V" , Nick Piggin , Hugh Dickins , 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: <20250123164358.2384447-1-roman.gushchin@linux.dev> Message-ID: <32128611-60d5-147c-7e82-7b1dfbe8236b@google.com> References: <20250123164358.2384447-1-roman.gushchin@linux.dev> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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(-) 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). 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). Arrgh. This is not what you wnat to hear, to get your slowdown fix in. Simplest just to ignore the existing range deficiency, I suppose. But where to reset vma_pfn? I'm torn, have to let you and others decide. Hugh > + */ > + tlb->vma_pfn = 0; > /* > - * Do not reset mmu_gather::vma_* fields here, we do not > + * Do not reset other mmu_gather::vma_* fields here, we do not > * call into tlb_start_vma() again to set them if there is an > * intermediate flush. > */ > @@ -449,7 +457,14 @@ tlb_update_vma_flags(struct mmu_gather *tlb, struct vm_area_struct *vma) > */ > tlb->vma_huge = is_vm_hugetlb_page(vma); > tlb->vma_exec = !!(vma->vm_flags & VM_EXEC); > - tlb->vma_pfn = !!(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)); > + > + /* > + * vma_pfn is checked and cleared by tlb_flush_mmu_pfnmap() > + * for a set of vma's, so it should be set if at least one vma > + * has VM_PFNMAP or VM_MIXEDMAP flags set. > + */ > + if (vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) > + tlb->vma_pfn = 1; > } > > static inline void tlb_flush_mmu_tlbonly(struct mmu_gather *tlb) > @@ -466,6 +481,20 @@ static inline void tlb_flush_mmu_tlbonly(struct mmu_gather *tlb) > __tlb_reset_range(tlb); > } > > +static inline void tlb_flush_mmu_pfnmap(struct mmu_gather *tlb) > +{ > + /* > + * VM_PFNMAP and VM_MIXEDMAP maps are fragile because the core mm > + * doesn't track the page mapcount -- there might not be page-frames > + * for these PFNs after all. Force flush TLBs for such ranges to avoid > + * munmap() vs unmap_mapping_range() races. > + * Ensure we have no stale TLB entries by the time this mapping is > + * removed from the rmap. > + */ > + if (unlikely(!tlb->fullmm && tlb->vma_pfn)) > + tlb_flush_mmu_tlbonly(tlb); > +} > + > static inline void tlb_remove_page_size(struct mmu_gather *tlb, > struct page *page, int page_size) > { > @@ -549,22 +578,14 @@ static inline void tlb_start_vma(struct mmu_gather *tlb, struct vm_area_struct * > > static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vma) > { > - if (tlb->fullmm) > + if (tlb->fullmm || IS_ENABLED(CONFIG_MMU_GATHER_MERGE_VMAS)) > return; > > /* > - * VM_PFNMAP is more fragile because the core mm will not track the > - * page mapcount -- there might not be page-frames for these PFNs after > - * all. Force flush TLBs for such ranges to avoid munmap() vs > - * unmap_mapping_range() races. > + * Do a TLB flush and reset the range at VMA boundaries; this avoids > + * the ranges growing with the unused space between consecutive VMAs. > */ > - if (tlb->vma_pfn || !IS_ENABLED(CONFIG_MMU_GATHER_MERGE_VMAS)) { > - /* > - * Do a TLB flush and reset the range at VMA boundaries; this avoids > - * the ranges growing with the unused space between consecutive VMAs. > - */ > - tlb_flush_mmu_tlbonly(tlb); > - } > + tlb_flush_mmu_tlbonly(tlb); > } > > /* > diff --git a/mm/memory.c b/mm/memory.c > index 398c031be9ba..c2a9effb2e32 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -365,6 +365,13 @@ void free_pgtables(struct mmu_gather *tlb, struct ma_state *mas, > { > struct unlink_vma_file_batch vb; > > + /* > + * VM_PFNMAP and VM_MIXEDMAP maps require a special handling here: > + * force flush TLBs for such ranges to avoid munmap() vs > + * unmap_mapping_range() races. > + */ > + tlb_flush_mmu_pfnmap(tlb); > + > do { > unsigned long addr = vma->vm_start; > struct vm_area_struct *next; > -- > 2.48.1.262.g85cc9f2d1e-goog