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 29366E77188 for ; Sat, 21 Dec 2024 01:11:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 537B36B0083; Fri, 20 Dec 2024 20:11:18 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 4E84A6B0088; Fri, 20 Dec 2024 20:11:18 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3AF7B6B0089; Fri, 20 Dec 2024 20:11:18 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 1D4836B0083 for ; Fri, 20 Dec 2024 20:11:18 -0500 (EST) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id BFA1B451D6 for ; Sat, 21 Dec 2024 01:11:17 +0000 (UTC) X-FDA: 82917186786.26.0EAA10D Received: from out-172.mta1.migadu.com (out-172.mta1.migadu.com [95.215.58.172]) by imf29.hostedemail.com (Postfix) with ESMTP id AA659120002 for ; Sat, 21 Dec 2024 01:10:26 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=RrggI2j1; spf=pass (imf29.hostedemail.com: domain of shakeel.butt@linux.dev designates 95.215.58.172 as permitted sender) smtp.mailfrom=shakeel.butt@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1734743439; 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=xg6jO4WG+G9lYTaoYK30yzXOhNwdRFY6Po+Vkj7w320=; b=TVcbW3D0z09D6Biv8vQeQ3sbrXW5BklrxcMLqlVtYQZM7Z94FasmlF3D7vijfr35vQpluC KsAIvUrto+A2b63sJ60wBL2cmaDDW9D0aSJoYfHkGXMIIY9A2BpYrztGF3tM07TyXFf6tA TzlojrzJ4FjewuP0PutD5LN1IeW8xIs= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=RrggI2j1; spf=pass (imf29.hostedemail.com: domain of shakeel.butt@linux.dev designates 95.215.58.172 as permitted sender) smtp.mailfrom=shakeel.butt@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1734743439; a=rsa-sha256; cv=none; b=iVR5H4Sww0paFgQcone0PjIuroa5Th9qe7cH1jRBSV+guASPxwp+A8cnn62Z1xbx7k/OrJ grs3zigafxLESjVqKUu7vbNzHnPZvJ3GvKN3u6p+1aPcYAYwU8dTnr40ZRbxW1rnnd5fzm QCepKtJwCH6DMdq0mmyaQ9EajqjrfuY= Date: Fri, 20 Dec 2024 17:10:31 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1734743473; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=xg6jO4WG+G9lYTaoYK30yzXOhNwdRFY6Po+Vkj7w320=; b=RrggI2j1Mh7kG/Kd+eOLut2t1LJnbnWCVA2C57KUjzd5aYmrrdgKne1VvYaUElH2nH4JhJ cdzNh44HsL8EhWzTLBmR+05kiZK5AONonNz/C+nMRAZWCJI5EZIz8MMR2IHi39Lgoy4CH6 POAHdHo9O04oHsk/06KSOXyL/7Wnk0I= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Shakeel Butt To: Rik van Riel Cc: David Hildenbrand , Andrew Morton , Chris Li , Ryan Roberts , "Matthew Wilcox (Oracle)" , linux-mm@kvack.org, kernel-team@meta.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm: remove unnecessary calls to lru_add_drain Message-ID: References: <20241219153253.3da9e8aa@fangorn> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20241219153253.3da9e8aa@fangorn> X-Migadu-Flow: FLOW_OUT X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: AA659120002 X-Rspam-User: X-Stat-Signature: wfmj96qxbq71focktf7y8e89in3oxa3d X-HE-Tag: 1734743426-153545 X-HE-Meta: U2FsdGVkX1+e18qj4RMv3k1XzkliH4d2UAdDYhlsAe46pNyTkoDyLhz0YHPA6mKSc7q7B5c4TjPKEoZrmQyzC+ZUfKrSBjEljnFnP6jL4JGTqJxj4ST6p3yYepv4DcR9s45YkvAwC0hutqDgma6fvsE9Y95fF9woujFsKi8pFVz0yQ1DiMharr4CCFYO9ymkw1+ZxFi5Qy+nlGYKbgzMisVrXDG9DvdGa1lpnsC2l5Gv844Mwg8kQqkG29s4zZ2aUNWw4dZsIn/5QJMl7/7Y3RTll27LFG2uWJ+vH06dO2S7EUgAmbCj4rA8WjCm2zp4Why5minK3rkwD4okAa6lHen3UyBuDDMFMBv3Yau+rxfHK7sMIAWbR2Xj+HmOl7MF5v9NimcQUTMC3W7MGYFK4zKkwDh+nt0tSJE1lr5JagtgqhAc9SMN5Pm7OBTFKTp6SUCq6b5OvXyZ9ch5ZdcK2O5w8+9JUBm35FCYte3HPG39JcyZzgMrFoLZVWZOMDHyHZ0KlcKQvUVJUrCDfU+Tr/mwS0EiRa7Oc52ZyLrEd+3AKgooJMVcS4F1yA68LmF9xrZMDIhzqzIfeeSRmdAHjz+4SCneAD+WilhCyE9/AEHqhcd1bDaShUxXT0TU4bPf6hJlKY8f5BngD91tSUj9p78SB69amCu8P4Qq4+gPlu3BCJs5COlAQmlDCO7vT61AiakL067jVa7dGFF44RQYrCwVvSr7570Ex5dhK8SimQOrg+R5hV2tqdTwxnAbILhIV2g7qT70/8GfwqV3kNur7iBIqT8uISfXEU1+x2i7RT6Efhr1iv/W+kekJ4Q8NRHAUzQLynopB9cez3s6LMOrBBXY1+m7kDmxzBjKE/gBCo6rQb7Vwz16Ko+aGex17aEDH6F3dv2iMR/IVRUq4ZAphnPNVKfkrutIrFGZZYSWm8n4fb7tWLhNhHwyuTrftH/yV5EfK2o+ZQgxwO1CjN6 +4kFWWZE uBw2Ju9RZpR8Go1otIx9rPqyd038OmrzIyrrydjo/BgLM0uZh3Zuxs7IpgTY95aPYNIZUN2niKK4SIOVihQwVqPM7CSQ7YaptaxxNX1vqKdPiK+HiBTrKBDWTzsQbifVnujwdi3HWOIMuTSsG74j+i1kHDhAqnjruoxlu8Pw4/goh1HCbFVt9xWfH5CJCVzvAxJAqsZPbWCXcDhv9pTKUKSrCJVB/3dQmBwJwCfnK/zOTTU+LnYBrjw+BPdSSC89zuY6iH0q729Q/qrHeqfimXRTdGQT9+2vvmYX3UJdovB5sksY= 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: I am trying to go beyond the first git commit to find the actual motivation for adding these lru_add_drain. On Thu, Dec 19, 2024 at 03:32:53PM -0500, Rik van Riel wrote: > There seem to be several categories of calls to lru_add_drain > and lru_add_drain_all. > > The first are code paths that recently allocated, swapped in, > or otherwise processed a batch of pages, and want them all on > the LRU. These drain pages that were recently allocated, > probably on the local CPU. > > A second category are code paths that are actively trying to > reclaim, migrate, or offline memory. These often use lru_add_drain_all, > to drain the caches on all CPUs. > > However, there also seem to be some other callers where we > aren't really doing either. They are calling lru_add_drain(), > despite operating on pages that may have been allocated > long ago, and quite possibly on different CPUs. > > Those calls are not likely to be effective at anything but > creating lock contention on the LRU locks. > > Remove the lru_add_drain calls in the latter category. > > Signed-off-by: Rik van Riel > Suggested-by: David Hildenbrand > --- > mm/memory.c | 1 - > mm/mmap.c | 2 -- > mm/swap_state.c | 1 - > mm/vma.c | 2 -- > 4 files changed, 6 deletions(-) > > diff --git a/mm/memory.c b/mm/memory.c > index 75c2dfd04f72..95ce298dc254 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -1935,7 +1935,6 @@ void zap_page_range_single(struct vm_area_struct *vma, unsigned long address, > struct mmu_notifier_range range; > struct mmu_gather tlb; > > - lru_add_drain(); The above was added in [1]. It seems like the motivation was that the lru_add_cache was holding to some freed pages for long period of time and some workload (AIM9) was suffering to go to reclaim to flush those pages and use them. By draining here, such a workload was going to reclaim less often. (I kind of extrapolate the reasoning). I think now it is ok to remove this draining as ratio of pages getting stuck in such a cache to the total RAM is drastically reduced and these pages becoming the main cause of slowdown is almost zero. [1] https://github.com/mpe/linux-fullhistory/commit/15317018be190db05f7420f27afd3d053aad48b5 > mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, > address, end); > hugetlb_zap_begin(vma, &range.start, &range.end); > diff --git a/mm/mmap.c b/mm/mmap.c > index d32b7e701058..ef57488f1020 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1660,7 +1660,6 @@ void exit_mmap(struct mm_struct *mm) > goto destroy; > } > > - lru_add_drain(); The above was added in [2]. I think it was just a move from the callee to multiple callers i.e. from unmap_page_range() to unmap_region() and exit_mmap(). For unmap_page_range(), lru_add_drain() was added in [1]. So, I think the same reason can apply to it i.e. we can remove it now. [2] https://github.com/mpe/linux-fullhistory/commit/5b0aee25a3c09b7c4fbb52a737fc9f8ec6761079 > flush_cache_mm(mm); > tlb_gather_mmu_fullmm(&tlb, mm); > /* update_hiwater_rss(mm) here? but nobody should be looking */ > @@ -2103,7 +2102,6 @@ int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift) > vma, new_start, length, false, true)) > return -ENOMEM; > > - lru_add_drain(); The above was added by commit b6a2fea39318e ("mm: variable length argument support"). From what I see, there was no reason provided and I couldn't find on lkml any discussion on this. I think it was just following a pattern from somewhere else of lru_add_drain() along with tlb_gather_mmu(). I think we can remove this one as well. > tlb_gather_mmu(&tlb, mm); > next = vma_next(&vmi); > if (new_end > old_start) { > diff --git a/mm/swap_state.c b/mm/swap_state.c > index e0c0321b8ff7..ca42b2be64d9 100644 > --- a/mm/swap_state.c > +++ b/mm/swap_state.c > @@ -317,7 +317,6 @@ void free_pages_and_swap_cache(struct encoded_page **pages, int nr) > struct folio_batch folios; > unsigned int refs[PAGEVEC_SIZE]; > > - lru_add_drain(); This was added in [1] as well and I think the same reason applies. > folio_batch_init(&folios); > for (int i = 0; i < nr; i++) { > struct folio *folio = page_folio(encoded_page_ptr(pages[i])); > diff --git a/mm/vma.c b/mm/vma.c > index 8e31b7e25aeb..d84e5ef6d15b 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -398,7 +398,6 @@ void unmap_region(struct ma_state *mas, struct vm_area_struct *vma, > struct mm_struct *mm = vma->vm_mm; > struct mmu_gather tlb; > > - lru_add_drain(); Same reason as for exit_mmap(). > tlb_gather_mmu(&tlb, mm); > update_hiwater_rss(mm); > unmap_vmas(&tlb, mas, vma, vma->vm_start, vma->vm_end, vma->vm_end, > @@ -1130,7 +1129,6 @@ static inline void vms_clear_ptes(struct vma_munmap_struct *vms, > * were isolated before we downgraded mmap_lock. > */ > mas_set(mas_detach, 1); > - lru_add_drain(); This is from 9c3ebeda8fb5a ("mm/vma: track start and end for munmap in vma_munmap_struct") and I think it was also just following the pattern. I think we can remove this one as well. > tlb_gather_mmu(&tlb, vms->vma->vm_mm); > update_hiwater_rss(vms->vma->vm_mm); > unmap_vmas(&tlb, mas_detach, vms->vma, vms->start, vms->end, I hope this much history is good enough to convince Andrew. With that please add: Acked-by: Shakeel Butt