* [PATCH v5 01/16] mm: list_lru: optimize memory consumption of arrays of per cgroup lists
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2022-01-07 0:05 ` Roman Gushchin
2021-12-20 8:56 ` [PATCH v5 02/16] mm: introduce kmem_cache_alloc_lru Muchun Song
` (14 subsequent siblings)
15 siblings, 1 reply; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
The list_lru uses an array (list_lru_memcg->lru) to store pointers
which point to the list_lru_one. And the array is per memcg per node.
Therefore, the size of the arrays will be 10K * number_of_node * 8 (
a pointer size on 64 bits system) when we run 10k containers in the
system. The memory consumption of the arrays becomes significant. The
more numa node, the more memory it consumes.
I have done a simple test, which creates 10K memcg and mount point
each in a two-node system. The memory consumption of the list_lru
will be 24464MB. After converting the array from per memcg per node
to per memcg, the memory consumption is going to be 21957MB. It is
reduces by 2.5GB. In our AMD servers with 8 numa nodes in those
sysuem, the memory consumption could be more significant. The savings
come from the list_lru_one heads, that it also simplifies the
alloc/dealloc path.
The new scheme looks like the following.
+----------+ mlrus +----------------+ mlru +----------------------+
| list_lru +---------->| list_lru_memcg +--------->| list_lru_per_memcg |
+----------+ +----------------+ +----------------------+
| list_lru_per_memcg |
+----------------------+
| ... |
+--------------+ node +----------------------+
| list_lru_one |<----------+ list_lru_per_memcg |
+--------------+ +----------------------+
| list_lru_one |
+--------------+
| ... |
+--------------+
| list_lru_one |
+--------------+
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
---
include/linux/list_lru.h | 17 ++--
mm/list_lru.c | 206 +++++++++++++++++------------------------------
2 files changed, 86 insertions(+), 137 deletions(-)
diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
index 1b5fceb565df..729a27b6ff53 100644
--- a/include/linux/list_lru.h
+++ b/include/linux/list_lru.h
@@ -31,10 +31,15 @@ struct list_lru_one {
long nr_items;
};
+struct list_lru_per_memcg {
+ /* array of per cgroup per node lists, indexed by node id */
+ struct list_lru_one node[0];
+};
+
struct list_lru_memcg {
- struct rcu_head rcu;
+ struct rcu_head rcu;
/* array of per cgroup lists, indexed by memcg_cache_id */
- struct list_lru_one *lru[];
+ struct list_lru_per_memcg *mlru[];
};
struct list_lru_node {
@@ -42,11 +47,7 @@ struct list_lru_node {
spinlock_t lock;
/* global list, used for the root cgroup in cgroup aware lrus */
struct list_lru_one lru;
-#ifdef CONFIG_MEMCG_KMEM
- /* for cgroup aware lrus points to per cgroup lists, otherwise NULL */
- struct list_lru_memcg __rcu *memcg_lrus;
-#endif
- long nr_items;
+ long nr_items;
} ____cacheline_aligned_in_smp;
struct list_lru {
@@ -55,6 +56,8 @@ struct list_lru {
struct list_head list;
int shrinker_id;
bool memcg_aware;
+ /* for cgroup aware lrus points to per cgroup lists, otherwise NULL */
+ struct list_lru_memcg __rcu *mlrus;
#endif
};
diff --git a/mm/list_lru.c b/mm/list_lru.c
index 0cd5e89ca063..7d1356241aa8 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -49,35 +49,37 @@ static int lru_shrinker_id(struct list_lru *lru)
}
static inline struct list_lru_one *
-list_lru_from_memcg_idx(struct list_lru_node *nlru, int idx)
+list_lru_from_memcg_idx(struct list_lru *lru, int nid, int idx)
{
- struct list_lru_memcg *memcg_lrus;
+ struct list_lru_memcg *mlrus;
+ struct list_lru_node *nlru = &lru->node[nid];
+
/*
* Either lock or RCU protects the array of per cgroup lists
- * from relocation (see memcg_update_list_lru_node).
+ * from relocation (see memcg_update_list_lru).
*/
- memcg_lrus = rcu_dereference_check(nlru->memcg_lrus,
- lockdep_is_held(&nlru->lock));
- if (memcg_lrus && idx >= 0)
- return memcg_lrus->lru[idx];
+ mlrus = rcu_dereference_check(lru->mlrus, lockdep_is_held(&nlru->lock));
+ if (mlrus && idx >= 0)
+ return &mlrus->mlru[idx]->node[nid];
return &nlru->lru;
}
static inline struct list_lru_one *
-list_lru_from_kmem(struct list_lru_node *nlru, void *ptr,
+list_lru_from_kmem(struct list_lru *lru, int nid, void *ptr,
struct mem_cgroup **memcg_ptr)
{
+ struct list_lru_node *nlru = &lru->node[nid];
struct list_lru_one *l = &nlru->lru;
struct mem_cgroup *memcg = NULL;
- if (!nlru->memcg_lrus)
+ if (!lru->mlrus)
goto out;
memcg = mem_cgroup_from_obj(ptr);
if (!memcg)
goto out;
- l = list_lru_from_memcg_idx(nlru, memcg_cache_id(memcg));
+ l = list_lru_from_memcg_idx(lru, nid, memcg_cache_id(memcg));
out:
if (memcg_ptr)
*memcg_ptr = memcg;
@@ -103,18 +105,18 @@ static inline bool list_lru_memcg_aware(struct list_lru *lru)
}
static inline struct list_lru_one *
-list_lru_from_memcg_idx(struct list_lru_node *nlru, int idx)
+list_lru_from_memcg_idx(struct list_lru *lru, int nid, int idx)
{
- return &nlru->lru;
+ return &lru->node[nid].lru;
}
static inline struct list_lru_one *
-list_lru_from_kmem(struct list_lru_node *nlru, void *ptr,
+list_lru_from_kmem(struct list_lru *lru, int nid, void *ptr,
struct mem_cgroup **memcg_ptr)
{
if (memcg_ptr)
*memcg_ptr = NULL;
- return &nlru->lru;
+ return &lru->node[nid].lru;
}
#endif /* CONFIG_MEMCG_KMEM */
@@ -127,7 +129,7 @@ bool list_lru_add(struct list_lru *lru, struct list_head *item)
spin_lock(&nlru->lock);
if (list_empty(item)) {
- l = list_lru_from_kmem(nlru, item, &memcg);
+ l = list_lru_from_kmem(lru, nid, item, &memcg);
list_add_tail(item, &l->list);
/* Set shrinker bit if the first element was added */
if (!l->nr_items++)
@@ -150,7 +152,7 @@ bool list_lru_del(struct list_lru *lru, struct list_head *item)
spin_lock(&nlru->lock);
if (!list_empty(item)) {
- l = list_lru_from_kmem(nlru, item, NULL);
+ l = list_lru_from_kmem(lru, nid, item, NULL);
list_del_init(item);
l->nr_items--;
nlru->nr_items--;
@@ -180,12 +182,11 @@ EXPORT_SYMBOL_GPL(list_lru_isolate_move);
unsigned long list_lru_count_one(struct list_lru *lru,
int nid, struct mem_cgroup *memcg)
{
- struct list_lru_node *nlru = &lru->node[nid];
struct list_lru_one *l;
long count;
rcu_read_lock();
- l = list_lru_from_memcg_idx(nlru, memcg_cache_id(memcg));
+ l = list_lru_from_memcg_idx(lru, nid, memcg_cache_id(memcg));
count = READ_ONCE(l->nr_items);
rcu_read_unlock();
@@ -206,16 +207,16 @@ unsigned long list_lru_count_node(struct list_lru *lru, int nid)
EXPORT_SYMBOL_GPL(list_lru_count_node);
static unsigned long
-__list_lru_walk_one(struct list_lru_node *nlru, int memcg_idx,
+__list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
list_lru_walk_cb isolate, void *cb_arg,
unsigned long *nr_to_walk)
{
-
+ struct list_lru_node *nlru = &lru->node[nid];
struct list_lru_one *l;
struct list_head *item, *n;
unsigned long isolated = 0;
- l = list_lru_from_memcg_idx(nlru, memcg_idx);
+ l = list_lru_from_memcg_idx(lru, nid, memcg_idx);
restart:
list_for_each_safe(item, n, &l->list) {
enum lru_status ret;
@@ -272,8 +273,8 @@ list_lru_walk_one(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
unsigned long ret;
spin_lock(&nlru->lock);
- ret = __list_lru_walk_one(nlru, memcg_cache_id(memcg), isolate, cb_arg,
- nr_to_walk);
+ ret = __list_lru_walk_one(lru, nid, memcg_cache_id(memcg), isolate,
+ cb_arg, nr_to_walk);
spin_unlock(&nlru->lock);
return ret;
}
@@ -288,8 +289,8 @@ list_lru_walk_one_irq(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
unsigned long ret;
spin_lock_irq(&nlru->lock);
- ret = __list_lru_walk_one(nlru, memcg_cache_id(memcg), isolate, cb_arg,
- nr_to_walk);
+ ret = __list_lru_walk_one(lru, nid, memcg_cache_id(memcg), isolate,
+ cb_arg, nr_to_walk);
spin_unlock_irq(&nlru->lock);
return ret;
}
@@ -308,7 +309,7 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
struct list_lru_node *nlru = &lru->node[nid];
spin_lock(&nlru->lock);
- isolated += __list_lru_walk_one(nlru, memcg_idx,
+ isolated += __list_lru_walk_one(lru, nid, memcg_idx,
isolate, cb_arg,
nr_to_walk);
spin_unlock(&nlru->lock);
@@ -328,166 +329,111 @@ static void init_one_lru(struct list_lru_one *l)
}
#ifdef CONFIG_MEMCG_KMEM
-static void __memcg_destroy_list_lru_node(struct list_lru_memcg *memcg_lrus,
- int begin, int end)
+static void memcg_destroy_list_lru_range(struct list_lru_memcg *mlrus,
+ int begin, int end)
{
int i;
for (i = begin; i < end; i++)
- kfree(memcg_lrus->lru[i]);
+ kfree(mlrus->mlru[i]);
}
-static int __memcg_init_list_lru_node(struct list_lru_memcg *memcg_lrus,
- int begin, int end)
+static int memcg_init_list_lru_range(struct list_lru_memcg *mlrus,
+ int begin, int end)
{
int i;
for (i = begin; i < end; i++) {
- struct list_lru_one *l;
+ int nid;
+ struct list_lru_per_memcg *mlru;
- l = kmalloc(sizeof(struct list_lru_one), GFP_KERNEL);
- if (!l)
+ mlru = kmalloc(struct_size(mlru, node, nr_node_ids), GFP_KERNEL);
+ if (!mlru)
goto fail;
- init_one_lru(l);
- memcg_lrus->lru[i] = l;
+ for_each_node(nid)
+ init_one_lru(&mlru->node[nid]);
+ mlrus->mlru[i] = mlru;
}
return 0;
fail:
- __memcg_destroy_list_lru_node(memcg_lrus, begin, i);
+ memcg_destroy_list_lru_range(mlrus, begin, i);
return -ENOMEM;
}
-static int memcg_init_list_lru_node(struct list_lru_node *nlru)
+static int memcg_init_list_lru(struct list_lru *lru, bool memcg_aware)
{
- struct list_lru_memcg *memcg_lrus;
+ struct list_lru_memcg *mlrus;
int size = memcg_nr_cache_ids;
- memcg_lrus = kvmalloc(struct_size(memcg_lrus, lru, size), GFP_KERNEL);
- if (!memcg_lrus)
+ lru->memcg_aware = memcg_aware;
+ if (!memcg_aware)
+ return 0;
+
+ mlrus = kvmalloc(struct_size(mlrus, mlru, size), GFP_KERNEL);
+ if (!mlrus)
return -ENOMEM;
- if (__memcg_init_list_lru_node(memcg_lrus, 0, size)) {
- kvfree(memcg_lrus);
+ if (memcg_init_list_lru_range(mlrus, 0, size)) {
+ kvfree(mlrus);
return -ENOMEM;
}
- RCU_INIT_POINTER(nlru->memcg_lrus, memcg_lrus);
+ RCU_INIT_POINTER(lru->mlrus, mlrus);
return 0;
}
-static void memcg_destroy_list_lru_node(struct list_lru_node *nlru)
+static void memcg_destroy_list_lru(struct list_lru *lru)
{
- struct list_lru_memcg *memcg_lrus;
+ struct list_lru_memcg *mlrus;
+
+ if (!list_lru_memcg_aware(lru))
+ return;
+
/*
* This is called when shrinker has already been unregistered,
* and nobody can use it. So, there is no need to use kvfree_rcu().
*/
- memcg_lrus = rcu_dereference_protected(nlru->memcg_lrus, true);
- __memcg_destroy_list_lru_node(memcg_lrus, 0, memcg_nr_cache_ids);
- kvfree(memcg_lrus);
+ mlrus = rcu_dereference_protected(lru->mlrus, true);
+ memcg_destroy_list_lru_range(mlrus, 0, memcg_nr_cache_ids);
+ kvfree(mlrus);
}
-static int memcg_update_list_lru_node(struct list_lru_node *nlru,
- int old_size, int new_size)
+static int memcg_update_list_lru(struct list_lru *lru, int old_size, int new_size)
{
struct list_lru_memcg *old, *new;
BUG_ON(old_size > new_size);
- old = rcu_dereference_protected(nlru->memcg_lrus,
+ old = rcu_dereference_protected(lru->mlrus,
lockdep_is_held(&list_lrus_mutex));
- new = kvmalloc(struct_size(new, lru, new_size), GFP_KERNEL);
+ new = kvmalloc(struct_size(new, mlru, new_size), GFP_KERNEL);
if (!new)
return -ENOMEM;
- if (__memcg_init_list_lru_node(new, old_size, new_size)) {
+ if (memcg_init_list_lru_range(new, old_size, new_size)) {
kvfree(new);
return -ENOMEM;
}
- memcpy(&new->lru, &old->lru, flex_array_size(new, lru, old_size));
- rcu_assign_pointer(nlru->memcg_lrus, new);
+ memcpy(&new->mlru, &old->mlru, flex_array_size(new, mlru, old_size));
+ rcu_assign_pointer(lru->mlrus, new);
kvfree_rcu(old, rcu);
return 0;
}
-static void memcg_cancel_update_list_lru_node(struct list_lru_node *nlru,
- int old_size, int new_size)
-{
- struct list_lru_memcg *memcg_lrus;
-
- memcg_lrus = rcu_dereference_protected(nlru->memcg_lrus,
- lockdep_is_held(&list_lrus_mutex));
- /* do not bother shrinking the array back to the old size, because we
- * cannot handle allocation failures here */
- __memcg_destroy_list_lru_node(memcg_lrus, old_size, new_size);
-}
-
-static int memcg_init_list_lru(struct list_lru *lru, bool memcg_aware)
-{
- int i;
-
- lru->memcg_aware = memcg_aware;
-
- if (!memcg_aware)
- return 0;
-
- for_each_node(i) {
- if (memcg_init_list_lru_node(&lru->node[i]))
- goto fail;
- }
- return 0;
-fail:
- for (i = i - 1; i >= 0; i--) {
- if (!lru->node[i].memcg_lrus)
- continue;
- memcg_destroy_list_lru_node(&lru->node[i]);
- }
- return -ENOMEM;
-}
-
-static void memcg_destroy_list_lru(struct list_lru *lru)
-{
- int i;
-
- if (!list_lru_memcg_aware(lru))
- return;
-
- for_each_node(i)
- memcg_destroy_list_lru_node(&lru->node[i]);
-}
-
-static int memcg_update_list_lru(struct list_lru *lru,
- int old_size, int new_size)
-{
- int i;
-
- for_each_node(i) {
- if (memcg_update_list_lru_node(&lru->node[i],
- old_size, new_size))
- goto fail;
- }
- return 0;
-fail:
- for (i = i - 1; i >= 0; i--) {
- if (!lru->node[i].memcg_lrus)
- continue;
-
- memcg_cancel_update_list_lru_node(&lru->node[i],
- old_size, new_size);
- }
- return -ENOMEM;
-}
-
static void memcg_cancel_update_list_lru(struct list_lru *lru,
int old_size, int new_size)
{
- int i;
+ struct list_lru_memcg *mlrus;
- for_each_node(i)
- memcg_cancel_update_list_lru_node(&lru->node[i],
- old_size, new_size);
+ mlrus = rcu_dereference_protected(lru->mlrus,
+ lockdep_is_held(&list_lrus_mutex));
+ /*
+ * Do not bother shrinking the array back to the old size, because we
+ * cannot handle allocation failures here.
+ */
+ memcg_destroy_list_lru_range(mlrus, old_size, new_size);
}
int memcg_update_all_list_lrus(int new_size)
@@ -524,8 +470,8 @@ static void memcg_drain_list_lru_node(struct list_lru *lru, int nid,
*/
spin_lock_irq(&nlru->lock);
- src = list_lru_from_memcg_idx(nlru, src_idx);
- dst = list_lru_from_memcg_idx(nlru, dst_idx);
+ src = list_lru_from_memcg_idx(lru, nid, src_idx);
+ dst = list_lru_from_memcg_idx(lru, nid, dst_idx);
list_splice_init(&src->list, &dst->list);
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v5 01/16] mm: list_lru: optimize memory consumption of arrays of per cgroup lists
2021-12-20 8:56 ` [PATCH v5 01/16] mm: list_lru: optimize memory consumption of arrays of per cgroup lists Muchun Song
@ 2022-01-07 0:05 ` Roman Gushchin
2022-01-09 4:49 ` Muchun Song
0 siblings, 1 reply; 45+ messages in thread
From: Roman Gushchin @ 2022-01-07 0:05 UTC (permalink / raw)
To: Muchun Song
Cc: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, shy828301,
alexs, richard.weiyang, david, trond.myklebust, anna.schumaker,
jaegeuk, chao, kari.argillander, linux-fsdevel, linux-kernel,
linux-mm, linux-nfs, zhengqi.arch, duanxiongchun, fam.zheng,
smuchun
On Mon, Dec 20, 2021 at 04:56:34PM +0800, Muchun Song wrote:
> The list_lru uses an array (list_lru_memcg->lru) to store pointers
> which point to the list_lru_one. And the array is per memcg per node.
> Therefore, the size of the arrays will be 10K * number_of_node * 8 (
> a pointer size on 64 bits system) when we run 10k containers in the
> system. The memory consumption of the arrays becomes significant. The
> more numa node, the more memory it consumes.
>
> I have done a simple test, which creates 10K memcg and mount point
> each in a two-node system. The memory consumption of the list_lru
> will be 24464MB. After converting the array from per memcg per node
> to per memcg, the memory consumption is going to be 21957MB. It is
> reduces by 2.5GB. In our AMD servers with 8 numa nodes in those
> sysuem, the memory consumption could be more significant. The savings
> come from the list_lru_one heads, that it also simplifies the
> alloc/dealloc path.
>
> The new scheme looks like the following.
>
> +----------+ mlrus +----------------+ mlru +----------------------+
> | list_lru +---------->| list_lru_memcg +--------->| list_lru_per_memcg |
> +----------+ +----------------+ +----------------------+
> | list_lru_per_memcg |
> +----------------------+
> | ... |
> +--------------+ node +----------------------+
> | list_lru_one |<----------+ list_lru_per_memcg |
> +--------------+ +----------------------+
> | list_lru_one |
> +--------------+
> | ... |
> +--------------+
> | list_lru_one |
> +--------------+
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
As much as I like the code changes (there is indeed a significant simplification!),
I don't like the commit message and title, because I wasn't able to understand
what the patch is doing and some parts look simply questionable. Overall it
sounds like you reduce the number of list_lru_one structures, which is not true.
How about something like this?
--
mm: list_lru: transpose the array of per-node per-memcg lru lists
The current scheme of maintaining per-node per-memcg lru lists looks like:
struct list_lru {
struct list_lru_node *node; (for each node)
struct list_lru_memcg *memcg_lrus;
struct list_lru_one *lru[]; (for each memcg)
}
By effectively transposing the two-dimension array of list_lru_one's structures
(per-node per-memcg => per-memcg per-node) it's possible to save some memory
and simplify alloc/dealloc paths. The new scheme looks like:
struct list_lru {
struct list_lru_memcg *mlrus;
struct list_lru_per_memcg *mlru[]; (for each memcg)
struct list_lru_one node[0]; (for each node)
}
Memory savings are coming from having fewer list_lru_memcg structures, which
contain an extra struct rcu_head to handle the destruction process.
--
But what worries me is that memory savings numbers you posted don't do up.
In theory we can save
16 (size of struct rcu_head) * 10000 (number of cgroups) * 2 (number of numa nodes) = 320k
per slab cache. Did you have a ton of mount points? Otherwise I don't understand
where these 2.5Gb are coming from.
Thanks!
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 01/16] mm: list_lru: optimize memory consumption of arrays of per cgroup lists
2022-01-07 0:05 ` Roman Gushchin
@ 2022-01-09 4:49 ` Muchun Song
2022-01-10 18:42 ` Roman Gushchin
0 siblings, 1 reply; 45+ messages in thread
From: Muchun Song @ 2022-01-09 4:49 UTC (permalink / raw)
To: Roman Gushchin
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Yang Shi, Alex Shi, Wei Yang,
Dave Chinner, trond.myklebust, anna.schumaker, jaegeuk, chao,
Kari Argillander, linux-fsdevel, LKML,
Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song
On Fri, Jan 7, 2022 at 8:05 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Mon, Dec 20, 2021 at 04:56:34PM +0800, Muchun Song wrote:
> > The list_lru uses an array (list_lru_memcg->lru) to store pointers
> > which point to the list_lru_one. And the array is per memcg per node.
> > Therefore, the size of the arrays will be 10K * number_of_node * 8 (
> > a pointer size on 64 bits system) when we run 10k containers in the
> > system. The memory consumption of the arrays becomes significant. The
> > more numa node, the more memory it consumes.
> >
> > I have done a simple test, which creates 10K memcg and mount point
> > each in a two-node system. The memory consumption of the list_lru
> > will be 24464MB. After converting the array from per memcg per node
> > to per memcg, the memory consumption is going to be 21957MB. It is
> > reduces by 2.5GB. In our AMD servers with 8 numa nodes in those
> > sysuem, the memory consumption could be more significant. The savings
> > come from the list_lru_one heads, that it also simplifies the
> > alloc/dealloc path.
> >
> > The new scheme looks like the following.
> >
> > +----------+ mlrus +----------------+ mlru +----------------------+
> > | list_lru +---------->| list_lru_memcg +--------->| list_lru_per_memcg |
> > +----------+ +----------------+ +----------------------+
> > | list_lru_per_memcg |
> > +----------------------+
> > | ... |
> > +--------------+ node +----------------------+
> > | list_lru_one |<----------+ list_lru_per_memcg |
> > +--------------+ +----------------------+
> > | list_lru_one |
> > +--------------+
> > | ... |
> > +--------------+
> > | list_lru_one |
> > +--------------+
> >
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>
> As much as I like the code changes (there is indeed a significant simplification!),
> I don't like the commit message and title, because I wasn't able to understand
> what the patch is doing and some parts look simply questionable. Overall it
> sounds like you reduce the number of list_lru_one structures, which is not true.
>
> How about something like this?
>
> --
> mm: list_lru: transpose the array of per-node per-memcg lru lists
>
> The current scheme of maintaining per-node per-memcg lru lists looks like:
> struct list_lru {
> struct list_lru_node *node; (for each node)
> struct list_lru_memcg *memcg_lrus;
> struct list_lru_one *lru[]; (for each memcg)
> }
>
> By effectively transposing the two-dimension array of list_lru_one's structures
> (per-node per-memcg => per-memcg per-node) it's possible to save some memory
> and simplify alloc/dealloc paths. The new scheme looks like:
> struct list_lru {
> struct list_lru_memcg *mlrus;
> struct list_lru_per_memcg *mlru[]; (for each memcg)
> struct list_lru_one node[0]; (for each node)
> }
>
> Memory savings are coming from having fewer list_lru_memcg structures, which
> contain an extra struct rcu_head to handle the destruction process.
My bad English. Actually, the saving is coming from not only 'struct rcu_head'
but also some pointer arrays used to store the pointer to 'struct list_lru_one'.
The array is per node and its size is 8 (a pointer) * num_memcgs. So the total
size of the arrays is 8 * num_nodes * memcg_nr_cache_ids. After this patch,
the size becomes 8 * memcg_nr_cache_ids. So the saving is
8 * (num_nodes - 1) * memcg_nr_cache_ids.
> --
>
> But what worries me is that memory savings numbers you posted don't do up.
> In theory we can save
> 16 (size of struct rcu_head) * 10000 (number of cgroups) * 2 (number of numa nodes) = 320k
> per slab cache. Did you have a ton of mount points? Otherwise I don't understand
> where these 2.5Gb are coming from.
memcg_nr_cache_ids is 12286 when creating 10k memcgs. So the saving
of arrays of one list_lru is 8 * 1 (number of numa nodes - 1) * 12286 = 96k.
There will be 2 * 10k list_lru when mounting 10k points. So the total
saving is 96k * 2 * 10k = 1920 M.
Thanks Roman.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 01/16] mm: list_lru: optimize memory consumption of arrays of per cgroup lists
2022-01-09 4:49 ` Muchun Song
@ 2022-01-10 18:42 ` Roman Gushchin
2022-01-11 3:19 ` Muchun Song
0 siblings, 1 reply; 45+ messages in thread
From: Roman Gushchin @ 2022-01-10 18:42 UTC (permalink / raw)
To: Muchun Song
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Yang Shi, Alex Shi, Wei Yang,
Dave Chinner, trond.myklebust, anna.schumaker, jaegeuk, chao,
Kari Argillander, linux-fsdevel, LKML,
Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song
On Sun, Jan 09, 2022 at 12:49:56PM +0800, Muchun Song wrote:
> On Fri, Jan 7, 2022 at 8:05 AM Roman Gushchin <guro@fb.com> wrote:
> >
> > On Mon, Dec 20, 2021 at 04:56:34PM +0800, Muchun Song wrote:
> > > The list_lru uses an array (list_lru_memcg->lru) to store pointers
> > > which point to the list_lru_one. And the array is per memcg per node.
> > > Therefore, the size of the arrays will be 10K * number_of_node * 8 (
> > > a pointer size on 64 bits system) when we run 10k containers in the
> > > system. The memory consumption of the arrays becomes significant. The
> > > more numa node, the more memory it consumes.
> > >
> > > I have done a simple test, which creates 10K memcg and mount point
> > > each in a two-node system. The memory consumption of the list_lru
> > > will be 24464MB. After converting the array from per memcg per node
> > > to per memcg, the memory consumption is going to be 21957MB. It is
> > > reduces by 2.5GB. In our AMD servers with 8 numa nodes in those
> > > sysuem, the memory consumption could be more significant. The savings
> > > come from the list_lru_one heads, that it also simplifies the
> > > alloc/dealloc path.
> > >
> > > The new scheme looks like the following.
> > >
> > > +----------+ mlrus +----------------+ mlru +----------------------+
> > > | list_lru +---------->| list_lru_memcg +--------->| list_lru_per_memcg |
> > > +----------+ +----------------+ +----------------------+
> > > | list_lru_per_memcg |
> > > +----------------------+
> > > | ... |
> > > +--------------+ node +----------------------+
> > > | list_lru_one |<----------+ list_lru_per_memcg |
> > > +--------------+ +----------------------+
> > > | list_lru_one |
> > > +--------------+
> > > | ... |
> > > +--------------+
> > > | list_lru_one |
> > > +--------------+
> > >
> > > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> >
> > As much as I like the code changes (there is indeed a significant simplification!),
> > I don't like the commit message and title, because I wasn't able to understand
> > what the patch is doing and some parts look simply questionable. Overall it
> > sounds like you reduce the number of list_lru_one structures, which is not true.
> >
> > How about something like this?
> >
> > --
> > mm: list_lru: transpose the array of per-node per-memcg lru lists
> >
> > The current scheme of maintaining per-node per-memcg lru lists looks like:
> > struct list_lru {
> > struct list_lru_node *node; (for each node)
> > struct list_lru_memcg *memcg_lrus;
> > struct list_lru_one *lru[]; (for each memcg)
> > }
> >
> > By effectively transposing the two-dimension array of list_lru_one's structures
> > (per-node per-memcg => per-memcg per-node) it's possible to save some memory
> > and simplify alloc/dealloc paths. The new scheme looks like:
> > struct list_lru {
> > struct list_lru_memcg *mlrus;
> > struct list_lru_per_memcg *mlru[]; (for each memcg)
> > struct list_lru_one node[0]; (for each node)
> > }
> >
> > Memory savings are coming from having fewer list_lru_memcg structures, which
> > contain an extra struct rcu_head to handle the destruction process.
>
> My bad English. Actually, the saving is coming from not only 'struct rcu_head'
> but also some pointer arrays used to store the pointer to 'struct list_lru_one'.
> The array is per node and its size is 8 (a pointer) * num_memcgs.
Nice! Please, add this to the commit log.
> So the total
> size of the arrays is 8 * num_nodes * memcg_nr_cache_ids. After this patch,
> the size becomes 8 * memcg_nr_cache_ids. So the saving is
>
> 8 * (num_nodes - 1) * memcg_nr_cache_ids.
>
> > --
> >
> > But what worries me is that memory savings numbers you posted don't do up.
> > In theory we can save
> > 16 (size of struct rcu_head) * 10000 (number of cgroups) * 2 (number of numa nodes) = 320k
> > per slab cache. Did you have a ton of mount points? Otherwise I don't understand
> > where these 2.5Gb are coming from.
>
> memcg_nr_cache_ids is 12286 when creating 10k memcgs. So the saving
> of arrays of one list_lru is 8 * 1 (number of numa nodes - 1) * 12286 = 96k.
> There will be 2 * 10k list_lru when mounting 10k points. So the total
> saving is 96k * 2 * 10k = 1920 M.
So, there are 10k cgroups _and_ 10k mountpoints. Please, make it obvious from
the commit log. Most users don't have that many mount points (and likely cgroups),
so they shouldn't expect Gb's in savings.
Thanks!
PS I hope to review the rest of the patchset till the end of this week.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 01/16] mm: list_lru: optimize memory consumption of arrays of per cgroup lists
2022-01-10 18:42 ` Roman Gushchin
@ 2022-01-11 3:19 ` Muchun Song
0 siblings, 0 replies; 45+ messages in thread
From: Muchun Song @ 2022-01-11 3:19 UTC (permalink / raw)
To: Roman Gushchin
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Yang Shi, Alex Shi, Wei Yang,
Dave Chinner, trond.myklebust, anna.schumaker, jaegeuk, chao,
Kari Argillander, linux-fsdevel, LKML,
Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song
On Tue, Jan 11, 2022 at 2:42 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Sun, Jan 09, 2022 at 12:49:56PM +0800, Muchun Song wrote:
> > On Fri, Jan 7, 2022 at 8:05 AM Roman Gushchin <guro@fb.com> wrote:
> > >
> > > On Mon, Dec 20, 2021 at 04:56:34PM +0800, Muchun Song wrote:
> > > > The list_lru uses an array (list_lru_memcg->lru) to store pointers
> > > > which point to the list_lru_one. And the array is per memcg per node.
> > > > Therefore, the size of the arrays will be 10K * number_of_node * 8 (
> > > > a pointer size on 64 bits system) when we run 10k containers in the
> > > > system. The memory consumption of the arrays becomes significant. The
> > > > more numa node, the more memory it consumes.
> > > >
> > > > I have done a simple test, which creates 10K memcg and mount point
> > > > each in a two-node system. The memory consumption of the list_lru
> > > > will be 24464MB. After converting the array from per memcg per node
> > > > to per memcg, the memory consumption is going to be 21957MB. It is
> > > > reduces by 2.5GB. In our AMD servers with 8 numa nodes in those
> > > > sysuem, the memory consumption could be more significant. The savings
> > > > come from the list_lru_one heads, that it also simplifies the
> > > > alloc/dealloc path.
> > > >
> > > > The new scheme looks like the following.
> > > >
> > > > +----------+ mlrus +----------------+ mlru +----------------------+
> > > > | list_lru +---------->| list_lru_memcg +--------->| list_lru_per_memcg |
> > > > +----------+ +----------------+ +----------------------+
> > > > | list_lru_per_memcg |
> > > > +----------------------+
> > > > | ... |
> > > > +--------------+ node +----------------------+
> > > > | list_lru_one |<----------+ list_lru_per_memcg |
> > > > +--------------+ +----------------------+
> > > > | list_lru_one |
> > > > +--------------+
> > > > | ... |
> > > > +--------------+
> > > > | list_lru_one |
> > > > +--------------+
> > > >
> > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > > > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> > >
> > > As much as I like the code changes (there is indeed a significant simplification!),
> > > I don't like the commit message and title, because I wasn't able to understand
> > > what the patch is doing and some parts look simply questionable. Overall it
> > > sounds like you reduce the number of list_lru_one structures, which is not true.
> > >
> > > How about something like this?
> > >
> > > --
> > > mm: list_lru: transpose the array of per-node per-memcg lru lists
> > >
> > > The current scheme of maintaining per-node per-memcg lru lists looks like:
> > > struct list_lru {
> > > struct list_lru_node *node; (for each node)
> > > struct list_lru_memcg *memcg_lrus;
> > > struct list_lru_one *lru[]; (for each memcg)
> > > }
> > >
> > > By effectively transposing the two-dimension array of list_lru_one's structures
> > > (per-node per-memcg => per-memcg per-node) it's possible to save some memory
> > > and simplify alloc/dealloc paths. The new scheme looks like:
> > > struct list_lru {
> > > struct list_lru_memcg *mlrus;
> > > struct list_lru_per_memcg *mlru[]; (for each memcg)
> > > struct list_lru_one node[0]; (for each node)
> > > }
> > >
> > > Memory savings are coming from having fewer list_lru_memcg structures, which
> > > contain an extra struct rcu_head to handle the destruction process.
> >
> > My bad English. Actually, the saving is coming from not only 'struct rcu_head'
> > but also some pointer arrays used to store the pointer to 'struct list_lru_one'.
> > The array is per node and its size is 8 (a pointer) * num_memcgs.
>
> Nice! Please, add this to the commit log.
Will do.
>
> > So the total
> > size of the arrays is 8 * num_nodes * memcg_nr_cache_ids. After this patch,
> > the size becomes 8 * memcg_nr_cache_ids. So the saving is
> >
> > 8 * (num_nodes - 1) * memcg_nr_cache_ids.
> >
> > > --
> > >
> > > But what worries me is that memory savings numbers you posted don't do up.
> > > In theory we can save
> > > 16 (size of struct rcu_head) * 10000 (number of cgroups) * 2 (number of numa nodes) = 320k
> > > per slab cache. Did you have a ton of mount points? Otherwise I don't understand
> > > where these 2.5Gb are coming from.
> >
> > memcg_nr_cache_ids is 12286 when creating 10k memcgs. So the saving
> > of arrays of one list_lru is 8 * 1 (number of numa nodes - 1) * 12286 = 96k.
> > There will be 2 * 10k list_lru when mounting 10k points. So the total
> > saving is 96k * 2 * 10k = 1920 M.
>
> So, there are 10k cgroups _and_ 10k mountpoints. Please, make it obvious from
> the commit log. Most users don't have that many mount points (and likely cgroups),
> so they shouldn't expect Gb's in savings.
I'll add those infos into the commit log.
>
> Thanks!
>
> PS I hope to review the rest of the patchset till the end of this week.
Thanks Roman.
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v5 02/16] mm: introduce kmem_cache_alloc_lru
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
2021-12-20 8:56 ` [PATCH v5 01/16] mm: list_lru: optimize memory consumption of arrays of per cgroup lists Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2022-01-07 3:04 ` Roman Gushchin
2021-12-20 8:56 ` [PATCH v5 03/16] fs: introduce alloc_inode_sb() to allocate filesystems specific inode Muchun Song
` (13 subsequent siblings)
15 siblings, 1 reply; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
We currently allocate scope for every memcg to be able to tracked on
every superblock instantiated in the system, regardless of whether
that superblock is even accessible to that memcg.
These huge memcg counts come from container hosts where memcgs are
confined to just a small subset of the total number of superblocks
that instantiated at any given point in time.
For these systems with huge container counts, list_lru does not need
the capability of tracking every memcg on every superblock. What it
comes down to is that adding the memcg to the list_lru at the first
insert. So introduce kmem_cache_alloc_lru to allocate objects and its
list_lru. In the later patch, we will convert all inode and dentry
allocation from kmem_cache_alloc to kmem_cache_alloc_lru.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
include/linux/list_lru.h | 4 ++
include/linux/memcontrol.h | 14 ++++++
include/linux/slab.h | 3 ++
mm/list_lru.c | 104 +++++++++++++++++++++++++++++++++++++++++----
mm/memcontrol.c | 14 ------
mm/slab.c | 39 +++++++++++------
mm/slab.h | 25 +++++++++--
mm/slob.c | 6 +++
mm/slub.c | 42 ++++++++++++------
9 files changed, 198 insertions(+), 53 deletions(-)
diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
index 729a27b6ff53..ab912c49334f 100644
--- a/include/linux/list_lru.h
+++ b/include/linux/list_lru.h
@@ -56,6 +56,8 @@ struct list_lru {
struct list_head list;
int shrinker_id;
bool memcg_aware;
+ /* protects ->mlrus->mlru[i] */
+ spinlock_t lock;
/* for cgroup aware lrus points to per cgroup lists, otherwise NULL */
struct list_lru_memcg __rcu *mlrus;
#endif
@@ -72,6 +74,8 @@ int __list_lru_init(struct list_lru *lru, bool memcg_aware,
#define list_lru_init_memcg(lru, shrinker) \
__list_lru_init((lru), true, NULL, shrinker)
+int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
+ gfp_t gfp);
int memcg_update_all_list_lrus(int num_memcgs);
void memcg_drain_all_list_lrus(int src_idx, struct mem_cgroup *dst_memcg);
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 0c5c403f4be6..561ba47760db 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -520,6 +520,20 @@ static inline struct mem_cgroup *page_memcg_check(struct page *page)
return (struct mem_cgroup *)(memcg_data & ~MEMCG_DATA_FLAGS_MASK);
}
+static inline struct mem_cgroup *get_mem_cgroup_from_objcg(struct obj_cgroup *objcg)
+{
+ struct mem_cgroup *memcg;
+
+ rcu_read_lock();
+retry:
+ memcg = obj_cgroup_memcg(objcg);
+ if (unlikely(!css_tryget(&memcg->css)))
+ goto retry;
+ rcu_read_unlock();
+
+ return memcg;
+}
+
#ifdef CONFIG_MEMCG_KMEM
/*
* folio_memcg_kmem - Check if the folio has the memcg_kmem flag set.
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 181045148b06..eccbd21d3753 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -135,6 +135,7 @@
#include <linux/kasan.h>
+struct list_lru;
struct mem_cgroup;
/*
* struct kmem_cache related prototypes
@@ -425,6 +426,8 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __alloc_size(1);
void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_slab_alignment __malloc;
+void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
+ gfp_t gfpflags) __assume_slab_alignment __malloc;
void kmem_cache_free(struct kmem_cache *s, void *objp);
/*
diff --git a/mm/list_lru.c b/mm/list_lru.c
index 7d1356241aa8..bffa80527723 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -13,6 +13,7 @@
#include <linux/mutex.h>
#include <linux/memcontrol.h>
#include "slab.h"
+#include "internal.h"
#ifdef CONFIG_MEMCG_KMEM
static LIST_HEAD(memcg_list_lrus);
@@ -338,22 +339,30 @@ static void memcg_destroy_list_lru_range(struct list_lru_memcg *mlrus,
kfree(mlrus->mlru[i]);
}
+static struct list_lru_per_memcg *memcg_init_list_lru_one(gfp_t gfp)
+{
+ int nid;
+ struct list_lru_per_memcg *mlru;
+
+ mlru = kmalloc(struct_size(mlru, node, nr_node_ids), gfp);
+ if (!mlru)
+ return NULL;
+
+ for_each_node(nid)
+ init_one_lru(&mlru->node[nid]);
+
+ return mlru;
+}
+
static int memcg_init_list_lru_range(struct list_lru_memcg *mlrus,
int begin, int end)
{
int i;
for (i = begin; i < end; i++) {
- int nid;
- struct list_lru_per_memcg *mlru;
-
- mlru = kmalloc(struct_size(mlru, node, nr_node_ids), GFP_KERNEL);
- if (!mlru)
+ mlrus->mlru[i] = memcg_init_list_lru_one(GFP_KERNEL);
+ if (!mlrus->mlru[i])
goto fail;
-
- for_each_node(nid)
- init_one_lru(&mlru->node[nid]);
- mlrus->mlru[i] = mlru;
}
return 0;
fail:
@@ -370,6 +379,8 @@ static int memcg_init_list_lru(struct list_lru *lru, bool memcg_aware)
if (!memcg_aware)
return 0;
+ spin_lock_init(&lru->lock);
+
mlrus = kvmalloc(struct_size(mlrus, mlru, size), GFP_KERNEL);
if (!mlrus)
return -ENOMEM;
@@ -416,8 +427,11 @@ static int memcg_update_list_lru(struct list_lru *lru, int old_size, int new_siz
return -ENOMEM;
}
+ spin_lock_irq(&lru->lock);
memcpy(&new->mlru, &old->mlru, flex_array_size(new, mlru, old_size));
rcu_assign_pointer(lru->mlrus, new);
+ spin_unlock_irq(&lru->lock);
+
kvfree_rcu(old, rcu);
return 0;
}
@@ -502,6 +516,78 @@ void memcg_drain_all_list_lrus(int src_idx, struct mem_cgroup *dst_memcg)
memcg_drain_list_lru(lru, src_idx, dst_memcg);
mutex_unlock(&list_lrus_mutex);
}
+
+static bool memcg_list_lru_allocated(struct mem_cgroup *memcg,
+ struct list_lru *lru)
+{
+ bool allocated;
+ int idx;
+
+ idx = memcg->kmemcg_id;
+ if (unlikely(idx < 0))
+ return true;
+
+ rcu_read_lock();
+ allocated = !!rcu_dereference(lru->mlrus)->mlru[idx];
+ rcu_read_unlock();
+
+ return allocated;
+}
+
+int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
+ gfp_t gfp)
+{
+ int i;
+ unsigned long flags;
+ struct list_lru_memcg *mlrus;
+ struct list_lru_memcg_table {
+ struct list_lru_per_memcg *mlru;
+ struct mem_cgroup *memcg;
+ } *table;
+
+ if (!list_lru_memcg_aware(lru) || memcg_list_lru_allocated(memcg, lru))
+ return 0;
+
+ gfp &= GFP_RECLAIM_MASK;
+ table = kmalloc_array(memcg->css.cgroup->level, sizeof(*table), gfp);
+ if (!table)
+ return -ENOMEM;
+
+ /*
+ * Because the list_lru can be reparented to the parent cgroup's
+ * list_lru, we should make sure that this cgroup and all its
+ * ancestors have allocated list_lru_per_memcg.
+ */
+ for (i = 0; memcg; memcg = parent_mem_cgroup(memcg), i++) {
+ if (memcg_list_lru_allocated(memcg, lru))
+ break;
+
+ table[i].memcg = memcg;
+ table[i].mlru = memcg_init_list_lru_one(gfp);
+ if (!table[i].mlru) {
+ while (i--)
+ kfree(table[i].mlru);
+ kfree(table);
+ return -ENOMEM;
+ }
+ }
+
+ spin_lock_irqsave(&lru->lock, flags);
+ mlrus = rcu_dereference_protected(lru->mlrus, true);
+ while (i--) {
+ int index = table[i].memcg->kmemcg_id;
+
+ if (mlrus->mlru[index])
+ kfree(table[i].mlru);
+ else
+ mlrus->mlru[index] = table[i].mlru;
+ }
+ spin_unlock_irqrestore(&lru->lock, flags);
+
+ kfree(table);
+
+ return 0;
+}
#else
static int memcg_init_list_lru(struct list_lru *lru, bool memcg_aware)
{
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6863a834ed42..d505b43d5f3b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2794,20 +2794,6 @@ static void commit_charge(struct folio *folio, struct mem_cgroup *memcg)
folio->memcg_data = (unsigned long)memcg;
}
-static struct mem_cgroup *get_mem_cgroup_from_objcg(struct obj_cgroup *objcg)
-{
- struct mem_cgroup *memcg;
-
- rcu_read_lock();
-retry:
- memcg = obj_cgroup_memcg(objcg);
- if (unlikely(!css_tryget(&memcg->css)))
- goto retry;
- rcu_read_unlock();
-
- return memcg;
-}
-
#ifdef CONFIG_MEMCG_KMEM
/*
* The allocated objcg pointers array is not accounted directly.
diff --git a/mm/slab.c b/mm/slab.c
index ca4822f6b2b6..9f9e5593f67e 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3219,7 +3219,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_
bool init = false;
flags &= gfp_allowed_mask;
- cachep = slab_pre_alloc_hook(cachep, &objcg, 1, flags);
+ cachep = slab_pre_alloc_hook(cachep, NULL, &objcg, 1, flags);
if (unlikely(!cachep))
return NULL;
@@ -3295,7 +3295,8 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
#endif /* CONFIG_NUMA */
static __always_inline void *
-slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
+slab_alloc(struct kmem_cache *cachep, struct list_lru *lru, gfp_t flags,
+ size_t orig_size, unsigned long caller)
{
unsigned long save_flags;
void *objp;
@@ -3303,7 +3304,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned lo
bool init = false;
flags &= gfp_allowed_mask;
- cachep = slab_pre_alloc_hook(cachep, &objcg, 1, flags);
+ cachep = slab_pre_alloc_hook(cachep, lru, &objcg, 1, flags);
if (unlikely(!cachep))
return NULL;
@@ -3492,6 +3493,18 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
__free_one(ac, objp);
}
+static __always_inline
+void *__kmem_cache_alloc_lru(struct kmem_cache *cachep, struct list_lru *lru,
+ gfp_t flags)
+{
+ void *ret = slab_alloc(cachep, lru, flags, cachep->object_size, _RET_IP_);
+
+ trace_kmem_cache_alloc(_RET_IP_, ret,
+ cachep->object_size, cachep->size, flags);
+
+ return ret;
+}
+
/**
* kmem_cache_alloc - Allocate an object
* @cachep: The cache to allocate from.
@@ -3504,15 +3517,17 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
*/
void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
{
- void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);
-
- trace_kmem_cache_alloc(_RET_IP_, ret,
- cachep->object_size, cachep->size, flags);
-
- return ret;
+ return __kmem_cache_alloc_lru(cachep, NULL, flags);
}
EXPORT_SYMBOL(kmem_cache_alloc);
+void *kmem_cache_alloc_lru(struct kmem_cache *cachep, struct list_lru *lru,
+ gfp_t flags)
+{
+ return __kmem_cache_alloc_lru(cachep, lru, flags);
+}
+EXPORT_SYMBOL(kmem_cache_alloc_lru);
+
static __always_inline void
cache_alloc_debugcheck_after_bulk(struct kmem_cache *s, gfp_t flags,
size_t size, void **p, unsigned long caller)
@@ -3529,7 +3544,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
size_t i;
struct obj_cgroup *objcg = NULL;
- s = slab_pre_alloc_hook(s, &objcg, size, flags);
+ s = slab_pre_alloc_hook(s, NULL, &objcg, size, flags);
if (!s)
return 0;
@@ -3570,7 +3585,7 @@ kmem_cache_alloc_trace(struct kmem_cache *cachep, gfp_t flags, size_t size)
{
void *ret;
- ret = slab_alloc(cachep, flags, size, _RET_IP_);
+ ret = slab_alloc(cachep, NULL, flags, size, _RET_IP_);
ret = kasan_kmalloc(cachep, ret, size, flags);
trace_kmalloc(_RET_IP_, ret,
@@ -3697,7 +3712,7 @@ static __always_inline void *__do_kmalloc(size_t size, gfp_t flags,
cachep = kmalloc_slab(size, flags);
if (unlikely(ZERO_OR_NULL_PTR(cachep)))
return cachep;
- ret = slab_alloc(cachep, flags, size, caller);
+ ret = slab_alloc(cachep, NULL, flags, size, caller);
ret = kasan_kmalloc(cachep, ret, size, flags);
trace_kmalloc(caller, ret,
diff --git a/mm/slab.h b/mm/slab.h
index 56ad7eea3ddf..5768252aad1c 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -46,6 +46,7 @@ struct kmem_cache {
#include <linux/kmemleak.h>
#include <linux/random.h>
#include <linux/sched/mm.h>
+#include <linux/list_lru.h>
/*
* State of the slab allocator.
@@ -269,6 +270,7 @@ static inline size_t obj_full_size(struct kmem_cache *s)
* Returns false if the allocation should fail.
*/
static inline bool memcg_slab_pre_alloc_hook(struct kmem_cache *s,
+ struct list_lru *lru,
struct obj_cgroup **objcgp,
size_t objects, gfp_t flags)
{
@@ -284,13 +286,26 @@ static inline bool memcg_slab_pre_alloc_hook(struct kmem_cache *s,
if (!objcg)
return true;
- if (obj_cgroup_charge(objcg, flags, objects * obj_full_size(s))) {
- obj_cgroup_put(objcg);
- return false;
+ if (lru) {
+ int ret;
+ struct mem_cgroup *memcg;
+
+ memcg = get_mem_cgroup_from_objcg(objcg);
+ ret = memcg_list_lru_alloc(memcg, lru, flags);
+ css_put(&memcg->css);
+
+ if (ret)
+ goto out;
}
+ if (obj_cgroup_charge(objcg, flags, objects * obj_full_size(s)))
+ goto out;
+
*objcgp = objcg;
return true;
+out:
+ obj_cgroup_put(objcg);
+ return false;
}
static inline void memcg_slab_post_alloc_hook(struct kmem_cache *s,
@@ -386,6 +401,7 @@ static inline void memcg_free_page_obj_cgroups(struct page *page)
}
static inline bool memcg_slab_pre_alloc_hook(struct kmem_cache *s,
+ struct list_lru *lru,
struct obj_cgroup **objcgp,
size_t objects, gfp_t flags)
{
@@ -484,6 +500,7 @@ static inline size_t slab_ksize(const struct kmem_cache *s)
}
static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s,
+ struct list_lru *lru,
struct obj_cgroup **objcgp,
size_t size, gfp_t flags)
{
@@ -494,7 +511,7 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s,
if (should_failslab(s, flags))
return NULL;
- if (!memcg_slab_pre_alloc_hook(s, objcgp, size, flags))
+ if (!memcg_slab_pre_alloc_hook(s, lru, objcgp, size, flags))
return NULL;
return s;
diff --git a/mm/slob.c b/mm/slob.c
index 03deee1e6a94..fd9d7c24921f 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -633,6 +633,12 @@ void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
}
EXPORT_SYMBOL(kmem_cache_alloc);
+
+void *kmem_cache_alloc_lru(struct kmem_cache *cachep, struct list_lru *lru, gfp_t flags)
+{
+ return slob_alloc_node(cachep, flags, NUMA_NO_NODE);
+}
+EXPORT_SYMBOL(kmem_cache_alloc_lru);
#ifdef CONFIG_NUMA
void *__kmalloc_node(size_t size, gfp_t gfp, int node)
{
diff --git a/mm/slub.c b/mm/slub.c
index a8626825a829..6d9f05d13c0e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3135,7 +3135,7 @@ static __always_inline void maybe_wipe_obj_freeptr(struct kmem_cache *s,
*
* Otherwise we can simply pick the next object from the lockless free list.
*/
-static __always_inline void *slab_alloc_node(struct kmem_cache *s,
+static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_lru *lru,
gfp_t gfpflags, int node, unsigned long addr, size_t orig_size)
{
void *object;
@@ -3145,7 +3145,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
struct obj_cgroup *objcg = NULL;
bool init = false;
- s = slab_pre_alloc_hook(s, &objcg, 1, gfpflags);
+ s = slab_pre_alloc_hook(s, lru, &objcg, 1, gfpflags);
if (!s)
return NULL;
@@ -3236,27 +3236,41 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
return object;
}
-static __always_inline void *slab_alloc(struct kmem_cache *s,
+static __always_inline void *slab_alloc(struct kmem_cache *s, struct list_lru *lru,
gfp_t gfpflags, unsigned long addr, size_t orig_size)
{
- return slab_alloc_node(s, gfpflags, NUMA_NO_NODE, addr, orig_size);
+ return slab_alloc_node(s, lru, gfpflags, NUMA_NO_NODE, addr, orig_size);
}
-void *kmem_cache_alloc(struct kmem_cache *s, gfp_t gfpflags)
+static __always_inline
+void *__kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
+ gfp_t gfpflags)
{
- void *ret = slab_alloc(s, gfpflags, _RET_IP_, s->object_size);
+ void *ret = slab_alloc(s, lru, gfpflags, _RET_IP_, s->object_size);
trace_kmem_cache_alloc(_RET_IP_, ret, s->object_size,
s->size, gfpflags);
return ret;
}
+
+void *kmem_cache_alloc(struct kmem_cache *s, gfp_t gfpflags)
+{
+ return __kmem_cache_alloc_lru(s, NULL, gfpflags);
+}
EXPORT_SYMBOL(kmem_cache_alloc);
+void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
+ gfp_t gfpflags)
+{
+ return __kmem_cache_alloc_lru(s, lru, gfpflags);
+}
+EXPORT_SYMBOL(kmem_cache_alloc_lru);
+
#ifdef CONFIG_TRACING
void *kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t gfpflags, size_t size)
{
- void *ret = slab_alloc(s, gfpflags, _RET_IP_, size);
+ void *ret = slab_alloc(s, NULL, gfpflags, _RET_IP_, size);
trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags);
ret = kasan_kmalloc(s, ret, size, gfpflags);
return ret;
@@ -3267,7 +3281,7 @@ EXPORT_SYMBOL(kmem_cache_alloc_trace);
#ifdef CONFIG_NUMA
void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t gfpflags, int node)
{
- void *ret = slab_alloc_node(s, gfpflags, node, _RET_IP_, s->object_size);
+ void *ret = slab_alloc_node(s, NULL, gfpflags, node, _RET_IP_, s->object_size);
trace_kmem_cache_alloc_node(_RET_IP_, ret,
s->object_size, s->size, gfpflags, node);
@@ -3281,7 +3295,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
gfp_t gfpflags,
int node, size_t size)
{
- void *ret = slab_alloc_node(s, gfpflags, node, _RET_IP_, size);
+ void *ret = slab_alloc_node(s, NULL, gfpflags, node, _RET_IP_, size);
trace_kmalloc_node(_RET_IP_, ret,
size, s->size, gfpflags, node);
@@ -3667,7 +3681,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
struct obj_cgroup *objcg = NULL;
/* memcg and kmem_cache debug support */
- s = slab_pre_alloc_hook(s, &objcg, size, flags);
+ s = slab_pre_alloc_hook(s, NULL, &objcg, size, flags);
if (unlikely(!s))
return false;
/*
@@ -4416,7 +4430,7 @@ void *__kmalloc(size_t size, gfp_t flags)
if (unlikely(ZERO_OR_NULL_PTR(s)))
return s;
- ret = slab_alloc(s, flags, _RET_IP_, size);
+ ret = slab_alloc(s, NULL, flags, _RET_IP_, size);
trace_kmalloc(_RET_IP_, ret, size, s->size, flags);
@@ -4464,7 +4478,7 @@ void *__kmalloc_node(size_t size, gfp_t flags, int node)
if (unlikely(ZERO_OR_NULL_PTR(s)))
return s;
- ret = slab_alloc_node(s, flags, node, _RET_IP_, size);
+ ret = slab_alloc_node(s, NULL, flags, node, _RET_IP_, size);
trace_kmalloc_node(_RET_IP_, ret, size, s->size, flags, node);
@@ -4922,7 +4936,7 @@ void *__kmalloc_track_caller(size_t size, gfp_t gfpflags, unsigned long caller)
if (unlikely(ZERO_OR_NULL_PTR(s)))
return s;
- ret = slab_alloc(s, gfpflags, caller, size);
+ ret = slab_alloc(s, NULL, gfpflags, caller, size);
/* Honor the call site pointer we received. */
trace_kmalloc(caller, ret, size, s->size, gfpflags);
@@ -4953,7 +4967,7 @@ void *__kmalloc_node_track_caller(size_t size, gfp_t gfpflags,
if (unlikely(ZERO_OR_NULL_PTR(s)))
return s;
- ret = slab_alloc_node(s, gfpflags, node, caller, size);
+ ret = slab_alloc_node(s, NULL, gfpflags, node, caller, size);
/* Honor the call site pointer we received. */
trace_kmalloc_node(caller, ret, size, s->size, gfpflags, node);
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v5 02/16] mm: introduce kmem_cache_alloc_lru
2021-12-20 8:56 ` [PATCH v5 02/16] mm: introduce kmem_cache_alloc_lru Muchun Song
@ 2022-01-07 3:04 ` Roman Gushchin
2022-01-09 6:21 ` Muchun Song
0 siblings, 1 reply; 45+ messages in thread
From: Roman Gushchin @ 2022-01-07 3:04 UTC (permalink / raw)
To: Muchun Song
Cc: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, shy828301,
alexs, richard.weiyang, david, trond.myklebust, anna.schumaker,
jaegeuk, chao, kari.argillander, linux-fsdevel, linux-kernel,
linux-mm, linux-nfs, zhengqi.arch, duanxiongchun, fam.zheng,
smuchun, cl, penberg, rientjes, vbabka
On Mon, Dec 20, 2021 at 04:56:35PM +0800, Muchun Song wrote:
> We currently allocate scope for every memcg to be able to tracked on
> every superblock instantiated in the system, regardless of whether
> that superblock is even accessible to that memcg.
>
> These huge memcg counts come from container hosts where memcgs are
> confined to just a small subset of the total number of superblocks
> that instantiated at any given point in time.
>
> For these systems with huge container counts, list_lru does not need
> the capability of tracking every memcg on every superblock. What it
> comes down to is that adding the memcg to the list_lru at the first
> insert. So introduce kmem_cache_alloc_lru to allocate objects and its
> list_lru. In the later patch, we will convert all inode and dentry
> allocation from kmem_cache_alloc to kmem_cache_alloc_lru.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
> include/linux/list_lru.h | 4 ++
> include/linux/memcontrol.h | 14 ++++++
> include/linux/slab.h | 3 ++
> mm/list_lru.c | 104 +++++++++++++++++++++++++++++++++++++++++----
> mm/memcontrol.c | 14 ------
> mm/slab.c | 39 +++++++++++------
> mm/slab.h | 25 +++++++++--
> mm/slob.c | 6 +++
> mm/slub.c | 42 ++++++++++++------
> 9 files changed, 198 insertions(+), 53 deletions(-)
>
> diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
> index 729a27b6ff53..ab912c49334f 100644
> --- a/include/linux/list_lru.h
> +++ b/include/linux/list_lru.h
> @@ -56,6 +56,8 @@ struct list_lru {
> struct list_head list;
> int shrinker_id;
> bool memcg_aware;
> + /* protects ->mlrus->mlru[i] */
> + spinlock_t lock;
> /* for cgroup aware lrus points to per cgroup lists, otherwise NULL */
> struct list_lru_memcg __rcu *mlrus;
> #endif
> @@ -72,6 +74,8 @@ int __list_lru_init(struct list_lru *lru, bool memcg_aware,
> #define list_lru_init_memcg(lru, shrinker) \
> __list_lru_init((lru), true, NULL, shrinker)
>
> +int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
> + gfp_t gfp);
> int memcg_update_all_list_lrus(int num_memcgs);
> void memcg_drain_all_list_lrus(int src_idx, struct mem_cgroup *dst_memcg);
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 0c5c403f4be6..561ba47760db 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -520,6 +520,20 @@ static inline struct mem_cgroup *page_memcg_check(struct page *page)
> return (struct mem_cgroup *)(memcg_data & ~MEMCG_DATA_FLAGS_MASK);
> }
>
> +static inline struct mem_cgroup *get_mem_cgroup_from_objcg(struct obj_cgroup *objcg)
> +{
> + struct mem_cgroup *memcg;
> +
> + rcu_read_lock();
> +retry:
> + memcg = obj_cgroup_memcg(objcg);
> + if (unlikely(!css_tryget(&memcg->css)))
> + goto retry;
> + rcu_read_unlock();
> +
> + return memcg;
> +}
> +
> #ifdef CONFIG_MEMCG_KMEM
> /*
> * folio_memcg_kmem - Check if the folio has the memcg_kmem flag set.
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 181045148b06..eccbd21d3753 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -135,6 +135,7 @@
>
> #include <linux/kasan.h>
>
> +struct list_lru;
> struct mem_cgroup;
> /*
> * struct kmem_cache related prototypes
> @@ -425,6 +426,8 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
>
> void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __alloc_size(1);
> void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_slab_alignment __malloc;
> +void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
> + gfp_t gfpflags) __assume_slab_alignment __malloc;
I'm not a big fan of this patch: I don't see why preparing the lru
infrastructure has to be integrated that deep into the slab code.
Why can't kmem_cache_alloc_lru() be a simple wrapper like (pseudo-code):
void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
gfp_t gfpflags) {
if (necessarily)
prepare_lru_infra();
return kmem_cache_alloc();
}
In the current form the patch breaks the API layering. Maybe it's strictly
necessarily, but we should have a __very__ strong reason for this.
Thanks!
cc Slab maintainers
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 02/16] mm: introduce kmem_cache_alloc_lru
2022-01-07 3:04 ` Roman Gushchin
@ 2022-01-09 6:21 ` Muchun Song
2022-01-10 18:47 ` Roman Gushchin
0 siblings, 1 reply; 45+ messages in thread
From: Muchun Song @ 2022-01-09 6:21 UTC (permalink / raw)
To: Roman Gushchin
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Yang Shi, Alex Shi, Wei Yang,
Dave Chinner, trond.myklebust, anna.schumaker, jaegeuk, chao,
Kari Argillander, linux-fsdevel, LKML,
Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song, Christoph Lameter,
Pekka Enberg, David Rientjes, Vlastimil Babka
On Fri, Jan 7, 2022 at 11:05 AM Roman Gushchin <guro@fb.com> wrote:
>
[...]
> > /*
> > * struct kmem_cache related prototypes
> > @@ -425,6 +426,8 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
> >
> > void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __alloc_size(1);
> > void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_slab_alignment __malloc;
> > +void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
> > + gfp_t gfpflags) __assume_slab_alignment __malloc;
>
> I'm not a big fan of this patch: I don't see why preparing the lru
> infrastructure has to be integrated that deep into the slab code.
>
> Why can't kmem_cache_alloc_lru() be a simple wrapper like (pseudo-code):
> void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
> gfp_t gfpflags) {
> if (necessarily)
> prepare_lru_infra();
> return kmem_cache_alloc();
> }
Hi Roman,
Actually, it can. But there is going to be some redundant code similar
like memcg_slab_pre_alloc_hook() does to detect the necessity of
prepare_lru_infra() in the new scheme of kmem_cache_alloc_lru().
I just want to reduce the redundant overhead.
Thanks.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 02/16] mm: introduce kmem_cache_alloc_lru
2022-01-09 6:21 ` Muchun Song
@ 2022-01-10 18:47 ` Roman Gushchin
2022-01-11 15:41 ` Vlastimil Babka
0 siblings, 1 reply; 45+ messages in thread
From: Roman Gushchin @ 2022-01-10 18:47 UTC (permalink / raw)
To: Muchun Song, Vlastimil Babka
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Yang Shi, Alex Shi, Wei Yang,
Dave Chinner, trond.myklebust, anna.schumaker, jaegeuk, chao,
Kari Argillander, linux-fsdevel, LKML,
Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song, Christoph Lameter,
Pekka Enberg, David Rientjes, Vlastimil Babka
On Sun, Jan 09, 2022 at 02:21:22PM +0800, Muchun Song wrote:
> On Fri, Jan 7, 2022 at 11:05 AM Roman Gushchin <guro@fb.com> wrote:
> >
> [...]
> > > /*
> > > * struct kmem_cache related prototypes
> > > @@ -425,6 +426,8 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
> > >
> > > void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __alloc_size(1);
> > > void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_slab_alignment __malloc;
> > > +void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
> > > + gfp_t gfpflags) __assume_slab_alignment __malloc;
> >
> > I'm not a big fan of this patch: I don't see why preparing the lru
> > infrastructure has to be integrated that deep into the slab code.
> >
> > Why can't kmem_cache_alloc_lru() be a simple wrapper like (pseudo-code):
> > void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
> > gfp_t gfpflags) {
> > if (necessarily)
> > prepare_lru_infra();
> > return kmem_cache_alloc();
> > }
>
> Hi Roman,
>
> Actually, it can. But there is going to be some redundant code similar
> like memcg_slab_pre_alloc_hook() does to detect the necessity of
> prepare_lru_infra() in the new scheme of kmem_cache_alloc_lru().
> I just want to reduce the redundant overhead.
Is this about getting a memcg pointer?
I doubt it's a good reason to make changes all over the slab code.
Another option to consider adding a new gfp flag.
Vlastimil, what do you think?
Thanks!
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 02/16] mm: introduce kmem_cache_alloc_lru
2022-01-10 18:47 ` Roman Gushchin
@ 2022-01-11 15:41 ` Vlastimil Babka
2022-01-11 17:54 ` Roman Gushchin
0 siblings, 1 reply; 45+ messages in thread
From: Vlastimil Babka @ 2022-01-11 15:41 UTC (permalink / raw)
To: Roman Gushchin, Muchun Song
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Yang Shi, Alex Shi, Wei Yang,
Dave Chinner, trond.myklebust, anna.schumaker, jaegeuk, chao,
Kari Argillander, linux-fsdevel, LKML,
Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song, Christoph Lameter,
Pekka Enberg, David Rientjes
On 1/10/22 19:47, Roman Gushchin wrote:
> On Sun, Jan 09, 2022 at 02:21:22PM +0800, Muchun Song wrote:
>> On Fri, Jan 7, 2022 at 11:05 AM Roman Gushchin <guro@fb.com> wrote:
>> >
>> [...]
>> > > /*
>> > > * struct kmem_cache related prototypes
>> > > @@ -425,6 +426,8 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
>> > >
>> > > void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __alloc_size(1);
>> > > void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_slab_alignment __malloc;
>> > > +void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
>> > > + gfp_t gfpflags) __assume_slab_alignment __malloc;
>> >
>> > I'm not a big fan of this patch: I don't see why preparing the lru
>> > infrastructure has to be integrated that deep into the slab code.
>> >
>> > Why can't kmem_cache_alloc_lru() be a simple wrapper like (pseudo-code):
>> > void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
>> > gfp_t gfpflags) {
>> > if (necessarily)
>> > prepare_lru_infra();
>> > return kmem_cache_alloc();
>> > }
>>
>> Hi Roman,
>>
>> Actually, it can. But there is going to be some redundant code similar
>> like memcg_slab_pre_alloc_hook() does to detect the necessity of
>> prepare_lru_infra() in the new scheme of kmem_cache_alloc_lru().
>> I just want to reduce the redundant overhead.
>
> Is this about getting a memcg pointer?
> I doubt it's a good reason to make changes all over the slab code.
> Another option to consider adding a new gfp flag.
I'm not sure how a flag would help as it seems we really need to pass a
specific list_lru pointer and work with that. I was thinking if there was
only one list_lru per class of object it could be part of struct kmem_cache,
but investigating kmem_cache_alloc_lru() callers I see lru parameters:
- &nfs4_xattr_cache_lru - this is fixed
- xas->xa_lru potentially not fixed, although the only caller of
xas_set_lru() passes &shadow_nodes so effectively fixed
- &sb->s_dentry_lru - dynamic, boo
> Vlastimil, what do you think?
Memcg code is already quite intertwined with slab code, for better or worse,
so I guess the extra lru parameter in a bunch of inline functions won't
change much. I don't immediately see a better solution.
> Thanks!
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 02/16] mm: introduce kmem_cache_alloc_lru
2022-01-11 15:41 ` Vlastimil Babka
@ 2022-01-11 17:54 ` Roman Gushchin
0 siblings, 0 replies; 45+ messages in thread
From: Roman Gushchin @ 2022-01-11 17:54 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Muchun Song, Matthew Wilcox, Andrew Morton, Johannes Weiner,
Michal Hocko, Vladimir Davydov, Shakeel Butt, Yang Shi, Alex Shi,
Wei Yang, Dave Chinner, trond.myklebust, anna.schumaker, jaegeuk,
chao, Kari Argillander, linux-fsdevel, LKML,
Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song, Christoph Lameter,
Pekka Enberg, David Rientjes
On Tue, Jan 11, 2022 at 04:41:29PM +0100, Vlastimil Babka wrote:
> On 1/10/22 19:47, Roman Gushchin wrote:
> > On Sun, Jan 09, 2022 at 02:21:22PM +0800, Muchun Song wrote:
> >> On Fri, Jan 7, 2022 at 11:05 AM Roman Gushchin <guro@fb.com> wrote:
> >> >
> >> [...]
> >> > > /*
> >> > > * struct kmem_cache related prototypes
> >> > > @@ -425,6 +426,8 @@ static __always_inline unsigned int __kmalloc_index(size_t size,
> >> > >
> >> > > void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __alloc_size(1);
> >> > > void *kmem_cache_alloc(struct kmem_cache *s, gfp_t flags) __assume_slab_alignment __malloc;
> >> > > +void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
> >> > > + gfp_t gfpflags) __assume_slab_alignment __malloc;
> >> >
> >> > I'm not a big fan of this patch: I don't see why preparing the lru
> >> > infrastructure has to be integrated that deep into the slab code.
> >> >
> >> > Why can't kmem_cache_alloc_lru() be a simple wrapper like (pseudo-code):
> >> > void *kmem_cache_alloc_lru(struct kmem_cache *s, struct list_lru *lru,
> >> > gfp_t gfpflags) {
> >> > if (necessarily)
> >> > prepare_lru_infra();
> >> > return kmem_cache_alloc();
> >> > }
> >>
> >> Hi Roman,
> >>
> >> Actually, it can. But there is going to be some redundant code similar
> >> like memcg_slab_pre_alloc_hook() does to detect the necessity of
> >> prepare_lru_infra() in the new scheme of kmem_cache_alloc_lru().
> >> I just want to reduce the redundant overhead.
> >
> > Is this about getting a memcg pointer?
> > I doubt it's a good reason to make changes all over the slab code.
> > Another option to consider adding a new gfp flag.
>
> I'm not sure how a flag would help as it seems we really need to pass a
> specific list_lru pointer and work with that. I was thinking if there was
> only one list_lru per class of object it could be part of struct kmem_cache,
> but investigating kmem_cache_alloc_lru() callers I see lru parameters:
>
> - &nfs4_xattr_cache_lru - this is fixed
> - xas->xa_lru potentially not fixed, although the only caller of
> xas_set_lru() passes &shadow_nodes so effectively fixed
> - &sb->s_dentry_lru - dynamic, boo
Indeed.
>
> > Vlastimil, what do you think?
>
> Memcg code is already quite intertwined with slab code, for better or worse,
> so I guess the extra lru parameter in a bunch of inline functions won't
> change much. I don't immediately see a better solution.
Ok then. Thanks for taking a look!
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v5 03/16] fs: introduce alloc_inode_sb() to allocate filesystems specific inode
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
2021-12-20 8:56 ` [PATCH v5 01/16] mm: list_lru: optimize memory consumption of arrays of per cgroup lists Muchun Song
2021-12-20 8:56 ` [PATCH v5 02/16] mm: introduce kmem_cache_alloc_lru Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2022-01-11 18:55 ` Roman Gushchin
2021-12-20 8:56 ` [PATCH v5 04/16] fs: allocate inode by using alloc_inode_sb() Muchun Song
` (12 subsequent siblings)
15 siblings, 1 reply; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
The allocated inode cache is supposed to be added to its memcg list_lru
which should be allocated as well in advance. That can be done by
kmem_cache_alloc_lru() which allocates object and list_lru. The file
systems is main user of it. So introduce alloc_inode_sb() to allocate
file system specific inodes and set up the inode reclaim context
properly. The file system is supposed to use alloc_inode_sb() to
allocate inodes. In the later patches, we will convert all users to the
new API.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
Documentation/filesystems/porting.rst | 5 +++++
fs/inode.c | 2 +-
include/linux/fs.h | 11 +++++++++++
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
index bf19fd6b86e7..c9c157d7b7bb 100644
--- a/Documentation/filesystems/porting.rst
+++ b/Documentation/filesystems/porting.rst
@@ -45,6 +45,11 @@ typically between calling iget_locked() and unlocking the inode.
At some point that will become mandatory.
+**mandatory**
+
+The foo_inode_info should always be allocated through alloc_inode_sb() rather
+than kmem_cache_alloc() or kmalloc() related.
+
---
**mandatory**
diff --git a/fs/inode.c b/fs/inode.c
index 6b80a51129d5..dcb1e6cad201 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -234,7 +234,7 @@ static struct inode *alloc_inode(struct super_block *sb)
if (ops->alloc_inode)
inode = ops->alloc_inode(sb);
else
- inode = kmem_cache_alloc(inode_cachep, GFP_KERNEL);
+ inode = alloc_inode_sb(sb, inode_cachep, GFP_KERNEL);
if (!inode)
return NULL;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bbf812ce89a8..4592f00ec5e7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -41,6 +41,7 @@
#include <linux/stddef.h>
#include <linux/mount.h>
#include <linux/cred.h>
+#include <linux/slab.h>
#include <asm/byteorder.h>
#include <uapi/linux/fs.h>
@@ -3178,6 +3179,16 @@ extern void free_inode_nonrcu(struct inode *inode);
extern int should_remove_suid(struct dentry *);
extern int file_remove_privs(struct file *);
+/*
+ * This must be used for allocating filesystems specific inodes to set
+ * up the inode reclaim context correctly.
+ */
+static inline void *
+alloc_inode_sb(struct super_block *sb, struct kmem_cache *cache, gfp_t gfp)
+{
+ return kmem_cache_alloc_lru(cache, &sb->s_inode_lru, gfp);
+}
+
extern void __insert_inode_hash(struct inode *, unsigned long hashval);
static inline void insert_inode_hash(struct inode *inode)
{
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v5 03/16] fs: introduce alloc_inode_sb() to allocate filesystems specific inode
2021-12-20 8:56 ` [PATCH v5 03/16] fs: introduce alloc_inode_sb() to allocate filesystems specific inode Muchun Song
@ 2022-01-11 18:55 ` Roman Gushchin
2022-01-12 2:54 ` Muchun Song
0 siblings, 1 reply; 45+ messages in thread
From: Roman Gushchin @ 2022-01-11 18:55 UTC (permalink / raw)
To: Muchun Song
Cc: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, shy828301,
alexs, richard.weiyang, david, trond.myklebust, anna.schumaker,
jaegeuk, chao, kari.argillander, linux-fsdevel, linux-kernel,
linux-mm, linux-nfs, zhengqi.arch, duanxiongchun, fam.zheng,
smuchun
On Mon, Dec 20, 2021 at 04:56:36PM +0800, Muchun Song wrote:
> The allocated inode cache is supposed to be added to its memcg list_lru
> which should be allocated as well in advance. That can be done by
> kmem_cache_alloc_lru() which allocates object and list_lru. The file
> systems is main user of it. So introduce alloc_inode_sb() to allocate
> file system specific inodes and set up the inode reclaim context
> properly. The file system is supposed to use alloc_inode_sb() to
> allocate inodes. In the later patches, we will convert all users to the
> new API.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
> Documentation/filesystems/porting.rst | 5 +++++
> fs/inode.c | 2 +-
> include/linux/fs.h | 11 +++++++++++
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index bf19fd6b86e7..c9c157d7b7bb 100644
> --- a/Documentation/filesystems/porting.rst
> +++ b/Documentation/filesystems/porting.rst
> @@ -45,6 +45,11 @@ typically between calling iget_locked() and unlocking the inode.
>
> At some point that will become mandatory.
>
> +**mandatory**
> +
> +The foo_inode_info should always be allocated through alloc_inode_sb() rather
> +than kmem_cache_alloc() or kmalloc() related.
I'd add a couple of words on why it has to be allocated this way.
> +
> ---
Reviewed-by: Roman Gushchin <guro@fb.com>
Thanks!
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v5 03/16] fs: introduce alloc_inode_sb() to allocate filesystems specific inode
2022-01-11 18:55 ` Roman Gushchin
@ 2022-01-12 2:54 ` Muchun Song
0 siblings, 0 replies; 45+ messages in thread
From: Muchun Song @ 2022-01-12 2:54 UTC (permalink / raw)
To: Roman Gushchin
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Yang Shi, Alex Shi, Wei Yang,
Dave Chinner, trond.myklebust, anna.schumaker, jaegeuk, chao,
Kari Argillander, linux-fsdevel, LKML,
Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song
On Wed, Jan 12, 2022 at 2:55 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Mon, Dec 20, 2021 at 04:56:36PM +0800, Muchun Song wrote:
> > The allocated inode cache is supposed to be added to its memcg list_lru
> > which should be allocated as well in advance. That can be done by
> > kmem_cache_alloc_lru() which allocates object and list_lru. The file
> > systems is main user of it. So introduce alloc_inode_sb() to allocate
> > file system specific inodes and set up the inode reclaim context
> > properly. The file system is supposed to use alloc_inode_sb() to
> > allocate inodes. In the later patches, we will convert all users to the
> > new API.
> >
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> > Documentation/filesystems/porting.rst | 5 +++++
> > fs/inode.c | 2 +-
> > include/linux/fs.h | 11 +++++++++++
> > 3 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> > index bf19fd6b86e7..c9c157d7b7bb 100644
> > --- a/Documentation/filesystems/porting.rst
> > +++ b/Documentation/filesystems/porting.rst
> > @@ -45,6 +45,11 @@ typically between calling iget_locked() and unlocking the inode.
> >
> > At some point that will become mandatory.
> >
> > +**mandatory**
> > +
> > +The foo_inode_info should always be allocated through alloc_inode_sb() rather
> > +than kmem_cache_alloc() or kmalloc() related.
>
> I'd add a couple of words on why it has to be allocated this way.
Will do.
> > +
> > ---
>
> Reviewed-by: Roman Gushchin <guro@fb.com>
Thanks.
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v5 04/16] fs: allocate inode by using alloc_inode_sb()
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (2 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 03/16] fs: introduce alloc_inode_sb() to allocate filesystems specific inode Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2022-01-11 18:58 ` Roman Gushchin
2021-12-20 8:56 ` [PATCH v5 05/16] f2fs: " Muchun Song
` (11 subsequent siblings)
15 siblings, 1 reply; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song,
Theodore Ts'o
The inode allocation is supposed to use alloc_inode_sb(), so convert
kmem_cache_alloc() of all filesystems to alloc_inode_sb().
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Acked-by: Theodore Ts'o <tytso@mit.edu> [ext4]
---
block/bdev.c | 2 +-
drivers/dax/super.c | 2 +-
fs/9p/vfs_inode.c | 2 +-
fs/adfs/super.c | 2 +-
fs/affs/super.c | 2 +-
fs/afs/super.c | 2 +-
fs/befs/linuxvfs.c | 2 +-
fs/bfs/inode.c | 2 +-
fs/btrfs/inode.c | 2 +-
fs/ceph/inode.c | 2 +-
fs/cifs/cifsfs.c | 2 +-
fs/coda/inode.c | 2 +-
fs/ecryptfs/super.c | 2 +-
fs/efs/super.c | 2 +-
fs/erofs/super.c | 2 +-
fs/exfat/super.c | 2 +-
fs/ext2/super.c | 2 +-
fs/ext4/super.c | 2 +-
fs/fat/inode.c | 2 +-
fs/freevxfs/vxfs_super.c | 2 +-
fs/fuse/inode.c | 2 +-
fs/gfs2/super.c | 2 +-
fs/hfs/super.c | 2 +-
fs/hfsplus/super.c | 2 +-
fs/hostfs/hostfs_kern.c | 2 +-
fs/hpfs/super.c | 2 +-
fs/hugetlbfs/inode.c | 2 +-
fs/isofs/inode.c | 2 +-
fs/jffs2/super.c | 2 +-
fs/jfs/super.c | 2 +-
fs/minix/inode.c | 2 +-
fs/nfs/inode.c | 2 +-
fs/nilfs2/super.c | 2 +-
fs/ntfs/inode.c | 2 +-
fs/ntfs3/super.c | 2 +-
fs/ocfs2/dlmfs/dlmfs.c | 2 +-
fs/ocfs2/super.c | 2 +-
fs/openpromfs/inode.c | 2 +-
fs/orangefs/super.c | 2 +-
fs/overlayfs/super.c | 2 +-
fs/proc/inode.c | 2 +-
fs/qnx4/inode.c | 2 +-
fs/qnx6/inode.c | 2 +-
fs/reiserfs/super.c | 2 +-
fs/romfs/super.c | 2 +-
fs/squashfs/super.c | 2 +-
fs/sysv/inode.c | 2 +-
fs/ubifs/super.c | 2 +-
fs/udf/super.c | 2 +-
fs/ufs/super.c | 2 +-
fs/vboxsf/super.c | 2 +-
fs/xfs/xfs_icache.c | 2 +-
fs/zonefs/super.c | 2 +-
ipc/mqueue.c | 2 +-
mm/shmem.c | 2 +-
net/socket.c | 2 +-
net/sunrpc/rpc_pipe.c | 2 +-
57 files changed, 57 insertions(+), 57 deletions(-)
diff --git a/block/bdev.c b/block/bdev.c
index b1d087e5e205..32c42b47c0e9 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -390,7 +390,7 @@ static struct kmem_cache * bdev_cachep __read_mostly;
static struct inode *bdev_alloc_inode(struct super_block *sb)
{
- struct bdev_inode *ei = kmem_cache_alloc(bdev_cachep, GFP_KERNEL);
+ struct bdev_inode *ei = alloc_inode_sb(sb, bdev_cachep, GFP_KERNEL);
if (!ei)
return NULL;
diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index b882cf8106ea..15275c438a4e 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -383,7 +383,7 @@ static struct inode *dax_alloc_inode(struct super_block *sb)
struct dax_device *dax_dev;
struct inode *inode;
- dax_dev = kmem_cache_alloc(dax_cache, GFP_KERNEL);
+ dax_dev = alloc_inode_sb(sb, dax_cache, GFP_KERNEL);
if (!dax_dev)
return NULL;
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 328c338ff304..1c5d8c56d94f 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -228,7 +228,7 @@ struct inode *v9fs_alloc_inode(struct super_block *sb)
{
struct v9fs_inode *v9inode;
- v9inode = kmem_cache_alloc(v9fs_inode_cache, GFP_KERNEL);
+ v9inode = alloc_inode_sb(sb, v9fs_inode_cache, GFP_KERNEL);
if (!v9inode)
return NULL;
#ifdef CONFIG_9P_FSCACHE
diff --git a/fs/adfs/super.c b/fs/adfs/super.c
index bdbd26e571ed..e8bfc38239cd 100644
--- a/fs/adfs/super.c
+++ b/fs/adfs/super.c
@@ -220,7 +220,7 @@ static struct kmem_cache *adfs_inode_cachep;
static struct inode *adfs_alloc_inode(struct super_block *sb)
{
struct adfs_inode_info *ei;
- ei = kmem_cache_alloc(adfs_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, adfs_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
return &ei->vfs_inode;
diff --git a/fs/affs/super.c b/fs/affs/super.c
index c609005a9eaa..4c5f30a83336 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -100,7 +100,7 @@ static struct inode *affs_alloc_inode(struct super_block *sb)
{
struct affs_inode_info *i;
- i = kmem_cache_alloc(affs_inode_cachep, GFP_KERNEL);
+ i = alloc_inode_sb(sb, affs_inode_cachep, GFP_KERNEL);
if (!i)
return NULL;
diff --git a/fs/afs/super.c b/fs/afs/super.c
index d110def8aa8e..92cb145e1c65 100644
--- a/fs/afs/super.c
+++ b/fs/afs/super.c
@@ -677,7 +677,7 @@ static struct inode *afs_alloc_inode(struct super_block *sb)
{
struct afs_vnode *vnode;
- vnode = kmem_cache_alloc(afs_inode_cachep, GFP_KERNEL);
+ vnode = alloc_inode_sb(sb, afs_inode_cachep, GFP_KERNEL);
if (!vnode)
return NULL;
diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
index c1ba13d19024..b4b3567ac655 100644
--- a/fs/befs/linuxvfs.c
+++ b/fs/befs/linuxvfs.c
@@ -277,7 +277,7 @@ befs_alloc_inode(struct super_block *sb)
{
struct befs_inode_info *bi;
- bi = kmem_cache_alloc(befs_inode_cachep, GFP_KERNEL);
+ bi = alloc_inode_sb(sb, befs_inode_cachep, GFP_KERNEL);
if (!bi)
return NULL;
return &bi->vfs_inode;
diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index fd691e4815c5..1926bec2c850 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -239,7 +239,7 @@ static struct kmem_cache *bfs_inode_cachep;
static struct inode *bfs_alloc_inode(struct super_block *sb)
{
struct bfs_inode_info *bi;
- bi = kmem_cache_alloc(bfs_inode_cachep, GFP_KERNEL);
+ bi = alloc_inode_sb(sb, bfs_inode_cachep, GFP_KERNEL);
if (!bi)
return NULL;
return &bi->vfs_inode;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b8c911a4a320..b8fb50c58e6b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9085,7 +9085,7 @@ struct inode *btrfs_alloc_inode(struct super_block *sb)
struct btrfs_inode *ei;
struct inode *inode;
- ei = kmem_cache_alloc(btrfs_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, btrfs_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index e3322fcb2e8d..9cab86ef94ed 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -447,7 +447,7 @@ struct inode *ceph_alloc_inode(struct super_block *sb)
struct ceph_inode_info *ci;
int i;
- ci = kmem_cache_alloc(ceph_inode_cachep, GFP_NOFS);
+ ci = alloc_inode_sb(sb, ceph_inode_cachep, GFP_NOFS);
if (!ci)
return NULL;
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index dca42aa87d30..e39b760eb9c2 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -353,7 +353,7 @@ static struct inode *
cifs_alloc_inode(struct super_block *sb)
{
struct cifsInodeInfo *cifs_inode;
- cifs_inode = kmem_cache_alloc(cifs_inode_cachep, GFP_KERNEL);
+ cifs_inode = alloc_inode_sb(sb, cifs_inode_cachep, GFP_KERNEL);
if (!cifs_inode)
return NULL;
cifs_inode->cifsAttrs = 0x20; /* default */
diff --git a/fs/coda/inode.c b/fs/coda/inode.c
index d9f1bd7153df..2185328b65c7 100644
--- a/fs/coda/inode.c
+++ b/fs/coda/inode.c
@@ -43,7 +43,7 @@ static struct kmem_cache * coda_inode_cachep;
static struct inode *coda_alloc_inode(struct super_block *sb)
{
struct coda_inode_info *ei;
- ei = kmem_cache_alloc(coda_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, coda_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
memset(&ei->c_fid, 0, sizeof(struct CodaFid));
diff --git a/fs/ecryptfs/super.c b/fs/ecryptfs/super.c
index 39116af0390f..0b1c878317ab 100644
--- a/fs/ecryptfs/super.c
+++ b/fs/ecryptfs/super.c
@@ -38,7 +38,7 @@ static struct inode *ecryptfs_alloc_inode(struct super_block *sb)
struct ecryptfs_inode_info *inode_info;
struct inode *inode = NULL;
- inode_info = kmem_cache_alloc(ecryptfs_inode_info_cache, GFP_KERNEL);
+ inode_info = alloc_inode_sb(sb, ecryptfs_inode_info_cache, GFP_KERNEL);
if (unlikely(!inode_info))
goto out;
if (ecryptfs_init_crypt_stat(&inode_info->crypt_stat)) {
diff --git a/fs/efs/super.c b/fs/efs/super.c
index 62b155b9366b..b287f47c165b 100644
--- a/fs/efs/super.c
+++ b/fs/efs/super.c
@@ -69,7 +69,7 @@ static struct kmem_cache * efs_inode_cachep;
static struct inode *efs_alloc_inode(struct super_block *sb)
{
struct efs_inode_info *ei;
- ei = kmem_cache_alloc(efs_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, efs_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
return &ei->vfs_inode;
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 6a969b1e0ee6..d3de5674ab81 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -83,7 +83,7 @@ static void erofs_inode_init_once(void *ptr)
static struct inode *erofs_alloc_inode(struct super_block *sb)
{
struct erofs_inode *vi =
- kmem_cache_alloc(erofs_inode_cachep, GFP_KERNEL);
+ alloc_inode_sb(sb, erofs_inode_cachep, GFP_KERNEL);
if (!vi)
return NULL;
diff --git a/fs/exfat/super.c b/fs/exfat/super.c
index 5539ffc20d16..1b24c9f61be1 100644
--- a/fs/exfat/super.c
+++ b/fs/exfat/super.c
@@ -182,7 +182,7 @@ static struct inode *exfat_alloc_inode(struct super_block *sb)
{
struct exfat_inode_info *ei;
- ei = kmem_cache_alloc(exfat_inode_cachep, GFP_NOFS);
+ ei = alloc_inode_sb(sb, exfat_inode_cachep, GFP_NOFS);
if (!ei)
return NULL;
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index d8d580b609ba..8deb03ae4742 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -180,7 +180,7 @@ static struct kmem_cache * ext2_inode_cachep;
static struct inode *ext2_alloc_inode(struct super_block *sb)
{
struct ext2_inode_info *ei;
- ei = kmem_cache_alloc(ext2_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, ext2_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
ei->i_block_alloc_info = NULL;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 4e33b5eca694..19c16d9b173d 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1292,7 +1292,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
{
struct ext4_inode_info *ei;
- ei = kmem_cache_alloc(ext4_inode_cachep, GFP_NOFS);
+ ei = alloc_inode_sb(sb, ext4_inode_cachep, GFP_NOFS);
if (!ei)
return NULL;
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index a6f1c6d426d1..38796681cc86 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -745,7 +745,7 @@ static struct kmem_cache *fat_inode_cachep;
static struct inode *fat_alloc_inode(struct super_block *sb)
{
struct msdos_inode_info *ei;
- ei = kmem_cache_alloc(fat_inode_cachep, GFP_NOFS);
+ ei = alloc_inode_sb(sb, fat_inode_cachep, GFP_NOFS);
if (!ei)
return NULL;
diff --git a/fs/freevxfs/vxfs_super.c b/fs/freevxfs/vxfs_super.c
index 578a5062706e..22eed5a73ac2 100644
--- a/fs/freevxfs/vxfs_super.c
+++ b/fs/freevxfs/vxfs_super.c
@@ -124,7 +124,7 @@ static struct inode *vxfs_alloc_inode(struct super_block *sb)
{
struct vxfs_inode_info *vi;
- vi = kmem_cache_alloc(vxfs_inode_cachep, GFP_KERNEL);
+ vi = alloc_inode_sb(sb, vxfs_inode_cachep, GFP_KERNEL);
if (!vi)
return NULL;
inode_init_once(&vi->vfs_inode);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 8b89e3ba7df3..6497603fd69c 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -73,7 +73,7 @@ static struct inode *fuse_alloc_inode(struct super_block *sb)
{
struct fuse_inode *fi;
- fi = kmem_cache_alloc(fuse_inode_cachep, GFP_KERNEL);
+ fi = alloc_inode_sb(sb, fuse_inode_cachep, GFP_KERNEL);
if (!fi)
return NULL;
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 0f93e8beca4d..55440fb51df9 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1427,7 +1427,7 @@ static struct inode *gfs2_alloc_inode(struct super_block *sb)
{
struct gfs2_inode *ip;
- ip = kmem_cache_alloc(gfs2_inode_cachep, GFP_KERNEL);
+ ip = alloc_inode_sb(sb, gfs2_inode_cachep, GFP_KERNEL);
if (!ip)
return NULL;
ip->i_flags = 0;
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 12d9bae39363..6764afa98a6f 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -162,7 +162,7 @@ static struct inode *hfs_alloc_inode(struct super_block *sb)
{
struct hfs_inode_info *i;
- i = kmem_cache_alloc(hfs_inode_cachep, GFP_KERNEL);
+ i = alloc_inode_sb(sb, hfs_inode_cachep, GFP_KERNEL);
return i ? &i->vfs_inode : NULL;
}
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index b9e3db3f855f..8479add998b5 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -624,7 +624,7 @@ static struct inode *hfsplus_alloc_inode(struct super_block *sb)
{
struct hfsplus_inode_info *i;
- i = kmem_cache_alloc(hfsplus_inode_cachep, GFP_KERNEL);
+ i = alloc_inode_sb(sb, hfsplus_inode_cachep, GFP_KERNEL);
return i ? &i->vfs_inode : NULL;
}
diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index d5c9d886cd9f..2123d2bed55b 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -222,7 +222,7 @@ static struct inode *hostfs_alloc_inode(struct super_block *sb)
{
struct hostfs_inode_info *hi;
- hi = kmem_cache_alloc(hostfs_inode_cache, GFP_KERNEL_ACCOUNT);
+ hi = alloc_inode_sb(sb, hostfs_inode_cache, GFP_KERNEL_ACCOUNT);
if (hi == NULL)
return NULL;
hi->fd = -1;
diff --git a/fs/hpfs/super.c b/fs/hpfs/super.c
index a7dbfc892022..1cb89595b875 100644
--- a/fs/hpfs/super.c
+++ b/fs/hpfs/super.c
@@ -232,7 +232,7 @@ static struct kmem_cache * hpfs_inode_cachep;
static struct inode *hpfs_alloc_inode(struct super_block *sb)
{
struct hpfs_inode_info *ei;
- ei = kmem_cache_alloc(hpfs_inode_cachep, GFP_NOFS);
+ ei = alloc_inode_sb(sb, hpfs_inode_cachep, GFP_NOFS);
if (!ei)
return NULL;
return &ei->vfs_inode;
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 49d2e686be74..7e0bd1c629e1 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -1109,7 +1109,7 @@ static struct inode *hugetlbfs_alloc_inode(struct super_block *sb)
if (unlikely(!hugetlbfs_dec_free_inodes(sbinfo)))
return NULL;
- p = kmem_cache_alloc(hugetlbfs_inode_cachep, GFP_KERNEL);
+ p = alloc_inode_sb(sb, hugetlbfs_inode_cachep, GFP_KERNEL);
if (unlikely(!p)) {
hugetlbfs_inc_free_inodes(sbinfo);
return NULL;
diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 0c6eacfcbeef..d7491692aea3 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -70,7 +70,7 @@ static struct kmem_cache *isofs_inode_cachep;
static struct inode *isofs_alloc_inode(struct super_block *sb)
{
struct iso_inode_info *ei;
- ei = kmem_cache_alloc(isofs_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, isofs_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
return &ei->vfs_inode;
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 81ca58c10b72..7ea37f49f1e1 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -39,7 +39,7 @@ static struct inode *jffs2_alloc_inode(struct super_block *sb)
{
struct jffs2_inode_info *f;
- f = kmem_cache_alloc(jffs2_inode_cachep, GFP_KERNEL);
+ f = alloc_inode_sb(sb, jffs2_inode_cachep, GFP_KERNEL);
if (!f)
return NULL;
return &f->vfs_inode;
diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index 24cbc9946e01..f1a13a74cddf 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -102,7 +102,7 @@ static struct inode *jfs_alloc_inode(struct super_block *sb)
{
struct jfs_inode_info *jfs_inode;
- jfs_inode = kmem_cache_alloc(jfs_inode_cachep, GFP_NOFS);
+ jfs_inode = alloc_inode_sb(sb, jfs_inode_cachep, GFP_NOFS);
if (!jfs_inode)
return NULL;
#ifdef CONFIG_QUOTA
diff --git a/fs/minix/inode.c b/fs/minix/inode.c
index a71f1cf894b9..8a0af80741b5 100644
--- a/fs/minix/inode.c
+++ b/fs/minix/inode.c
@@ -63,7 +63,7 @@ static struct kmem_cache * minix_inode_cachep;
static struct inode *minix_alloc_inode(struct super_block *sb)
{
struct minix_inode_info *ei;
- ei = kmem_cache_alloc(minix_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, minix_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
return &ei->vfs_inode;
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index fda530d5e764..59a63f1bd9ee 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -2241,7 +2241,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
struct inode *nfs_alloc_inode(struct super_block *sb)
{
struct nfs_inode *nfsi;
- nfsi = kmem_cache_alloc(nfs_inode_cachep, GFP_KERNEL);
+ nfsi = alloc_inode_sb(sb, nfs_inode_cachep, GFP_KERNEL);
if (!nfsi)
return NULL;
nfsi->flags = 0UL;
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 63e5fa74016c..3e05c98631ec 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -151,7 +151,7 @@ struct inode *nilfs_alloc_inode(struct super_block *sb)
{
struct nilfs_inode_info *ii;
- ii = kmem_cache_alloc(nilfs_inode_cachep, GFP_NOFS);
+ ii = alloc_inode_sb(sb, nilfs_inode_cachep, GFP_NOFS);
if (!ii)
return NULL;
ii->i_bh = NULL;
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 4474adb393ca..fca18ac72b4f 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -310,7 +310,7 @@ struct inode *ntfs_alloc_big_inode(struct super_block *sb)
ntfs_inode *ni;
ntfs_debug("Entering.");
- ni = kmem_cache_alloc(ntfs_big_inode_cache, GFP_NOFS);
+ ni = alloc_inode_sb(sb, ntfs_big_inode_cache, GFP_NOFS);
if (likely(ni != NULL)) {
ni->state = 0;
return VFS_I(ni);
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
index 29813200c7af..278dcf502410 100644
--- a/fs/ntfs3/super.c
+++ b/fs/ntfs3/super.c
@@ -399,7 +399,7 @@ static struct kmem_cache *ntfs_inode_cachep;
static struct inode *ntfs_alloc_inode(struct super_block *sb)
{
- struct ntfs_inode *ni = kmem_cache_alloc(ntfs_inode_cachep, GFP_NOFS);
+ struct ntfs_inode *ni = alloc_inode_sb(sb, ntfs_inode_cachep, GFP_NOFS);
if (!ni)
return NULL;
diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c
index fa0a14f199eb..e360543ad7e7 100644
--- a/fs/ocfs2/dlmfs/dlmfs.c
+++ b/fs/ocfs2/dlmfs/dlmfs.c
@@ -280,7 +280,7 @@ static struct inode *dlmfs_alloc_inode(struct super_block *sb)
{
struct dlmfs_inode_private *ip;
- ip = kmem_cache_alloc(dlmfs_inode_cache, GFP_NOFS);
+ ip = alloc_inode_sb(sb, dlmfs_inode_cache, GFP_NOFS);
if (!ip)
return NULL;
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index 1286b88b6fa1..0452d5835ffe 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -549,7 +549,7 @@ static struct inode *ocfs2_alloc_inode(struct super_block *sb)
{
struct ocfs2_inode_info *oi;
- oi = kmem_cache_alloc(ocfs2_inode_cachep, GFP_NOFS);
+ oi = alloc_inode_sb(sb, ocfs2_inode_cachep, GFP_NOFS);
if (!oi)
return NULL;
diff --git a/fs/openpromfs/inode.c b/fs/openpromfs/inode.c
index f825176ff4ed..f0b7f4d51a17 100644
--- a/fs/openpromfs/inode.c
+++ b/fs/openpromfs/inode.c
@@ -335,7 +335,7 @@ static struct inode *openprom_alloc_inode(struct super_block *sb)
{
struct op_inode_info *oi;
- oi = kmem_cache_alloc(op_inode_cachep, GFP_KERNEL);
+ oi = alloc_inode_sb(sb, op_inode_cachep, GFP_KERNEL);
if (!oi)
return NULL;
diff --git a/fs/orangefs/super.c b/fs/orangefs/super.c
index d90d8addbfc2..5254256a224d 100644
--- a/fs/orangefs/super.c
+++ b/fs/orangefs/super.c
@@ -107,7 +107,7 @@ static struct inode *orangefs_alloc_inode(struct super_block *sb)
{
struct orangefs_inode_s *orangefs_inode;
- orangefs_inode = kmem_cache_alloc(orangefs_inode_cache, GFP_KERNEL);
+ orangefs_inode = alloc_inode_sb(sb, orangefs_inode_cache, GFP_KERNEL);
if (!orangefs_inode)
return NULL;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 265181c110ae..a1987aef2dfb 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -174,7 +174,7 @@ static struct kmem_cache *ovl_inode_cachep;
static struct inode *ovl_alloc_inode(struct super_block *sb)
{
- struct ovl_inode *oi = kmem_cache_alloc(ovl_inode_cachep, GFP_KERNEL);
+ struct ovl_inode *oi = alloc_inode_sb(sb, ovl_inode_cachep, GFP_KERNEL);
if (!oi)
return NULL;
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 599eb724ff2d..cc0a406d3a19 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -66,7 +66,7 @@ static struct inode *proc_alloc_inode(struct super_block *sb)
{
struct proc_inode *ei;
- ei = kmem_cache_alloc(proc_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, proc_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
ei->pid = NULL;
diff --git a/fs/qnx4/inode.c b/fs/qnx4/inode.c
index 3fb7fc819b4f..a635bb6615e9 100644
--- a/fs/qnx4/inode.c
+++ b/fs/qnx4/inode.c
@@ -338,7 +338,7 @@ static struct kmem_cache *qnx4_inode_cachep;
static struct inode *qnx4_alloc_inode(struct super_block *sb)
{
struct qnx4_inode_info *ei;
- ei = kmem_cache_alloc(qnx4_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, qnx4_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
return &ei->vfs_inode;
diff --git a/fs/qnx6/inode.c b/fs/qnx6/inode.c
index 61191f7bdf62..9d8e7e9788a1 100644
--- a/fs/qnx6/inode.c
+++ b/fs/qnx6/inode.c
@@ -597,7 +597,7 @@ static struct kmem_cache *qnx6_inode_cachep;
static struct inode *qnx6_alloc_inode(struct super_block *sb)
{
struct qnx6_inode_info *ei;
- ei = kmem_cache_alloc(qnx6_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, qnx6_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
return &ei->vfs_inode;
diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index 82e09901462e..756c4916c7ae 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -639,7 +639,7 @@ static struct kmem_cache *reiserfs_inode_cachep;
static struct inode *reiserfs_alloc_inode(struct super_block *sb)
{
struct reiserfs_inode_info *ei;
- ei = kmem_cache_alloc(reiserfs_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, reiserfs_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
atomic_set(&ei->openers, 0);
diff --git a/fs/romfs/super.c b/fs/romfs/super.c
index 259f684d9236..9e6bbb4219de 100644
--- a/fs/romfs/super.c
+++ b/fs/romfs/super.c
@@ -375,7 +375,7 @@ static struct inode *romfs_alloc_inode(struct super_block *sb)
{
struct romfs_inode_info *inode;
- inode = kmem_cache_alloc(romfs_inode_cachep, GFP_KERNEL);
+ inode = alloc_inode_sb(sb, romfs_inode_cachep, GFP_KERNEL);
return inode ? &inode->vfs_inode : NULL;
}
diff --git a/fs/squashfs/super.c b/fs/squashfs/super.c
index bb44ff4c5cc6..32565dafa7f3 100644
--- a/fs/squashfs/super.c
+++ b/fs/squashfs/super.c
@@ -551,7 +551,7 @@ static void __exit exit_squashfs_fs(void)
static struct inode *squashfs_alloc_inode(struct super_block *sb)
{
struct squashfs_inode_info *ei =
- kmem_cache_alloc(squashfs_inode_cachep, GFP_KERNEL);
+ alloc_inode_sb(sb, squashfs_inode_cachep, GFP_KERNEL);
return ei ? &ei->vfs_inode : NULL;
}
diff --git a/fs/sysv/inode.c b/fs/sysv/inode.c
index be47263b8605..9e8d4a6fb2f3 100644
--- a/fs/sysv/inode.c
+++ b/fs/sysv/inode.c
@@ -306,7 +306,7 @@ static struct inode *sysv_alloc_inode(struct super_block *sb)
{
struct sysv_inode_info *si;
- si = kmem_cache_alloc(sysv_inode_cachep, GFP_KERNEL);
+ si = alloc_inode_sb(sb, sysv_inode_cachep, GFP_KERNEL);
if (!si)
return NULL;
return &si->vfs_inode;
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index f0fb25727d96..73b51f8e6817 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -268,7 +268,7 @@ static struct inode *ubifs_alloc_inode(struct super_block *sb)
{
struct ubifs_inode *ui;
- ui = kmem_cache_alloc(ubifs_inode_slab, GFP_NOFS);
+ ui = alloc_inode_sb(sb, ubifs_inode_slab, GFP_NOFS);
if (!ui)
return NULL;
diff --git a/fs/udf/super.c b/fs/udf/super.c
index f26b5e0b84b6..48871615e489 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -136,7 +136,7 @@ static struct kmem_cache *udf_inode_cachep;
static struct inode *udf_alloc_inode(struct super_block *sb)
{
struct udf_inode_info *ei;
- ei = kmem_cache_alloc(udf_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, udf_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
diff --git a/fs/ufs/super.c b/fs/ufs/super.c
index 00a01471ea05..23377c1baed9 100644
--- a/fs/ufs/super.c
+++ b/fs/ufs/super.c
@@ -1443,7 +1443,7 @@ static struct inode *ufs_alloc_inode(struct super_block *sb)
{
struct ufs_inode_info *ei;
- ei = kmem_cache_alloc(ufs_inode_cachep, GFP_NOFS);
+ ei = alloc_inode_sb(sb, ufs_inode_cachep, GFP_NOFS);
if (!ei)
return NULL;
diff --git a/fs/vboxsf/super.c b/fs/vboxsf/super.c
index 37dd3fe5b1e9..d2f6df69f611 100644
--- a/fs/vboxsf/super.c
+++ b/fs/vboxsf/super.c
@@ -241,7 +241,7 @@ static struct inode *vboxsf_alloc_inode(struct super_block *sb)
{
struct vboxsf_inode *sf_i;
- sf_i = kmem_cache_alloc(vboxsf_inode_cachep, GFP_NOFS);
+ sf_i = alloc_inode_sb(sb, vboxsf_inode_cachep, GFP_NOFS);
if (!sf_i)
return NULL;
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index da4af2142a2b..57b4fd7d7c5e 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -77,7 +77,7 @@ xfs_inode_alloc(
* XXX: If this didn't occur in transactions, we could drop GFP_NOFAIL
* and return NULL here on ENOMEM.
*/
- ip = kmem_cache_alloc(xfs_inode_cache, GFP_KERNEL | __GFP_NOFAIL);
+ ip = alloc_inode_sb(mp->m_super, xfs_inode_cache, GFP_KERNEL | __GFP_NOFAIL);
if (inode_init_always(mp->m_super, VFS_I(ip))) {
kmem_cache_free(xfs_inode_cache, ip);
diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index 259ee2bda492..e88e16a1529a 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -1137,7 +1137,7 @@ static struct inode *zonefs_alloc_inode(struct super_block *sb)
{
struct zonefs_inode_info *zi;
- zi = kmem_cache_alloc(zonefs_inode_cachep, GFP_KERNEL);
+ zi = alloc_inode_sb(sb, zonefs_inode_cachep, GFP_KERNEL);
if (!zi)
return NULL;
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 5becca9be867..7c08eb3c258d 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -486,7 +486,7 @@ static struct inode *mqueue_alloc_inode(struct super_block *sb)
{
struct mqueue_inode_info *ei;
- ei = kmem_cache_alloc(mqueue_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, mqueue_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
return &ei->vfs_inode;
diff --git a/mm/shmem.c b/mm/shmem.c
index 18f93c2d68f1..b8490cbbc1a0 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3714,7 +3714,7 @@ static struct kmem_cache *shmem_inode_cachep;
static struct inode *shmem_alloc_inode(struct super_block *sb)
{
struct shmem_inode_info *info;
- info = kmem_cache_alloc(shmem_inode_cachep, GFP_KERNEL);
+ info = alloc_inode_sb(sb, shmem_inode_cachep, GFP_KERNEL);
if (!info)
return NULL;
return &info->vfs_inode;
diff --git a/net/socket.c b/net/socket.c
index 7f64a6eccf63..cee567ccd99c 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -300,7 +300,7 @@ static struct inode *sock_alloc_inode(struct super_block *sb)
{
struct socket_alloc *ei;
- ei = kmem_cache_alloc(sock_inode_cachep, GFP_KERNEL);
+ ei = alloc_inode_sb(sb, sock_inode_cachep, GFP_KERNEL);
if (!ei)
return NULL;
init_waitqueue_head(&ei->socket.wq.wait);
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index ee5336d73fdd..7ed4accc480d 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -197,7 +197,7 @@ static struct inode *
rpc_alloc_inode(struct super_block *sb)
{
struct rpc_inode *rpci;
- rpci = kmem_cache_alloc(rpc_inode_cachep, GFP_KERNEL);
+ rpci = alloc_inode_sb(sb, rpc_inode_cachep, GFP_KERNEL);
if (!rpci)
return NULL;
return &rpci->vfs_inode;
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v5 04/16] fs: allocate inode by using alloc_inode_sb()
2021-12-20 8:56 ` [PATCH v5 04/16] fs: allocate inode by using alloc_inode_sb() Muchun Song
@ 2022-01-11 18:58 ` Roman Gushchin
2022-01-12 2:55 ` Muchun Song
0 siblings, 1 reply; 45+ messages in thread
From: Roman Gushchin @ 2022-01-11 18:58 UTC (permalink / raw)
To: Muchun Song
Cc: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, shy828301,
alexs, richard.weiyang, david, trond.myklebust, anna.schumaker,
jaegeuk, chao, kari.argillander, linux-fsdevel, linux-kernel,
linux-mm, linux-nfs, zhengqi.arch, duanxiongchun, fam.zheng,
smuchun, Theodore Ts'o
On Mon, Dec 20, 2021 at 04:56:37PM +0800, Muchun Song wrote:
> The inode allocation is supposed to use alloc_inode_sb(), so convert
> kmem_cache_alloc() of all filesystems to alloc_inode_sb().
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Acked-by: Theodore Ts'o <tytso@mit.edu> [ext4]
LGTM
Acked-by: Roman Gushchin <guro@fb.com>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v5 04/16] fs: allocate inode by using alloc_inode_sb()
2022-01-11 18:58 ` Roman Gushchin
@ 2022-01-12 2:55 ` Muchun Song
0 siblings, 0 replies; 45+ messages in thread
From: Muchun Song @ 2022-01-12 2:55 UTC (permalink / raw)
To: Roman Gushchin
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Yang Shi, Alex Shi, Wei Yang,
Dave Chinner, trond.myklebust, anna.schumaker, jaegeuk, chao,
Kari Argillander, linux-fsdevel, LKML,
Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song, Theodore Ts'o
On Wed, Jan 12, 2022 at 2:58 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Mon, Dec 20, 2021 at 04:56:37PM +0800, Muchun Song wrote:
> > The inode allocation is supposed to use alloc_inode_sb(), so convert
> > kmem_cache_alloc() of all filesystems to alloc_inode_sb().
> >
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > Acked-by: Theodore Ts'o <tytso@mit.edu> [ext4]
>
> LGTM
>
> Acked-by: Roman Gushchin <guro@fb.com>
Thanks Roman.
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v5 05/16] f2fs: allocate inode by using alloc_inode_sb()
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (3 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 04/16] fs: allocate inode by using alloc_inode_sb() Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2022-01-11 19:03 ` Roman Gushchin
2021-12-20 8:56 ` [PATCH v5 06/16] nfs42: use a specific kmem_cache to allocate nfs4_xattr_entry Muchun Song
` (10 subsequent siblings)
15 siblings, 1 reply; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
The inode allocation is supposed to use alloc_inode_sb(), so convert
kmem_cache_alloc() to alloc_inode_sb().
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
fs/f2fs/super.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 040b6d02e1d8..6cdbf520b435 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1311,8 +1311,12 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
{
struct f2fs_inode_info *fi;
- fi = f2fs_kmem_cache_alloc(f2fs_inode_cachep,
- GFP_F2FS_ZERO, false, F2FS_SB(sb));
+ if (time_to_inject(F2FS_SB(sb), FAULT_SLAB_ALLOC)) {
+ f2fs_show_injection_info(F2FS_SB(sb), FAULT_SLAB_ALLOC);
+ return NULL;
+ }
+
+ fi = alloc_inode_sb(sb, f2fs_inode_cachep, GFP_F2FS_ZERO);
if (!fi)
return NULL;
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v5 05/16] f2fs: allocate inode by using alloc_inode_sb()
2021-12-20 8:56 ` [PATCH v5 05/16] f2fs: " Muchun Song
@ 2022-01-11 19:03 ` Roman Gushchin
0 siblings, 0 replies; 45+ messages in thread
From: Roman Gushchin @ 2022-01-11 19:03 UTC (permalink / raw)
To: Muchun Song
Cc: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, shy828301,
alexs, richard.weiyang, david, trond.myklebust, anna.schumaker,
jaegeuk, chao, kari.argillander, linux-fsdevel, linux-kernel,
linux-mm, linux-nfs, zhengqi.arch, duanxiongchun, fam.zheng,
smuchun
On Mon, Dec 20, 2021 at 04:56:38PM +0800, Muchun Song wrote:
> The inode allocation is supposed to use alloc_inode_sb(), so convert
> kmem_cache_alloc() to alloc_inode_sb().
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
LGTM
Acked-by: Roman Gushchin <guro@fb.com>
> ---
> fs/f2fs/super.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 040b6d02e1d8..6cdbf520b435 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1311,8 +1311,12 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
> {
> struct f2fs_inode_info *fi;
>
> - fi = f2fs_kmem_cache_alloc(f2fs_inode_cachep,
> - GFP_F2FS_ZERO, false, F2FS_SB(sb));
> + if (time_to_inject(F2FS_SB(sb), FAULT_SLAB_ALLOC)) {
> + f2fs_show_injection_info(F2FS_SB(sb), FAULT_SLAB_ALLOC);
> + return NULL;
> + }
> +
> + fi = alloc_inode_sb(sb, f2fs_inode_cachep, GFP_F2FS_ZERO);
> if (!fi)
> return NULL;
>
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v5 06/16] nfs42: use a specific kmem_cache to allocate nfs4_xattr_entry
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (4 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 05/16] f2fs: " Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2021-12-20 8:56 ` [PATCH v5 07/16] mm: dcache: use kmem_cache_alloc_lru() to allocate dentry Muchun Song
` (9 subsequent siblings)
15 siblings, 0 replies; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
If we want to add the allocated objects to its list_lru, we should use
kmem_cache_alloc_lru() to allocate objects. So intruduce
nfs4_xattr_entry_cachep which is used to allocate nfs4_xattr_entry.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
fs/nfs/nfs42xattr.c | 95 ++++++++++++++++++++++++++---------------------------
1 file changed, 47 insertions(+), 48 deletions(-)
diff --git a/fs/nfs/nfs42xattr.c b/fs/nfs/nfs42xattr.c
index 1c4d2a05b401..5b7af9080db0 100644
--- a/fs/nfs/nfs42xattr.c
+++ b/fs/nfs/nfs42xattr.c
@@ -81,7 +81,7 @@ struct nfs4_xattr_entry {
struct hlist_node hnode;
struct list_head lru;
struct list_head dispose;
- char *xattr_name;
+ const char *xattr_name;
void *xattr_value;
size_t xattr_size;
struct nfs4_xattr_bucket *bucket;
@@ -98,6 +98,7 @@ static struct list_lru nfs4_xattr_entry_lru;
static struct list_lru nfs4_xattr_large_entry_lru;
static struct kmem_cache *nfs4_xattr_cache_cachep;
+static struct kmem_cache *nfs4_xattr_entry_cachep;
/*
* Hashing helper functions.
@@ -177,49 +178,27 @@ nfs4_xattr_alloc_entry(const char *name, const void *value,
{
struct nfs4_xattr_entry *entry;
void *valp;
- char *namep;
- size_t alloclen, slen;
- char *buf;
- uint32_t flags;
+ const char *namep;
+ uint32_t flags = len > PAGE_SIZE ? NFS4_XATTR_ENTRY_EXTVAL : 0;
+ gfp_t gfp = GFP_KERNEL_ACCOUNT | GFP_NOFS;
+ struct list_lru *lru;
BUILD_BUG_ON(sizeof(struct nfs4_xattr_entry) +
XATTR_NAME_MAX + 1 > PAGE_SIZE);
- alloclen = sizeof(struct nfs4_xattr_entry);
- if (name != NULL) {
- slen = strlen(name) + 1;
- alloclen += slen;
- } else
- slen = 0;
-
- if (alloclen + len <= PAGE_SIZE) {
- alloclen += len;
- flags = 0;
- } else {
- flags = NFS4_XATTR_ENTRY_EXTVAL;
- }
-
- buf = kmalloc(alloclen, GFP_KERNEL_ACCOUNT | GFP_NOFS);
- if (buf == NULL)
+ lru = flags & NFS4_XATTR_ENTRY_EXTVAL ? &nfs4_xattr_large_entry_lru :
+ &nfs4_xattr_entry_lru;
+ entry = kmem_cache_alloc_lru(nfs4_xattr_entry_cachep, lru, gfp);
+ if (!entry)
return NULL;
- entry = (struct nfs4_xattr_entry *)buf;
-
- if (name != NULL) {
- namep = buf + sizeof(struct nfs4_xattr_entry);
- memcpy(namep, name, slen);
- } else {
- namep = NULL;
- }
-
-
- if (flags & NFS4_XATTR_ENTRY_EXTVAL) {
- valp = kvmalloc(len, GFP_KERNEL_ACCOUNT | GFP_NOFS);
- if (valp == NULL) {
- kfree(buf);
- return NULL;
- }
- } else if (len != 0) {
- valp = buf + sizeof(struct nfs4_xattr_entry) + slen;
+ namep = kstrdup_const(name, gfp);
+ if (!namep && name)
+ goto free_buf;
+
+ if (len != 0) {
+ valp = kvmalloc(len, gfp);
+ if (!valp)
+ goto free_name;
} else
valp = NULL;
@@ -232,23 +211,23 @@ nfs4_xattr_alloc_entry(const char *name, const void *value,
entry->flags = flags;
entry->xattr_value = valp;
- kref_init(&entry->ref);
entry->xattr_name = namep;
entry->xattr_size = len;
- entry->bucket = NULL;
- INIT_LIST_HEAD(&entry->lru);
- INIT_LIST_HEAD(&entry->dispose);
- INIT_HLIST_NODE(&entry->hnode);
return entry;
+free_name:
+ kfree_const(namep);
+free_buf:
+ kmem_cache_free(nfs4_xattr_entry_cachep, entry);
+ return NULL;
}
static void
nfs4_xattr_free_entry(struct nfs4_xattr_entry *entry)
{
- if (entry->flags & NFS4_XATTR_ENTRY_EXTVAL)
- kvfree(entry->xattr_value);
- kfree(entry);
+ kvfree(entry->xattr_value);
+ kfree_const(entry->xattr_name);
+ kmem_cache_free(nfs4_xattr_entry_cachep, entry);
}
static void
@@ -289,7 +268,7 @@ nfs4_xattr_alloc_cache(void)
{
struct nfs4_xattr_cache *cache;
- cache = kmem_cache_alloc(nfs4_xattr_cache_cachep,
+ cache = kmem_cache_alloc_lru(nfs4_xattr_cache_cachep, &nfs4_xattr_cache_lru,
GFP_KERNEL_ACCOUNT | GFP_NOFS);
if (cache == NULL)
return NULL;
@@ -992,6 +971,17 @@ static void nfs4_xattr_cache_init_once(void *p)
INIT_LIST_HEAD(&cache->dispose);
}
+static void nfs4_xattr_entry_init_once(void *p)
+{
+ struct nfs4_xattr_entry *entry = p;
+
+ kref_init(&entry->ref);
+ entry->bucket = NULL;
+ INIT_LIST_HEAD(&entry->lru);
+ INIT_LIST_HEAD(&entry->dispose);
+ INIT_HLIST_NODE(&entry->hnode);
+}
+
int __init nfs4_xattr_cache_init(void)
{
int ret = 0;
@@ -1003,6 +993,13 @@ int __init nfs4_xattr_cache_init(void)
if (nfs4_xattr_cache_cachep == NULL)
return -ENOMEM;
+ nfs4_xattr_entry_cachep = kmem_cache_create("nfs4_xattr_entry",
+ sizeof(struct nfs4_xattr_entry), 0,
+ (SLAB_RECLAIM_ACCOUNT | SLAB_MEM_SPREAD | SLAB_ACCOUNT),
+ nfs4_xattr_entry_init_once);
+ if (!nfs4_xattr_entry_cachep)
+ goto out5;
+
ret = list_lru_init_memcg(&nfs4_xattr_large_entry_lru,
&nfs4_xattr_large_entry_shrinker);
if (ret)
@@ -1040,6 +1037,8 @@ int __init nfs4_xattr_cache_init(void)
out3:
list_lru_destroy(&nfs4_xattr_large_entry_lru);
out4:
+ kmem_cache_destroy(nfs4_xattr_entry_cachep);
+out5:
kmem_cache_destroy(nfs4_xattr_cache_cachep);
return ret;
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v5 07/16] mm: dcache: use kmem_cache_alloc_lru() to allocate dentry
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (5 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 06/16] nfs42: use a specific kmem_cache to allocate nfs4_xattr_entry Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2022-01-11 19:05 ` Roman Gushchin
2021-12-20 8:56 ` [PATCH v5 08/16] xarray: use kmem_cache_alloc_lru to allocate xa_node Muchun Song
` (8 subsequent siblings)
15 siblings, 1 reply; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
Like inode cache, the dentry will also be added to its memcg list_lru.
So replace kmem_cache_alloc() with kmem_cache_alloc_lru() to allocate
dentry.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
fs/dcache.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index cf871a81f4fd..36d4806d7284 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1741,7 +1741,8 @@ static struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
char *dname;
int err;
- dentry = kmem_cache_alloc(dentry_cache, GFP_KERNEL);
+ dentry = kmem_cache_alloc_lru(dentry_cache, &sb->s_dentry_lru,
+ GFP_KERNEL);
if (!dentry)
return NULL;
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v5 07/16] mm: dcache: use kmem_cache_alloc_lru() to allocate dentry
2021-12-20 8:56 ` [PATCH v5 07/16] mm: dcache: use kmem_cache_alloc_lru() to allocate dentry Muchun Song
@ 2022-01-11 19:05 ` Roman Gushchin
0 siblings, 0 replies; 45+ messages in thread
From: Roman Gushchin @ 2022-01-11 19:05 UTC (permalink / raw)
To: Muchun Song
Cc: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, shy828301,
alexs, richard.weiyang, david, trond.myklebust, anna.schumaker,
jaegeuk, chao, kari.argillander, linux-fsdevel, linux-kernel,
linux-mm, linux-nfs, zhengqi.arch, duanxiongchun, fam.zheng,
smuchun
On Mon, Dec 20, 2021 at 04:56:40PM +0800, Muchun Song wrote:
> Like inode cache, the dentry will also be added to its memcg list_lru.
> So replace kmem_cache_alloc() with kmem_cache_alloc_lru() to allocate
> dentry.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Acked-by: Roman Gushchin <guro@fb.com>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v5 08/16] xarray: use kmem_cache_alloc_lru to allocate xa_node
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (6 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 07/16] mm: dcache: use kmem_cache_alloc_lru() to allocate dentry Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2022-01-11 19:14 ` Roman Gushchin
2021-12-20 8:56 ` [PATCH v5 09/16] mm: memcontrol: move memcg_online_kmem() to mem_cgroup_css_online() Muchun Song
` (7 subsequent siblings)
15 siblings, 1 reply; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
The workingset will add the xa_node to the shadow_nodes list. So the
allocation of xa_node should be done by kmem_cache_alloc_lru(). Using
xas_set_lru() to pass the list_lru which we want to insert xa_node
into to set up the xa_node reclaim context correctly.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
---
include/linux/swap.h | 5 ++++-
include/linux/xarray.h | 9 ++++++++-
lib/xarray.c | 10 +++++-----
mm/workingset.c | 2 +-
4 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/include/linux/swap.h b/include/linux/swap.h
index d1ea44b31f19..1ae9d3473c02 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -334,9 +334,12 @@ void workingset_activation(struct folio *folio);
/* Only track the nodes of mappings with shadow entries */
void workingset_update_node(struct xa_node *node);
+extern struct list_lru shadow_nodes;
#define mapping_set_update(xas, mapping) do { \
- if (!dax_mapping(mapping) && !shmem_mapping(mapping)) \
+ if (!dax_mapping(mapping) && !shmem_mapping(mapping)) { \
xas_set_update(xas, workingset_update_node); \
+ xas_set_lru(xas, &shadow_nodes); \
+ } \
} while (0)
/* linux/mm/page_alloc.c */
diff --git a/include/linux/xarray.h b/include/linux/xarray.h
index a91e3d90df8a..31f3e5ef3c7b 100644
--- a/include/linux/xarray.h
+++ b/include/linux/xarray.h
@@ -1317,6 +1317,7 @@ struct xa_state {
struct xa_node *xa_node;
struct xa_node *xa_alloc;
xa_update_node_t xa_update;
+ struct list_lru *xa_lru;
};
/*
@@ -1336,7 +1337,8 @@ struct xa_state {
.xa_pad = 0, \
.xa_node = XAS_RESTART, \
.xa_alloc = NULL, \
- .xa_update = NULL \
+ .xa_update = NULL, \
+ .xa_lru = NULL, \
}
/**
@@ -1613,6 +1615,11 @@ static inline void xas_set_update(struct xa_state *xas, xa_update_node_t update)
xas->xa_update = update;
}
+static inline void xas_set_lru(struct xa_state *xas, struct list_lru *lru)
+{
+ xas->xa_lru = lru;
+}
+
/**
* xas_next_entry() - Advance iterator to next present entry.
* @xas: XArray operation state.
diff --git a/lib/xarray.c b/lib/xarray.c
index f5d8f54907b4..e9b818abc823 100644
--- a/lib/xarray.c
+++ b/lib/xarray.c
@@ -302,7 +302,7 @@ bool xas_nomem(struct xa_state *xas, gfp_t gfp)
}
if (xas->xa->xa_flags & XA_FLAGS_ACCOUNT)
gfp |= __GFP_ACCOUNT;
- xas->xa_alloc = kmem_cache_alloc(radix_tree_node_cachep, gfp);
+ xas->xa_alloc = kmem_cache_alloc_lru(radix_tree_node_cachep, xas->xa_lru, gfp);
if (!xas->xa_alloc)
return false;
xas->xa_alloc->parent = NULL;
@@ -334,10 +334,10 @@ static bool __xas_nomem(struct xa_state *xas, gfp_t gfp)
gfp |= __GFP_ACCOUNT;
if (gfpflags_allow_blocking(gfp)) {
xas_unlock_type(xas, lock_type);
- xas->xa_alloc = kmem_cache_alloc(radix_tree_node_cachep, gfp);
+ xas->xa_alloc = kmem_cache_alloc_lru(radix_tree_node_cachep, xas->xa_lru, gfp);
xas_lock_type(xas, lock_type);
} else {
- xas->xa_alloc = kmem_cache_alloc(radix_tree_node_cachep, gfp);
+ xas->xa_alloc = kmem_cache_alloc_lru(radix_tree_node_cachep, xas->xa_lru, gfp);
}
if (!xas->xa_alloc)
return false;
@@ -371,7 +371,7 @@ static void *xas_alloc(struct xa_state *xas, unsigned int shift)
if (xas->xa->xa_flags & XA_FLAGS_ACCOUNT)
gfp |= __GFP_ACCOUNT;
- node = kmem_cache_alloc(radix_tree_node_cachep, gfp);
+ node = kmem_cache_alloc_lru(radix_tree_node_cachep, xas->xa_lru, gfp);
if (!node) {
xas_set_err(xas, -ENOMEM);
return NULL;
@@ -1014,7 +1014,7 @@ void xas_split_alloc(struct xa_state *xas, void *entry, unsigned int order,
void *sibling = NULL;
struct xa_node *node;
- node = kmem_cache_alloc(radix_tree_node_cachep, gfp);
+ node = kmem_cache_alloc_lru(radix_tree_node_cachep, xas->xa_lru, gfp);
if (!node)
goto nomem;
node->array = xas->xa;
diff --git a/mm/workingset.c b/mm/workingset.c
index 8c03afe1d67c..979c7130c266 100644
--- a/mm/workingset.c
+++ b/mm/workingset.c
@@ -429,7 +429,7 @@ void workingset_activation(struct folio *folio)
* point where they would still be useful.
*/
-static struct list_lru shadow_nodes;
+struct list_lru shadow_nodes;
void workingset_update_node(struct xa_node *node)
{
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v5 08/16] xarray: use kmem_cache_alloc_lru to allocate xa_node
2021-12-20 8:56 ` [PATCH v5 08/16] xarray: use kmem_cache_alloc_lru to allocate xa_node Muchun Song
@ 2022-01-11 19:14 ` Roman Gushchin
0 siblings, 0 replies; 45+ messages in thread
From: Roman Gushchin @ 2022-01-11 19:14 UTC (permalink / raw)
To: Muchun Song
Cc: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, shy828301,
alexs, richard.weiyang, david, trond.myklebust, anna.schumaker,
jaegeuk, chao, kari.argillander, linux-fsdevel, linux-kernel,
linux-mm, linux-nfs, zhengqi.arch, duanxiongchun, fam.zheng,
smuchun
On Mon, Dec 20, 2021 at 04:56:41PM +0800, Muchun Song wrote:
> The workingset will add the xa_node to the shadow_nodes list. So the
> allocation of xa_node should be done by kmem_cache_alloc_lru(). Using
> xas_set_lru() to pass the list_lru which we want to insert xa_node
> into to set up the xa_node reclaim context correctly.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Roman Gushchin <guro@fb.com>
Thanks!
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v5 09/16] mm: memcontrol: move memcg_online_kmem() to mem_cgroup_css_online()
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (7 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 08/16] xarray: use kmem_cache_alloc_lru to allocate xa_node Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2022-01-11 19:17 ` Roman Gushchin
2021-12-20 8:56 ` [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed Muchun Song
` (6 subsequent siblings)
15 siblings, 1 reply; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
It will simplify the code if moving memcg_online_kmem() to
mem_cgroup_css_online() and do not need to set ->kmemcg_id
to -1 to indicate the memcg is offline. In the next patch,
->kmemcg_id will be used to sync list lru reparenting which
requires not to change ->kmemcg_id.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
mm/memcontrol.c | 37 ++++++++++++++++---------------------
1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index d505b43d5f3b..ec7a62f39326 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3604,7 +3604,8 @@ static int memcg_online_kmem(struct mem_cgroup *memcg)
if (cgroup_memory_nokmem)
return 0;
- BUG_ON(memcg->kmemcg_id >= 0);
+ if (unlikely(mem_cgroup_is_root(memcg)))
+ return 0;
memcg_id = memcg_alloc_cache_id();
if (memcg_id < 0)
@@ -3630,7 +3631,10 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
struct mem_cgroup *parent;
int kmemcg_id;
- if (memcg->kmemcg_id == -1)
+ if (cgroup_memory_nokmem)
+ return;
+
+ if (unlikely(mem_cgroup_is_root(memcg)))
return;
parent = parent_mem_cgroup(memcg);
@@ -3640,7 +3644,6 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
memcg_reparent_objcgs(memcg, parent);
kmemcg_id = memcg->kmemcg_id;
- BUG_ON(kmemcg_id < 0);
/*
* After we have finished memcg_reparent_objcgs(), all list_lrus
@@ -3651,7 +3654,6 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
memcg_drain_all_list_lrus(kmemcg_id, parent);
memcg_free_cache_id(kmemcg_id);
- memcg->kmemcg_id = -1;
}
#else
static int memcg_online_kmem(struct mem_cgroup *memcg)
@@ -5159,7 +5161,6 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
{
struct mem_cgroup *parent = mem_cgroup_from_css(parent_css);
struct mem_cgroup *memcg, *old_memcg;
- long error = -ENOMEM;
old_memcg = set_active_memcg(parent);
memcg = mem_cgroup_alloc();
@@ -5188,34 +5189,26 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
return &memcg->css;
}
- /* The following stuff does not apply to the root */
- error = memcg_online_kmem(memcg);
- if (error)
- goto fail;
-
if (cgroup_subsys_on_dfl(memory_cgrp_subsys) && !cgroup_memory_nosocket)
static_branch_inc(&memcg_sockets_enabled_key);
return &memcg->css;
-fail:
- mem_cgroup_id_remove(memcg);
- mem_cgroup_free(memcg);
- return ERR_PTR(error);
}
static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
{
struct mem_cgroup *memcg = mem_cgroup_from_css(css);
+ if (memcg_online_kmem(memcg))
+ goto remove_id;
+
/*
* A memcg must be visible for expand_shrinker_info()
* by the time the maps are allocated. So, we allocate maps
* here, when for_each_mem_cgroup() can't skip it.
*/
- if (alloc_shrinker_info(memcg)) {
- mem_cgroup_id_remove(memcg);
- return -ENOMEM;
- }
+ if (alloc_shrinker_info(memcg))
+ goto offline_kmem;
/* Online state pins memcg ID, memcg ID pins CSS */
refcount_set(&memcg->id.ref, 1);
@@ -5225,6 +5218,11 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css)
queue_delayed_work(system_unbound_wq, &stats_flush_dwork,
2UL*HZ);
return 0;
+offline_kmem:
+ memcg_offline_kmem(memcg);
+remove_id:
+ mem_cgroup_id_remove(memcg);
+ return -ENOMEM;
}
static void mem_cgroup_css_offline(struct cgroup_subsys_state *css)
@@ -5282,9 +5280,6 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
cancel_work_sync(&memcg->high_work);
mem_cgroup_remove_from_trees(memcg);
free_shrinker_info(memcg);
-
- /* Need to offline kmem if online_css() fails */
- memcg_offline_kmem(memcg);
mem_cgroup_free(memcg);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v5 09/16] mm: memcontrol: move memcg_online_kmem() to mem_cgroup_css_online()
2021-12-20 8:56 ` [PATCH v5 09/16] mm: memcontrol: move memcg_online_kmem() to mem_cgroup_css_online() Muchun Song
@ 2022-01-11 19:17 ` Roman Gushchin
0 siblings, 0 replies; 45+ messages in thread
From: Roman Gushchin @ 2022-01-11 19:17 UTC (permalink / raw)
To: Muchun Song
Cc: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, shy828301,
alexs, richard.weiyang, david, trond.myklebust, anna.schumaker,
jaegeuk, chao, kari.argillander, linux-fsdevel, linux-kernel,
linux-mm, linux-nfs, zhengqi.arch, duanxiongchun, fam.zheng,
smuchun
On Mon, Dec 20, 2021 at 04:56:42PM +0800, Muchun Song wrote:
> It will simplify the code if moving memcg_online_kmem() to
> mem_cgroup_css_online() and do not need to set ->kmemcg_id
> to -1 to indicate the memcg is offline. In the next patch,
> ->kmemcg_id will be used to sync list lru reparenting which
> requires not to change ->kmemcg_id.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Nice!
Acked-by: Roman Gushchin <guro@fb.com>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (8 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 09/16] mm: memcontrol: move memcg_online_kmem() to mem_cgroup_css_online() Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2022-01-06 11:00 ` Michal Koutný
2022-01-11 20:00 ` Roman Gushchin
2021-12-20 8:56 ` [PATCH v5 11/16] mm: list_lru: rename memcg_drain_all_list_lrus to memcg_reparent_list_lrus Muchun Song
` (5 subsequent siblings)
15 siblings, 2 replies; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
In our server, we found a suspected memory leak problem. The kmalloc-32
consumes more than 6GB of memory. Other kmem_caches consume less than
2GB memory.
After our in-depth analysis, the memory consumption of kmalloc-32 slab
cache is the cause of list_lru_one allocation.
crash> p memcg_nr_cache_ids
memcg_nr_cache_ids = $2 = 24574
memcg_nr_cache_ids is very large and memory consumption of each list_lru
can be calculated with the following formula.
num_numa_node * memcg_nr_cache_ids * 32 (kmalloc-32)
There are 4 numa nodes in our system, so each list_lru consumes ~3MB.
crash> list super_blocks | wc -l
952
Every mount will register 2 list lrus, one is for inode, another is for
dentry. There are 952 super_blocks. So the total memory is 952 * 2 * 3
MB (~5.6GB). But the number of memory cgroup is less than 500. So I
guess more than 12286 containers have been deployed on this machine (I
do not know why there are so many containers, it may be a user's bug or
the user really want to do that). And memcg_nr_cache_ids has not been
reduced to a suitable value. This can waste a lot of memory.
Now the infrastructure for dynamic list_lru_one allocation is ready, so
remove statically allocated memory code to save memory.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
include/linux/list_lru.h | 7 +--
mm/list_lru.c | 121 ++++++++++++++++++++++++++---------------------
mm/memcontrol.c | 6 ++-
3 files changed, 77 insertions(+), 57 deletions(-)
diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
index ab912c49334f..c36db6dc2a65 100644
--- a/include/linux/list_lru.h
+++ b/include/linux/list_lru.h
@@ -32,14 +32,15 @@ struct list_lru_one {
};
struct list_lru_per_memcg {
+ struct rcu_head rcu;
/* array of per cgroup per node lists, indexed by node id */
- struct list_lru_one node[0];
+ struct list_lru_one node[];
};
struct list_lru_memcg {
struct rcu_head rcu;
/* array of per cgroup lists, indexed by memcg_cache_id */
- struct list_lru_per_memcg *mlru[];
+ struct list_lru_per_memcg __rcu *mlru[];
};
struct list_lru_node {
@@ -77,7 +78,7 @@ int __list_lru_init(struct list_lru *lru, bool memcg_aware,
int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
gfp_t gfp);
int memcg_update_all_list_lrus(int num_memcgs);
-void memcg_drain_all_list_lrus(int src_idx, struct mem_cgroup *dst_memcg);
+void memcg_drain_all_list_lrus(struct mem_cgroup *src, struct mem_cgroup *dst);
/**
* list_lru_add: add an element to the lru list's tail
diff --git a/mm/list_lru.c b/mm/list_lru.c
index bffa80527723..fc938d8ff48f 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -60,8 +60,12 @@ list_lru_from_memcg_idx(struct list_lru *lru, int nid, int idx)
* from relocation (see memcg_update_list_lru).
*/
mlrus = rcu_dereference_check(lru->mlrus, lockdep_is_held(&nlru->lock));
- if (mlrus && idx >= 0)
- return &mlrus->mlru[idx]->node[nid];
+ if (mlrus && idx >= 0) {
+ struct list_lru_per_memcg *mlru;
+
+ mlru = rcu_dereference_check(mlrus->mlru[idx], true);
+ return mlru ? &mlru->node[nid] : NULL;
+ }
return &nlru->lru;
}
@@ -188,7 +192,7 @@ unsigned long list_lru_count_one(struct list_lru *lru,
rcu_read_lock();
l = list_lru_from_memcg_idx(lru, nid, memcg_cache_id(memcg));
- count = READ_ONCE(l->nr_items);
+ count = l ? READ_ONCE(l->nr_items) : 0;
rcu_read_unlock();
if (unlikely(count < 0))
@@ -217,8 +221,11 @@ __list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
struct list_head *item, *n;
unsigned long isolated = 0;
- l = list_lru_from_memcg_idx(lru, nid, memcg_idx);
restart:
+ l = list_lru_from_memcg_idx(lru, nid, memcg_idx);
+ if (!l)
+ goto out;
+
list_for_each_safe(item, n, &l->list) {
enum lru_status ret;
@@ -262,6 +269,7 @@ __list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx,
BUG();
}
}
+out:
return isolated;
}
@@ -354,20 +362,25 @@ static struct list_lru_per_memcg *memcg_init_list_lru_one(gfp_t gfp)
return mlru;
}
-static int memcg_init_list_lru_range(struct list_lru_memcg *mlrus,
- int begin, int end)
+static void memcg_list_lru_free(struct list_lru *lru, int src_idx)
{
- int i;
+ struct list_lru_memcg *mlrus;
+ struct list_lru_per_memcg *mlru;
- for (i = begin; i < end; i++) {
- mlrus->mlru[i] = memcg_init_list_lru_one(GFP_KERNEL);
- if (!mlrus->mlru[i])
- goto fail;
- }
- return 0;
-fail:
- memcg_destroy_list_lru_range(mlrus, begin, i);
- return -ENOMEM;
+ spin_lock_irq(&lru->lock);
+ mlrus = rcu_dereference_protected(lru->mlrus, true);
+ mlru = rcu_dereference_protected(mlrus->mlru[src_idx], true);
+ rcu_assign_pointer(mlrus->mlru[src_idx], NULL);
+ spin_unlock_irq(&lru->lock);
+
+ /*
+ * The __list_lru_walk_one() can walk the list of this node.
+ * We need kvfree_rcu() here. And the walking of the list
+ * is under lru->node[nid]->lock, which can serve as a RCU
+ * read-side critical section.
+ */
+ if (mlru)
+ kvfree_rcu(mlru, rcu);
}
static int memcg_init_list_lru(struct list_lru *lru, bool memcg_aware)
@@ -381,14 +394,10 @@ static int memcg_init_list_lru(struct list_lru *lru, bool memcg_aware)
spin_lock_init(&lru->lock);
- mlrus = kvmalloc(struct_size(mlrus, mlru, size), GFP_KERNEL);
+ mlrus = kvzalloc(struct_size(mlrus, mlru, size), GFP_KERNEL);
if (!mlrus)
return -ENOMEM;
- if (memcg_init_list_lru_range(mlrus, 0, size)) {
- kvfree(mlrus);
- return -ENOMEM;
- }
RCU_INIT_POINTER(lru->mlrus, mlrus);
return 0;
@@ -422,13 +431,9 @@ static int memcg_update_list_lru(struct list_lru *lru, int old_size, int new_siz
if (!new)
return -ENOMEM;
- if (memcg_init_list_lru_range(new, old_size, new_size)) {
- kvfree(new);
- return -ENOMEM;
- }
-
spin_lock_irq(&lru->lock);
memcpy(&new->mlru, &old->mlru, flex_array_size(new, mlru, old_size));
+ memset(&new->mlru[old_size], 0, flex_array_size(new, mlru, new_size - old_size));
rcu_assign_pointer(lru->mlrus, new);
spin_unlock_irq(&lru->lock);
@@ -436,20 +441,6 @@ static int memcg_update_list_lru(struct list_lru *lru, int old_size, int new_siz
return 0;
}
-static void memcg_cancel_update_list_lru(struct list_lru *lru,
- int old_size, int new_size)
-{
- struct list_lru_memcg *mlrus;
-
- mlrus = rcu_dereference_protected(lru->mlrus,
- lockdep_is_held(&list_lrus_mutex));
- /*
- * Do not bother shrinking the array back to the old size, because we
- * cannot handle allocation failures here.
- */
- memcg_destroy_list_lru_range(mlrus, old_size, new_size);
-}
-
int memcg_update_all_list_lrus(int new_size)
{
int ret = 0;
@@ -460,15 +451,10 @@ int memcg_update_all_list_lrus(int new_size)
list_for_each_entry(lru, &memcg_list_lrus, list) {
ret = memcg_update_list_lru(lru, old_size, new_size);
if (ret)
- goto fail;
+ break;
}
-out:
mutex_unlock(&list_lrus_mutex);
return ret;
-fail:
- list_for_each_entry_continue_reverse(lru, &memcg_list_lrus, list)
- memcg_cancel_update_list_lru(lru, old_size, new_size);
- goto out;
}
static void memcg_drain_list_lru_node(struct list_lru *lru, int nid,
@@ -485,6 +471,8 @@ static void memcg_drain_list_lru_node(struct list_lru *lru, int nid,
spin_lock_irq(&nlru->lock);
src = list_lru_from_memcg_idx(lru, nid, src_idx);
+ if (!src)
+ goto out;
dst = list_lru_from_memcg_idx(lru, nid, dst_idx);
list_splice_init(&src->list, &dst->list);
@@ -494,7 +482,7 @@ static void memcg_drain_list_lru_node(struct list_lru *lru, int nid,
set_shrinker_bit(dst_memcg, nid, lru_shrinker_id(lru));
src->nr_items = 0;
}
-
+out:
spin_unlock_irq(&nlru->lock);
}
@@ -505,15 +493,41 @@ static void memcg_drain_list_lru(struct list_lru *lru,
for_each_node(i)
memcg_drain_list_lru_node(lru, i, src_idx, dst_memcg);
+
+ memcg_list_lru_free(lru, src_idx);
}
-void memcg_drain_all_list_lrus(int src_idx, struct mem_cgroup *dst_memcg)
+void memcg_drain_all_list_lrus(struct mem_cgroup *src, struct mem_cgroup *dst)
{
+ struct cgroup_subsys_state *css;
struct list_lru *lru;
+ int src_idx = src->kmemcg_id;
+
+ /*
+ * Change kmemcg_id of this cgroup and all its descendants to the
+ * parent's id, and then move all entries from this cgroup's list_lrus
+ * to ones of the parent.
+ *
+ * After we have finished, all list_lrus corresponding to this cgroup
+ * are guaranteed to remain empty. So we can safely free this cgroup's
+ * list lrus in memcg_list_lru_free().
+ *
+ * Changing ->kmemcg_id to the parent can prevent memcg_list_lru_alloc()
+ * from allocating list lrus for this cgroup after memcg_list_lru_free()
+ * call.
+ */
+ rcu_read_lock();
+ css_for_each_descendant_pre(css, &src->css) {
+ struct mem_cgroup *memcg;
+
+ memcg = mem_cgroup_from_css(css);
+ memcg->kmemcg_id = dst->kmemcg_id;
+ }
+ rcu_read_unlock();
mutex_lock(&list_lrus_mutex);
list_for_each_entry(lru, &memcg_list_lrus, list)
- memcg_drain_list_lru(lru, src_idx, dst_memcg);
+ memcg_drain_list_lru(lru, src_idx, dst);
mutex_unlock(&list_lrus_mutex);
}
@@ -528,7 +542,7 @@ static bool memcg_list_lru_allocated(struct mem_cgroup *memcg,
return true;
rcu_read_lock();
- allocated = !!rcu_dereference(lru->mlrus)->mlru[idx];
+ allocated = !!rcu_access_pointer(rcu_dereference(lru->mlrus)->mlru[idx]);
rcu_read_unlock();
return allocated;
@@ -576,11 +590,12 @@ int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
mlrus = rcu_dereference_protected(lru->mlrus, true);
while (i--) {
int index = table[i].memcg->kmemcg_id;
+ struct list_lru_per_memcg *mlru = table[i].mlru;
- if (mlrus->mlru[index])
- kfree(table[i].mlru);
+ if (index < 0 || rcu_dereference_protected(mlrus->mlru[index], true))
+ kfree(mlru);
else
- mlrus->mlru[index] = table[i].mlru;
+ rcu_assign_pointer(mlrus->mlru[index], mlru);
}
spin_unlock_irqrestore(&lru->lock, flags);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ec7a62f39326..94d8f742c32e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3643,6 +3643,10 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
memcg_reparent_objcgs(memcg, parent);
+ /*
+ * memcg_drain_all_list_lrus() can change memcg->kmemcg_id.
+ * Cache it to local @kmemcg_id.
+ */
kmemcg_id = memcg->kmemcg_id;
/*
@@ -3651,7 +3655,7 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
* The ordering is imposed by list_lru_node->lock taken by
* memcg_drain_all_list_lrus().
*/
- memcg_drain_all_list_lrus(kmemcg_id, parent);
+ memcg_drain_all_list_lrus(memcg, parent);
memcg_free_cache_id(kmemcg_id);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed
2021-12-20 8:56 ` [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed Muchun Song
@ 2022-01-06 11:00 ` Michal Koutný
2022-01-12 13:22 ` Muchun Song
2022-01-11 20:00 ` Roman Gushchin
1 sibling, 1 reply; 45+ messages in thread
From: Michal Koutný @ 2022-01-06 11:00 UTC (permalink / raw)
To: Muchun Song
Cc: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander, linux-fsdevel,
linux-kernel, linux-mm, linux-nfs, zhengqi.arch, duanxiongchun,
fam.zheng, smuchun
On Mon, Dec 20, 2021 at 04:56:43PM +0800, Muchun Song <songmuchun@bytedance.com> wrote:
(Thanks for pointing me here.)
> -void memcg_drain_all_list_lrus(int src_idx, struct mem_cgroup *dst_memcg)
> +void memcg_drain_all_list_lrus(struct mem_cgroup *src, struct mem_cgroup *dst)
> {
> + struct cgroup_subsys_state *css;
> struct list_lru *lru;
> + int src_idx = src->kmemcg_id;
> +
> + /*
> + * Change kmemcg_id of this cgroup and all its descendants to the
> + * parent's id, and then move all entries from this cgroup's list_lrus
> + * to ones of the parent.
> + *
> + * After we have finished, all list_lrus corresponding to this cgroup
> + * are guaranteed to remain empty. So we can safely free this cgroup's
> + * list lrus in memcg_list_lru_free().
> + *
> + * Changing ->kmemcg_id to the parent can prevent memcg_list_lru_alloc()
> + * from allocating list lrus for this cgroup after memcg_list_lru_free()
> + * call.
> + */
> + rcu_read_lock();
> + css_for_each_descendant_pre(css, &src->css) {
> + struct mem_cgroup *memcg;
> +
> + memcg = mem_cgroup_from_css(css);
> + memcg->kmemcg_id = dst->kmemcg_id;
> + }
> + rcu_read_unlock();
Do you envision using this function anywhere else beside offlining?
If not, you shouldn't need traversing whole subtree because normally
parents are offlined only after children (see cgroup_subsys_state.online_cnt).
>
> mutex_lock(&list_lrus_mutex);
> list_for_each_entry(lru, &memcg_list_lrus, list)
> - memcg_drain_list_lru(lru, src_idx, dst_memcg);
> + memcg_drain_list_lru(lru, src_idx, dst);
> mutex_unlock(&list_lrus_mutex);
If you do, then here you only drain list_lru of the subtree root but not
the descendants anymore.
So I do get that mem_cgroup.kmemcg_id refernces the "effective"
kmemcg_id after offlining, so that proper list_lrus are used afterwards.
I wonder -- is this necessary when objcgs are reparented too? IOW would
anyone query the offlined child's kmemcg_id?
(Maybe it's worth explaining better in the commit message, I think even
current approach is OK (better safe than sorry).)
Thanks,
Michal
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed
2022-01-06 11:00 ` Michal Koutný
@ 2022-01-12 13:22 ` Muchun Song
2022-01-13 13:32 ` Michal Koutný
0 siblings, 1 reply; 45+ messages in thread
From: Muchun Song @ 2022-01-12 13:22 UTC (permalink / raw)
To: Michal Koutný
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Roman Gushchin, Yang Shi,
Alex Shi, Wei Yang, Dave Chinner, trond.myklebust,
anna.schumaker, jaegeuk, chao, Kari Argillander, linux-fsdevel,
LKML, Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song
On Thu, Jan 6, 2022 at 7:00 PM Michal Koutný <mkoutny@suse.com> wrote:
>
> On Mon, Dec 20, 2021 at 04:56:43PM +0800, Muchun Song <songmuchun@bytedance.com> wrote:
> (Thanks for pointing me here.)
>
> > -void memcg_drain_all_list_lrus(int src_idx, struct mem_cgroup *dst_memcg)
> > +void memcg_drain_all_list_lrus(struct mem_cgroup *src, struct mem_cgroup *dst)
> > {
> > + struct cgroup_subsys_state *css;
> > struct list_lru *lru;
> > + int src_idx = src->kmemcg_id;
> > +
> > + /*
> > + * Change kmemcg_id of this cgroup and all its descendants to the
> > + * parent's id, and then move all entries from this cgroup's list_lrus
> > + * to ones of the parent.
> > + *
> > + * After we have finished, all list_lrus corresponding to this cgroup
> > + * are guaranteed to remain empty. So we can safely free this cgroup's
> > + * list lrus in memcg_list_lru_free().
> > + *
> > + * Changing ->kmemcg_id to the parent can prevent memcg_list_lru_alloc()
> > + * from allocating list lrus for this cgroup after memcg_list_lru_free()
> > + * call.
> > + */
> > + rcu_read_lock();
> > + css_for_each_descendant_pre(css, &src->css) {
> > + struct mem_cgroup *memcg;
> > +
> > + memcg = mem_cgroup_from_css(css);
> > + memcg->kmemcg_id = dst->kmemcg_id;
> > + }
> > + rcu_read_unlock();
>
> Do you envision using this function anywhere else beside offlining?
> If not, you shouldn't need traversing whole subtree because normally
> parents are offlined only after children (see cgroup_subsys_state.online_cnt).
>
> >
> > mutex_lock(&list_lrus_mutex);
> > list_for_each_entry(lru, &memcg_list_lrus, list)
> > - memcg_drain_list_lru(lru, src_idx, dst_memcg);
> > + memcg_drain_list_lru(lru, src_idx, dst);
> > mutex_unlock(&list_lrus_mutex);
>
> If you do, then here you only drain list_lru of the subtree root but not
> the descendants anymore.
>
> So I do get that mem_cgroup.kmemcg_id refernces the "effective"
> kmemcg_id after offlining, so that proper list_lrus are used afterwards.
>
> I wonder -- is this necessary when objcgs are reparented too? IOW would
> anyone query the offlined child's kmemcg_id?
> (Maybe it's worth explaining better in the commit message, I think even
> current approach is OK (better safe than sorry).)
>
Sorry for the late reply.
Image a bunch of memcg as follows. C's parent is B, B's parent is A and
A's parent is root. The numbers in parentheses are their kmemcg_id
respectively.
root(-1) -> A(0) -> B(1) -> C(2)
CPU0: CPU1:
memcg_list_lru_alloc(C)
memcg_drain_all_list_lrus(C)
memcg_drain_all_list_lrus(B)
// Now C and B are offline. The
// kmemcg_id becomes the following if
// we do not the kmemcg_id of its
// descendants in
// memcg_drain_all_list_lrus().
//
// root(-1) -> A(0) -> B(0) -> C(1)
for (i = 0; memcg; memcg = parent_mem_cgroup(memcg), i++) {
// allocate struct list_lru_per_memcg for memcg C
table[i].mlru = memcg_init_list_lru_one(gfp);
}
spin_lock_irqsave(&lru->lock, flags);
while (i--) {
// here index = 1
int index = table[i].memcg->kmemcg_id;
struct list_lru_per_memcg *mlru = table[i].mlru;
if (index < 0 || rcu_dereference_protected(mlrus->mlru[index], true))
kfree(mlru);
else
// mlrus->mlru[index] will be assigned a new value regardless
// memcg C is already offline.
rcu_assign_pointer(mlrus->mlru[index], mlru);
}
spin_unlock_irqrestore(&lru->lock, flags);
So changing ->kmemcg_id of all its descendants can prevent
memcg_list_lru_alloc() from allocating list lrus for the offlined
cgroup after memcg_list_lru_free() calling.
Thanks.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed
2022-01-12 13:22 ` Muchun Song
@ 2022-01-13 13:32 ` Michal Koutný
2022-01-18 12:05 ` Muchun Song
0 siblings, 1 reply; 45+ messages in thread
From: Michal Koutný @ 2022-01-13 13:32 UTC (permalink / raw)
To: Muchun Song
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Roman Gushchin, Yang Shi,
Alex Shi, Wei Yang, Dave Chinner, trond.myklebust,
anna.schumaker, jaegeuk, chao, Kari Argillander, linux-fsdevel,
LKML, Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song
On Wed, Jan 12, 2022 at 09:22:36PM +0800, Muchun Song <songmuchun@bytedance.com> wrote:
> root(-1) -> A(0) -> B(1) -> C(2)
>
> CPU0: CPU1:
> memcg_list_lru_alloc(C)
> memcg_drain_all_list_lrus(C)
> memcg_drain_all_list_lrus(B)
> // Now C and B are offline. The
> // kmemcg_id becomes the following if
> // we do not the kmemcg_id of its
> // descendants in
> // memcg_drain_all_list_lrus().
> //
> // root(-1) -> A(0) -> B(0) -> C(1)
>
> for (i = 0; memcg; memcg = parent_mem_cgroup(memcg), i++) {
> // allocate struct list_lru_per_memcg for memcg C
> table[i].mlru = memcg_init_list_lru_one(gfp);
> }
>
> spin_lock_irqsave(&lru->lock, flags);
> while (i--) {
> // here index = 1
> int index = table[i].memcg->kmemcg_id;
>
> struct list_lru_per_memcg *mlru = table[i].mlru;
> if (index < 0 || rcu_dereference_protected(mlrus->mlru[index], true))
> kfree(mlru);
> else
> // mlrus->mlru[index] will be assigned a new value regardless
> // memcg C is already offline.
> rcu_assign_pointer(mlrus->mlru[index], mlru);
> }
> spin_unlock_irqrestore(&lru->lock, flags);
>
> So changing ->kmemcg_id of all its descendants can prevent
> memcg_list_lru_alloc() from allocating list lrus for the offlined
> cgroup after memcg_list_lru_free() calling.
Thanks for the illustrative example. I can see how this can be a problem
in a general call of memcg_list_lru_alloc(C).
However, the code, as I understand it, resolves the memcg to which lru
allocation should be associated via get_mem_cgroup_from_objcg() and
memcg_reparent_list_lrus(C) comes after memcg_reparent_objcgs(C, B),
i.e. the allocation would target B (or even A if after
memcg_reparent_objcgs(B, A))?
It seems to me like "wasting" the existing objcg reparenting mechanism.
Or what do you think could be a problem relying on it?
Thanks,
Michal
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed
2022-01-13 13:32 ` Michal Koutný
@ 2022-01-18 12:05 ` Muchun Song
2022-01-19 9:33 ` Michal Koutný
0 siblings, 1 reply; 45+ messages in thread
From: Muchun Song @ 2022-01-18 12:05 UTC (permalink / raw)
To: Michal Koutný
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Roman Gushchin, Yang Shi,
Alex Shi, Wei Yang, Dave Chinner, trond.myklebust,
anna.schumaker, jaegeuk, chao, Kari Argillander, linux-fsdevel,
LKML, Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song
On Thu, Jan 13, 2022 at 9:32 PM Michal Koutný <mkoutny@suse.com> wrote:
>
> On Wed, Jan 12, 2022 at 09:22:36PM +0800, Muchun Song <songmuchun@bytedance.com> wrote:
> > root(-1) -> A(0) -> B(1) -> C(2)
> >
> > CPU0: CPU1:
> > memcg_list_lru_alloc(C)
> > memcg_drain_all_list_lrus(C)
> > memcg_drain_all_list_lrus(B)
> > // Now C and B are offline. The
> > // kmemcg_id becomes the following if
> > // we do not the kmemcg_id of its
> > // descendants in
> > // memcg_drain_all_list_lrus().
> > //
> > // root(-1) -> A(0) -> B(0) -> C(1)
> >
> > for (i = 0; memcg; memcg = parent_mem_cgroup(memcg), i++) {
> > // allocate struct list_lru_per_memcg for memcg C
> > table[i].mlru = memcg_init_list_lru_one(gfp);
> > }
> >
> > spin_lock_irqsave(&lru->lock, flags);
> > while (i--) {
> > // here index = 1
> > int index = table[i].memcg->kmemcg_id;
> >
> > struct list_lru_per_memcg *mlru = table[i].mlru;
> > if (index < 0 || rcu_dereference_protected(mlrus->mlru[index], true))
> > kfree(mlru);
> > else
> > // mlrus->mlru[index] will be assigned a new value regardless
> > // memcg C is already offline.
> > rcu_assign_pointer(mlrus->mlru[index], mlru);
> > }
> > spin_unlock_irqrestore(&lru->lock, flags);
> >
>
> > So changing ->kmemcg_id of all its descendants can prevent
> > memcg_list_lru_alloc() from allocating list lrus for the offlined
> > cgroup after memcg_list_lru_free() calling.
>
> Thanks for the illustrative example. I can see how this can be a problem
> in a general call of memcg_list_lru_alloc(C).
>
> However, the code, as I understand it, resolves the memcg to which lru
> allocation should be associated via get_mem_cgroup_from_objcg() and
> memcg_reparent_list_lrus(C) comes after memcg_reparent_objcgs(C, B),
> i.e. the allocation would target B (or even A if after
> memcg_reparent_objcgs(B, A))?
>
> It seems to me like "wasting" the existing objcg reparenting mechanism.
> Or what do you think could be a problem relying on it?
>
I have thought about this. It's a little different to rely on objcg
reparenting since the user can get memcg from objcg and
then does not realize the memcg has reparented. Although it
can check memcg->objcg to know whether the memcg is
reparented, it should also prevent this memcg from being
reparented throughout memcg_list_lru_alloc(). Maybe
holding css_set_lock can do that. I do not think this
is a good choice. Do you have any thoughts about this?
Thanks.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed
2022-01-18 12:05 ` Muchun Song
@ 2022-01-19 9:33 ` Michal Koutný
2022-01-21 5:28 ` Muchun Song
0 siblings, 1 reply; 45+ messages in thread
From: Michal Koutný @ 2022-01-19 9:33 UTC (permalink / raw)
To: Muchun Song
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Roman Gushchin, Yang Shi,
Alex Shi, Wei Yang, Dave Chinner, trond.myklebust,
anna.schumaker, jaegeuk, chao, Kari Argillander, linux-fsdevel,
LKML, Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song
On Tue, Jan 18, 2022 at 08:05:44PM +0800, Muchun Song <songmuchun@bytedance.com> wrote:
> I have thought about this. It's a little different to rely on objcg
> reparenting since the user can get memcg from objcg and
> then does not realize the memcg has reparented.
When you pointed that out, I'm now also wondering how
memcg_list_lru_alloc() would be synchronized against
reparenting/renumbering of kmemcg_ids. What I suspect is that newly
allocated mlru may be stored into the xarray with a stale kmemcg_id.
> Maybe holding css_set_lock can do that. I do not think this
> is a good choice.
I agree, it doesn't sound well.
> Do you have any thoughts about this?
Thoughts / questions of what I don't undestand well:
- Why do you allocate mlrus for all ancestors in memcg_list_lru_alloc()?
- It'd be sufficient to allocate just for the current memcg.
- Possibly allocate ancestors upon reparenting (to simplify the
allocation from slab_pre_alloc_hook itself).
- What is the per-kmemcg_id lookup good for?
- I observe most calls of list_lru_from_memcg_idx() come from callers
that know memcg (or even objcg).
- The non-specific use case seems list_lru_walk_node() working with
per-node and not per-memcg projection.
- Consequently that is only used over all nodes anyway
(list_lru_walk().
- The idea behind this question is -- attach the list_lrus to
obj_cgroup (and decomission the kmemcg_id completely).
(Not necessarily part of this series but independent approach.)
Thanks,
Michal
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed
2022-01-19 9:33 ` Michal Koutný
@ 2022-01-21 5:28 ` Muchun Song
0 siblings, 0 replies; 45+ messages in thread
From: Muchun Song @ 2022-01-21 5:28 UTC (permalink / raw)
To: Michal Koutný
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Roman Gushchin, Yang Shi,
Alex Shi, Wei Yang, Dave Chinner, trond.myklebust,
anna.schumaker, jaegeuk, chao, Kari Argillander, linux-fsdevel,
LKML, Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song
On Wed, Jan 19, 2022 at 5:33 PM Michal Koutný <mkoutny@suse.com> wrote:
>
> On Tue, Jan 18, 2022 at 08:05:44PM +0800, Muchun Song <songmuchun@bytedance.com> wrote:
> > I have thought about this. It's a little different to rely on objcg
> > reparenting since the user can get memcg from objcg and
> > then does not realize the memcg has reparented.
>
> When you pointed that out, I'm now also wondering how
> memcg_list_lru_alloc() would be synchronized against
> reparenting/renumbering of kmemcg_ids. What I suspect is that newly
> allocated mlru may be stored into the xarray with a stale kmemcg_id.
The synchronization is based on the lock of list_lru->lock.
memcg_list_lru_free() will help us do housekeeping.
>
> > Maybe holding css_set_lock can do that. I do not think this
> > is a good choice.
>
> I agree, it doesn't sound well.
>
> > Do you have any thoughts about this?
>
> Thoughts / questions of what I don't undestand well:
> - Why do you allocate mlrus for all ancestors in memcg_list_lru_alloc()?
It's because we need to be reparenting.
> - It'd be sufficient to allocate just for the current memcg.
> - Possibly allocate ancestors upon reparenting (to simplify the
> allocation from slab_pre_alloc_hook itself).
I agree it is nice to only allocate for current memcg, but
reparenting cannot handle failure of memory allocation.
>
> - What is the per-kmemcg_id lookup good for?
> - I observe most calls of list_lru_from_memcg_idx() come from callers
> that know memcg (or even objcg).
> - The non-specific use case seems list_lru_walk_node() working with
> per-node and not per-memcg projection.
> - Consequently that is only used over all nodes anyway
> (list_lru_walk().
> - The idea behind this question is -- attach the list_lrus to
> obj_cgroup (and decommission the kmemcg_id completely).
> (Not necessarily part of this series but independent approach.)
>
I have some questions about this thought.
We would attach more than one list_lrus to obj_cgroup,
right? How to arrange those list_lrus, array or linked-list?
Thanks.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed
2021-12-20 8:56 ` [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed Muchun Song
2022-01-06 11:00 ` Michal Koutný
@ 2022-01-11 20:00 ` Roman Gushchin
2022-01-12 4:48 ` Muchun Song
1 sibling, 1 reply; 45+ messages in thread
From: Roman Gushchin @ 2022-01-11 20:00 UTC (permalink / raw)
To: Muchun Song
Cc: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, shy828301,
alexs, richard.weiyang, david, trond.myklebust, anna.schumaker,
jaegeuk, chao, kari.argillander, linux-fsdevel, linux-kernel,
linux-mm, linux-nfs, zhengqi.arch, duanxiongchun, fam.zheng,
smuchun
On Mon, Dec 20, 2021 at 04:56:43PM +0800, Muchun Song wrote:
> In our server, we found a suspected memory leak problem. The kmalloc-32
> consumes more than 6GB of memory. Other kmem_caches consume less than
> 2GB memory.
>
> After our in-depth analysis, the memory consumption of kmalloc-32 slab
> cache is the cause of list_lru_one allocation.
>
> crash> p memcg_nr_cache_ids
> memcg_nr_cache_ids = $2 = 24574
>
> memcg_nr_cache_ids is very large and memory consumption of each list_lru
> can be calculated with the following formula.
>
> num_numa_node * memcg_nr_cache_ids * 32 (kmalloc-32)
>
> There are 4 numa nodes in our system, so each list_lru consumes ~3MB.
>
> crash> list super_blocks | wc -l
> 952
>
> Every mount will register 2 list lrus, one is for inode, another is for
> dentry. There are 952 super_blocks. So the total memory is 952 * 2 * 3
> MB (~5.6GB). But the number of memory cgroup is less than 500. So I
> guess more than 12286 containers have been deployed on this machine (I
> do not know why there are so many containers, it may be a user's bug or
> the user really want to do that). And memcg_nr_cache_ids has not been
> reduced to a suitable value. This can waste a lot of memory.
But on the other side you increase the size of struct list_lru_per_memcg,
so if number of cgroups is close to memcg_nr_cache_ids, we can actually
waste more memory. I'm not saying the change is not worth it, but would be
nice to add some real-world numbers.
Or it's all irrelevant and is done as a preparation to the conversion to xarray?
If so, please, make it clear.
Thanks!
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed
2022-01-11 20:00 ` Roman Gushchin
@ 2022-01-12 4:48 ` Muchun Song
0 siblings, 0 replies; 45+ messages in thread
From: Muchun Song @ 2022-01-12 4:48 UTC (permalink / raw)
To: Roman Gushchin
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Yang Shi, Alex Shi, Wei Yang,
Dave Chinner, trond.myklebust, anna.schumaker, jaegeuk, chao,
Kari Argillander, linux-fsdevel, LKML,
Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song
On Wed, Jan 12, 2022 at 4:00 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Mon, Dec 20, 2021 at 04:56:43PM +0800, Muchun Song wrote:
> > In our server, we found a suspected memory leak problem. The kmalloc-32
> > consumes more than 6GB of memory. Other kmem_caches consume less than
> > 2GB memory.
> >
> > After our in-depth analysis, the memory consumption of kmalloc-32 slab
> > cache is the cause of list_lru_one allocation.
> >
> > crash> p memcg_nr_cache_ids
> > memcg_nr_cache_ids = $2 = 24574
> >
> > memcg_nr_cache_ids is very large and memory consumption of each list_lru
> > can be calculated with the following formula.
> >
> > num_numa_node * memcg_nr_cache_ids * 32 (kmalloc-32)
> >
> > There are 4 numa nodes in our system, so each list_lru consumes ~3MB.
> >
> > crash> list super_blocks | wc -l
> > 952
> >
> > Every mount will register 2 list lrus, one is for inode, another is for
> > dentry. There are 952 super_blocks. So the total memory is 952 * 2 * 3
> > MB (~5.6GB). But the number of memory cgroup is less than 500. So I
> > guess more than 12286 containers have been deployed on this machine (I
> > do not know why there are so many containers, it may be a user's bug or
> > the user really want to do that). And memcg_nr_cache_ids has not been
> > reduced to a suitable value. This can waste a lot of memory.
>
> But on the other side you increase the size of struct list_lru_per_memcg,
> so if number of cgroups is close to memcg_nr_cache_ids, we can actually
> waste more memory.
The saving comes from the fact that we currently allocate scope for every
memcg to be able to be tracked on every superblock instantiated in the system,
regardless of whether that superblock is even accessible to that memcg.
In theory, increasing struct list_lru_per_memcg is not significant, most
savings is from decreasing the number of allocations of struct
list_lru_per_memcg.
> I'm not saying the change is not worth it, but would be
> nice to add some real-world numbers.
OK. I will do a test.
>
> Or it's all irrelevant and is done as a preparation to the conversion to xarray?
Right. It's also a preparation to transfer to xarray.
> If so, please, make it clear.
Will do.
Thanks.
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v5 11/16] mm: list_lru: rename memcg_drain_all_list_lrus to memcg_reparent_list_lrus
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (9 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2021-12-20 8:56 ` [PATCH v5 12/16] mm: list_lru: replace linear array with xarray Muchun Song
` (4 subsequent siblings)
15 siblings, 0 replies; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
The purpose of the memcg_drain_all_list_lrus() is list_lrus reparenting.
It is very similar to memcg_reparent_objcgs(). Rename it to
memcg_reparent_list_lrus() so that the name can more consistent with
memcg_reparent_objcgs().
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
include/linux/list_lru.h | 2 +-
mm/list_lru.c | 24 ++++++++++++------------
mm/memcontrol.c | 6 +++---
3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
index c36db6dc2a65..4b00fd8cb373 100644
--- a/include/linux/list_lru.h
+++ b/include/linux/list_lru.h
@@ -78,7 +78,7 @@ int __list_lru_init(struct list_lru *lru, bool memcg_aware,
int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
gfp_t gfp);
int memcg_update_all_list_lrus(int num_memcgs);
-void memcg_drain_all_list_lrus(struct mem_cgroup *src, struct mem_cgroup *dst);
+void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *parent);
/**
* list_lru_add: add an element to the lru list's tail
diff --git a/mm/list_lru.c b/mm/list_lru.c
index fc938d8ff48f..488dacd1f8ff 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -457,8 +457,8 @@ int memcg_update_all_list_lrus(int new_size)
return ret;
}
-static void memcg_drain_list_lru_node(struct list_lru *lru, int nid,
- int src_idx, struct mem_cgroup *dst_memcg)
+static void memcg_reparent_list_lru_node(struct list_lru *lru, int nid,
+ int src_idx, struct mem_cgroup *dst_memcg)
{
struct list_lru_node *nlru = &lru->node[nid];
int dst_idx = dst_memcg->kmemcg_id;
@@ -486,22 +486,22 @@ static void memcg_drain_list_lru_node(struct list_lru *lru, int nid,
spin_unlock_irq(&nlru->lock);
}
-static void memcg_drain_list_lru(struct list_lru *lru,
- int src_idx, struct mem_cgroup *dst_memcg)
+static void memcg_reparent_list_lru(struct list_lru *lru,
+ int src_idx, struct mem_cgroup *dst_memcg)
{
int i;
for_each_node(i)
- memcg_drain_list_lru_node(lru, i, src_idx, dst_memcg);
+ memcg_reparent_list_lru_node(lru, i, src_idx, dst_memcg);
memcg_list_lru_free(lru, src_idx);
}
-void memcg_drain_all_list_lrus(struct mem_cgroup *src, struct mem_cgroup *dst)
+void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *parent)
{
struct cgroup_subsys_state *css;
struct list_lru *lru;
- int src_idx = src->kmemcg_id;
+ int src_idx = memcg->kmemcg_id;
/*
* Change kmemcg_id of this cgroup and all its descendants to the
@@ -517,17 +517,17 @@ void memcg_drain_all_list_lrus(struct mem_cgroup *src, struct mem_cgroup *dst)
* call.
*/
rcu_read_lock();
- css_for_each_descendant_pre(css, &src->css) {
- struct mem_cgroup *memcg;
+ css_for_each_descendant_pre(css, &memcg->css) {
+ struct mem_cgroup *child;
- memcg = mem_cgroup_from_css(css);
- memcg->kmemcg_id = dst->kmemcg_id;
+ child = mem_cgroup_from_css(css);
+ child->kmemcg_id = parent->kmemcg_id;
}
rcu_read_unlock();
mutex_lock(&list_lrus_mutex);
list_for_each_entry(lru, &memcg_list_lrus, list)
- memcg_drain_list_lru(lru, src_idx, dst);
+ memcg_reparent_list_lru(lru, src_idx, parent);
mutex_unlock(&list_lrus_mutex);
}
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 94d8f742c32e..a19b1a1c8ea9 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3644,7 +3644,7 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
memcg_reparent_objcgs(memcg, parent);
/*
- * memcg_drain_all_list_lrus() can change memcg->kmemcg_id.
+ * memcg_reparent_list_lrus() can change memcg->kmemcg_id.
* Cache it to local @kmemcg_id.
*/
kmemcg_id = memcg->kmemcg_id;
@@ -3653,9 +3653,9 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
* After we have finished memcg_reparent_objcgs(), all list_lrus
* corresponding to this cgroup are guaranteed to remain empty.
* The ordering is imposed by list_lru_node->lock taken by
- * memcg_drain_all_list_lrus().
+ * memcg_reparent_list_lrus().
*/
- memcg_drain_all_list_lrus(memcg, parent);
+ memcg_reparent_list_lrus(memcg, parent);
memcg_free_cache_id(kmemcg_id);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v5 12/16] mm: list_lru: replace linear array with xarray
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (10 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 11/16] mm: list_lru: rename memcg_drain_all_list_lrus to memcg_reparent_list_lrus Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2021-12-20 8:56 ` [PATCH v5 13/16] mm: memcontrol: reuse memory cgroup ID for kmem ID Muchun Song
` (3 subsequent siblings)
15 siblings, 0 replies; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
If we run 10k containers in the system, the size of the
list_lru_memcg->lrus can be ~96KB per list_lru. When we decrease the
number containers, the size of the array will not be shrinked. It is
not scalable. The xarray is a good choice for this case. We can save
a lot of memory when there are tens of thousands continers in the
system. If we use xarray, we also can remove the logic code of
resizing array, which can simplify the code.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
include/linux/list_lru.h | 13 +--
include/linux/memcontrol.h | 23 ------
mm/list_lru.c | 202 +++++++++++++++------------------------------
mm/memcontrol.c | 77 ++---------------
4 files changed, 73 insertions(+), 242 deletions(-)
diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
index 4b00fd8cb373..572c263561ac 100644
--- a/include/linux/list_lru.h
+++ b/include/linux/list_lru.h
@@ -11,6 +11,7 @@
#include <linux/list.h>
#include <linux/nodemask.h>
#include <linux/shrinker.h>
+#include <linux/xarray.h>
struct mem_cgroup;
@@ -37,12 +38,6 @@ struct list_lru_per_memcg {
struct list_lru_one node[];
};
-struct list_lru_memcg {
- struct rcu_head rcu;
- /* array of per cgroup lists, indexed by memcg_cache_id */
- struct list_lru_per_memcg __rcu *mlru[];
-};
-
struct list_lru_node {
/* protects all lists on the node, including per cgroup */
spinlock_t lock;
@@ -57,10 +52,7 @@ struct list_lru {
struct list_head list;
int shrinker_id;
bool memcg_aware;
- /* protects ->mlrus->mlru[i] */
- spinlock_t lock;
- /* for cgroup aware lrus points to per cgroup lists, otherwise NULL */
- struct list_lru_memcg __rcu *mlrus;
+ struct xarray xa;
#endif
};
@@ -77,7 +69,6 @@ int __list_lru_init(struct list_lru *lru, bool memcg_aware,
int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
gfp_t gfp);
-int memcg_update_all_list_lrus(int num_memcgs);
void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *parent);
/**
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 561ba47760db..3fc437162add 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -1711,18 +1711,6 @@ void obj_cgroup_uncharge(struct obj_cgroup *objcg, size_t size);
extern struct static_key_false memcg_kmem_enabled_key;
-extern int memcg_nr_cache_ids;
-void memcg_get_cache_ids(void);
-void memcg_put_cache_ids(void);
-
-/*
- * Helper macro to loop through all memcg-specific caches. Callers must still
- * check if the cache is valid (it is either valid or NULL).
- * the slab_mutex must be held when looping through those caches
- */
-#define for_each_memcg_cache_index(_idx) \
- for ((_idx) = 0; (_idx) < memcg_nr_cache_ids; (_idx)++)
-
static inline bool memcg_kmem_enabled(void)
{
return static_branch_likely(&memcg_kmem_enabled_key);
@@ -1779,9 +1767,6 @@ static inline void __memcg_kmem_uncharge_page(struct page *page, int order)
{
}
-#define for_each_memcg_cache_index(_idx) \
- for (; NULL; )
-
static inline bool memcg_kmem_enabled(void)
{
return false;
@@ -1792,14 +1777,6 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
return -1;
}
-static inline void memcg_get_cache_ids(void)
-{
-}
-
-static inline void memcg_put_cache_ids(void)
-{
-}
-
static inline struct mem_cgroup *mem_cgroup_from_obj(void *p)
{
return NULL;
diff --git a/mm/list_lru.c b/mm/list_lru.c
index 488dacd1f8ff..8dc1dabb9f05 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -52,21 +52,12 @@ static int lru_shrinker_id(struct list_lru *lru)
static inline struct list_lru_one *
list_lru_from_memcg_idx(struct list_lru *lru, int nid, int idx)
{
- struct list_lru_memcg *mlrus;
- struct list_lru_node *nlru = &lru->node[nid];
+ if (list_lru_memcg_aware(lru) && idx >= 0) {
+ struct list_lru_per_memcg *mlru = xa_load(&lru->xa, idx);
- /*
- * Either lock or RCU protects the array of per cgroup lists
- * from relocation (see memcg_update_list_lru).
- */
- mlrus = rcu_dereference_check(lru->mlrus, lockdep_is_held(&nlru->lock));
- if (mlrus && idx >= 0) {
- struct list_lru_per_memcg *mlru;
-
- mlru = rcu_dereference_check(mlrus->mlru[idx], true);
return mlru ? &mlru->node[nid] : NULL;
}
- return &nlru->lru;
+ return &lru->node[nid].lru;
}
static inline struct list_lru_one *
@@ -77,7 +68,7 @@ list_lru_from_kmem(struct list_lru *lru, int nid, void *ptr,
struct list_lru_one *l = &nlru->lru;
struct mem_cgroup *memcg = NULL;
- if (!lru->mlrus)
+ if (!list_lru_memcg_aware(lru))
goto out;
memcg = mem_cgroup_from_obj(ptr);
@@ -309,16 +300,20 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
unsigned long *nr_to_walk)
{
long isolated = 0;
- int memcg_idx;
isolated += list_lru_walk_one(lru, nid, NULL, isolate, cb_arg,
nr_to_walk);
+
+#ifdef CONFIG_MEMCG_KMEM
if (*nr_to_walk > 0 && list_lru_memcg_aware(lru)) {
- for_each_memcg_cache_index(memcg_idx) {
+ struct list_lru_per_memcg *mlru;
+ unsigned long index;
+
+ xa_for_each(&lru->xa, index, mlru) {
struct list_lru_node *nlru = &lru->node[nid];
spin_lock(&nlru->lock);
- isolated += __list_lru_walk_one(lru, nid, memcg_idx,
+ isolated += __list_lru_walk_one(lru, nid, index,
isolate, cb_arg,
nr_to_walk);
spin_unlock(&nlru->lock);
@@ -327,6 +322,8 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
break;
}
}
+#endif
+
return isolated;
}
EXPORT_SYMBOL_GPL(list_lru_walk_node);
@@ -338,15 +335,6 @@ static void init_one_lru(struct list_lru_one *l)
}
#ifdef CONFIG_MEMCG_KMEM
-static void memcg_destroy_list_lru_range(struct list_lru_memcg *mlrus,
- int begin, int end)
-{
- int i;
-
- for (i = begin; i < end; i++)
- kfree(mlrus->mlru[i]);
-}
-
static struct list_lru_per_memcg *memcg_init_list_lru_one(gfp_t gfp)
{
int nid;
@@ -364,14 +352,7 @@ static struct list_lru_per_memcg *memcg_init_list_lru_one(gfp_t gfp)
static void memcg_list_lru_free(struct list_lru *lru, int src_idx)
{
- struct list_lru_memcg *mlrus;
- struct list_lru_per_memcg *mlru;
-
- spin_lock_irq(&lru->lock);
- mlrus = rcu_dereference_protected(lru->mlrus, true);
- mlru = rcu_dereference_protected(mlrus->mlru[src_idx], true);
- rcu_assign_pointer(mlrus->mlru[src_idx], NULL);
- spin_unlock_irq(&lru->lock);
+ struct list_lru_per_memcg *mlru = xa_erase_irq(&lru->xa, src_idx);
/*
* The __list_lru_walk_one() can walk the list of this node.
@@ -383,78 +364,27 @@ static void memcg_list_lru_free(struct list_lru *lru, int src_idx)
kvfree_rcu(mlru, rcu);
}
-static int memcg_init_list_lru(struct list_lru *lru, bool memcg_aware)
+static inline void memcg_init_list_lru(struct list_lru *lru, bool memcg_aware)
{
- struct list_lru_memcg *mlrus;
- int size = memcg_nr_cache_ids;
-
+ if (memcg_aware)
+ xa_init_flags(&lru->xa, XA_FLAGS_LOCK_IRQ);
lru->memcg_aware = memcg_aware;
- if (!memcg_aware)
- return 0;
-
- spin_lock_init(&lru->lock);
-
- mlrus = kvzalloc(struct_size(mlrus, mlru, size), GFP_KERNEL);
- if (!mlrus)
- return -ENOMEM;
-
- RCU_INIT_POINTER(lru->mlrus, mlrus);
-
- return 0;
}
static void memcg_destroy_list_lru(struct list_lru *lru)
{
- struct list_lru_memcg *mlrus;
+ XA_STATE(xas, &lru->xa, 0);
+ struct list_lru_per_memcg *mlru;
if (!list_lru_memcg_aware(lru))
return;
- /*
- * This is called when shrinker has already been unregistered,
- * and nobody can use it. So, there is no need to use kvfree_rcu().
- */
- mlrus = rcu_dereference_protected(lru->mlrus, true);
- memcg_destroy_list_lru_range(mlrus, 0, memcg_nr_cache_ids);
- kvfree(mlrus);
-}
-
-static int memcg_update_list_lru(struct list_lru *lru, int old_size, int new_size)
-{
- struct list_lru_memcg *old, *new;
-
- BUG_ON(old_size > new_size);
-
- old = rcu_dereference_protected(lru->mlrus,
- lockdep_is_held(&list_lrus_mutex));
- new = kvmalloc(struct_size(new, mlru, new_size), GFP_KERNEL);
- if (!new)
- return -ENOMEM;
-
- spin_lock_irq(&lru->lock);
- memcpy(&new->mlru, &old->mlru, flex_array_size(new, mlru, old_size));
- memset(&new->mlru[old_size], 0, flex_array_size(new, mlru, new_size - old_size));
- rcu_assign_pointer(lru->mlrus, new);
- spin_unlock_irq(&lru->lock);
-
- kvfree_rcu(old, rcu);
- return 0;
-}
-
-int memcg_update_all_list_lrus(int new_size)
-{
- int ret = 0;
- struct list_lru *lru;
- int old_size = memcg_nr_cache_ids;
-
- mutex_lock(&list_lrus_mutex);
- list_for_each_entry(lru, &memcg_list_lrus, list) {
- ret = memcg_update_list_lru(lru, old_size, new_size);
- if (ret)
- break;
+ xas_lock_irq(&xas);
+ xas_for_each(&xas, mlru, ULONG_MAX) {
+ kfree(mlru);
+ xas_store(&xas, NULL);
}
- mutex_unlock(&list_lrus_mutex);
- return ret;
+ xas_unlock_irq(&xas);
}
static void memcg_reparent_list_lru_node(struct list_lru *lru, int nid,
@@ -521,7 +451,7 @@ void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *paren
struct mem_cgroup *child;
child = mem_cgroup_from_css(css);
- child->kmemcg_id = parent->kmemcg_id;
+ WRITE_ONCE(child->kmemcg_id, parent->kmemcg_id);
}
rcu_read_unlock();
@@ -531,21 +461,12 @@ void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *paren
mutex_unlock(&list_lrus_mutex);
}
-static bool memcg_list_lru_allocated(struct mem_cgroup *memcg,
- struct list_lru *lru)
+static inline bool memcg_list_lru_allocated(struct mem_cgroup *memcg,
+ struct list_lru *lru)
{
- bool allocated;
- int idx;
+ int idx = memcg->kmemcg_id;
- idx = memcg->kmemcg_id;
- if (unlikely(idx < 0))
- return true;
-
- rcu_read_lock();
- allocated = !!rcu_access_pointer(rcu_dereference(lru->mlrus)->mlru[idx]);
- rcu_read_unlock();
-
- return allocated;
+ return idx < 0 || xa_load(&lru->xa, idx);
}
int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
@@ -558,6 +479,7 @@ int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
struct list_lru_per_memcg *mlru;
struct mem_cgroup *memcg;
} *table;
+ XA_STATE(xas, &lru->xa, 0);
if (!list_lru_memcg_aware(lru) || memcg_list_lru_allocated(memcg, lru))
return 0;
@@ -586,27 +508,48 @@ int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
}
}
- spin_lock_irqsave(&lru->lock, flags);
- mlrus = rcu_dereference_protected(lru->mlrus, true);
+ xas_lock_irqsave(&xas, flags);
while (i--) {
- int index = table[i].memcg->kmemcg_id;
+ int index = READ_ONCE(table[i].memcg->kmemcg_id);
struct list_lru_per_memcg *mlru = table[i].mlru;
- if (index < 0 || rcu_dereference_protected(mlrus->mlru[index], true))
+ xas_set(&xas, index);
+retry:
+ if (unlikely(index < 0 || xas_error(&xas) || xas_load(&xas))) {
kfree(mlru);
- else
- rcu_assign_pointer(mlrus->mlru[index], mlru);
+ } else {
+ xas_store(&xas, mlru);
+ if (xas_error(&xas) == -ENOMEM) {
+ xas_unlock_irqrestore(&xas, flags);
+ if (xas_nomem(&xas, gfp))
+ xas_set_err(&xas, 0);
+ xas_lock_irqsave(&xas, flags);
+ /*
+ * The xas lock has been released, this memcg
+ * can be reparented before us. So reload
+ * memcg id. More details see the comments
+ * in memcg_reparent_list_lrus().
+ */
+ index = READ_ONCE(table[i].memcg->kmemcg_id);
+ if (index < 0)
+ xas_set_err(&xas, 0);
+ else if (!xas_error(&xas) && index != xas.xa_index)
+ xas_set(&xas, index);
+ goto retry;
+ }
+ }
}
- spin_unlock_irqrestore(&lru->lock, flags);
-
+ /* xas_nomem() is used to free memory instead of memory allocation. */
+ if (xas.xa_alloc)
+ xas_nomem(&xas, gfp);
+ xas_unlock_irqrestore(&xas, flags);
kfree(table);
- return 0;
+ return xas_error(&xas);
}
#else
-static int memcg_init_list_lru(struct list_lru *lru, bool memcg_aware)
+static inline void memcg_init_list_lru(struct list_lru *lru, bool memcg_aware)
{
- return 0;
}
static void memcg_destroy_list_lru(struct list_lru *lru)
@@ -618,7 +561,6 @@ int __list_lru_init(struct list_lru *lru, bool memcg_aware,
struct lock_class_key *key, struct shrinker *shrinker)
{
int i;
- int err = -ENOMEM;
#ifdef CONFIG_MEMCG_KMEM
if (shrinker)
@@ -626,11 +568,10 @@ int __list_lru_init(struct list_lru *lru, bool memcg_aware,
else
lru->shrinker_id = -1;
#endif
- memcg_get_cache_ids();
lru->node = kcalloc(nr_node_ids, sizeof(*lru->node), GFP_KERNEL);
if (!lru->node)
- goto out;
+ return -ENOMEM;
for_each_node(i) {
spin_lock_init(&lru->node[i].lock);
@@ -639,18 +580,10 @@ int __list_lru_init(struct list_lru *lru, bool memcg_aware,
init_one_lru(&lru->node[i].lru);
}
- err = memcg_init_list_lru(lru, memcg_aware);
- if (err) {
- kfree(lru->node);
- /* Do this so a list_lru_destroy() doesn't crash: */
- lru->node = NULL;
- goto out;
- }
-
+ memcg_init_list_lru(lru, memcg_aware);
list_lru_register(lru);
-out:
- memcg_put_cache_ids();
- return err;
+
+ return 0;
}
EXPORT_SYMBOL_GPL(__list_lru_init);
@@ -660,8 +593,6 @@ void list_lru_destroy(struct list_lru *lru)
if (!lru->node)
return;
- memcg_get_cache_ids();
-
list_lru_unregister(lru);
memcg_destroy_list_lru(lru);
@@ -671,6 +602,5 @@ void list_lru_destroy(struct list_lru *lru)
#ifdef CONFIG_MEMCG_KMEM
lru->shrinker_id = -1;
#endif
- memcg_put_cache_ids();
}
EXPORT_SYMBOL_GPL(list_lru_destroy);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a19b1a1c8ea9..28d6d2564f9d 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -351,42 +351,17 @@ static void memcg_reparent_objcgs(struct mem_cgroup *memcg,
* This will be used as a shrinker list's index.
* The main reason for not using cgroup id for this:
* this works better in sparse environments, where we have a lot of memcgs,
- * but only a few kmem-limited. Or also, if we have, for instance, 200
- * memcgs, and none but the 200th is kmem-limited, we'd have to have a
- * 200 entry array for that.
- *
- * The current size of the caches array is stored in memcg_nr_cache_ids. It
- * will double each time we have to increase it.
+ * but only a few kmem-limited.
*/
static DEFINE_IDA(memcg_cache_ida);
-int memcg_nr_cache_ids;
-
-/* Protects memcg_nr_cache_ids */
-static DECLARE_RWSEM(memcg_cache_ids_sem);
-
-void memcg_get_cache_ids(void)
-{
- down_read(&memcg_cache_ids_sem);
-}
-
-void memcg_put_cache_ids(void)
-{
- up_read(&memcg_cache_ids_sem);
-}
/*
- * MIN_SIZE is different than 1, because we would like to avoid going through
- * the alloc/free process all the time. In a small machine, 4 kmem-limited
- * cgroups is a reasonable guess. In the future, it could be a parameter or
- * tunable, but that is strictly not necessary.
- *
* MAX_SIZE should be as large as the number of cgrp_ids. Ideally, we could get
* this constant directly from cgroup, but it is understandable that this is
* better kept as an internal representation in cgroup.c. In any case, the
* cgrp_id space is not getting any smaller, and we don't have to necessarily
* increase ours as well if it increases.
*/
-#define MEMCG_CACHES_MIN_SIZE 4
#define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX
/*
@@ -2910,49 +2885,6 @@ __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void)
return objcg;
}
-static int memcg_alloc_cache_id(void)
-{
- int id, size;
- int err;
-
- id = ida_simple_get(&memcg_cache_ida,
- 0, MEMCG_CACHES_MAX_SIZE, GFP_KERNEL);
- if (id < 0)
- return id;
-
- if (id < memcg_nr_cache_ids)
- return id;
-
- /*
- * There's no space for the new id in memcg_caches arrays,
- * so we have to grow them.
- */
- down_write(&memcg_cache_ids_sem);
-
- size = 2 * (id + 1);
- if (size < MEMCG_CACHES_MIN_SIZE)
- size = MEMCG_CACHES_MIN_SIZE;
- else if (size > MEMCG_CACHES_MAX_SIZE)
- size = MEMCG_CACHES_MAX_SIZE;
-
- err = memcg_update_all_list_lrus(size);
- if (!err)
- memcg_nr_cache_ids = size;
-
- up_write(&memcg_cache_ids_sem);
-
- if (err) {
- ida_simple_remove(&memcg_cache_ida, id);
- return err;
- }
- return id;
-}
-
-static void memcg_free_cache_id(int id)
-{
- ida_simple_remove(&memcg_cache_ida, id);
-}
-
/*
* obj_cgroup_uncharge_pages: uncharge a number of kernel pages from a objcg
* @objcg: object cgroup to uncharge
@@ -3607,13 +3539,14 @@ static int memcg_online_kmem(struct mem_cgroup *memcg)
if (unlikely(mem_cgroup_is_root(memcg)))
return 0;
- memcg_id = memcg_alloc_cache_id();
+ memcg_id = ida_alloc_max(&memcg_cache_ida, MEMCG_CACHES_MAX_SIZE - 1,
+ GFP_KERNEL);
if (memcg_id < 0)
return memcg_id;
objcg = obj_cgroup_alloc();
if (!objcg) {
- memcg_free_cache_id(memcg_id);
+ ida_free(&memcg_cache_ida, memcg_id);
return -ENOMEM;
}
objcg->memcg = memcg;
@@ -3657,7 +3590,7 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
*/
memcg_reparent_list_lrus(memcg, parent);
- memcg_free_cache_id(kmemcg_id);
+ ida_free(&memcg_cache_ida, kmemcg_id);
}
#else
static int memcg_online_kmem(struct mem_cgroup *memcg)
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v5 13/16] mm: memcontrol: reuse memory cgroup ID for kmem ID
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (11 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 12/16] mm: list_lru: replace linear array with xarray Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2021-12-20 9:27 ` Mika Penttilä
2022-01-05 17:03 ` Michal Koutný
2021-12-20 8:56 ` [PATCH v5 14/16] mm: memcontrol: fix cannot alloc the maximum memcg ID Muchun Song
` (2 subsequent siblings)
15 siblings, 2 replies; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
There are two idrs being used by memory cgroup, one is for kmem ID,
another is for memory cgroup ID. The maximum ID of both is 64Ki.
Both of them can limit the total number of memory cgroups. Actually,
we can reuse memory cgroup ID for kmem ID to simplify the code.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
include/linux/memcontrol.h | 1 +
mm/memcontrol.c | 46 ++++++++--------------------------------------
2 files changed, 9 insertions(+), 38 deletions(-)
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 3fc437162add..7b472f805d77 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -56,6 +56,7 @@ struct mem_cgroup_reclaim_cookie {
#ifdef CONFIG_MEMCG
#define MEM_CGROUP_ID_SHIFT 16
+#define MEM_CGROUP_ID_MIN 1
#define MEM_CGROUP_ID_MAX USHRT_MAX
struct mem_cgroup_id {
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 28d6d2564f9d..04f75055f518 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -348,23 +348,6 @@ static void memcg_reparent_objcgs(struct mem_cgroup *memcg,
}
/*
- * This will be used as a shrinker list's index.
- * The main reason for not using cgroup id for this:
- * this works better in sparse environments, where we have a lot of memcgs,
- * but only a few kmem-limited.
- */
-static DEFINE_IDA(memcg_cache_ida);
-
-/*
- * MAX_SIZE should be as large as the number of cgrp_ids. Ideally, we could get
- * this constant directly from cgroup, but it is understandable that this is
- * better kept as an internal representation in cgroup.c. In any case, the
- * cgrp_id space is not getting any smaller, and we don't have to necessarily
- * increase ours as well if it increases.
- */
-#define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX
-
-/*
* A lot of the calls to the cache allocation functions are expected to be
* inlined by the compiler. Since the calls to memcg_slab_pre_alloc_hook() are
* conditional to this static branch, we'll have to allow modules that does
@@ -3528,10 +3511,12 @@ static u64 mem_cgroup_read_u64(struct cgroup_subsys_state *css,
}
#ifdef CONFIG_MEMCG_KMEM
+#define MEM_CGROUP_KMEM_ID_MIN -1
+#define MEM_CGROUP_ID_DIFF (MEM_CGROUP_ID_MIN - MEM_CGROUP_KMEM_ID_MIN)
+
static int memcg_online_kmem(struct mem_cgroup *memcg)
{
struct obj_cgroup *objcg;
- int memcg_id;
if (cgroup_memory_nokmem)
return 0;
@@ -3539,22 +3524,16 @@ static int memcg_online_kmem(struct mem_cgroup *memcg)
if (unlikely(mem_cgroup_is_root(memcg)))
return 0;
- memcg_id = ida_alloc_max(&memcg_cache_ida, MEMCG_CACHES_MAX_SIZE - 1,
- GFP_KERNEL);
- if (memcg_id < 0)
- return memcg_id;
-
objcg = obj_cgroup_alloc();
- if (!objcg) {
- ida_free(&memcg_cache_ida, memcg_id);
+ if (!objcg)
return -ENOMEM;
- }
+
objcg->memcg = memcg;
rcu_assign_pointer(memcg->objcg, objcg);
static_branch_enable(&memcg_kmem_enabled_key);
- memcg->kmemcg_id = memcg_id;
+ memcg->kmemcg_id = memcg->id.id - MEM_CGROUP_ID_DIFF;
return 0;
}
@@ -3562,7 +3541,6 @@ static int memcg_online_kmem(struct mem_cgroup *memcg)
static void memcg_offline_kmem(struct mem_cgroup *memcg)
{
struct mem_cgroup *parent;
- int kmemcg_id;
if (cgroup_memory_nokmem)
return;
@@ -3577,20 +3555,12 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
memcg_reparent_objcgs(memcg, parent);
/*
- * memcg_reparent_list_lrus() can change memcg->kmemcg_id.
- * Cache it to local @kmemcg_id.
- */
- kmemcg_id = memcg->kmemcg_id;
-
- /*
* After we have finished memcg_reparent_objcgs(), all list_lrus
* corresponding to this cgroup are guaranteed to remain empty.
* The ordering is imposed by list_lru_node->lock taken by
* memcg_reparent_list_lrus().
*/
memcg_reparent_list_lrus(memcg, parent);
-
- ida_free(&memcg_cache_ida, kmemcg_id);
}
#else
static int memcg_online_kmem(struct mem_cgroup *memcg)
@@ -5043,7 +5013,7 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
return ERR_PTR(error);
memcg->id.id = idr_alloc(&mem_cgroup_idr, NULL,
- 1, MEM_CGROUP_ID_MAX,
+ MEM_CGROUP_ID_MIN, MEM_CGROUP_ID_MAX,
GFP_KERNEL);
if (memcg->id.id < 0) {
error = memcg->id.id;
@@ -5071,7 +5041,7 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
spin_lock_init(&memcg->event_list_lock);
memcg->socket_pressure = jiffies;
#ifdef CONFIG_MEMCG_KMEM
- memcg->kmemcg_id = -1;
+ memcg->kmemcg_id = MEM_CGROUP_KMEM_ID_MIN;
INIT_LIST_HEAD(&memcg->objcg_list);
#endif
#ifdef CONFIG_CGROUP_WRITEBACK
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH v5 13/16] mm: memcontrol: reuse memory cgroup ID for kmem ID
2021-12-20 8:56 ` [PATCH v5 13/16] mm: memcontrol: reuse memory cgroup ID for kmem ID Muchun Song
@ 2021-12-20 9:27 ` Mika Penttilä
2022-01-05 17:03 ` Michal Koutný
1 sibling, 0 replies; 45+ messages in thread
From: Mika Penttilä @ 2021-12-20 9:27 UTC (permalink / raw)
To: Muchun Song, willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb,
guro, shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun
Hi,
On 20.12.2021 10.56, Muchun Song wrote:
> There are two idrs being used by memory cgroup, one is for kmem ID,
> another is for memory cgroup ID. The maximum ID of both is 64Ki.
> Both of them can limit the total number of memory cgroups. Actually,
> we can reuse memory cgroup ID for kmem ID to simplify the code.
>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
> include/linux/memcontrol.h | 1 +
> mm/memcontrol.c | 46 ++++++++--------------------------------------
> 2 files changed, 9 insertions(+), 38 deletions(-)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 3fc437162add..7b472f805d77 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -56,6 +56,7 @@ struct mem_cgroup_reclaim_cookie {
> #ifdef CONFIG_MEMCG
>
> #define MEM_CGROUP_ID_SHIFT 16
> +#define MEM_CGROUP_ID_MIN 1
> #define MEM_CGROUP_ID_MAX USHRT_MAX
>
> struct mem_cgroup_id {
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 28d6d2564f9d..04f75055f518 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -348,23 +348,6 @@ static void memcg_reparent_objcgs(struct mem_cgroup *memcg,
> }
>
> /*
> - * This will be used as a shrinker list's index.
> - * The main reason for not using cgroup id for this:
> - * this works better in sparse environments, where we have a lot of memcgs,
> - * but only a few kmem-limited.
> - */
> -static DEFINE_IDA(memcg_cache_ida);
> -
> -/*
> - * MAX_SIZE should be as large as the number of cgrp_ids. Ideally, we could get
> - * this constant directly from cgroup, but it is understandable that this is
> - * better kept as an internal representation in cgroup.c. In any case, the
> - * cgrp_id space is not getting any smaller, and we don't have to necessarily
> - * increase ours as well if it increases.
> - */
> -#define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX
> -
> -/*
> * A lot of the calls to the cache allocation functions are expected to be
> * inlined by the compiler. Since the calls to memcg_slab_pre_alloc_hook() are
> * conditional to this static branch, we'll have to allow modules that does
> @@ -3528,10 +3511,12 @@ static u64 mem_cgroup_read_u64(struct cgroup_subsys_state *css,
> }
>
> #ifdef CONFIG_MEMCG_KMEM
> +#define MEM_CGROUP_KMEM_ID_MIN -1
Maybe
#define MEM_CGROUP_KMEM_ID_MIN 0
to be the first allocated id, so MEM_CGROUP_ID_DIFF is 1, not 2
> +#define MEM_CGROUP_ID_DIFF (MEM_CGROUP_ID_MIN - MEM_CGROUP_KMEM_ID_MIN)
> +
> static int memcg_online_kmem(struct mem_cgroup *memcg)
> {
> struct obj_cgroup *objcg;
> - int memcg_id;
>
> if (cgroup_memory_nokmem)
> return 0;
> @@ -3539,22 +3524,16 @@ static int memcg_online_kmem(struct mem_cgroup *memcg)
> if (unlikely(mem_cgroup_is_root(memcg)))
> return 0;
>
> - memcg_id = ida_alloc_max(&memcg_cache_ida, MEMCG_CACHES_MAX_SIZE - 1,
> - GFP_KERNEL);
> - if (memcg_id < 0)
> - return memcg_id;
> -
> objcg = obj_cgroup_alloc();
> - if (!objcg) {
> - ida_free(&memcg_cache_ida, memcg_id);
> + if (!objcg)
> return -ENOMEM;
> - }
> +
> objcg->memcg = memcg;
> rcu_assign_pointer(memcg->objcg, objcg);
>
> static_branch_enable(&memcg_kmem_enabled_key);
>
> - memcg->kmemcg_id = memcg_id;
> + memcg->kmemcg_id = memcg->id.id - MEM_CGROUP_ID_DIFF;
>
> return 0;
> }
> @@ -3562,7 +3541,6 @@ static int memcg_online_kmem(struct mem_cgroup *memcg)
> static void memcg_offline_kmem(struct mem_cgroup *memcg)
> {
> struct mem_cgroup *parent;
> - int kmemcg_id;
>
> if (cgroup_memory_nokmem)
> return;
> @@ -3577,20 +3555,12 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
> memcg_reparent_objcgs(memcg, parent);
>
> /*
> - * memcg_reparent_list_lrus() can change memcg->kmemcg_id.
> - * Cache it to local @kmemcg_id.
> - */
> - kmemcg_id = memcg->kmemcg_id;
> -
> - /*
> * After we have finished memcg_reparent_objcgs(), all list_lrus
> * corresponding to this cgroup are guaranteed to remain empty.
> * The ordering is imposed by list_lru_node->lock taken by
> * memcg_reparent_list_lrus().
> */
> memcg_reparent_list_lrus(memcg, parent);
> -
> - ida_free(&memcg_cache_ida, kmemcg_id);
> }
> #else
> static int memcg_online_kmem(struct mem_cgroup *memcg)
> @@ -5043,7 +5013,7 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
> return ERR_PTR(error);
>
> memcg->id.id = idr_alloc(&mem_cgroup_idr, NULL,
> - 1, MEM_CGROUP_ID_MAX,
> + MEM_CGROUP_ID_MIN, MEM_CGROUP_ID_MAX,
> GFP_KERNEL);
> if (memcg->id.id < 0) {
> error = memcg->id.id;
> @@ -5071,7 +5041,7 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
> spin_lock_init(&memcg->event_list_lock);
> memcg->socket_pressure = jiffies;
> #ifdef CONFIG_MEMCG_KMEM
> - memcg->kmemcg_id = -1;
> + memcg->kmemcg_id = MEM_CGROUP_KMEM_ID_MIN;
> INIT_LIST_HEAD(&memcg->objcg_list);
> #endif
> #ifdef CONFIG_CGROUP_WRITEBACK
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH v5 13/16] mm: memcontrol: reuse memory cgroup ID for kmem ID
2021-12-20 8:56 ` [PATCH v5 13/16] mm: memcontrol: reuse memory cgroup ID for kmem ID Muchun Song
2021-12-20 9:27 ` Mika Penttilä
@ 2022-01-05 17:03 ` Michal Koutný
2022-01-06 3:34 ` Muchun Song
1 sibling, 1 reply; 45+ messages in thread
From: Michal Koutný @ 2022-01-05 17:03 UTC (permalink / raw)
To: Muchun Song
Cc: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander, linux-fsdevel,
linux-kernel, linux-mm, linux-nfs, zhengqi.arch, duanxiongchun,
fam.zheng, smuchun
On Mon, Dec 20, 2021 at 04:56:46PM +0800, Muchun Song <songmuchun@bytedance.com> wrote:
> There are two idrs being used by memory cgroup, one is for kmem ID,
> another is for memory cgroup ID. The maximum ID of both is 64Ki.
> Both of them can limit the total number of memory cgroups.
> Actually, we can reuse memory cgroup ID for kmem ID to simplify the
> code.
An interesting improvement.
I'm a bit dense -- what's the purpose the MEM_CGROUP_ID_DIFF offset?
Couldn't this deduplication be extended to only use mem_cgroup.id.id
instead of mem_cgroup.kmemcg_id? (With a boolean telling whether kmem
accounting is active.)
Thanks,
Michal
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v5 13/16] mm: memcontrol: reuse memory cgroup ID for kmem ID
2022-01-05 17:03 ` Michal Koutný
@ 2022-01-06 3:34 ` Muchun Song
0 siblings, 0 replies; 45+ messages in thread
From: Muchun Song @ 2022-01-06 3:34 UTC (permalink / raw)
To: Michal Koutný, Mika Penttilä
Cc: Matthew Wilcox, Andrew Morton, Johannes Weiner, Michal Hocko,
Vladimir Davydov, Shakeel Butt, Roman Gushchin, Yang Shi,
Alex Shi, Wei Yang, Dave Chinner, trond.myklebust,
anna.schumaker, jaegeuk, chao, Kari Argillander, linux-fsdevel,
LKML, Linux Memory Management List, linux-nfs, Qi Zheng,
Xiongchun duan, Fam Zheng, Muchun Song
On Thu, Jan 6, 2022 at 1:03 AM Michal Koutný <mkoutny@suse.com> wrote:
>
> On Mon, Dec 20, 2021 at 04:56:46PM +0800, Muchun Song <songmuchun@bytedance.com> wrote:
> > There are two idrs being used by memory cgroup, one is for kmem ID,
> > another is for memory cgroup ID. The maximum ID of both is 64Ki.
> > Both of them can limit the total number of memory cgroups.
> > Actually, we can reuse memory cgroup ID for kmem ID to simplify the
> > code.
>
> An interesting improvement.
>
> I'm a bit dense -- what's the purpose the MEM_CGROUP_ID_DIFF offset?
Hi Michal and Mika,
MEM_CGROUP_ID_DIFF is introduced to be consistent with before
that kmem ID starts with -1 and has no holes. Actually, it can be dropped
and make memcg->kmemcg_id equal to memcg->id.id directly.
> Couldn't this deduplication be extended to only use mem_cgroup.id.id
> instead of mem_cgroup.kmemcg_id? (With a boolean telling whether kmem
> accounting is active.)
>
Not easy to completely remove memcg->kmemcg_id since this filed
will be used to sync list_lru reparenting which will change
memcg->kmemcg_id to its parent's kmem ID (more details refers to
memcg_drain_all_list_lrus() in patch 10 of this series).
Thanks.
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v5 14/16] mm: memcontrol: fix cannot alloc the maximum memcg ID
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (12 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 13/16] mm: memcontrol: reuse memory cgroup ID for kmem ID Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2021-12-20 8:56 ` [PATCH v5 15/16] mm: list_lru: rename list_lru_per_memcg to list_lru_memcg Muchun Song
2021-12-20 8:56 ` [PATCH v5 16/16] mm: memcontrol: rename memcg_cache_id to memcg_kmem_id Muchun Song
15 siblings, 0 replies; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
The idr_alloc() does not include @max ID. So in the current implementation,
the maximum memcg ID is 65534 instead of 65535. It seems a bug. So fix this.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
mm/memcontrol.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 04f75055f518..fdd0007554a0 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5013,7 +5013,7 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
return ERR_PTR(error);
memcg->id.id = idr_alloc(&mem_cgroup_idr, NULL,
- MEM_CGROUP_ID_MIN, MEM_CGROUP_ID_MAX,
+ MEM_CGROUP_ID_MIN, MEM_CGROUP_ID_MAX + 1,
GFP_KERNEL);
if (memcg->id.id < 0) {
error = memcg->id.id;
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v5 15/16] mm: list_lru: rename list_lru_per_memcg to list_lru_memcg
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (13 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 14/16] mm: memcontrol: fix cannot alloc the maximum memcg ID Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
2021-12-20 8:56 ` [PATCH v5 16/16] mm: memcontrol: rename memcg_cache_id to memcg_kmem_id Muchun Song
15 siblings, 0 replies; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
The name of list_lru_memcg was occupied before and became free since last
commit. Rename list_lru_per_memcg to list_lru_memcg since the name is brief.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
include/linux/list_lru.h | 2 +-
mm/list_lru.c | 18 +++++++++---------
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
index 572c263561ac..b35968ee9fb5 100644
--- a/include/linux/list_lru.h
+++ b/include/linux/list_lru.h
@@ -32,7 +32,7 @@ struct list_lru_one {
long nr_items;
};
-struct list_lru_per_memcg {
+struct list_lru_memcg {
struct rcu_head rcu;
/* array of per cgroup per node lists, indexed by node id */
struct list_lru_one node[];
diff --git a/mm/list_lru.c b/mm/list_lru.c
index 8dc1dabb9f05..38f711e9b56e 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -53,7 +53,7 @@ static inline struct list_lru_one *
list_lru_from_memcg_idx(struct list_lru *lru, int nid, int idx)
{
if (list_lru_memcg_aware(lru) && idx >= 0) {
- struct list_lru_per_memcg *mlru = xa_load(&lru->xa, idx);
+ struct list_lru_memcg *mlru = xa_load(&lru->xa, idx);
return mlru ? &mlru->node[nid] : NULL;
}
@@ -306,7 +306,7 @@ unsigned long list_lru_walk_node(struct list_lru *lru, int nid,
#ifdef CONFIG_MEMCG_KMEM
if (*nr_to_walk > 0 && list_lru_memcg_aware(lru)) {
- struct list_lru_per_memcg *mlru;
+ struct list_lru_memcg *mlru;
unsigned long index;
xa_for_each(&lru->xa, index, mlru) {
@@ -335,10 +335,10 @@ static void init_one_lru(struct list_lru_one *l)
}
#ifdef CONFIG_MEMCG_KMEM
-static struct list_lru_per_memcg *memcg_init_list_lru_one(gfp_t gfp)
+static struct list_lru_memcg *memcg_init_list_lru_one(gfp_t gfp)
{
int nid;
- struct list_lru_per_memcg *mlru;
+ struct list_lru_memcg *mlru;
mlru = kmalloc(struct_size(mlru, node, nr_node_ids), gfp);
if (!mlru)
@@ -352,7 +352,7 @@ static struct list_lru_per_memcg *memcg_init_list_lru_one(gfp_t gfp)
static void memcg_list_lru_free(struct list_lru *lru, int src_idx)
{
- struct list_lru_per_memcg *mlru = xa_erase_irq(&lru->xa, src_idx);
+ struct list_lru_memcg *mlru = xa_erase_irq(&lru->xa, src_idx);
/*
* The __list_lru_walk_one() can walk the list of this node.
@@ -374,7 +374,7 @@ static inline void memcg_init_list_lru(struct list_lru *lru, bool memcg_aware)
static void memcg_destroy_list_lru(struct list_lru *lru)
{
XA_STATE(xas, &lru->xa, 0);
- struct list_lru_per_memcg *mlru;
+ struct list_lru_memcg *mlru;
if (!list_lru_memcg_aware(lru))
return;
@@ -476,7 +476,7 @@ int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
unsigned long flags;
struct list_lru_memcg *mlrus;
struct list_lru_memcg_table {
- struct list_lru_per_memcg *mlru;
+ struct list_lru_memcg *mlru;
struct mem_cgroup *memcg;
} *table;
XA_STATE(xas, &lru->xa, 0);
@@ -492,7 +492,7 @@ int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
/*
* Because the list_lru can be reparented to the parent cgroup's
* list_lru, we should make sure that this cgroup and all its
- * ancestors have allocated list_lru_per_memcg.
+ * ancestors have allocated list_lru_memcg.
*/
for (i = 0; memcg; memcg = parent_mem_cgroup(memcg), i++) {
if (memcg_list_lru_allocated(memcg, lru))
@@ -511,7 +511,7 @@ int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
xas_lock_irqsave(&xas, flags);
while (i--) {
int index = READ_ONCE(table[i].memcg->kmemcg_id);
- struct list_lru_per_memcg *mlru = table[i].mlru;
+ struct list_lru_memcg *mlru = table[i].mlru;
xas_set(&xas, index);
retry:
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread* [PATCH v5 16/16] mm: memcontrol: rename memcg_cache_id to memcg_kmem_id
2021-12-20 8:56 [PATCH v5 00/16] Optimize list lru memory consumption Muchun Song
` (14 preceding siblings ...)
2021-12-20 8:56 ` [PATCH v5 15/16] mm: list_lru: rename list_lru_per_memcg to list_lru_memcg Muchun Song
@ 2021-12-20 8:56 ` Muchun Song
15 siblings, 0 replies; 45+ messages in thread
From: Muchun Song @ 2021-12-20 8:56 UTC (permalink / raw)
To: willy, akpm, hannes, mhocko, vdavydov.dev, shakeelb, guro,
shy828301, alexs, richard.weiyang, david, trond.myklebust,
anna.schumaker, jaegeuk, chao, kari.argillander
Cc: linux-fsdevel, linux-kernel, linux-mm, linux-nfs, zhengqi.arch,
duanxiongchun, fam.zheng, smuchun, Muchun Song
The memcg_cache_id() introduced by commit 2633d7a02823 ("slab/slub:
consider a memcg parameter in kmem_create_cache") is used to index
in the kmem_cache->memcg_params->memcg_caches array. Since
kmem_cache->memcg_params.memcg_caches has been removed by commit
9855609bde03 ("mm: memcg/slab: use a single set of kmem_caches for
all accounted allocations"). So the name does not need to reflect
cache related. Just rename it to memcg_kmem_id. And it can reflect
kmem related.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
include/linux/memcontrol.h | 4 ++--
mm/list_lru.c | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 7b472f805d77..94ed3a124191 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -1735,7 +1735,7 @@ static inline void memcg_kmem_uncharge_page(struct page *page, int order)
* A helper for accessing memcg's kmem_id, used for getting
* corresponding LRU lists.
*/
-static inline int memcg_cache_id(struct mem_cgroup *memcg)
+static inline int memcg_kmem_id(struct mem_cgroup *memcg)
{
return memcg ? memcg->kmemcg_id : -1;
}
@@ -1773,7 +1773,7 @@ static inline bool memcg_kmem_enabled(void)
return false;
}
-static inline int memcg_cache_id(struct mem_cgroup *memcg)
+static inline int memcg_kmem_id(struct mem_cgroup *memcg)
{
return -1;
}
diff --git a/mm/list_lru.c b/mm/list_lru.c
index 38f711e9b56e..8b402373e965 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -75,7 +75,7 @@ list_lru_from_kmem(struct list_lru *lru, int nid, void *ptr,
if (!memcg)
goto out;
- l = list_lru_from_memcg_idx(lru, nid, memcg_cache_id(memcg));
+ l = list_lru_from_memcg_idx(lru, nid, memcg_kmem_id(memcg));
out:
if (memcg_ptr)
*memcg_ptr = memcg;
@@ -182,7 +182,7 @@ unsigned long list_lru_count_one(struct list_lru *lru,
long count;
rcu_read_lock();
- l = list_lru_from_memcg_idx(lru, nid, memcg_cache_id(memcg));
+ l = list_lru_from_memcg_idx(lru, nid, memcg_kmem_id(memcg));
count = l ? READ_ONCE(l->nr_items) : 0;
rcu_read_unlock();
@@ -273,7 +273,7 @@ list_lru_walk_one(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
unsigned long ret;
spin_lock(&nlru->lock);
- ret = __list_lru_walk_one(lru, nid, memcg_cache_id(memcg), isolate,
+ ret = __list_lru_walk_one(lru, nid, memcg_kmem_id(memcg), isolate,
cb_arg, nr_to_walk);
spin_unlock(&nlru->lock);
return ret;
@@ -289,7 +289,7 @@ list_lru_walk_one_irq(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
unsigned long ret;
spin_lock_irq(&nlru->lock);
- ret = __list_lru_walk_one(lru, nid, memcg_cache_id(memcg), isolate,
+ ret = __list_lru_walk_one(lru, nid, memcg_kmem_id(memcg), isolate,
cb_arg, nr_to_walk);
spin_unlock_irq(&nlru->lock);
return ret;
--
2.11.0
^ permalink raw reply related [flat|nested] 45+ messages in thread