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 8E748ECAAD2 for ; Thu, 1 Sep 2022 06:35:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 133826B0072; Thu, 1 Sep 2022 02:35:58 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0E2C38D0001; Thu, 1 Sep 2022 02:35:58 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EECB56B0074; Thu, 1 Sep 2022 02:35:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id DA8E96B0072 for ; Thu, 1 Sep 2022 02:35:57 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id AE0781C6A1D for ; Thu, 1 Sep 2022 06:35:57 +0000 (UTC) X-FDA: 79862556354.15.A053ED4 Received: from out0.migadu.com (out0.migadu.com [94.23.1.103]) by imf18.hostedemail.com (Postfix) with ESMTP id 225AD1C0045 for ; Thu, 1 Sep 2022 06:35:56 +0000 (UTC) Content-Type: text/plain; charset=utf-8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1662014154; h=from:from: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; bh=WTuSSuXzsxgm/pJnPyg0/dYgXHbNeLosZzuNLaeJoqU=; b=nYeM+RziafZZOs9dEJQdBPX1W+Mss8DguL3pJKnzb63YiBHaXuDIx7Y2x2K2lcKIiVNrY2 ks2/8LW2brGpdxKhvr76jp9llS69W4hbJdbpH+sTy5wICIGMHGa1AyXSDsMr8YYuUJyo2R d+hUoDsfN4o+1C82zppKEiB+ZnKvRGo= MIME-Version: 1.0 Subject: Re: [PATCH RESEND] mm: hugetlb: simplify per-node sysfs creation and removal X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Muchun Song In-Reply-To: <685F3238-D727-4C1F-9BA4-36651ABCD91A@linux.dev> Date: Thu, 1 Sep 2022 14:35:42 +0800 Cc: Muchun Song , Greg KH , rafael@kernel.org, Mike Kravetz , Andrew Morton , Oscar Salvador , andi@firstfloor.org, linux-kernel@vger.kernel.org, Linux MM , David Rientjes Content-Transfer-Encoding: quoted-printable Message-Id: References: <20220819080029.12241-1-songmuchun@bytedance.com> <60933ffc-b850-976c-78a0-0ee6e0ea9ef0@redhat.com> <685F3238-D727-4C1F-9BA4-36651ABCD91A@linux.dev> To: David Hildenbrand X-Migadu-Flow: FLOW_OUT X-Migadu-Auth-User: linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1662014157; 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=WTuSSuXzsxgm/pJnPyg0/dYgXHbNeLosZzuNLaeJoqU=; b=7WGNnf/4bpMJ5JjaJO862MQrrjMeiQ4e1I9sWDEQYWykmGY8bFOs7RaV+oaF+Ae+5UxJIy 3qLyygtykRqZXp5hpJy+UVv8RMELDD2hKfs2fzKy2DVCRM/wjTaGXcI288G3T0D3rzADHj w95gSoBGfblI32pM+TsOfq+FeMm21sU= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=nYeM+Rzi; spf=pass (imf18.hostedemail.com: domain of muchun.song@linux.dev designates 94.23.1.103 as permitted sender) smtp.mailfrom=muchun.song@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1662014157; a=rsa-sha256; cv=none; b=FdZkTciWtT7fcVxRBv2Ld5Om8S4orFx9j0hiBoH41KF26/NfMTgOTh9PTcIHDFfpdw4NZs v8I/CyFoWRBWH3bDAfowJQkXDvCkzDyl2n3jZwMJ/jnR1Lx+uU7M8Gyap9DIFTeSMONxg5 GHPRVAByCc/woVvkyXzv+Wv1y6wSSrs= X-Rspamd-Queue-Id: 225AD1C0045 Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=nYeM+Rzi; spf=pass (imf18.hostedemail.com: domain of muchun.song@linux.dev designates 94.23.1.103 as permitted sender) smtp.mailfrom=muchun.song@linux.dev; dmarc=pass (policy=none) header.from=linux.dev X-Rspam-User: X-Rspamd-Server: rspam10 X-Stat-Signature: jcabiikog1yj36hdnair467p9tbgntrn X-HE-Tag: 1662014156-886890 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: > On Aug 24, 2022, at 11:23, Muchun Song wrote: >=20 >=20 >=20 >> On Aug 23, 2022, at 18:21, David Hildenbrand = wrote: >>=20 >> On 19.08.22 10:00, Muchun Song wrote: >>> The following commit offload per-node sysfs creation and removal to = a kworker and >>> did not say why it is needed. And it also said "I don't know that = this is >>> absolutely required". It seems like the author was not sure as = well. Since it >>> only complicates the code, this patch will revert the changes to = simplify the code. >>>=20 >>> 39da08cb074c ("hugetlb: offload per node attribute registrations") >>>=20 >>> We could use memory hotplug notifier to do per-node sysfs creation = and removal >>> instead of inserting those operations to node registration and = unregistration. >>> Then, it can reduce the code coupling between node.c and hugetlb.c. = Also, it can >>> simplify the code. >>>=20 >>> Signed-off-by: Muchun Song >>=20 >>=20 >> [...] >>=20 >>> @@ -683,7 +626,6 @@ static int register_node(struct node *node, int = num) >>> void unregister_node(struct node *node) >>> { >>> compaction_unregister_node(node); >>> - hugetlb_unregister_node(node); /* no-op, if memoryless = node */ >>> node_remove_accesses(node); >>> node_remove_caches(node); >>> device_unregister(&node->dev); >>> @@ -905,74 +847,8 @@ void register_memory_blocks_under_node(int nid, = unsigned long start_pfn, >>> (void *)&nid, func); >>> return; >>> } >>=20 >> [...] >>=20 >>> /* >>> * Create all node devices, which will properly link the node >>> * to applicable memory block devices and already created cpu = devices. >>> diff --git a/include/linux/node.h b/include/linux/node.h >>> index 40d641a8bfb0..ea817b507f54 100644 >>> --- a/include/linux/node.h >>> +++ b/include/linux/node.h >>> @@ -2,15 +2,15 @@ >>> /* >>> * include/linux/node.h - generic node definition >>> * >>> - * This is mainly for topological representation. We define the=20 >>> - * basic 'struct node' here, which can be embedded in per-arch=20 >>> + * This is mainly for topological representation. We define the >>> + * basic 'struct node' here, which can be embedded in per-arch >>> * definitions of processors. >>> * >>> * Basic handling of the devices is done in drivers/base/node.c >>> - * and system devices are handled in drivers/base/sys.c.=20 >>> + * and system devices are handled in drivers/base/sys.c. >>> * >>> * Nodes are exported via driverfs in the class/node/devices/ >>> - * directory.=20 >>> + * directory. >>=20 >> Unrelated changes. >=20 > Yep, a minor cleanup BTW. >=20 >>=20 >>> */ >>> #ifndef _LINUX_NODE_H_ >>> #define _LINUX_NODE_H_ >>> @@ -18,7 +18,6 @@ >>> #include >>> #include >>> #include >>> -#include >>>=20 >>> /** >>> * struct node_hmem_attrs - heterogeneous memory performance = attributes >>> @@ -84,10 +83,6 @@ static inline void node_set_perf_attrs(unsigned = int nid, >>> struct node { >>> struct device dev; >>> struct list_head access_list; >>> - >>> -#if defined(CONFIG_MEMORY_HOTPLUG) && defined(CONFIG_HUGETLBFS) >>> - struct work_struct node_work; >>> -#endif >>> #ifdef CONFIG_HMEM_REPORTING >>> struct list_head cache_attrs; >>> struct device *cache_dev; >>> @@ -96,7 +91,6 @@ struct node { >>>=20 >>> struct memory_block; >>> extern struct node *node_devices[]; >>> -typedef void (*node_registration_func_t)(struct node *); >>>=20 >>> #if defined(CONFIG_MEMORY_HOTPLUG) && defined(CONFIG_NUMA) >>> void register_memory_blocks_under_node(int nid, unsigned long = start_pfn, >>> @@ -144,11 +138,6 @@ extern void = unregister_memory_block_under_nodes(struct memory_block *mem_blk); >>> extern int register_memory_node_under_compute_node(unsigned int = mem_nid, >>> unsigned int cpu_nid, >>> unsigned access); >>> - >>> -#ifdef CONFIG_HUGETLBFS >>> -extern void register_hugetlbfs_with_node(node_registration_func_t = doregister, >>> - node_registration_func_t = unregister); >>> -#endif >>> #else >>> static inline void node_dev_init(void) >>> { >>> @@ -176,11 +165,6 @@ static inline int = unregister_cpu_under_node(unsigned int cpu, unsigned int nid) >>> static inline void unregister_memory_block_under_nodes(struct = memory_block *mem_blk) >>> { >>> } >>> - >>> -static inline void = register_hugetlbfs_with_node(node_registration_func_t reg, >>> - node_registration_func_t = unreg) >>> -{ >>> -} >>> #endif >>>=20 >>> #define to_node(device) container_of(device, struct node, dev) >>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>> index 536a52c29035..9a72499486c1 100644 >>> --- a/mm/hugetlb.c >>> +++ b/mm/hugetlb.c >>> @@ -33,6 +33,7 @@ >>> #include >>> #include >>> #include >>> +#include >>>=20 >>> #include >>> #include >>> @@ -3967,19 +3968,19 @@ static void hugetlb_unregister_node(struct = node *node) >>> * Register hstate attributes for a single node device. >>> * No-op if attributes already registered. >>> */ >>> -static void hugetlb_register_node(struct node *node) >>> +static int hugetlb_register_node(struct node *node) >>> { >>> struct hstate *h; >>> struct node_hstate *nhs =3D &node_hstates[node->dev.id]; >>> int err; >>>=20 >>> if (nhs->hugepages_kobj) >>> - return; /* already allocated */ >>> + return 0; /* already allocated */ >>>=20 >>> nhs->hugepages_kobj =3D kobject_create_and_add("hugepages", >>> = &node->dev.kobj); >>> if (!nhs->hugepages_kobj) >>> - return; >>> + return -ENOMEM; >>>=20 >>> for_each_hstate(h) { >>> err =3D hugetlb_sysfs_add_hstate(h, nhs->hugepages_kobj, >>> @@ -3989,9 +3990,28 @@ static void hugetlb_register_node(struct node = *node) >>> pr_err("HugeTLB: Unable to add hstate %s for = node %d\n", >>> h->name, node->dev.id); >>> hugetlb_unregister_node(node); >>> - break; >>> + return -ENOMEM; >>> } >>> } >>> + return 0; >>> +} >>> + >>> +static int __meminit hugetlb_memory_callback(struct notifier_block = *self, >>> + unsigned long action, void = *arg) >>> +{ >>> + int ret =3D 0; >>> + struct memory_notify *mnb =3D arg; >>> + int nid =3D mnb->status_change_nid; >>> + >>> + if (nid =3D=3D NUMA_NO_NODE) >>> + return NOTIFY_DONE; >>> + >>> + if (action =3D=3D MEM_GOING_ONLINE) >>> + ret =3D hugetlb_register_node(node_devices[nid]); >>> + else if (action =3D=3D MEM_CANCEL_ONLINE || action =3D=3D = MEM_OFFLINE) >>> + hugetlb_unregister_node(node_devices[nid]); >>> + >>> + return notifier_from_errno(ret); >>> } >>>=20 >>> /* >>> @@ -4003,18 +4023,11 @@ static void __init = hugetlb_register_all_nodes(void) >>> { >>> int nid; >>>=20 >>> - for_each_node_state(nid, N_MEMORY) { >>> - struct node *node =3D node_devices[nid]; >>> - if (node->dev.id =3D=3D nid) >>> - hugetlb_register_node(node); >>> - } >>> - >>> - /* >>> - * Let the node device driver know we're here so it can >>> - * [un]register hstate attributes on node hotplug. >>> - */ >>> - register_hugetlbfs_with_node(hugetlb_register_node, >>> - hugetlb_unregister_node); >>> + get_online_mems(); >>> + hotplug_memory_notifier(hugetlb_memory_callback, 0); >>> + for_each_node_state(nid, N_MEMORY) >>> + hugetlb_register_node(node_devices[nid]); >>> + put_online_mems(); >>> } >>> #else /* !CONFIG_NUMA */ >>=20 >> Do we really *need* the memory hotplug notifier and the added = complexity >> due for handling memory-less nodes? Hi David, After some tries, I think it may not reduce the complexity. = node_dev_init() is called at early stage before hugetlb_register_all_nodes(). So we need = to add a mechanism to detect if the hugetlb subsystem finishes = initialization in node_dev_init() so that it can determine to help hugetlb create = /sysfs files, the mechanism is similar with the changes in drivers/base/node.c = of commit 9a30523066cd ("hugetlb: add per node hstate attributes=E2=80=9D). = This approach may add more code than the memory-notify-based approach like this patch implemented. And it also add the code coupling between node.c and = hugetlb.c. So I tend to use memory hotplug notifier. What=E2=80=99s your opinion? Thanks, Muchun >=20 > I have found the commit introduced this mechanism, see commit: >=20 > 4faf8d950ec4 ("hugetlb: handle memory hot-plug events") >=20 > =46rom the commit message, I think it is a suggestion from David = Rientjes. > I didn=E2=80=99t see any reasons why we need it. So Cc David Rientjes = (Maybe > he knew more context). The committer Lee and the reviewer Andi=E2=80=99s= email > is invalid (don=E2=80=99t Cc them) >=20 >>=20 >> Why can't we simply register/unregister sysfs entries in >> register_node/unregister_node and call it a day? >>=20 >=20 > At least, I agree with you. Before I change to this way, let=E2=80=99s = wait for > some potential comments from David Rientjes. >=20 >=20 > Thanks. >=20 >> TBH, we should just have sysfs entries for memory-less nodes and not >> care about such (corner) cases. >>=20 >>=20 >> --=20 >> Thanks, >>=20 >> David / dhildenb