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 4B0AACD1288 for ; Thu, 4 Apr 2024 00:21:37 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C078B6B00A1; Wed, 3 Apr 2024 20:21:36 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BB6E56B00A2; Wed, 3 Apr 2024 20:21:36 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A575A6B00A3; Wed, 3 Apr 2024 20:21:36 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 885746B00A1 for ; Wed, 3 Apr 2024 20:21:36 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 53AC81209BA for ; Thu, 4 Apr 2024 00:21:36 +0000 (UTC) X-FDA: 81969945792.28.F75F824 Received: from mail-yw1-f179.google.com (mail-yw1-f179.google.com [209.85.128.179]) by imf24.hostedemail.com (Postfix) with ESMTP id 9C1AE180009 for ; Thu, 4 Apr 2024 00:21:33 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=bytedance.com header.s=google header.b=EfOqrrKy; spf=pass (imf24.hostedemail.com: domain of horenchuang@bytedance.com designates 209.85.128.179 as permitted sender) smtp.mailfrom=horenchuang@bytedance.com; dmarc=pass (policy=quarantine) header.from=bytedance.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1712190094; 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=qO60jqsH0RDRW8MepZgYWUNAloZS/uU+ZxX94+OfopY=; b=h/wX3PIkI0a0BuDjfbWjUZNSD1SvG5AxMARdJ1JakxjCvnF/tOpOHCH77Jm4hMyh5XSFW5 jK9iAcV74T/cFggmSEjMbhn6re3UIu6lRX4NF7Eted7VUfcZKe/+3+9QVQ4kXdjhKwSgAY Bjzl68zT5Fsnipi5igOmtaLUVfDcJNg= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=bytedance.com header.s=google header.b=EfOqrrKy; spf=pass (imf24.hostedemail.com: domain of horenchuang@bytedance.com designates 209.85.128.179 as permitted sender) smtp.mailfrom=horenchuang@bytedance.com; dmarc=pass (policy=quarantine) header.from=bytedance.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712190094; a=rsa-sha256; cv=none; b=f0M6H1GUs5WWK4j+4+5h75DTEmtLm8rQdCkcTtO0HX+OmvkImvbR7PvfCT1PmY9OWIMCmQ FEaci7lLyf684Cd1SrJjBSOLIEhRqLUs5i/IGkw/KLqAYc/rmUIWIyULPxUKfztfVOjZzq ChEi9X5BPz6eFdf6DwZBLbLfKhJBd+g= Received: by mail-yw1-f179.google.com with SMTP id 00721157ae682-6154a3df493so12272497b3.1 for ; Wed, 03 Apr 2024 17:21:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1712190092; x=1712794892; 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=qO60jqsH0RDRW8MepZgYWUNAloZS/uU+ZxX94+OfopY=; b=EfOqrrKyES/KfdID+c4C0EM8uLEvptjuwjJs3gF06zDr+qDChOZZR1Gm9/2DVAXVDH sPQY7kRkb7qUu/4fi8buoDCTMhOmEBpsmZ+gR3o5FE7pm0m1ygqm7Uo1aZQDmqR3laO+ O8moqNkyik6SZgSMosZlmyiaHAuo0yFEHg/t+Sa4MWfih0erdfRXi7Qxpc9WxG5XqwBz HelT4qRkdKrUGysIaoH62bYPPTrsFQpQyp73cpU4Js1Q+6SODRGRJzHJ7Gs9mYPigu9L lvoOo8HF1QA5ZcrpuP9HHRFN/yJiobKN5Q3D8d2+kyMUO8C1MDSnbHfUW3p+LfMcJxNY pX2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712190092; x=1712794892; 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=qO60jqsH0RDRW8MepZgYWUNAloZS/uU+ZxX94+OfopY=; b=P3iRhs+x1MjG+Mb12GNKTJcbnABRDA1dpGPCtqRsPaPht0OEYG0Hvpaeq1HS9hnW8L GbGVRG2hynA6C7oDkpCEJQ0hzjF+QjoRf+YDJmnhpOmwb9hBa5Ax5AOQjdxxTTPCzSEA XUtqDEX4yCvzKkgYv18vCQLnCZ52pQW/UP1Mpno/0sCYnl34UFCi1S4wMaf3JWi4y0ep faO38eNcQhHJnW8jKowQhXugRfkPDcP+o2Uu3HaHccJbhUu6SYq5AykRB95KEMX4dbaH zngWsjDYx6G4rKLpwq2ASzEcgv8vGAbbjZDp2zmXFnr+9eW5Q+NiZGxLa3VqKCBnpYKk dAcQ== X-Forwarded-Encrypted: i=1; AJvYcCVaPip9C+ZyXjhiUCxrPcm2nuGFh/QMuCyl3Miho6Mljrk4Cr0qq8Y9oSH5Wxnv2zhddHDo7hAjgMz771sCFtiK/Zc= X-Gm-Message-State: AOJu0YyLAQq820WMyx+I2epaby9gHcMzHZFpF1XLwb9GtscgliaFYC7e 8tKUfv2lKkpO9DFJu9CYWFN7aBiKgCGIxphS/K3C4SgvSKC2U7gf72pG5EZ1TOl9X5/Q6W7Y/DS +03nXrK+OSQfnxW9NfW+ixhjeBxy/85KZPg913w== X-Google-Smtp-Source: AGHT+IHSYTr5M+525vcAEY5Aj2ib0qO6jpryNktw+PRCGQjz5qKA3WKMPc+zBf2F+1T8pwG+v1Tqmm74FhKD/Wr0Snw= X-Received: by 2002:a05:6902:1029:b0:dc7:46fd:4998 with SMTP id x9-20020a056902102900b00dc746fd4998mr1050153ybt.13.1712190092377; Wed, 03 Apr 2024 17:21:32 -0700 (PDT) MIME-Version: 1.0 References: <20240402001739.2521623-1-horenchuang@bytedance.com> <20240402001739.2521623-3-horenchuang@bytedance.com> <20240403180425.00003be0@Huawei.com> In-Reply-To: <20240403180425.00003be0@Huawei.com> From: "Ho-Ren (Jack) Chuang" Date: Wed, 3 Apr 2024 17:21:21 -0700 Message-ID: Subject: Re: [PATCH v10 2/2] memory tier: create CPUless memory tiers after obtaining HMAT info To: Jonathan Cameron Cc: "Huang, Ying" , 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 , Dan Williams , Vishal Verma , Dave Jiang , Andrew Morton , nvdimm@lists.linux.dev, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, Linux Memory Management List , "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: 9C1AE180009 X-Rspam-User: X-Stat-Signature: hjxkocf69733p4m8nej87jfeqorx69cb X-Rspamd-Server: rspam01 X-HE-Tag: 1712190093-545802 X-HE-Meta: U2FsdGVkX1/kwEh8IK4tZNjIlrKsZMbLUrIs3C7MvhIhAdBuNOF3YefCRQXSJv1vyPtx0DlOOuYSm0kkWu2+cPDWroD3uBASTn9srf4K38BOeTpk0+thXZxbooFUzdZsUwh7c9QV70oL8HfDa6sP+iT6MBBcnWPwNE9GVOaZc+2S+e+O6TBngV2WG/P7EsX60k1Z4X12oj0X26Z1aLZ1UK+tS6+jBCOWjKFeR88GD2QBtx3jojkb5jafl2jCgqqPcYEjRaoYQYv7Dh206kbDEY6p9RKN1pHFmeuA2w3O5urlKjuYzGtrHZCi2R1YJgw9L/nbWI1MxCkc8SSy63wglG5PtbCQMWG5vc4T0zZYOMgnO2/9UG1E1+dzeswbO2KbBdF7tKzyFXjNhibGzQUaYYv925vfIlEGCazR92vqWZPfCDgQHoieEuC+A+x+xzT0EEkj6WqtxKw5RsirgqKmIZu3uR5BALiyn8IgnTf6yDNKgBKxXq+jzYYOoti/Lj5Px4UK/Tk4Sq8c7AUXM8Q5DMBoPTEYbj38pulXAhY63BEdD3nYEesDgZ8vCaOFAgIwpdI7VAWpGIq0oIAfvfF4H6HFlI/7mHgd4iZtafryj6KcSHOxuIkwFRoRjYJlBY/ZOH3TpbmkMzM0rKKK2pYP63qxdQvpZuZSXTerUefTk7DbtaUOd0tDLD5l/ZWfmMmpydp4qiwIM+Ogu2H88V7YxmwesZkfwXY0+gmBB7TkaZ09kH3Zq6HaEhyvv/OWR7iQ6HN5m2H66uVxcc5uRe/nDSvl5iov+Lwsvm6GJPAl3kVA3fvQ8owVe6cOoT+9IHT0uQ5bX8Lf8NjsRreo4n63xlvHOvhkbZlcCT2T4ummCJoi5M5c5Q0EMsJwlpkPUPmYtfkebFH2t/9ixiYXpnoF05a4TOk8ASdoCrurc87GfKs8rsw75R9l1cXMUWuOk9uBCzTBg4vTWbZ2u9ToA9k zegxYxTc 4djU38N3hUahsIgsl03Ez1WvEFowFvrvZe1G4A0gnq19SMO56UuSh0qfdhjQpfynT3Y5B18zremdJarL48q1uksNTIka0VQm6IqCnoELURhyH41DPeRD4X5ed7iB+YWcL+ZujbvFoe7luTjcxP9TH03tMeuEZ3+tlLPSvleODkSd5idA/1OurZ3lKSUX7hsnS2b33AxvDs1VL59vXu5/vFknsGbR+XJfYsENZCADT0zV0fyLwPG1LIhqmudhh/BOs1VC4nDF3eMavPIsNnw3oMMt7yAek23vnlypbAH01v2/wlLdlGDY7U1j6yA+4iMsSl7B/hl+6faj2F4q5F5cOoqY14w== 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 Jonathan, Thank you for your feedback. I will fix them (inlined) in the next V11. On Wed, Apr 3, 2024 at 10:04=E2=80=AFAM Jonathan Cameron wrote: > > A few minor comments inline. > > > diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.= h > > index a44c03c2ba3a..16769552a338 100644 > > --- a/include/linux/memory-tiers.h > > +++ b/include/linux/memory-tiers.h > > @@ -140,12 +140,13 @@ static inline int mt_perf_to_adistance(struct acc= ess_coordinate *perf, int *adis > > return -EIO; > > } > > > > -struct memory_dev_type *mt_find_alloc_memory_type(int adist, struct li= st_head *memory_types) > > +static inline struct memory_dev_type *mt_find_alloc_memory_type(int ad= ist, > > + struct list_head *memory_types) > > { > > return NULL; > > } > > > > -void mt_put_memory_types(struct list_head *memory_types) > > +static inline void mt_put_memory_types(struct list_head *memory_types) > > { > Why in this patch and not previous one? I've also noticed this issue. I will fix it in the next V11. > > > > } > > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c > > index 974af10cfdd8..44fa10980d37 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; > > Does the rename add anything major to the patch? > If not I'd leave it alone to reduce the churn and give > a more readable patch. If it is worth doing perhaps > a precursor patch? > Either name works. Keeping it the same name will make the code easier to follow. I agree! Thanks. > > + 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"); > > + } > > + } > > + > > + __init_node_memory_type(node, mtype); > > > > - memtype =3D node_memory_types[node].memtype; > > - node_set(node, memtype->nodes); > > - memtier =3D find_create_memory_tier(memtype); > > + 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,33 @@ 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_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. > > + */ > > Does the comment refer to this path, or to ones where memtype is set? > Yes, the comment is for explaining why the if condition is used. > > + set_node_memory_tier(nid); > > Given the large comment I would add {} to help with readability. > You could flip the logic to reduce indent > for_each_node_state(nid, N_MEMORY) { > if (node_memory_types[nid].memtype) > continue; > /* > * Some device drivers may have initialized memory tiers > * between `memory_tier_init()` and `memory_tier_late_ini= t()`, > * potentially bringing online memory nodes and > * configuring memory tiers. Exclude them here. > */ > set_node_memory_tier(nid); > > I will change it accordingly. > > + > > + 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); > > As below, this is a classic case where guard() will help readability. > I will change it accordingly. > > if (default_dram_perf_error) { > > rc =3D -EIO; > > goto out; > > @@ -716,23 +760,30 @@ 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) > > { > > - if (default_dram_perf_error) > > - return -EIO; > > + int rc =3D 0; > > Looks like rc is set in all paths that reach where it issued. > Using guard(mutex), I will no longer need `int rc`. Replace `rc =3D` with `return XXX`. > > > > - if (default_dram_perf_ref_nid =3D=3D NUMA_NO_NODE) > > - return -ENOENT; > > + mutex_lock(&default_dram_perf_lock); > > This would benefit quite a lot from > guard(mutex)(&default_dram_perf_lock); > and direct returns throughout. > Got it. I will change it accordingly. > > > + if (default_dram_perf_error) { > > + rc =3D -EIO; > > + goto out; > > + } > > > > if (perf->read_latency + perf->write_latency =3D=3D 0 || > > - perf->read_bandwidth + perf->write_bandwidth =3D=3D 0) > > - return -EINVAL; > > + perf->read_bandwidth + perf->write_bandwidth =3D=3D 0) { > > + rc =3D -EINVAL; > > + goto out; > > + } > > > > - mutex_lock(&memory_tier_lock); > > + 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,9 +796,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); > > > > - return 0; > > +out: > > + mutex_unlock(&default_dram_perf_lock); > > + return rc; > > } > > EXPORT_SYMBOL_GPL(mt_perf_to_adistance); > > > > @@ -858,7 +910,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); > > Unusual indenting. Align with just after ( > Aligning with "(" will exceed 100 columns. Would that be acceptable? > > if (IS_ERR(default_dram_type)) > > panic("%s() failed to allocate default DRAM tier\n", __fu= nc__); > > > > @@ -868,6 +921,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 > > I think this wraps at just over 80 chars. Seems silly to wrap so tightly= and not > quite fit under 80. (this is about 83 chars. > I can fix this. I have a question. From my patch, this is <80 chars. However, in an email, this is >80 chars. Does that mean we need to count the number of chars in an email, not in a patch? Or if I missed something? like vim configuration or? > > + * initialized. > > + */ > > + continue; > > + > > memtier =3D set_node_memory_tier(node); > > if (IS_ERR(memtier)) > > /* > --=20 Best regards, Ho-Ren (Jack) Chuang =E8=8E=8A=E8=B3=80=E4=BB=BB