Message ID | 20231213072805.74201-1-jianfeng.w.wang@oracle.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:3b04:b0:fb:cd0c:d3e with SMTP id c4csp7602774dys; Tue, 12 Dec 2023 23:28:44 -0800 (PST) X-Google-Smtp-Source: AGHT+IFYcbcKSkx78obaMj2izayaRi2kTOCNgqWeVVQyW6qdqrFn81cR/ABIquh/Od9TYBjGokZw X-Received: by 2002:a05:6902:e08:b0:d9a:bddd:f714 with SMTP id df8-20020a0569020e0800b00d9abdddf714mr5387704ybb.9.1702452523998; Tue, 12 Dec 2023 23:28:43 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1702452523; cv=none; d=google.com; s=arc-20160816; b=AxBo8fRyv5jJUlFzFmoZfd+nmusWnP4vgHov7b+ItXK1/WzLOkpIX3GanBTUsGHShZ P8Eij08ceeSZN+ZZGTE7HfJfF/mvFFE0V1GMFBarTT1EDSAXR0oph06Q9QsGmqfOsg8H g1JkpSBgyYipIjnwbykvf9285EPHFpeJaKbRAiTGIWbc0XLaSxou7piXqkSmRB2uwzra IWpLQpXO8kgTVXbyOf39m06CR0vOT+6X4Slf96Bwdfns0yTcCIWwz6RO1KLk7XCrMfT3 NV9xzm78abu8Zu0K3HZH0g8SobnS0jH6LXB+qb8y/08hWso6jDgK6PBkUw+sflQJ8p8y /mKw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=RbfgNqZ+9WSCH8jBvjKjM/9Hh1JGThZebaRgzWJuirs=; fh=DIJ01Ae6+3TNHKEdae74IbciJB1Kgg4ryiuG3K+I8lE=; b=qnpAvbdc+ngJTPl+DAHi968CzBe1t//b2VWLO8mQePkB4IaGkp/eFyEVXRAe7k0Yaj 7WwwlNLzNDGFkVXPGj5B+1UrmhCZLrP1mUxRGpEDsmKO5uf6h13qok6j78ijQ9viIk/9 0Z2pOFVl1CTnyUFiSgsfFeU3XWeOI+jlTsNR13n+ZkBmLpD/7fWytIvKqSRgPvf4u5GP zZph9Bmi/wRjgROKzIG+tEG1cQdFASg1a1gqb09t6a6qNcg20Tjgg9xldCB0BwfexouZ OHOagEQF7RtfOaW8G5hhBOrWrZmGa77AaBlVqPEc1wzD8hC+75RWmU8gZPReWAiqYtWH ae8Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@oracle.com header.s=corp-2023-11-20 header.b=jxCTjV64; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=oracle.com Received: from howler.vger.email (howler.vger.email. [2620:137:e000::3:4]) by mx.google.com with ESMTPS id w10-20020a17090a8a0a00b0028ad2ee70absi1488421pjn.39.2023.12.12.23.28.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 12 Dec 2023 23:28:43 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4; Authentication-Results: mx.google.com; dkim=pass header.i=@oracle.com header.s=corp-2023-11-20 header.b=jxCTjV64; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=oracle.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id ADC4E8225080; Tue, 12 Dec 2023 23:28:40 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233024AbjLMH2b (ORCPT <rfc822;dexuan.linux@gmail.com> + 99 others); Wed, 13 Dec 2023 02:28:31 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50158 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233009AbjLMH2a (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 13 Dec 2023 02:28:30 -0500 Received: from mx0a-00069f02.pphosted.com (mx0a-00069f02.pphosted.com [205.220.165.32]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 64523AD for <linux-kernel@vger.kernel.org>; Tue, 12 Dec 2023 23:28:33 -0800 (PST) Received: from pps.filterd (m0246617.ppops.net [127.0.0.1]) by mx0b-00069f02.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3BD7DwYO024904; Wed, 13 Dec 2023 07:28:29 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=from : to : cc : subject : date : message-id : mime-version : content-transfer-encoding; s=corp-2023-11-20; bh=RbfgNqZ+9WSCH8jBvjKjM/9Hh1JGThZebaRgzWJuirs=; b=jxCTjV64zJHBZ4WQ0uI5LhDxFcSWVEIK+CoUmbV+1nzHgr3gCloRMxhYV3BvHdteemRc +m45CNBRcD66ycEwlSN7CFAXJrJHtxCl2n7B87t7IvmpfbV7gElu0vKXJbnkKxkpdqQ6 uVFsk71rIaoKjdLiQXwuXG6DydU2MS1cVM7aXJUcj8IIOa0L/qDOcBF/T0rrgfg0lriA 8GJP5K5eVyKTaAfKqRkSCIAAXRv48c0AC7Gg6lYrJy/cUtxqpz3AN6whvIAWmAKYBTS6 VTqFUlk4Ryqe1+BCLAHvL18peaTlOQasVle3svyhWcQ0c7y9m0M+N+RgssShK/JvP/13 6g== Received: from iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com (iadpaimrmta01.appoci.oracle.com [130.35.100.223]) by mx0b-00069f02.pphosted.com (PPS) with ESMTPS id 3uvgsufja0-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 13 Dec 2023 07:28:29 +0000 Received: from pps.filterd (iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com [127.0.0.1]) by iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com (8.17.1.19/8.17.1.19) with ESMTP id 3BD70Idl010147; Wed, 13 Dec 2023 07:28:27 GMT Received: from pps.reinject (localhost [127.0.0.1]) by iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com (PPS) with ESMTPS id 3uvep7tej6-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 13 Dec 2023 07:28:27 +0000 Received: from iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com (iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 3BD7SRdv020500; Wed, 13 Dec 2023 07:28:27 GMT Received: from jfwang-mac.us.oracle.com (dhcp-10-159-129-184.vpn.oracle.com [10.159.129.184]) by iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com (PPS) with ESMTP id 3uvep7te97-1; Wed, 13 Dec 2023 07:28:27 +0000 From: Jianfeng Wang <jianfeng.w.wang@oracle.com> To: akpm@linux-foundation.org Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Jianfeng Wang <jianfeng.w.wang@oracle.com> Subject: [PATCH] mm: remove redundant lru_add_drain() prior to unmapping pages Date: Tue, 12 Dec 2023 23:28:05 -0800 Message-ID: <20231213072805.74201-1-jianfeng.w.wang@oracle.com> X-Mailer: git-send-email 2.42.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-12-12_14,2023-12-12_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 suspectscore=0 phishscore=0 malwarescore=0 spamscore=0 mlxlogscore=999 bulkscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000 definitions=main-2312130053 X-Proofpoint-ORIG-GUID: HraMxghePdLtKHJbsbQnLR9EzC-2LppB X-Proofpoint-GUID: HraMxghePdLtKHJbsbQnLR9EzC-2LppB X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Tue, 12 Dec 2023 23:28:40 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1785150857765127288 X-GMAIL-MSGID: 1785150857765127288 |
Series |
mm: remove redundant lru_add_drain() prior to unmapping pages
|
|
Commit Message
Jianfeng Wang
Dec. 13, 2023, 7:28 a.m. UTC
When unmapping VMA pages, pages will be gathered in batch and released by
tlb_finish_mmu() if CONFIG_MMU_GATHER_NO_GATHER is not set. The function
tlb_finish_mmu() is responsible for calling free_pages_and_swap_cache(),
which calls lru_add_drain() to drain cached pages in folio_batch before
releasing gathered pages. Thus, it is redundant to call lru_add_drain()
before gathering pages, if CONFIG_MMU_GATHER_NO_GATHER is not set.
Remove lru_add_drain() prior to gathering and unmapping pages in
exit_mmap() and unmap_region() if CONFIG_MMU_GATHER_NO_GATHER is not set.
Note that the page unmapping process in oom_killer (e.g., in
__oom_reap_task_mm()) also uses tlb_finish_mmu() and does not have
redundant lru_add_drain(). So, this commit makes the code more consistent.
Signed-off-by: Jianfeng Wang <jianfeng.w.wang@oracle.com>
---
mm/mmap.c | 4 ++++
1 file changed, 4 insertions(+)
Comments
On Tue, 2023-12-12 at 23:28 -0800, Jianfeng Wang wrote: > When unmapping VMA pages, pages will be gathered in batch and released by > tlb_finish_mmu() if CONFIG_MMU_GATHER_NO_GATHER is not set. The function > tlb_finish_mmu() is responsible for calling free_pages_and_swap_cache(), > which calls lru_add_drain() to drain cached pages in folio_batch before > releasing gathered pages. Thus, it is redundant to call lru_add_drain() > before gathering pages, if CONFIG_MMU_GATHER_NO_GATHER is not set. > > Remove lru_add_drain() prior to gathering and unmapping pages in > exit_mmap() and unmap_region() if CONFIG_MMU_GATHER_NO_GATHER is not set. > > Note that the page unmapping process in oom_killer (e.g., in > __oom_reap_task_mm()) also uses tlb_finish_mmu() and does not have > redundant lru_add_drain(). So, this commit makes the code more consistent. > > Signed-off-by: Jianfeng Wang <jianfeng.w.wang@oracle.com> > --- > mm/mmap.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/mm/mmap.c b/mm/mmap.c > index 1971bfffcc03..0451285dee4f 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -2330,7 +2330,9 @@ static void unmap_region(struct mm_struct *mm, struct ma_state *mas, > struct mmu_gather tlb; > unsigned long mt_start = mas->index; > > +#ifdef CONFIG_MMU_GATHER_NO_GATHER In your comment you say skip lru_add_drain() when CONFIG_MMU_GATHER_NO_GATHER is *not* set. So shouldn't this be #ifndef CONFIG_MMU_GATHER_NO_GATHER ? > lru_add_drain(); > +#endif > tlb_gather_mmu(&tlb, mm); > update_hiwater_rss(mm); > unmap_vmas(&tlb, mas, vma, start, end, tree_end, mm_wr_locked); > @@ -3300,7 +3302,9 @@ void exit_mmap(struct mm_struct *mm) > return; > } > > +#ifdef CONFIG_MMU_GATHER_NO_GATHER same question as above. > lru_add_drain(); > +#endif > flush_cache_mm(mm); > tlb_gather_mmu_fullmm(&tlb, mm); > /* update_hiwater_rss(mm) here? but nobody should be looking */
On 12/13/23 2:57 PM, Tim Chen wrote: > On Tue, 2023-12-12 at 23:28 -0800, Jianfeng Wang wrote: >> When unmapping VMA pages, pages will be gathered in batch and released by >> tlb_finish_mmu() if CONFIG_MMU_GATHER_NO_GATHER is not set. The function >> tlb_finish_mmu() is responsible for calling free_pages_and_swap_cache(), >> which calls lru_add_drain() to drain cached pages in folio_batch before >> releasing gathered pages. Thus, it is redundant to call lru_add_drain() >> before gathering pages, if CONFIG_MMU_GATHER_NO_GATHER is not set. >> >> Remove lru_add_drain() prior to gathering and unmapping pages in >> exit_mmap() and unmap_region() if CONFIG_MMU_GATHER_NO_GATHER is not set. >> >> Note that the page unmapping process in oom_killer (e.g., in >> __oom_reap_task_mm()) also uses tlb_finish_mmu() and does not have >> redundant lru_add_drain(). So, this commit makes the code more consistent. >> >> Signed-off-by: Jianfeng Wang <jianfeng.w.wang@oracle.com> >> --- >> mm/mmap.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/mm/mmap.c b/mm/mmap.c >> index 1971bfffcc03..0451285dee4f 100644 >> --- a/mm/mmap.c >> +++ b/mm/mmap.c >> @@ -2330,7 +2330,9 @@ static void unmap_region(struct mm_struct *mm, struct ma_state *mas, >> struct mmu_gather tlb; >> unsigned long mt_start = mas->index; >> >> +#ifdef CONFIG_MMU_GATHER_NO_GATHER > > In your comment you say skip lru_add_drain() when CONFIG_MMU_GATHER_NO_GATHER > is *not* set. So shouldn't this be > > #ifndef CONFIG_MMU_GATHER_NO_GATHER ? > Hi Tim, The mmu_gather feature is used to gather pages produced by unmap_vmas() and release them in batch in tlb_finish_mmu(). The feature is *on* if CONFIG_MMU_GATHER_NO_GATHER is *not* set. Note that: tlb_finish_mmu() will call free_pages_and_swap_cache()/lru_add_drain() only when the feature is on. Yes, this commit aims to skip lru_add_drain() when CONFIG_MMU_GATHER_NO_GATHER is *not* set (i.e. when the mmu_gather feature is on) because it is redundant. If CONFIG_MMU_GATHER_NO_GATHER is set, pages will be released in unmap_vmas(). tlb_finish_mmu() will not call lru_add_drain(). So, it is still necessary to keep the lru_add_drain() call to clear cached pages before unmap_vmas(), as folio_batchs hold a reference count for pages in them. The same applies to the other case. Thanks, - Jianfeng >> lru_add_drain(); >> +#endif >> tlb_gather_mmu(&tlb, mm); >> update_hiwater_rss(mm); >> unmap_vmas(&tlb, mas, vma, start, end, tree_end, mm_wr_locked); >> @@ -3300,7 +3302,9 @@ void exit_mmap(struct mm_struct *mm) >> return; >> } >> >> +#ifdef CONFIG_MMU_GATHER_NO_GATHER > > same question as above. > >> lru_add_drain(); >> +#endif >> flush_cache_mm(mm); >> tlb_gather_mmu_fullmm(&tlb, mm); >> /* update_hiwater_rss(mm) here? but nobody should be looking */ >
On Wed, 2023-12-13 at 17:03 -0800, Jianfeng Wang wrote: > On 12/13/23 2:57 PM, Tim Chen wrote: > > On Tue, 2023-12-12 at 23:28 -0800, Jianfeng Wang wrote: > > > When unmapping VMA pages, pages will be gathered in batch and released by > > > tlb_finish_mmu() if CONFIG_MMU_GATHER_NO_GATHER is not set. The function > > > tlb_finish_mmu() is responsible for calling free_pages_and_swap_cache(), > > > which calls lru_add_drain() to drain cached pages in folio_batch before > > > releasing gathered pages. Thus, it is redundant to call lru_add_drain() > > > before gathering pages, if CONFIG_MMU_GATHER_NO_GATHER is not set. > > > > > > Remove lru_add_drain() prior to gathering and unmapping pages in > > > exit_mmap() and unmap_region() if CONFIG_MMU_GATHER_NO_GATHER is not set. > > > > > > Note that the page unmapping process in oom_killer (e.g., in > > > __oom_reap_task_mm()) also uses tlb_finish_mmu() and does not have > > > redundant lru_add_drain(). So, this commit makes the code more consistent. > > > > > > Signed-off-by: Jianfeng Wang <jianfeng.w.wang@oracle.com> > > > --- > > > mm/mmap.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > > index 1971bfffcc03..0451285dee4f 100644 > > > --- a/mm/mmap.c > > > +++ b/mm/mmap.c > > > @@ -2330,7 +2330,9 @@ static void unmap_region(struct mm_struct *mm, struct ma_state *mas, > > > struct mmu_gather tlb; > > > unsigned long mt_start = mas->index; > > > > > > +#ifdef CONFIG_MMU_GATHER_NO_GATHER > > > > In your comment you say skip lru_add_drain() when CONFIG_MMU_GATHER_NO_GATHER > > is *not* set. So shouldn't this be > > > > #ifndef CONFIG_MMU_GATHER_NO_GATHER ? > > > Hi Tim, > > The mmu_gather feature is used to gather pages produced by unmap_vmas() and > release them in batch in tlb_finish_mmu(). The feature is *on* if > CONFIG_MMU_GATHER_NO_GATHER is *not* set. Note that: tlb_finish_mmu() will call > free_pages_and_swap_cache()/lru_add_drain() only when the feature is on. Thanks for the explanation. Looking at the code, lru_add_drain() is executed for #ifndef CONFIG_MMU_GATHER_NO_GATHER in tlb_finish_mmu(). So the logic of your patch is fine. The #ifndef CONFIG_MMU_GATHER_NO_GATHER means mmu_gather feature is on. The double negative throws me off on in my first read of your commit log. Suggest that you add a comment in code to make it easier for future code maintenence: /* defer lru_add_drain() to tlb_finish_mmu() for ifndef CONFIG_MMU_GATHER_NO_GATHER */ Is your change of skipping the extra lru_add_drain() motivated by some performance reason in a workload? Wonder whether it is worth adding an extra ifdef in the code. Tim > > Yes, this commit aims to skip lru_add_drain() when CONFIG_MMU_GATHER_NO_GATHER > is *not* set (i.e. when the mmu_gather feature is on) because it is redundant. > > If CONFIG_MMU_GATHER_NO_GATHER is set, pages will be released in unmap_vmas(). > tlb_finish_mmu() will not call lru_add_drain(). So, it is still necessary to > keep the lru_add_drain() call to clear cached pages before unmap_vmas(), as > folio_batchs hold a reference count for pages in them. > > The same applies to the other case. > > Thanks, > - Jianfeng > > > > lru_add_drain(); > > > +#endif > > > tlb_gather_mmu(&tlb, mm); > > > update_hiwater_rss(mm); > > > unmap_vmas(&tlb, mas, vma, start, end, tree_end, mm_wr_locked); > > > @@ -3300,7 +3302,9 @@ void exit_mmap(struct mm_struct *mm) > > > return; > > > } > > > > > > +#ifdef CONFIG_MMU_GATHER_NO_GATHER > > > > same question as above. > > > > > lru_add_drain(); > > > +#endif > > > flush_cache_mm(mm); > > > tlb_gather_mmu_fullmm(&tlb, mm); > > > /* update_hiwater_rss(mm) here? but nobody should be looking */ > >
On 12/14/23 9:57 AM, Tim Chen wrote: > On Wed, 2023-12-13 at 17:03 -0800, Jianfeng Wang wrote: >> On 12/13/23 2:57 PM, Tim Chen wrote: >>> On Tue, 2023-12-12 at 23:28 -0800, Jianfeng Wang wrote: >>>> When unmapping VMA pages, pages will be gathered in batch and released by >>>> tlb_finish_mmu() if CONFIG_MMU_GATHER_NO_GATHER is not set. The function >>>> tlb_finish_mmu() is responsible for calling free_pages_and_swap_cache(), >>>> which calls lru_add_drain() to drain cached pages in folio_batch before >>>> releasing gathered pages. Thus, it is redundant to call lru_add_drain() >>>> before gathering pages, if CONFIG_MMU_GATHER_NO_GATHER is not set. >>>> >>>> Remove lru_add_drain() prior to gathering and unmapping pages in >>>> exit_mmap() and unmap_region() if CONFIG_MMU_GATHER_NO_GATHER is not set. >>>> >>>> Note that the page unmapping process in oom_killer (e.g., in >>>> __oom_reap_task_mm()) also uses tlb_finish_mmu() and does not have >>>> redundant lru_add_drain(). So, this commit makes the code more consistent. >>>> >>>> Signed-off-by: Jianfeng Wang <jianfeng.w.wang@oracle.com> >>>> --- >>>> mm/mmap.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/mm/mmap.c b/mm/mmap.c >>>> index 1971bfffcc03..0451285dee4f 100644 >>>> --- a/mm/mmap.c >>>> +++ b/mm/mmap.c >>>> @@ -2330,7 +2330,9 @@ static void unmap_region(struct mm_struct *mm, struct ma_state *mas, >>>> struct mmu_gather tlb; >>>> unsigned long mt_start = mas->index; >>>> >>>> +#ifdef CONFIG_MMU_GATHER_NO_GATHER >>> >>> In your comment you say skip lru_add_drain() when CONFIG_MMU_GATHER_NO_GATHER >>> is *not* set. So shouldn't this be >>> >>> #ifndef CONFIG_MMU_GATHER_NO_GATHER ? >>> >> Hi Tim, >> >> The mmu_gather feature is used to gather pages produced by unmap_vmas() and >> release them in batch in tlb_finish_mmu(). The feature is *on* if >> CONFIG_MMU_GATHER_NO_GATHER is *not* set. Note that: tlb_finish_mmu() will call >> free_pages_and_swap_cache()/lru_add_drain() only when the feature is on. > > Thanks for the explanation. > > Looking at the code, lru_add_drain() is executed for #ifndef CONFIG_MMU_GATHER_NO_GATHER > in tlb_finish_mmu(). So the logic of your patch is fine. > > The #ifndef CONFIG_MMU_GATHER_NO_GATHER means > mmu_gather feature is on. The double negative throws me off on in my first read > of your commit log. > > Suggest that you add a comment in code to make it easier for > future code maintenence: > > /* defer lru_add_drain() to tlb_finish_mmu() for ifndef CONFIG_MMU_GATHER_NO_GATHER */ > > Is your change of skipping the extra lru_add_drain() motivated by some performance reason > in a workload? Wonder whether it is worth adding an extra ifdef in the code. > > Tim > Okay, great suggestion. We observe heavy contention on the LRU lock, introduced by lru_add_drain() and release_pages() for a prod workload, and we're trying to reduce the level of contention. lru_add_drain() is a complex function that first takes a local CPU lock and iterate through *all* folio_batches to see if there are pages to be moved to and between LRU lists. At that point, any page in these folio_batches will trigger acquiring the per-LRU spin lock and increase the level of lock contention. Applying the change can avoid calling lru_add_drain() unnecessarily, which is a source of lock contention. Together with the comment line suggested by you, I believe this also increases code readability to clarify the mmu_gather feature. - Jianfeng >> >> Yes, this commit aims to skip lru_add_drain() when CONFIG_MMU_GATHER_NO_GATHER >> is *not* set (i.e. when the mmu_gather feature is on) because it is redundant. >> >> If CONFIG_MMU_GATHER_NO_GATHER is set, pages will be released in unmap_vmas(). >> tlb_finish_mmu() will not call lru_add_drain(). So, it is still necessary to >> keep the lru_add_drain() call to clear cached pages before unmap_vmas(), as >> folio_batchs hold a reference count for pages in them. >> >> The same applies to the other case. >> >> Thanks, >> - Jianfeng >> >>>> lru_add_drain(); >>>> +#endif >>>> tlb_gather_mmu(&tlb, mm); >>>> update_hiwater_rss(mm); >>>> unmap_vmas(&tlb, mas, vma, start, end, tree_end, mm_wr_locked); >>>> @@ -3300,7 +3302,9 @@ void exit_mmap(struct mm_struct *mm) >>>> return; >>>> } >>>> >>>> +#ifdef CONFIG_MMU_GATHER_NO_GATHER >>> >>> same question as above. >>> >>>> lru_add_drain(); >>>> +#endif >>>> flush_cache_mm(mm); >>>> tlb_gather_mmu_fullmm(&tlb, mm); >>>> /* update_hiwater_rss(mm) here? but nobody should be looking */ >>> >
diff --git a/mm/mmap.c b/mm/mmap.c index 1971bfffcc03..0451285dee4f 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -2330,7 +2330,9 @@ static void unmap_region(struct mm_struct *mm, struct ma_state *mas, struct mmu_gather tlb; unsigned long mt_start = mas->index; +#ifdef CONFIG_MMU_GATHER_NO_GATHER lru_add_drain(); +#endif tlb_gather_mmu(&tlb, mm); update_hiwater_rss(mm); unmap_vmas(&tlb, mas, vma, start, end, tree_end, mm_wr_locked); @@ -3300,7 +3302,9 @@ void exit_mmap(struct mm_struct *mm) return; } +#ifdef CONFIG_MMU_GATHER_NO_GATHER lru_add_drain(); +#endif flush_cache_mm(mm); tlb_gather_mmu_fullmm(&tlb, mm); /* update_hiwater_rss(mm) here? but nobody should be looking */