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 0D788C54E67 for ; Wed, 27 Mar 2024 06:55:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 75CB16B0098; Wed, 27 Mar 2024 02:55:23 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6E6196B0099; Wed, 27 Mar 2024 02:55:23 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 539336B009A; Wed, 27 Mar 2024 02:55:23 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 3B9E76B0098 for ; Wed, 27 Mar 2024 02:55:23 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id CDB77A0A65 for ; Wed, 27 Mar 2024 06:55:22 +0000 (UTC) X-FDA: 81941907684.17.C9D38CD Received: from mail-yb1-f169.google.com (mail-yb1-f169.google.com [209.85.219.169]) by imf26.hostedemail.com (Postfix) with ESMTP id 45196140009 for ; Wed, 27 Mar 2024 06:55:20 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=bytedance.com header.s=google header.b=GxqV4i0q; dmarc=pass (policy=quarantine) header.from=bytedance.com; spf=pass (imf26.hostedemail.com: domain of horenchuang@bytedance.com designates 209.85.219.169 as permitted sender) smtp.mailfrom=horenchuang@bytedance.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1711522521; 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=2gBdAxMvMnjsczXEA3QG1dVLh7HlPI7SpPCL+teq1rw=; b=unEXztKbVbJd3YYZknNu2uk446tKidgkhSg4ERDr/U7xjQX4YN4vd8Sl8+/gPiHm6ZCdwT py1MTdrSeWtekBLYWpTgILfFVE+WyaGvzC68qdS686fkXqzqSeLwTbwY5OVDRgy7JNsfZr vnTKNGlwewhNWRGm5XBvLoO3VLLxLSM= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=bytedance.com header.s=google header.b=GxqV4i0q; dmarc=pass (policy=quarantine) header.from=bytedance.com; spf=pass (imf26.hostedemail.com: domain of horenchuang@bytedance.com designates 209.85.219.169 as permitted sender) smtp.mailfrom=horenchuang@bytedance.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1711522521; a=rsa-sha256; cv=none; b=XLrrLUfO4Slt6im09xlML/xqIoin1xAL15FJQlXGM2af+8/yT/GFEiijWRm6t/tlomYXbx UkZyXgVlxRA0bjANU7f5NRafpC2QI3XUWaITNDHGgEFjtvPodzl1g7AIU0W8V7/LjNzjzm SJn/Z2AR1R9pn0DdtffY16VkLmOBMcM= Received: by mail-yb1-f169.google.com with SMTP id 3f1490d57ef6-dc742543119so6135329276.0 for ; Tue, 26 Mar 2024 23:55:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1711522519; x=1712127319; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=2gBdAxMvMnjsczXEA3QG1dVLh7HlPI7SpPCL+teq1rw=; b=GxqV4i0q+9zgpXhaBIUBviIylRrylJZAyIvOEJMZFopzcx/TngVZJIM/O/zcRVtalI /+IJQodtv5keWDJh8Th5Y3ALKDVNOLePHx4f/bkRjtgeItFaM0eRDQb7NHaus9ZfS/TE PiCeF97rKZcsE1MAuHolkHWwgUtt9pryiN12o+Jjtsc0N7KZWH0WRE+SdJVukIYeiC7/ P4FuL+7jc0a5/aeMgyMGUuxKdpB0DfXdWtkb+zTzQMH4cHAmQeJlygXJ+QRJ897236Oz /jU2wc4Iw6h0jaeuzicONwkOR/yHrA6v/TvHWxvqxIrM1zUsMt+Z9HolbMOg8BjjoWCv zxYw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711522519; x=1712127319; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=2gBdAxMvMnjsczXEA3QG1dVLh7HlPI7SpPCL+teq1rw=; b=pmTWu2UB2HzK+TdRN9DnTgBzEEj8a3lNO4cBTLfp3IiLdwAigZBWvPvnPu01B+6ENR o1xAAwSR5MEwcCGxY2qXVdLZHjpGzOuFGVnY704PCPxCkfl3FEiYI6DJem17l7lHpE7H e0+uGibN5KZdQWPdDgnQXdICn7Xzw1EO5CWkvk9rflMHXFeB6Jo0GrwO8Hm5bxh8j3o2 fBDZ7vgA/ILj8zk6wZDqq5QuH2hWUSH1b3ryeSG564xEb3mNE/Ojx6gi6l9JhOU3dGGt oPVTCgWvql6K7v3KXwsqAZaZG12wKFZBUWEU+OAMJupekA6O/rLzfA89JuyJOZ3YMtRf 7xfQ== X-Forwarded-Encrypted: i=1; AJvYcCVkSjYVtttZDPnSkFg5qUwYMUITtATjpvTr6ZuGsrjxbmtP0C+CZboYo5nev4kMuUCUWHiklYyzjDRYDTyozsC1VRw= X-Gm-Message-State: AOJu0YzyMW+KzCxpIh32ZAMb1NXQFEwrgVEHI0yVgAgNhQC8/BVgYf9s hp4alkUw4mdXpnyXt0lVe5no7FRHOwHim5ApkfJN23y4l//csG78R6+FoLULu526b0oJ0HJGPuX pbpBK2HWySiqV4lZrojWXUcmpnVW024U0UIht/A== X-Google-Smtp-Source: AGHT+IGneaW9WE/Y0nsKRo0D7anGiHOi7fBK7mx80rP9apScsfFYIjDTSuH8hoPcCeMmppqQ4avESF7WEPInBimAGLE= X-Received: by 2002:a25:a009:0:b0:dcf:b5b7:c72 with SMTP id x9-20020a25a009000000b00dcfb5b70c72mr2835965ybh.0.1711522518886; Tue, 26 Mar 2024 23:55:18 -0700 (PDT) MIME-Version: 1.0 References: <20240327041646.3258110-1-horenchuang@bytedance.com> <20240327041646.3258110-3-horenchuang@bytedance.com> <874jcsnryv.fsf@yhuang6-desk2.ccr.corp.intel.com> In-Reply-To: <874jcsnryv.fsf@yhuang6-desk2.ccr.corp.intel.com> From: "Ho-Ren (Jack) Chuang" Date: Tue, 26 Mar 2024 23:55:07 -0700 Message-ID: Subject: Re: [External] Re: [PATCH v5 2/2] memory tier: create CPUless memory tiers after obtaining HMAT info To: "Huang, Ying" Cc: 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 , Dan Williams , Vishal Verma , Dave Jiang , Andrew Morton , nvdimm@lists.linux.dev, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, "Ho-Ren (Jack) Chuang" , "Ho-Ren (Jack) Chuang" , qemu-devel@nongnu.org, Hao Xiang Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 45196140009 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: aqg3qyyhq76fwcnaamknjzyr9fr3io1y X-HE-Tag: 1711522520-773375 X-HE-Meta: U2FsdGVkX1+1fYSZ57yu8vZfLDidCQ/9DpcGkvdnhRCft+ZHOQNKU/xv7+WVaPdZZfw1Ib9ivnTYXrkiPrCMto7E2x/9Wt7GoVUoAuQkk9JX64AFnTLs4JJQrzI68gqtz9xzToztvkxEk1Q8B47ywyD2Va1/lJtB75Vk6+664iVlO8Zw3j5u5/MYZcjaiIDIhCACdDs9x6KVGF/sIVVk/+jCsGWsrOFEqx/+gQF8b6XrRwBWrOiNIdJYLwlfxvz8FICOpF4voGjZBuM6+2ijGXd7XDAnAmc95ksFVLDVjMNfTIzqUkslWFijqLUevpF16Q48MxcHwpGR9gRyslUfYmLugnl8X1X3bUq/M1Ef6/7P8vNNvqEPyjMrMyRGird09nynLJNRW5760/shj8Ng7V8M+Biftyu/4ft7R36NrFX0Eul/biGq15oyTXsHtNgLY4EQZf7aDmBqRqa37GCpFjtajlSBsL64pSt+qzVAM4caj5YAsPmjtiswzXkQc0gObF9LywIMRrPjDHRurV6HFtcJe84cbYmRw/449xyKuNW7uBCkOHABebjQP7nHwyuDWpSnnAW8fCR+45/fjYj3kEWbch+FcTea4JVyh/i8+jQMETtUdv7bVsFoIUbAffv9JgGuyCGef7BvKWCIyR9BmLh3NWAtm7zhWsPpqkKa6eR9PFZoh3dTnQOQmViV3nryzJEsbv327MP09hlHlJZbcm2UdD6py2qcevsmVsmWo9NBzCbG/Wb9xpz0LxahuoMMgymLpEC1bIu0xtwAP/9GfaWuDb/z4Eoac73vrtcDMGTRMzv+qd+1PXVDFHeykbZ8TALz+M5aT5xI7v3kOeicNjkPzgfpKm/OAoICvZWA2RT5P/lskB7673naqrUgjeOUzb5sJHe88m6KFJvm6cjfyB6H0kc69HKoSlUfrKQ32yfyau4mExttUcX0nB5SDGLShDL8ismsvky3WPcZT/G VQ2fudAA x3oHyB19zFPTTQ1BTTUUhQ4DFEDtOt3u47yI5UlIq1bRnqw6B1mOrdziTyTEz1jXn+Bkw4lAm6QElJLXD8x+psIKTFmtcMTqdEd0Jg3k2pxgUeCdsBqv+dGzQVB2jB6bNRvYV5Kj4P34lj/Z/8228//Txuqp3VBRmUHVyHW5IIBLjSn0gdbQZslzpSk/aeSdkYNDtOGlo6sZ10LzGecn0YFUF+DXImfGzf8oM8pkM/QPATeQEuuEtlY0Ue/qGXjAVX/1begjW19t2MjIlMRcSpZ5pMK0IZ9C8D8LDQo64Ec5EY9np7h35sFWJ4w== 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: On Tue, Mar 26, 2024 at 10:52=E2=80=AFPM Huang, Ying = wrote: > > "Ho-Ren (Jack) Chuang" writes: > > > The current implementation treats emulated memory devices, such as > > CXL1.1 type3 memory, as normal DRAM when they are emulated as normal me= mory > > (E820_TYPE_RAM). However, these emulated devices have different > > characteristics than traditional DRAM, making it important to > > distinguish them. Thus, we modify the tiered memory initialization proc= ess > > to introduce a delay specifically for CPUless NUMA nodes. This delay > > ensures that the memory tier initialization for these nodes is deferred > > until HMAT information is obtained during the boot process. Finally, > > demotion tables are recalculated at the end. > > > > * late_initcall(memory_tier_late_init); > > Some device drivers may have initialized memory tiers between > > `memory_tier_init()` and `memory_tier_late_init()`, potentially bringin= g > > online memory nodes and configuring memory tiers. They should be exclud= ed > > in the late init. > > > > * Handle cases where there is no HMAT when creating memory tiers > > There is a scenario where a CPUless node does not provide HMAT informat= ion. > > If no HMAT is specified, it falls back to using the default DRAM tier. > > > > * Introduce another new lock `default_dram_perf_lock` for adist calcula= tion > > In the current implementation, iterating through CPUlist nodes requires > > holding the `memory_tier_lock`. However, `mt_calc_adistance()` will end= up > > trying to acquire the same lock, leading to a potential deadlock. > > Therefore, we propose introducing a standalone `default_dram_perf_lock`= to > > protect `default_dram_perf_*`. This approach not only avoids deadlock > > but also prevents holding a large lock simultaneously. Besides, this pa= tch > > slightly shortens the time holding the lock by putting the lock closer = to > > what it protects as well. > > > > * Upgrade `set_node_memory_tier` to support additional cases, including > > default DRAM, late CPUless, and hot-plugged initializations. > > To cover hot-plugged memory nodes, `mt_calc_adistance()` and > > `mt_find_alloc_memory_type()` are moved into `set_node_memory_tier()` t= o > > handle cases where memtype is not initialized and where HMAT informatio= n is > > available. > > > > * Introduce `default_memory_types` for those memory types that are not > > initialized by device drivers. > > Because late initialized memory and default DRAM memory need to be mana= ged, > > a default memory type is created for storing all memory types that are > > not initialized by device drivers and as a fallback. > > > > * Fix a deadlock bug in `mt_perf_to_adistance` > > Because an error path was not handled properly in `mt_perf_to_adistance= `, > > unlock before returning the error. > > > > Signed-off-by: Ho-Ren (Jack) Chuang > > Signed-off-by: Hao Xiang > > --- > > mm/memory-tiers.c | 85 +++++++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 72 insertions(+), 13 deletions(-) > > > > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c > > index 974af10cfdd8..610db9581ba4 100644 > > --- a/mm/memory-tiers.c > > +++ b/mm/memory-tiers.c > > @@ -36,6 +36,11 @@ struct node_memory_type_map { > > > > static DEFINE_MUTEX(memory_tier_lock); > > static LIST_HEAD(memory_tiers); > > +/* > > + * The list is used to store all memory types that are not created > > + * by a device driver. > > + */ > > +static LIST_HEAD(default_memory_types); > > static struct node_memory_type_map node_memory_types[MAX_NUMNODES]; > > struct memory_dev_type *default_dram_type; > > > > @@ -108,6 +113,8 @@ static struct demotion_nodes *node_demotion __read_= mostly; > > > > static BLOCKING_NOTIFIER_HEAD(mt_adistance_algorithms); > > > > +/* The lock is used to protect `default_dram_perf*` info and nid. */ > > +static DEFINE_MUTEX(default_dram_perf_lock); > > static bool default_dram_perf_error; > > static struct access_coordinate default_dram_perf; > > static int default_dram_perf_ref_nid =3D NUMA_NO_NODE; > > @@ -505,7 +512,8 @@ static inline void __init_node_memory_type(int node= , struct memory_dev_type *mem > > static struct memory_tier *set_node_memory_tier(int node) > > { > > struct memory_tier *memtier; > > - struct memory_dev_type *memtype; > > + struct memory_dev_type *mtype =3D default_dram_type; > > + int adist =3D MEMTIER_ADISTANCE_DRAM; > > pg_data_t *pgdat =3D NODE_DATA(node); > > > > > > @@ -514,11 +522,20 @@ static struct memory_tier *set_node_memory_tier(i= nt node) > > if (!node_state(node, N_MEMORY)) > > return ERR_PTR(-EINVAL); > > > > - __init_node_memory_type(node, default_dram_type); > > + mt_calc_adistance(node, &adist); > > + if (node_memory_types[node].memtype =3D=3D NULL) { > > + mtype =3D mt_find_alloc_memory_type(adist, &default_memor= y_types); > > + if (IS_ERR(mtype)) { > > + mtype =3D default_dram_type; > > + pr_info("Failed to allocate a memory type. Fall b= ack.\n"); > > + } > > + } > > > > - memtype =3D node_memory_types[node].memtype; > > - node_set(node, memtype->nodes); > > - memtier =3D find_create_memory_tier(memtype); > > + __init_node_memory_type(node, mtype); > > + > > + mtype =3D node_memory_types[node].memtype; > > + node_set(node, mtype->nodes); > > + memtier =3D find_create_memory_tier(mtype); > > if (!IS_ERR(memtier)) > > rcu_assign_pointer(pgdat->memtier, memtier); > > return memtier; > > @@ -655,6 +672,34 @@ void mt_put_memory_types(struct list_head *memory_= types) > > } > > EXPORT_SYMBOL_GPL(mt_put_memory_types); > > > > +/* > > + * This is invoked via `late_initcall()` to initialize memory tiers fo= r > > + * CPU-less memory nodes after driver initialization, which is > > + * expected to provide `adistance` algorithms. > > + */ > > +static int __init memory_tier_late_init(void) > > +{ > > + int nid; > > + > > + mutex_lock(&memory_tier_lock); > > + for_each_node_state(nid, N_MEMORY) > > + if (!node_state(nid, N_CPU) && > > + node_memory_types[nid].memtype =3D=3D NULL) > > + /* > > + * Some device drivers may have initialized memor= y tiers > > + * between `memory_tier_init()` and `memory_tier_= late_init()`, > > + * potentially bringing online memory nodes and > > + * configuring memory tiers. Exclude them here. > > + */ > > + set_node_memory_tier(nid); > > + > > + establish_demotion_targets(); > > + mutex_unlock(&memory_tier_lock); > > + > > + return 0; > > +} > > +late_initcall(memory_tier_late_init); > > + > > static void dump_hmem_attrs(struct access_coordinate *coord, const cha= r *prefix) > > { > > pr_info( > > @@ -668,7 +713,6 @@ int mt_set_default_dram_perf(int nid, struct access= _coordinate *perf, > > { > > int rc =3D 0; > > > > - mutex_lock(&memory_tier_lock); > > if (default_dram_perf_error) { > > rc =3D -EIO; > > goto out; > > @@ -680,6 +724,7 @@ int mt_set_default_dram_perf(int nid, struct access= _coordinate *perf, > > goto out; > > } > > > > + mutex_lock(&default_dram_perf_lock); > > Why do you move the position of locking? mutex_lock/unlock() will be > unbalance for error path above. > Because you've mentioned below that moving the lock to the beginning of the function will make the code easier to understand, I will move the lock to the beginning of the function. Perhaps the explanation may no longer be relevant; because reading `default_dram_perf_error` and `perf->*` do not require holding `default_dram_perf_lock`, but I forgot to replace "rc =3D -EXXX ; goto out;" with return -EXXX. > > if (default_dram_perf_ref_nid =3D=3D NUMA_NO_NODE) { > > default_dram_perf =3D *perf; > > default_dram_perf_ref_nid =3D nid; > > @@ -716,23 +761,26 @@ int mt_set_default_dram_perf(int nid, struct acce= ss_coordinate *perf, > > } > > > > out: > > - mutex_unlock(&memory_tier_lock); > > + mutex_unlock(&default_dram_perf_lock); > > return rc; > > } > > > > int mt_perf_to_adistance(struct access_coordinate *perf, int *adist) > > { > > + int rc =3D 0; > > + > > if (default_dram_perf_error) > > return -EIO; > > > > - if (default_dram_perf_ref_nid =3D=3D NUMA_NO_NODE) > > - return -ENOENT; > > - > > if (perf->read_latency + perf->write_latency =3D=3D 0 || > > perf->read_bandwidth + perf->write_bandwidth =3D=3D 0) > > return -EINVAL; > > > > - mutex_lock(&memory_tier_lock); > > + mutex_lock(&default_dram_perf_lock); > > It may be a little better to move lock position at the begin of the > function. This will not avoid race condition (not harmful in practice) > but it will make code easier to be understood. > No problem. I will move the lock to the beginning of the function and take care of all error paths. > > + if (default_dram_perf_ref_nid =3D=3D NUMA_NO_NODE) { > > + rc =3D -ENOENT; > > + goto out; > > + } > > /* > > * The abstract distance of a memory node is in direct proportion= to > > * its memory latency (read + write) and inversely proportional t= o its > > @@ -745,8 +793,10 @@ int mt_perf_to_adistance(struct access_coordinate = *perf, int *adist) > > (default_dram_perf.read_latency + default_dram_perf.write= _latency) * > > (default_dram_perf.read_bandwidth + default_dram_perf.wri= te_bandwidth) / > > (perf->read_bandwidth + perf->write_bandwidth); > > - mutex_unlock(&memory_tier_lock); > > + mutex_unlock(&default_dram_perf_lock); > > > > +out: > > + mutex_unlock(&default_dram_perf_lock); > > return 0; > > } > > EXPORT_SYMBOL_GPL(mt_perf_to_adistance); > > @@ -858,7 +908,8 @@ static int __init memory_tier_init(void) > > * For now we can have 4 faster memory tiers with smaller adistan= ce > > * than default DRAM tier. > > */ > > - default_dram_type =3D alloc_memory_type(MEMTIER_ADISTANCE_DRAM); > > + default_dram_type =3D mt_find_alloc_memory_type(MEMTIER_ADISTANCE= _DRAM, > > + &= default_memory_types); > > if (IS_ERR(default_dram_type)) > > panic("%s() failed to allocate default DRAM tier\n", __fu= nc__); > > > > @@ -868,6 +919,14 @@ static int __init memory_tier_init(void) > > * types assigned. > > */ > > for_each_node_state(node, N_MEMORY) { > > + if (!node_state(node, N_CPU)) > > + /* > > + * Defer memory tier initialization on CPUless nu= ma nodes. > > + * These will be initialized after firmware and d= evices are > > + * initialized. > > + */ > > + continue; > > + > > memtier =3D set_node_memory_tier(node); > > if (IS_ERR(memtier)) > > /* > > -- > Best Regards, > Huang, Ying --=20 Best regards, Ho-Ren (Jack) Chuang =E8=8E=8A=E8=B3=80=E4=BB=BB