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 C5FFFCD128A for ; Wed, 3 Apr 2024 23:13:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 45B206B0087; Wed, 3 Apr 2024 19:13:38 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 40B306B0088; Wed, 3 Apr 2024 19:13:38 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2D2C46B0089; Wed, 3 Apr 2024 19:13:38 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 108996B0087 for ; Wed, 3 Apr 2024 19:13:38 -0400 (EDT) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id B7BF1407D6 for ; Wed, 3 Apr 2024 23:13:37 +0000 (UTC) X-FDA: 81969774474.02.981768D Received: from mail-yb1-f182.google.com (mail-yb1-f182.google.com [209.85.219.182]) by imf29.hostedemail.com (Postfix) with ESMTP id 01BC2120017 for ; Wed, 3 Apr 2024 23:13:35 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=bytedance.com header.s=google header.b=CtqmvQtP; spf=pass (imf29.hostedemail.com: domain of horenchuang@bytedance.com designates 209.85.219.182 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=1712186016; 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=SXQkJndwO2OFb1B4uAhm6DSgUpXUEz9T4TSxXyfE+FM=; b=67YWgiFhneYlL3mrCpWNHi7BZe9pPAsh1uqAKuKYIo0a7uc5S0YGU0RDxWv+FxKJVUExFT 0JorAqOqie7scbMTny0lO0VGt2sxVxYQC+axjQzR9i0lcB4GX8MvrRn5LReb4XeQsudUD1 epzmZKTJEbwlsm/FoQJMY5sh4dBa4ww= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712186016; a=rsa-sha256; cv=none; b=8Nq0lyaPZ1gPJxPSEYUonrUGOjtyaKsXUszOJV7NHbJmHmnMTgjA23cLm0VA71h4xX/MHS zHwJaP4M+8E9o/uXxOtot8WjiAujxFasNW6uXzpbiVj9Y5eaWL9V7ozg5g2IBzK5Unw7cY RzY8D+m9ljF/CsL3BeYBif9NaBSn9iQ= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=bytedance.com header.s=google header.b=CtqmvQtP; spf=pass (imf29.hostedemail.com: domain of horenchuang@bytedance.com designates 209.85.219.182 as permitted sender) smtp.mailfrom=horenchuang@bytedance.com; dmarc=pass (policy=quarantine) header.from=bytedance.com Received: by mail-yb1-f182.google.com with SMTP id 3f1490d57ef6-dc6d8bd618eso412442276.3 for ; Wed, 03 Apr 2024 16:13:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1712186015; x=1712790815; 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=SXQkJndwO2OFb1B4uAhm6DSgUpXUEz9T4TSxXyfE+FM=; b=CtqmvQtPuNj5Xpz9kdGLcEiq718bfFbjkB9yfiobmLEmjeHrDOQ+MCJx2dI/pax/R+ RVgOMyeqCuwZKw/jKuE40aoBR3AnwG35hRZAy7QQE0DFq8sZu6SCLbycFia+p3M17FlD 25+JGq8DfGsg0v3BxOAm7EycZ17/lif39wDgU7cnKSUUkChBsuK7BzjlyOhQ+5vkZVXw 4bS37wKii+PauvGNm+LuwamMtKZz7LqdEZweGFLVGQhB4Gq3Sj1F2OyWPw8kbEx19Hpe mlvRJKOilgQgF8yEjCQycTTR9XvOzW3BJiPFKkihovz1bKyLl2bFUgDcJyqx0vsV/hGX HuqA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712186015; x=1712790815; 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=SXQkJndwO2OFb1B4uAhm6DSgUpXUEz9T4TSxXyfE+FM=; b=Gbv8D3EKN+CW0fJtPb5zsmWgfS25W5zlValIKSHV8jILX9KH07bJDo9Cs6lT9ariNe J/lYYflM+Vx1/7MmalC+gJ4lvNrNujQ0CpfvmZMuTdvdEXResnbLBLfdxT7avKioRoRu M2uZDIc/nI1uX6an34VhoLi6N4jkqpRT/+YDBFo3o9KDg1NGikrFPLyq/LPnS2AoTyhH 6hPz+H+214z5o84zJYCHqmhokp+1V+7MahsIYy6z0xEPrtz72m80Ze+le+zvAHwccj6t Qlk0kvu2zCuekvyNBKEh2xptP9/7krcaGTHx5GKqIvfdidDRmgTmLnve9AUyp1sO8K7n Y6OA== X-Forwarded-Encrypted: i=1; AJvYcCVj5H/zYrskM1T0l46f0Jqsd9JQC0oejDo4ZyBOdmQ7SmD1hJcGb5+EP7YaHxyXzy8FU2VkOkG0H6cFph+F19P/76k= X-Gm-Message-State: AOJu0YxmQhGfH5FEtSHHcFG9QVfEQG8v2zxnSNR6sirJtWPP9D/zAhpx yBeSLAwQ4bfHFn3RiLUnTgnZjPR1JYE1tS4tDFTTbgplk4o9jPAE315JoWdyYpSRFmUFpV9/dzA 7UbrwlfzqtkgIL6O2BrpUdq+pZKDtTgUgn9vdyw== X-Google-Smtp-Source: AGHT+IFVeLOeseHn7jgUi35tqoQdrprRgFyBUayTJ7qIeLYhEuQo2z8SpiV7Er/5CVQCTXOehRej6DG8/caODWdPemM= X-Received: by 2002:a25:8189:0:b0:dc2:3936:5fa5 with SMTP id p9-20020a258189000000b00dc239365fa5mr828148ybk.51.1712186014869; Wed, 03 Apr 2024 16:13:34 -0700 (PDT) MIME-Version: 1.0 References: <20240402001739.2521623-1-horenchuang@bytedance.com> <20240402001739.2521623-2-horenchuang@bytedance.com> <20240403175201.00000c2c@Huawei.com> In-Reply-To: <20240403175201.00000c2c@Huawei.com> From: "Ho-Ren (Jack) Chuang" Date: Wed, 3 Apr 2024 16:13:24 -0700 Message-ID: Subject: Re: [PATCH v10 1/2] memory tier: dax/kmem: introduce an abstract layer for finding, allocating, and putting memory types 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: 67jh3i7af8n8rux1uarq8skxco7aimi6 X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 01BC2120017 X-Rspam-User: X-HE-Tag: 1712186015-459460 X-HE-Meta: U2FsdGVkX1/wEwesDiWAYfAOHxtsgd5c/Wwtlj7B1IVoKc0xa0ab9viklmFf0w115nhicQmzoWxCaqSIuL6LJD8g3c2jUbS9CDQkHiAfOEvt67pfRnEjTcf7mbNmaUHlSIXvoeMJ/bFe/ARkwaA3SvS6nnrhnMDRCMM/m/Xexqa1MNuOWhR1xA/+S6yV8BhbVgpqtzdSTfguTkOyNJP42ow6ntBo1IyFLwkUfP7ozIuNu/Mkq+o1GyYMoXy59Gg59sMirxXjvJcjv344i9D3f1hsCNkUjL/IHcoErn/ELB5IDeNoAFhoAenCVvu6kLsR8ARh/FgACK3VyQj77HEfdD1UvQoiBkqnwSddAl8ySf6jcB+FvZGi2DuHNdHoLqgtAtHTvZLxj5ny4yRDhUo0XieksM2hlErY3RnzDuCOfJVS/XNwcfw7vSal71mVXSHaLayBMHa/IAD6v7lFvcGRDBQozu/+cAb31LGxJr2PbxqeIHtzQ6kF7Y4/S/Ho0CnMni3uj5eWcbiNtsGIeK6VaIP+x/sxSKE/bxjqHXOxtqohC0+8q+C1Y1i2I71Y7GTt4VOqenOCk3h13Zinq/T8vH77/zgH3d2/c6vs7cZ+NhhezvxruY2B2tc/ja0uWQfUh98XxSdTZVnI0oBCCsH/xpgGNg2Ej/n+mam9BnHF/WFNE8yuNlhxHzfXjz2mlNsnF4MtcyKqJgOBvZFQpkwkLqPm5F9B5ryUNI8fagnW7L8QzV9y7X9NjYmqdeqH0s13h/rBzr3vfAOcUl0VajyhZp2rP6Fq9MOAqsp/+Vx89sbgpV8CtguIKps9PKRB5dozUUtTgCPgJPzbtx4rvDqNAE/Za2ivqh4/slf+/7vwAoSPM/SRZPjPWbJHN6GCpXEy7w6ttB7FmJ2rW4o588Wuba1+uW9S04KV0pyrWxtt7B2pFFRfrL16zBreIyViDEmuTT3iOx8clBHCHKcsonT cKFn56f1 eVpsNpAzIEuAzWv6+Z/ipMtDlEiPWp95/xYUNu2Nka5uGiLwGKKT1sqCXzLKwE+Ogg40+bZV/wjtOLP6z5hZAt8kM6XZsg8ERA9iP2BGy44r4r7lwyukN3Y77uwqkgsE8DynAo6p+9S7JOvlIeJIDeXxhCd1nyzvP/0NZvxfDbX7EzPcxnlFTScV4AveeI9RxARr4JYRfeqpY28BZAqki1scvE3B7SRCUcM5bht5EtqiLRuokyjJOrEXRwINXjV8Ks7HeR+gtA0GFRsxcunoE42TmFYvhs7UtnrI90W/3waruXDZcbEOHN1/ygHyaCJ3PJ60NfjztXs+sPOUxPtGzbjSeMcfELt6zAamlzGRAx++lqMIULz9sdlAsQZ+DNQ8equg1Z6WALvoMk4wKUl18/d8AfMUb5RgQEI6NMkFrH6eFnyI= 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, Thanks for your feedback. I will fix them (inlined) in the next V11. No worries, it's never too late! On Wed, Apr 3, 2024 at 9:52=E2=80=AFAM Jonathan Cameron wrote: > > On Tue, 2 Apr 2024 00:17:37 +0000 > "Ho-Ren (Jack) Chuang" wrote: > > > Since different memory devices require finding, allocating, and putting > > memory types, these common steps are abstracted in this patch, > > enhancing the scalability and conciseness of the code. > > > > Signed-off-by: Ho-Ren (Jack) Chuang > > Reviewed-by: "Huang, Ying" > > Hi, > > I know this is a late entry to the discussion but a few comments inline. > (sorry I didn't look earlier!) > > All opportunities to improve code complexity and readability as a result > of your factoring out. > > Jonathan > > > > --- > > drivers/dax/kmem.c | 20 ++------------------ > > include/linux/memory-tiers.h | 13 +++++++++++++ > > mm/memory-tiers.c | 32 ++++++++++++++++++++++++++++++++ > > 3 files changed, 47 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c > > index 42ee360cf4e3..01399e5b53b2 100644 > > --- a/drivers/dax/kmem.c > > +++ b/drivers/dax/kmem.c > > @@ -55,21 +55,10 @@ static LIST_HEAD(kmem_memory_types); > > > > static struct memory_dev_type *kmem_find_alloc_memory_type(int adist) > > { > > - bool found =3D false; > > struct memory_dev_type *mtype; > > > > mutex_lock(&kmem_memory_type_lock); > could use > > guard(mutex)(&kmem_memory_type_lock); > return mt_find_alloc_memory_type(adist, &kmem_memory_types); > I will change it accordingly. > I'm fine if you ignore this comment though as may be other functions in > here that could take advantage of the cleanup.h stuff in a future patch. > > > - list_for_each_entry(mtype, &kmem_memory_types, list) { > > - if (mtype->adistance =3D=3D adist) { > > - found =3D true; > > - break; > > - } > > - } > > - if (!found) { > > - mtype =3D alloc_memory_type(adist); > > - if (!IS_ERR(mtype)) > > - list_add(&mtype->list, &kmem_memory_types); > > - } > > + mtype =3D mt_find_alloc_memory_type(adist, &kmem_memory_types); > > mutex_unlock(&kmem_memory_type_lock); > > > > return mtype; > > > diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.= h > > index 69e781900082..a44c03c2ba3a 100644 > > --- a/include/linux/memory-tiers.h > > +++ b/include/linux/memory-tiers.h > > @@ -48,6 +48,9 @@ int mt_calc_adistance(int node, int *adist); > > int mt_set_default_dram_perf(int nid, struct access_coordinate *perf, > > const char *source); > > int mt_perf_to_adistance(struct access_coordinate *perf, int *adist); > > +struct memory_dev_type *mt_find_alloc_memory_type(int adist, > > + struct list_head = *memory_types); > > That indent looks unusual. Align the start of struct with start of int. > I can make this aligned but it will show another warning: "WARNING: line length of 131 exceeds 100 columns" Is this ok? > > +void mt_put_memory_types(struct list_head *memory_types); > > #ifdef CONFIG_MIGRATION > > int next_demotion_node(int node); > > void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets); > > @@ -136,5 +139,15 @@ static inline int mt_perf_to_adistance(struct acce= ss_coordinate *perf, int *adis > > { > > return -EIO; > > } > > + > > +struct memory_dev_type *mt_find_alloc_memory_type(int adist, struct li= st_head *memory_types) > > +{ > > + return NULL; > > +} > > + > > +void mt_put_memory_types(struct list_head *memory_types) > > +{ > > + > No blank line needed here. Will fix. > > +} > > #endif /* CONFIG_NUMA */ > > #endif /* _LINUX_MEMORY_TIERS_H */ > > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c > > index 0537664620e5..974af10cfdd8 100644 > > --- a/mm/memory-tiers.c > > +++ b/mm/memory-tiers.c > > @@ -623,6 +623,38 @@ void clear_node_memory_type(int node, struct memor= y_dev_type *memtype) > > } > > EXPORT_SYMBOL_GPL(clear_node_memory_type); > > > > +struct memory_dev_type *mt_find_alloc_memory_type(int adist, struct li= st_head *memory_types) > > Breaking this out as a separate function provides opportunity to improve = it. > Maybe a follow up patch makes sense given it would no longer be a straigh= t > forward code move. However in my view it would be simple enough to be ob= vious > even within this patch. > I will just keep this as is for now to minimize the changes aka mistakes. > > +{ > > + bool found =3D false; > > + struct memory_dev_type *mtype; > > + > > + list_for_each_entry(mtype, memory_types, list) { > > + if (mtype->adistance =3D=3D adist) { > > + found =3D true; > > Why not return here? > return mtype; > Yes, I can return here. I will do that and take care of the ptr returning at this point. > > + break; > > + } > > + } > > + if (!found) { > > If returning above, no need for found variable - just do this uncondition= ally. > + I suggest you flip logic for simpler to follow code flow. > It's more code but I think a bit easier to read as error handling is > out of the main simple flow. > > mtype =3D alloc_memory_type(adist); > if (IS_ERR(mtype)) > return mtype; > > list_add(&mtype->list, memory_types); > > return mtype; > Good idea! I will change it accordingly. > > + mtype =3D alloc_memory_type(adist); > > + if (!IS_ERR(mtype)) > > + list_add(&mtype->list, memory_types); > > + } > > + > > + return mtype; > > +} > > +EXPORT_SYMBOL_GPL(mt_find_alloc_memory_type); > > + > > +void mt_put_memory_types(struct list_head *memory_types) > > +{ > > + struct memory_dev_type *mtype, *mtn; > > + > > + list_for_each_entry_safe(mtype, mtn, memory_types, list) { > > + list_del(&mtype->list); > > + put_memory_type(mtype); > > + } > > +} > > +EXPORT_SYMBOL_GPL(mt_put_memory_types); > > + > > static void dump_hmem_attrs(struct access_coordinate *coord, const cha= r *prefix) > > { > > pr_info( > --=20 Best regards, Ho-Ren (Jack) Chuang =E8=8E=8A=E8=B3=80=E4=BB=BB