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 440C5C2BD09 for ; Mon, 1 Jul 2024 05:15:52 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5E35F6B0088; Mon, 1 Jul 2024 01:15:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5936F6B0089; Mon, 1 Jul 2024 01:15:51 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 434266B008A; Mon, 1 Jul 2024 01:15:51 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 201CC6B0088 for ; Mon, 1 Jul 2024 01:15:51 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 85F751A099A for ; Mon, 1 Jul 2024 05:15:50 +0000 (UTC) X-FDA: 82290021660.10.69C93E2 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.18]) by imf16.hostedemail.com (Postfix) with ESMTP id D0BBD18001A for ; Mon, 1 Jul 2024 05:15:47 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=Ac34Kva2; spf=pass (imf16.hostedemail.com: domain of ying.huang@intel.com designates 198.175.65.18 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1719810920; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=F6MVoj/vmBWbXUCPbLQvLB0q8j83+XTzEFo3GroGsC0=; b=C3H0UZhfjr92qA3So3GMC2UCOwdqn2nhxwtWOMsgOh8RYIkA5dyF0qH01FxuBcvnB0+db3 8qRyayMHf69549KM6pJ9tGVDHQ75tOZ8tvVex50j6KhukmJrDkJ2Age795PReiDsgClodZ i8IhexB3B4knDBPSZsFoXgOMYgcKfKo= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=Ac34Kva2; spf=pass (imf16.hostedemail.com: domain of ying.huang@intel.com designates 198.175.65.18 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1719810920; a=rsa-sha256; cv=none; b=ACh1k9/Kr+brOykKjoXFwCE/54QahBIv0sznyvQZ0Tk/JtwHPGYCpCSYm/Jr2RczQYfEvj wNKp160Ne5cQbqwCIZp2rSNY+Wm4mksMKhBc74pIBb/UCiep6ng6kaS1dcLc+bK+Gy63gK KQBlz4mipITSpZF43uR1LPo5PY9Byr8= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1719810949; x=1751346949; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version; bh=33LnhJ7+BqQZikbci+g6bJ6dGM6/pya/Tnvx+wHf3/o=; b=Ac34Kva2e1SZoCktKHzxgcXNddyWR5IwBBOmYeZHfP/8L59oJfXXsqyE syKV4eJ3b/x/E5f6hdHTH2/Yw3P9PALUflNgHVtiVR6sipJx6DzVIom/r SNY9cIZ4DfQTTnKeCxj+t6f9wFvc0+cWtkjtlMvrlZMy0QtLWwhN2WNhJ NY+BqljjQ/G/axiCYBUYhZDtGYlvHb8xepT2xEmLwj7nZpaqtfJ08bvSM C5KcQ8IXHjypINsw1laL7fjcG7ky91Aa5vMjIP5phtjAP+KmmXnCd6pxV jg7NKMAxfjgl4JYpkFS18Ql1vulDibsnTYq9erufmJKVdEU8++URVBi1Y Q==; X-CSE-ConnectionGUID: Bvff9mJNQmOdYlBYXdmktw== X-CSE-MsgGUID: kjC1YIcFRpejaRbttknDbw== X-IronPort-AV: E=McAfee;i="6700,10204,11119"; a="17051617" X-IronPort-AV: E=Sophos;i="6.09,175,1716274800"; d="scan'208";a="17051617" Received: from orviesa006.jf.intel.com ([10.64.159.146]) by orvoesa110.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Jun 2024 22:15:47 -0700 X-CSE-ConnectionGUID: tRBL+MYsRYyKY5Q3fRDiVA== X-CSE-MsgGUID: yXSuGEvLRIqOj0oi+miQLQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.09,175,1716274800"; d="scan'208";a="45803392" Received: from unknown (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by orviesa006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Jun 2024 22:15:40 -0700 From: "Huang, Ying" To: "Ho-Ren (Jack) Chuang" Cc: "Jonathan Cameron" , "Gregory Price" , aneesh.kumar@linux.ibm.com, mhocko@suse.com, tj@kernel.org, john@jagalactic.com, "Eishan Mirakhur" , "Vinicius Tavares Petrucci" , "Ravis OpenSrc" , "Alistair Popple" , "Srinivasulu Thanneeru" , "SeongJae Park" , "Rafael J. Wysocki" , Len Brown , Andrew Morton , Dave Jiang , Dan Williams , linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, "Ho-Ren (Jack) Chuang" , "Ho-Ren (Jack) Chuang" , "Ho-Ren (Jack) Chuang" , linux-cxl@vger.kernel.org, qemu-devel@nongnu.org Subject: Re: [PATCH v2 1/1] memory tier: consolidate the initialization of memory tiers In-Reply-To: <20240628060925.303309-2-horen.chuang@linux.dev> (Ho-Ren Chuang's message of "Fri, 28 Jun 2024 06:09:23 +0000") References: <20240628060925.303309-1-horen.chuang@linux.dev> <20240628060925.303309-2-horen.chuang@linux.dev> Date: Mon, 01 Jul 2024 13:13:49 +0800 Message-ID: <87tth9ofsi.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: D0BBD18001A X-Stat-Signature: 5o1r5wgn757635dxopwt3pckrr97pq9b X-Rspam-User: X-HE-Tag: 1719810947-479845 X-HE-Meta: U2FsdGVkX1/1VW9PSu53zhoMBzxDbI9piOmV/7HIfeUnWJLUubXLKIVO4QkO2NeNdNrSOkmftTgKnGtNgm+fBJNOR8h+u1D7kr5EV9n+DXFqRgMbql2Wo1qr9qgynUoKYxwBRG7NR0jaFDa1SHtNiA4IRah401IFw2BdFn1vNvx1j64i0DBYSIXAIS1Wlr585eR4Emcw5oTav46hQE7t/8lPj22Fi5+G43x4HjrTg3mVjYIKLnbNZzcoMWmtB5x3Xhmeobdw683qWN/zAMnnKFvDhhY57cnywfdLja6EQTHfcPuk6+UbjalO7yseqvtfKFQPNvQwG6wnGAW6E4cGbzUi+5+xeGw41em18RGOaOhQiXVch+np67vmpgo4p1x+ax2QUuHSY0FpUZRpK8hPrq6cxcrmspqEar2xDV6Iu/f9kswRA2onhAbsrOkfR/SJkWcdUoVCkWU7WFtU+k2fYmz+xLNWBOhvWpt4BEUNKUzE19Elu3lKzrNnCu5o1em8kvQ5eF5cI6/4y/IhWGBJu+UMSXO547vUrIDf0hzRnd+sMj+7QaV0VW6aBSgiFGccYNzZlyeHldsulhMWOdW4zzcBSCGmoQXVklLzvVhAQQSEsBgIUuXylUaHDiYnq5xrwxlnv3r1rA+biuugbQYCeFeq9whVDC9cGZpvCM3ztPTlqyFXhciZnePn213ore8N/A0esINjOwuhwr2UgdmXDqiXxjUFHjh9OqVWW8auK2Zh/NvEJY6ZpTcp3RCIe6OX4EXdyQvtd9x6lNKzLn3Xa+R7rIgJStN1ZIUva1HbnVylhtidITcCY2Wl8e5StLkpVeY+xeGVZN+28San68PmGwB807awKDHiilU9IycrTvtAdTKkJfQkfOVuqwhAVZFJcSfu1/mcCEzlyPMX6P9ZYzECJh03UAUCLhaFCOKrqjrprtdvV3RpWLDOmNW29n5qeMhKd7yqH8H3XMVNu5i +8FkRzqz oBVf7lKIlTLaHThC5/afegXKaXVqHVvqMq4kYKnvTy76UJBX2PUfT1aKE8oHYPfd2CfGrA+BPCxKZNBsqSOu/jthDAXStyT/RYgLlUh2t/t9U0CWJ2Y8bM+hW3/KRYUZFc+h/V2ccfU4IFm4Gs/fv+x4v5fdpUwttGsPkbb0Ut8TRSgB35hkG9DbggnQZ0BALjhT2k/pV3KpgLyFXrkTcoAj21KK0mTmT0a66IQ9JagQecmzjmzrWps6ypC4zYaVMA+2E+eBX1tcRbyPAaNeVzdCk9soAXZeby6P66f5QPqzbbiE= 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: List-Subscribe: List-Unsubscribe: Hi, Jack, "Ho-Ren (Jack) Chuang" writes: I suggest you to merge the [0/1] with the change log here. [0/1] describes why do we need the patch. The below text describes some details. Just don't use "---" to separate them. We need both parts in the final commit message. > If we simply move the set_node_memory_tier() from memory_tier_init() > to late_initcall(), it will result in HMAT not registering > the mt_adistance_algorithm callback function, because > set_node_memory_tier() is not performed during the memory tiering > initialization phase, leading to a lack of correct default_dram > information. > > Therefore, we introduced a nodemask to pass the information of the > default DRAM nodes. The reason for not choosing to reuse > default_dram_type->nodes is that it is not clean enough. So in the end, > we use a __initdata variable, which is a variable that is released once > initialization is complete, including both CPU and memory nodes for HMAT > to iterate through. > > Besides, since default_dram_type may be checked/used during the > initialization process of HMAT and drivers, it is better to keep the > allocation of default_dram_type in memory_tier_init(). Why do we need it? IIRC, we have deleted its usage in hmat.c. > Signed-off-by: Ho-Ren (Jack) Chuang > Suggested-by: Jonathan Cameron > --- > drivers/acpi/numa/hmat.c | 5 +-- > include/linux/memory-tiers.h | 2 ++ > mm/memory-tiers.c | 59 +++++++++++++++--------------------- > 3 files changed, 28 insertions(+), 38 deletions(-) > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c > index 2c8ccc91ebe6..a2f9e7a4b479 100644 > --- a/drivers/acpi/numa/hmat.c > +++ b/drivers/acpi/numa/hmat.c > @@ -940,10 +940,7 @@ static int hmat_set_default_dram_perf(void) > struct memory_target *target; > struct access_coordinate *attrs; > > - if (!default_dram_type) > - return -EIO; > - > - for_each_node_mask(nid, default_dram_type->nodes) { > + for_each_node_mask(nid, default_dram_nodes) { > pxm = node_to_pxm(nid); > target = find_mem_target(pxm); > if (!target) > diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h > index 0d70788558f4..fa61ad9c4d75 100644 > --- a/include/linux/memory-tiers.h > +++ b/include/linux/memory-tiers.h > @@ -38,6 +38,7 @@ struct access_coordinate; > #ifdef CONFIG_NUMA > extern bool numa_demotion_enabled; > extern struct memory_dev_type *default_dram_type; Can we remove the above line? > +extern nodemask_t default_dram_nodes __initdata; We don't need to use __initdata in variable declaration. > struct memory_dev_type *alloc_memory_type(int adistance); > void put_memory_type(struct memory_dev_type *memtype); > void init_node_memory_type(int node, struct memory_dev_type *default_type); > @@ -76,6 +77,7 @@ static inline bool node_is_toptier(int node) > > #define numa_demotion_enabled false > #define default_dram_type NULL > +#define default_dram_nodes NODE_MASK_NONE Should we use after "default_dram_nodes"? > /* > * CONFIG_NUMA implementation returns non NULL error. > */ > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c > index 6632102bd5c9..a19a90c3ad36 100644 > --- a/mm/memory-tiers.c > +++ b/mm/memory-tiers.c > @@ -43,6 +43,7 @@ static LIST_HEAD(memory_tiers); > static LIST_HEAD(default_memory_types); > static struct node_memory_type_map node_memory_types[MAX_NUMNODES]; > struct memory_dev_type *default_dram_type; > +nodemask_t default_dram_nodes __initdata = NODE_MASK_NONE; > > static const struct bus_type memory_tier_subsys = { > .name = "memory_tiering", > @@ -671,28 +672,38 @@ EXPORT_SYMBOL_GPL(mt_put_memory_types); > > /* > * This is invoked via `late_initcall()` to initialize memory tiers for > - * CPU-less memory nodes after driver initialization, which is > - * expected to provide `adistance` algorithms. > + * memory nodes, both with and without CPUs. After the initialization of > + * firmware and devices, adistance algorithms are expected to be provided. > */ > static int __init memory_tier_late_init(void) > { > int nid; > + struct memory_tier *memtier; > > + get_online_mems(); > guard(mutex)(&memory_tier_lock); > + /* > + * Look at all the existing and uninitialized N_MEMORY nodes and > + * add them to default memory tier or to a tier if we already have > + * memory types assigned. > + */ If the memory type of the node has been assigned, we will skip it in the following code. So, I think that we need to revise the comments. > for_each_node_state(nid, N_MEMORY) { > /* > - * Some device drivers may have initialized memory tiers > - * between `memory_tier_init()` and `memory_tier_late_init()`, > - * potentially bringing online memory nodes and > - * configuring memory tiers. Exclude them here. > + * Some device drivers may have initialized > + * memory tiers, potentially bringing memory nodes > + * online and configuring memory tiers. > + * Exclude them here. > */ > if (node_memory_types[nid].memtype) > continue; > > - set_node_memory_tier(nid); > + memtier = set_node_memory_tier(nid); > + if (IS_ERR(memtier)) > + /* Continue with memtiers we are able to setup. */ > + break; > } > - > establish_demotion_targets(); > + put_online_mems(); > > return 0; > } > @@ -875,8 +886,7 @@ static int __meminit memtier_hotplug_callback(struct notifier_block *self, > > static int __init memory_tier_init(void) > { > - int ret, node; > - struct memory_tier *memtier; > + int ret; > > ret = subsys_virtual_register(&memory_tier_subsys, NULL); > if (ret) > @@ -887,7 +897,8 @@ static int __init memory_tier_init(void) > GFP_KERNEL); > WARN_ON(!node_demotion); > #endif > - mutex_lock(&memory_tier_lock); > + > + guard(mutex)(&memory_tier_lock); > /* > * For now we can have 4 faster memory tiers with smaller adistance > * than default DRAM tier. > @@ -897,29 +908,9 @@ static int __init memory_tier_init(void) > if (IS_ERR(default_dram_type)) > panic("%s() failed to allocate default DRAM tier\n", __func__); > > - /* > - * Look at all the existing N_MEMORY nodes and add them to > - * default memory tier or to a tier if we already have memory > - * types assigned. > - */ > - for_each_node_state(node, N_MEMORY) { > - if (!node_state(node, N_CPU)) > - /* > - * Defer memory tier initialization on > - * CPUless numa nodes. These will be initialized > - * after firmware and devices are initialized. > - */ > - continue; > - > - memtier = set_node_memory_tier(node); > - if (IS_ERR(memtier)) > - /* > - * Continue with memtiers we are able to setup > - */ > - break; > - } > - establish_demotion_targets(); > - mutex_unlock(&memory_tier_lock); > + /* Record nodes with memory and CPU to set default DRAM performance. */ > + nodes_and(default_dram_nodes, node_states[N_MEMORY], > + node_states[N_CPU]); > > hotplug_memory_notifier(memtier_hotplug_callback, MEMTIER_HOTPLUG_PRI); > return 0; -- Best Regards, Huang, Ying