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 X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 43426C433ED for ; Fri, 30 Apr 2021 08:20:36 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 9426D613EF for ; Fri, 30 Apr 2021 08:20:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9426D613EF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 0EA046B0093; Fri, 30 Apr 2021 04:20:35 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 09AD96B0095; Fri, 30 Apr 2021 04:20:35 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E7D0A6B0098; Fri, 30 Apr 2021 04:20:34 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0245.hostedemail.com [216.40.44.245]) by kanga.kvack.org (Postfix) with ESMTP id C7F276B0093 for ; Fri, 30 Apr 2021 04:20:34 -0400 (EDT) Received: from smtpin28.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 7D318181AF5C6 for ; Fri, 30 Apr 2021 08:20:34 +0000 (UTC) X-FDA: 78088336788.28.477179C Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by imf27.hostedemail.com (Postfix) with ESMTP id E484380192FB for ; Fri, 30 Apr 2021 08:20:08 +0000 (UTC) Received: from DGGEMS413-HUB.china.huawei.com (unknown [172.30.72.59]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4FWlc729JXz19M5r; Fri, 30 Apr 2021 16:17:59 +0800 (CST) Received: from [10.174.176.174] (10.174.176.174) by DGGEMS413-HUB.china.huawei.com (10.3.19.213) with Microsoft SMTP Server id 14.3.498.0; Fri, 30 Apr 2021 16:20:24 +0800 Subject: Re: [PATCH v2 3/5] mm/huge_memory.c: add missing read-only THP checking in transparent_hugepage_enabled() To: David Hildenbrand , CC: , , , , , , , , , , , , References: <20210429132648.305447-1-linmiaohe@huawei.com> <20210429132648.305447-4-linmiaohe@huawei.com> <68c8c4a8-c4f8-83db-7326-dabeea74239c@redhat.com> <9b511ad9-0ba1-e896-4eb5-0e91ca4b97ab@huawei.com> <9c340151-6dbb-504c-e205-3edda6a5aff8@redhat.com> From: Miaohe Lin Message-ID: Date: Fri, 30 Apr 2021 16:20:24 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <9c340151-6dbb-504c-e205-3edda6a5aff8@redhat.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US X-Originating-IP: [10.174.176.174] X-CFilter-Loop: Reflected Authentication-Results: imf27.hostedemail.com; dkim=none; spf=pass (imf27.hostedemail.com: domain of linmiaohe@huawei.com designates 45.249.212.190 as permitted sender) smtp.mailfrom=linmiaohe@huawei.com; dmarc=pass (policy=none) header.from=huawei.com X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: E484380192FB X-Stat-Signature: qko48bzfqwy1riz3s5nc3fg7tukd9a87 Received-SPF: none (huawei.com>: No applicable sender policy available) receiver=imf27; identity=mailfrom; envelope-from=""; helo=szxga04-in.huawei.com; client-ip=45.249.212.190 X-HE-DKIM-Result: none/none X-HE-Tag: 1619770808-63657 Content-Transfer-Encoding: quoted-printable 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: On 2021/4/30 15:49, David Hildenbrand wrote: > On 30.04.21 03:57, Miaohe Lin wrote: >> On 2021/4/29 22:57, David Hildenbrand wrote: >>> On 29.04.21 15:26, Miaohe Lin wrote: >>>> Since commit 99cb0dbd47a1 ("mm,thp: add read-only THP support for >>>> (non-shmem) FS"), read-only THP file mapping is supported. But it >>>> forgot to add checking for it in transparent_hugepage_enabled(). >>>> To fix it, we add checking for read-only THP file mapping and also >>>> introduce helper transhuge_vma_enabled() to check whether thp is >>>> enabled for specified vma to reduce duplicated code. >>>> >>>> Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shm= em) FS") >>>> Signed-off-by: Miaohe Lin >>>> --- >>>> =C2=A0=C2=A0 include/linux/huge_mm.h | 21 +++++++++++++++++---- >>>> =C2=A0=C2=A0 mm/huge_memory.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 |=C2=A0 6 ++++++ >>>> =C2=A0=C2=A0 mm/khugepaged.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 |=C2=A0 4 +--- >>>> =C2=A0=C2=A0 mm/shmem.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 3 +-- >>>> =C2=A0=C2=A0 4 files changed, 25 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h >>>> index 0a526f211fec..f460b74619fc 100644 >>>> --- a/include/linux/huge_mm.h >>>> +++ b/include/linux/huge_mm.h >>>> @@ -115,6 +115,16 @@ extern struct kobj_attribute shmem_enabled_attr= ; >>>> =C2=A0=C2=A0 =C2=A0 extern unsigned long transparent_hugepage_flags; >>>> =C2=A0=C2=A0 +static inline bool transhuge_vma_enabled(struct vm_are= a_struct *vma, >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long v= m_flags) >>> >>> You're passing the vma already, why do you pass vma->vm_flags separat= ely? It's sufficient to pass in the vma only. >>> >> >> Many thanks for comment! IMO, vm_flags may not always equal to vma->vm= _flags. When hugepage_vma_check() >> is called from collapse_pte_mapped_thp, vma_flags =3D vma->vm_flags | = VM_HUGEPAGE. So I think we should >> pass vm_flags here. >=20 > Oh, sorry, I missed the hugepage_vma_check() user. That's unfortunate. Yes, that's unfortunate. >=20 >>>> =C2=A0=C2=A0 static inline void prep_transhuge_page(struct page *pag= e) {} >>>> =C2=A0=C2=A0 =C2=A0 static inline bool is_transparent_hugepage(struc= t page *page) >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>>> index 76ca1eb2a223..e24a96de2e37 100644 >>>> --- a/mm/huge_memory.c >>>> +++ b/mm/huge_memory.c >>>> @@ -68,12 +68,18 @@ bool transparent_hugepage_enabled(struct vm_area= _struct *vma) >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* The addr is used to check if= the vma size fits */ >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned long addr =3D (vma->vm= _end & HPAGE_PMD_MASK) - HPAGE_PMD_SIZE; >>>> =C2=A0=C2=A0 +=C2=A0=C2=A0=C2=A0 if (!transhuge_vma_enabled(vma, vma= ->vm_flags)) >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return false; >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!transhuge_vma_suitable(vma= , addr)) >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return = false; >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (vma_is_anonymous(vma)) >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return = __transparent_hugepage_enabled(vma); >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (vma_is_shmem(vma)) >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return = shmem_huge_enabled(vma); >>>> +=C2=A0=C2=A0=C2=A0 if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && v= ma->vm_file && >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 !inode_is_open_for_write= (vma->vm_file->f_inode) && >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (vma->vm_flags & VM_EXEC= )) >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return true; >>> >>> Nit: I'm really wondering why we have 3 different functions that soun= d like they are doing the same thing >>> >>> transparent_hugepage_enabled(vma) >>> transhuge_vma_enabled() >>> transhuge_vma_suitable() >>> >>> Which check belongs where? Does it really have to be that complicated= ? >>> >> >> IMO, transhuge_vma_suitable() checks whether pgoff , vm_start and vm_e= nd is possible for thp. >> transhuge_vma_enabled() checks whether thp is explicitly disabled thro= ugh madvise. >> And transparent_hugepage_enabled() use these helpers to get the conclu= sion whether thp is >> enabled for specified vma. >> >> Any suggestions? >=20 > transparent_hugepage_enabled() vs. transhuge_vma_enabled() is really su= b-optimal naming. I guess "transparent_hugepage_active()" would have been= clearer (enabled + suitable + applicable). Cannot really give a good sug= gestion here on how to name transhuge_vma_enabled() differently. >=20 I think transparent_hugepage_active() sounds better too. >=20 > We now have >=20 > transparent_hugepage_enabled() > -> transhuge_vma_enabled() > -> __transparent_hugepage_enabled() -> transhuge_vma_enabled() > -> shmem_huge_enabled() -> transhuge_vma_enabled() >=20 > That looks sub-optimal as well. Maybe we should have a >=20 > static inline bool file_thp_enabled(struct vma *vma) > { > =C2=A0=C2=A0=C2=A0=C2=A0return transhuge_vma_enabled() && > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 IS_ENABLED= (CONFIG_READ_ONLY_THP_FOR_FS) && > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 !inode_is_= open_for_write(vma->vm_file->f_inode) && > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (vma->vm_f= lags & VM_EXEC)) > } >=20 > and in transparent_hugepage_enabled() only do a >=20 > if (vma->vm_file) > =C2=A0=C2=A0=C2=A0=C2=A0return file_thp_enabled(vma); >=20 Really good suggestion! I would try to do this one in next version. Many = thanks for your time and suggestion! :) >=20 > Or move the transhuge_vma_enabled() check completely to transparent_hug= epage_enabled() if possible. >=20