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 40196CDB474 for ; Tue, 17 Oct 2023 09:58:47 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D726F80037; Tue, 17 Oct 2023 05:58:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D22558D000C; Tue, 17 Oct 2023 05:58:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C112480037; Tue, 17 Oct 2023 05:58:46 -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 B1E808D000C for ; Tue, 17 Oct 2023 05:58:46 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 854E61CBBCF for ; Tue, 17 Oct 2023 09:58:46 +0000 (UTC) X-FDA: 81354504252.19.907C619 Received: from out-204.mta1.migadu.com (out-204.mta1.migadu.com [95.215.58.204]) by imf15.hostedemail.com (Postfix) with ESMTP id 1412CA0020 for ; Tue, 17 Oct 2023 09:58:41 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=Tdmv2TK9; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf15.hostedemail.com: domain of yajun.deng@linux.dev designates 95.215.58.204 as permitted sender) smtp.mailfrom=yajun.deng@linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1697536723; h=from:from:sender: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:dkim-signature; bh=9AnajFnpSm9h3WHytb+us6ByAc92RA8ylloa0bDcwnk=; b=iCJJkfoOPq7MYRTQ7EenDD6Gp5g9ErDQBcaXQGrBpLRhlH2L1SzBFWzYxSxMN0pRiVNT12 vdWakySTrFxp9IkkCY+pLu/WsIg5cuDNw6ix6EVzC0Rs0amuQTz44icE5iVwl4QJyrC3w7 /+ZlC5WC9PgPE/WScd/7vVPMww8xtH0= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=Tdmv2TK9; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf15.hostedemail.com: domain of yajun.deng@linux.dev designates 95.215.58.204 as permitted sender) smtp.mailfrom=yajun.deng@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1697536723; a=rsa-sha256; cv=none; b=UytR3PdRhqvQLbMCiCMN3o05zgufr7NezDveHcHjkFCLPB0pU3iqLpNNEw6gCmsZufNorE 90Pg1YYOJ9n1ua68w8J5CU/gKTnuucbFR+7YRbuRt0Nb7gYsOaebvVKKvmyl74FD73bmKX oPG5iDQpZAPOtD0T8GkAHN0YxgMyDOw= Message-ID: <8a18b38a-9b49-b583-3a54-54fcd843fb9d@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1697536716; h=from:from: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=9AnajFnpSm9h3WHytb+us6ByAc92RA8ylloa0bDcwnk=; b=Tdmv2TK9bxyJocAygdHBWmXesOjPNuQj06JMRWgz6SgVUkLL+gNa3I7cK8vSyMT/Pmatb4 hhgdwwufGr22yUd1jhKw8Lh40uUfpKJKK+se9FgEj7/+t0uh9DNe6oeb/eKvjRkgAhfIlg zb25Fv3ayWDxAObAxZyi19E8lr8rLR8= Date: Tue, 17 Oct 2023 17:58:24 +0800 MIME-Version: 1.0 Subject: Re: [PATCH v4 2/2] mm: Init page count in reserve_bootmem_region when MEMINIT_EARLY Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yajun Deng To: David Hildenbrand , Mike Rapoport Cc: akpm@linux-foundation.org, mike.kravetz@oracle.com, muchun.song@linux.dev, willy@infradead.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20230928083302.386202-1-yajun.deng@linux.dev> <20230928083302.386202-3-yajun.deng@linux.dev> <20230929083018.GU3303@kernel.org> <2f8c4741-5c7f-272d-9cef-9fda9fbc7ca6@linux.dev> <5382bf2d-5aa0-1498-8169-3248be4b5af3@linux.dev> <38cd0cb9-efe9-b98a-2768-ccb48da8b812@linux.dev> <20231013084827.GT3303@kernel.org> <1c91dd62-886d-bb05-8aee-22191a8a2d8e@linux.dev> <20231016063357.GU3303@kernel.org> <0d890048-be58-5050-02fa-21768059aa0d@linux.dev> <23302f67-eb69-265a-ab2d-9c55715e2843@redhat.com> <121772bf-4c1d-3d23-f266-60ce2e879193@linux.dev> In-Reply-To: <121772bf-4c1d-3d23-f266-60ce2e879193@linux.dev> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Rspamd-Queue-Id: 1412CA0020 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: 3fors59ztdegb143jtx8gw61bid9yuow X-HE-Tag: 1697536721-175986 X-HE-Meta: U2FsdGVkX19yDpVktX3KVTjjGV5HbOIUtntD2HY159xiCYjQyXFmPn0cZffcH4eygIHJq4DoppRvK6uP5WclWNLHuvu9GBrpoOofOYwylhHMa9s7tXwH7LbLgFJJDhEoB84z7D1RRaRTEwAk5CtCp07ihp4vEzQmZHzbihD8ZyUPbNu5GiQN8TuOezuVgogmCQCvW0o1eVHBz6lj/LYeRhizKfw/49oLkiLS7z4YwGHo+AeapjhR9ea6lknyMHn3fTfLHHbraHAb41RHb5dksOPw2JYgupHU+TTqZq7XcEGdmk/vIkpmwzyL5PPzUA6oZwbrDtQR5XLBLd8wwKCCm9YiEV+mcpIM84T4ZtPlU3hN7BbZoGS1h+Mluuc1ttmNDH9VsjCZfMVB8p7cBy6JU6x+vTgZzPJvOF/lDKwLSUg6qDlneXGK+kmIJ893QimQTojZ+LUGspmJRkjkfC8kk2Nns44lJtm/5eLhZTY3YcyHI7fwLBbRhJRdwpT63RCLj7+RBpbR5dnfTCHGfOBe3g3XUF96R6TsxOb5WC9i+EBgJijbjfKdJvAJU1vySPiashY5Enuqrq5r6gaEzuCPf2bz6SbZ767xNO1Fpf/yS09ls435LBON9ydpYmWbYo8rJClJDuMd65SAnt11LFTvVBQ4GOjDYTrC0ro515H8T6sVFxugktHJCbYOmHz+327kBaYvycqew1uQVpvrB0qnHdTvjiCQlpd8CBFnjVcr+GNiGZonPGaPttGhrge+JmeU/dYuQbg3ppDyhrFgiOP9fgSN0YLcvCjAQdtHzdx4FaSU8iKLbweImGr517QFVKWTzxue5Up+Oqg8RT9k07pUYWh84JfhjKgYnkEoPBMFsT+UNPetGoYtSavC6cHtFVxRc2UBbu1ZiOlVFD3UXGyLHGZCwAgGhr2McQ/6pZDW1LvjwblVCBOUfgASxsrHznvq/AwNtXXvi893/y5US20 ihptAE8Q 9RPHiNhxb+PierQKX1MM6mFqWRNUe6Rfi9HyOtcYCk+WpMuJBIqMfyTkNuesYLzpR/GqPBijj2+rbqhNjhuk4SCPxW+2D4amUHoAvGASUkPQW+NuvpyAeyiFb94+YVWm//f5jm3IemA/pMXGTjSD5R+G6iDImdcbjOUmBVVAzP1Lxvb3toaCosbQn0DM81Em0zRBU 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 2023/10/16 18:17, Yajun Deng wrote: > > On 2023/10/16 16:36, David Hildenbrand wrote: >> On 16.10.23 10:32, Yajun Deng wrote: >>> >>> On 2023/10/16 16:16, David Hildenbrand wrote: >>>> On 16.10.23 10:10, Yajun Deng wrote: >>>>> >>>>> On 2023/10/16 14:33, Mike Rapoport wrote: >>>>>> On Fri, Oct 13, 2023 at 05:29:19PM +0800, Yajun Deng wrote: >>>>>>> On 2023/10/13 16:48, Mike Rapoport wrote: >>>>>>>> On Thu, Oct 12, 2023 at 05:53:22PM +0800, Yajun Deng wrote: >>>>>>>>> On 2023/10/12 17:23, David Hildenbrand wrote: >>>>>>>>>> On 10.10.23 04:31, Yajun Deng wrote: >>>>>>>>>>> On 2023/10/8 16:57, Yajun Deng wrote: >>>>>>>>>>>>> That looks wrong. if the page count would by pure luck be 0 >>>>>>>>>>>>> already for hotplugged memory, you wouldn't clear the >>>>>>>>>>>>> reserved >>>>>>>>>>>>> flag. >>>>>>>>>>>>> >>>>>>>>>>>>> These changes make me a bit nervous. >>>>>>>>>>>> Is 'if (page_count(page) || PageReserved(page))' be safer? Or >>>>>>>>>>>> do I >>>>>>>>>>>> need to do something else? >>>>>>>>>>>> >>>>>>>>>>> How about the following if statement? But it needs to add more >>>>>>>>>>> patch >>>>>>>>>>> like v1 ([PATCH 2/4] mm: Introduce MEMINIT_LATE context). >>>>>>>>>>> >>>>>>>>>>> It'll be safer, but more complex. Please comment... >>>>>>>>>>> >>>>>>>>>>>         if (context != MEMINIT_EARLY || (page_count(page) || >>>>>>>>>>> PageReserved(page)) { >>>>>>>>>>> >>>>>>>>>> Ideally we could make initialization only depend on the context, >>>>>>>>>> and not >>>>>>>>>> check for count or the reserved flag. >>>>>>>>>> >>>>>>>>> This link is v1, >>>>>>>>> https://lore.kernel.org/all/20230922070923.355656-1-yajun.deng@linux.dev/ >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> If we could make initialization only depend on the context, I'll >>>>>>>>> modify it >>>>>>>>> based on v1. >>>>>>>> Although ~20% improvement looks impressive, this is only >>>>>>>> optimization of a >>>>>>>> fraction of the boot time, and realistically, how much 56 msec >>>>>>>> saves from >>>>>>>> the total boot time when you boot a machine with 190G of RAM? >>>>>>> There are a lot of factors that can affect the total boot time. 56 >>>>>>> msec >>>>>>> saves may be insignificant. >>>>>>> >>>>>>> But if we look at the boot log, we'll see there's a significant >>>>>>> time jump. >>>>>>> >>>>>>> before: >>>>>>> >>>>>>> [    0.250334] ACPI: PM-Timer IO Port: 0x508 >>>>>>> [    0.618994] Memory: 173413056K/199884452K available (18440K >>>>>>> kernel code, >>>>>>> >>>>>>> after: >>>>>>> >>>>>>> [    0.260229] software IO TLB: area num 32. >>>>>>> [    0.563497] Memory: 173413056K/199884452K available (18440K >>>>>>> kernel code, >>>>>>> Memory: >>>>>>> Memory initialization is time consuming in the boot log. >>>>>> You just confirmed that 56 msec is insignificant and then you send >>>>>> again >>>>>> the improvement of ~60 msec in memory initialization. >>>>>> >>>>>> What does this improvement gain in percentage of total boot time? >>>>> >>>>> >>>>> before: >>>>> >>>>> [   10.692708] Run /init as init process >>>>> >>>>> >>>>> after: >>>>> >>>>> [   10.666290] Run /init as init process >>>>> >>>>> >>>>> About 0.25%. The total boot time is variable, depending on how many >>>>> drivers need to be initialized. >>>>> >>>>> >>>>>>>> I still think the improvement does not justify the churn, added >>>>>>>> complexity >>>>>>>> and special casing of different code paths of initialization of >>>>>>>> struct pages. >>>>>>> >>>>>>> Because there is a loop, if the order is MAX_ORDER, the loop will >>>>>>> run 1024 >>>>>>> times. The following 'if' would be safer: >>>>>>> >>>>>>> 'if (context != MEMINIT_EARLY || (page_count(page) || >> >>>>>>> PageReserved(page)) >>>>>>> {' >>>>>> No, it will not. >>>>>> >>>>>> As the matter of fact any condition here won't be 'safer' because it >>>>>> makes >>>>>> the code more complex and less maintainable. >>>>>> Any future change in __free_pages_core() or one of it's callers will >>>>>> have >>>>>> to reason what will happen with that condition after the change. >>>>> >>>>> >>>>> To avoid introducing MEMINIT_LATE context and make code simpler. This >>>>> might be a better option. >>>>> >>>>> if (page_count(page) || PageReserved(page)) >>>> >>>> I'll have to side with Mike here; this change might not be worth it. >>>> >>> >>> Okay, I got it. Thanks! >> >> IMHO instead of adding more checks to that code we should try to >> unify that handling such that we can just remove it. As expressed, at >> least from the memory hotplug perspective there are still reasons why >> we need that; I can provide some guidance on how to eventually >> achieve that, but it might end up in a bit of work ... > > > Yes, we can't remove it right now. If we want to do that, we have to > clean up rely on page count and PageReserved first. How about making __free_pages_core separate, like: void __init __free_pages_core_early(struct page *page, unsigned int order) {         unsigned int nr_pages = 1 << order;         atomic_long_add(nr_pages, &page_zone(page)->managed_pages);         if (page_contains_unaccepted(page, order)) {                 if (order == MAX_ORDER && __free_unaccepted(page))                         return;                 accept_page(page, order);         }         /*          * Bypass PCP and place fresh pages right to the tail, primarily          * relevant for memory onlining.          */         __free_pages_ok(page, order, FPI_TO_TAIL); } void __free_pages_core(struct page *page, unsigned int order) {         unsigned int nr_pages = 1 << order;         struct page *p = page;         unsigned int loop;         /*          * When initializing the memmap, __init_single_page() sets the refcount          * of all pages to 1 ("allocated"/"not free"). We have to set the          * refcount of all involved pages to 0.          */         prefetchw(p);         for (loop = 0; loop < (nr_pages - 1); loop++, p++) {                 prefetchw(p + 1);                 __ClearPageReserved(p);                 set_page_count(p, 0);         }         __ClearPageReserved(p);         set_page_count(p, 0);         __free_pages_core_early(page, order); } We only change the caller we need to __free_pages_core_early, it doesn't affect other callers. > >> >> Anyhow, thanks for bringing up that topic; it reminded me that I >> still have pending cleanups to not rely on PageReserved on the memory >> hotplug path. >>