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). But now there are less than 500
containers in the system. And memcg_nr_cache_ids has not been reduced
to a suitable value. This can waste a lot of memory. If we want to reduce
memcg_nr_cache_ids, we have to reboot the server. This is not what we
want.
So this patchset will dynamically adjust the value of memcg_nr_cache_ids
to keep healthy memory consumption. In this case, we may be able to restore
a healthy environment even if the users have created tens of thousands of
memory cgroups and then destroyed those memory cgroups. This patchset also
contains some code simplification.
Muchun Song (9):
mm: list_lru: fix list_lru_count_one() return value
mm: memcontrol: remove kmemcg_id reparenting
mm: list_lru: rename memcg_drain_all_list_lrus to
memcg_reparent_list_lrus
mm: memcontrol: remove the kmem states
mm: memcontrol: move memcg_online_kmem() to mem_cgroup_css_online()
mm: list_lru: support for shrinking list lru
ida: introduce ida_max() to return the maximum allocated ID
mm: memcontrol: shrink the list lru size
mm: memcontrol: rename memcg_{get,put}_cache_ids to
memcg_list_lru_resize_{lock,unlock}
include/linux/idr.h | 1 +
include/linux/list_lru.h | 2 +-
include/linux/memcontrol.h | 15 ++----
lib/idr.c | 40 +++++++++++++++
mm/list_lru.c | 89 +++++++++++++++++++++++++--------
mm/memcontrol.c | 121 +++++++++++++++++++++++++--------------------
6 files changed, 183 insertions(+), 85 deletions(-)
--
2.11.0
Since commit 2788cf0c401c ("memcg: reparent list_lrus and free kmemcg_id
on css offline"), the ->nr_items can be negative during memory cgroup
reparenting. In this case, list_lru_count_one() can returns an unusually
large value. In order to not surprise the user. So return zero when
->nr_items is negative.
Signed-off-by: Muchun Song <[email protected]>
---
mm/list_lru.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/mm/list_lru.c b/mm/list_lru.c
index cd58790d0fb3..4962d48d4410 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -176,13 +176,16 @@ unsigned long list_lru_count_one(struct list_lru *lru,
{
struct list_lru_node *nlru = &lru->node[nid];
struct list_lru_one *l;
- unsigned long count;
+ long count;
rcu_read_lock();
l = list_lru_from_memcg_idx(nlru, memcg_cache_id(memcg));
count = READ_ONCE(l->nr_items);
rcu_read_unlock();
+ if (unlikely(count < 0))
+ count = 0;
+
return count;
}
EXPORT_SYMBOL_GPL(list_lru_count_one);
--
2.11.0
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). But now there are less than 500
containers in the system. And memcg_nr_cache_ids has not been reduced
to a suitable value. This can waste a lot of memory. If we want to reduce
memcg_nr_cache_ids, we have to reboot the server. This is not what we
want. So this patch will dynamically adjust the value of
memcg_nr_cache_ids to keep healthy memory consumption. In this case, we
may be able to restore a healthy environment even if the users have
created tens of thousands of memory cgroups.
In this patch, I adjusted the calculation formula of memcg_nr_cache_ids
from "size = 2 * (id + 1)" to "size = 2 * id" in memcg_alloc_cache_id().
Because this can make things more simple when shrink the list lru size.
Signed-off-by: Muchun Song <[email protected]>
---
mm/memcontrol.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 49 insertions(+), 3 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 1610d501e7b5..f8cdd87cf693 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -362,6 +362,8 @@ static void memcg_reparent_objcgs(struct mem_cgroup *memcg,
static DEFINE_IDA(memcg_cache_ida);
int memcg_nr_cache_ids;
+static int kmemcg_max_id;
+
/* Protects memcg_nr_cache_ids */
static DECLARE_RWSEM(memcg_cache_ids_sem);
@@ -2856,8 +2858,11 @@ static int memcg_alloc_cache_id(void)
if (id < 0)
return id;
- if (id < memcg_nr_cache_ids)
+ if (id < memcg_nr_cache_ids) {
+ if (id > kmemcg_max_id)
+ kmemcg_max_id = id;
return id;
+ }
/*
* There's no space for the new id in memcg_caches arrays,
@@ -2865,15 +2870,17 @@ static int memcg_alloc_cache_id(void)
*/
down_write(&memcg_cache_ids_sem);
- size = 2 * (id + 1);
+ size = 2 * id;
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)
+ if (!err) {
memcg_nr_cache_ids = size;
+ kmemcg_max_id = id;
+ }
up_write(&memcg_cache_ids_sem);
@@ -2884,9 +2891,48 @@ static int memcg_alloc_cache_id(void)
return id;
}
+static inline int nearest_fit_id(int id)
+{
+ if (unlikely(id < MEMCG_CACHES_MIN_SIZE))
+ return MEMCG_CACHES_MIN_SIZE;
+
+ return 1 << (__fls(id) + 1);
+}
+
+/*
+ * memcg_alloc_cache_id() and memcg_free_cache_id() are serialized by
+ * cgroup_mutex. So there is no race on kmemcg_max_id.
+ */
static void memcg_free_cache_id(int id)
{
ida_simple_remove(&memcg_cache_ida, id);
+
+ if (kmemcg_max_id == id) {
+ /*
+ * In order to avoid @memcg_nr_cache_ids bouncing between
+ * @memcg_nr_cache_ids / 2 and @memcg_nr_cache_ids. We only
+ * shrink the list lru size when @kmemcg_max_id is smaller
+ * than @memcg_nr_cache_ids / 3.
+ */
+ int size = memcg_nr_cache_ids / 3;
+
+ kmemcg_max_id = ida_max(&memcg_cache_ida);
+ if (kmemcg_max_id < size) {
+ /*
+ * Find the first value greater than @kmemcg_max_id
+ * which can fit our need. And shrink the list lru
+ * to this size.
+ */
+ size = nearest_fit_id(kmemcg_max_id);
+
+ down_write(&memcg_cache_ids_sem);
+ if (size != memcg_nr_cache_ids) {
+ memcg_update_all_list_lrus(size);
+ memcg_nr_cache_ids = size;
+ }
+ up_write(&memcg_cache_ids_sem);
+ }
+ }
}
/*
--
2.11.0
The rwsem is held for writing during list lru arrays relocation and
memcg_nr_cache_ids updates. Therefore memcg_get_cache_ids implies
memcg_nr_cache_ids cannot be updated. It acts as a lock primitive.
So rename it to a more suitable name.
Signed-off-by: Muchun Song <[email protected]>
---
include/linux/memcontrol.h | 8 ++++----
mm/list_lru.c | 8 ++++----
mm/memcontrol.c | 4 ++--
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 6350c563c7b8..e8ba6ee1b369 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -1635,8 +1635,8 @@ 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);
+void memcg_list_lru_resize_lock(void);
+void memcg_list_lru_resize_unlock(void);
/*
* Helper macro to loop through all memcg-specific caches. Callers must still
@@ -1711,11 +1711,11 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
return -1;
}
-static inline void memcg_get_cache_ids(void)
+static inline void memcg_list_lru_resize_lock(void)
{
}
-static inline void memcg_put_cache_ids(void)
+static inline void memcg_list_lru_resize_unlock(void)
{
}
diff --git a/mm/list_lru.c b/mm/list_lru.c
index 3ee5239922c9..e0ba0641b4e1 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -640,7 +640,7 @@ int __list_lru_init(struct list_lru *lru, bool memcg_aware,
else
lru->shrinker_id = -1;
#endif
- memcg_get_cache_ids();
+ memcg_list_lru_resize_lock();
lru->node = kcalloc(nr_node_ids, sizeof(*lru->node), GFP_KERNEL);
if (!lru->node)
@@ -663,7 +663,7 @@ int __list_lru_init(struct list_lru *lru, bool memcg_aware,
list_lru_register(lru);
out:
- memcg_put_cache_ids();
+ memcg_list_lru_resize_unlock();
return err;
}
EXPORT_SYMBOL_GPL(__list_lru_init);
@@ -674,7 +674,7 @@ void list_lru_destroy(struct list_lru *lru)
if (!lru->node)
return;
- memcg_get_cache_ids();
+ memcg_list_lru_resize_lock();
list_lru_unregister(lru);
@@ -685,6 +685,6 @@ void list_lru_destroy(struct list_lru *lru)
#ifdef CONFIG_MEMCG_KMEM
lru->shrinker_id = -1;
#endif
- memcg_put_cache_ids();
+ memcg_list_lru_resize_unlock();
}
EXPORT_SYMBOL_GPL(list_lru_destroy);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f8cdd87cf693..437465611845 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -367,12 +367,12 @@ static int kmemcg_max_id;
/* Protects memcg_nr_cache_ids */
static DECLARE_RWSEM(memcg_cache_ids_sem);
-void memcg_get_cache_ids(void)
+void memcg_list_lru_resize_lock(void)
{
down_read(&memcg_cache_ids_sem);
}
-void memcg_put_cache_ids(void)
+void memcg_list_lru_resize_unlock(void)
{
up_read(&memcg_cache_ids_sem);
}
--
2.11.0
Introduce ida_max() to return the maximum allocated ID. This will be
used by memory cgroup in the later patch.
Signed-off-by: Muchun Song <[email protected]>
---
include/linux/idr.h | 1 +
lib/idr.c | 40 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 41 insertions(+)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index a0dce14090a9..c3968a6348d1 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -255,6 +255,7 @@ struct ida {
int ida_alloc_range(struct ida *, unsigned int min, unsigned int max, gfp_t);
void ida_free(struct ida *, unsigned int id);
void ida_destroy(struct ida *ida);
+int ida_max(struct ida *ida);
/**
* ida_alloc() - Allocate an unused ID.
diff --git a/lib/idr.c b/lib/idr.c
index f4ab4f4aa3c7..bcfcaae89aa7 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -553,6 +553,46 @@ void ida_destroy(struct ida *ida)
}
EXPORT_SYMBOL(ida_destroy);
+/**
+ * ida_max() - Return the maximum allocated ID.
+ * @ida: IDA handle.
+ *
+ * Context: Any context. It is safe to call this function without
+ * locking in your code.
+ *
+ * Return: The maximum allocated ID, or %-ENOSPC if the @ida is empty
+ */
+int ida_max(struct ida *ida)
+{
+ XA_STATE(xas, &ida->xa, 0);
+ struct ida_bitmap *curr, *prev;
+ unsigned long flags;
+ unsigned int bit, index;
+
+ xas_lock_irqsave(&xas, flags);
+ if (ida_is_empty(ida)) {
+ xas_unlock_irqrestore(&xas, flags);
+ return -ENOSPC;
+ }
+
+ xas_for_each(&xas, curr, ULONG_MAX) {
+ prev = curr;
+ index = xas.xa_index;
+ }
+
+ if (xa_is_value(prev)) {
+ unsigned long val = xa_to_value(prev);
+
+ bit = find_last_bit(&val, BITS_PER_XA_VALUE);
+ } else {
+ bit = find_last_bit(prev->bitmap, IDA_BITMAP_BITS);
+ }
+
+ xas_unlock_irqrestore(&xas, flags);
+
+ return index * IDA_BITMAP_BITS + bit;
+}
+
#ifndef __KERNEL__
extern void xa_dump_index(unsigned long index, unsigned int shift);
#define IDA_CHUNK_SHIFT ilog2(IDA_BITMAP_BITS)
--
2.11.0
On Wed, Apr 28, 2021 at 2:54 AM Muchun Song <[email protected]> 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). But now there are less than 500
> containers in the system. And memcg_nr_cache_ids has not been reduced
> to a suitable value. This can waste a lot of memory. If we want to reduce
> memcg_nr_cache_ids, we have to reboot the server. This is not what we
> want.
>
> So this patchset will dynamically adjust the value of memcg_nr_cache_ids
> to keep healthy memory consumption. In this case, we may be able to restore
> a healthy environment even if the users have created tens of thousands of
> memory cgroups and then destroyed those memory cgroups. This patchset also
> contains some code simplification.
>
There was a recent discussion [1] on the same issue. Did you get the
chance to take a look at that. I have not gone through this patch
series yet but will do in the next couple of weeks.
[1] https://lore.kernel.org/linux-fsdevel/[email protected]/
On Thu, Apr 29, 2021 at 7:32 AM Shakeel Butt <[email protected]> wrote:
>
> On Wed, Apr 28, 2021 at 2:54 AM Muchun Song <[email protected]> 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). But now there are less than 500
> > containers in the system. And memcg_nr_cache_ids has not been reduced
> > to a suitable value. This can waste a lot of memory. If we want to reduce
> > memcg_nr_cache_ids, we have to reboot the server. This is not what we
> > want.
> >
> > So this patchset will dynamically adjust the value of memcg_nr_cache_ids
> > to keep healthy memory consumption. In this case, we may be able to restore
> > a healthy environment even if the users have created tens of thousands of
> > memory cgroups and then destroyed those memory cgroups. This patchset also
> > contains some code simplification.
> >
>
> There was a recent discussion [1] on the same issue. Did you get the
> chance to take a look at that. I have not gone through this patch
> series yet but will do in the next couple of weeks.
>
> [1] https://lore.kernel.org/linux-fsdevel/[email protected]/
Thanks for your reminder.
No, I haven't. But now I have looked at this. The issue is very similar
to mine. But Bharata seems to want to run 10k containers. And
optimize the memory consumption of list_lru_one in this case.
This is not what I do. I want to try to shrink the size of the list lrus
when the number of memcgs is reduced from tens of thousands
to hundreds.
Thanks.
On Wed, Apr 28, 2021 at 05:49:47PM +0800, Muchun Song wrote:
> Introduce ida_max() to return the maximum allocated ID. This will be
> used by memory cgroup in the later patch.
Please also add a lower-level xa_max as this funcationalty would also
come in handy at the xarray level in a few places.
On Thu, Apr 29, 2021 at 2:49 PM Christoph Hellwig <[email protected]> wrote:
>
> On Wed, Apr 28, 2021 at 05:49:47PM +0800, Muchun Song wrote:
> > Introduce ida_max() to return the maximum allocated ID. This will be
> > used by memory cgroup in the later patch.
>
> Please also add a lower-level xa_max as this funcationalty would also
> come in handy at the xarray level in a few places.
Good suggestion. This way is also efficient. Thanks.
On Wed, Apr 28, 2021 at 05:49:40PM +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
The more I see people trying to work around this, the more I think
that the way memcgs have been grafted into the list_lru is back to
front.
We currently allocate scope for every memcg to be able to tracked on
every not 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.
IOWs, 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 the list_lru is only needed for a
given memcg if that memcg is instatiating and freeing objects on a
given list_lru.
Which makes me think we should be moving more towards "add the memcg
to the list_lru at the first insert" model rather than "instantiate
all at memcg init time just in case". The model we originally came
up with for supprting memcgs is really starting to show it's limits,
and we should address those limitations rahter than hack more
complexity into the system that does nothing to remove the
limitations that are causing the problems in the first place.
> 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). But now there are less than 500
> containers in the system. And memcg_nr_cache_ids has not been reduced
> to a suitable value. This can waste a lot of memory. If we want to reduce
> memcg_nr_cache_ids, we have to reboot the server. This is not what we
> want.
Exactly my point. This model is broken and doesn't scale to large
counts of either memcgs or superblocks.
We need a different model for dynamically adding, removing and
mapping memcgs to LRU lists they are actually using so that we can
efficiently scale to tens of thousands of memcgs instances along
with tens of thousands of unique superblock instants. That's the
real problem that needs solving here.
> So this patchset will dynamically adjust the value of memcg_nr_cache_ids
> to keep healthy memory consumption.
Gigabytes of RAM for largely unused memcg list_lrus on every
superblock is not healthy. It's highly inefficient because the
infrastructure we currently have was never designed to scale to
these numbers of unique containers and superblocks...
Cheers,
Dave.
--
Dave Chinner
[email protected]
On Fri, Apr 30, 2021 at 10:49:03AM +1000, Dave Chinner wrote:
> On Wed, Apr 28, 2021 at 05:49:40PM +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
>
> The more I see people trying to work around this, the more I think
> that the way memcgs have been grafted into the list_lru is back to
> front.
>
> We currently allocate scope for every memcg to be able to tracked on
> every not 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.
>
> IOWs, 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 the list_lru is only needed for a
> given memcg if that memcg is instatiating and freeing objects on a
> given list_lru.
>
> Which makes me think we should be moving more towards "add the memcg
> to the list_lru at the first insert" model rather than "instantiate
> all at memcg init time just in case". The model we originally came
> up with for supprting memcgs is really starting to show it's limits,
> and we should address those limitations rahter than hack more
> complexity into the system that does nothing to remove the
> limitations that are causing the problems in the first place.
I totally agree.
It looks like the initial implementation of the whole kernel memory accounting
and memcg-aware shrinkers was based on the idea that the number of memory
cgroups is relatively small and stable. With systemd creating a separate cgroup
for everything including short-living processes it simple not true anymore.
Thanks!
On Thu, Apr 29, 2021 at 06:39:40PM -0700, Roman Gushchin wrote:
> On Fri, Apr 30, 2021 at 10:49:03AM +1000, Dave Chinner wrote:
> > On Wed, Apr 28, 2021 at 05:49:40PM +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
> >
> > The more I see people trying to work around this, the more I think
> > that the way memcgs have been grafted into the list_lru is back to
> > front.
> >
> > We currently allocate scope for every memcg to be able to tracked on
> > every not 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.
> >
> > IOWs, 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 the list_lru is only needed for a
> > given memcg if that memcg is instatiating and freeing objects on a
> > given list_lru.
> >
> > Which makes me think we should be moving more towards "add the memcg
> > to the list_lru at the first insert" model rather than "instantiate
> > all at memcg init time just in case". The model we originally came
> > up with for supprting memcgs is really starting to show it's limits,
> > and we should address those limitations rahter than hack more
> > complexity into the system that does nothing to remove the
> > limitations that are causing the problems in the first place.
>
> I totally agree.
>
> It looks like the initial implementation of the whole kernel memory accounting
> and memcg-aware shrinkers was based on the idea that the number of memory
> cgroups is relatively small and stable.
Yes, that was one of the original assumptions - tens to maybe low
hundreds of memcgs at most. The other was that memcgs weren't NUMA
aware, and so would only need a single LRU list per memcg. Hence the
total overhead even with "lots" of memcgsi and superblocks the
overhead wasn't that great.
Then came "memcgs need to be NUMA aware" because of the size of the
machines they were being use for resrouce management in, and that
greatly increased the per-memcg, per LRU overhead. Now we're talking
about needing to support a couple of orders of magnitude more memcgs
and superblocks than were originally designed for.
So, really, we're way beyond the original design scope of this
subsystem now.
> With systemd creating a separate cgroup
> for everything including short-living processes it simple not true anymore.
Yeah, that too. Everything is much more dynamic these days...
Cheers,
Dave.
--
Dave Chinner
[email protected]
On Fri, Apr 30, 2021 at 11:27 AM Dave Chinner <[email protected]> wrote:
>
> On Thu, Apr 29, 2021 at 06:39:40PM -0700, Roman Gushchin wrote:
> > On Fri, Apr 30, 2021 at 10:49:03AM +1000, Dave Chinner wrote:
> > > On Wed, Apr 28, 2021 at 05:49:40PM +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
> > >
> > > The more I see people trying to work around this, the more I think
> > > that the way memcgs have been grafted into the list_lru is back to
> > > front.
> > >
> > > We currently allocate scope for every memcg to be able to tracked on
> > > every not 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.
> > >
> > > IOWs, 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 the list_lru is only needed for a
> > > given memcg if that memcg is instatiating and freeing objects on a
> > > given list_lru.
> > >
> > > Which makes me think we should be moving more towards "add the memcg
> > > to the list_lru at the first insert" model rather than "instantiate
> > > all at memcg init time just in case". The model we originally came
> > > up with for supprting memcgs is really starting to show it's limits,
> > > and we should address those limitations rahter than hack more
> > > complexity into the system that does nothing to remove the
> > > limitations that are causing the problems in the first place.
> >
> > I totally agree.
> >
> > It looks like the initial implementation of the whole kernel memory accounting
> > and memcg-aware shrinkers was based on the idea that the number of memory
> > cgroups is relatively small and stable.
>
> Yes, that was one of the original assumptions - tens to maybe low
> hundreds of memcgs at most. The other was that memcgs weren't NUMA
> aware, and so would only need a single LRU list per memcg. Hence the
> total overhead even with "lots" of memcgsi and superblocks the
> overhead wasn't that great.
>
> Then came "memcgs need to be NUMA aware" because of the size of the
> machines they were being use for resrouce management in, and that
> greatly increased the per-memcg, per LRU overhead. Now we're talking
> about needing to support a couple of orders of magnitude more memcgs
> and superblocks than were originally designed for.
>
> So, really, we're way beyond the original design scope of this
> subsystem now.
Got it. So it is better to allocate the structure of the list_lru_node
dynamically. We should only allocate it when it is really demanded.
But allocating memory by using GFP_ATOMIC in list_lru_add() is
not a good idea. So we should allocate the memory out of
list_lru_add(). I can propose an approach that may work.
Before start, we should know about the following rules of list lrus.
- Only objects allocated with __GFP_ACCOUNT need to allocate
the struct list_lru_node.
- The caller of allocating memory must know which list_lru the
object will insert.
So we can allocate struct list_lru_node when allocating the
object instead of allocating it when list_lru_add(). It is easy, because
we already know the list_lru and memcg which the object belongs
to. So we can introduce a new helper to allocate the object and
list_lru_node. Like below.
void *list_lru_kmem_cache_alloc(struct list_lru *lru, struct kmem_cache *s,
gfp_t gfpflags)
{
void *ret = kmem_cache_alloc(s, gfpflags);
if (ret && (gfpflags & __GFP_ACCOUNT)) {
struct mem_cgroup *memcg = mem_cgroup_from_obj(ret);
if (mem_cgroup_is_root(memcg))
return ret;
/* Allocate per-memcg list_lru_node, if it already
allocated, do nothing. */
memcg_list_lru_node_alloc(lru, memcg,
page_to_nid(virt_to_page(ret)), gfpflags);
}
return ret;
}
If the user wants to insert the allocated object to its lru list in
the feature. The
user should use list_lru_kmem_cache_alloc() instead of kmem_cache_alloc().
I have looked at the code closely. There are 3 different kmem_caches that
need to use this new API to allocate memory. They are inode_cachep,
dentry_cache and radix_tree_node_cachep. I think that it is easy to migrate.
Hi Roman and Dave,
What do you think about this approach? If there is no problem, I can provide
a preliminary patchset within a week.
Thanks.
>
> > With systemd creating a separate cgroup
> > for everything including short-living processes it simple not true anymore.
>
> Yeah, that too. Everything is much more dynamic these days...
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> [email protected]
On Fri, Apr 30, 2021 at 04:32:39PM +0800, Muchun Song wrote:
> On Fri, Apr 30, 2021 at 11:27 AM Dave Chinner <[email protected]> wrote:
> >
> > On Thu, Apr 29, 2021 at 06:39:40PM -0700, Roman Gushchin wrote:
> > > On Fri, Apr 30, 2021 at 10:49:03AM +1000, Dave Chinner wrote:
> > > > On Wed, Apr 28, 2021 at 05:49:40PM +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
> > > >
> > > > The more I see people trying to work around this, the more I think
> > > > that the way memcgs have been grafted into the list_lru is back to
> > > > front.
> > > >
> > > > We currently allocate scope for every memcg to be able to tracked on
> > > > every not 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.
> > > >
> > > > IOWs, 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 the list_lru is only needed for a
> > > > given memcg if that memcg is instatiating and freeing objects on a
> > > > given list_lru.
> > > >
> > > > Which makes me think we should be moving more towards "add the memcg
> > > > to the list_lru at the first insert" model rather than "instantiate
> > > > all at memcg init time just in case". The model we originally came
> > > > up with for supprting memcgs is really starting to show it's limits,
> > > > and we should address those limitations rahter than hack more
> > > > complexity into the system that does nothing to remove the
> > > > limitations that are causing the problems in the first place.
> > >
> > > I totally agree.
> > >
> > > It looks like the initial implementation of the whole kernel memory accounting
> > > and memcg-aware shrinkers was based on the idea that the number of memory
> > > cgroups is relatively small and stable.
> >
> > Yes, that was one of the original assumptions - tens to maybe low
> > hundreds of memcgs at most. The other was that memcgs weren't NUMA
> > aware, and so would only need a single LRU list per memcg. Hence the
> > total overhead even with "lots" of memcgsi and superblocks the
> > overhead wasn't that great.
> >
> > Then came "memcgs need to be NUMA aware" because of the size of the
> > machines they were being use for resrouce management in, and that
> > greatly increased the per-memcg, per LRU overhead. Now we're talking
> > about needing to support a couple of orders of magnitude more memcgs
> > and superblocks than were originally designed for.
> >
> > So, really, we're way beyond the original design scope of this
> > subsystem now.
>
> Got it. So it is better to allocate the structure of the list_lru_node
> dynamically. We should only allocate it when it is really demanded.
> But allocating memory by using GFP_ATOMIC in list_lru_add() is
> not a good idea. So we should allocate the memory out of
> list_lru_add(). I can propose an approach that may work.
>
> Before start, we should know about the following rules of list lrus.
>
> - Only objects allocated with __GFP_ACCOUNT need to allocate
> the struct list_lru_node.
> - The caller of allocating memory must know which list_lru the
> object will insert.
>
> So we can allocate struct list_lru_node when allocating the
> object instead of allocating it when list_lru_add(). It is easy, because
> we already know the list_lru and memcg which the object belongs
> to. So we can introduce a new helper to allocate the object and
> list_lru_node. Like below.
>
> void *list_lru_kmem_cache_alloc(struct list_lru *lru, struct kmem_cache *s,
> gfp_t gfpflags)
> {
> void *ret = kmem_cache_alloc(s, gfpflags);
>
> if (ret && (gfpflags & __GFP_ACCOUNT)) {
> struct mem_cgroup *memcg = mem_cgroup_from_obj(ret);
>
> if (mem_cgroup_is_root(memcg))
> return ret;
>
> /* Allocate per-memcg list_lru_node, if it already
> allocated, do nothing. */
> memcg_list_lru_node_alloc(lru, memcg,
> page_to_nid(virt_to_page(ret)), gfpflags);
> }
>
> return ret;
> }
>
> If the user wants to insert the allocated object to its lru list in
> the feature. The
> user should use list_lru_kmem_cache_alloc() instead of kmem_cache_alloc().
> I have looked at the code closely. There are 3 different kmem_caches that
> need to use this new API to allocate memory. They are inode_cachep,
> dentry_cache and radix_tree_node_cachep. I think that it is easy to migrate.
>
> Hi Roman and Dave,
>
> What do you think about this approach? If there is no problem, I can provide
> a preliminary patchset within a week.
At a very first glance it looks similar to what Bharata proposed, but with some
additional tricks. It would be nice to find a common ground here. In general,
I think it's a right direction.
In general I believe we might need some more fundamental changes, but I don't
have a specific recipe yet. I need to think more of it.
Thanks!
On Fri, Apr 30, 2021 at 04:32:39PM +0800, Muchun Song wrote:
> Before start, we should know about the following rules of list lrus.
>
> - Only objects allocated with __GFP_ACCOUNT need to allocate
> the struct list_lru_node.
> - The caller of allocating memory must know which list_lru the
> object will insert.
>
> So we can allocate struct list_lru_node when allocating the
> object instead of allocating it when list_lru_add(). It is easy, because
> we already know the list_lru and memcg which the object belongs
> to. So we can introduce a new helper to allocate the object and
> list_lru_node. Like below.
I feel like there may be a simpler solution, although I'm not really
familiar with the list_lru situation. The three caches you mention:
> I have looked at the code closely. There are 3 different kmem_caches that
> need to use this new API to allocate memory. They are inode_cachep,
> dentry_cache and radix_tree_node_cachep. I think that it is easy to migrate.
are all filesystem. So if there's a way of knowing which filesystems
are exposed to each container, we can allocate the list_lru structures at
"mount" time rather than at first allocation for a given cache/lru/memcg
combination.
On Fri, Apr 30, 2021 at 04:32:39PM +0800, Muchun Song wrote:
> On Fri, Apr 30, 2021 at 11:27 AM Dave Chinner <[email protected]> wrote:
> >
> > On Thu, Apr 29, 2021 at 06:39:40PM -0700, Roman Gushchin wrote:
> > > On Fri, Apr 30, 2021 at 10:49:03AM +1000, Dave Chinner wrote:
> > > > On Wed, Apr 28, 2021 at 05:49:40PM +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
> > > >
> > > > The more I see people trying to work around this, the more I think
> > > > that the way memcgs have been grafted into the list_lru is back to
> > > > front.
> > > >
> > > > We currently allocate scope for every memcg to be able to tracked on
> > > > every not 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.
> > > >
> > > > IOWs, 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 the list_lru is only needed for a
> > > > given memcg if that memcg is instatiating and freeing objects on a
> > > > given list_lru.
> > > >
> > > > Which makes me think we should be moving more towards "add the memcg
> > > > to the list_lru at the first insert" model rather than "instantiate
> > > > all at memcg init time just in case". The model we originally came
> > > > up with for supprting memcgs is really starting to show it's limits,
> > > > and we should address those limitations rahter than hack more
> > > > complexity into the system that does nothing to remove the
> > > > limitations that are causing the problems in the first place.
> > >
> > > I totally agree.
> > >
> > > It looks like the initial implementation of the whole kernel memory accounting
> > > and memcg-aware shrinkers was based on the idea that the number of memory
> > > cgroups is relatively small and stable.
> >
> > Yes, that was one of the original assumptions - tens to maybe low
> > hundreds of memcgs at most. The other was that memcgs weren't NUMA
> > aware, and so would only need a single LRU list per memcg. Hence the
> > total overhead even with "lots" of memcgsi and superblocks the
> > overhead wasn't that great.
> >
> > Then came "memcgs need to be NUMA aware" because of the size of the
> > machines they were being use for resrouce management in, and that
> > greatly increased the per-memcg, per LRU overhead. Now we're talking
> > about needing to support a couple of orders of magnitude more memcgs
> > and superblocks than were originally designed for.
> >
> > So, really, we're way beyond the original design scope of this
> > subsystem now.
>
> Got it. So it is better to allocate the structure of the list_lru_node
> dynamically. We should only allocate it when it is really demanded.
> But allocating memory by using GFP_ATOMIC in list_lru_add() is
> not a good idea. So we should allocate the memory out of
> list_lru_add(). I can propose an approach that may work.
>
> Before start, we should know about the following rules of list lrus.
>
> - Only objects allocated with __GFP_ACCOUNT need to allocate
> the struct list_lru_node.
This seems .... misguided. inode and dentry caches are already
marked as accounted, so individual calls to allocate from these
slabs do not need this annotation.
> - The caller of allocating memory must know which list_lru the
> object will insert.
>
> So we can allocate struct list_lru_node when allocating the
> object instead of allocating it when list_lru_add(). It is easy, because
> we already know the list_lru and memcg which the object belongs
> to. So we can introduce a new helper to allocate the object and
> list_lru_node. Like below.
>
> void *list_lru_kmem_cache_alloc(struct list_lru *lru, struct kmem_cache *s,
> gfp_t gfpflags)
> {
> void *ret = kmem_cache_alloc(s, gfpflags);
>
> if (ret && (gfpflags & __GFP_ACCOUNT)) {
> struct mem_cgroup *memcg = mem_cgroup_from_obj(ret);
>
> if (mem_cgroup_is_root(memcg))
> return ret;
>
> /* Allocate per-memcg list_lru_node, if it already
> allocated, do nothing. */
> memcg_list_lru_node_alloc(lru, memcg,
> page_to_nid(virt_to_page(ret)), gfpflags);
If we are allowing kmem_cache_alloc() to fail, then we can allow
memcg_list_lru_node_alloc() to fail, too.
Also, why put this outside kmem_cache_alloc()? Node id and memcg is
already known internally to kmem_cache_alloc() when allocating from
a slab, so why not associate the slab allocation with the LRU
directly when doing the memcg accounting and so avoid doing costly
duplicate work on every allocation?
i.e. the list-lru was moved inside the mm/ dir because "it's a mm
specific construct only", so why not actually make use of that
designation to internalise this entire memcg management issue into
the slab allocation routines? i.e. an API like
kmem_cache_alloc_lru(cache, lru, gfpflags) allows this to be
completely internalised and efficiently implemented with minimal
change to callers. It also means that memory allocation callers
don't need to know anything about memcg management, which is always
a win....
> }
>
> return ret;
> }
>
> If the user wants to insert the allocated object to its lru list in
> the feature. The
> user should use list_lru_kmem_cache_alloc() instead of kmem_cache_alloc().
> I have looked at the code closely. There are 3 different kmem_caches that
> need to use this new API to allocate memory. They are inode_cachep,
> dentry_cache and radix_tree_node_cachep. I think that it is easy to migrate.
It might work, but I think you may have overlooked the complexity
of inode allocation for filesystems. i.e. alloc_inode() calls out
to filesystem allocation functions more often than it allocates
directly from the inode_cachep. i.e. Most filesystems provide
their own ->alloc_inode superblock operation, and they allocate
inodes out of their own specific slab caches, not the inode_cachep.
And then you have filesystems like XFS, where alloc_inode() will
never be called, and implement ->alloc_inode as:
/* Catch misguided souls that try to use this interface on XFS */
STATIC struct inode *
xfs_fs_alloc_inode(
struct super_block *sb)
{
BUG();
return NULL;
}
Because all the inode caching and allocation is internal to XFS and
VFS inode management interfaces are not used.
So I suspect that an external wrapper function is not the way to go
here - either internalising the LRU management into the slab
allocation or adding the memcg code to alloc_inode() and filesystem
specific routines would make a lot more sense to me.
Cheers,
Dave.
--
Dave Chinner
[email protected]
On Mon, May 3, 2021 at 7:58 AM Dave Chinner <[email protected]> wrote:
>
> On Fri, Apr 30, 2021 at 04:32:39PM +0800, Muchun Song wrote:
> > On Fri, Apr 30, 2021 at 11:27 AM Dave Chinner <[email protected]> wrote:
> > >
> > > On Thu, Apr 29, 2021 at 06:39:40PM -0700, Roman Gushchin wrote:
> > > > On Fri, Apr 30, 2021 at 10:49:03AM +1000, Dave Chinner wrote:
> > > > > On Wed, Apr 28, 2021 at 05:49:40PM +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
> > > > >
> > > > > The more I see people trying to work around this, the more I think
> > > > > that the way memcgs have been grafted into the list_lru is back to
> > > > > front.
> > > > >
> > > > > We currently allocate scope for every memcg to be able to tracked on
> > > > > every not 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.
> > > > >
> > > > > IOWs, 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 the list_lru is only needed for a
> > > > > given memcg if that memcg is instatiating and freeing objects on a
> > > > > given list_lru.
> > > > >
> > > > > Which makes me think we should be moving more towards "add the memcg
> > > > > to the list_lru at the first insert" model rather than "instantiate
> > > > > all at memcg init time just in case". The model we originally came
> > > > > up with for supprting memcgs is really starting to show it's limits,
> > > > > and we should address those limitations rahter than hack more
> > > > > complexity into the system that does nothing to remove the
> > > > > limitations that are causing the problems in the first place.
> > > >
> > > > I totally agree.
> > > >
> > > > It looks like the initial implementation of the whole kernel memory accounting
> > > > and memcg-aware shrinkers was based on the idea that the number of memory
> > > > cgroups is relatively small and stable.
> > >
> > > Yes, that was one of the original assumptions - tens to maybe low
> > > hundreds of memcgs at most. The other was that memcgs weren't NUMA
> > > aware, and so would only need a single LRU list per memcg. Hence the
> > > total overhead even with "lots" of memcgsi and superblocks the
> > > overhead wasn't that great.
> > >
> > > Then came "memcgs need to be NUMA aware" because of the size of the
> > > machines they were being use for resrouce management in, and that
> > > greatly increased the per-memcg, per LRU overhead. Now we're talking
> > > about needing to support a couple of orders of magnitude more memcgs
> > > and superblocks than were originally designed for.
> > >
> > > So, really, we're way beyond the original design scope of this
> > > subsystem now.
> >
> > Got it. So it is better to allocate the structure of the list_lru_node
> > dynamically. We should only allocate it when it is really demanded.
> > But allocating memory by using GFP_ATOMIC in list_lru_add() is
> > not a good idea. So we should allocate the memory out of
> > list_lru_add(). I can propose an approach that may work.
> >
> > Before start, we should know about the following rules of list lrus.
> >
> > - Only objects allocated with __GFP_ACCOUNT need to allocate
> > the struct list_lru_node.
>
> This seems .... misguided. inode and dentry caches are already
> marked as accounted, so individual calls to allocate from these
> slabs do not need this annotation.
Sorry for the confusion. You are right.
>
> > - The caller of allocating memory must know which list_lru the
> > object will insert.
> >
> > So we can allocate struct list_lru_node when allocating the
> > object instead of allocating it when list_lru_add(). It is easy, because
> > we already know the list_lru and memcg which the object belongs
> > to. So we can introduce a new helper to allocate the object and
> > list_lru_node. Like below.
> >
> > void *list_lru_kmem_cache_alloc(struct list_lru *lru, struct kmem_cache *s,
> > gfp_t gfpflags)
> > {
> > void *ret = kmem_cache_alloc(s, gfpflags);
> >
> > if (ret && (gfpflags & __GFP_ACCOUNT)) {
> > struct mem_cgroup *memcg = mem_cgroup_from_obj(ret);
> >
> > if (mem_cgroup_is_root(memcg))
> > return ret;
> >
> > /* Allocate per-memcg list_lru_node, if it already
> > allocated, do nothing. */
> > memcg_list_lru_node_alloc(lru, memcg,
> > page_to_nid(virt_to_page(ret)), gfpflags);
>
> If we are allowing kmem_cache_alloc() to fail, then we can allow
> memcg_list_lru_node_alloc() to fail, too.
>
> Also, why put this outside kmem_cache_alloc()? Node id and memcg is
> already known internally to kmem_cache_alloc() when allocating from
> a slab, so why not associate the slab allocation with the LRU
> directly when doing the memcg accounting and so avoid doing costly
> duplicate work on every allocation?
>
> i.e. the list-lru was moved inside the mm/ dir because "it's a mm
> specific construct only", so why not actually make use of that
> designation to internalise this entire memcg management issue into
> the slab allocation routines? i.e. an API like
Yeah, we can.
> kmem_cache_alloc_lru(cache, lru, gfpflags) allows this to be
> completely internalised and efficiently implemented with minimal
> change to callers. It also means that memory allocation callers
> don't need to know anything about memcg management, which is always
> a win....
Great idea. It's efficient. I'd give it a try.
>
> > }
> >
> > return ret;
> > }
> >
> > If the user wants to insert the allocated object to its lru list in
> > the feature. The
> > user should use list_lru_kmem_cache_alloc() instead of kmem_cache_alloc().
> > I have looked at the code closely. There are 3 different kmem_caches that
> > need to use this new API to allocate memory. They are inode_cachep,
> > dentry_cache and radix_tree_node_cachep. I think that it is easy to migrate.
>
> It might work, but I think you may have overlooked the complexity
> of inode allocation for filesystems. i.e. alloc_inode() calls out
> to filesystem allocation functions more often than it allocates
> directly from the inode_cachep. i.e. Most filesystems provide
> their own ->alloc_inode superblock operation, and they allocate
> inodes out of their own specific slab caches, not the inode_cachep.
I didn't realize this before. You are right. Most filesystems
have their own kmem_cache instead of inode_cachep.
We need a lot of filesystems special to be changed.
Thanks for your reminder.
>
> And then you have filesystems like XFS, where alloc_inode() will
> never be called, and implement ->alloc_inode as:
>
> /* Catch misguided souls that try to use this interface on XFS */
> STATIC struct inode *
> xfs_fs_alloc_inode(
> struct super_block *sb)
> {
> BUG();
> return NULL;
> }
>
> Because all the inode caching and allocation is internal to XFS and
> VFS inode management interfaces are not used.
>
> So I suspect that an external wrapper function is not the way to go
> here - either internalising the LRU management into the slab
> allocation or adding the memcg code to alloc_inode() and filesystem
> specific routines would make a lot more sense to me.
Sure. If we introduce kmem_cache_alloc_lru, all filesystems
need to migrate to kmem_cache_alloc_lru. I cannot figure out
an approach that does not need to change filesystems code.
Thanks.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> [email protected]
On Mon, May 03, 2021 at 02:33:21PM +0800, Muchun Song wrote:
> On Mon, May 3, 2021 at 7:58 AM Dave Chinner <[email protected]> wrote:
> > > If the user wants to insert the allocated object to its lru list in
> > > the feature. The
> > > user should use list_lru_kmem_cache_alloc() instead of kmem_cache_alloc().
> > > I have looked at the code closely. There are 3 different kmem_caches that
> > > need to use this new API to allocate memory. They are inode_cachep,
> > > dentry_cache and radix_tree_node_cachep. I think that it is easy to migrate.
> >
> > It might work, but I think you may have overlooked the complexity
> > of inode allocation for filesystems. i.e. alloc_inode() calls out
> > to filesystem allocation functions more often than it allocates
> > directly from the inode_cachep. i.e. Most filesystems provide
> > their own ->alloc_inode superblock operation, and they allocate
> > inodes out of their own specific slab caches, not the inode_cachep.
>
> I didn't realize this before. You are right. Most filesystems
> have their own kmem_cache instead of inode_cachep.
> We need a lot of filesystems special to be changed.
> Thanks for your reminder.
>
> >
> > And then you have filesystems like XFS, where alloc_inode() will
> > never be called, and implement ->alloc_inode as:
> >
> > /* Catch misguided souls that try to use this interface on XFS */
> > STATIC struct inode *
> > xfs_fs_alloc_inode(
> > struct super_block *sb)
> > {
> > BUG();
> > return NULL;
> > }
> >
> > Because all the inode caching and allocation is internal to XFS and
> > VFS inode management interfaces are not used.
> >
> > So I suspect that an external wrapper function is not the way to go
> > here - either internalising the LRU management into the slab
> > allocation or adding the memcg code to alloc_inode() and filesystem
> > specific routines would make a lot more sense to me.
>
> Sure. If we introduce kmem_cache_alloc_lru, all filesystems
> need to migrate to kmem_cache_alloc_lru. I cannot figure out
> an approach that does not need to change filesystems code.
Right, I don't think there's a way to avoid changing all the
filesystem code if we are touching the cache allocation routines.
However, if we hide it all inside the allocation routine, then
the changes to each filesystem is effectively just a 1-liner like:
- inode = kmem_cache_alloc(inode_cache, GFP_NOFS);
+ inode = kmem_cache_alloc_lru(inode_cache, sb->s_inode_lru, GFP_NOFS);
Or perhaps, define a generic wrapper function like:
static inline void *
alloc_inode_sb(struct superblock *sb, struct kmem_cache *cache, gfp_flags_t gfp)
{
return kmem_cache_alloc_lru(cache, sb->s_inode_lru, gfp);
}
And then each filesystem ends up with:
- inode = kmem_cache_alloc(inode_cache, GFP_NOFS);
+ inode = alloc_inode_sb(sb, inode_cache, GFP_NOFS);
so that all the superblock LRU stuff is also hidden from the
filesystems...
Cheers,
Dave.
--
Dave Chinner
[email protected]
On Wed, May 5, 2021 at 9:13 AM Dave Chinner <[email protected]> wrote:
>
> On Mon, May 03, 2021 at 02:33:21PM +0800, Muchun Song wrote:
> > On Mon, May 3, 2021 at 7:58 AM Dave Chinner <[email protected]> wrote:
> > > > If the user wants to insert the allocated object to its lru list in
> > > > the feature. The
> > > > user should use list_lru_kmem_cache_alloc() instead of kmem_cache_alloc().
> > > > I have looked at the code closely. There are 3 different kmem_caches that
> > > > need to use this new API to allocate memory. They are inode_cachep,
> > > > dentry_cache and radix_tree_node_cachep. I think that it is easy to migrate.
> > >
> > > It might work, but I think you may have overlooked the complexity
> > > of inode allocation for filesystems. i.e. alloc_inode() calls out
> > > to filesystem allocation functions more often than it allocates
> > > directly from the inode_cachep. i.e. Most filesystems provide
> > > their own ->alloc_inode superblock operation, and they allocate
> > > inodes out of their own specific slab caches, not the inode_cachep.
> >
> > I didn't realize this before. You are right. Most filesystems
> > have their own kmem_cache instead of inode_cachep.
> > We need a lot of filesystems special to be changed.
> > Thanks for your reminder.
> >
> > >
> > > And then you have filesystems like XFS, where alloc_inode() will
> > > never be called, and implement ->alloc_inode as:
> > >
> > > /* Catch misguided souls that try to use this interface on XFS */
> > > STATIC struct inode *
> > > xfs_fs_alloc_inode(
> > > struct super_block *sb)
> > > {
> > > BUG();
> > > return NULL;
> > > }
> > >
> > > Because all the inode caching and allocation is internal to XFS and
> > > VFS inode management interfaces are not used.
> > >
> > > So I suspect that an external wrapper function is not the way to go
> > > here - either internalising the LRU management into the slab
> > > allocation or adding the memcg code to alloc_inode() and filesystem
> > > specific routines would make a lot more sense to me.
> >
> > Sure. If we introduce kmem_cache_alloc_lru, all filesystems
> > need to migrate to kmem_cache_alloc_lru. I cannot figure out
> > an approach that does not need to change filesystems code.
>
> Right, I don't think there's a way to avoid changing all the
> filesystem code if we are touching the cache allocation routines.
> However, if we hide it all inside the allocation routine, then
> the changes to each filesystem is effectively just a 1-liner like:
>
> - inode = kmem_cache_alloc(inode_cache, GFP_NOFS);
> + inode = kmem_cache_alloc_lru(inode_cache, sb->s_inode_lru, GFP_NOFS);
>
> Or perhaps, define a generic wrapper function like:
>
> static inline void *
> alloc_inode_sb(struct superblock *sb, struct kmem_cache *cache, gfp_flags_t gfp)
> {
> return kmem_cache_alloc_lru(cache, sb->s_inode_lru, gfp);
> }
Good idea. I am doing this. A preliminary patch is expected next week.
Thanks.
>
> And then each filesystem ends up with:
>
> - inode = kmem_cache_alloc(inode_cache, GFP_NOFS);
> + inode = alloc_inode_sb(sb, inode_cache, GFP_NOFS);
>
> so that all the superblock LRU stuff is also hidden from the
> filesystems...
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> [email protected]