From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <SRS0=If0M=MO=kvack.org=owner-linux-mm@kernel.org>
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.4 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 AA037C07E9D
	for <linux-mm@archiver.kernel.org>; Thu, 22 Jul 2021 04:41:06 +0000 (UTC)
Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17])
	by mail.kernel.org (Postfix) with ESMTP id 0FBDB61244
	for <linux-mm@archiver.kernel.org>; Thu, 22 Jul 2021 04:41:05 +0000 (UTC)
DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0FBDB61244
Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com
Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org
Received: by kanga.kvack.org (Postfix)
	id 8C0956B0036; Thu, 22 Jul 2021 00:41:05 -0400 (EDT)
Received: by kanga.kvack.org (Postfix, from userid 40)
	id 870126B005D; Thu, 22 Jul 2021 00:41:05 -0400 (EDT)
X-Delivered-To: int-list-linux-mm@kvack.org
Received: by kanga.kvack.org (Postfix, from userid 63042)
	id 737466B006C; Thu, 22 Jul 2021 00:41:05 -0400 (EDT)
X-Delivered-To: linux-mm@kvack.org
Received: from forelay.hostedemail.com (smtprelay0246.hostedemail.com [216.40.44.246])
	by kanga.kvack.org (Postfix) with ESMTP id 54ACF6B0036
	for <linux-mm@kvack.org>; Thu, 22 Jul 2021 00:41:05 -0400 (EDT)
Received: from smtpin20.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251])
	by forelay03.hostedemail.com (Postfix) with ESMTP id EB6B38248076
	for <linux-mm@kvack.org>; Thu, 22 Jul 2021 04:41:04 +0000 (UTC)
X-FDA: 78388974048.20.EB5641E
Received: from foss.arm.com (foss.arm.com [217.140.110.172])
	by imf29.hostedemail.com (Postfix) with ESMTP id 41452901E59B
	for <linux-mm@kvack.org>; Thu, 22 Jul 2021 04:41:04 +0000 (UTC)
Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14])
	by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 758D0D6E;
	Wed, 21 Jul 2021 21:41:03 -0700 (PDT)
Received: from [10.163.65.134] (unknown [10.163.65.134])
	by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E845C3F66F;
	Wed, 21 Jul 2021 21:41:00 -0700 (PDT)
Subject: Re: [PATCH v3 01/12] mm/debug_vm_pgtable: Introduce struct
 pgtable_debug_args
To: Gavin Shan <gshan@redhat.com>, linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org, catalin.marinas@arm.com, will@kernel.org,
 akpm@linux-foundation.org, chuhu@redhat.com, shan.gavin@gmail.com
References: <20210719130613.334901-1-gshan@redhat.com>
 <20210719130613.334901-2-gshan@redhat.com>
 <ab0f9daa-0c49-e74c-e073-6e03a3cabb07@arm.com>
 <280a5740-b5dc-4b78-3a38-67e5adbb0afd@redhat.com>
