| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2a90a68d-f495-c9c2-594c-c30d7c911d38@linux.alibaba.com>
Date: Tue, 20 Aug 2019 09:07:49 -0700
From: Yang Shi <yang.shi@...ux.alibaba.com>
To: Kirill Tkhai <ktkhai@...tuozzo.com>,
kirill.shutemov@...ux.intel.com, hannes@...xchg.org,
mhocko@...e.com, hughd@...gle.com, shakeelb@...gle.com,
rientjes@...gle.com, cai@....pw, akpm@...ux-foundation.org
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [v5 PATCH 3/4] mm: shrinker: make shrinker not depend on memcg
kmem
On 8/20/19 4:01 AM, Kirill Tkhai wrote:
> On 07.08.2019 05:17, Yang Shi wrote:
>> Currently shrinker is just allocated and can work when memcg kmem is
>> enabled. But, THP deferred split shrinker is not slab shrinker, it
>> doesn't make too much sense to have such shrinker depend on memcg kmem.
>> It should be able to reclaim THP even though memcg kmem is disabled.
>>
>> Introduce a new shrinker flag, SHRINKER_NONSLAB, for non-slab shrinker.
>> When memcg kmem is disabled, just such shrinkers can be called in
>> shrinking memcg slab.
>>
>> Cc: Kirill Tkhai <ktkhai@...tuozzo.com>
>> Cc: Johannes Weiner <hannes@...xchg.org>
>> Cc: Michal Hocko <mhocko@...e.com>
>> Cc: "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>
>> Cc: Hugh Dickins <hughd@...gle.com>
>> Cc: Shakeel Butt <shakeelb@...gle.com>
>> Cc: David Rientjes <rientjes@...gle.com>
>> Cc: Qian Cai <cai@....pw>
>> Acked-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
>> Signed-off-by: Yang Shi <yang.shi@...ux.alibaba.com>
> Looks OK for me. But some doubts about naming.
>
> SHRINKER_NONSLAB. There are a lot of shrinkers, which are not
> related to slab. For example, mmu_shrinker in arch/x86/kvm/mmu.c.
> Intuitively and without mm knowledge, I assume, I would be surprised
> why it's not masked as NONSLAB. Can we improve this in some way?
Actually, SHRINKER_NONSLAB just makes sense when the shrinker is also
MEMCG_AWARE for now.
I didn't think of a better name, any suggestion? I could add some
comment to explain this, non-MEMCG_AWARE shrinker should not care about
setting this flag even though it is non-slab.
And, once this patch is in Linus's tree, I will double check if there is
any MEMCG_AWARE non-slab shrinker although my quick search didn't show
others except inode/dcache and workingset node.
>
> The rest looks OK for me.
>
> Reviewed-by: Kirill Tkhai <ktkhai@...tuozzo.com>
>
>> ---
>> include/linux/memcontrol.h | 19 ++++++++-------
>> include/linux/shrinker.h | 3 ++-
>> mm/memcontrol.c | 9 +------
>> mm/vmscan.c | 60 ++++++++++++++++++++++++----------------------
>> 4 files changed, 45 insertions(+), 46 deletions(-)
>>
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index 44c4146..5771816 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -128,9 +128,8 @@ struct mem_cgroup_per_node {
>>
>> struct mem_cgroup_reclaim_iter iter[DEF_PRIORITY + 1];
>>
>> -#ifdef CONFIG_MEMCG_KMEM
>> struct memcg_shrinker_map __rcu *shrinker_map;
>> -#endif
>> +
>> struct rb_node tree_node; /* RB tree node */
>> unsigned long usage_in_excess;/* Set to the value by which */
>> /* the soft limit is exceeded*/
>> @@ -1253,6 +1252,11 @@ static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
>> } while ((memcg = parent_mem_cgroup(memcg)));
>> return false;
>> }
>> +
>> +extern int memcg_expand_shrinker_maps(int new_id);
>> +
>> +extern void memcg_set_shrinker_bit(struct mem_cgroup *memcg,
>> + int nid, int shrinker_id);
>> #else
>> #define mem_cgroup_sockets_enabled 0
>> static inline void mem_cgroup_sk_alloc(struct sock *sk) { };
>> @@ -1261,6 +1265,11 @@ static inline bool mem_cgroup_under_socket_pressure(struct mem_cgroup *memcg)
>> {
>> return false;
>> }
>> +
>> +static inline void memcg_set_shrinker_bit(struct mem_cgroup *memcg,
>> + int nid, int shrinker_id)
>> +{
>> +}
>> #endif
>>
>> struct kmem_cache *memcg_kmem_get_cache(struct kmem_cache *cachep);
>> @@ -1332,10 +1341,6 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
>> return memcg ? memcg->kmemcg_id : -1;
>> }
>>
>> -extern int memcg_expand_shrinker_maps(int new_id);
>> -
>> -extern void memcg_set_shrinker_bit(struct mem_cgroup *memcg,
>> - int nid, int shrinker_id);
>> #else
>>
>> static inline int memcg_kmem_charge(struct page *page, gfp_t gfp, int order)
>> @@ -1377,8 +1382,6 @@ static inline void memcg_put_cache_ids(void)
>> {
>> }
>>
>> -static inline void memcg_set_shrinker_bit(struct mem_cgroup *memcg,
>> - int nid, int shrinker_id) { }
>> #endif /* CONFIG_MEMCG_KMEM */
>>
>> #endif /* _LINUX_MEMCONTROL_H */
>> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
>> index 9443caf..9e112d6 100644
>> --- a/include/linux/shrinker.h
>> +++ b/include/linux/shrinker.h
>> @@ -69,7 +69,7 @@ struct shrinker {
>>
>> /* These are for internal use */
>> struct list_head list;
>> -#ifdef CONFIG_MEMCG_KMEM
>> +#ifdef CONFIG_MEMCG
>> /* ID in shrinker_idr */
>> int id;
>> #endif
>> @@ -81,6 +81,7 @@ struct shrinker {
>> /* Flags */
>> #define SHRINKER_NUMA_AWARE (1 << 0)
>> #define SHRINKER_MEMCG_AWARE (1 << 1)
>> +#define SHRINKER_NONSLAB (1 << 2)
>>
>> extern int prealloc_shrinker(struct shrinker *shrinker);
>> extern void register_shrinker_prepared(struct shrinker *shrinker);
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index cdbb7a8..d90ded1 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -313,6 +313,7 @@ void memcg_put_cache_ids(void)
>> EXPORT_SYMBOL(memcg_kmem_enabled_key);
>>
>> struct workqueue_struct *memcg_kmem_cache_wq;
>> +#endif
>>
>> static int memcg_shrinker_map_size;
>> static DEFINE_MUTEX(memcg_shrinker_map_mutex);
>> @@ -436,14 +437,6 @@ void memcg_set_shrinker_bit(struct mem_cgroup *memcg, int nid, int shrinker_id)
>> }
>> }
>>
>> -#else /* CONFIG_MEMCG_KMEM */
>> -static int memcg_alloc_shrinker_maps(struct mem_cgroup *memcg)
>> -{
>> - return 0;
>> -}
>> -static void memcg_free_shrinker_maps(struct mem_cgroup *memcg) { }
>> -#endif /* CONFIG_MEMCG_KMEM */
>> -
>> /**
>> * mem_cgroup_css_from_page - css of the memcg associated with a page
>> * @page: page of interest
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index b1b5e5f..093b76d 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -174,11 +174,22 @@ struct scan_control {
>> */
>> unsigned long vm_total_pages;
>>
>> +static void set_task_reclaim_state(struct task_struct *task,
>> + struct reclaim_state *rs)
>> +{
>> + /* Check for an overwrite */
>> + WARN_ON_ONCE(rs && task->reclaim_state);
>> +
>> + /* Check for the nulling of an already-nulled member */
>> + WARN_ON_ONCE(!rs && !task->reclaim_state);
>> +
>> + task->reclaim_state = rs;
>> +}
>> +
>> static LIST_HEAD(shrinker_list);
>> static DECLARE_RWSEM(shrinker_rwsem);
>>
>> -#ifdef CONFIG_MEMCG_KMEM
>> -
>> +#ifdef CONFIG_MEMCG
>> /*
>> * We allow subsystems to populate their shrinker-related
>> * LRU lists before register_shrinker_prepared() is called
>> @@ -230,30 +241,7 @@ static void unregister_memcg_shrinker(struct shrinker *shrinker)
>> idr_remove(&shrinker_idr, id);
>> up_write(&shrinker_rwsem);
>> }
>> -#else /* CONFIG_MEMCG_KMEM */
>> -static int prealloc_memcg_shrinker(struct shrinker *shrinker)
>> -{
>> - return 0;
>> -}
>>
>> -static void unregister_memcg_shrinker(struct shrinker *shrinker)
>> -{
>> -}
>> -#endif /* CONFIG_MEMCG_KMEM */
>> -
>> -static void set_task_reclaim_state(struct task_struct *task,
>> - struct reclaim_state *rs)
>> -{
>> - /* Check for an overwrite */
>> - WARN_ON_ONCE(rs && task->reclaim_state);
>> -
>> - /* Check for the nulling of an already-nulled member */
>> - WARN_ON_ONCE(!rs && !task->reclaim_state);
>> -
>> - task->reclaim_state = rs;
>> -}
>> -
>> -#ifdef CONFIG_MEMCG
>> static bool global_reclaim(struct scan_control *sc)
>> {
>> return !sc->target_mem_cgroup;
>> @@ -308,6 +296,15 @@ static bool memcg_congested(pg_data_t *pgdat,
>>
>> }
>> #else
>> +static int prealloc_memcg_shrinker(struct shrinker *shrinker)
>> +{
>> + return 0;
>> +}
>> +
>> +static void unregister_memcg_shrinker(struct shrinker *shrinker)
>> +{
>> +}
>> +
>> static bool global_reclaim(struct scan_control *sc)
>> {
>> return true;
>> @@ -594,7 +591,7 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
>> return freed;
>> }
>>
>> -#ifdef CONFIG_MEMCG_KMEM
>> +#ifdef CONFIG_MEMCG
>> static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
>> struct mem_cgroup *memcg, int priority)
>> {
>> @@ -602,7 +599,7 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
>> unsigned long ret, freed = 0;
>> int i;
>>
>> - if (!memcg_kmem_enabled() || !mem_cgroup_online(memcg))
>> + if (!mem_cgroup_online(memcg))
>> return 0;
>>
>> if (!down_read_trylock(&shrinker_rwsem))
>> @@ -628,6 +625,11 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
>> continue;
>> }
>>
>> + /* Call non-slab shrinkers even though kmem is disabled */
>> + if (!memcg_kmem_enabled() &&
>> + !(shrinker->flags & SHRINKER_NONSLAB))
>> + continue;
>> +
>> ret = do_shrink_slab(&sc, shrinker, priority);
>> if (ret == SHRINK_EMPTY) {
>> clear_bit(i, map->map);
>> @@ -664,13 +666,13 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
>> up_read(&shrinker_rwsem);
>> return freed;
>> }
>> -#else /* CONFIG_MEMCG_KMEM */
>> +#else /* CONFIG_MEMCG */
>> static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
>> struct mem_cgroup *memcg, int priority)
>> {
>> return 0;
>> }
>> -#endif /* CONFIG_MEMCG_KMEM */
>> +#endif /* CONFIG_MEMCG */
>>
>> /**
>> * shrink_slab - shrink slab caches
>>
Powered by blists - more mailing lists