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 8EDA3C0015E for ; Mon, 24 Jul 2023 14:34:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D98248E0002; Mon, 24 Jul 2023 10:33:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D47FE8E0001; Mon, 24 Jul 2023 10:33:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C10378E0002; Mon, 24 Jul 2023 10:33:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id B27CF8E0001 for ; Mon, 24 Jul 2023 10:33:59 -0400 (EDT) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 8D8FC1A06DA for ; Mon, 24 Jul 2023 14:33:59 +0000 (UTC) X-FDA: 81046749798.25.0E46FA6 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf28.hostedemail.com (Postfix) with ESMTP id 8D770C001E for ; Mon, 24 Jul 2023 14:33:55 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=hC4layOn; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf28.hostedemail.com: domain of david@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=david@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1690209235; a=rsa-sha256; cv=none; b=bWmL/jG8YP+QlvoGY7SV2oMvt3a6Q7ClGBCxT1Da4MBNZfAS3k63Ps6nSZAPkvmgovqpdn V9/thr0dwi8fCFwRE2LLUBEx+fzGhwfvRE0R1EkhyT5s5l3B9YU0NNpb/F85qFW8n9D1Wr N271yc38VemANKUfASXqR+ZGUArZK7c= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=hC4layOn; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf28.hostedemail.com: domain of david@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=david@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1690209235; 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=QuAwZXj3hY8+uff4gHFej9mLNWJqFkLD2znR2KTvWws=; b=qENrCI5b6kNNdjOwcfRw1ajWwid+c+zGfPSmCCbBzt96son+B2GwFyTUIQbn9JlE0nXuj3 nvhENE+XSMPP/inOfgHhjEcTmIeTKuTm62OC97RZSIQ4c3YExb5jQZcV5y8SxEw/vO8upS gkNfiEnspABVwnIgUtsk2EMCYP5M1dc= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1690209234; 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=QuAwZXj3hY8+uff4gHFej9mLNWJqFkLD2znR2KTvWws=; b=hC4layOn+WeJn+A2qkQDgoOKU/5MN5HuQoO+T+g9J349gkisHf6ffM+R83zFclT2pVVV+I KcLKamKG+3vZ6xpPvCgimR4AMbPnyP91vwgWlFN+gaooy0GKPt+Z3CU/lg0spM/th4i3ix 6T4DPmCE7p2YdQE1f16JZjWMT5khv/w= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-82-uhaPzat4Mx2wp-J0qSfnPQ-1; Mon, 24 Jul 2023 10:33:53 -0400 X-MC-Unique: uhaPzat4Mx2wp-J0qSfnPQ-1 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-3faabd8fd33so21600785e9.0 for ; Mon, 24 Jul 2023 07:33:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690209232; x=1690814032; h=content-transfer-encoding:in-reply-to:subject:organization:from :references:cc:to:content-language:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=QuAwZXj3hY8+uff4gHFej9mLNWJqFkLD2znR2KTvWws=; b=MZU0Zxlt9agRz/n/1u+QB2lzH3q6nteasm4GxOtlejLajd1yBN/QDUy8LSEL1wx0ET lqL4caHQpFqqh7njpeD/JSFeMKIwc/PAAWIxZqK5n4XrMnyuTTbH+CZJT0OjmIL/KN5f ZHxZfHzli3QmHCllxe9b1pgJdoTYeejNJzxao+drcxMGguETeAn6CXqW/HnQ4V9nl4/F kRspkd86H2w8Ff///yZX8kMefi3q94z3Ntdu2vr/Hn8xLdT1kH6q98/nFp3DgMbw8yFg Jt/NZGOOFAyOx8RZ3TJr2MO1JOzd0BmbFdi8pcTe8vwmESTS0cXUGy3BnGUyLi1B8bb9 f9+w== X-Gm-Message-State: ABy/qLbWT/vA7EzDEmya4TU/KTTpTNQY68lez6nmitLeCpiAQVpwtygr n+hZZqownPkvQMsnlfZOCspK16iAG3fKoVP0gCvDDiKkso8TmXPfcynRj4F64BVOEZJtzelZd/5 3jvLxgAOjGzQ= X-Received: by 2002:adf:e70d:0:b0:313:ede8:9f13 with SMTP id c13-20020adfe70d000000b00313ede89f13mr6911134wrm.32.1690209231981; Mon, 24 Jul 2023 07:33:51 -0700 (PDT) X-Google-Smtp-Source: APBJJlEPtHEZfW45h1GMyO5AvAf8f8oOZ5lizueDmlksmAWhGklwwVI9CZukkB7lJTtAERgsDlMM9Q== X-Received: by 2002:adf:e70d:0:b0:313:ede8:9f13 with SMTP id c13-20020adfe70d000000b00313ede89f13mr6911119wrm.32.1690209231524; Mon, 24 Jul 2023 07:33:51 -0700 (PDT) Received: from ?IPV6:2003:cb:c73d:bb00:91a5:d1c:3a7e:4c77? (p200300cbc73dbb0091a50d1c3a7e4c77.dip0.t-ipconnect.de. [2003:cb:c73d:bb00:91a5:d1c:3a7e:4c77]) by smtp.gmail.com with ESMTPSA id a23-20020a5d4577000000b00314315071bbsm13111541wrc.38.2023.07.24.07.33.50 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 24 Jul 2023 07:33:51 -0700 (PDT) Message-ID: Date: Mon, 24 Jul 2023 16:33:50 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 To: "Aneesh Kumar K.V" , linux-mm@kvack.org, akpm@linux-foundation.org, mpe@ellerman.id.au, linuxppc-dev@lists.ozlabs.org, npiggin@gmail.com, christophe.leroy@csgroup.eu Cc: Oscar Salvador , Michal Hocko , Vishal Verma References: <20230718024409.95742-1-aneesh.kumar@linux.ibm.com> <20230718024409.95742-5-aneesh.kumar@linux.ibm.com> From: David Hildenbrand Organization: Red Hat Subject: Re: [PATCH v4 4/6] mm/hotplug: Allow pageblock alignment via altmap reservation In-Reply-To: <20230718024409.95742-5-aneesh.kumar@linux.ibm.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 8D770C001E X-Stat-Signature: kk5apjgcd8b4ftgau999sapufsdtrx9q X-HE-Tag: 1690209235-760761 X-HE-Meta: U2FsdGVkX18ZU5w/IykDgXooH0JVhayDEaKmGrJCKXYUsyybH+aYPKMdL3coChqN7FzjT1sShTuapZ7x5tcS3feAMdlWuEPPWJM/GuagKSKp/qvlz3iuurc0aGW6I3D/MxKTHEgy/owc1CAPLK++d9vruZqVDcjxsiHans6lzzYnf/wn45O3EXd+C5pzuMs6NO/XfsDIp8kVbQpCBVOP8xOn1Prkg91o8daEuI1EZJYqKWB1agcePmCQmRgI4r1dOgOxWBFyCA1VfX4whZVekWcjiENM1jTxNtOCKR+vRsqtp+uagnJiTjU2DRYFJbSmZCcEPuw9ncYTRt7YIsH2XTs/TcKVcdxAOtz5haf1e06giMs91ADqm5Ls2k3JZHmsXpSCJAMt24BbQkV4FnajkbMMnrot+cjhVSnauSHG9Azc4lBBJoVxETJm1Q45H9cpsCtVOI0Qi5nGRQLkIS4XQygJj/EWLpg3wM0n0XfTi6opmlUpXigXcFGWSgdzzQxAicpE7YxVCfuL04f/WrrzuCEeVb/LlDPEe2UpA5CT0ujoAsYkJa/lZXsN3uL0ujUoX7/jcOHI5WE0+SxS/WJ40ucNGP5s3P8l7dOR6OOdYh8+faC+JFiw7E6koY3v2Pvu0LwtWOab3xe0s00KMVZTShV528ILtEJ2Vk+M0GxrjXQTWHao5KYGr8c4q2pMvDvsa3ZQRd4URvt0Uyb1euo8+zg7zmcVaM3sXVHBbxFSd5Xquo+/ydsOD+MSCmtTXUSEaXzYwzSm9D82CIeDTQgqsoddFg589oSbfgRHa41iEilUfC9OjOA2rxtw6bU9Sj2zZz+XKm0F1kO/qBbHhPhM2O9IseghOMKySRLblID7JGpeiIVyf0B8JxsQ5IQll/NxMmx67pPenRDefzNJYVsRRlcUh2ma1GUu0jEDwQfwr/TQsWyPrlldjFqOkD+BCVwICK35YFsUsH1EYcUi/ek +8bpn0S9 8Zc/NSu3G5quJxPBserhBjBQSC57lN8DpNxYaErNUr3jTPdtGzGrO2bbmrUwYvfH3cDD6yeDwJizm+nVHu2usVDGw4rxPV1TQKx2a8RwHsvJ1Rst1hxoOM6pMnn8EAAp690sdL8ybLyuoGt7bvW3vs3hyj05eRxVLBwrf4fDWguMr0bg1HxCTR2GVumsnBU3sg2pxYnEr5eON7iXCL2XoSkGouXXmT56SQco7d4jSnwMV5QYNoINBpWAc3u88B4YzIZBrYt7wgyNebYLBUQBuimtV1cuNpv9tHkvDU5wouTR+DvYxpEhWSXUIqtj6L/cQOUj8ludBQI+Q1W2UBo6QUL0Mjal3KIohFIYLO0Jr+HY+z3NbGAHKXmQZrQuetOM1aMyMy3Yo5XKvAfH6M7PFZm5pfGepUwsM1jUh 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 18.07.23 04:44, Aneesh Kumar K.V wrote: > Add a new kconfig option that can be selected if we want to allow That description seems outdated. > pageblock alignment by reserving pages in the vmemmap altmap area. > This implies we will be reserving some pages for every memoryblock > This also allows the memmap on memory feature to be widely useful > with different memory block size values. Can you add some more meat to the description, and especially, in which cases this might be desired and in which cases it might be completely undesired? Let's assume we hotplug a 1 GiB DIMM on arm64/64k. With 512 MiB pageblocks, we'd waste 50% of the hotplugged memory. Also, see below on the case where we could end up with 100% wasted memory, which we want to block compeltely. Also, I wonder if we can avoid talking about "page reservation" or "altmap reservation", that's rather an implementation detail. For example, I'd call this patch "mm/hotplug: Support memmap_on_memory when memmap is not aligned to pageblocks" > > Signed-off-by: Aneesh Kumar K.V > --- > mm/memory_hotplug.c | 109 ++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 96 insertions(+), 13 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 5921c81fcb70..c409f5ff6a59 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -41,17 +41,85 @@ > #include "internal.h" > #include "shuffle.h" > > +enum { > + MEMMAP_ON_MEMORY_DISABLE = 0, > + MEMMAP_ON_MEMORY_ENABLE, > + MEMMAP_ON_MEMORY_FORCE, > +}; > + > +static int memmap_mode __read_mostly = MEMMAP_ON_MEMORY_DISABLE; > + > +static inline unsigned long memory_block_align_base(unsigned long size) > +{ Can we start with something like this instead? memory_block_memmap_size() might be reasonable to put into the previous patch. static inline unsigned long memory_block_memmap_size(void) { return PHYS_PFN(memory_block_size_bytes()) * sizeof(struct page); } /* * In "forced" memmap_on_memory mode, we always align the vmemmap size up to cover * full pageblocks. That way, we can add memory even if the vmemmap size is not properly * aligned, however, we might waste memory. */ static inline unsigned long memory_block_memmap_on_memory_size(void) { unsigned long size = memory_block_memmap_size(); if (memmap_mode != MEMMAP_ON_MEMORY_FORCE) return size; return ALIGN(size, PFN_PHYS(pageblock_nr_pages)); } > + if (memmap_mode == MEMMAP_ON_MEMORY_FORCE) { > + unsigned long align; > + unsigned long nr_vmemmap_pages = size >> PAGE_SHIFT; > + unsigned long vmemmap_size; > + > + vmemmap_size = DIV_ROUND_UP(nr_vmemmap_pages * sizeof(struct page), PAGE_SIZE); > + align = pageblock_align(vmemmap_size) - vmemmap_size; > + return align; > + } else > + return 0; > +} > + > #ifdef CONFIG_MHP_MEMMAP_ON_MEMORY > /* > * memory_hotplug.memmap_on_memory parameter > */ > -static bool memmap_on_memory __ro_after_init; > -module_param(memmap_on_memory, bool, 0444); > -MODULE_PARM_DESC(memmap_on_memory, "Enable memmap on memory for memory hotplug"); > +static int set_memmap_mode(const char *val, const struct kernel_param *kp) > +{ > + int ret, mode; > + bool enabled; > + > + if (sysfs_streq(val, "force") || sysfs_streq(val, "FORCE")) { > + mode = MEMMAP_ON_MEMORY_FORCE; > + goto matched; > + } > + > + ret = kstrtobool(val, &enabled); > + if (ret < 0) > + return ret; > + if (enabled) > + mode = MEMMAP_ON_MEMORY_ENABLE; > + else > + mode = MEMMAP_ON_MEMORY_DISABLE; > + > +matched: > + *((int *)kp->arg) = mode; > + if (mode == MEMMAP_ON_MEMORY_FORCE) { > + pr_info("Memory hotplug will reserve %ld pages in each memory block\n", > + memory_block_align_base(memory_block_size_bytes())); > + } > + return 0; > +} > + > +static int get_memmap_mode(char *buffer, const struct kernel_param *kp) > +{ > + if (*((int *)kp->arg) == MEMMAP_ON_MEMORY_FORCE) > + return sprintf(buffer, "force\n"); > + if (*((int *)kp->arg) == MEMMAP_ON_MEMORY_ENABLE) > + return sprintf(buffer, "y\n"); > + > + return sprintf(buffer, "n\n"); param_get_bool() uses uppercase Y / N. Maybe just return the uppercase variants here as well. > +} > + > +static const struct kernel_param_ops memmap_mode_ops = { > + .set = set_memmap_mode, > + .get = get_memmap_mode, > +}; > +module_param_cb(memmap_on_memory, &memmap_mode_ops, &memmap_mode, 0444); > +MODULE_PARM_DESC(memmap_on_memory, "Enable memmap on memory for memory hotplug\n" > + "With value \"force\" it could result in memory wastage due to memmap size limitations \n" > + "For example, if the memmap for a memory block requires 1 MiB, but the pageblock \n" > + "size is 2 MiB, 1 MiB of hotplugged memory will be wasted. Note that there are \n" > + "still cases where the feature cannot be enforced: for example, if the memmap is \n" > + "smaller than a single page, or if the architecture does not support the forced \n" > + "mode in all configurations. (y/n/force)"); That's a bit mouthful. Can we simplify and put the full doc into Documentation/admin-guide/mm/memory-hotplug.rst ? > > static inline bool mhp_memmap_on_memory(void) > { > - return memmap_on_memory; > + return !!memmap_mode; Maybe better "memmap_mode != MEMMAP_ON_MEMORY_DISABLE" > } > #else > static inline bool mhp_memmap_on_memory(void) > @@ -1264,7 +1332,6 @@ static inline bool arch_supports_memmap_on_memory(unsigned long size) > > static bool mhp_supports_memmap_on_memory(unsigned long size) > { > - > unsigned long nr_vmemmap_pages = size >> PAGE_SHIFT; > unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page); > unsigned long remaining_size = size - vmemmap_size; > @@ -1295,10 +1362,23 @@ static bool mhp_supports_memmap_on_memory(unsigned long size) > * altmap as an alternative source of memory, and we do not exactly > * populate a single PMD. > */ > - return mhp_memmap_on_memory() && > - size == memory_block_size_bytes() && > - IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT)) && > - arch_supports_memmap_on_memory(size); > + if (!mhp_memmap_on_memory() || size != memory_block_size_bytes()) > + return false; > + > + /* > + * Make sure the vmemmap allocation is fully contained > + * so that we always allocate vmemmap memory from altmap area. > + */ > + if (!IS_ALIGNED(vmemmap_size, PAGE_SIZE)) > + return false; > + /* > + * Without page reservation remaining pages should be pageblock aligned. > + */ > + if (memmap_mode != MEMMAP_ON_MEMORY_FORCE && > + !IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT))) > + return false; With our new helper, this becomes the following: memmap_on_memory_size = memory_block_memmap_on_memory_size(); if (!IS_ALIGNED(memmap_on_memory_size, PFN_PHYS(pageblock_nr_pages)) /* We're not allowed to waste any memory for the memmap. */ return false; if (memmap_on_memory_size == memory_block_size_bytes()) /* No effective hotplugged memory doesn't make sense. */ return false; > + return arch_supports_memmap_on_memory(size); > } > > /* > @@ -1311,7 +1391,11 @@ int __ref add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags) > { > struct mhp_params params = { .pgprot = pgprot_mhp(PAGE_KERNEL) }; > enum memblock_flags memblock_flags = MEMBLOCK_NONE; > - struct vmem_altmap mhp_altmap = {}; > + struct vmem_altmap mhp_altmap = { > + .base_pfn = PHYS_PFN(res->start), > + .end_pfn = PHYS_PFN(res->end), > + .reserve = memory_block_align_base(resource_size(res)), Can you remind me why we have to set reserve here at all? IOW, can't we simply set .free = memory_block_memmap_on_memory_size(); end then pass mhp_altmap.alloc + mhp_altmap.free to create_memory_block_devices() instead? -- Cheers, David / dhildenb