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 332EACD37B0 for ; Mon, 18 Sep 2023 09:03:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AFE1E6B02BC; Mon, 18 Sep 2023 05:03:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AAEF36B02BE; Mon, 18 Sep 2023 05:03:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 976F86B02BF; Mon, 18 Sep 2023 05:03:45 -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 83ED96B02BC for ; Mon, 18 Sep 2023 05:03:45 -0400 (EDT) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 4C485140C4E for ; Mon, 18 Sep 2023 09:03:45 +0000 (UTC) X-FDA: 81249130410.24.A088EFF Received: from out-210.mta1.migadu.com (out-210.mta1.migadu.com [95.215.58.210]) by imf22.hostedemail.com (Postfix) with ESMTP id 39E11C000F for ; Mon, 18 Sep 2023 09:03:42 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=qVx4MQld; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf22.hostedemail.com: domain of muchun.song@linux.dev designates 95.215.58.210 as permitted sender) smtp.mailfrom=muchun.song@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695027823; a=rsa-sha256; cv=none; b=1YJ/+CWxFpqv1E3N3yWC0XI/HQxi1g+a255gTIS+MeEDD/sE+wFFKFlOS6c3Kl7XsY8sHQ TibDl8NQdKIJOAeZk0s62wzaMUjxw36dBHAbd7kEhjYi1aq3rZLc7nneLyWvcIEBFb0tzB NLMLOeoEZkFBGaHjNei37nfQBj1gp8A= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=qVx4MQld; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf22.hostedemail.com: domain of muchun.song@linux.dev designates 95.215.58.210 as permitted sender) smtp.mailfrom=muchun.song@linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1695027823; 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=H8FIKu/oc8C0T9NCBKrRDhMs8EoaTRCvaUvMqpcHrIE=; b=LtyWrAt9Ood5s11/RXPOA4nhjqWVvlT4RAgUYjUGRWngZKFqBgp7ZJpAGQBJ7/3zuxdDfm poST6JjHxtOfG8YNHOq8RttV+bku+BZYm606Rwl9QPYV0gZ60o2o99pINbdXx2NgigN9QR 3Fbn6O84husLWk4P9vGRVVeMLRmXmaI= Message-ID: <4aff0e17-b40f-406d-65fd-72a2bacfcc1a@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1695027820; 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=H8FIKu/oc8C0T9NCBKrRDhMs8EoaTRCvaUvMqpcHrIE=; b=qVx4MQldgaytiWlxDgNucPdP9OuA/qjxMAdjnc13310KRUWLZNWoHrexZyvR+hvPyYBHxg IKrlmpeS12OTQ0YOUZZ3pwo87Q0nou3HMhgw2kdwxdzSHTdsaeTrJXgw0dduFIPhicYa6o ihWs8+51lRQGImekl3PVipwfJQtmJQQ= Date: Mon, 18 Sep 2023 17:03:31 +0800 MIME-Version: 1.0 Subject: Re: [PATCH v6 01/45] mm: shrinker: add infrastructure for dynamically allocating shrinker To: Qi Zheng Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, akpm@linux-foundation.org, david@fromorbit.com, tkhai@ya.ru, vbabka@suse.cz, roman.gushchin@linux.dev, djwong@kernel.org, brauner@kernel.org, paulmck@kernel.org, tytso@mit.edu, steven.price@arm.com, cel@kernel.org, senozhatsky@chromium.org, yujie.liu@intel.com, gregkh@linuxfoundation.org References: <20230911094444.68966-1-zhengqi.arch@bytedance.com> <20230911094444.68966-2-zhengqi.arch@bytedance.com> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Muchun Song In-Reply-To: <20230911094444.68966-2-zhengqi.arch@bytedance.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 39E11C000F X-Stat-Signature: dy4c5may5w7rog7zuh3hgrusj6i51icp X-HE-Tag: 1695027822-583588 X-HE-Meta: U2FsdGVkX1/q2phqxC/NrZHPRxz0a8kpcfBYEROXY9idt7LRdVS84uFifoYyiu7YwY7wDALgmhggJYed9B1GZK5CewGQUty05PMCNBHOTFS5lXX76mvSrVJTyDLpwZiAe/is6JN9F8cbSPr/jcJWklrfgelnmERrzqAKF3vj7qPpzPNHW+lzX3coUI+bkP51z59NzzImU6ufUvDXc8YDzOHSNTFAMuiI3H8VIIHkQg/2vq27yKj1bOKRnmjY5XsT5Z9NzhR8gQsqgAxgLn+8OkORPdtHEb2KUiXDPLDEoZqdnoU1/R7xC0FzaK7HtzX3Bb/LisW14gvgMXydbZ+s3KeG+ICo04Nbg5UkuiYVOLeh2DpF8Sp+1QesJcKoNbY90/zHDquxX+UUEkfJI1yzV12DyssYqk6JoKun0uVE9n8rcKyzdkRcOMj30mnIprJrycqXbk4Fe78NzjA/CMiqJQq9e07MOR5k1rblA4qzPPAe1v8pRHxX5GwMV+cqMssURHx11z4aTBTO0qwXxQv7ki+xM+2HZ8bUGEcVEhP0v6SO3LDq/DV91BOxY0d0Xzl6AMa6CwMU+DjomRGx5wVc8IWixaSyxcFuFz+ym6dVEURH8crn9S4c95W7znFCCGLcPeuH2P8+pKNVdwrPPqdVbihhCblPLDuVBQKZS4LMk+kGg+CSdyzyGCt/PA+vFpAr3KCZKevSu2lu2Or/Dwkcbi3vV5X6PCIuyoraNSvRjIRsGgAxal0FZU4SG/mnOrPJk7uDPNaIpS65hLlVeqcCN7/Wlcni9Ai7BF/PDuIcv5VwToRalsUri3UpxmK0KnnhPM2siDYewvRAD6LujUgptHTdhdBlV1BP+UABXuABfrHUuVxfFdvoeZretLCxongE/uSHeYdEjbJjymvj47+ORh0LuqchfCGT3HZpp54pWujVnaN9/H0u5DrueRJ/ZhYIA4eaqWd+XHSllOU23DR 9AgBoktz 4zgH3Ut6yjQpsQGMN4Uqv19Q9kfKefA/i5+GznXdbpWCS1OMeeWq3/ZMsacRQ+7SjTCiZNoT/kp+Jmw/haVU+JhjXY6bNpMJj1nI9vBeT68i9eYn948agbTIOI8WiqErqffLf78Icmry5R/Lzt6Le7uYw6w== 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 2023/9/11 17:44, Qi Zheng wrote: > Currently, the shrinker instances can be divided into the following three > types: > > a) global shrinker instance statically defined in the kernel, such as > workingset_shadow_shrinker. > > b) global shrinker instance statically defined in the kernel modules, such > as mmu_shrinker in x86. > > c) shrinker instance embedded in other structures. > > For case a, the memory of shrinker instance is never freed. For case b, > the memory of shrinker instance will be freed after synchronize_rcu() when > the module is unloaded. For case c, the memory of shrinker instance will > be freed along with the structure it is embedded in. > > In preparation for implementing lockless slab shrink, we need to > dynamically allocate those shrinker instances in case c, then the memory > can be dynamically freed alone by calling kfree_rcu(). > > So this commit adds the following new APIs for dynamically allocating > shrinker, and add a private_data field to struct shrinker to record and > get the original embedded structure. > > 1. shrinker_alloc() > > Used to allocate shrinker instance itself and related memory, it will > return a pointer to the shrinker instance on success and NULL on failure. > > 2. shrinker_register() > > Used to register the shrinker instance, which is same as the current > register_shrinker_prepared(). > > 3. shrinker_free() > > Used to unregister (if needed) and free the shrinker instance. > > In order to simplify shrinker-related APIs and make shrinker more > independent of other kernel mechanisms, subsequent submissions will use > the above API to convert all shrinkers (including case a and b) to > dynamically allocated, and then remove all existing APIs. > > This will also have another advantage mentioned by Dave Chinner: > > ``` > The other advantage of this is that it will break all the existing > out of tree code and third party modules using the old API and will > no longer work with a kernel using lockless slab shrinkers. They > need to break (both at the source and binary levels) to stop bad > things from happening due to using unconverted shrinkers in the new > setup. > ``` > > Signed-off-by: Qi Zheng > --- > include/linux/shrinker.h | 7 +++ > mm/internal.h | 11 +++++ > mm/shrinker.c | 102 +++++++++++++++++++++++++++++++++++++++ > mm/shrinker_debug.c | 17 ++++++- > 4 files changed, 135 insertions(+), 2 deletions(-) > > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h > index 6b5843c3b827..3f3fd9974ce5 100644 > --- a/include/linux/shrinker.h > +++ b/include/linux/shrinker.h > @@ -70,6 +70,8 @@ struct shrinker { > int seeks; /* seeks to recreate an obj */ > unsigned flags; > > + void *private_data; > + > /* These are for internal use */ > struct list_head list; > #ifdef CONFIG_MEMCG > @@ -95,6 +97,11 @@ struct shrinker { > * non-MEMCG_AWARE shrinker should not have this flag set. > */ > #define SHRINKER_NONSLAB (1 << 3) > +#define SHRINKER_ALLOCATED (1 << 4) It is better to add a comment here to tell users it is only used by internals of shrinker. The users are not supposed to pass this flag to shrink APIs. > + > +struct shrinker *shrinker_alloc(unsigned int flags, const char *fmt, ...); > +void shrinker_register(struct shrinker *shrinker); > +void shrinker_free(struct shrinker *shrinker); > > extern int __printf(2, 3) prealloc_shrinker(struct shrinker *shrinker, > const char *fmt, ...); > diff --git a/mm/internal.h b/mm/internal.h > index 0471d6326d01..5587cae20ebf 100644 > --- a/mm/internal.h > +++ b/mm/internal.h > @@ -1161,6 +1161,9 @@ unsigned long shrink_slab(gfp_t gfp_mask, int nid, struct mem_cgroup *memcg, > > #ifdef CONFIG_SHRINKER_DEBUG > extern int shrinker_debugfs_add(struct shrinker *shrinker); > +extern int shrinker_debugfs_name_alloc(struct shrinker *shrinker, > + const char *fmt, va_list ap); > +extern void shrinker_debugfs_name_free(struct shrinker *shrinker); > extern struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker, > int *debugfs_id); > extern void shrinker_debugfs_remove(struct dentry *debugfs_entry, > @@ -1170,6 +1173,14 @@ static inline int shrinker_debugfs_add(struct shrinker *shrinker) > { > return 0; > } > +static inline int shrinker_debugfs_name_alloc(struct shrinker *shrinker, > + const char *fmt, va_list ap) > +{ > + return 0; > +} > +static inline void shrinker_debugfs_name_free(struct shrinker *shrinker) > +{ > +} > static inline struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker, > int *debugfs_id) > { > diff --git a/mm/shrinker.c b/mm/shrinker.c > index a16cd448b924..201211a67827 100644 > --- a/mm/shrinker.c > +++ b/mm/shrinker.c > @@ -550,6 +550,108 @@ unsigned long shrink_slab(gfp_t gfp_mask, int nid, struct mem_cgroup *memcg, > return freed; > } > > +struct shrinker *shrinker_alloc(unsigned int flags, const char *fmt, ...) > +{ > + struct shrinker *shrinker; > + unsigned int size; > + va_list ap; > + int err; > + > + shrinker = kzalloc(sizeof(struct shrinker), GFP_KERNEL); > + if (!shrinker) > + return NULL; > + > + va_start(ap, fmt); > + err = shrinker_debugfs_name_alloc(shrinker, fmt, ap); > + va_end(ap); > + if (err) > + goto err_name; > + > + shrinker->flags = flags | SHRINKER_ALLOCATED; > + shrinker->seeks = DEFAULT_SEEKS; > + > + if (flags & SHRINKER_MEMCG_AWARE) { > + err = prealloc_memcg_shrinker(shrinker); > + if (err == -ENOSYS) > + shrinker->flags &= ~SHRINKER_MEMCG_AWARE; > + else if (err == 0) > + goto done; > + else > + goto err_flags; Actually, the code here is a little confusing me when I fist look at it. I think there could be some improvements here. Something like:         if (flags & SHRINKER_MEMCG_AWARE) {                 err = prealloc_memcg_shrinker(shrinker);                 if (err == -ENOSYS) {                         /* Memcg is not supported and fallback to non-memcg-aware shrinker. */                         shrinker->flags &= ~SHRINKER_MEMCG_AWARE;                         goto non-memcg;                 }                 if (err)                     goto err_flags;                 return shrinker;         } non-memcg:         [...]         return shrinker; In this case, the code becomes more clear (at least for me). We have split the code into two part, one is handling memcg-aware case, another is non-memcg-aware case. Any side will have a explicit "return" keyword to return once succeeds. It is a little implicit that the previous one uses "goto done". And the tag of "non-memcg" is also a good annotation to tell us the following code handles non-memcg-aware case. > + } > + > + /* > + * The nr_deferred is available on per memcg level for memcg aware > + * shrinkers, so only allocate nr_deferred in the following cases: > + * - non memcg aware shrinkers > + * - !CONFIG_MEMCG > + * - memcg is disabled by kernel command line > + */ > + size = sizeof(*shrinker->nr_deferred); > + if (flags & SHRINKER_NUMA_AWARE) > + size *= nr_node_ids; > + > + shrinker->nr_deferred = kzalloc(size, GFP_KERNEL); > + if (!shrinker->nr_deferred) > + goto err_flags; > + > +done: > + return shrinker; > + > +err_flags: > + shrinker_debugfs_name_free(shrinker); > +err_name: > + kfree(shrinker); > + return NULL; > +} > +EXPORT_SYMBOL_GPL(shrinker_alloc); > + > +void shrinker_register(struct shrinker *shrinker) > +{ > + if (unlikely(!(shrinker->flags & SHRINKER_ALLOCATED))) { > + pr_warn("Must use shrinker_alloc() to dynamically allocate the shrinker"); > + return; > + } > + > + down_write(&shrinker_rwsem); > + list_add_tail(&shrinker->list, &shrinker_list); > + shrinker->flags |= SHRINKER_REGISTERED; > + shrinker_debugfs_add(shrinker); > + up_write(&shrinker_rwsem); > +} > +EXPORT_SYMBOL_GPL(shrinker_register); > + > +void shrinker_free(struct shrinker *shrinker) > +{ > + struct dentry *debugfs_entry = NULL; > + int debugfs_id; > + > + if (!shrinker) > + return; > + > + down_write(&shrinker_rwsem); > + if (shrinker->flags & SHRINKER_REGISTERED) { > + list_del(&shrinker->list); > + debugfs_entry = shrinker_debugfs_detach(shrinker, &debugfs_id); > + shrinker->flags &= ~SHRINKER_REGISTERED; > + } else { > + shrinker_debugfs_name_free(shrinker); We could remove shrinker_debugfs_name_free() calling from shrinker_debugfs_detach(), then we could call shrinker_debugfs_name_free() anyway, otherwise, it it a little weird for me. And the srinker name is allocated from shrinker_alloc(), so I think it it reasonable for shrinker_free() to free the shrinker name. Thanks. > + } > + > + if (shrinker->flags & SHRINKER_MEMCG_AWARE) > + unregister_memcg_shrinker(shrinker); > + up_write(&shrinker_rwsem); > + > + if (debugfs_entry) > + shrinker_debugfs_remove(debugfs_entry, debugfs_id); > + > + kfree(shrinker->nr_deferred); > + shrinker->nr_deferred = NULL; > + > + kfree(shrinker); > +} > +EXPORT_SYMBOL_GPL(shrinker_free); > + > /* > * Add a shrinker callback to be called from the vm. > */ > diff --git a/mm/shrinker_debug.c b/mm/shrinker_debug.c > index e4ce509f619e..38452f539f40 100644 > --- a/mm/shrinker_debug.c > +++ b/mm/shrinker_debug.c > @@ -193,6 +193,20 @@ int shrinker_debugfs_add(struct shrinker *shrinker) > return 0; > } > > +int shrinker_debugfs_name_alloc(struct shrinker *shrinker, const char *fmt, > + va_list ap) > +{ > + shrinker->name = kvasprintf_const(GFP_KERNEL, fmt, ap); > + > + return shrinker->name ? 0 : -ENOMEM; > +} > + > +void shrinker_debugfs_name_free(struct shrinker *shrinker) > +{ > + kfree_const(shrinker->name); > + shrinker->name = NULL; > +} It it better to move both helpers to internal.h and mark them as inline since both are very simple enough. Thanks. > + > int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...) > { > struct dentry *entry; > @@ -241,8 +255,7 @@ struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker, > > lockdep_assert_held(&shrinker_rwsem); > > - kfree_const(shrinker->name); > - shrinker->name = NULL; > + shrinker_debugfs_name_free(shrinker); > > *debugfs_id = entry ? shrinker->debugfs_id : -1; > shrinker->debugfs_entry = NULL;