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 A3751C433F5 for ; Tue, 19 Apr 2022 08:52:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9CACC6B00C9; Tue, 19 Apr 2022 04:52:49 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 954858D0047; Tue, 19 Apr 2022 04:52:49 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7CC646B00CC; Tue, 19 Apr 2022 04:52:49 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.hostedemail.com [64.99.140.26]) by kanga.kvack.org (Postfix) with ESMTP id 665CF6B00C9 for ; Tue, 19 Apr 2022 04:52:49 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 1B06716EC for ; Tue, 19 Apr 2022 08:52:49 +0000 (UTC) X-FDA: 79373013258.10.D46AB4B Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by imf07.hostedemail.com (Postfix) with ESMTP id ECA604000E for ; Tue, 19 Apr 2022 08:52:47 +0000 (UTC) Received: from kwepemi500024.china.huawei.com (unknown [172.30.72.57]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4KjHc014LhzfZDR; Tue, 19 Apr 2022 16:52:00 +0800 (CST) Received: from kwepemm600017.china.huawei.com (7.193.23.234) by kwepemi500024.china.huawei.com (7.221.188.100) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Tue, 19 Apr 2022 16:52:43 +0800 Received: from [10.174.179.234] (10.174.179.234) by kwepemm600017.china.huawei.com (7.193.23.234) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Tue, 19 Apr 2022 16:52:42 +0800 Message-ID: Date: Tue, 19 Apr 2022 16:52:41 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH -next v4 3/4] arm64: mm: add support for page table check To: Anshuman Khandual , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , , "H. Peter Anvin" , Pasha Tatashin , Andrew Morton , Catalin Marinas , Will Deacon , Paul Walmsley , Palmer Dabbelt , Albert Ou CC: , , , , Kefeng Wang , Guohanjun References: <20220418034444.520928-1-tongtiangen@huawei.com> <20220418034444.520928-4-tongtiangen@huawei.com> <1c314feb-cd78-2bb3-462e-4ea3cefe122e@arm.com> <88a70fa7-b1c8-6f5c-1018-df673949785c@arm.com> From: Tong Tiangen In-Reply-To: <88a70fa7-b1c8-6f5c-1018-df673949785c@arm.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.174.179.234] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To kwepemm600017.china.huawei.com (7.193.23.234) X-CFilter-Loop: Reflected X-Stat-Signature: xadqsobhnhbrhuxpsj3b5pymq4se9fan X-Rspam-User: Authentication-Results: imf07.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf07.hostedemail.com: domain of tongtiangen@huawei.com designates 45.249.212.187 as permitted sender) smtp.mailfrom=tongtiangen@huawei.com X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: ECA604000E X-HE-Tag: 1650358367-859746 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: 在 2022/4/19 15:10, Anshuman Khandual 写道: > > > On 4/18/22 21:17, Tong Tiangen wrote: >> >> >> 在 2022/4/18 17:28, Anshuman Khandual 写道: >>> On 4/18/22 09:14, Tong Tiangen wrote: >>>> From: Kefeng Wang [...] >>> >>> Could you explain what was expected during pmdp_collapse_flush() which when >>> failed, triggered this BUG_ON() ? This counter seems to be page table check >>> specific, could it just go wrong ? I have not looked into the details about >>> page table check mechanism. >>> >>> - Anshuman >>> . >> >> Hi Anshuman: >> >> Thanks for your job. >> >> Let me briefly explain the principle of page table check(PTC). >> >> PTC introduces the following struct for page mapping type count: >> struct page_table_check { >>         atomic_t anon_map_count; >>         atomic_t file_map_count; >> }; >> This structure can be obtained by "lookup_page_ext(page)" > > > Right. > >> >> When page table entries are set(pud/pmd/pte), page_table_check_set()  is called to increase the page mapping count, Also check for errors (eg:if a page is used for anonymous mapping, then the page cannot be used for file mapping at the same time). >> >> When page table entries are clear(pud/pmd/pte), page_table_check_clear()  is called to decrease the page mapping count, Also check for errors. >> >> The error check rules are described in the following documents: Documentation/vm/page_table_check.rst > > Snippet from that document. > > +-------------------+-------------------+-------------------+------------------+ > | Current Mapping | New mapping | Permissions | Rule | > +===================+===================+===================+==================+ > | Anonymous | Anonymous | Read | Allow | > +-------------------+-------------------+-------------------+------------------+ > | Anonymous | Anonymous | Read / Write | Prohibit | > +-------------------+-------------------+-------------------+------------------+ > | Anonymous | Named | Any | Prohibit | > +-------------------+-------------------+-------------------+------------------+ > | Named | Anonymous | Any | Prohibit | > +-------------------+-------------------+-------------------+------------------+ > | Named | Named | Any | Allow | > +-------------------+-------------------+-------------------+------------------+ > > Does 'Named' refer to file mapping ? Also what does 'Prohibit' imply here ? The > check will call out a BUG_ON() in such cases ? Right， Named means file mapping, Prohibit here trigger BUG_ON. > > page_table_check_clear() > { > > if (anon) { > BUG_ON(atomic_read(&ptc->file_map_count)); > BUG_ON(atomic_dec_return(&ptc->anon_map_count) < 0); > } else { > BUG_ON(atomic_read(&ptc->anon_map_count)); > BUG_ON(atomic_dec_return(&ptc->file_map_count) < 0); > } > } > > So in the clear path, there are two checks > > - If the current mapping is Anon, file_map_count cannot be positive and other way > - Decrement the applicable counter ensuring that it does not turn negative > > page_table_check_set() > { > if (anon) { > BUG_ON(atomic_read(&ptc->file_map_count)); > BUG_ON(atomic_inc_return(&ptc->anon_map_count) > 1 && rw); > } else { > BUG_ON(atomic_read(&ptc->anon_map_count)); > BUG_ON(atomic_inc_return(&ptc->file_map_count) < 0); > } > } > > So in the set path, there are two checks > > - If the current mapping is anon, file_map_count cannot be positive and other way > - Anon mapping cannot be RW if the page has been mapped more than once > - But then why check for negative values for file_map_count after increment ? Check for negative after increment is logically OK and <=0 should be more reasonable. > > Is there any other checks, which this test ensures, that I might be missing ? The following checks are performed when page table entry are allocated/released: __page_table_check_zero() { BUG_ON(atomic_read(&ptc->anon_map_count)); BUG_ON(atomic_read(&ptc->file_map_count)); } > >> >> The setting and clearing of page table entries are symmetrical. > > This assumption should be true for any user accessible mapping, for this test to work ? Right, if not, here is BUG_ON. However, as Pasha said: "this being new on ARM64, it is possible that the bug is in PTC/khugepaged itself." > > Also why PUD_PAGE_SIZE/PMD_PAGE_SIZE are being used here instead of directly using > generic macros such as PUD_SIZE/PMD_SIZE ? Is there a specific reason ? I did code optimization for this, in patch 1/4 of this patchset: +#ifndef PMD_PAGE_SIZE +#define PMD_PAGE_SIZE PMD_SIZE +#endif + +#ifndef PUD_PAGE_SIZE +#define PUD_PAGE_SIZE PUD_SIZE +#endif Thank you. Tong. > >> >> Here __page_table_check_pmd_clear() trigger BUGON which indicates that the pmd entry file mapping count has become negative. >> >> I guess if PTC didn't detect this exception, would there have been any problems? > > I am looking into this, not sure for now. > .