Re: [PATCH v2 44/47] mm: shrinker: make global slab shrink lockless
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(>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(>refcount)) complete(>done); } void shrinker_free() { . refcount_dec(>refcount); I guess what you mean is shrinker_put(), because here may be the last refcount. Yes, I did. wait_for_completion(>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(>list); up_write(_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(_rwsem); - list_add_tail(>list, _list); - shrinker->flags |= SHRINKER_REGISTERED; + refcount_set(>refcount, 1); + + spin_lock(_lock); + list_add_tail_rcu(>list, _list); + spin_unlock(_lock); + shrinker_debugfs_add(shrinker); - up_write(_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(_rwsem); list_add_tail_rcu(>list, _list); shrinker->flags |= SHRINKER_REGISTERED; shrinker_debugfs_add(shrinker); up_write(_rwsem); /* * now the shrinker is fully set up, take the first * reference to it to indicate that lookup operations are
Re: [PATCH v2 44/47] mm: shrinker: make global slab shrink lockless
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(>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(>refcount)) > > complete(>done); > > } > > > > void shrinker_free() > > { > > . > > refcount_dec(>refcount); > > I guess what you mean is shrinker_put(), because here may be the last > refcount. Yes, I did. > > wait_for_completion(>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(>list); > > up_write(_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(_rwsem); > > > - list_add_tail(>list, _list); > > > - shrinker->flags |= SHRINKER_REGISTERED; > > > + refcount_set(>refcount, 1); > > > + > > > + spin_lock(_lock); > > > + list_add_tail_rcu(>list, _list); > > > + spin_unlock(_lock); > > > + > > > shrinker_debugfs_add(shrinker); > > > - up_write(_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(_rwsem); > > list_add_tail_rcu(>list, _list); > > shrinker->flags |= SHRINKER_REGISTERED; > > shrinker_debugfs_add(shrinker); > > up_write(_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(>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 > > > #include > > > #include > > > +#include > > > /*
Re: [PATCH v2 44/47] mm: shrinker: make global slab shrink lockless
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) ramfs735238 60.00 12.37363.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) ramfs746677 60.00 12.22367.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-ptikhomi...@virtuozzo.com/ [2]. https://lore.kernel.org/lkml/20230313112819.38938-1-zhengqi.a...@bytedance.com/ [3]. https://lore.kernel.org/lkml/202305230837.db2c233f-yujie@intel.com/ [4]. https://lore.kernel.org/all/20230609081518.3039120-1-qi.zh...@linux.dev/ [5]. https://lore.kernel.org/lkml/zijhou1d55d4h...@dread.disaster.area/ Signed-off-by: Qi Zheng --- 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 #include +#include #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
Re: [PATCH v2 44/47] mm: shrinker: make global slab shrink lockless
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) > ramfs735238 60.00 12.37363.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) > ramfs746677 60.00 12.22367.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-ptikhomi...@virtuozzo.com/ > [2]. > https://lore.kernel.org/lkml/20230313112819.38938-1-zhengqi.a...@bytedance.com/ > [3]. https://lore.kernel.org/lkml/202305230837.db2c233f-yujie@intel.com/ > [4]. https://lore.kernel.org/all/20230609081518.3039120-1-qi.zh...@linux.dev/ > [5]. https://lore.kernel.org/lkml/zijhou1d55d4h...@dread.disaster.area/ > > Signed-off-by: Qi Zheng > --- > 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 > #include > +#include > > #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; > +
[PATCH v2 44/47] mm: shrinker: make global slab shrink lockless
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) ramfs735238 60.00 12.37363.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) ramfs746677 60.00 12.22367.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-ptikhomi...@virtuozzo.com/ [2]. https://lore.kernel.org/lkml/20230313112819.38938-1-zhengqi.a...@bytedance.com/ [3]. https://lore.kernel.org/lkml/202305230837.db2c233f-yujie@intel.com/ [4]. https://lore.kernel.org/all/20230609081518.3039120-1-qi.zh...@linux.dev/ [5]. https://lore.kernel.org/lkml/zijhou1d55d4h...@dread.disaster.area/ Signed-off-by: Qi Zheng --- 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 #include +#include #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