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=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 7B63FC11F67 for ; Wed, 14 Jul 2021 05:25:28 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 113D761363 for ; Wed, 14 Jul 2021 05:25:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 113D761363 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 D9F5C6B0088; Wed, 14 Jul 2021 01:25:27 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D50466B008C; Wed, 14 Jul 2021 01:25:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BC9B46B0092; Wed, 14 Jul 2021 01:25:27 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0033.hostedemail.com [216.40.44.33]) by kanga.kvack.org (Postfix) with ESMTP id 915F36B0088 for ; Wed, 14 Jul 2021 01:25:27 -0400 (EDT) Received: from smtpin39.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 8E9842BFA0 for ; Wed, 14 Jul 2021 05:25:26 +0000 (UTC) X-FDA: 78360055452.39.4AC10F8 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf24.hostedemail.com (Postfix) with ESMTP id BC9AAB00009E for ; Wed, 14 Jul 2021 05:25:25 +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 CF72B6D; Tue, 13 Jul 2021 22:25:24 -0700 (PDT) Received: from [10.163.65.222] (unknown [10.163.65.222]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 89AF83F694; Tue, 13 Jul 2021 22:25:22 -0700 (PDT) Subject: Re: [PATCH 00/12] mm/debug_vm_pgtable: Enhancements To: Gavin Shan , linux-mm@kvack.org Cc: linux-kernel@vger.kernel.org, catalin.marinas@arm.com, will@kernel.org, akpm@linux-foundation.org, shan.gavin@gmail.com, chuhu@redhat.com References: <20210706061748.161258-1-gshan@redhat.com> <42a26202-10f7-e744-3fc5-c9e5a7445193@arm.com> From: Anshuman Khandual Message-ID: <30a938df-f5c8-910c-8ddc-52a2d2a0a11e@arm.com> Date: Wed, 14 Jul 2021 10:56:09 +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: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Authentication-Results: imf24.hostedemail.com; dkim=none; spf=pass (imf24.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-Rspamd-Server: rspam02 X-Stat-Signature: ukbq3mhi9k5mbt77h6r5odtwi61by7e3 X-Rspamd-Queue-Id: BC9AAB00009E X-HE-Tag: 1626240325-944439 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 7/13/21 6:50 AM, Gavin Shan wrote: > Hi Anshuman, >=20 > On 7/12/21 2:14 PM, Anshuman Khandual wrote: >> Though I have not jumped into the details for all individual >> patches here but still have some high level questions below. >> >> On 7/6/21 11:47 AM, Gavin Shan wrote: >>> There are couple of issues with current implementations and this seri= es >>> tries to resolve the issues: >>> >>> =C2=A0=C2=A0 (a) All needed information are scattered in variables, p= assed to various >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 test functions. The code is orga= nized in pretty much relaxed fashion. >> All these variables are first prepared in debug_vm_pgtable(), before >> getting passed into respective individual test functions. Also these >> test functions receive only the required number of variables not all. >> Adding a structure that captures all test parameters at once before >> passing them down will be unnecessary. I am still wondering what will >> be the real benefit of this large code churn ? >> >=20 > Thanks for your review. There are couple of reasons to have "struct vm_= pgtable_debug". >=20 > (1) With the struct, the old and new implementation can coexist. In thi= s way, > =C2=A0=C2=A0=C2=A0 the patches in this series can be stacked up easily. Makes sense. > (2) I think passing single struct to individual test functions improves= the > =C2=A0=C2=A0=C2=A0 code readability. Besides, it also makes the empty s= tubs simplified. Empty stub simplified - reduced argument set in the empty stubs ? > (3) The code can be extended easily if we need in future. Agreed. >=20 >>> >>> =C2=A0=C2=A0 (b) The page isn't allocated from buddy during page tabl= e entry modifying >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 tests. The page can be invalid, = conflicting to the implementations >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 of set_{pud, pmd, pte}_at() on A= RM64. The target page is accessed >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 so that the iCache can be flushe= d when execution permission is given >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 on ARM64. Besides, the target pa= ge can be unmapped and access to >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 it causes kernel crash. >> >> Using 'start_kernel' based method for struct page usage, enabled this >> test to run on platforms which might not have enough memory required >> for various individual test functions. This method is not a problem fo= r >> tests that just need an aligned pfn (which creates a page table entry) >> not a real struct page. >> >> But not allocating and owning the struct page might be problematic for >> tests that expect a real struct page and transform its state via set_ >> {pud, pmd, pte}_at() functions as reported here. >> >=20 > Yeah, I totally agree. The series follows what you explained: Except th= e > test cases where set_{pud, pmd, pte}_at() is used, the allocated page > is used. For other test cases, 'start_kernel' based PFN is used as befo= re. >=20 >>> >>> "struct vm_pgtable_debug" is introduced to address issue (a). For iss= ue >>> (b), the used page is allocated from buddy in page table entry modify= ing >>> tests. The corresponding tets will be skipped if we fail to allocate = the >>> (huge) page. For other test cases, the original page around to kernel >>> symbol (@start_kernel) is still used. >> >> For all basic pfn requiring tests, existing 'start_kernel' based metho= d >> should continue but allocate a struct page for other tests which chang= e >> the passed struct page. Skipping the tests when allocation fails is th= e >> right thing to do. >> >=20 > Yes, it's exactly what this series does. Hope you can jump into the det= ails > when you get a chance :) I have already started looking into the series. But still wondering if the huge page memory allocation change and the arm64 specific page fix should be completed first, before getting into the new structure based arguments (in a separate series). Although the end result would still remain the same, the transition there would be better I guess. Do you see any challenges in achieving that ? - Anshuman