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.1 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,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 B5C43C47E48 for ; Thu, 15 Jul 2021 05:17:45 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 6501861360 for ; Thu, 15 Jul 2021 05:17:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6501861360 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B4A298D0081; Thu, 15 Jul 2021 01:17:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B20368D0065; Thu, 15 Jul 2021 01:17:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9C1288D0081; Thu, 15 Jul 2021 01:17:45 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0179.hostedemail.com [216.40.44.179]) by kanga.kvack.org (Postfix) with ESMTP id 764E18D0065 for ; Thu, 15 Jul 2021 01:17:45 -0400 (EDT) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 5CD65184B609A for ; Thu, 15 Jul 2021 05:17:44 +0000 (UTC) X-FDA: 78363664848.09.E39E390 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by imf30.hostedemail.com (Postfix) with ESMTP id 03D18E0016B0 for ; Thu, 15 Jul 2021 05:17:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1626326263; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=nrxR9YJIRDun9r5WDjFAvm6/X83B6W+94/3z0nnD2QQ=; b=STx1ymHFpQDSDkoVphDiiLVy931wqAW4h7z7kNBH2kvdc/qRVRS78sxcB8MI8Xlp3nW219 JYHhqVI+8TS5tW7nXeS8AbXJhbuLRm9UckRRRjGYFqz+UhYxMqJwBGHfM+szCzmUIaRlsy dVwBT7WnvQTojjhptzI3Qc+9eh1sRoY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-541-HZsmSFz7P76Jwfa3DWQxIw-1; Thu, 15 Jul 2021 01:17:39 -0400 X-MC-Unique: HZsmSFz7P76Jwfa3DWQxIw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id EAA30802E61; Thu, 15 Jul 2021 05:17:37 +0000 (UTC) Received: from [10.64.54.195] (vpn2-54-195.bne.redhat.com [10.64.54.195]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D528A60854; Thu, 15 Jul 2021 05:17:34 +0000 (UTC) Reply-To: Gavin Shan Subject: Re: [PATCH 00/12] mm/debug_vm_pgtable: Enhancements To: Anshuman Khandual , 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> <30a938df-f5c8-910c-8ddc-52a2d2a0a11e@arm.com> From: Gavin Shan Message-ID: Date: Thu, 15 Jul 2021 15:17:53 +1000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.0 MIME-Version: 1.0 In-Reply-To: <30a938df-f5c8-910c-8ddc-52a2d2a0a11e@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=STx1ymHF; spf=none (imf30.hostedemail.com: domain of gshan@redhat.com has no SPF policy when checking 216.205.24.124) smtp.mailfrom=gshan@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-Stat-Signature: oyptitkxsqtykw8bq5739wxbdci3zuui X-Rspamd-Queue-Id: 03D18E0016B0 X-Rspamd-Server: rspam01 X-HE-Tag: 1626326263-89957 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: Hi Anshuman, On 7/14/21 3:26 PM, Anshuman Khandual wrote: > On 7/13/21 6:50 AM, Gavin Shan wrote: >> 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 ser= ies >>>> tries to resolve the issues: >>>> >>>> =C2=A0=C2=A0 (a) All needed information are scattered in variables,= passed to various >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 test functions. The code is or= ganized 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 ? >>> >> >> Thanks for your review. There are couple of reasons to have "struct vm= _pgtable_debug". >> >> (1) With the struct, the old and new implementation can coexist. In th= is way, >> =C2=A0=C2=A0=C2=A0 the patches in this series can be stacked up easil= y. >=20 > Makes sense. >=20 >> (2) I think passing single struct to individual test functions improve= s the >> =C2=A0=C2=A0=C2=A0 code readability. Besides, it also makes the empty= stubs simplified. >=20 > Empty stub simplified - reduced argument set in the empty stubs ? >=20 Yes. >> (3) The code can be extended easily if we need in future. >=20 > Agreed. >=20 >> >>>> >>>> =C2=A0=C2=A0 (b) The page isn't allocated from buddy during page ta= ble 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= ARM64. The target page is accessed >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 so that the iCache can be flus= hed when execution permission is given >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 on ARM64. Besides, the target = page 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 f= or >>> 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 fo= r >>> tests that expect a real struct page and transform its state via set_ >>> {pud, pmd, pte}_at() functions as reported here. >>> >> >> Yeah, I totally agree. The series follows what you explained: Except t= he >> 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 bef= ore. >> >>>> >>>> "struct vm_pgtable_debug" is introduced to address issue (a). For is= sue >>>> (b), the used page is allocated from buddy in page table entry modif= ying >>>> tests. The corresponding tets will be skipped if we fail to allocate= the >>>> (huge) page. For other test cases, the original page around to kerne= l >>>> symbol (@start_kernel) is still used. >>> >>> For all basic pfn requiring tests, existing 'start_kernel' based meth= od >>> should continue but allocate a struct page for other tests which chan= ge >>> the passed struct page. Skipping the tests when allocation fails is t= he >>> right thing to do. >>> >> >> Yes, it's exactly what this series does. Hope you can jump into the de= tails >> when you get a chance :) >=20 > 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 ? >=20 Thanks for your time to review in details. As I can understand, the reaso= n to have the fix for easy backporting to stable-kernel and I didn't do tha= t because of couple of facts: (1) The changes included in this series only affects only one source file, so backporting the whole series isn't hard. (2) There will be more redundant code if we include the fix before switch= ing to "struct vm_pgtable_debug". It's unnecessary. So lets keep the patch layout we had if you agree. Actually, the issues a= re found during the testing with RHEL downstream kernel. Once it's settled d= own, I will backport the whole series to RHEL downstream kernel. Thanks, Gavin