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 81CBFC433F5 for ; Thu, 16 Dec 2021 13:17:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CAD5A6B0075; Thu, 16 Dec 2021 08:17:16 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C35976B0078; Thu, 16 Dec 2021 08:17:16 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AD66D6B007B; Thu, 16 Dec 2021 08:17:16 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0230.hostedemail.com [216.40.44.230]) by kanga.kvack.org (Postfix) with ESMTP id 9B50F6B0075 for ; Thu, 16 Dec 2021 08:17:16 -0500 (EST) Received: from smtpin14.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 604668249980 for ; Thu, 16 Dec 2021 13:17:06 +0000 (UTC) X-FDA: 78923708052.14.A58EDF0 Received: from mail-lf1-f44.google.com (mail-lf1-f44.google.com [209.85.167.44]) by imf27.hostedemail.com (Postfix) with ESMTP id 99B1E4001C for ; Thu, 16 Dec 2021 13:17:05 +0000 (UTC) Received: by mail-lf1-f44.google.com with SMTP id z7so49567551lfi.11 for ; Thu, 16 Dec 2021 05:17:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=ZEl4Y00oH7WwScJtlGP5aTXSfGdKcfwYAoC6lzWZO8I=; b=HNvbWZ0Gysgkzi6U/8oyZTqkrPAYdQTD8d/zjA0boJ/igUmu/gw9xlqZ2N29VQhhZi 1AawyZtM/yKCi2zbKKDwnFOLLEejN7T4ypVLAmYm1VVDP/NG0cgnMbR7Za+9hFB7LymP Ss/Zeb9ed3HEzCdz1rMB5x1226tBVAXlGJxgQYNQgNHrRR8WQ7Hub3LPNDenKXK4ViVb toUR67d70+vBbjcCywZI1QhtVLK3grPgyuUDJJK1tSjb7E84BJ5s09MB7wKoCHqe0mOg HXWyonfjMdq2LU1J3Jc3VWQmv5TTq2oOBeU+jFkpsCll6ZWQj26Xz1xtQnaB37y/ywIX vi5A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=ZEl4Y00oH7WwScJtlGP5aTXSfGdKcfwYAoC6lzWZO8I=; b=vN4EjZJQgpyklNxtv53PoEsDLMavs/gkEf7SBIuXsmlePvrNJ1qlWTjHI3u+YHk5kp gt1/duBI6ZMiFlKtDSC8BCGasIwura1lgsIPLT793cc45y9huLc/nVmFYYWXfo97oawp HGYW916iqerMkG8zN/SzEF+pswnaRkkKudZN8Wx/HWhAlk/ixofnL7BQnwcZXZZVRlph XzYjRGwEo72NEYyLTN9IYoYFhdJSY79XkDklQSUtIP/bwFHQlWA8LOx9cXS0BB8NWjiE y00vg2/zApLI8fhfSs/XUZq4FlhkI6F6WvEyI4dzObIZitJ4WIjVo5K0C6wV8/bUkghy rNfA== X-Gm-Message-State: AOAM532S/y6DMJwkUSJGfF8+qGHWbEmNP+aPnsWJ6B1KrWNJaODWC9kv 3XmmmkRhAOxrU/I6KU5yH3g= X-Google-Smtp-Source: ABdhPJzhjxOfZDJXTzBo/hzVaaUy/p/Pag0ohFHVGE9yVGL+fBjP8FXJKUXCD1oAS3m9ijuhOIypgQ== X-Received: by 2002:a19:7116:: with SMTP id m22mr13810622lfc.491.1639660624158; Thu, 16 Dec 2021 05:17:04 -0800 (PST) Received: from localhost.localdomain ([131.228.2.20]) by smtp.gmail.com with ESMTPSA id b26sm865506lff.148.2021.12.16.05.17.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 16 Dec 2021 05:17:03 -0800 (PST) Subject: Re: [PATCH V2] mm/gup.c: stricter check on THP migration entry during follow_pmd_mask To: "Huang, Ying" Cc: Zi Yan , linux-mm@kvack.org, "Kirill A. Shutemov" , akpm@linux-foundation.org References: <20211211151014.650778-1-lixinhai.lxh@gmail.com> <5c0f5b5c-7c22-a7ab-4add-fa6bd11f7af8@gmail.com> <875yrpszca.fsf@yhuang6-desk2.ccr.corp.intel.com> From: Li Xinhai Message-ID: <59d77170-6845-2765-bebd-7bb08fd428ee@gmail.com> Date: Thu, 16 Dec 2021 21:16:55 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <875yrpszca.fsf@yhuang6-desk2.ccr.corp.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=HNvbWZ0G; spf=pass (imf27.hostedemail.com: domain of lixinhai.lxh@gmail.com designates 209.85.167.44 as permitted sender) smtp.mailfrom=lixinhai.lxh@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Rspamd-Queue-Id: 99B1E4001C X-Stat-Signature: kor9qse7a8f316p9oginrt4bbsf1gaej X-Rspamd-Server: rspam04 X-HE-Tag: 1639660625-829308 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 12/16/21 4:55 PM, Huang, Ying wrote: > Li Xinhai writes: >=20 >> On 12/15/21 10:46 PM, Zi Yan wrote: >>> On 15 Dec 2021, at 7:47, Li Xinhai wrote: >>> >>>> On 12/11/21 11:16 PM, Li Xinhai wrote: >>>>> >>>>> >>>>> On 12/11/21 11:10 PM, Li Xinhai wrote: >>>>>> When BUG_ON check for THP migration entry, the exsiting code only = check >>>>>> thp_migration_supported case, but not for !thp_migration_supported= case. >>>>>> To make the BUG_ON check consistent, we need catch both cases. >>>>>> >>>>>> Move the BUG_ON check one step eariler, because if the bug happen = we >>>>>> should know it instead of depend on FOLL_MIGRATION been used by ca= ller. >>>>>> >>>>>> Because pmdval instead of *pmd is read by the is_pmd_migration_ent= ry() >>>>>> check, the existing code don't help to avoid useless locking withi= n >>>>>> pmd_migration_entry_wait(), so remove that check. >>>>>> >>>>>> Signed-off-by: Li Xinhai >>>>>> --- >>>>> V1->V2: >>>>> Move the BUG_ON() check before if(!(flags & FOLL_MIGRATION)); and a= dd comments >>>>> for it. >>>>> >>>> Yan, Ying and Kirill have been worked on this part of code, may help= to review. >>>> >>>> This change was based on my code review, no real bug has been observ= ed. >>>> >>>> >>>>>> mm/gup.c | 13 +++++++++---- >>>>>> 1 file changed, 9 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/mm/gup.c b/mm/gup.c >>>>>> index 2c51e9748a6a..94d0e586ca0b 100644 >>>>>> --- a/mm/gup.c >>>>>> +++ b/mm/gup.c >>>>>> @@ -642,12 +642,17 @@ static struct page *follow_pmd_mask(struct v= m_area_struct *vma, >>>>>> =A0=A0 } >>>>>> retry: >>>>>> =A0=A0 if (!pmd_present(pmdval)) { >>>>>> +=A0=A0=A0 /* >>>>>> +=A0=A0=A0=A0 * Should never reach here, if thp migration is not s= upported; >>>>>> +=A0=A0=A0=A0 * Otherwise, it must be a thp miration entry. >>>>>> +=A0=A0=A0=A0 */ >>>>>> +=A0=A0=A0 VM_BUG_ON(!thp_migration_supported() || >>>>>> +=A0=A0=A0=A0=A0=A0=A0=A0 !is_pmd_migration_entry(pmdval)); >>>>>> + >>> This means VM_BUG_ON will be triggered when pmdval is not present >>> and THP migration >>> is not enabled. This can happen when it is pmd_none(). That is not a = bug and should >>> just return no_page_table(vma, flags). >>> >> >> Thanks for review! >> The pmd_none() has been checked at the beginning of follow_pmd_mask() = and before >> the 'retry' loop start again, so that possibility is excluded. >> >> We will have VM_BUG_ON(1) if thp_migration_supported() is false, or >> VM_BUG_ON(!is_pmd_migration_entry(pmdval)) if thp_migration_supported(= ) is true, by >> compiler. >> >> If we left !thp_migration_supported() case for the VM_BUG_ON(), it wil= l cause >> misunderstanding that that case is not a bug at the code context. >=20 > I think your code works. The patch description may be improved. If > !thp_migration_supported() and !pmd_present(), the original code may > dead loop in theory. >=20 Thanks for review. I will mention this potential dead loop in the commit message. > Best Regards, > Huang, Ying >=20 >>>>>> =A0=A0=A0=A0 if (likely(!(flags & FOLL_MIGRATION))) >>>>>> =A0=A0=A0=A0=A0=A0 return no_page_table(vma, flags); >>>>>> -=A0=A0=A0 VM_BUG_ON(thp_migration_supported() && >>>>>> -=A0=A0=A0=A0=A0=A0=A0=A0 !is_pmd_migration_entry(pmdval)); >>>>>> -=A0=A0=A0 if (is_pmd_migration_entry(pmdval)) >>>>>> -=A0=A0=A0=A0=A0 pmd_migration_entry_wait(mm, pmd); >>>>>> + >>>>>> +=A0=A0=A0 pmd_migration_entry_wait(mm, pmd); >>>>>> =A0=A0=A0=A0 pmdval =3D READ_ONCE(*pmd); >>>>>> =A0=A0=A0=A0 /* >>>>>> =A0=A0=A0=A0=A0 * MADV_DONTNEED may convert the pmd to null beca= use >>>>>> >>> -- >>> Best Regards, >>> Yan, Zi >>>