| Message ID | 20230724094354.90817-1-zhengqi.arch@bytedance.com |
|---|---|
| Headers | show |
| Series | use refcount+RCU method to implement lockless slab shrink | expand |
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > Use new APIs to dynamically allocate the xen-backend shrinker. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com> Thanks.
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > Use new APIs to dynamically allocate the gfs2-glock shrinker. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com> Thanks.
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > Use new APIs to dynamically allocate the nfs-xattr shrinkers. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > Use new APIs to dynamically allocate the nfsd-filecache shrinker. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > Use new APIs to dynamically allocate the dquota-cache shrinker. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
On 2023/7/24 17:43, Qi Zheng wrote: > Use new APIs to dynamically allocate the thp-zero and thp-deferred_split > shrinkers. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > --- > mm/huge_memory.c | 69 +++++++++++++++++++++++++++++++----------------- > 1 file changed, 45 insertions(+), 24 deletions(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 8c94b34024a2..4db5a1834d81 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -65,7 +65,11 @@ unsigned long transparent_hugepage_flags __read_mostly = > (1<<TRANSPARENT_HUGEPAGE_DEFRAG_KHUGEPAGED_FLAG)| > (1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG); > > -static struct shrinker deferred_split_shrinker; > +static struct shrinker *deferred_split_shrinker; > +static unsigned long deferred_split_count(struct shrinker *shrink, > + struct shrink_control *sc); > +static unsigned long deferred_split_scan(struct shrinker *shrink, > + struct shrink_control *sc); > > static atomic_t huge_zero_refcount; > struct page *huge_zero_page __read_mostly; > @@ -229,11 +233,7 @@ static unsigned long shrink_huge_zero_page_scan(struct shrinker *shrink, > return 0; > } > > -static struct shrinker huge_zero_page_shrinker = { > - .count_objects = shrink_huge_zero_page_count, > - .scan_objects = shrink_huge_zero_page_scan, > - .seeks = DEFAULT_SEEKS, > -}; > +static struct shrinker *huge_zero_page_shrinker; Same as patch #17. > > #ifdef CONFIG_SYSFS > static ssize_t enabled_show(struct kobject *kobj, > @@ -454,6 +454,40 @@ static inline void hugepage_exit_sysfs(struct kobject *hugepage_kobj) > } > #endif /* CONFIG_SYSFS */ > > +static int thp_shrinker_init(void) Better to declare it as __init. > +{ > + huge_zero_page_shrinker = shrinker_alloc(0, "thp-zero"); > + if (!huge_zero_page_shrinker) > + return -ENOMEM; > + > + deferred_split_shrinker = shrinker_alloc(SHRINKER_NUMA_AWARE | > + SHRINKER_MEMCG_AWARE | > + SHRINKER_NONSLAB, > + "thp-deferred_split"); > + if (!deferred_split_shrinker) { > + shrinker_free_non_registered(huge_zero_page_shrinker); > + return -ENOMEM; > + } > + > + huge_zero_page_shrinker->count_objects = shrink_huge_zero_page_count; > + huge_zero_page_shrinker->scan_objects = shrink_huge_zero_page_scan; > + huge_zero_page_shrinker->seeks = DEFAULT_SEEKS; > + shrinker_register(huge_zero_page_shrinker); > + > + deferred_split_shrinker->count_objects = deferred_split_count; > + deferred_split_shrinker->scan_objects = deferred_split_scan; > + deferred_split_shrinker->seeks = DEFAULT_SEEKS; > + shrinker_register(deferred_split_shrinker); > + > + return 0; > +} > + > +static void thp_shrinker_exit(void) Same as here. > +{ > + shrinker_unregister(huge_zero_page_shrinker); > + shrinker_unregister(deferred_split_shrinker); > +} > + > static int __init hugepage_init(void) > { > int err; > @@ -482,12 +516,9 @@ static int __init hugepage_init(void) > if (err) > goto err_slab; > > - err = register_shrinker(&huge_zero_page_shrinker, "thp-zero"); > - if (err) > - goto err_hzp_shrinker; > - err = register_shrinker(&deferred_split_shrinker, "thp-deferred_split"); > + err = thp_shrinker_init(); > if (err) > - goto err_split_shrinker; > + goto err_shrinker; > > /* > * By default disable transparent hugepages on smaller systems, > @@ -505,10 +536,8 @@ static int __init hugepage_init(void) > > return 0; > err_khugepaged: > - unregister_shrinker(&deferred_split_shrinker); > -err_split_shrinker: > - unregister_shrinker(&huge_zero_page_shrinker); > -err_hzp_shrinker: > + thp_shrinker_exit(); > +err_shrinker: > khugepaged_destroy(); > err_slab: > hugepage_exit_sysfs(hugepage_kobj); > @@ -2851,7 +2880,7 @@ void deferred_split_folio(struct folio *folio) > #ifdef CONFIG_MEMCG > if (memcg) > set_shrinker_bit(memcg, folio_nid(folio), > - deferred_split_shrinker.id); > + deferred_split_shrinker->id); > #endif > } > spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags); > @@ -2925,14 +2954,6 @@ static unsigned long deferred_split_scan(struct shrinker *shrink, > return split; > } > > -static struct shrinker deferred_split_shrinker = { > - .count_objects = deferred_split_count, > - .scan_objects = deferred_split_scan, > - .seeks = DEFAULT_SEEKS, > - .flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE | > - SHRINKER_NONSLAB, > -}; > - > #ifdef CONFIG_DEBUG_FS > static void split_huge_pages_all(void) > {
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > Use new APIs to dynamically allocate the sunrpc_cred shrinker. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > Use new APIs to dynamically allocate the mm-shadow shrinker. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > --- > mm/workingset.c | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/mm/workingset.c b/mm/workingset.c > index 4686ae363000..4bc85f739b13 100644 > --- a/mm/workingset.c > +++ b/mm/workingset.c > @@ -762,12 +762,7 @@ static unsigned long scan_shadow_nodes(struct shrinker *shrinker, > NULL); > } > > -static struct shrinker workingset_shadow_shrinker = { > - .count_objects = count_shadow_nodes, > - .scan_objects = scan_shadow_nodes, > - .seeks = 0, /* ->count reports only fully expendable nodes */ > - .flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE, > -}; > +static struct shrinker *workingset_shadow_shrinker; Same as patch #17.
On 2023/7/24 17:43, Qi Zheng wrote: > In preparation for implementing lockless slab shrink, use new APIs to > dynamically allocate the drm-msm_gem shrinker, so that it can be freed > asynchronously using kfree_rcu(). Then it doesn't need to wait for RCU > read-side critical section when releasing the struct msm_drm_private. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com> A nit bellow. > --- > drivers/gpu/drm/msm/msm_drv.c | 4 ++- > drivers/gpu/drm/msm/msm_drv.h | 4 +-- > drivers/gpu/drm/msm/msm_gem_shrinker.c | 36 ++++++++++++++++---------- > 3 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 891eff8433a9..7f6933be703f 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -461,7 +461,9 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv) > if (ret) > goto err_msm_uninit; > > - msm_gem_shrinker_init(ddev); > + ret = msm_gem_shrinker_init(ddev); > + if (ret) > + goto err_msm_uninit; > > if (priv->kms_init) { > ret = priv->kms_init(ddev); > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index e13a8cbd61c9..84523d4a1e58 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -217,7 +217,7 @@ struct msm_drm_private { > } vram; > > struct notifier_block vmap_notifier; > - struct shrinker shrinker; > + struct shrinker *shrinker; > > struct drm_atomic_state *pm_state; > > @@ -279,7 +279,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_to_scan); > #endif > > -void msm_gem_shrinker_init(struct drm_device *dev); > +int msm_gem_shrinker_init(struct drm_device *dev); > void msm_gem_shrinker_cleanup(struct drm_device *dev); > > int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma); > diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c > index f38296ad8743..7daab1298c11 100644 > --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c > +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c > @@ -34,8 +34,7 @@ static bool can_block(struct shrink_control *sc) > static unsigned long > msm_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc) > { > - struct msm_drm_private *priv = > - container_of(shrinker, struct msm_drm_private, shrinker); > + struct msm_drm_private *priv = shrinker->private_data; > unsigned count = priv->lru.dontneed.count; > > if (can_swap()) > @@ -100,8 +99,7 @@ active_evict(struct drm_gem_object *obj) > static unsigned long > msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) > { > - struct msm_drm_private *priv = > - container_of(shrinker, struct msm_drm_private, shrinker); > + struct msm_drm_private *priv = shrinker->private_data; > struct { > struct drm_gem_lru *lru; > bool (*shrink)(struct drm_gem_object *obj); > @@ -148,10 +146,11 @@ msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_to_scan) > struct shrink_control sc = { > .nr_to_scan = nr_to_scan, > }; > - int ret; > + unsigned long ret = SHRINK_STOP; > > fs_reclaim_acquire(GFP_KERNEL); > - ret = msm_gem_shrinker_scan(&priv->shrinker, &sc); > + if (priv->shrinker) > + ret = msm_gem_shrinker_scan(priv->shrinker, &sc); > fs_reclaim_release(GFP_KERNEL); > > return ret; > @@ -210,16 +209,27 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr) > * > * This function registers and sets up the msm shrinker. > */ > -void msm_gem_shrinker_init(struct drm_device *dev) > +int msm_gem_shrinker_init(struct drm_device *dev) > { > struct msm_drm_private *priv = dev->dev_private; > - priv->shrinker.count_objects = msm_gem_shrinker_count; > - priv->shrinker.scan_objects = msm_gem_shrinker_scan; > - priv->shrinker.seeks = DEFAULT_SEEKS; > - WARN_ON(register_shrinker(&priv->shrinker, "drm-msm_gem")); > + > + priv->shrinker = shrinker_alloc(0, "drm-msm_gem"); > + if (!priv->shrinker) { Just "if (WARN_ON(!priv->shrinker))" > + WARN_ON(1); > + return -ENOMEM; > + } > + > + priv->shrinker->count_objects = msm_gem_shrinker_count; > + priv->shrinker->scan_objects = msm_gem_shrinker_scan; > + priv->shrinker->seeks = DEFAULT_SEEKS; > + priv->shrinker->private_data = priv; > + > + shrinker_register(priv->shrinker); > > priv->vmap_notifier.notifier_call = msm_gem_shrinker_vmap; > WARN_ON(register_vmap_purge_notifier(&priv->vmap_notifier)); > + > + return 0; > } > > /** > @@ -232,8 +242,8 @@ void msm_gem_shrinker_cleanup(struct drm_device *dev) > { > struct msm_drm_private *priv = dev->dev_private; > > - if (priv->shrinker.nr_deferred) { > + if (priv->shrinker) { > WARN_ON(unregister_vmap_purge_notifier(&priv->vmap_notifier)); > - unregister_shrinker(&priv->shrinker); > + shrinker_unregister(priv->shrinker); > } > }
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > In preparation for implementing lockless slab shrink, use new APIs to > dynamically allocate the md-raid5 shrinker, so that it can be freed > asynchronously using kfree_rcu(). Then it doesn't need to wait for RCU > read-side critical section when releasing the struct r5conf. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
On 2023/7/24 17:43, Qi Zheng wrote: > In preparation for implementing lockless slab shrink, use new APIs to > dynamically allocate the md-bcache shrinker, so that it can be freed > asynchronously using kfree_rcu(). Then it doesn't need to wait for RCU > read-side critical section when releasing the struct cache_set. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > --- > drivers/md/bcache/bcache.h | 2 +- > drivers/md/bcache/btree.c | 27 ++++++++++++++++----------- > drivers/md/bcache/sysfs.c | 3 ++- > 3 files changed, 19 insertions(+), 13 deletions(-) > > diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h > index 5a79bb3c272f..c622bc50f81b 100644 > --- a/drivers/md/bcache/bcache.h > +++ b/drivers/md/bcache/bcache.h > @@ -541,7 +541,7 @@ struct cache_set { > struct bio_set bio_split; > > /* For the btree cache */ > - struct shrinker shrink; > + struct shrinker *shrink; > > /* For the btree cache and anything allocation related */ > struct mutex bucket_lock; > diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c > index fd121a61f17c..c176c7fc77d9 100644 > --- a/drivers/md/bcache/btree.c > +++ b/drivers/md/bcache/btree.c > @@ -667,7 +667,7 @@ static int mca_reap(struct btree *b, unsigned int min_order, bool flush) > static unsigned long bch_mca_scan(struct shrinker *shrink, > struct shrink_control *sc) > { > - struct cache_set *c = container_of(shrink, struct cache_set, shrink); > + struct cache_set *c = shrink->private_data; > struct btree *b, *t; > unsigned long i, nr = sc->nr_to_scan; > unsigned long freed = 0; > @@ -734,7 +734,7 @@ static unsigned long bch_mca_scan(struct shrinker *shrink, > static unsigned long bch_mca_count(struct shrinker *shrink, > struct shrink_control *sc) > { > - struct cache_set *c = container_of(shrink, struct cache_set, shrink); > + struct cache_set *c = shrink->private_data; > > if (c->shrinker_disabled) > return 0; > @@ -752,8 +752,8 @@ void bch_btree_cache_free(struct cache_set *c) > > closure_init_stack(&cl); > > - if (c->shrink.list.next) > - unregister_shrinker(&c->shrink); > + if (c->shrink) > + shrinker_unregister(c->shrink); > > mutex_lock(&c->bucket_lock); > > @@ -828,14 +828,19 @@ int bch_btree_cache_alloc(struct cache_set *c) > c->verify_data = NULL; > #endif > > - c->shrink.count_objects = bch_mca_count; > - c->shrink.scan_objects = bch_mca_scan; > - c->shrink.seeks = 4; > - c->shrink.batch = c->btree_pages * 2; > + c->shrink = shrinker_alloc(0, "md-bcache:%pU", c->set_uuid); > + if (!c->shrink) { > + pr_warn("bcache: %s: could not allocate shrinker\n", __func__); > + return -ENOMEM; Seems you have cheanged the semantic of this. In the past, it is better to have a shrinker, but now it becomes a mandatory. Right? I don't know if it is acceptable. From my point of view, just do the cleanup, don't change any behaviour. > + } > + > + c->shrink->count_objects = bch_mca_count; > + c->shrink->scan_objects = bch_mca_scan; > + c->shrink->seeks = 4; > + c->shrink->batch = c->btree_pages * 2; > + c->shrink->private_data = c; > > - if (register_shrinker(&c->shrink, "md-bcache:%pU", c->set_uuid)) > - pr_warn("bcache: %s: could not register shrinker\n", > - __func__); > + shrinker_register(c->shrink); > > return 0; > } > diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c > index 0e2c1880f60b..45d8af755de6 100644 > --- a/drivers/md/bcache/sysfs.c > +++ b/drivers/md/bcache/sysfs.c > @@ -866,7 +866,8 @@ STORE(__bch_cache_set) > > sc.gfp_mask = GFP_KERNEL; > sc.nr_to_scan = strtoul_or_return(buf); > - c->shrink.scan_objects(&c->shrink, &sc); > + if (c->shrink) > + c->shrink->scan_objects(c->shrink, &sc); > } > > sysfs_strtoul_clamp(congested_read_threshold_us,
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > In preparation for implementing lockless slab shrink, use new APIs to > dynamically allocate the virtio-balloon shrinker, so that it can be freed > asynchronously using kfree_rcu(). Then it doesn't need to wait for RCU > read-side critical section when releasing the struct virtio_balloon. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > In preparation for implementing lockless slab shrink, use new APIs to > dynamically allocate the mbcache shrinker, so that it can be freed > asynchronously using kfree_rcu(). Then it doesn't need to wait for RCU > read-side critical section when releasing the struct mb_cache. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > In preparation for implementing lockless slab shrink, use new APIs to > dynamically allocate the xfs-inodegc shrinker, so that it can be freed > asynchronously using kfree_rcu(). Then it doesn't need to wait for RCU > read-side critical section when releasing the struct xfs_mount. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > In preparation for implementing lockless slab shrink, use new APIs to > dynamically allocate the mm-zspool shrinker, so that it can be freed > asynchronously using kfree_rcu(). Then it doesn't need to wait for RCU > read-side critical section when releasing the struct zs_pool. > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
On Mon, Jul 24, 2023 at 05:43:51PM +0800, Qi Zheng wrote: > The shrinker_rwsem is a global read-write lock in shrinkers subsystem, > which protects most operations such as slab shrink, registration and > unregistration of shrinkers, etc. This can easily cause problems in the > following cases. > > 1) When the memory pressure is high and there are many filesystems > mounted or unmounted at the same time, slab shrink will be affected > (down_read_trylock() failed). > > Such as the real workload mentioned by Kirill Tkhai: > > ``` > One of the real workloads from my experience is start > of an overcommitted node containing many starting > containers after node crash (or many resuming containers > after reboot for kernel update). In these cases memory > pressure is huge, and the node goes round in long reclaim. > ``` > > 2) If a shrinker is blocked (such as the case mentioned > in [1]) and a writer comes in (such as mount a fs), > then this writer will be blocked and cause all > subsequent shrinker-related operations to be blocked. > > Even if there is no competitor when shrinking slab, there may still be a > problem. The down_read_trylock() may become a perf hotspot with frequent > calls to shrink_slab(). Because of the poor multicore scalability of > atomic operations, this can lead to a significant drop in IPC > (instructions per cycle). > > We used to implement the lockless slab shrink with SRCU [2], but then > kernel test robot reported -88.8% regression in > stress-ng.ramfs.ops_per_sec test case [3], so we reverted it [4]. > > This commit uses the refcount+RCU method [5] proposed by Dave Chinner > to re-implement the lockless global slab shrink. The memcg slab shrink is > handled in the subsequent patch. > > For now, all shrinker instances are converted to dynamically allocated and > will be freed by kfree_rcu(). So we can use rcu_read_{lock,unlock}() to > ensure that the shrinker instance is valid. > > And the shrinker instance will not be run again after unregistration. So > the structure that records the pointer of shrinker instance can be safely > freed without waiting for the RCU read-side critical section. > > In this way, while we implement the lockless slab shrink, we don't need to > be blocked in unregister_shrinker(). > > The following are the test results: > > stress-ng --timeout 60 --times --verify --metrics-brief --ramfs 9 & > > 1) Before applying this patchset: > > setting to a 60 second run per stressor > dispatching hogs: 9 ramfs > stressor bogo ops real time usr time sys time bogo ops/s bogo ops/s > (secs) (secs) (secs) (real time) (usr+sys time) > ramfs 735238 60.00 12.37 363.70 12253.05 1955.08 > for a 60.01s run time: > 1440.27s available CPU time > 12.36s user time ( 0.86%) > 363.70s system time ( 25.25%) > 376.06s total time ( 26.11%) > load average: 10.79 4.47 1.69 > passed: 9: ramfs (9) > failed: 0 > skipped: 0 > successful run completed in 60.01s (1 min, 0.01 secs) > > 2) After applying this patchset: > > setting to a 60 second run per stressor > dispatching hogs: 9 ramfs > stressor bogo ops real time usr time sys time bogo ops/s bogo ops/s > (secs) (secs) (secs) (real time) (usr+sys time) > ramfs 746677 60.00 12.22 367.75 12443.70 1965.13 > for a 60.01s run time: > 1440.26s available CPU time > 12.21s user time ( 0.85%) > 367.75s system time ( 25.53%) > 379.96s total time ( 26.38%) > load average: 8.37 2.48 0.86 > passed: 9: ramfs (9) > failed: 0 > skipped: 0 > successful run completed in 60.01s (1 min, 0.01 secs) > > We can see that the ops/s has hardly changed. > > [1]. https://lore.kernel.org/lkml/20191129214541.3110-1-ptikhomirov@virtuozzo.com/ > [2]. https://lore.kernel.org/lkml/20230313112819.38938-1-zhengqi.arch@bytedance.com/ > [3]. https://lore.kernel.org/lkml/202305230837.db2c233f-yujie.liu@intel.com/ > [4]. https://lore.kernel.org/all/20230609081518.3039120-1-qi.zheng@linux.dev/ > [5]. https://lore.kernel.org/lkml/ZIJhou1d55d4H1s0@dread.disaster.area/ > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > --- > include/linux/shrinker.h | 19 +++++++--- > mm/shrinker.c | 75 ++++++++++++++++++++++++++-------------- > mm/shrinker_debug.c | 52 +++++++++++++++++++++------- > 3 files changed, 104 insertions(+), 42 deletions(-) > > diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h > index 36977a70bebb..335da93cccee 100644 > --- a/include/linux/shrinker.h > +++ b/include/linux/shrinker.h > @@ -4,6 +4,7 @@ > > #include <linux/atomic.h> > #include <linux/types.h> > +#include <linux/refcount.h> > > #define SHRINKER_UNIT_BITS BITS_PER_LONG > > @@ -86,6 +87,10 @@ struct shrinker { > long batch; /* reclaim batch size, 0 = default */ > int seeks; /* seeks to recreate an obj */ > unsigned flags; > + bool registered; > + > + refcount_t refcount; > + struct rcu_head rcu; > > void *private_data; > > @@ -106,14 +111,13 @@ struct shrinker { > #define DEFAULT_SEEKS 2 /* A good number if you don't know better. */ > > /* Flags */ > -#define SHRINKER_REGISTERED (1 << 0) > -#define SHRINKER_NUMA_AWARE (1 << 1) > -#define SHRINKER_MEMCG_AWARE (1 << 2) > +#define SHRINKER_NUMA_AWARE (1 << 0) > +#define SHRINKER_MEMCG_AWARE (1 << 1) > /* > * It just makes sense when the shrinker is also MEMCG_AWARE for now, > * non-MEMCG_AWARE shrinker should not have this flag set. > */ > -#define SHRINKER_NONSLAB (1 << 3) > +#define SHRINKER_NONSLAB (1 << 2) > > unsigned long shrink_slab(gfp_t gfp_mask, int nid, struct mem_cgroup *memcg, > int priority); > @@ -122,6 +126,13 @@ void shrinker_free_non_registered(struct shrinker *shrinker); > void shrinker_register(struct shrinker *shrinker); > void shrinker_unregister(struct shrinker *shrinker); > > +static inline bool shrinker_try_get(struct shrinker *shrinker) > +{ > + return READ_ONCE(shrinker->registered) && > + refcount_inc_not_zero(&shrinker->refcount); > +} Why do we care about shrinker->registered here? If we don't set the refcount to 1 until we have fully initialised everything, then the shrinker code can key entirely off the reference count and none of the lookup code needs to care about whether the shrinker is registered or not. > +void shrinker_put(struct shrinker *shrinker); > + > #ifdef CONFIG_SHRINKER_DEBUG > extern int shrinker_debugfs_add(struct shrinker *shrinker); > extern struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker, > diff --git a/mm/shrinker.c b/mm/shrinker.c > index 8a1fe844f1a4..8e3334749552 100644 > --- a/mm/shrinker.c > +++ b/mm/shrinker.c > @@ -2,10 +2,13 @@ > #include <linux/memcontrol.h> > #include <linux/rwsem.h> > #include <linux/shrinker.h> > +#include <linux/rculist.h> > +#include <linux/spinlock.h> > #include <trace/events/vmscan.h> > > LIST_HEAD(shrinker_list); > DECLARE_RWSEM(shrinker_rwsem); > +DEFINE_SPINLOCK(shrinker_lock); > > #ifdef CONFIG_MEMCG > static int shrinker_nr_max; > @@ -450,6 +453,18 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl, > return freed; > } > > +void shrinker_put(struct shrinker *shrinker) > +{ > + if (refcount_dec_and_test(&shrinker->refcount)) { > + spin_lock(&shrinker_lock); > + list_del_rcu(&shrinker->list); > + spin_unlock(&shrinker_lock); > + > + kfree(shrinker->nr_deferred); > + kfree_rcu(shrinker, rcu); > + } > +} Urk, no. We want the shrinker_free() code to block waiting for the shrinker reference count to go to zero, because the shrinkers can reference structures that are associated with the path that is freeing the shrinker. i.e. we do not want to free the superblock of a filesystem whilst the shrinker is still running, but the way you've done this is that the shrinker can run whilst the structure that contains it has been torn down. This should use a completion, then it is always safe under rcu_read_lock(). This also gets rid of the shrinker_lock spin lock, which only exists because we can't take a blocking lock under rcu_read_lock(). i.e: void shrinker_put(struct shrinker *shrinker) { if (refcount_dec_and_test(&shrinker->refcount)) complete(&shrinker->done); } void shrinker_free() { ..... refcount_dec(&shrinker->refcount); wait_for_completion(&shrinker->done); /* * lookups on the shrinker will now all fail as refcount has * fallen to zero. We can now remove it from the lists and * free it. */ down_write(shrinker_rwsem); list_del_rcu(&shrinker->list); up_write(&shrinker_rwsem); call_rcu(shrinker->rcu_head, shrinker_free_rcu_cb); } .... > @@ -686,11 +711,14 @@ EXPORT_SYMBOL(shrinker_free_non_registered); > > void shrinker_register(struct shrinker *shrinker) > { > - down_write(&shrinker_rwsem); > - list_add_tail(&shrinker->list, &shrinker_list); > - shrinker->flags |= SHRINKER_REGISTERED; > + refcount_set(&shrinker->refcount, 1); > + > + spin_lock(&shrinker_lock); > + list_add_tail_rcu(&shrinker->list, &shrinker_list); > + spin_unlock(&shrinker_lock); > + > shrinker_debugfs_add(shrinker); > - up_write(&shrinker_rwsem); > + WRITE_ONCE(shrinker->registered, true); > } > EXPORT_SYMBOL(shrinker_register); This just looks wrong - you are trying to use WRITE_ONCE() as a release barrier to indicate that the shrinker is now set up fully. That's not necessary - the refcount is an atomic and along with the rcu locks they should provides all the barriers we need. i.e. void shrinker_register(struct shrinker *shrinker) { down_write(&shrinker_rwsem); list_add_tail_rcu(&shrinker->list, &shrinker_list); shrinker->flags |= SHRINKER_REGISTERED; shrinker_debugfs_add(shrinker); up_write(&shrinker_rwsem); /* * now the shrinker is fully set up, take the first * reference to it to indicate that lookup operations are * now allowed to use it via shrinker_try_get(). */ refcount_set(&shrinker->refcount, 1); } > diff --git a/mm/shrinker_debug.c b/mm/shrinker_debug.c > index f1becfd45853..c5573066adbf 100644 > --- a/mm/shrinker_debug.c > +++ b/mm/shrinker_debug.c > @@ -5,6 +5,7 @@ > #include <linux/seq_file.h> > #include <linux/shrinker.h> > #include <linux/memcontrol.h> > +#include <linux/rculist.h> > > /* defined in vmscan.c */ > extern struct rw_semaphore shrinker_rwsem; > @@ -161,17 +162,21 @@ int shrinker_debugfs_add(struct shrinker *shrinker) > { > struct dentry *entry; > char buf[128]; > - int id; > - > - lockdep_assert_held(&shrinker_rwsem); > + int id, ret = 0; > > /* debugfs isn't initialized yet, add debugfs entries later. */ > if (!shrinker_debugfs_root) > return 0; > > + down_write(&shrinker_rwsem); > + if (shrinker->debugfs_entry) > + goto fail; > + > id = ida_alloc(&shrinker_debugfs_ida, GFP_KERNEL); > - if (id < 0) > - return id; > + if (id < 0) { > + ret = id; > + goto fail; > + } > shrinker->debugfs_id = id; > > snprintf(buf, sizeof(buf), "%s-%d", shrinker->name, id); > @@ -180,7 +185,8 @@ int shrinker_debugfs_add(struct shrinker *shrinker) > entry = debugfs_create_dir(buf, shrinker_debugfs_root); > if (IS_ERR(entry)) { > ida_free(&shrinker_debugfs_ida, id); > - return PTR_ERR(entry); > + ret = PTR_ERR(entry); > + goto fail; > } > shrinker->debugfs_entry = entry; > > @@ -188,7 +194,10 @@ int shrinker_debugfs_add(struct shrinker *shrinker) > &shrinker_debugfs_count_fops); > debugfs_create_file("scan", 0220, entry, shrinker, > &shrinker_debugfs_scan_fops); > - return 0; > + > +fail: > + up_write(&shrinker_rwsem); > + return ret; > } > > int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...) > @@ -243,6 +252,11 @@ struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker, > shrinker->name = NULL; > > *debugfs_id = entry ? shrinker->debugfs_id : -1; > + /* > + * Ensure that shrinker->registered has been set to false before > + * shrinker->debugfs_entry is set to NULL. > + */ > + smp_wmb(); > shrinker->debugfs_entry = NULL; > > return entry; > @@ -266,14 +280,26 @@ static int __init shrinker_debugfs_init(void) > shrinker_debugfs_root = dentry; > > /* Create debugfs entries for shrinkers registered at boot */ > - down_write(&shrinker_rwsem); > - list_for_each_entry(shrinker, &shrinker_list, list) > + rcu_read_lock(); > + list_for_each_entry_rcu(shrinker, &shrinker_list, list) { > + if (!shrinker_try_get(shrinker)) > + continue; > + rcu_read_unlock(); > + > if (!shrinker->debugfs_entry) { > - ret = shrinker_debugfs_add(shrinker); > - if (ret) > - break; > + /* Paired with smp_wmb() in shrinker_debugfs_detach() */ > + smp_rmb(); > + if (READ_ONCE(shrinker->registered)) > + ret = shrinker_debugfs_add(shrinker); > } > - up_write(&shrinker_rwsem); > + > + rcu_read_lock(); > + shrinker_put(shrinker); > + > + if (ret) > + break; > + } > + rcu_read_unlock(); > > return ret; > } And all this churn and complexity can go away because the shrinker_rwsem is still used to protect shrinker_register() entirely.... -Dave.
Hi Dave, On 2023/7/26 16:08, Dave Chinner wrote: > On Mon, Jul 24, 2023 at 05:43:51PM +0800, Qi Zheng wrote: >> The shrinker_rwsem is a global read-write lock in shrinkers subsystem, >> which protects most operations such as slab shrink, registration and >> unregistration of shrinkers, etc. This can easily cause problems in the >> following cases. >> >> 1) When the memory pressure is high and there are many filesystems >> mounted or unmounted at the same time, slab shrink will be affected >> (down_read_trylock() failed). >> >> Such as the real workload mentioned by Kirill Tkhai: >> >> ``` >> One of the real workloads from my experience is start >> of an overcommitted node containing many starting >> containers after node crash (or many resuming containers >> after reboot for kernel update). In these cases memory >> pressure is huge, and the node goes round in long reclaim. >> ``` >> >> 2) If a shrinker is blocked (such as the case mentioned >> in [1]) and a writer comes in (such as mount a fs), >> then this writer will be blocked and cause all >> subsequent shrinker-related operations to be blocked. >> >> Even if there is no competitor when shrinking slab, there may still be a >> problem. The down_read_trylock() may become a perf hotspot with frequent >> calls to shrink_slab(). Because of the poor multicore scalability of >> atomic operations, this can lead to a significant drop in IPC >> (instructions per cycle). >> >> We used to implement the lockless slab shrink with SRCU [2], but then >> kernel test robot reported -88.8% regression in >> stress-ng.ramfs.ops_per_sec test case [3], so we reverted it [4]. >> >> This commit uses the refcount+RCU method [5] proposed by Dave Chinner >> to re-implement the lockless global slab shrink. The memcg slab shrink is >> handled in the subsequent patch. >> >> For now, all shrinker instances are converted to dynamically allocated and >> will be freed by kfree_rcu(). So we can use rcu_read_{lock,unlock}() to >> ensure that the shrinker instance is valid. >> >> And the shrinker instance will not be run again after unregistration. So >> the structure that records the pointer of shrinker instance can be safely >> freed without waiting for the RCU read-side critical section. >> >> In this way, while we implement the lockless slab shrink, we don't need to >> be blocked in unregister_shrinker(). >> >> The following are the test results: >> >> stress-ng --timeout 60 --times --verify --metrics-brief --ramfs 9 & >> >> 1) Before applying this patchset: >> >> setting to a 60 second run per stressor >> dispatching hogs: 9 ramfs >> stressor bogo ops real time usr time sys time bogo ops/s bogo ops/s >> (secs) (secs) (secs) (real time) (usr+sys time) >> ramfs 735238 60.00 12.37 363.70 12253.05 1955.08 >> for a 60.01s run time: >> 1440.27s available CPU time >> 12.36s user time ( 0.86%) >> 363.70s system time ( 25.25%) >> 376.06s total time ( 26.11%) >> load average: 10.79 4.47 1.69 >> passed: 9: ramfs (9) >> failed: 0 >> skipped: 0 >> successful run completed in 60.01s (1 min, 0.01 secs) >> >> 2) After applying this patchset: >> >> setting to a 60 second run per stressor >> dispatching hogs: 9 ramfs >> stressor bogo ops real time usr time sys time bogo ops/s bogo ops/s >> (secs) (secs) (secs) (real time) (usr+sys time) >> ramfs 746677 60.00 12.22 367.75 12443.70 1965.13 >> for a 60.01s run time: >> 1440.26s available CPU time >> 12.21s user time ( 0.85%) >> 367.75s system time ( 25.53%) >> 379.96s total time ( 26.38%) >> load average: 8.37 2.48 0.86 >> passed: 9: ramfs (9) >> failed: 0 >> skipped: 0 >> successful run completed in 60.01s (1 min, 0.01 secs) >> >> We can see that the ops/s has hardly changed. >> >> [1]. https://lore.kernel.org/lkml/20191129214541.3110-1-ptikhomirov@virtuozzo.com/ >> [2]. https://lore.kernel.org/lkml/20230313112819.38938-1-zhengqi.arch@bytedance.com/ >> [3]. https://lore.kernel.org/lkml/202305230837.db2c233f-yujie.liu@intel.com/ >> [4]. https://lore.kernel.org/all/20230609081518.3039120-1-qi.zheng@linux.dev/ >> [5]. https://lore.kernel.org/lkml/ZIJhou1d55d4H1s0@dread.disaster.area/ >> >> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >> --- >> include/linux/shrinker.h | 19 +++++++--- >> mm/shrinker.c | 75 ++++++++++++++++++++++++++-------------- >> mm/shrinker_debug.c | 52 +++++++++++++++++++++------- >> 3 files changed, 104 insertions(+), 42 deletions(-) >> >> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h >> index 36977a70bebb..335da93cccee 100644 >> --- a/include/linux/shrinker.h >> +++ b/include/linux/shrinker.h >> @@ -4,6 +4,7 @@ >> >> #include <linux/atomic.h> >> #include <linux/types.h> >> +#include <linux/refcount.h> >> >> #define SHRINKER_UNIT_BITS BITS_PER_LONG >> >> @@ -86,6 +87,10 @@ struct shrinker { >> long batch; /* reclaim batch size, 0 = default */ >> int seeks; /* seeks to recreate an obj */ >> unsigned flags; >> + bool registered; >> + >> + refcount_t refcount; >> + struct rcu_head rcu; >> >> void *private_data; >> >> @@ -106,14 +111,13 @@ struct shrinker { >> #define DEFAULT_SEEKS 2 /* A good number if you don't know better. */ >> >> /* Flags */ >> -#define SHRINKER_REGISTERED (1 << 0) >> -#define SHRINKER_NUMA_AWARE (1 << 1) >> -#define SHRINKER_MEMCG_AWARE (1 << 2) >> +#define SHRINKER_NUMA_AWARE (1 << 0) >> +#define SHRINKER_MEMCG_AWARE (1 << 1) >> /* >> * It just makes sense when the shrinker is also MEMCG_AWARE for now, >> * non-MEMCG_AWARE shrinker should not have this flag set. >> */ >> -#define SHRINKER_NONSLAB (1 << 3) >> +#define SHRINKER_NONSLAB (1 << 2) >> >> unsigned long shrink_slab(gfp_t gfp_mask, int nid, struct mem_cgroup *memcg, >> int priority); >> @@ -122,6 +126,13 @@ void shrinker_free_non_registered(struct shrinker *shrinker); >> void shrinker_register(struct shrinker *shrinker); >> void shrinker_unregister(struct shrinker *shrinker); >> >> +static inline bool shrinker_try_get(struct shrinker *shrinker) >> +{ >> + return READ_ONCE(shrinker->registered) && >> + refcount_inc_not_zero(&shrinker->refcount); >> +} > > Why do we care about shrinker->registered here? If we don't set > the refcount to 1 until we have fully initialised everything, then > the shrinker code can key entirely off the reference count and > none of the lookup code needs to care about whether the shrinker is > registered or not. The purpose of checking shrinker->registered here is to stop running shrinker after calling shrinker_free(), which can prevent the following situations from happening: CPU 0 CPU 1 shrinker_try_get() shrinker_try_get() shrinker_put() shrinker_try_get() shrinker_put() This chance of this happening is small, but theoretically possible, so I added this check. > >> +void shrinker_put(struct shrinker *shrinker); >> + >> #ifdef CONFIG_SHRINKER_DEBUG >> extern int shrinker_debugfs_add(struct shrinker *shrinker); >> extern struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker, >> diff --git a/mm/shrinker.c b/mm/shrinker.c >> index 8a1fe844f1a4..8e3334749552 100644 >> --- a/mm/shrinker.c >> +++ b/mm/shrinker.c >> @@ -2,10 +2,13 @@ >> #include <linux/memcontrol.h> >> #include <linux/rwsem.h> >> #include <linux/shrinker.h> >> +#include <linux/rculist.h> >> +#include <linux/spinlock.h> >> #include <trace/events/vmscan.h> >> >> LIST_HEAD(shrinker_list); >> DECLARE_RWSEM(shrinker_rwsem); >> +DEFINE_SPINLOCK(shrinker_lock); >> >> #ifdef CONFIG_MEMCG >> static int shrinker_nr_max; >> @@ -450,6 +453,18 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl, >> return freed; >> } >> >> +void shrinker_put(struct shrinker *shrinker) >> +{ >> + if (refcount_dec_and_test(&shrinker->refcount)) { >> + spin_lock(&shrinker_lock); >> + list_del_rcu(&shrinker->list); >> + spin_unlock(&shrinker_lock); >> + >> + kfree(shrinker->nr_deferred); >> + kfree_rcu(shrinker, rcu); >> + } >> +} > > Urk, no. > > We want the shrinker_free() code to block waiting for the shrinker > reference count to go to zero, because the shrinkers can reference > structures that are associated with the path that is freeing the > shrinker. > > i.e. we do not want to free the superblock of a filesystem whilst > the shrinker is still running, but the way you've done this is that > the shrinker can run whilst the structure that contains it has been > torn down. Oh, I missed this. I will change it back to use completion as soon as possible. > > This should use a completion, then it is always safe under > rcu_read_lock(). This also gets rid of the shrinker_lock spin lock, > which only exists because we can't take a blocking lock under > rcu_read_lock(). i.e: > > > void shrinker_put(struct shrinker *shrinker) > { > if (refcount_dec_and_test(&shrinker->refcount)) > complete(&shrinker->done); > } > > void shrinker_free() > { > ..... > refcount_dec(&shrinker->refcount); I guess what you mean is shrinker_put(), because here may be the last refcount. > wait_for_completion(&shrinker->done); > /* > * lookups on the shrinker will now all fail as refcount has > * fallen to zero. We can now remove it from the lists and > * free it. > */ > down_write(shrinker_rwsem); > list_del_rcu(&shrinker->list); > up_write(&shrinker_rwsem); > call_rcu(shrinker->rcu_head, shrinker_free_rcu_cb); > } > > .... > >> @@ -686,11 +711,14 @@ EXPORT_SYMBOL(shrinker_free_non_registered); >> >> void shrinker_register(struct shrinker *shrinker) >> { >> - down_write(&shrinker_rwsem); >> - list_add_tail(&shrinker->list, &shrinker_list); >> - shrinker->flags |= SHRINKER_REGISTERED; >> + refcount_set(&shrinker->refcount, 1); >> + >> + spin_lock(&shrinker_lock); >> + list_add_tail_rcu(&shrinker->list, &shrinker_list); >> + spin_unlock(&shrinker_lock); >> + >> shrinker_debugfs_add(shrinker); >> - up_write(&shrinker_rwsem); >> + WRITE_ONCE(shrinker->registered, true); >> } >> EXPORT_SYMBOL(shrinker_register); > > This just looks wrong - you are trying to use WRITE_ONCE() as a > release barrier to indicate that the shrinker is now set up fully. > That's not necessary - the refcount is an atomic and along with the > rcu locks they should provides all the barriers we need. i.e. The reason I used WRITE_ONCE() here is because the shrinker->registered will be read and written concurrently (read in shrinker_try_get() and written in shrinker_free()), which is why I added shrinker::registered field instead of using SHRINKER_REGISTERED flag (this can reduce the addition of WRITE_ONCE()/READ_ONCE()). > > void shrinker_register(struct shrinker *shrinker) > { > down_write(&shrinker_rwsem); > list_add_tail_rcu(&shrinker->list, &shrinker_list); > shrinker->flags |= SHRINKER_REGISTERED; > shrinker_debugfs_add(shrinker); > up_write(&shrinker_rwsem); > > /* > * now the shrinker is fully set up, take the first > * reference to it to indicate that lookup operations are > * now allowed to use it via shrinker_try_get(). > */ > refcount_set(&shrinker->refcount, 1); > } > >> diff --git a/mm/shrinker_debug.c b/mm/shrinker_debug.c >> index f1becfd45853..c5573066adbf 100644 >> --- a/mm/shrinker_debug.c >> +++ b/mm/shrinker_debug.c >> @@ -5,6 +5,7 @@ >> #include <linux/seq_file.h> >> #include <linux/shrinker.h> >> #include <linux/memcontrol.h> >> +#include <linux/rculist.h> >> >> /* defined in vmscan.c */ >> extern struct rw_semaphore shrinker_rwsem; >> @@ -161,17 +162,21 @@ int shrinker_debugfs_add(struct shrinker *shrinker) >> { >> struct dentry *entry; >> char buf[128]; >> - int id; >> - >> - lockdep_assert_held(&shrinker_rwsem); >> + int id, ret = 0; >> >> /* debugfs isn't initialized yet, add debugfs entries later. */ >> if (!shrinker_debugfs_root) >> return 0; >> >> + down_write(&shrinker_rwsem); >> + if (shrinker->debugfs_entry) >> + goto fail; >> + >> id = ida_alloc(&shrinker_debugfs_ida, GFP_KERNEL); >> - if (id < 0) >> - return id; >> + if (id < 0) { >> + ret = id; >> + goto fail; >> + } >> shrinker->debugfs_id = id; >> >> snprintf(buf, sizeof(buf), "%s-%d", shrinker->name, id); >> @@ -180,7 +185,8 @@ int shrinker_debugfs_add(struct shrinker *shrinker) >> entry = debugfs_create_dir(buf, shrinker_debugfs_root); >> if (IS_ERR(entry)) { >> ida_free(&shrinker_debugfs_ida, id); >> - return PTR_ERR(entry); >> + ret = PTR_ERR(entry); >> + goto fail; >> } >> shrinker->debugfs_entry = entry; >> >> @@ -188,7 +194,10 @@ int shrinker_debugfs_add(struct shrinker *shrinker) >> &shrinker_debugfs_count_fops); >> debugfs_create_file("scan", 0220, entry, shrinker, >> &shrinker_debugfs_scan_fops); >> - return 0; >> + >> +fail: >> + up_write(&shrinker_rwsem); >> + return ret; >> } >> >> int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...) >> @@ -243,6 +252,11 @@ struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker, >> shrinker->name = NULL; >> >> *debugfs_id = entry ? shrinker->debugfs_id : -1; >> + /* >> + * Ensure that shrinker->registered has been set to false before >> + * shrinker->debugfs_entry is set to NULL. >> + */ >> + smp_wmb(); >> shrinker->debugfs_entry = NULL; >> >> return entry; >> @@ -266,14 +280,26 @@ static int __init shrinker_debugfs_init(void) >> shrinker_debugfs_root = dentry; >> >> /* Create debugfs entries for shrinkers registered at boot */ >> - down_write(&shrinker_rwsem); >> - list_for_each_entry(shrinker, &shrinker_list, list) >> + rcu_read_lock(); >> + list_for_each_entry_rcu(shrinker, &shrinker_list, list) { >> + if (!shrinker_try_get(shrinker)) >> + continue; >> + rcu_read_unlock(); >> + >> if (!shrinker->debugfs_entry) { >> - ret = shrinker_debugfs_add(shrinker); >> - if (ret) >> - break; >> + /* Paired with smp_wmb() in shrinker_debugfs_detach() */ >> + smp_rmb(); >> + if (READ_ONCE(shrinker->registered)) >> + ret = shrinker_debugfs_add(shrinker); >> } >> - up_write(&shrinker_rwsem); >> + >> + rcu_read_lock(); >> + shrinker_put(shrinker); >> + >> + if (ret) >> + break; >> + } >> + rcu_read_unlock(); >> >> return ret; >> } > > And all this churn and complexity can go away because the > shrinker_rwsem is still used to protect shrinker_register() > entirely.... My consideration is that during this process, there may be a driver probe failure and then shrinker_free() is called (the shrinker_debugfs_init() is called in late_initcall stage). In this case, we need to use RCU+refcount to ensure that the shrinker is not freed. And after switching back to using completion, the smp_rmb()/smp_wmb() is no longer needed. Thanks, Qi > > -Dave.
On 2023/7/26 15:10, Muchun Song wrote: > > > On 2023/7/24 17:43, Qi Zheng wrote: >> Use new APIs to dynamically allocate the thp-zero and thp-deferred_split >> shrinkers. >> >> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >> --- >> mm/huge_memory.c | 69 +++++++++++++++++++++++++++++++----------------- >> 1 file changed, 45 insertions(+), 24 deletions(-) >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index 8c94b34024a2..4db5a1834d81 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -65,7 +65,11 @@ unsigned long transparent_hugepage_flags >> __read_mostly = >> (1<<TRANSPARENT_HUGEPAGE_DEFRAG_KHUGEPAGED_FLAG)| >> (1<<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG); >> -static struct shrinker deferred_split_shrinker; >> +static struct shrinker *deferred_split_shrinker; >> +static unsigned long deferred_split_count(struct shrinker *shrink, >> + struct shrink_control *sc); >> +static unsigned long deferred_split_scan(struct shrinker *shrink, >> + struct shrink_control *sc); >> static atomic_t huge_zero_refcount; >> struct page *huge_zero_page __read_mostly; >> @@ -229,11 +233,7 @@ static unsigned long >> shrink_huge_zero_page_scan(struct shrinker *shrink, >> return 0; >> } >> -static struct shrinker huge_zero_page_shrinker = { >> - .count_objects = shrink_huge_zero_page_count, >> - .scan_objects = shrink_huge_zero_page_scan, >> - .seeks = DEFAULT_SEEKS, >> -}; >> +static struct shrinker *huge_zero_page_shrinker; > > Same as patch #17. OK, will do. > >> #ifdef CONFIG_SYSFS >> static ssize_t enabled_show(struct kobject *kobj, >> @@ -454,6 +454,40 @@ static inline void hugepage_exit_sysfs(struct >> kobject *hugepage_kobj) >> } >> #endif /* CONFIG_SYSFS */ >> +static int thp_shrinker_init(void) > > Better to declare it as __init. Will do. > >> +{ >> + huge_zero_page_shrinker = shrinker_alloc(0, "thp-zero"); >> + if (!huge_zero_page_shrinker) >> + return -ENOMEM; >> + >> + deferred_split_shrinker = shrinker_alloc(SHRINKER_NUMA_AWARE | >> + SHRINKER_MEMCG_AWARE | >> + SHRINKER_NONSLAB, >> + "thp-deferred_split"); >> + if (!deferred_split_shrinker) { >> + shrinker_free_non_registered(huge_zero_page_shrinker); >> + return -ENOMEM; >> + } >> + >> + huge_zero_page_shrinker->count_objects = >> shrink_huge_zero_page_count; >> + huge_zero_page_shrinker->scan_objects = shrink_huge_zero_page_scan; >> + huge_zero_page_shrinker->seeks = DEFAULT_SEEKS; >> + shrinker_register(huge_zero_page_shrinker); >> + >> + deferred_split_shrinker->count_objects = deferred_split_count; >> + deferred_split_shrinker->scan_objects = deferred_split_scan; >> + deferred_split_shrinker->seeks = DEFAULT_SEEKS; >> + shrinker_register(deferred_split_shrinker); >> + >> + return 0; >> +} >> + >> +static void thp_shrinker_exit(void) > > Same as here. Will do. > >> +{ >> + shrinker_unregister(huge_zero_page_shrinker); >> + shrinker_unregister(deferred_split_shrinker); >> +} >> + >> static int __init hugepage_init(void) >> { >> int err; >> @@ -482,12 +516,9 @@ static int __init hugepage_init(void) >> if (err) >> goto err_slab; >> - err = register_shrinker(&huge_zero_page_shrinker, "thp-zero"); >> - if (err) >> - goto err_hzp_shrinker; >> - err = register_shrinker(&deferred_split_shrinker, >> "thp-deferred_split"); >> + err = thp_shrinker_init(); >> if (err) >> - goto err_split_shrinker; >> + goto err_shrinker; >> /* >> * By default disable transparent hugepages on smaller systems, >> @@ -505,10 +536,8 @@ static int __init hugepage_init(void) >> return 0; >> err_khugepaged: >> - unregister_shrinker(&deferred_split_shrinker); >> -err_split_shrinker: >> - unregister_shrinker(&huge_zero_page_shrinker); >> -err_hzp_shrinker: >> + thp_shrinker_exit(); >> +err_shrinker: >> khugepaged_destroy(); >> err_slab: >> hugepage_exit_sysfs(hugepage_kobj); >> @@ -2851,7 +2880,7 @@ void deferred_split_folio(struct folio *folio) >> #ifdef CONFIG_MEMCG >> if (memcg) >> set_shrinker_bit(memcg, folio_nid(folio), >> - deferred_split_shrinker.id); >> + deferred_split_shrinker->id); >> #endif >> } >> spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags); >> @@ -2925,14 +2954,6 @@ static unsigned long deferred_split_scan(struct >> shrinker *shrink, >> return split; >> } >> -static struct shrinker deferred_split_shrinker = { >> - .count_objects = deferred_split_count, >> - .scan_objects = deferred_split_scan, >> - .seeks = DEFAULT_SEEKS, >> - .flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE | >> - SHRINKER_NONSLAB, >> -}; >> - >> #ifdef CONFIG_DEBUG_FS >> static void split_huge_pages_all(void) >> { >
On 2023/7/26 15:13, Muchun Song wrote: > > >> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: >> >> Use new APIs to dynamically allocate the mm-shadow shrinker. >> >> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >> --- >> mm/workingset.c | 26 ++++++++++++++------------ >> 1 file changed, 14 insertions(+), 12 deletions(-) >> >> diff --git a/mm/workingset.c b/mm/workingset.c >> index 4686ae363000..4bc85f739b13 100644 >> --- a/mm/workingset.c >> +++ b/mm/workingset.c >> @@ -762,12 +762,7 @@ static unsigned long scan_shadow_nodes(struct shrinker *shrinker, >> NULL); >> } >> >> -static struct shrinker workingset_shadow_shrinker = { >> - .count_objects = count_shadow_nodes, >> - .scan_objects = scan_shadow_nodes, >> - .seeks = 0, /* ->count reports only fully expendable nodes */ >> - .flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE, >> -}; >> +static struct shrinker *workingset_shadow_shrinker; > > > Same as patch #17. OK, will do.
> On Jul 24, 2023, at 17:43, Qi Zheng <zhengqi.arch@bytedance.com> wrote: > > Currently, we maintain two linear arrays per node per memcg, which are > shrinker_info::map and shrinker_info::nr_deferred. And we need to resize > them when the shrinker_nr_max is exceeded, that is, allocate a new array, > and then copy the old array to the new array, and finally free the old > array by RCU. > > For shrinker_info::map, we do set_bit() under the RCU lock, so we may set > the value into the old map which is about to be freed. This may cause the > value set to be lost. The current solution is not to copy the old map when > resizing, but to set all the corresponding bits in the new map to 1. This > solves the data loss problem, but bring the overhead of more pointless > loops while doing memcg slab shrink. > > For shrinker_info::nr_deferred, we will only modify it under the read lock > of shrinker_rwsem, so it will not run concurrently with the resizing. But > after we make memcg slab shrink lockless, there will be the same data loss > problem as shrinker_info::map, and we can't work around it like the map. > > For such resizable arrays, the most straightforward idea is to change it > to xarray, like we did for list_lru [1]. We need to do xa_store() in the > list_lru_add()-->set_shrinker_bit(), but this will cause memory > allocation, and the list_lru_add() doesn't accept failure. A possible > solution is to pre-allocate, but the location of pre-allocation is not > well determined. > > Therefore, this commit chooses to introduce a secondary array for > shrinker_info::{map, nr_deferred}, so that we only need to copy this > secondary array every time the size is resized. Then even if we get the > old secondary array under the RCU lock, the found map and nr_deferred are > also true, so no data is lost. > > [1]. https://lore.kernel.org/all/20220228122126.37293-13-songmuchun@bytedance.com/ > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> Reviewed-by: Muchun Song <songmuchun@bytedance.com>
On 2023/7/26 15:24, Muchun Song wrote: > > > On 2023/7/24 17:43, Qi Zheng wrote: >> In preparation for implementing lockless slab shrink, use new APIs to >> dynamically allocate the drm-msm_gem shrinker, so that it can be freed >> asynchronously using kfree_rcu(). Then it doesn't need to wait for RCU >> read-side critical section when releasing the struct msm_drm_private. >> >> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> > > Reviewed-by: Muchun Song <songmuchun@bytedance.com> > > A nit bellow. > >> --- >> drivers/gpu/drm/msm/msm_drv.c | 4 ++- >> drivers/gpu/drm/msm/msm_drv.h | 4 +-- >> drivers/gpu/drm/msm/msm_gem_shrinker.c | 36 ++++++++++++++++---------- >> 3 files changed, 28 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/msm_drv.c >> b/drivers/gpu/drm/msm/msm_drv.c >> index 891eff8433a9..7f6933be703f 100644 >> --- a/drivers/gpu/drm/msm/msm_drv.c >> +++ b/drivers/gpu/drm/msm/msm_drv.c >> @@ -461,7 +461,9 @@ static int msm_drm_init(struct device *dev, const >> struct drm_driver *drv) >> if (ret) >> goto err_msm_uninit; >> - msm_gem_shrinker_init(ddev); >> + ret = msm_gem_shrinker_init(ddev); >> + if (ret) >> + goto err_msm_uninit; >> if (priv->kms_init) { >> ret = priv->kms_init(ddev); >> diff --git a/drivers/gpu/drm/msm/msm_drv.h >> b/drivers/gpu/drm/msm/msm_drv.h >> index e13a8cbd61c9..84523d4a1e58 100644 >> --- a/drivers/gpu/drm/msm/msm_drv.h >> +++ b/drivers/gpu/drm/msm/msm_drv.h >> @@ -217,7 +217,7 @@ struct msm_drm_private { >> } vram; >> struct notifier_block vmap_notifier; >> - struct shrinker shrinker; >> + struct shrinker *shrinker; >> struct drm_atomic_state *pm_state; >> @@ -279,7 +279,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, >> void *data, >> unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, >> unsigned long nr_to_scan); >> #endif >> -void msm_gem_shrinker_init(struct drm_device *dev); >> +int msm_gem_shrinker_init(struct drm_device *dev); >> void msm_gem_shrinker_cleanup(struct drm_device *dev); >> int msm_gem_prime_mmap(struct drm_gem_object *obj, struct >> vm_area_struct *vma); >> diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c >> b/drivers/gpu/drm/msm/msm_gem_shrinker.c >> index f38296ad8743..7daab1298c11 100644 >> --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c >> +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c >> @@ -34,8 +34,7 @@ static bool can_block(struct shrink_control *sc) >> static unsigned long >> msm_gem_shrinker_count(struct shrinker *shrinker, struct >> shrink_control *sc) >> { >> - struct msm_drm_private *priv = >> - container_of(shrinker, struct msm_drm_private, shrinker); >> + struct msm_drm_private *priv = shrinker->private_data; >> unsigned count = priv->lru.dontneed.count; >> if (can_swap()) >> @@ -100,8 +99,7 @@ active_evict(struct drm_gem_object *obj) >> static unsigned long >> msm_gem_shrinker_scan(struct shrinker *shrinker, struct >> shrink_control *sc) >> { >> - struct msm_drm_private *priv = >> - container_of(shrinker, struct msm_drm_private, shrinker); >> + struct msm_drm_private *priv = shrinker->private_data; >> struct { >> struct drm_gem_lru *lru; >> bool (*shrink)(struct drm_gem_object *obj); >> @@ -148,10 +146,11 @@ msm_gem_shrinker_shrink(struct drm_device *dev, >> unsigned long nr_to_scan) >> struct shrink_control sc = { >> .nr_to_scan = nr_to_scan, >> }; >> - int ret; >> + unsigned long ret = SHRINK_STOP; >> fs_reclaim_acquire(GFP_KERNEL); >> - ret = msm_gem_shrinker_scan(&priv->shrinker, &sc); >> + if (priv->shrinker) >> + ret = msm_gem_shrinker_scan(priv->shrinker, &sc); >> fs_reclaim_release(GFP_KERNEL); >> return ret; >> @@ -210,16 +209,27 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, >> unsigned long event, void *ptr) >> * >> * This function registers and sets up the msm shrinker. >> */ >> -void msm_gem_shrinker_init(struct drm_device *dev) >> +int msm_gem_shrinker_init(struct drm_device *dev) >> { >> struct msm_drm_private *priv = dev->dev_private; >> - priv->shrinker.count_objects = msm_gem_shrinker_count; >> - priv->shrinker.scan_objects = msm_gem_shrinker_scan; >> - priv->shrinker.seeks = DEFAULT_SEEKS; >> - WARN_ON(register_shrinker(&priv->shrinker, "drm-msm_gem")); >> + >> + priv->shrinker = shrinker_alloc(0, "drm-msm_gem"); >> + if (!priv->shrinker) { > > Just "if (WARN_ON(!priv->shrinker))" As suggested by Steven Pric in patch #24, this warning is unnecessary, so I will remove it in the next version. > >> + WARN_ON(1); >> + return -ENOMEM; >> + } >> + >> + priv->shrinker->count_objects = msm_gem_shrinker_count; >> + priv->shrinker->scan_objects = msm_gem_shrinker_scan; >> + priv->shrinker->seeks = DEFAULT_SEEKS; >> + priv->shrinker->private_data = priv; >> + >> + shrinker_register(priv->shrinker); >> priv->vmap_notifier.notifier_call = msm_gem_shrinker_vmap; >> WARN_ON(register_vmap_purge_notifier(&priv->vmap_notifier)); >> + >> + return 0; >> } >> /** >> @@ -232,8 +242,8 @@ void msm_gem_shrinker_cleanup(struct drm_device *dev) >> { >> struct msm_drm_private *priv = dev->dev_private; >> - if (priv->shrinker.nr_deferred) { >> + if (priv->shrinker) { >> WARN_ON(unregister_vmap_purge_notifier(&priv->vmap_notifier)); >> - unregister_shrinker(&priv->shrinker); >> + shrinker_unregister(priv->shrinker); >> } >> } >
On 2023/7/26 15:32, Muchun Song wrote: > > > On 2023/7/24 17:43, Qi Zheng wrote: >> In preparation for implementing lockless slab shrink, use new APIs to >> dynamically allocate the md-bcache shrinker, so that it can be freed >> asynchronously using kfree_rcu(). Then it doesn't need to wait for RCU >> read-side critical section when releasing the struct cache_set. >> >> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> >> --- >> drivers/md/bcache/bcache.h | 2 +- >> drivers/md/bcache/btree.c | 27 ++++++++++++++++----------- >> drivers/md/bcache/sysfs.c | 3 ++- >> 3 files changed, 19 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h >> index 5a79bb3c272f..c622bc50f81b 100644 >> --- a/drivers/md/bcache/bcache.h >> +++ b/drivers/md/bcache/bcache.h >> @@ -541,7 +541,7 @@ struct cache_set { >> struct bio_set bio_split; >> /* For the btree cache */ >> - struct shrinker shrink; >> + struct shrinker *shrink; >> /* For the btree cache and anything allocation related */ >> struct mutex bucket_lock; >> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c >> index fd121a61f17c..c176c7fc77d9 100644 >> --- a/drivers/md/bcache/btree.c >> +++ b/drivers/md/bcache/btree.c >> @@ -667,7 +667,7 @@ static int mca_reap(struct btree *b, unsigned int >> min_order, bool flush) >> static unsigned long bch_mca_scan(struct shrinker *shrink, >> struct shrink_control *sc) >> { >> - struct cache_set *c = container_of(shrink, struct cache_set, >> shrink); >> + struct cache_set *c = shrink->private_data; >> struct btree *b, *t; >> unsigned long i, nr = sc->nr_to_scan; >> unsigned long freed = 0; >> @@ -734,7 +734,7 @@ static unsigned long bch_mca_scan(struct shrinker >> *shrink, >> static unsigned long bch_mca_count(struct shrinker *shrink, >> struct shrink_control *sc) >> { >> - struct cache_set *c = container_of(shrink, struct cache_set, >> shrink); >> + struct cache_set *c = shrink->private_data; >> if (c->shrinker_disabled) >> return 0; >> @@ -752,8 +752,8 @@ void bch_btree_cache_free(struct cache_set *c) >> closure_init_stack(&cl); >> - if (c->shrink.list.next) >> - unregister_shrinker(&c->shrink); >> + if (c->shrink) >> + shrinker_unregister(c->shrink); >> mutex_lock(&c->bucket_lock); >> @@ -828,14 +828,19 @@ int bch_btree_cache_alloc(struct cache_set *c) >> c->verify_data = NULL; >> #endif >> - c->shrink.count_objects = bch_mca_count; >> - c->shrink.scan_objects = bch_mca_scan; >> - c->shrink.seeks = 4; >> - c->shrink.batch = c->btree_pages * 2; >> + c->shrink = shrinker_alloc(0, "md-bcache:%pU", c->set_uuid); >> + if (!c->shrink) { >> + pr_warn("bcache: %s: could not allocate shrinker\n", __func__); >> + return -ENOMEM; > > Seems you have cheanged the semantic of this. In the past, > it is better to have a shrinker, but now it becomes a mandatory. > Right? I don't know if it is acceptable. From my point of view, > just do the cleanup, don't change any behaviour. Oh, should return 0 here, will do. > >> + } >> + >> + c->shrink->count_objects = bch_mca_count; >> + c->shrink->scan_objects = bch_mca_scan; >> + c->shrink->seeks = 4; >> + c->shrink->batch = c->btree_pages * 2; >> + c->shrink->private_data = c; >> - if (register_shrinker(&c->shrink, "md-bcache:%pU", c->set_uuid)) >> - pr_warn("bcache: %s: could not register shrinker\n", >> - __func__); >> + shrinker_register(c->shrink); >> return 0; >> } >> diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c >> index 0e2c1880f60b..45d8af755de6 100644 >> --- a/drivers/md/bcache/sysfs.c >> +++ b/drivers/md/bcache/sysfs.c >> @@ -866,7 +866,8 @@ STORE(__bch_cache_set) >> sc.gfp_mask = GFP_KERNEL; >> sc.nr_to_scan = strtoul_or_return(buf); >> - c->shrink.scan_objects(&c->shrink, &sc); >> + if (c->shrink) >> + c->shrink->scan_objects(c->shrink, &sc); >> } >> sysfs_strtoul_clamp(congested_read_threshold_us, >
On Wed, Jul 26, 2023 at 05:14:09PM +0800, Qi Zheng wrote: > On 2023/7/26 16:08, Dave Chinner wrote: > > On Mon, Jul 24, 2023 at 05:43:51PM +0800, Qi Zheng wrote: > > > @@ -122,6 +126,13 @@ void shrinker_free_non_registered(struct shrinker *shrinker); > > > void shrinker_register(struct shrinker *shrinker); > > > void shrinker_unregister(struct shrinker *shrinker); > > > +static inline bool shrinker_try_get(struct shrinker *shrinker) > > > +{ > > > + return READ_ONCE(shrinker->registered) && > > > + refcount_inc_not_zero(&shrinker->refcount); > > > +} > > > > Why do we care about shrinker->registered here? If we don't set > > the refcount to 1 until we have fully initialised everything, then > > the shrinker code can key entirely off the reference count and > > none of the lookup code needs to care about whether the shrinker is > > registered or not. > > The purpose of checking shrinker->registered here is to stop running > shrinker after calling shrinker_free(), which can prevent the following > situations from happening: > > CPU 0 CPU 1 > > shrinker_try_get() > > shrinker_try_get() > > shrinker_put() > shrinker_try_get() > shrinker_put() I don't see any race here? What is wrong with having multiple active users at once? > > > > This should use a completion, then it is always safe under > > rcu_read_lock(). This also gets rid of the shrinker_lock spin lock, > > which only exists because we can't take a blocking lock under > > rcu_read_lock(). i.e: > > > > > > void shrinker_put(struct shrinker *shrinker) > > { > > if (refcount_dec_and_test(&shrinker->refcount)) > > complete(&shrinker->done); > > } > > > > void shrinker_free() > > { > > ..... > > refcount_dec(&shrinker->refcount); > > I guess what you mean is shrinker_put(), because here may be the last > refcount. Yes, I did. > > wait_for_completion(&shrinker->done); > > /* > > * lookups on the shrinker will now all fail as refcount has > > * fallen to zero. We can now remove it from the lists and > > * free it. > > */ > > down_write(shrinker_rwsem); > > list_del_rcu(&shrinker->list); > > up_write(&shrinker_rwsem); > > call_rcu(shrinker->rcu_head, shrinker_free_rcu_cb); > > } > > > > .... > > > > > @@ -686,11 +711,14 @@ EXPORT_SYMBOL(shrinker_free_non_registered); > > > void shrinker_register(struct shrinker *shrinker) > > > { > > > - down_write(&shrinker_rwsem); > > > - list_add_tail(&shrinker->list, &shrinker_list); > > > - shrinker->flags |= SHRINKER_REGISTERED; > > > + refcount_set(&shrinker->refcount, 1); > > > + > > > + spin_lock(&shrinker_lock); > > > + list_add_tail_rcu(&shrinker->list, &shrinker_list); > > > + spin_unlock(&shrinker_lock); > > > + > > > shrinker_debugfs_add(shrinker); > > > - up_write(&shrinker_rwsem); > > > + WRITE_ONCE(shrinker->registered, true); > > > } > > > EXPORT_SYMBOL(shrinker_register); > > > > This just looks wrong - you are trying to use WRITE_ONCE() as a > > release barrier to indicate that the shrinker is now set up fully. > > That's not necessary - the refcount is an atomic and along with the > > rcu locks they should provides all the barriers we need. i.e. > > The reason I used WRITE_ONCE() here is because the shrinker->registered > will be read and written concurrently (read in shrinker_try_get() and > written in shrinker_free()), which is why I added shrinker::registered > field instead of using SHRINKER_REGISTERED flag (this can reduce the > addition of WRITE_ONCE()/READ_ONCE()). Using WRITE_ONCE/READ_ONCE doesn't provide memory barriers needed to use the field like this. You need release/acquire memory ordering here. i.e. smp_store_release()/smp_load_acquire(). As it is, the refcount_inc_not_zero() provides a control dependency, as documented in include/linux/refcount.h, refcount_dec_and_test() provides release memory ordering. The only thing I think we may need is a write barrier before refcount_set(), such that if refcount_inc_not_zero() sees a non-zero value, it is guaranteed to see an initialised structure... i.e. refcounts provide all the existence and initialisation guarantees. Hence I don't see the need to use shrinker->registered like this and it can remain a bit flag protected by the shrinker_rwsem(). > > void shrinker_register(struct shrinker *shrinker) > > { > > down_write(&shrinker_rwsem); > > list_add_tail_rcu(&shrinker->list, &shrinker_list); > > shrinker->flags |= SHRINKER_REGISTERED; > > shrinker_debugfs_add(shrinker); > > up_write(&shrinker_rwsem); > > > > /* > > * now the shrinker is fully set up, take the first > > * reference to it to indicate that lookup operations are > > * now allowed to use it via shrinker_try_get(). > > */ > > refcount_set(&shrinker->refcount, 1); > > } > > > > > diff --git a/mm/shrinker_debug.c b/mm/shrinker_debug.c > > > index f1becfd45853..c5573066adbf 100644 > > > --- a/mm/shrinker_debug.c > > > +++ b/mm/shrinker_debug.c > > > @@ -5,6 +5,7 @@ > > > #include <linux/seq_file.h> > > > #include <linux/shrinker.h> > > > #include <linux/memcontrol.h> > > > +#include <linux/rculist.h> > > > /* defined in vmscan.c */ > > > extern struct rw_semaphore shrinker_rwsem; > > > @@ -161,17 +162,21 @@ int shrinker_debugfs_add(struct shrinker *shrinker) > > > { > > > struct dentry *entry; > > > char buf[128]; > > > - int id; > > > - > > > - lockdep_assert_held(&shrinker_rwsem); > > > + int id, ret = 0; > > > /* debugfs isn't initialized yet, add debugfs entries later. */ > > > if (!shrinker_debugfs_root) > > > return 0; > > > + down_write(&shrinker_rwsem); > > > + if (shrinker->debugfs_entry) > > > + goto fail; > > > + > > > id = ida_alloc(&shrinker_debugfs_ida, GFP_KERNEL); > > > - if (id < 0) > > > - return id; > > > + if (id < 0) { > > > + ret = id; > > > + goto fail; > > > + } > > > shrinker->debugfs_id = id; > > > snprintf(buf, sizeof(buf), "%s-%d", shrinker->name, id); > > > @@ -180,7 +185,8 @@ int shrinker_debugfs_add(struct shrinker *shrinker) > > > entry = debugfs_create_dir(buf, shrinker_debugfs_root); > > > if (IS_ERR(entry)) { > > > ida_free(&shrinker_debugfs_ida, id); > > > - return PTR_ERR(entry); > > > + ret = PTR_ERR(entry); > > > + goto fail; > > > } > > > shrinker->debugfs_entry = entry; > > > @@ -188,7 +194,10 @@ int shrinker_debugfs_add(struct shrinker *shrinker) > > > &shrinker_debugfs_count_fops); > > > debugfs_create_file("scan", 0220, entry, shrinker, > > > &shrinker_debugfs_scan_fops); > > > - return 0; > > > + > > > +fail: > > > + up_write(&shrinker_rwsem); > > > + return ret; > > > } > > > int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...) > > > @@ -243,6 +252,11 @@ struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker, > > > shrinker->name = NULL; > > > *debugfs_id = entry ? shrinker->debugfs_id : -1; > > > + /* > > > + * Ensure that shrinker->registered has been set to false before > > > + * shrinker->debugfs_entry is set to NULL. > > > + */ > > > + smp_wmb(); > > > shrinker->debugfs_entry = NULL; > > > return entry; > > > @@ -266,14 +280,26 @@ static int __init shrinker_debugfs_init(void) > > > shrinker_debugfs_root = dentry; > > > /* Create debugfs entries for shrinkers registered at boot */ > > > - down_write(&shrinker_rwsem); > > > - list_for_each_entry(shrinker, &shrinker_list, list) > > > + rcu_read_lock(); > > > + list_for_each_entry_rcu(shrinker, &shrinker_list, list) { > > > + if (!shrinker_try_get(shrinker)) > > > + continue; > > > + rcu_read_unlock(); > > > + > > > if (!shrinker->debugfs_entry) { > > > - ret = shrinker_debugfs_add(shrinker); > > > - if (ret) > > > - break; > > > + /* Paired with smp_wmb() in shrinker_debugfs_detach() */ > > > + smp_rmb(); > > > + if (READ_ONCE(shrinker->registered)) > > > + ret = shrinker_debugfs_add(shrinker); > > > } > > > - up_write(&shrinker_rwsem); > > > + > > > + rcu_read_lock(); > > > + shrinker_put(shrinker); > > > + > > > + if (ret) > > > + break; > > > + } > > > + rcu_read_unlock(); > > > return ret; > > > } > > > > And all this churn and complexity can go away because the > > shrinker_rwsem is still used to protect shrinker_register() > > entirely.... > > My consideration is that during this process, there may be a > driver probe failure and then shrinker_free() is called (the > shrinker_debugfs_init() is called in late_initcall stage). In > this case, we need to use RCU+refcount to ensure that the shrinker > is not freed. Yeah, you're trying to work around the lack of a wait_for_completion() call in shrinker_free(). With that, this doesn't need RCU at all, and the iteration can be done fully under the shrinker_rwsem() safely and so none of this code needs to change. Cheers, Dave.
Hi Dave, On 2023/7/27 07:09, Dave Chinner wrote: > On Wed, Jul 26, 2023 at 05:14:09PM +0800, Qi Zheng wrote: >> On 2023/7/26 16:08, Dave Chinner wrote: >>> On Mon, Jul 24, 2023 at 05:43:51PM +0800, Qi Zheng wrote: >>>> @@ -122,6 +126,13 @@ void shrinker_free_non_registered(struct shrinker *shrinker); >>>> void shrinker_register(struct shrinker *shrinker); >>>> void shrinker_unregister(struct shrinker *shrinker); >>>> +static inline bool shrinker_try_get(struct shrinker *shrinker) >>>> +{ >>>> + return READ_ONCE(shrinker->registered) && >>>> + refcount_inc_not_zero(&shrinker->refcount); >>>> +} >>> >>> Why do we care about shrinker->registered here? If we don't set >>> the refcount to 1 until we have fully initialised everything, then >>> the shrinker code can key entirely off the reference count and >>> none of the lookup code needs to care about whether the shrinker is >>> registered or not. >> >> The purpose of checking shrinker->registered here is to stop running >> shrinker after calling shrinker_free(), which can prevent the following >> situations from happening: >> >> CPU 0 CPU 1 >> >> shrinker_try_get() >> >> shrinker_try_get() >> >> shrinker_put() >> shrinker_try_get() >> shrinker_put() > > I don't see any race here? What is wrong with having multiple active > users at once? Maybe I'm overthinking. What I think is that if there are multiple users at once, it may cause the above-mentioned livelock, which will cause shrinker_free() to wait for a long time. But this probability should be very low. > >>> >>> This should use a completion, then it is always safe under >>> rcu_read_lock(). This also gets rid of the shrinker_lock spin lock, >>> which only exists because we can't take a blocking lock under >>> rcu_read_lock(). i.e: >>> >>> >>> void shrinker_put(struct shrinker *shrinker) >>> { >>> if (refcount_dec_and_test(&shrinker->refcount)) >>> complete(&shrinker->done); >>> } >>> >>> void shrinker_free() >>> { >>> ..... >>> refcount_dec(&shrinker->refcount); >> >> I guess what you mean is shrinker_put(), because here may be the last >> refcount. > > Yes, I did. > >>> wait_for_completion(&shrinker->done); >>> /* >>> * lookups on the shrinker will now all fail as refcount has >>> * fallen to zero. We can now remove it from the lists and >>> * free it. >>> */ >>> down_write(shrinker_rwsem); >>> list_del_rcu(&shrinker->list); >>> up_write(&shrinker_rwsem); >>> call_rcu(shrinker->rcu_head, shrinker_free_rcu_cb); >>> } >>> >>> .... >>> >>>> @@ -686,11 +711,14 @@ EXPORT_SYMBOL(shrinker_free_non_registered); >>>> void shrinker_register(struct shrinker *shrinker) >>>> { >>>> - down_write(&shrinker_rwsem); >>>> - list_add_tail(&shrinker->list, &shrinker_list); >>>> - shrinker->flags |= SHRINKER_REGISTERED; >>>> + refcount_set(&shrinker->refcount, 1); >>>> + >>>> + spin_lock(&shrinker_lock); >>>> + list_add_tail_rcu(&shrinker->list, &shrinker_list); >>>> + spin_unlock(&shrinker_lock); >>>> + >>>> shrinker_debugfs_add(shrinker); >>>> - up_write(&shrinker_rwsem); >>>> + WRITE_ONCE(shrinker->registered, true); >>>> } >>>> EXPORT_SYMBOL(shrinker_register); >>> >>> This just looks wrong - you are trying to use WRITE_ONCE() as a >>> release barrier to indicate that the shrinker is now set up fully. >>> That's not necessary - the refcount is an atomic and along with the >>> rcu locks they should provides all the barriers we need. i.e. >> >> The reason I used WRITE_ONCE() here is because the shrinker->registered >> will be read and written concurrently (read in shrinker_try_get() and >> written in shrinker_free()), which is why I added shrinker::registered >> field instead of using SHRINKER_REGISTERED flag (this can reduce the >> addition of WRITE_ONCE()/READ_ONCE()). > > Using WRITE_ONCE/READ_ONCE doesn't provide memory barriers needed to > use the field like this. You need release/acquire memory ordering > here. i.e. smp_store_release()/smp_load_acquire(). > > As it is, the refcount_inc_not_zero() provides a control dependency, > as documented in include/linux/refcount.h, refcount_dec_and_test() > provides release memory ordering. The only thing I think we may need > is a write barrier before refcount_set(), such that if > refcount_inc_not_zero() sees a non-zero value, it is guaranteed to > see an initialised structure... > > i.e. refcounts provide all the existence and initialisation > guarantees. Hence I don't see the need to use shrinker->registered > like this and it can remain a bit flag protected by the > shrinker_rwsem(). Ah, I didn't consider the memory order with refcount when I added WRITE_ONCE/READ_ONCE to shrinker->registered, just didn't want KCSAN to complain (there are multiple visitors at the same time, one of which is a writer). And the livelock case mentioned above is indeed unlikely to happen, so I will delete shrinker->registered in the next version. > > >>> void shrinker_register(struct shrinker *shrinker) >>> { >>> down_write(&shrinker_rwsem); >>> list_add_tail_rcu(&shrinker->list, &shrinker_list); >>> shrinker->flags |= SHRINKER_REGISTERED; >>> shrinker_debugfs_add(shrinker); >>> up_write(&shrinker_rwsem); >>> >>> /* >>> * now the shrinker is fully set up, take the first >>> * reference to it to indicate that lookup operations are >>> * now allowed to use it via shrinker_try_get(). >>> */ >>> refcount_set(&shrinker->refcount, 1); >>> } >>> >>>> diff --git a/mm/shrinker_debug.c b/mm/shrinker_debug.c >>>> index f1becfd45853..c5573066adbf 100644 >>>> --- a/mm/shrinker_debug.c >>>> +++ b/mm/shrinker_debug.c >>>> @@ -5,6 +5,7 @@ >>>> #include <linux/seq_file.h> >>>> #include <linux/shrinker.h> >>>> #include <linux/memcontrol.h> >>>> +#include <linux/rculist.h> >>>> /* defined in vmscan.c */ >>>> extern struct rw_semaphore shrinker_rwsem; >>>> @@ -161,17 +162,21 @@ int shrinker_debugfs_add(struct shrinker *shrinker) >>>> { >>>> struct dentry *entry; >>>> char buf[128]; >>>> - int id; >>>> - >>>> - lockdep_assert_held(&shrinker_rwsem); >>>> + int id, ret = 0; >>>> /* debugfs isn't initialized yet, add debugfs entries later. */ >>>> if (!shrinker_debugfs_root) >>>> return 0; >>>> + down_write(&shrinker_rwsem); >>>> + if (shrinker->debugfs_entry) >>>> + goto fail; >>>> + >>>> id = ida_alloc(&shrinker_debugfs_ida, GFP_KERNEL); >>>> - if (id < 0) >>>> - return id; >>>> + if (id < 0) { >>>> + ret = id; >>>> + goto fail; >>>> + } >>>> shrinker->debugfs_id = id; >>>> snprintf(buf, sizeof(buf), "%s-%d", shrinker->name, id); >>>> @@ -180,7 +185,8 @@ int shrinker_debugfs_add(struct shrinker *shrinker) >>>> entry = debugfs_create_dir(buf, shrinker_debugfs_root); >>>> if (IS_ERR(entry)) { >>>> ida_free(&shrinker_debugfs_ida, id); >>>> - return PTR_ERR(entry); >>>> + ret = PTR_ERR(entry); >>>> + goto fail; >>>> } >>>> shrinker->debugfs_entry = entry; >>>> @@ -188,7 +194,10 @@ int shrinker_debugfs_add(struct shrinker *shrinker) >>>> &shrinker_debugfs_count_fops); >>>> debugfs_create_file("scan", 0220, entry, shrinker, >>>> &shrinker_debugfs_scan_fops); >>>> - return 0; >>>> + >>>> +fail: >>>> + up_write(&shrinker_rwsem); >>>> + return ret; >>>> } >>>> int shrinker_debugfs_rename(struct shrinker *shrinker, const char *fmt, ...) >>>> @@ -243,6 +252,11 @@ struct dentry *shrinker_debugfs_detach(struct shrinker *shrinker, >>>> shrinker->name = NULL; >>>> *debugfs_id = entry ? shrinker->debugfs_id : -1; >>>> + /* >>>> + * Ensure that shrinker->registered has been set to false before >>>> + * shrinker->debugfs_entry is set to NULL. >>>> + */ >>>> + smp_wmb(); >>>> shrinker->debugfs_entry = NULL; >>>> return entry; >>>> @@ -266,14 +280,26 @@ static int __init shrinker_debugfs_init(void) >>>> shrinker_debugfs_root = dentry; >>>> /* Create debugfs entries for shrinkers registered at boot */ >>>> - down_write(&shrinker_rwsem); >>>> - list_for_each_entry(shrinker, &shrinker_list, list) >>>> + rcu_read_lock(); >>>> + list_for_each_entry_rcu(shrinker, &shrinker_list, list) { >>>> + if (!shrinker_try_get(shrinker)) >>>> + continue; >>>> + rcu_read_unlock(); >>>> + >>>> if (!shrinker->debugfs_entry) { >>>> - ret = shrinker_debugfs_add(shrinker); >>>> - if (ret) >>>> - break; >>>> + /* Paired with smp_wmb() in shrinker_debugfs_detach() */ >>>> + smp_rmb(); >>>> + if (READ_ONCE(shrinker->registered)) >>>> + ret = shrinker_debugfs_add(shrinker); >>>> } >>>> - up_write(&shrinker_rwsem); >>>> + >>>> + rcu_read_lock(); >>>> + shrinker_put(shrinker); >>>> + >>>> + if (ret) >>>> + break; >>>> + } >>>> + rcu_read_unlock(); >>>> return ret; >>>> } >>> >>> And all this churn and complexity can go away because the >>> shrinker_rwsem is still used to protect shrinker_register() >>> entirely.... >> >> My consideration is that during this process, there may be a >> driver probe failure and then shrinker_free() is called (the >> shrinker_debugfs_init() is called in late_initcall stage). In >> this case, we need to use RCU+refcount to ensure that the shrinker >> is not freed. > > Yeah, you're trying to work around the lack of a > wait_for_completion() call in shrinker_free(). > > With that, this doesn't need RCU at all, and the iteration can be > done fully under the shrinker_rwsem() safely and so none of this > code needs to change. Oh, indeed, here does not need to be changed. Thanks, Qi > > Cheers, > > Dave.
Hi all, 1. Background ============= We used to implement the lockless slab shrink with SRCU [1], but then kernel test robot reported -88.8% regression in stress-ng.ramfs.ops_per_sec test case [2], so we reverted it [3]. This patch series aims to re-implement the lockless slab shrink using the refcount+RCU method proposed by Dave Chinner [4]. [1]. https://lore.kernel.org/lkml/20230313112819.38938-1-zhengqi.arch@bytedance.com/ [2]. https://lore.kernel.org/lkml/202305230837.db2c233f-yujie.liu@intel.com/ [3]. https://lore.kernel.org/all/20230609081518.3039120-1-qi.zheng@linux.dev/ [4]. https://lore.kernel.org/lkml/ZIJhou1d55d4H1s0@dread.disaster.area/ 2. Implementation ================= 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(). This patchset 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() 2. shrinker_free_non_registered() 3. shrinker_register() 4. shrinker_unregister() In order to simplify shrinker-related APIs and make shrinker more independent of other kernel mechanisms, this patchset uses the above APIs 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 uncoverted shrinkers in the new setup. ``` Then we free the shrinker by calling kfree_rcu(), and use rcu_read_{lock,unlock}() to ensure that the shrinker instance is valid. And the shrinker::refcount mechanism ensures that the shrinker instance will not be run again after unregistration. So the structure that records the pointer of shrinker instance can be safely freed without waiting for the RCU read-side critical section. In this way, while we implement the lockless slab shrink, we don't need to be blocked in unregister_shrinker() to wait RCU read-side critical section. PATCH 1: move shrinker-related code into a separate file PATCH 2: remove redundant shrinker_rwsem in debugfs operations PATCH 3: add infrastructure for dynamically allocating shrinker PATCH 4 ~ 21: dynamically allocate the shrinker instances in case a and b PATCH 22 ~ 40: dynamically allocate the shrinker instances in case c PATCH 41: remove old APIs PATCH 42: introduce pool_shrink_rwsem to implement private synchronize_shrinkers() PATCH 43: add a secondary array for shrinker_info::{map, nr_deferred} PATCH 44 ~ 45: implement the lockless slab shrink PATCH 46 ~ 47: convert shrinker_rwsem to mutex 3. Testing ========== 3.1 slab shrink stress test --------------------------- We can reproduce the down_read_trylock() hotspot through the following script: ``` DIR="/root/shrinker/memcg/mnt" do_create() { mkdir -p /sys/fs/cgroup/memory/test echo 4G > /sys/fs/cgroup/memory/test/memory.limit_in_bytes for i in `seq 0 $1`; do mkdir -p /sys/fs/cgroup/memory/test/$i; echo $$ > /sys/fs/cgroup/memory/test/$i/cgroup.procs; mkdir -p $DIR/$i; done } do_mount() { for i in `seq $1 $2`; do mount -t tmpfs $i $DIR/$i; done } do_touch() { for i in `seq $1 $2`; do echo $$ > /sys/fs/cgroup/memory/test/$i/cgroup.procs; dd if=/dev/zero of=$DIR/$i/file$i bs=1M count=1 & done } case "$1" in touch) do_touch $2 $3 ;; test) do_create 4000 do_mount 0 4000 do_touch 0 3000 ;; *) exit 1 ;; esac ``` Save the above script, then run test and touch commands. Then we can use the following perf command to view hotspots: perf top -U -F 999 1) Before applying this patchset: 40.44% [kernel] [k] down_read_trylock 17.59% [kernel] [k] up_read 13.64% [kernel] [k] pv_native_safe_halt 11.90% [kernel] [k] shrink_slab 8.21% [kernel] [k] idr_find 2.71% [kernel] [k] _find_next_bit 1.36% [kernel] [k] shrink_node 0.81% [kernel] [k] shrink_lruvec 0.80% [kernel] [k] __radix_tree_lookup 0.50% [kernel] [k] do_shrink_slab 0.21% [kernel] [k] list_lru_count_one 0.16% [kernel] [k] mem_cgroup_iter 2) After applying this patchset: 60.17% [kernel] [k] shrink_slab 20.42% [kernel] [k] pv_native_safe_halt 3.03% [kernel] [k] do_shrink_slab 2.73% [kernel] [k] shrink_node 2.27% [kernel] [k] shrink_lruvec 2.00% [kernel] [k] __rcu_read_unlock 1.92% [kernel] [k] mem_cgroup_iter 0.98% [kernel] [k] __rcu_read_lock 0.91% [kernel] [k] osq_lock 0.63% [kernel] [k] mem_cgroup_calculate_protection 0.55% [kernel] [k] shrinker_put 0.46% [kernel] [k] list_lru_count_one We can see that the first perf hotspot becomes shrink_slab, which is what we expect. 3.2 registeration and unregisteration stress test ------------------------------------------------- Run the command below to test: stress-ng --timeout 60 --times --verify --metrics-brief --ramfs 9 & 1) Before applying this patchset: setting to a 60 second run per stressor dispatching hogs: 9 ramfs stressor bogo ops real time usr time sys time bogo ops/s bogo ops/s (secs) (secs) (secs) (real time) (usr+sys time) ramfs 735238 60.00 12.37 363.70 12253.05 1955.08 for a 60.01s run time: 1440.27s available CPU time 12.36s user time ( 0.86%) 363.70s system time ( 25.25%) 376.06s total time ( 26.11%) load average: 10.79 4.47 1.69 passed: 9: ramfs (9) failed: 0 skipped: 0 successful run completed in 60.01s (1 min, 0.01 secs) 2) After applying this patchset: setting to a 60 second run per stressor dispatching hogs: 9 ramfs stressor bogo ops real time usr time sys time bogo ops/s bogo ops/s (secs) (secs) (secs) (real time) (usr+sys time) ramfs 746677 60.00 12.22 367.75 12443.70 1965.13 for a 60.01s run time: 1440.26s available CPU time 12.21s user time ( 0.85%) 367.75s system time ( 25.53%) 379.96s total time ( 26.38%) load average: 8.37 2.48 0.86 passed: 9: ramfs (9) failed: 0 skipped: 0 successful run completed in 60.01s (1 min, 0.01 secs) We can see that the ops/s has hardly changed. This series is based on next-20230711, and the [PATCH v2 05/49] depends on the patch: https://lore.kernel.org/lkml/20230625154937.64316-1-qi.zheng@linux.dev/. Comments and suggestions are welcome. Thanks, Qi Changelog in v1 -> v2: - implement the new APIs and convert all shrinkers to use it. (suggested by Dave Chinner) - fix UAF in PATCH [05/29] (pointed by Steven Price) - add a secondary array for shrinker_info::{map, nr_deferred} - re-implement the lockless slab shrink (Since unifying the processing of global and memcg slab shrink needs to modify the startup sequence (As I mentioned in https://lore.kernel.org/lkml/38b14080-4ce5-d300-8a0a-c630bca6806b@bytedance.com/), I finally choose to process them separately.) - collect Acked-bys Qi Zheng (47): mm: vmscan: move shrinker-related code into a separate file mm: shrinker: remove redundant shrinker_rwsem in debugfs operations mm: shrinker: add infrastructure for dynamically allocating shrinker kvm: mmu: dynamically allocate the x86-mmu shrinker binder: dynamically allocate the android-binder shrinker drm/ttm: dynamically allocate the drm-ttm_pool shrinker xenbus/backend: dynamically allocate the xen-backend shrinker erofs: dynamically allocate the erofs-shrinker f2fs: dynamically allocate the f2fs-shrinker gfs2: dynamically allocate the gfs2-glock shrinker gfs2: dynamically allocate the gfs2-qd shrinker NFSv4.2: dynamically allocate the nfs-xattr shrinkers nfs: dynamically allocate the nfs-acl shrinker nfsd: dynamically allocate the nfsd-filecache shrinker quota: dynamically allocate the dquota-cache shrinker ubifs: dynamically allocate the ubifs-slab shrinker rcu: dynamically allocate the rcu-lazy shrinker rcu: dynamically allocate the rcu-kfree shrinker mm: thp: dynamically allocate the thp-related shrinkers sunrpc: dynamically allocate the sunrpc_cred shrinker mm: workingset: dynamically allocate the mm-shadow shrinker drm/i915: dynamically allocate the i915_gem_mm shrinker drm/msm: dynamically allocate the drm-msm_gem shrinker drm/panfrost: dynamically allocate the drm-panfrost shrinker dm: dynamically allocate the dm-bufio shrinker dm zoned: dynamically allocate the dm-zoned-meta shrinker md/raid5: dynamically allocate the md-raid5 shrinker bcache: dynamically allocate the md-bcache shrinker vmw_balloon: dynamically allocate the vmw-balloon shrinker virtio_balloon: dynamically allocate the virtio-balloon shrinker mbcache: dynamically allocate the mbcache shrinker ext4: dynamically allocate the ext4-es shrinker jbd2,ext4: dynamically allocate the jbd2-journal shrinker nfsd: dynamically allocate the nfsd-client shrinker nfsd: dynamically allocate the nfsd-reply shrinker xfs: dynamically allocate the xfs-buf shrinker xfs: dynamically allocate the xfs-inodegc shrinker xfs: dynamically allocate the xfs-qm shrinker zsmalloc: dynamically allocate the mm-zspool shrinker fs: super: dynamically allocate the s_shrink mm: shrinker: remove old APIs drm/ttm: introduce pool_shrink_rwsem mm: shrinker: add a secondary array for shrinker_info::{map, nr_deferred} mm: shrinker: make global slab shrink lockless mm: shrinker: make memcg slab shrink lockless mm: shrinker: hold write lock to reparent shrinker nr_deferred mm: shrinker: convert shrinker_rwsem to mutex arch/x86/kvm/mmu/mmu.c | 18 +- drivers/android/binder_alloc.c | 31 +- drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 30 +- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/msm/msm_drv.c | 4 +- drivers/gpu/drm/msm/msm_drv.h | 4 +- drivers/gpu/drm/msm/msm_gem_shrinker.c | 36 +- drivers/gpu/drm/panfrost/panfrost_device.h | 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 6 +- drivers/gpu/drm/panfrost/panfrost_gem.h | 2 +- .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 32 +- drivers/gpu/drm/ttm/ttm_pool.c | 38 +- drivers/md/bcache/bcache.h | 2 +- drivers/md/bcache/btree.c | 27 +- drivers/md/bcache/sysfs.c | 3 +- drivers/md/dm-bufio.c | 26 +- drivers/md/dm-cache-metadata.c | 2 +- drivers/md/dm-zoned-metadata.c | 28 +- drivers/md/raid5.c | 25 +- drivers/md/raid5.h | 2 +- drivers/misc/vmw_balloon.c | 38 +- drivers/virtio/virtio_balloon.c | 25 +- drivers/xen/xenbus/xenbus_probe_backend.c | 17 +- fs/btrfs/super.c | 2 +- fs/erofs/utils.c | 20 +- fs/ext4/ext4.h | 2 +- fs/ext4/extents_status.c | 22 +- fs/f2fs/super.c | 32 +- fs/gfs2/glock.c | 20 +- fs/gfs2/main.c | 6 +- fs/gfs2/quota.c | 26 +- fs/gfs2/quota.h | 3 +- fs/jbd2/journal.c | 27 +- fs/kernfs/mount.c | 2 +- fs/mbcache.c | 23 +- fs/nfs/nfs42xattr.c | 87 +- fs/nfs/super.c | 20 +- fs/nfsd/filecache.c | 22 +- fs/nfsd/netns.h | 4 +- fs/nfsd/nfs4state.c | 20 +- fs/nfsd/nfscache.c | 31 +- fs/proc/root.c | 2 +- fs/quota/dquot.c | 17 +- fs/super.c | 39 +- fs/ubifs/super.c | 22 +- fs/xfs/xfs_buf.c | 25 +- fs/xfs/xfs_buf.h | 2 +- fs/xfs/xfs_icache.c | 26 +- fs/xfs/xfs_mount.c | 4 +- fs/xfs/xfs_mount.h | 2 +- fs/xfs/xfs_qm.c | 26 +- fs/xfs/xfs_qm.h | 2 +- include/linux/fs.h | 2 +- include/linux/jbd2.h | 2 +- include/linux/memcontrol.h | 12 +- include/linux/shrinker.h | 54 +- kernel/rcu/tree.c | 21 +- kernel/rcu/tree_nocb.h | 19 +- mm/Makefile | 4 +- mm/huge_memory.c | 69 +- mm/shrinker.c | 772 ++++++++++++++++++ mm/shrinker_debug.c | 76 +- mm/vmscan.c | 701 ---------------- mm/workingset.c | 26 +- mm/zsmalloc.c | 28 +- net/sunrpc/auth.c | 19 +- 66 files changed, 1516 insertions(+), 1225 deletions(-) create mode 100644 mm/shrinker.c