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 7C6C6C47DD9 for ; Fri, 22 Mar 2024 18:26:48 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0D1236B0083; Fri, 22 Mar 2024 14:26:48 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 080F86B0088; Fri, 22 Mar 2024 14:26:48 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E63EA6B008A; Fri, 22 Mar 2024 14:26:47 -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 CF11D6B0083 for ; Fri, 22 Mar 2024 14:26:47 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 92C62A0C31 for ; Fri, 22 Mar 2024 18:26:47 +0000 (UTC) X-FDA: 81925506054.21.A61632A Received: from mail-yb1-f181.google.com (mail-yb1-f181.google.com [209.85.219.181]) by imf08.hostedemail.com (Postfix) with ESMTP id 1E88B16000C for ; Fri, 22 Mar 2024 18:26:44 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=bytedance.com header.s=google header.b=JueXInZo; dmarc=pass (policy=quarantine) header.from=bytedance.com; spf=pass (imf08.hostedemail.com: domain of horenchuang@bytedance.com designates 209.85.219.181 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=1711132005; 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=6AUICcSJg/mLbMOQlQg8ZPtye+1Y8JCvK5pGCwWXtOo=; b=2/IPl9QvrHg5xlxq6u+fg3IeSIpEo3Oybao+IOFSuj3gUOqJ1M4KYX+Uv6ZS4guEVoACwF q+afSTce8y3rj3D1eFNfmtuTrh2IcZ+vxCi5QOX2OmTxxb9k7TNjZ6DVw1kxco4gfC1Ez9 VWuySfQERHVZiMuilsg/aB6G5HE58qg= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=bytedance.com header.s=google header.b=JueXInZo; dmarc=pass (policy=quarantine) header.from=bytedance.com; spf=pass (imf08.hostedemail.com: domain of horenchuang@bytedance.com designates 209.85.219.181 as permitted sender) smtp.mailfrom=horenchuang@bytedance.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1711132005; a=rsa-sha256; cv=none; b=omwl46aI6BjuSzxeO40ShWl860NH2HiONAd89N8gQNDN4LS/UoKAYUiuFzk3vGca343xf+ b3gzLmv5jso6dLOfEFhHCR7o6F4TRa/aK1eV5IHIbpD2uFeZRgxranWsEK9mu4yhi+ssLb zS2WbtnDWgbpj+WHCH7hZXSv1ovQJAI= Received: by mail-yb1-f181.google.com with SMTP id 3f1490d57ef6-dc236729a2bso2344553276.0 for ; Fri, 22 Mar 2024 11:26:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1711132004; x=1711736804; 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=6AUICcSJg/mLbMOQlQg8ZPtye+1Y8JCvK5pGCwWXtOo=; b=JueXInZohexHKoy4cPFT2tHPCeD3En2PjMiM2Cbwit1tXx5llwWP+EtFSwwpXi/UO/ 1f4AS2a+8QsvK9Y7rKLoe28NONxxlOnpLJMMQJHnEnBFQDUZ8BDQ7gx/RytRtu7cVimj BQ4q/uXTETuZw/l2KIULb+PC1wQ4t+zd2Z4GQ1ks9Lf74zIk6oey6LPqfi/HPnjgiclX rurVRj3JIqRFS+yGFilCxWRe+QJbSkD+weGpVrxJCdQVv34/NMnxAMSs08TLpZ9+3mAX cTY68XqPlv0cZ2PIOKc6tarEilsPaHwtSIbfA13N3/t/J1YNg4gHW5mwvN0ZS3LtYD3S eHyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711132004; x=1711736804; 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=6AUICcSJg/mLbMOQlQg8ZPtye+1Y8JCvK5pGCwWXtOo=; b=HUbbubDyJJIh+bDuBnwUWA6Kj4sKNXBuX/K5aSiRZjZN5YhO8h5eX/dTrKma9XFVF6 5ZGYGm1BTvNcQxOiX68daRHo37la9sYjKCav2S3Dg4P8vVmcEPORftGtnZle8+u/Igsq mTqDIa6xp3ktJ/u/FOOZ2AYmq7rnmCBsYiq1W8B0MPNUll54AT+Y0hCmA4RAnGjbb8O1 T7Bk9fL3Ifm7ntTL1619uY+SwRPXZKLlTR3M6j+qPxlXldreyrwC4mYvQXpTIa1hWATR ahc8DhNL8nvveJXxoHPzv6+KPNI0sWjfEB3YItVoG0tCeX4uSTXrDmF4iuxqUQT86gkV k4eQ== X-Forwarded-Encrypted: i=1; AJvYcCVkkkEG82/aE1freXpOPZUPm/q3Z8CuKiUOqwQU0wi+Jp+q7U3lf3EJSeXzWQLjS/V8lXUecUtdO1mH9sWwRk9UOZU= X-Gm-Message-State: AOJu0Yy0xk2GH++AYHCLppQaE1uOOnEhPbyle/IW5DoVDtZi9Lg2U/Wl YLITGKv9qqBuxtjuF/Q9xq/y1GV6+MjWInHj6sZWQwkJAj4uGY81kWYNB5o3SLvVvWG2l3KxH9g L6OvC6SEc5sT8V2YB/vS2kOy+qJL9BtmuKXXtdg== X-Google-Smtp-Source: AGHT+IFq+Vld2u9CEv92mxcPdGGVK+HlqojAbAarhNNW8pq520kH61lUaodFCsAExfRXZuH3hgY+ToVYiDIEhie12xw= X-Received: by 2002:a25:5303:0:b0:dda:a550:4e92 with SMTP id h3-20020a255303000000b00ddaa5504e92mr161782ybb.46.1711132004008; Fri, 22 Mar 2024 11:26:44 -0700 (PDT) MIME-Version: 1.0 References: <20240322070356.315922-1-horenchuang@bytedance.com> <20240322070356.315922-3-horenchuang@bytedance.com> <87cyrmr773.fsf@yhuang6-desk2.ccr.corp.intel.com> In-Reply-To: <87cyrmr773.fsf@yhuang6-desk2.ccr.corp.intel.com> From: "Ho-Ren (Jack) Chuang" Date: Fri, 22 Mar 2024 11:26:33 -0700 Message-ID: Subject: Re: [External] Re: [PATCH v4 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-Server: rspam09 X-Rspamd-Queue-Id: 1E88B16000C X-Stat-Signature: pep6yat33un8ygik6efuz4c6tq1tuqx8 X-Rspam-User: X-HE-Tag: 1711132004-796899 X-HE-Meta: U2FsdGVkX19rpnnKO7I09Gl4dpTa1P8RCgzN86FuOuAiLtS54bCwANyLZMo6+Z/T2uQ4CcNjKuLPc3vEf6R72m9enlRFxuCJs9NXYXX1JlE30suA+y5TK5ICFsbvStInlmWbzVnou3P4jvigbG91KwL23gqYkhEO+k0nO+0ywbpVgKvsC5kxi3mEhZS0CuqVVQIcBX8EU3txRZ+Uxw59fKmORnTjeGbezRJMEbF2EIspCCw1J90tMd9lZ/XE4J8WpukBspFk66g/8x5w7Fw4AWGH6es8OfAP86uOA6knGhdabg4LMUC03zuRVlQPuJ8262uWjuOGvPAU6LU31M+dkE4VItSNMJYVftdQ5YL7wnDvEQGFTDbGmm7J2EOLxdpCXjZbWQC+xe5+zkNScWDrzBtrrB2XtmJA1A6w5S4TJowUE8NbydKsiykGSpODI2OqY0v7LQ7dS0MHNXDI16kWNSp1sRiq82KC6xsPdgPdOqqCDRHWSFX3sL7UswRYnLz1CW7B7J3LqH7rHOXZVP2x7KAcaBxhN5+5fj2Mz3XMH3GYEZt0cUmSNsjmD447zu3WSoMaPZYi+XRgzwE3AsHxqli54FZJRhTnbRORCheI2PoKK/zwXI+l1ELLQuOvYOGtC1Q7aSLR/5AKyKxPNLH3Im7taC8Z6uQYLldq4pYdm4AqNI9hHvSoFP8dvcExG2gOp3x7Jp0SIVIlVBe8D0Ug1CscS5GYbXlia9WDLqGMyiFJU7JrvMBBZ1Mcsb/CKLLpA/cRKVhUS5nzZtMA6joErWhqy3CbtF0uNORvC/T72y5fW9lGKe7fT3QX80+0b7FOzIqqUPTwZA5MXjVuNAygTU0ONHWy+CNTGM/Uj+FC/pYCieqyIsFCpi/u/bhSWHjeCecuIYkpArri1p+FRm2GNeUlAvGSdJ+QUYWj4JszOKe/U106WM0REMmcECJqm9fVgkFQA21DDDGGY14rZlV sM1H0/BQ /DfSLAqdSr73cZvxwzPsj3HajYSrJaufazW+UMpjHAVwJpwmux6ip0GHByDjZevz8T87gCxuyYKlvU5E= 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 Fri, Mar 22, 2024 at 1:41=E2=80=AFAM 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. > > > > * 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. > > > > Signed-off-by: Ho-Ren (Jack) Chuang > > Signed-off-by: Hao Xiang > > --- > > mm/memory-tiers.c | 73 ++++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 63 insertions(+), 10 deletions(-) > > > > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c > > index 974af10cfdd8..9396330fa162 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,7 @@ static struct demotion_nodes *node_demotion __read_= mostly; > > > > static BLOCKING_NOTIFIER_HEAD(mt_adistance_algorithms); > > > > +static DEFINE_MUTEX(default_dram_perf_lock); > > Better to add comments about what is protected by this lock. > Thank you. I will add a comment like this: + /* The lock is used to protect `default_dram_perf*` info and nid. */ +static DEFINE_MUTEX(default_dram_perf_lock); I also found an error path was not handled and found the lock could be put closer to what it protects. I will have them fixed in V5. > > 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 +511,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; > > mtype may be referenced without initialization now below. > Good catch! Thank you. Please check below. I may found a potential NULL pointer dereference. > > + int adist =3D MEMTIER_ADISTANCE_DRAM; > > pg_data_t *pgdat =3D NODE_DATA(node); > > > > > > @@ -514,11 +521,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); If the `mtype` could be NULL, would there be a potential NULL pointer dereference. Do you have a preferred idea to fix this if needed? > > + memtier =3D find_create_memory_tier(mtype); > > if (!IS_ERR(memtier)) > > rcu_assign_pointer(pgdat->memtier, memtier); > > return memtier; > > @@ -655,6 +671,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 +712,7 @@ int mt_set_default_dram_perf(int nid, struct access= _coordinate *perf, > > { > > int rc =3D 0; > > > > - mutex_lock(&memory_tier_lock); > > + mutex_lock(&default_dram_perf_lock); > > if (default_dram_perf_error) { > > rc =3D -EIO; > > goto out; > > @@ -716,7 +760,7 @@ int mt_set_default_dram_perf(int nid, struct access= _coordinate *perf, > > } > > > > out: > > - mutex_unlock(&memory_tier_lock); > > + mutex_unlock(&default_dram_perf_lock); > > return rc; > > } > > > > @@ -732,7 +776,7 @@ int mt_perf_to_adistance(struct access_coordinate *= perf, int *adist) > > perf->read_bandwidth + perf->write_bandwidth =3D=3D 0) > > return -EINVAL; > > > > - mutex_lock(&memory_tier_lock); > > + mutex_lock(&default_dram_perf_lock); > > /* > > * The abstract distance of a memory node is in direct proportion= to > > * its memory latency (read + write) and inversely proportional t= o its > > @@ -745,7 +789,7 @@ 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); > > > > return 0; > > } > > @@ -858,7 +902,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 +913,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