On Tue, Jul 21, 2015 at 8:19 AM, Michal Hocko <mhocko@xxxxxxxxxx> wrote:
> On Thu 16-07-15 13:55:13, Davidlohr Bueso wrote:
>> The shrinker_rwsem is a global lock that protects the shrinker_list,
>> serializing a shrinking call with register/unregistering the shrinker
>> itself. As such, this lock is taken mostly for reading. In the unlikely
>> case that the the list is being modified, we simply return indicating
>> we want to iterate again. However, the only caller of shrink_slab()
>> that acknowledges this return is drop_slab_node(), so in practice, the
>> rest of the callers never try again.
>>
>> This patch proposes replacing the rwsem with an srcu aware list of
>> shrinkers, where (un)registering tasks use a spinlock. Upon shrinker
>> calls, the srcu read lock will guarantee the existence of the structure,
>> thus safely optimizing the common (read locked) case. These guarantees
>> also allow us to cleanup and simplify the code, getting rid of the
>> ugly trylock mechanism to retry the shrinker operation when the list
>> is concurrently being modified. As Michal pointed this is only worth
>> mentioning for unregister purposes.
>>
>> Signed-off-by: Davidlohr Bueso <dbueso@xxxxxxx>
>
> Looks reasonable to me. I am pretty sure that any performance
> improvements are close to 0 but the code looks better to me.
>
> Acked-by: Michal Hocko <mhocko@xxxxxxxx>
The version of this commit in linux-next currently fails on arm allnoconfig:
mm/built-in.o: In function `shrink_slab.part.66.constprop.73':
slub.c:(.text+0xce10): undefined reference to `__srcu_read_lock'
slub.c:(.text+0xd00c): undefined reference to `__srcu_read_unlock'
mm/built-in.o: In function `unregister_shrinker':
slub.c:(.text+0xd0f4): undefined reference to `synchronize_srcu'
mm/built-in.o:(.data+0x1c8): undefined reference to `process_srcu'
make: *** [vmlinux] Error 1
Bisect says:
dab937da82f9504fbac75592a1825614318d73e7 is the first bad commit
commit dab937da82f9504fbac75592a1825614318d73e7
Author: Davidlohr Bueso <dave@xxxxxxxxxxxx>
Date: Fri Jul 24 09:11:26 2015 +1000
mm: srcu-ify shrinkers
Paul.
--
>> ---
>> Changes from v1:
>> - Got rid of the trylock, per mhocko.
>>
>> fs/super.c | 8 ++++----
>> mm/vmscan.c | 39 ++++++++++++++++++---------------------
>> 2 files changed, 22 insertions(+), 25 deletions(-)
>>
>> diff --git a/fs/super.c b/fs/super.c
>> index b613723..c917817 100644
>> --- a/fs/super.c
>> +++ b/fs/super.c
>> @@ -49,8 +49,8 @@ static char *sb_writers_name[SB_FREEZE_LEVELS] = {
>> * One thing we have to be careful of with a per-sb shrinker is that we don't
>> * drop the last active reference to the superblock from within the shrinker.
>> * If that happens we could trigger unregistering the shrinker from within the
>> - * shrinker path and that leads to deadlock on the shrinker_rwsem. Hence we
>> - * take a passive reference to the superblock to avoid this from occurring.
>> + * shrinker path. Hence we take a passive reference to the superblock to avoid
>> + * this from occurring.
>> */
>> static unsigned long super_cache_scan(struct shrinker *shrink,
>> struct shrink_control *sc)
>> @@ -121,8 +121,8 @@ static unsigned long super_cache_count(struct shrinker *shrink,
>> * Don't call trylock_super as it is a potential
>> * scalability bottleneck. The counts could get updated
>> * between super_cache_count and super_cache_scan anyway.
>> - * Call to super_cache_count with shrinker_rwsem held
>> - * ensures the safety of call to list_lru_shrink_count() and
>> + * SRCU guarantees object validity across this call -- thus
>> + * it is safe to call list_lru_shrink_count() and
>> * s_op->nr_cached_objects().
>> */
>> if (sb->s_op && sb->s_op->nr_cached_objects)
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index c8d8282..fc820cf 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -36,7 +36,7 @@
>> #include <linux/cpuset.h>
>> #include <linux/compaction.h>
>> #include <linux/notifier.h>
>> -#include <linux/rwsem.h>
>> +#include <linux/srcu.h>
>> #include <linux/delay.h>
>> #include <linux/kthread.h>
>> #include <linux/freezer.h>
>> @@ -146,8 +146,9 @@ int vm_swappiness = 60;
>> */
>> unsigned long vm_total_pages;
>>
>> +DEFINE_STATIC_SRCU(shrinker_srcu);
>> static LIST_HEAD(shrinker_list);
>> -static DECLARE_RWSEM(shrinker_rwsem);
>> +static DEFINE_SPINLOCK(shrinker_list_lock);
>>
>> #ifdef CONFIG_MEMCG
>> static bool global_reclaim(struct scan_control *sc)
>> @@ -242,9 +243,9 @@ int register_shrinker(struct shrinker *shrinker)
>> if (!shrinker->nr_deferred)
>> return -ENOMEM;
>>
>> - down_write(&shrinker_rwsem);
>> - list_add_tail(&shrinker->list, &shrinker_list);
>> - up_write(&shrinker_rwsem);
>> + spin_lock(&shrinker_list_lock);
>> + list_add_tail_rcu(&shrinker->list, &shrinker_list);
>> + spin_unlock(&shrinker_list_lock);
>> return 0;
>> }
>> EXPORT_SYMBOL(register_shrinker);
>> @@ -254,9 +255,14 @@ EXPORT_SYMBOL(register_shrinker);
>> */
>> void unregister_shrinker(struct shrinker *shrinker)
>> {
>> - down_write(&shrinker_rwsem);
>> - list_del(&shrinker->list);
>> - up_write(&shrinker_rwsem);
>> + spin_lock(&shrinker_list_lock);
>> + list_del_rcu(&shrinker->list);
>> + spin_unlock(&shrinker_list_lock);
>> + /*
>> + * Before freeing nr_deferred, ensure all srcu
>> + * readers are done with their critical region.
>> + */
>> + synchronize_srcu(&shrinker_srcu);
>> kfree(shrinker->nr_deferred);
>> }
>> EXPORT_SYMBOL(unregister_shrinker);
>> @@ -408,6 +414,7 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
>> unsigned long nr_scanned,
>> unsigned long nr_eligible)
>> {
>> + int idx;
>> struct shrinker *shrinker;
>> unsigned long freed = 0;
>>
>> @@ -417,18 +424,9 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
>> if (nr_scanned == 0)
>> nr_scanned = SWAP_CLUSTER_MAX;
>>
>> - if (!down_read_trylock(&shrinker_rwsem)) {
>> - /*
>> - * If we would return 0, our callers would understand that we
>> - * have nothing else to shrink and give up trying. By returning
>> - * 1 we keep it going and assume we'll be able to shrink next
>> - * time.
>> - */
>> - freed = 1;
>> - goto out;
>> - }
>> + idx = srcu_read_lock(&shrinker_srcu);
>>
>> - list_for_each_entry(shrinker, &shrinker_list, list) {
>> + list_for_each_entry_rcu(shrinker, &shrinker_list, list) {
>> struct shrink_control sc = {
>> .gfp_mask = gfp_mask,
>> .nid = nid,
>> @@ -444,8 +442,7 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
>> freed += do_shrink_slab(&sc, shrinker, nr_scanned, nr_eligible);
>> }
>>
>> - up_read(&shrinker_rwsem);
>> -out:
>> + srcu_read_unlock(&shrinker_srcu, idx);
>> cond_resched();
>> return freed;
>> }
>> --
>> 2.1.4
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>
> --
> Michal Hocko
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-next" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
