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 54006C43334 for ; Wed, 8 Jun 2022 12:17:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7299C6B009C; Wed, 8 Jun 2022 08:17:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6D7ED6B009E; Wed, 8 Jun 2022 08:17:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5C7156B00A2; Wed, 8 Jun 2022 08:17:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 4EECF6B009C for ; Wed, 8 Jun 2022 08:17:41 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay12.hostedemail.com (Postfix) with ESMTP id 14D17120FCB for ; Wed, 8 Jun 2022 12:17:41 +0000 (UTC) X-FDA: 79554969522.16.745A079 Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by imf11.hostedemail.com (Postfix) with ESMTP id 028A740050 for ; Wed, 8 Jun 2022 12:17:39 +0000 (UTC) Received: from canpemm500002.china.huawei.com (unknown [172.30.72.55]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4LJ5m24yFhzgYBN; Wed, 8 Jun 2022 20:15:46 +0800 (CST) Received: from [10.174.177.76] (10.174.177.76) by canpemm500002.china.huawei.com (7.192.104.244) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Wed, 8 Jun 2022 20:17:35 +0800 Subject: Re: [PATCH] mm/page_alloc: make calling prep_compound_head more reliable To: Joao Martins , Andrew Morton CC: , , Dan Williams References: <20220607144157.36411-1-linmiaohe@huawei.com> <20220607113257.84b1bdd993f19be26b8c4944@linux-foundation.org> <65e5da9c-32d1-17d7-d8c6-96cbfac23fec@oracle.com> From: Miaohe Lin Message-ID: <4a30f026-789a-9235-2fbd-f553e4d7b45d@huawei.com> Date: Wed, 8 Jun 2022 20:17:35 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <65e5da9c-32d1-17d7-d8c6-96cbfac23fec@oracle.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.174.177.76] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To canpemm500002.china.huawei.com (7.192.104.244) X-CFilter-Loop: Reflected X-Stat-Signature: h95s5juupg4i4r6scjnbgz8gtporg89w Authentication-Results: imf11.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf11.hostedemail.com: domain of linmiaohe@huawei.com designates 45.249.212.187 as permitted sender) smtp.mailfrom=linmiaohe@huawei.com X-Rspam-User: X-Rspamd-Server: rspam11 X-Rspamd-Queue-Id: 028A740050 X-HE-Tag: 1654690659-521749 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 2022/6/8 3:17, Joao Martins wrote: > On 6/7/22 19:32, Andrew Morton wrote: >> >> Let's cc Joao. >> >> On Tue, 7 Jun 2022 22:41:57 +0800 Miaohe Lin wrote: >> >>> compound_pincount_ptr is stored at first tail page instead of second tail >>> page now. >> >> "now"? Some identifiable commit did this? >> > > I think this was in: > > commit5232c63f46fd ("mm: Make compound_pincount always available") Thanks for identifying it. > >>> And if it or some other field changes again in the future, data >>> overwritten might happen. Calling prep_compound_head() outside the loop >>> to prevent such possible issue. No functional change intended. >>> >>> ... >>> >>> --- a/mm/page_alloc.c >>> +++ b/mm/page_alloc.c >>> @@ -6772,17 +6772,8 @@ static void __ref memmap_init_compound(struct page *head, >>> __init_zone_device_page(page, pfn, zone_idx, nid, pgmap); >>> prep_compound_tail(head, pfn - head_pfn); >>> set_page_count(page, 0); >>> - >>> - /* >>> - * The first tail page stores compound_mapcount_ptr() and >>> - * compound_order() and the second tail page stores >>> - * compound_pincount_ptr(). Call prep_compound_head() after >>> - * the first and second tail pages have been initialized to >>> - * not have the data overwritten. >>> - */ >>> - if (pfn == head_pfn + 2) >>> - prep_compound_head(head, order); >>> } >>> + prep_compound_head(head, order); >>> } >>> >>> void __ref memmap_init_zone_device(struct zone *zone, >> > > memmap_init_compound() is only called in pmem case. > > The idea to make this /right after/ we initialize the offending tail pages has > to do with @altmap case wheere struct pages are placed in PMEM and thus taking > advantage of the likelyhood of those tail struct pages being cached given that > we will read them right after in prep_compound_head(). > > I agree with the general sentiment of making this 'more resilient' to compound > pages structure changes by moving prep_compound_head() after all tail pages are > initialized, although I need to express a concern about this making altmap possibly > being affected or regressed. Considering on 2M compound pages we will access first and > second tail pages /after/ initializing 32768 struct pages, or after touching/initializing > 256K struct pages. Many thanks for your explanation. IIUC, the below change should be preferred? diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 4c7d99ee58b4..048df5d78add 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -6771,13 +6771,18 @@ static void __ref memmap_init_compound(struct page *head, set_page_count(page, 0); /* - * The first tail page stores compound_mapcount_ptr() and - * compound_order() and the second tail page stores - * compound_pincount_ptr(). Call prep_compound_head() after - * the first and second tail pages have been initialized to - * not have the data overwritten. + * The first tail page stores compound_mapcount_ptr(), + * compound_order() and compound_pincount_ptr(). Call + * prep_compound_head() after the first tail page have + * been initialized to not have the data overwritten. + * + * Note the idea to make this right after we initialize + * the offending tail pages is trying to take advantage + * of the likelihood of those tail struct pages being + * cached given that we will read them right after in + * prep_compound_head(). */ - if (pfn == head_pfn + 2) + if (unlikely(pfn == head_pfn + 1)) prep_compound_head(head, order); } } Or am I miss something? Thanks! > . >