From: Anshuman Khandual <anshuman.khandual@arm.com>
Message-ID: <04a4618f-9899-1518-cee1-0a48cb4df4c6@arm.com>
Date: Thu, 22 Jul 2021 10:11:50 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <280a5740-b5dc-4b78-3a38-67e5adbb0afd@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Authentication-Results: imf29.hostedemail.com;
	dkim=none;
	spf=pass (imf29.hostedemail.com: domain of anshuman.khandual@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=anshuman.khandual@arm.com;
	dmarc=pass (policy=none) header.from=arm.com
X-Stat-Signature: qbr4ep8jtuwqyez3q63whf99qy4yjz98
X-Rspamd-Queue-Id: 41452901E59B
X-Rspamd-Server: rspam01
X-HE-Tag: 1626928864-814157
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: <linux-mm.kvack.org>

On 7/21/21 3:50 PM, Gavin Shan wrote:
> Hi Anshuman,
>=20
> On 7/21/21 3:44 PM, Anshuman Khandual wrote:
>> On 7/19/21 6:36 PM, Gavin Shan wrote:
>>> In debug_vm_pgtable(), there are many local variables introduced to
>>> track the needed information and they are passed to the functions for
>>> various test cases. It'd better to introduce a struct as place holder
>>> for these information. With it, what the functions for various test
>>> cases need is the struct, to simplify the code. It also makes code
>>> easier to be maintained.
>>>
>>> Besides, set_xxx_at() could access the data on the corresponding page=
s
>>> in the page table modifying tests. So the accessed pages in the tests
>>> should have been allocated from buddy. Otherwise, we're accessing pag=
es
>>> that aren't owned by us. This causes issues like page flag corruption=
.
>>>
>>> This introduces "struct pgtable_debug_args". The struct is initialize=
d
>>> and destroyed, but the information in the struct isn't used yet. They
>>> will be used in subsequent patches.
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>> =C2=A0 mm/debug_vm_pgtable.c | 197 ++++++++++++++++++++++++++++++++++=
+++++++-
>>> =C2=A0 1 file changed, 196 insertions(+), 1 deletion(-)
>>>
>=20
> I saw you've finished the review on PATCH[v3 01/12] and PATCH[v3 02/12]=
.
> I will wait to integrate your comments to v4 until you finish the revie=
w
> on all patches in v3 series.
>=20
>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>>> index 1c922691aa61..ea153ff40d23 100644
>>> --- a/mm/debug_vm_pgtable.c
>>> +++ b/mm/debug_vm_pgtable.c
>>> @@ -58,6 +58,36 @@
>>> =C2=A0 #define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_=
SKIP_MASK)
>>> =C2=A0 #define RANDOM_NZVALUE=C2=A0=C2=A0=C2=A0 GENMASK(7, 0)
>>> =C2=A0 +struct pgtable_debug_args {
>>> +=C2=A0=C2=A0=C2=A0 struct mm_struct=C2=A0=C2=A0=C2=A0 *mm;
>>> +=C2=A0=C2=A0=C2=A0 struct vm_area_struct=C2=A0=C2=A0=C2=A0 *vma;
>>> +
>>> +=C2=A0=C2=A0=C2=A0 pgd_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 *pgdp;
>>> +=C2=A0=C2=A0=C2=A0 p4d_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 *p4dp;
>>> +=C2=A0=C2=A0=C2=A0 pud_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 *pudp;
>>> +=C2=A0=C2=A0=C2=A0 pmd_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 *pmdp;
>>> +=C2=A0=C2=A0=C2=A0 pte_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 *ptep;
>>> +
>>> +=C2=A0=C2=A0=C2=A0 p4d_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 *start_p4dp;
>>> +=C2=A0=C2=A0=C2=A0 pud_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 *start_pudp;
>>> +=C2=A0=C2=A0=C2=A0 pmd_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 *start_pmdp;
>>> +=C2=A0=C2=A0=C2=A0 pgtable_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
 start_ptep;
>>> +
>>> +=C2=A0=C2=A0=C2=A0 unsigned long=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 vaddr;
>>> +=C2=A0=C2=A0=C2=A0 pgprot_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
 page_prot;
>>> +=C2=A0=C2=A0=C2=A0 pgprot_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
 page_prot_none;
>>> +
>>> +=C2=A0=C2=A0=C2=A0 unsigned long=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 pud_pfn;
>>> +=C2=A0=C2=A0=C2=A0 unsigned long=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 pmd_pfn;
>>> +=C2=A0=C2=A0=C2=A0 unsigned long=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 pte_pfn;
>>> +
>>> +=C2=A0=C2=A0=C2=A0 unsigned long=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 fixed_pgd_pfn;
>>> +=C2=A0=C2=A0=C2=A0 unsigned long=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 fixed_p4d_pfn;
>>> +=C2=A0=C2=A0=C2=A0 unsigned long=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 fixed_pud_pfn;
>>> +=C2=A0=C2=A0=C2=A0 unsigned long=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 fixed_pmd_pfn;
>>> +=C2=A0=C2=A0=C2=A0 unsigned long=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 fixed_pte_pfn;
>>> +};
>>> +
>>> =C2=A0 static void __init pte_basic_tests(unsigned long pfn, int idx)
>>> =C2=A0 {
>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pgprot_t prot =3D protection_map[idx];
>>> @@ -955,8 +985,167 @@ static unsigned long __init get_random_vaddr(vo=
id)
>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return random_vaddr;
>>> =C2=A0 }
>>> =C2=A0 +static void __init destroy_args(struct pgtable_debug_args *ar=
gs)
>>> +{
>>> +=C2=A0=C2=A0=C2=A0 struct page *page =3D NULL;
>>> +
>>> +=C2=A0=C2=A0=C2=A0 /* Free (huge) page */
>>> +=C2=A0=C2=A0=C2=A0 if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 IS_ENABLED(CONFIG_HAVE_AR=
CH_TRANSPARENT_HUGEPAGE_PUD) &&
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 has_transparent_hugepage(=
) &&
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 args->pud_pfn !=3D ULONG_=
MAX) {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 page =3D pfn_to_page(args=
->pud_pfn);
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __free_pages(page, HPAGE_=
PUD_SHIFT - PAGE_SHIFT);
>>> +=C2=A0=C2=A0=C2=A0 } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE=
) &&
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 has_tra=
nsparent_hugepage() &&
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 args->p=
md_pfn !=3D ULONG_MAX) {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 page =3D pfn_to_page(args=
->pmd_pfn);
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __free_pages(page, HPAGE_=
PMD_ORDER);
>>> +=C2=A0=C2=A0=C2=A0 } else if (args->pte_pfn !=3D ULONG_MAX) {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 page =3D pfn_to_page(args=
->pte_pfn);
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __free_pages(page, 0);
>>> +=C2=A0=C2=A0=C2=A0 }
>>> +
>>> +=C2=A0=C2=A0=C2=A0 /* Free page table */
>>> +=C2=A0=C2=A0=C2=A0 if (args->start_ptep) {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pte_free(args->mm, args->=
start_ptep);
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mm_dec_nr_ptes(args->mm);
>>> +=C2=A0=C2=A0=C2=A0 }
>>> +
>>> +=C2=A0=C2=A0=C2=A0 if (args->start_pmdp) {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pmd_free(args->mm, args->=
start_pmdp);
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mm_dec_nr_pmds(args->mm);
>>> +=C2=A0=C2=A0=C2=A0 }
>>> +
>>> +=C2=A0=C2=A0=C2=A0 if (args->start_pudp) {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pud_free(args->mm, args->=
start_pudp);
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mm_dec_nr_puds(args->mm);
>>> +=C2=A0=C2=A0=C2=A0 }
>>> +
>>> +=C2=A0=C2=A0=C2=A0 if (args->start_p4dp)
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 p4d_free(args->mm, args->=
p4dp);
>>> +
>>> +=C2=A0=C2=A0=C2=A0 /* Free vma and mm struct */
>>> +=C2=A0=C2=A0=C2=A0 if (args->vma)
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 vm_area_free(args->vma);
>>> +=C2=A0=C2=A0=C2=A0 if (args->mm)
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mmdrop(args->mm);
>>> +}
>>> +
>>> +static int __init init_args(struct pgtable_debug_args *args)
>>> +{
>>> +=C2=A0=C2=A0=C2=A0 struct page *page =3D NULL;
>>> +=C2=A0=C2=A0=C2=A0 phys_addr_t phys;
>>> +=C2=A0=C2=A0=C2=A0 int ret =3D 0;
>>> +
>>> +=C2=A0=C2=A0=C2=A0 /* Initialize the debugging data */
>>> +=C2=A0=C2=A0=C2=A0 memset(args, 0, sizeof(*args));
>>> +=C2=A0=C2=A0=C2=A0 args->page_prot=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D=
 vm_get_page_prot(VMFLAGS);
>>> +=C2=A0=C2=A0=C2=A0 args->page_prot_none =3D __P000;
>>
>> Please preserve the existing comments before this assignment.
>>
>> =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 * __P000 (or ev=
en __S000) will help create page table entries with
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * PROT_NONE per=
mission as required for pxx_protnone_tests().
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
>>
>=20
> Sure. I will combine the comments in v4 as below:
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0/*
> =C2=A0=C2=A0=C2=A0=C2=A0 * Initialize the debugging arguments.
> =C2=A0=C2=A0=C2=A0=C2=A0 *
> =C2=A0=C2=A0=C2=A0=C2=A0 * __P000 (or even __S000) will help create pag=
e table entries with
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * PROT_NONE permission=
 as required for pxx_protnone_tests().
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
>=20
>=20
>>> +=C2=A0=C2=A0=C2=A0 args->pud_pfn=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 =3D ULONG_MAX;
>>> +=C2=A0=C2=A0=C2=A0 args->pmd_pfn=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 =3D ULONG_MAX;
>>> +=C2=A0=C2=A0=C2=A0 args->pte_pfn=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 =3D ULONG_MAX;
>>> +=C2=A0=C2=A0=C2=A0 args->fixed_pgd_pfn=C2=A0 =3D ULONG_MAX;
>>> +=C2=A0=C2=A0=C2=A0 args->fixed_p4d_pfn=C2=A0 =3D ULONG_MAX;
>>> +=C2=A0=C2=A0=C2=A0 args->fixed_pud_pfn=C2=A0 =3D ULONG_MAX;
>>> +=C2=A0=C2=A0=C2=A0 args->fixed_pmd_pfn=C2=A0 =3D ULONG_MAX;
>>> +=C2=A0=C2=A0=C2=A0 args->fixed_pte_pfn=C2=A0 =3D ULONG_MAX;
>>> +
>>> +=C2=A0=C2=A0=C2=A0 /* Allocate mm and vma */
>>> +=C2=A0=C2=A0=C2=A0 args->mm =3D mm_alloc();
>>> +=C2=A0=C2=A0=C2=A0 if (!args->mm) {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_err("Failed to allocat=
e mm struct\n");
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D -ENOMEM;
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto error;
>>> +=C2=A0=C2=A0=C2=A0 }
>>> +
>>> +=C2=A0=C2=A0=C2=A0 args->vma =3D vm_area_alloc(args->mm);
>>> +=C2=A0=C2=A0=C2=A0 if (!args->vma) {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_err("Failed to allocat=
e vma\n");
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D -ENOMEM;
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto error;
>>> +=C2=A0=C2=A0=C2=A0 }
>>> +
>>> +=C2=A0=C2=A0=C2=A0 /* Figure out the virtual address and allocate pa=
ge table entries */
>>> +=C2=A0=C2=A0=C2=A0 args->vaddr =3D get_random_vaddr();
>>
>> Please group args->vaddr's init with page_prot and page_prot_none abov=
e.
>>
>=20
> Yes, It will make the code tidy. I'll move this line accordingly in v4,
> but the related comments will be dropped as the code is self-explanator=
y.
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Allocate page table entri=
es */
>=20
>>> +=C2=A0=C2=A0=C2=A0 args->pgdp =3D pgd_offset(args->mm, args->vaddr);
>>> +=C2=A0=C2=A0=C2=A0 args->p4dp =3D p4d_alloc(args->mm, args->pgdp, ar=
gs->vaddr);
>>> +=C2=A0=C2=A0=C2=A0 args->pudp =3D args->p4dp ?
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 pud_alloc(args->mm, args->p4dp, args->vaddr) : NULL;
>>> +=C2=A0=C2=A0=C2=A0 args->pmdp =3D args->pudp ?
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 pmd_alloc(args->mm, args->pudp, args->vaddr) : NULL;
>>> +=C2=A0=C2=A0=C2=A0 args->ptep =3D args->pmdp ?
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 pte_alloc_map(args->mm, args->pmdp, args->vaddr) : NULL;
>>> +=C2=A0=C2=A0=C2=A0 if (!args->ptep) {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_err("Failed to allocat=
e page table\n");
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D -ENOMEM;
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto error;
>>> +=C2=A0=C2=A0=C2=A0 }
>>
>> Why not just assert that all page table level pointers are allocated
>> successfully, otherwise bail out the test completely. Something like
>> this at each level.
>>
>> =C2=A0=C2=A0=C2=A0=C2=A0if (!args->p4dp) {
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_err("Failed to allocate =
page table\n");
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D -ENOMEM;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto error;
>> =C2=A0=C2=A0=C2=A0=C2=A0}
>>
>> Is there any value in proceeding with the test when some page table
>> pointers have not been allocated. Also individual tests do not cross
>> check these pointers. Also asserting successful allocations will
>> make the freeing path simpler, as I had mentioned earlier.
>>
>=20
> There is no tests will be carried out if we fail to allocate any level
> of page table entries. For other questions, please refer below response=
.
> In summary, this snippet needs to be combined with next snippet, as bel=
ow.
>=20
>>> +
>>> +=C2=A0=C2=A0=C2=A0 /*
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * The above page table entries will be modi=
fied. Lets save the
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * page table entries so that they can be re=
leased when the tests
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * are completed.
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 */
>>> +=C2=A0=C2=A0=C2=A0 args->start_p4dp =3D p4d_offset(args->pgdp, 0UL);
>>> +=C2=A0=C2=A0=C2=A0 args->start_pudp =3D pud_offset(args->p4dp, 0UL);
>>> +=C2=A0=C2=A0=C2=A0 args->start_pmdp =3D pmd_offset(args->pudp, 0UL);
>>> +=C2=A0=C2=A0=C2=A0 args->start_ptep =3D pmd_pgtable(READ_ONCE(*(args=
->pmdp)));
>>
>> If the above page table pointers have been validated to be allocated
>> successfully, we could add these here.
>>
>> =C2=A0=C2=A0=C2=A0=C2=A0WARN_ON(!args->start_p4dp)
>> =C2=A0=C2=A0=C2=A0=C2=A0WARN_ON(!args->start_pudp)
>> =C2=A0=C2=A0=C2=A0=C2=A0WARN_ON(!args->start_pmdp)
>> =C2=A0=C2=A0=C2=A0=C2=A0WARN_ON(!args->start_ptep)
>>
>> Afterwards all those if (args->start_pxdp) checks in the freeing path
>> will not be required anymore.
>>
>=20
> The check on @args->start_pxdp is still needed in destroy_args() for
> couple of cases: (1) destroy_args() is called on failing to allocate
> @args->mm or @args->vma. That time, no page table entries are allocated=
.
> (2) It's possible to fail allocating current level of page table entrie=
s
> even the previous levels of page table entries are allocated successful=
ly.

This makes sense as destroy_args() is getting called if any of these
allocations fails during init_args(). Did not realize that earlier.

>=20
> So Lets change these (above) two snippets as below in v4:
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0/*
> =C2=A0=C2=A0=C2=A0=C2=A0 * Allocate page table entries. The allocated p=
age table entries
> =C2=A0=C2=A0=C2=A0=C2=A0 * will be modified in the tests. Lets save the=
 page table entries
> =C2=A0=C2=A0=C2=A0=C2=A0 * so that they can be released when the tests =
are completed.
> =C2=A0=C2=A0=C2=A0=C2=A0 */
> =C2=A0=C2=A0=C2=A0=C2=A0args->pgdp =3D pgd_offset(args->mm, args->vaddr=
);
> =C2=A0=C2=A0=C2=A0=C2=A0args->p4dp =3D p4d_alloc(args->mm, args->pgdp, =
args->vaddr);
> =C2=A0=C2=A0=C2=A0=C2=A0if (!args->p4dp) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_err("Failed to allocate p=
4d entries\n");
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D -ENOMEM;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto error;
> =C2=A0=C2=A0=C2=A0=C2=A0}
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0args->start_p4dp =3D p4d_offset(args->pgdp, 0UL=
);

Dont bring the arg->start_pxdp assignments here. If all page table level
pointer allocations succeed, they all get assigned together like we have
right now. Although a sanity check afterwards like the following, might
still be better.

WARN_ON(!args->start_p4dp)
WARN_ON(!args->start_pudp)
WARN_ON(!args->start_pmdp)
WARN_ON(!args->start_ptep)

> =C2=A0=C2=A0=C2=A0=C2=A0args->pudp =3D pud_alloc(args->mm, args->p4dp, =
args->vaddr);
> =C2=A0=C2=A0=C2=A0=C2=A0if (!args->pudp) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_err("Failed to allocate p=
ud entries\n");
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D -ENOMEM;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto error;
> =C2=A0=C2=A0=C2=A0=C2=A0}
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0args->pmdp =3D pmd_alloc(args->mm, args->pudp, =
args->vaddr);
> =C2=A0=C2=A0=C2=A0=C2=A0if (!args->pmdp) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_err("Failed to allocate P=
MD entries\n");
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D -ENOMEM;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto error;
> =C2=A0=C2=A0=C2=A0=C2=A0}
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0args->start_pmdp =3D pmd_offset(args->pudp, 0UL=
);
> =C2=A0=C2=A0=C2=A0=C2=A0args->ptep =3D pte_alloc_map(args->mm, args->pm=
dp, args->vaddr);
> =C2=A0=C2=A0=C2=A0=C2=A0if (!args->ptep) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_err("Failed to allocate p=
age table\n");
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ret =3D -ENOMEM;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto error;
> =C2=A0=C2=A0=C2=A0=C2=A0}
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0args->start_ptep =3D pmd_pgtable(READ_ONCE(*(ar=
gs->pmdp)));
>=20
>>> +
>>> +=C2=A0=C2=A0=C2=A0 /*
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * Figure out the fixed addresses, which are=
 all around the kernel
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * symbol (@start_kernel). The corresponding=
 PFNs might be invalid,
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * but it's fine as the following tests won'=
t access the pages.
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 */
>>> +=C2=A0=C2=A0=C2=A0 phys =3D __pa_symbol(&start_kernel);
>>> +=C2=A0=C2=A0=C2=A0 args->fixed_pgd_pfn =3D __phys_to_pfn(phys & PGDI=
R_MASK);
>>> +=C2=A0=C2=A0=C2=A0 args->fixed_p4d_pfn =3D __phys_to_pfn(phys & P4D_=
MASK);
>>> +=C2=A0=C2=A0=C2=A0 args->fixed_pud_pfn =3D __phys_to_pfn(phys & PUD_=
MASK);
>>> +=C2=A0=C2=A0=C2=A0 args->fixed_pmd_pfn =3D __phys_to_pfn(phys & PMD_=
MASK);
>>> +=C2=A0=C2=A0=C2=A0 args->fixed_pte_pfn =3D __phys_to_pfn(phys & PAGE=
_MASK);
>>> +
>>> +=C2=A0=C2=A0=C2=A0 /*
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * Allocate (huge) pages because some of the=
 tests need to access
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * the data in the pages. The corresponding =
tests will be skipped
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 * if we fail to allocate (huge) pages.
>>> +=C2=A0=C2=A0=C2=A0=C2=A0 */
>>> +=C2=A0=C2=A0=C2=A0 if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 IS_ENABLED(CONFIG_HAVE_AR=
CH_TRANSPARENT_HUGEPAGE_PUD) &&
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 has_transparent_hugepage(=
)) {
>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 page =3D alloc_pages(GFP_=
KERNEL | __GFP_NOWARN,
>>> +=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 HPAGE_PUD_SHIFT - PAGE_SHIFT);
>>
>> Please drop __GFP_NOWARN and instead use something like alloc_contig_p=
ages()
>> when required allocation order exceed (MAX_ORDER - 1). Else the test m=
ight
>> not be able to execute on platform configurations, where PUD THP is en=
abled.
>>
>=20
> Yes, It's correct that alloc_contig_pages() should be used here, depend=
ing
> on CONFIG_CONTIG_ALLOC. Otherwise, alloc_pages(...__GFP_NOWARN...) is s=
till
> used as we're doing. This snippet will be changed like below in v4:

First 'order > (MAX_ORDER - 1)' needs to be established before calling in=
to
alloc_contig_pages() without __GFP_NOWARN and set a new flag indicating t=
hat
there is contig page allocated. But if 'order <=3D (MAX_ORDER - 1)', then=
 call
alloc_pages(..) without  __GFP_NOWARN. There is no need to add  __GFP_NOW=
ARN
in any case. In case CONFIG_CONTIG_ALLOC is not available, directly retur=
n a
NULL as that would have been the case with alloc_pages(...__GFP_NOWARN...=
) as
well.

Symbol alloc_contig_pages() is not available outside CONFIG_CONTIG_ALLOC.=
 So
IS_ENABLED() construct will not work, unless there is an empty stub added=
 in
the header. Otherwise #ifdef CONFIG_CONTIG_ALLOC needs to be used instead=
.

Regardless please do test this on a x86 platform with PUD based THP in or=
der
to make sure every thing works as expected.

>=20
> =C2=A0=C2=A0=C2=A0=C2=A0/*
> =C2=A0=C2=A0=C2=A0=C2=A0 * Allocate (huge) pages because some of the te=
sts need to access
> =C2=A0=C2=A0=C2=A0=C2=A0 * the data in the pages. The corresponding tes=
ts will be skipped
> =C2=A0=C2=A0=C2=A0=C2=A0 * if we fail to allocate (huge) pages.
> =C2=A0=C2=A0=C2=A0=C2=A0 */
> =C2=A0=C2=A0=C2=A0=C2=A0if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 IS_ENABLED(CONFIG_HAVE_ARCH_=
TRANSPARENT_HUGEPAGE_PUD) &&
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 IS_ENABLED(CONFIG_CONTIG_ALL=
OC)) &&
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 has_transparent_hugepage()) =
{
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 page =3D alloc_contig_pages(=
(1 << (HPAGE_PUD_SHIFT - PAGE_SHIFT)),
> =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 GFP_KERNEL | __GFP=
_NOWARN,
> =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 first_online_node,=
 NULL);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (page) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 args=
->is_contiguous_pud_page =3D true;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 args=
->pud_pfn =3D page_to_pfn(page);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 args=
->pmd_pfn =3D args->pud_pfn;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 args=
->pte_pfn =3D args->pud_pfn;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 retu=
rn 0;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
> =C2=A0=C2=A0=C2=A0=C2=A0}
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 IS_ENABLED(CONFIG_HAVE_ARCH_=
TRANSPARENT_HUGEPAGE_PUD) &&
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 has_transparent_hugepage()) =
{
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 page =3D alloc_pages(GFP_KER=
NEL | __GFP_NOWARN,
> =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 HPAGE_PUD_SHIFT - PAGE_SHIFT);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (page) {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 args=
->is_contiguous_pud_page =3D false;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 args=
->pud_pfn =3D page_to_pfn(page);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 args=
->pmd_pfn =3D args->pud_pfn;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 args=
->pte_pfn =3D args->pud_pfn;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 retu=
rn 0;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
> =C2=A0=C2=A0=C2=A0=C2=A0}
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0[... The logic to allocate PMD huge page or pag=
e is kept as of being]

IIRC it is also not guaranteed that PMD_SHIFT <=3D (MAX_ORDER - 1). Hence
this same scheme should be followed for PMD level allocation as well.

> =C2=A0=C2=A0=C2=A0 [... The code to release the PUD huge page needs cha=
nges based on @args->is_contiguous_pud_page]

Right, a flag would be needed to call the appropriate free function.