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 CFF05C433F5 for ; Thu, 16 Dec 2021 02:00:36 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E94E66B0071; Wed, 15 Dec 2021 21:00:25 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E1E0E6B0073; Wed, 15 Dec 2021 21:00:25 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C6F8F6B0074; Wed, 15 Dec 2021 21:00:25 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0090.hostedemail.com [216.40.44.90]) by kanga.kvack.org (Postfix) with ESMTP id B213C6B0071 for ; Wed, 15 Dec 2021 21:00:25 -0500 (EST) Received: from smtpin22.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 60FB282499B9 for ; Thu, 16 Dec 2021 02:00:15 +0000 (UTC) X-FDA: 78922002390.22.12312CA Received: from mail-lf1-f50.google.com (mail-lf1-f50.google.com [209.85.167.50]) by imf11.hostedemail.com (Postfix) with ESMTP id 3B2EB40018 for ; Thu, 16 Dec 2021 02:00:13 +0000 (UTC) Received: by mail-lf1-f50.google.com with SMTP id d38so2243689lfv.0 for ; Wed, 15 Dec 2021 18:00:14 -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=UEWZiUL0C23L9ienDj1WFZkD3KDzwk9UaOpjo1EBbTs=; b=ixKf4hCm2yrctkM0B+dPhr+Ln0g2TwOt62K0gu3NdwtN9ermgi0/gRYcnjWVKPG20G /Km2uS9f/trYsT/K3VtqVrKm5+VGRrEO7IoY+y/4iVTjRE2sqjCJbsgrSbYUZ8DjY/RS O8J1GfCdUXoVmRArrKNMh24k+C/oMLePOAsdwN362wAOdwpqieuCCWTg9Nkh/hutu/SG dHGxLasgcDF/JT15oN9a6cF545RWS3GN1al1Kt98P0njgiAliCS8Hw0wOiKsIZ9D7b21 eoaFH/zo9bqUfiySYYOO6EHQ8y1pZFHLPyiUU+xymg760mgQbd50RJo1D2cO/FDgOEBm kiCQ== 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=UEWZiUL0C23L9ienDj1WFZkD3KDzwk9UaOpjo1EBbTs=; b=7PozCivEzOcmMEjNzTLFVrVuG8PsejTbfXnVhc1bIuHIESKIbo4UF1yjFyZbXfN9S5 qj/ICieWpMbBP+6X/+dHui93/zr0fVXJt5UyWYBZG7Io5zfL2AjTu+WAy5KPiWBghJwi ulLmRmtsPSEfntNHELldNlYw4RiaNAVcxgkIivMkZ3gq0nWv6LodQhn5KruEnTQ1glAT Tu4CvVZdw9iZ8GaUf7v+K4LOagVU0b+TQ6PXhBoEoBOzZ50yL2OulDhZltQ0SlgH8kyc 9tOa+yV6E1c6aCz8gPhFLz86rq8vl7BP5oxR5UsmOqgmc4R0kpiiQ9u9C6+YMmWpDjjH 6jeA== X-Gm-Message-State: AOAM533R0iZrjM5h4l5aYMVkvQD48cP47MJrS6FPuTxDni/N76J1RUiw 2s+1C4E7oYKIrpTcptSbE+U= X-Google-Smtp-Source: ABdhPJyA18sx9mbQkjI1lphlIZw3y3ehE5OTb63eBDhaA4eX2MKY4lrHOnjdHcm9yrAJmubFSGM/rg== X-Received: by 2002:ac2:435a:: with SMTP id o26mr2773541lfl.338.1639620013380; Wed, 15 Dec 2021 18:00:13 -0800 (PST) Received: from localhost.localdomain ([131.228.2.20]) by smtp.gmail.com with ESMTPSA id e14sm593677lfs.150.2021.12.15.18.00.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 15 Dec 2021 18:00:12 -0800 (PST) Subject: Re: [PATCH V2] mm/gup.c: stricter check on THP migration entry during follow_pmd_mask To: Zi Yan Cc: linux-mm@kvack.org, Huang Ying , "Kirill A. Shutemov" , akpm@linux-foundation.org References: <20211211151014.650778-1-lixinhai.lxh@gmail.com> <5c0f5b5c-7c22-a7ab-4add-fa6bd11f7af8@gmail.com> From: Li Xinhai Message-ID: Date: Thu, 16 Dec 2021 10:00:04 +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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US X-Rspamd-Queue-Id: 3B2EB40018 X-Stat-Signature: 5mirobn8yh1yitg7h88rhbnibfcttyar Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=ixKf4hCm; spf=pass (imf11.hostedemail.com: domain of lixinhai.lxh@gmail.com designates 209.85.167.50 as permitted sender) smtp.mailfrom=lixinhai.lxh@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Rspamd-Server: rspam10 X-HE-Tag: 1639620013-476160 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/15/21 10:46 PM, Zi Yan wrote: > On 15 Dec 2021, at 7:47, Li Xinhai wrote: >=20 >> 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 ch= eck >>>> thp_migration_supported case, but not for !thp_migration_supported c= ase. >>>> 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 call= er. >>>> >>>> Because pmdval instead of *pmd is read by the is_pmd_migration_entry= () >>>> check, the existing code don't help to avoid useless locking within >>>> 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 add= comments >>> for it. >>> >> Yan, Ying and Kirill have been worked on this part of code, may help t= o review. >> >> This change was based on my code review, no real bug has been observed= . >> >> >>>> =C2=A0 mm/gup.c | 13 +++++++++---- >>>> =C2=A0 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 vm_= area_struct *vma, >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >>>> =C2=A0 retry: >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!pmd_present(pmdval)) { >>>> +=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 * Should never rea= ch here, if thp migration is not supported; >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Otherwise, it mu= st be a thp miration entry. >>>> +=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 VM_BUG_ON(!thp_migration= _supported() || >>>> +=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 !is_pmd_migration_entry(pmdval)); >>>> + >=20 > This means VM_BUG_ON will be triggered when pmdval is not present and T= HP migration > is not enabled. This can happen when it is pmd_none(). That is not a bu= g and should > just return no_page_table(vma, flags). >=20 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() i= s true, by compiler. If we left !thp_migration_supported() case for the VM_BUG_ON(), it will c= ause misunderstanding that that case is not a bug at the code context. >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (likely(!= (flags & FOLL_MIGRATION))) >>>> =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 return no_page_table(vma, flags); >>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 VM_BUG_ON(thp_migration_= supported() && >>>> -=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 !is_pmd_migration_entry(pmdval)); >>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (is_pmd_migration_ent= ry(pmdval)) >>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 = pmd_migration_entry_wait(mm, pmd); >>>> + >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pmd_migration_entry_wait= (mm, pmd); >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pmdval =3D R= EAD_ONCE(*pmd); >>>> =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 * MADV= _DONTNEED may convert the pmd to null because >>>> >=20 > -- > Best Regards, > Yan, Zi >=20