* [RFC PATCH 0/3] Make deferred split shrinker memcg aware
@ 2019-05-28 12:44 Yang Shi
2019-05-28 12:44 ` [PATCH 1/3] mm: thp: make " Yang Shi
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Yang Shi @ 2019-05-28 12:44 UTC (permalink / raw)
To: ktkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm
Cc: yang.shi, linux-mm, linux-kernel
I got some reports from our internal application team about memcg OOM.
Even though the application has been killed by oom killer, there are
still a lot THPs reside, page reclaim doesn't reclaim them at all.
Some investigation shows they are on deferred split queue, memcg direct
reclaim can't shrink them since THP deferred split shrinker is not memcg
aware, this may cause premature OOM in memcg. The issue can be
reproduced easily by the below test:
$ cgcreate -g memory:thp
$ echo 4G > /sys/fs/cgroup/memory/thp/memory/limit_in_bytes
$ cgexec -g memory:thp ./transhuge-stress 4000
transhuge-stress comes from kernel selftest.
It is easy to hit OOM, but there are still a lot THP on the deferred split
queue, memcg direct reclaim can't touch them since the deferred split
shrinker is not memcg aware.
Convert deferred split shrinker memcg aware by introducing per memcg deferred
split queue. The THP should be on either per node or per memcg deferred
split queue if it belongs to a memcg. When the page is immigrated to the
other memcg, it will be immigrated to the target memcg's deferred split queue
too.
And, move deleting THP from deferred split queue in page free before memcg
uncharge so that the page's memcg information is available.
Reuse the second tail page's deferred_list for per memcg list since the same
THP can't be on multiple deferred split queues at the same time.
Remove THP specific destructor since it is not used anymore with memcg aware
THP shrinker (Please see the commit log of patch 2/3 for the details).
Make deferred split shrinker not depend on memcg kmem since it is not slab.
It doesn't make sense to not shrink THP even though memcg kmem is disabled.
With the above change the test demonstrated above doesn't trigger OOM anymore
even though with cgroup.memory=nokmem.
Yang Shi (3):
mm: thp: make deferred split shrinker memcg aware
mm: thp: remove THP destructor
mm: shrinker: make shrinker not depend on memcg kmem
include/linux/huge_mm.h | 24 +++++++++
include/linux/memcontrol.h | 6 +++
include/linux/mm.h | 3 --
include/linux/mm_types.h | 7 ++-
include/linux/shrinker.h | 3 +-
mm/huge_memory.c | 181 ++++++++++++++++++++++++++++++++++++++++++++++++-------------------
mm/memcontrol.c | 20 ++++++++
mm/page_alloc.c | 3 --
mm/swap.c | 4 ++
mm/vmscan.c | 27 +++-------
10 files changed, 198 insertions(+), 80 deletions(-)
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware
2019-05-28 12:44 [RFC PATCH 0/3] Make deferred split shrinker memcg aware Yang Shi
@ 2019-05-28 12:44 ` Yang Shi
2019-05-28 14:42 ` Kirill Tkhai
2019-05-30 12:07 ` Kirill A. Shutemov
2019-05-28 12:44 ` [PATCH 2/3] mm: thp: remove THP destructor Yang Shi
` (2 subsequent siblings)
3 siblings, 2 replies; 20+ messages in thread
From: Yang Shi @ 2019-05-28 12:44 UTC (permalink / raw)
To: ktkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm
Cc: yang.shi, linux-mm, linux-kernel
Currently THP deferred split shrinker is not memcg aware, this may cause
premature OOM with some configuration. For example the below test would
run into premature OOM easily:
$ cgcreate -g memory:thp
$ echo 4G > /sys/fs/cgroup/memory/thp/memory/limit_in_bytes
$ cgexec -g memory:thp transhuge-stress 4000
transhuge-stress comes from kernel selftest.
It is easy to hit OOM, but there are still a lot THP on the deferred
split queue, memcg direct reclaim can't touch them since the deferred
split shrinker is not memcg aware.
Convert deferred split shrinker memcg aware by introducing per memcg
deferred split queue. The THP should be on either per node or per memcg
deferred split queue if it belongs to a memcg. When the page is
immigrated to the other memcg, it will be immigrated to the target
memcg's deferred split queue too.
And, move deleting THP from deferred split queue in page free before
memcg uncharge so that the page's memcg information is available.
Reuse the second tail page's deferred_list for per memcg list since the
same THP can't be on multiple deferred split queues.
Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Shakeel Butt <shakeelb@google.com>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
include/linux/huge_mm.h | 24 ++++++
include/linux/memcontrol.h | 6 ++
include/linux/mm_types.h | 7 +-
mm/huge_memory.c | 182 +++++++++++++++++++++++++++++++++------------
mm/memcontrol.c | 20 +++++
mm/swap.c | 4 +
6 files changed, 194 insertions(+), 49 deletions(-)
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 7cd5c15..f6d1cde 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -250,6 +250,26 @@ static inline bool thp_migration_supported(void)
return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
}
+static inline struct list_head *page_deferred_list(struct page *page)
+{
+ /*
+ * Global deferred list in the second tail pages is occupied by
+ * compound_head.
+ */
+ return &page[2].deferred_list;
+}
+
+static inline struct list_head *page_memcg_deferred_list(struct page *page)
+{
+ /*
+ * Memcg deferred list in the second tail pages is occupied by
+ * compound_head.
+ */
+ return &page[2].memcg_deferred_list;
+}
+
+extern void del_thp_from_deferred_split_queue(struct page *);
+
#else /* CONFIG_TRANSPARENT_HUGEPAGE */
#define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })
#define HPAGE_PMD_MASK ({ BUILD_BUG(); 0; })
@@ -368,6 +388,10 @@ static inline bool thp_migration_supported(void)
{
return false;
}
+
+static inline void del_thp_from_deferred_split_queue(struct page *page)
+{
+}
#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
#endif /* _LINUX_HUGE_MM_H */
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index bc74d6a..9ff5fab 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -316,6 +316,12 @@ struct mem_cgroup {
struct list_head event_list;
spinlock_t event_list_lock;
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+ struct list_head split_queue;
+ unsigned long split_queue_len;
+ spinlock_t split_queue_lock;
+#endif
+
struct mem_cgroup_per_node *nodeinfo[0];
/* WARNING: nodeinfo must be the last member here */
};
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 8ec38b1..405f5e6 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -139,7 +139,12 @@ struct page {
struct { /* Second tail page of compound page */
unsigned long _compound_pad_1; /* compound_head */
unsigned long _compound_pad_2;
- struct list_head deferred_list;
+ union {
+ /* Global THP deferred split list */
+ struct list_head deferred_list;
+ /* Memcg THP deferred split list */
+ struct list_head memcg_deferred_list;
+ };
};
struct { /* Page table pages */
unsigned long _pt_pad_1; /* compound_head */
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 9f8bce9..0b9cfe1 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -492,12 +492,6 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
return pmd;
}
-static inline struct list_head *page_deferred_list(struct page *page)
-{
- /* ->lru in the tail pages is occupied by compound_head. */
- return &page[2].deferred_list;
-}
-
void prep_transhuge_page(struct page *page)
{
/*
@@ -505,7 +499,10 @@ void prep_transhuge_page(struct page *page)
* as list_head: assuming THP order >= 2
*/
- INIT_LIST_HEAD(page_deferred_list(page));
+ if (mem_cgroup_disabled())
+ INIT_LIST_HEAD(page_deferred_list(page));
+ else
+ INIT_LIST_HEAD(page_memcg_deferred_list(page));
set_compound_page_dtor(page, TRANSHUGE_PAGE_DTOR);
}
@@ -2664,6 +2661,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
bool mlocked;
unsigned long flags;
pgoff_t end;
+ struct mem_cgroup *memcg = head->mem_cgroup;
VM_BUG_ON_PAGE(is_huge_zero_page(page), page);
VM_BUG_ON_PAGE(!PageLocked(page), page);
@@ -2744,17 +2742,30 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
}
/* Prevent deferred_split_scan() touching ->_refcount */
- spin_lock(&pgdata->split_queue_lock);
+ if (!memcg)
+ spin_lock(&pgdata->split_queue_lock);
+ else
+ spin_lock(&memcg->split_queue_lock);
count = page_count(head);
mapcount = total_mapcount(head);
if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) {
- if (!list_empty(page_deferred_list(head))) {
- pgdata->split_queue_len--;
- list_del(page_deferred_list(head));
+ if (!memcg) {
+ if (!list_empty(page_deferred_list(head))) {
+ pgdata->split_queue_len--;
+ list_del(page_deferred_list(head));
+ }
+ } else {
+ if (!list_empty(page_memcg_deferred_list(head))) {
+ memcg->split_queue_len--;
+ list_del(page_memcg_deferred_list(head));
+ }
}
if (mapping)
__dec_node_page_state(page, NR_SHMEM_THPS);
- spin_unlock(&pgdata->split_queue_lock);
+ if (!memcg)
+ spin_unlock(&pgdata->split_queue_lock);
+ else
+ spin_unlock(&memcg->split_queue_lock);
__split_huge_page(page, list, end, flags);
if (PageSwapCache(head)) {
swp_entry_t entry = { .val = page_private(head) };
@@ -2771,7 +2782,10 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
dump_page(page, "total_mapcount(head) > 0");
BUG();
}
- spin_unlock(&pgdata->split_queue_lock);
+ if (!memcg)
+ spin_unlock(&pgdata->split_queue_lock);
+ else
+ spin_unlock(&memcg->split_queue_lock);
fail: if (mapping)
xa_unlock(&mapping->i_pages);
spin_unlock_irqrestore(&pgdata->lru_lock, flags);
@@ -2791,17 +2805,40 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
return ret;
}
-void free_transhuge_page(struct page *page)
+void del_thp_from_deferred_split_queue(struct page *page)
{
struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
unsigned long flags;
+ struct mem_cgroup *memcg = compound_head(page)->mem_cgroup;
- spin_lock_irqsave(&pgdata->split_queue_lock, flags);
- if (!list_empty(page_deferred_list(page))) {
- pgdata->split_queue_len--;
- list_del(page_deferred_list(page));
+ /*
+ * The THP may be not on LRU at this point, e.g. the old page of
+ * NUMA migration. And PageTransHuge is not enough to distinguish
+ * with other compound page, e.g. skb, THP destructor is not used
+ * anymore and will be removed, so the compound order sounds like
+ * the only choice here.
+ */
+ if (PageTransHuge(page) && compound_order(page) == HPAGE_PMD_ORDER) {
+ if (!memcg) {
+ spin_lock_irqsave(&pgdata->split_queue_lock, flags);
+ if (!list_empty(page_deferred_list(page))) {
+ pgdata->split_queue_len--;
+ list_del(page_deferred_list(page));
+ }
+ spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
+ } else {
+ spin_lock_irqsave(&memcg->split_queue_lock, flags);
+ if (!list_empty(page_memcg_deferred_list(page))) {
+ memcg->split_queue_len--;
+ list_del(page_memcg_deferred_list(page));
+ }
+ spin_unlock_irqrestore(&memcg->split_queue_lock, flags);
+ }
}
- spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
+}
+
+void free_transhuge_page(struct page *page)
+{
free_compound_page(page);
}
@@ -2809,23 +2846,41 @@ void deferred_split_huge_page(struct page *page)
{
struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
unsigned long flags;
+ struct mem_cgroup *memcg;
VM_BUG_ON_PAGE(!PageTransHuge(page), page);
- spin_lock_irqsave(&pgdata->split_queue_lock, flags);
- if (list_empty(page_deferred_list(page))) {
- count_vm_event(THP_DEFERRED_SPLIT_PAGE);
- list_add_tail(page_deferred_list(page), &pgdata->split_queue);
- pgdata->split_queue_len++;
+ memcg = compound_head(page)->mem_cgroup;
+ if (!memcg) {
+ spin_lock_irqsave(&pgdata->split_queue_lock, flags);
+ if (list_empty(page_deferred_list(page))) {
+ count_vm_event(THP_DEFERRED_SPLIT_PAGE);
+ list_add_tail(page_deferred_list(page), &pgdata->split_queue);
+ pgdata->split_queue_len++;
+ }
+ spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
+ } else {
+ spin_lock_irqsave(&memcg->split_queue_lock, flags);
+ if (list_empty(page_memcg_deferred_list(page))) {
+ count_vm_event(THP_DEFERRED_SPLIT_PAGE);
+ list_add_tail(page_memcg_deferred_list(page),
+ &memcg->split_queue);
+ memcg->split_queue_len++;
+ memcg_set_shrinker_bit(memcg, page_to_nid(page),
+ deferred_split_shrinker.id);
+ }
+ spin_unlock_irqrestore(&memcg->split_queue_lock, flags);
}
- spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
}
static unsigned long deferred_split_count(struct shrinker *shrink,
struct shrink_control *sc)
{
- struct pglist_data *pgdata = NODE_DATA(sc->nid);
- return READ_ONCE(pgdata->split_queue_len);
+ if (!sc->memcg) {
+ struct pglist_data *pgdata = NODE_DATA(sc->nid);
+ return READ_ONCE(pgdata->split_queue_len);
+ } else
+ return READ_ONCE(sc->memcg->split_queue_len);
}
static unsigned long deferred_split_scan(struct shrinker *shrink,
@@ -2837,22 +2892,40 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
struct page *page;
int split = 0;
- spin_lock_irqsave(&pgdata->split_queue_lock, flags);
- /* Take pin on all head pages to avoid freeing them under us */
- list_for_each_safe(pos, next, &pgdata->split_queue) {
- page = list_entry((void *)pos, struct page, mapping);
- page = compound_head(page);
- if (get_page_unless_zero(page)) {
- list_move(page_deferred_list(page), &list);
- } else {
- /* We lost race with put_compound_page() */
- list_del_init(page_deferred_list(page));
- pgdata->split_queue_len--;
+ if (!sc->memcg) {
+ spin_lock_irqsave(&pgdata->split_queue_lock, flags);
+ /* Take pin on all head pages to avoid freeing them under us */
+ list_for_each_safe(pos, next, &pgdata->split_queue) {
+ page = list_entry((void *)pos, struct page, mapping);
+ page = compound_head(page);
+ if (get_page_unless_zero(page)) {
+ list_move(page_deferred_list(page), &list);
+ } else {
+ /* We lost race with put_compound_page() */
+ list_del_init(page_deferred_list(page));
+ pgdata->split_queue_len--;
+ }
+ if (!--sc->nr_to_scan)
+ break;
}
- if (!--sc->nr_to_scan)
- break;
+ spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
+ } else {
+ spin_lock_irqsave(&sc->memcg->split_queue_lock, flags);
+ list_for_each_safe(pos, next, &sc->memcg->split_queue) {
+ page = list_entry((void *)pos, struct page, mapping);
+ page = compound_head(page);
+ if (get_page_unless_zero(page)) {
+ list_move(page_memcg_deferred_list(page), &list);
+ } else {
+ /* We lost race with put_compound_page() */
+ list_del_init(page_memcg_deferred_list(page));
+ sc->memcg->split_queue_len--;
+ }
+ if (!--sc->nr_to_scan)
+ break;
+ }
+ spin_unlock_irqrestore(&sc->memcg->split_queue_lock, flags);
}
- spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
list_for_each_safe(pos, next, &list) {
page = list_entry((void *)pos, struct page, mapping);
@@ -2866,16 +2939,29 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
put_page(page);
}
- spin_lock_irqsave(&pgdata->split_queue_lock, flags);
- list_splice_tail(&list, &pgdata->split_queue);
- spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
-
+ if (!sc->memcg) {
+ spin_lock_irqsave(&pgdata->split_queue_lock, flags);
+ list_splice_tail(&list, &pgdata->split_queue);
+ spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
+ } else {
+ spin_lock_irqsave(&sc->memcg->split_queue_lock, flags);
+ list_splice_tail(&list, &sc->memcg->split_queue);
+ spin_unlock_irqrestore(&sc->memcg->split_queue_lock, flags);
+ }
/*
* Stop shrinker if we didn't split any page, but the queue is empty.
* This can happen if pages were freed under us.
*/
- if (!split && list_empty(&pgdata->split_queue))
- return SHRINK_STOP;
+ if (!split) {
+ if (!sc->memcg) {
+ if (list_empty(&pgdata->split_queue))
+ return SHRINK_STOP;
+ } else {
+ if (list_empty(&sc->memcg->split_queue))
+ return SHRINK_STOP;
+ }
+ }
+
return split;
}
@@ -2883,7 +2969,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
.count_objects = deferred_split_count,
.scan_objects = deferred_split_scan,
.seeks = DEFAULT_SEEKS,
- .flags = SHRINKER_NUMA_AWARE,
+ .flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE,
};
#ifdef CONFIG_DEBUG_FS
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index e50a2db..6418fa0 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4579,6 +4579,11 @@ static struct mem_cgroup *mem_cgroup_alloc(void)
#ifdef CONFIG_CGROUP_WRITEBACK
INIT_LIST_HEAD(&memcg->cgwb_list);
#endif
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+ spin_lock_init(&memcg->split_queue_lock);
+ INIT_LIST_HEAD(&memcg->split_queue);
+ memcg->split_queue_len = 0;
+#endif
idr_replace(&mem_cgroup_idr, memcg, memcg->id.id);
return memcg;
fail:
@@ -4949,6 +4954,21 @@ static int mem_cgroup_move_account(struct page *page,
__mod_memcg_state(to, NR_WRITEBACK, nr_pages);
}
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+ if (compound && !list_empty(page_memcg_deferred_list(page))) {
+ spin_lock(&from->split_queue_lock);
+ list_del(page_memcg_deferred_list(page));
+ from->split_queue_len--;
+ spin_unlock(&from->split_queue_lock);
+
+ spin_lock(&to->split_queue_lock);
+ list_add_tail(page_memcg_deferred_list(page),
+ &to->split_queue);
+ to->split_queue_len++;
+ spin_unlock(&to->split_queue_lock);
+ }
+#endif
+
/*
* It is safe to change page->mem_cgroup here because the page
* is referenced, charged, and isolated - we can't race with
diff --git a/mm/swap.c b/mm/swap.c
index 3a75722..3348295 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -69,6 +69,10 @@ static void __page_cache_release(struct page *page)
del_page_from_lru_list(page, lruvec, page_off_lru(page));
spin_unlock_irqrestore(&pgdat->lru_lock, flags);
}
+
+ /* Delete THP from deferred split queue before memcg uncharge */
+ del_thp_from_deferred_split_queue(page);
+
__ClearPageWaiters(page);
mem_cgroup_uncharge(page);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/3] mm: thp: remove THP destructor
2019-05-28 12:44 [RFC PATCH 0/3] Make deferred split shrinker memcg aware Yang Shi
2019-05-28 12:44 ` [PATCH 1/3] mm: thp: make " Yang Shi
@ 2019-05-28 12:44 ` Yang Shi
2019-05-28 12:44 ` [PATCH 3/3] mm: shrinker: make shrinker not depend on memcg kmem Yang Shi
2019-05-29 1:22 ` [RFC PATCH 0/3] Make deferred split shrinker memcg aware David Rientjes
3 siblings, 0 replies; 20+ messages in thread
From: Yang Shi @ 2019-05-28 12:44 UTC (permalink / raw)
To: ktkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm
Cc: yang.shi, linux-mm, linux-kernel
The THP destructor is used to delete THP from per node deferred split
queue, now the operation is moved out of it, so the destructor is not
used anymore, remove it.
Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Shakeel Butt <shakeelb@google.com>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
include/linux/mm.h | 3 ---
mm/huge_memory.c | 6 ------
mm/page_alloc.c | 3 ---
3 files changed, 12 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 0e8834a..e543984 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -740,9 +740,6 @@ enum compound_dtor_id {
#ifdef CONFIG_HUGETLB_PAGE
HUGETLB_PAGE_DTOR,
#endif
-#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- TRANSHUGE_PAGE_DTOR,
-#endif
NR_COMPOUND_DTORS,
};
extern compound_page_dtor * const compound_page_dtors[];
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 0b9cfe1..91a709e 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -503,7 +503,6 @@ void prep_transhuge_page(struct page *page)
INIT_LIST_HEAD(page_deferred_list(page));
else
INIT_LIST_HEAD(page_memcg_deferred_list(page));
- set_compound_page_dtor(page, TRANSHUGE_PAGE_DTOR);
}
static unsigned long __thp_get_unmapped_area(struct file *filp, unsigned long len,
@@ -2837,11 +2836,6 @@ void del_thp_from_deferred_split_queue(struct page *page)
}
}
-void free_transhuge_page(struct page *page)
-{
- free_compound_page(page);
-}
-
void deferred_split_huge_page(struct page *page)
{
struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3b13d39..7d39b91 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -261,9 +261,6 @@ bool pm_suspended_storage(void)
#ifdef CONFIG_HUGETLB_PAGE
free_huge_page,
#endif
-#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- free_transhuge_page,
-#endif
};
int min_free_kbytes = 1024;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/3] mm: shrinker: make shrinker not depend on memcg kmem
2019-05-28 12:44 [RFC PATCH 0/3] Make deferred split shrinker memcg aware Yang Shi
2019-05-28 12:44 ` [PATCH 1/3] mm: thp: make " Yang Shi
2019-05-28 12:44 ` [PATCH 2/3] mm: thp: remove THP destructor Yang Shi
@ 2019-05-28 12:44 ` Yang Shi
2019-05-30 12:08 ` Kirill A. Shutemov
2019-05-29 1:22 ` [RFC PATCH 0/3] Make deferred split shrinker memcg aware David Rientjes
3 siblings, 1 reply; 20+ messages in thread
From: Yang Shi @ 2019-05-28 12:44 UTC (permalink / raw)
To: ktkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm
Cc: yang.shi, linux-mm, linux-kernel
Currently shrinker is just allocated and can work when memcg kmem is
enabled. But, THP deferred split shrinker is not slab shrinker, it
doesn't make too much sense to have such shrinker depend on memcg kmem.
It should be able to reclaim THP even though memcg kmem is disabled.
Introduce a new shrinker flag, SHRINKER_NONSLAB, for non-slab shrinker,
i.e. THP deferred split shrinker. When memcg kmem is disabled, just
such shrinkers can be called in shrinking memcg slab.
Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Shakeel Butt <shakeelb@google.com>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
include/linux/shrinker.h | 3 +--
mm/huge_memory.c | 3 ++-
mm/vmscan.c | 27 ++++++---------------------
3 files changed, 9 insertions(+), 24 deletions(-)
diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 9443caf..e18dc00 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -69,10 +69,8 @@ struct shrinker {
/* These are for internal use */
struct list_head list;
-#ifdef CONFIG_MEMCG_KMEM
/* ID in shrinker_idr */
int id;
-#endif
/* objs pending delete, per node */
atomic_long_t *nr_deferred;
};
@@ -81,6 +79,7 @@ struct shrinker {
/* Flags */
#define SHRINKER_NUMA_AWARE (1 << 0)
#define SHRINKER_MEMCG_AWARE (1 << 1)
+#define SHRINKER_NONSLAB (1 << 3)
extern int prealloc_shrinker(struct shrinker *shrinker);
extern void register_shrinker_prepared(struct shrinker *shrinker);
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 91a709e..b01fdc3 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2963,7 +2963,8 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
.count_objects = deferred_split_count,
.scan_objects = deferred_split_scan,
.seeks = DEFAULT_SEEKS,
- .flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE,
+ .flags = SHRINKER_NUMA_AWARE | SHRINKER_MEMCG_AWARE |
+ SHRINKER_NONSLAB,
};
#ifdef CONFIG_DEBUG_FS
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 7acd0af..62000ae 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -174,8 +174,6 @@ struct scan_control {
static LIST_HEAD(shrinker_list);
static DECLARE_RWSEM(shrinker_rwsem);
-#ifdef CONFIG_MEMCG_KMEM
-
/*
* We allow subsystems to populate their shrinker-related
* LRU lists before register_shrinker_prepared() is called
@@ -227,16 +225,6 @@ static void unregister_memcg_shrinker(struct shrinker *shrinker)
idr_remove(&shrinker_idr, id);
up_write(&shrinker_rwsem);
}
-#else /* CONFIG_MEMCG_KMEM */
-static int prealloc_memcg_shrinker(struct shrinker *shrinker)
-{
- return 0;
-}
-
-static void unregister_memcg_shrinker(struct shrinker *shrinker)
-{
-}
-#endif /* CONFIG_MEMCG_KMEM */
#ifdef CONFIG_MEMCG
static bool global_reclaim(struct scan_control *sc)
@@ -579,7 +567,6 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
return freed;
}
-#ifdef CONFIG_MEMCG_KMEM
static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
struct mem_cgroup *memcg, int priority)
{
@@ -587,7 +574,7 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
unsigned long ret, freed = 0;
int i;
- if (!memcg_kmem_enabled() || !mem_cgroup_online(memcg))
+ if (!mem_cgroup_online(memcg))
return 0;
if (!down_read_trylock(&shrinker_rwsem))
@@ -613,6 +600,11 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
continue;
}
+ /* Call non-slab shrinkers even though kmem is disabled */
+ if (!memcg_kmem_enabled() &&
+ !(shrinker->flags & SHRINKER_NONSLAB))
+ continue;
+
ret = do_shrink_slab(&sc, shrinker, priority);
if (ret == SHRINK_EMPTY) {
clear_bit(i, map->map);
@@ -649,13 +641,6 @@ static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
up_read(&shrinker_rwsem);
return freed;
}
-#else /* CONFIG_MEMCG_KMEM */
-static unsigned long shrink_slab_memcg(gfp_t gfp_mask, int nid,
- struct mem_cgroup *memcg, int priority)
-{
- return 0;
-}
-#endif /* CONFIG_MEMCG_KMEM */
/**
* shrink_slab - shrink slab caches
--
1.8.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware
2019-05-28 12:44 ` [PATCH 1/3] mm: thp: make " Yang Shi
@ 2019-05-28 14:42 ` Kirill Tkhai
2019-05-29 2:43 ` Yang Shi
2019-05-30 12:07 ` Kirill A. Shutemov
1 sibling, 1 reply; 20+ messages in thread
From: Kirill Tkhai @ 2019-05-28 14:42 UTC (permalink / raw)
To: Yang Shi, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm
Cc: linux-mm, linux-kernel
Hi, Yang,
On 28.05.2019 15:44, Yang Shi wrote:
> Currently THP deferred split shrinker is not memcg aware, this may cause
> premature OOM with some configuration. For example the below test would
> run into premature OOM easily:
>
> $ cgcreate -g memory:thp
> $ echo 4G > /sys/fs/cgroup/memory/thp/memory/limit_in_bytes
> $ cgexec -g memory:thp transhuge-stress 4000
>
> transhuge-stress comes from kernel selftest.
>
> It is easy to hit OOM, but there are still a lot THP on the deferred
> split queue, memcg direct reclaim can't touch them since the deferred
> split shrinker is not memcg aware.
>
> Convert deferred split shrinker memcg aware by introducing per memcg
> deferred split queue. The THP should be on either per node or per memcg
> deferred split queue if it belongs to a memcg. When the page is
> immigrated to the other memcg, it will be immigrated to the target
> memcg's deferred split queue too.
>
> And, move deleting THP from deferred split queue in page free before
> memcg uncharge so that the page's memcg information is available.
>
> Reuse the second tail page's deferred_list for per memcg list since the
> same THP can't be on multiple deferred split queues.
>
> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Shakeel Butt <shakeelb@google.com>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> ---
> include/linux/huge_mm.h | 24 ++++++
> include/linux/memcontrol.h | 6 ++
> include/linux/mm_types.h | 7 +-
> mm/huge_memory.c | 182 +++++++++++++++++++++++++++++++++------------
> mm/memcontrol.c | 20 +++++
> mm/swap.c | 4 +
> 6 files changed, 194 insertions(+), 49 deletions(-)
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index 7cd5c15..f6d1cde 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -250,6 +250,26 @@ static inline bool thp_migration_supported(void)
> return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
> }
>
> +static inline struct list_head *page_deferred_list(struct page *page)
> +{
> + /*
> + * Global deferred list in the second tail pages is occupied by
> + * compound_head.
> + */
> + return &page[2].deferred_list;
> +}
> +
> +static inline struct list_head *page_memcg_deferred_list(struct page *page)
> +{
> + /*
> + * Memcg deferred list in the second tail pages is occupied by
> + * compound_head.
> + */
> + return &page[2].memcg_deferred_list;
> +}
> +
> +extern void del_thp_from_deferred_split_queue(struct page *);
> +
> #else /* CONFIG_TRANSPARENT_HUGEPAGE */
> #define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })
> #define HPAGE_PMD_MASK ({ BUILD_BUG(); 0; })
> @@ -368,6 +388,10 @@ static inline bool thp_migration_supported(void)
> {
> return false;
> }
> +
> +static inline void del_thp_from_deferred_split_queue(struct page *page)
> +{
> +}
> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>
> #endif /* _LINUX_HUGE_MM_H */
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index bc74d6a..9ff5fab 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -316,6 +316,12 @@ struct mem_cgroup {
> struct list_head event_list;
> spinlock_t event_list_lock;
>
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> + struct list_head split_queue;
> + unsigned long split_queue_len;
> + spinlock_t split_queue_lock;
> +#endif
> +
> struct mem_cgroup_per_node *nodeinfo[0];
> /* WARNING: nodeinfo must be the last member here */
> };
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 8ec38b1..405f5e6 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -139,7 +139,12 @@ struct page {
> struct { /* Second tail page of compound page */
> unsigned long _compound_pad_1; /* compound_head */
> unsigned long _compound_pad_2;
> - struct list_head deferred_list;
> + union {
> + /* Global THP deferred split list */
> + struct list_head deferred_list;
> + /* Memcg THP deferred split list */
> + struct list_head memcg_deferred_list;
Why we need two namesakes for this list entry?
For me it looks redundantly: it does not give additional information,
but it leads to duplication (and we have two helpers page_deferred_list()
and page_memcg_deferred_list() instead of one).
> + };
> };
> struct { /* Page table pages */
> unsigned long _pt_pad_1; /* compound_head */
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 9f8bce9..0b9cfe1 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -492,12 +492,6 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
> return pmd;
> }
>
> -static inline struct list_head *page_deferred_list(struct page *page)
> -{
> - /* ->lru in the tail pages is occupied by compound_head. */
> - return &page[2].deferred_list;
> -}
> -
> void prep_transhuge_page(struct page *page)
> {
> /*
> @@ -505,7 +499,10 @@ void prep_transhuge_page(struct page *page)
> * as list_head: assuming THP order >= 2
> */
>
> - INIT_LIST_HEAD(page_deferred_list(page));
> + if (mem_cgroup_disabled())
> + INIT_LIST_HEAD(page_deferred_list(page));
> + else
> + INIT_LIST_HEAD(page_memcg_deferred_list(page));
> set_compound_page_dtor(page, TRANSHUGE_PAGE_DTOR);
> }
>
> @@ -2664,6 +2661,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
> bool mlocked;
> unsigned long flags;
> pgoff_t end;
> + struct mem_cgroup *memcg = head->mem_cgroup;
>
> VM_BUG_ON_PAGE(is_huge_zero_page(page), page);
> VM_BUG_ON_PAGE(!PageLocked(page), page);
> @@ -2744,17 +2742,30 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
> }
>
> /* Prevent deferred_split_scan() touching ->_refcount */
> - spin_lock(&pgdata->split_queue_lock);
> + if (!memcg)
> + spin_lock(&pgdata->split_queue_lock);
> + else
> + spin_lock(&memcg->split_queue_lock);
> count = page_count(head);
> mapcount = total_mapcount(head);
> if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) {
> - if (!list_empty(page_deferred_list(head))) {
> - pgdata->split_queue_len--;
> - list_del(page_deferred_list(head));
> + if (!memcg) {
> + if (!list_empty(page_deferred_list(head))) {
> + pgdata->split_queue_len--;
> + list_del(page_deferred_list(head));
> + }
> + } else {
> + if (!list_empty(page_memcg_deferred_list(head))) {
> + memcg->split_queue_len--;
> + list_del(page_memcg_deferred_list(head));
> + }
> }
> if (mapping)
> __dec_node_page_state(page, NR_SHMEM_THPS);
> - spin_unlock(&pgdata->split_queue_lock);
> + if (!memcg)
> + spin_unlock(&pgdata->split_queue_lock);
> + else
> + spin_unlock(&memcg->split_queue_lock);
> __split_huge_page(page, list, end, flags);
> if (PageSwapCache(head)) {
> swp_entry_t entry = { .val = page_private(head) };
> @@ -2771,7 +2782,10 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
> dump_page(page, "total_mapcount(head) > 0");
> BUG();
> }
> - spin_unlock(&pgdata->split_queue_lock);
> + if (!memcg)
> + spin_unlock(&pgdata->split_queue_lock);
> + else
> + spin_unlock(&memcg->split_queue_lock);
> fail: if (mapping)
> xa_unlock(&mapping->i_pages);
> spin_unlock_irqrestore(&pgdata->lru_lock, flags);
> @@ -2791,17 +2805,40 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
> return ret;
> }
>
> -void free_transhuge_page(struct page *page)
> +void del_thp_from_deferred_split_queue(struct page *page)
> {
> struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
> unsigned long flags;
> + struct mem_cgroup *memcg = compound_head(page)->mem_cgroup;
>
> - spin_lock_irqsave(&pgdata->split_queue_lock, flags);
> - if (!list_empty(page_deferred_list(page))) {
> - pgdata->split_queue_len--;
> - list_del(page_deferred_list(page));
> + /*
> + * The THP may be not on LRU at this point, e.g. the old page of
> + * NUMA migration. And PageTransHuge is not enough to distinguish
> + * with other compound page, e.g. skb, THP destructor is not used
> + * anymore and will be removed, so the compound order sounds like
> + * the only choice here.
> + */
> + if (PageTransHuge(page) && compound_order(page) == HPAGE_PMD_ORDER) {
> + if (!memcg) {
> + spin_lock_irqsave(&pgdata->split_queue_lock, flags);
> + if (!list_empty(page_deferred_list(page))) {
> + pgdata->split_queue_len--;
> + list_del(page_deferred_list(page));
> + }
> + spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
> + } else {
> + spin_lock_irqsave(&memcg->split_queue_lock, flags);
> + if (!list_empty(page_memcg_deferred_list(page))) {
> + memcg->split_queue_len--;
> + list_del(page_memcg_deferred_list(page));
> + }
> + spin_unlock_irqrestore(&memcg->split_queue_lock, flags);
Such the patterns look like a duplication of functionality, we already have
in list_lru: it handles both root_mem_cgroup and all children memcg.
Should we try to reuse that code, and to switch huge pages shrinker
into generic code?
(Yeah, currently we allocate memcg_cache_ida IDS only for kmem, but we may
consider to allocate them for any cases, since now we have new memcg shrinkers
like you introduce).
[...]
Thanks,
Kirill
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 0/3] Make deferred split shrinker memcg aware
2019-05-28 12:44 [RFC PATCH 0/3] Make deferred split shrinker memcg aware Yang Shi
` (2 preceding siblings ...)
2019-05-28 12:44 ` [PATCH 3/3] mm: shrinker: make shrinker not depend on memcg kmem Yang Shi
@ 2019-05-29 1:22 ` David Rientjes
2019-05-29 2:34 ` Yang Shi
3 siblings, 1 reply; 20+ messages in thread
From: David Rientjes @ 2019-05-29 1:22 UTC (permalink / raw)
To: Yang Shi
Cc: ktkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm,
linux-mm, linux-kernel
On Tue, 28 May 2019, Yang Shi wrote:
>
> I got some reports from our internal application team about memcg OOM.
> Even though the application has been killed by oom killer, there are
> still a lot THPs reside, page reclaim doesn't reclaim them at all.
>
> Some investigation shows they are on deferred split queue, memcg direct
> reclaim can't shrink them since THP deferred split shrinker is not memcg
> aware, this may cause premature OOM in memcg. The issue can be
> reproduced easily by the below test:
>
Right, we've also encountered this. I talked to Kirill about it a week or
so ago where the suggestion was to split all compound pages on the
deferred split queues under the presence of even memory pressure.
That breaks cgroup isolation and perhaps unfairly penalizes workloads that
are running attached to other memcg hierarchies that are not under
pressure because their compound pages are now split as a side effect.
There is a benefit to keeping these compound pages around while not under
memory pressure if all pages are subsequently mapped again.
> $ cgcreate -g memory:thp
> $ echo 4G > /sys/fs/cgroup/memory/thp/memory/limit_in_bytes
> $ cgexec -g memory:thp ./transhuge-stress 4000
>
> transhuge-stress comes from kernel selftest.
>
> It is easy to hit OOM, but there are still a lot THP on the deferred split
> queue, memcg direct reclaim can't touch them since the deferred split
> shrinker is not memcg aware.
>
Yes, we have seen this on at least 4.15 as well.
> Convert deferred split shrinker memcg aware by introducing per memcg deferred
> split queue. The THP should be on either per node or per memcg deferred
> split queue if it belongs to a memcg. When the page is immigrated to the
> other memcg, it will be immigrated to the target memcg's deferred split queue
> too.
>
> And, move deleting THP from deferred split queue in page free before memcg
> uncharge so that the page's memcg information is available.
>
> Reuse the second tail page's deferred_list for per memcg list since the same
> THP can't be on multiple deferred split queues at the same time.
>
> Remove THP specific destructor since it is not used anymore with memcg aware
> THP shrinker (Please see the commit log of patch 2/3 for the details).
>
> Make deferred split shrinker not depend on memcg kmem since it is not slab.
> It doesn't make sense to not shrink THP even though memcg kmem is disabled.
>
> With the above change the test demonstrated above doesn't trigger OOM anymore
> even though with cgroup.memory=nokmem.
>
I'm curious if your internal applications team is also asking for
statistics on how much memory can be freed if the deferred split queues
can be shrunk? We have applications that monitor their own memory usage
through memcg stats or usage and proactively try to reduce that usage when
it is growing too large. The deferred split queues have significantly
increased both memcg usage and rss when they've upgraded kernels.
How are your applications monitoring how much memory from deferred split
queues can be freed on memory pressure? Any thoughts on providing it as a
memcg stat?
Thanks!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 0/3] Make deferred split shrinker memcg aware
2019-05-29 1:22 ` [RFC PATCH 0/3] Make deferred split shrinker memcg aware David Rientjes
@ 2019-05-29 2:34 ` Yang Shi
2019-05-29 21:07 ` David Rientjes
0 siblings, 1 reply; 20+ messages in thread
From: Yang Shi @ 2019-05-29 2:34 UTC (permalink / raw)
To: David Rientjes
Cc: ktkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm,
linux-mm, linux-kernel
On 5/29/19 9:22 AM, David Rientjes wrote:
> On Tue, 28 May 2019, Yang Shi wrote:
>
>> I got some reports from our internal application team about memcg OOM.
>> Even though the application has been killed by oom killer, there are
>> still a lot THPs reside, page reclaim doesn't reclaim them at all.
>>
>> Some investigation shows they are on deferred split queue, memcg direct
>> reclaim can't shrink them since THP deferred split shrinker is not memcg
>> aware, this may cause premature OOM in memcg. The issue can be
>> reproduced easily by the below test:
>>
> Right, we've also encountered this. I talked to Kirill about it a week or
> so ago where the suggestion was to split all compound pages on the
> deferred split queues under the presence of even memory pressure.
>
> That breaks cgroup isolation and perhaps unfairly penalizes workloads that
> are running attached to other memcg hierarchies that are not under
> pressure because their compound pages are now split as a side effect.
> There is a benefit to keeping these compound pages around while not under
> memory pressure if all pages are subsequently mapped again.
Yes, I do agree. I tried other approaches too, it sounds making deferred
split queue per memcg is the optimal one.
>
>> $ cgcreate -g memory:thp
>> $ echo 4G > /sys/fs/cgroup/memory/thp/memory/limit_in_bytes
>> $ cgexec -g memory:thp ./transhuge-stress 4000
>>
>> transhuge-stress comes from kernel selftest.
>>
>> It is easy to hit OOM, but there are still a lot THP on the deferred split
>> queue, memcg direct reclaim can't touch them since the deferred split
>> shrinker is not memcg aware.
>>
> Yes, we have seen this on at least 4.15 as well.
>
>> Convert deferred split shrinker memcg aware by introducing per memcg deferred
>> split queue. The THP should be on either per node or per memcg deferred
>> split queue if it belongs to a memcg. When the page is immigrated to the
>> other memcg, it will be immigrated to the target memcg's deferred split queue
>> too.
>>
>> And, move deleting THP from deferred split queue in page free before memcg
>> uncharge so that the page's memcg information is available.
>>
>> Reuse the second tail page's deferred_list for per memcg list since the same
>> THP can't be on multiple deferred split queues at the same time.
>>
>> Remove THP specific destructor since it is not used anymore with memcg aware
>> THP shrinker (Please see the commit log of patch 2/3 for the details).
>>
>> Make deferred split shrinker not depend on memcg kmem since it is not slab.
>> It doesn't make sense to not shrink THP even though memcg kmem is disabled.
>>
>> With the above change the test demonstrated above doesn't trigger OOM anymore
>> even though with cgroup.memory=nokmem.
>>
> I'm curious if your internal applications team is also asking for
> statistics on how much memory can be freed if the deferred split queues
> can be shrunk? We have applications that monitor their own memory usage
No, but this reminds me. The THPs on deferred split queue should be
accounted into available memory too.
> through memcg stats or usage and proactively try to reduce that usage when
> it is growing too large. The deferred split queues have significantly
> increased both memcg usage and rss when they've upgraded kernels.
>
> How are your applications monitoring how much memory from deferred split
> queues can be freed on memory pressure? Any thoughts on providing it as a
> memcg stat?
I don't think they have such monitor. I saw rss_huge is abormal in memcg
stat even after the application is killed by oom, so I realized the
deferred split queue may play a role here.
The memcg stat doesn't have counters for available memory as global
vmstat. It may be better to have such statistics, or extending
reclaimable "slab" to shrinkable/reclaimable "memory".
>
> Thanks!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware
2019-05-28 14:42 ` Kirill Tkhai
@ 2019-05-29 2:43 ` Yang Shi
2019-05-29 8:14 ` Kirill Tkhai
0 siblings, 1 reply; 20+ messages in thread
From: Yang Shi @ 2019-05-29 2:43 UTC (permalink / raw)
To: Kirill Tkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm
Cc: linux-mm, linux-kernel
On 5/28/19 10:42 PM, Kirill Tkhai wrote:
> Hi, Yang,
>
> On 28.05.2019 15:44, Yang Shi wrote:
>> Currently THP deferred split shrinker is not memcg aware, this may cause
>> premature OOM with some configuration. For example the below test would
>> run into premature OOM easily:
>>
>> $ cgcreate -g memory:thp
>> $ echo 4G > /sys/fs/cgroup/memory/thp/memory/limit_in_bytes
>> $ cgexec -g memory:thp transhuge-stress 4000
>>
>> transhuge-stress comes from kernel selftest.
>>
>> It is easy to hit OOM, but there are still a lot THP on the deferred
>> split queue, memcg direct reclaim can't touch them since the deferred
>> split shrinker is not memcg aware.
>>
>> Convert deferred split shrinker memcg aware by introducing per memcg
>> deferred split queue. The THP should be on either per node or per memcg
>> deferred split queue if it belongs to a memcg. When the page is
>> immigrated to the other memcg, it will be immigrated to the target
>> memcg's deferred split queue too.
>>
>> And, move deleting THP from deferred split queue in page free before
>> memcg uncharge so that the page's memcg information is available.
>>
>> Reuse the second tail page's deferred_list for per memcg list since the
>> same THP can't be on multiple deferred split queues.
>>
>> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: Shakeel Butt <shakeelb@google.com>
>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>> ---
>> include/linux/huge_mm.h | 24 ++++++
>> include/linux/memcontrol.h | 6 ++
>> include/linux/mm_types.h | 7 +-
>> mm/huge_memory.c | 182 +++++++++++++++++++++++++++++++++------------
>> mm/memcontrol.c | 20 +++++
>> mm/swap.c | 4 +
>> 6 files changed, 194 insertions(+), 49 deletions(-)
>>
>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>> index 7cd5c15..f6d1cde 100644
>> --- a/include/linux/huge_mm.h
>> +++ b/include/linux/huge_mm.h
>> @@ -250,6 +250,26 @@ static inline bool thp_migration_supported(void)
>> return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
>> }
>>
>> +static inline struct list_head *page_deferred_list(struct page *page)
>> +{
>> + /*
>> + * Global deferred list in the second tail pages is occupied by
>> + * compound_head.
>> + */
>> + return &page[2].deferred_list;
>> +}
>> +
>> +static inline struct list_head *page_memcg_deferred_list(struct page *page)
>> +{
>> + /*
>> + * Memcg deferred list in the second tail pages is occupied by
>> + * compound_head.
>> + */
>> + return &page[2].memcg_deferred_list;
>> +}
>> +
>> +extern void del_thp_from_deferred_split_queue(struct page *);
>> +
>> #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>> #define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })
>> #define HPAGE_PMD_MASK ({ BUILD_BUG(); 0; })
>> @@ -368,6 +388,10 @@ static inline bool thp_migration_supported(void)
>> {
>> return false;
>> }
>> +
>> +static inline void del_thp_from_deferred_split_queue(struct page *page)
>> +{
>> +}
>> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>
>> #endif /* _LINUX_HUGE_MM_H */
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index bc74d6a..9ff5fab 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -316,6 +316,12 @@ struct mem_cgroup {
>> struct list_head event_list;
>> spinlock_t event_list_lock;
>>
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> + struct list_head split_queue;
>> + unsigned long split_queue_len;
>> + spinlock_t split_queue_lock;
>> +#endif
>> +
>> struct mem_cgroup_per_node *nodeinfo[0];
>> /* WARNING: nodeinfo must be the last member here */
>> };
>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>> index 8ec38b1..405f5e6 100644
>> --- a/include/linux/mm_types.h
>> +++ b/include/linux/mm_types.h
>> @@ -139,7 +139,12 @@ struct page {
>> struct { /* Second tail page of compound page */
>> unsigned long _compound_pad_1; /* compound_head */
>> unsigned long _compound_pad_2;
>> - struct list_head deferred_list;
>> + union {
>> + /* Global THP deferred split list */
>> + struct list_head deferred_list;
>> + /* Memcg THP deferred split list */
>> + struct list_head memcg_deferred_list;
> Why we need two namesakes for this list entry?
>
> For me it looks redundantly: it does not give additional information,
> but it leads to duplication (and we have two helpers page_deferred_list()
> and page_memcg_deferred_list() instead of one).
Yes, kind of. Actually I was also wondering if this is worth or not. My
point is this may improve the code readability. We can figure out what
split queue (per node or per memcg) is being manipulated just by the
name of the list.
If the most people thought this is unnecessary, I'm definitely ok to
just keep one name.
>
>> + };
>> };
>> struct { /* Page table pages */
>> unsigned long _pt_pad_1; /* compound_head */
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 9f8bce9..0b9cfe1 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -492,12 +492,6 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
>> return pmd;
>> }
>>
>> -static inline struct list_head *page_deferred_list(struct page *page)
>> -{
>> - /* ->lru in the tail pages is occupied by compound_head. */
>> - return &page[2].deferred_list;
>> -}
>> -
>> void prep_transhuge_page(struct page *page)
>> {
>> /*
>> @@ -505,7 +499,10 @@ void prep_transhuge_page(struct page *page)
>> * as list_head: assuming THP order >= 2
>> */
>>
>> - INIT_LIST_HEAD(page_deferred_list(page));
>> + if (mem_cgroup_disabled())
>> + INIT_LIST_HEAD(page_deferred_list(page));
>> + else
>> + INIT_LIST_HEAD(page_memcg_deferred_list(page));
>> set_compound_page_dtor(page, TRANSHUGE_PAGE_DTOR);
>> }
>>
>> @@ -2664,6 +2661,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>> bool mlocked;
>> unsigned long flags;
>> pgoff_t end;
>> + struct mem_cgroup *memcg = head->mem_cgroup;
>>
>> VM_BUG_ON_PAGE(is_huge_zero_page(page), page);
>> VM_BUG_ON_PAGE(!PageLocked(page), page);
>> @@ -2744,17 +2742,30 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>> }
>>
>> /* Prevent deferred_split_scan() touching ->_refcount */
>> - spin_lock(&pgdata->split_queue_lock);
>> + if (!memcg)
>> + spin_lock(&pgdata->split_queue_lock);
>> + else
>> + spin_lock(&memcg->split_queue_lock);
>> count = page_count(head);
>> mapcount = total_mapcount(head);
>> if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) {
>> - if (!list_empty(page_deferred_list(head))) {
>> - pgdata->split_queue_len--;
>> - list_del(page_deferred_list(head));
>> + if (!memcg) {
>> + if (!list_empty(page_deferred_list(head))) {
>> + pgdata->split_queue_len--;
>> + list_del(page_deferred_list(head));
>> + }
>> + } else {
>> + if (!list_empty(page_memcg_deferred_list(head))) {
>> + memcg->split_queue_len--;
>> + list_del(page_memcg_deferred_list(head));
>> + }
>> }
>> if (mapping)
>> __dec_node_page_state(page, NR_SHMEM_THPS);
>> - spin_unlock(&pgdata->split_queue_lock);
>> + if (!memcg)
>> + spin_unlock(&pgdata->split_queue_lock);
>> + else
>> + spin_unlock(&memcg->split_queue_lock);
>> __split_huge_page(page, list, end, flags);
>> if (PageSwapCache(head)) {
>> swp_entry_t entry = { .val = page_private(head) };
>> @@ -2771,7 +2782,10 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>> dump_page(page, "total_mapcount(head) > 0");
>> BUG();
>> }
>> - spin_unlock(&pgdata->split_queue_lock);
>> + if (!memcg)
>> + spin_unlock(&pgdata->split_queue_lock);
>> + else
>> + spin_unlock(&memcg->split_queue_lock);
>> fail: if (mapping)
>> xa_unlock(&mapping->i_pages);
>> spin_unlock_irqrestore(&pgdata->lru_lock, flags);
>> @@ -2791,17 +2805,40 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>> return ret;
>> }
>>
>> -void free_transhuge_page(struct page *page)
>> +void del_thp_from_deferred_split_queue(struct page *page)
>> {
>> struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
>> unsigned long flags;
>> + struct mem_cgroup *memcg = compound_head(page)->mem_cgroup;
>>
>> - spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>> - if (!list_empty(page_deferred_list(page))) {
>> - pgdata->split_queue_len--;
>> - list_del(page_deferred_list(page));
>> + /*
>> + * The THP may be not on LRU at this point, e.g. the old page of
>> + * NUMA migration. And PageTransHuge is not enough to distinguish
>> + * with other compound page, e.g. skb, THP destructor is not used
>> + * anymore and will be removed, so the compound order sounds like
>> + * the only choice here.
>> + */
>> + if (PageTransHuge(page) && compound_order(page) == HPAGE_PMD_ORDER) {
>> + if (!memcg) {
>> + spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>> + if (!list_empty(page_deferred_list(page))) {
>> + pgdata->split_queue_len--;
>> + list_del(page_deferred_list(page));
>> + }
>> + spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
>> + } else {
>> + spin_lock_irqsave(&memcg->split_queue_lock, flags);
>> + if (!list_empty(page_memcg_deferred_list(page))) {
>> + memcg->split_queue_len--;
>> + list_del(page_memcg_deferred_list(page));
>> + }
>> + spin_unlock_irqrestore(&memcg->split_queue_lock, flags);
> Such the patterns look like a duplication of functionality, we already have
> in list_lru: it handles both root_mem_cgroup and all children memcg.
Would you please point me to some example code?
>
> Should we try to reuse that code, and to switch huge pages shrinker
> into generic code?
Definitely if it is feasible.
>
> (Yeah, currently we allocate memcg_cache_ida IDS only for kmem, but we may
> consider to allocate them for any cases, since now we have new memcg shrinkers
> like you introduce).
The patch 3/3 removes the restriction.
>
> [...]
>
> Thanks,
> Kirill
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware
2019-05-29 2:43 ` Yang Shi
@ 2019-05-29 8:14 ` Kirill Tkhai
2019-05-29 11:25 ` Yang Shi
0 siblings, 1 reply; 20+ messages in thread
From: Kirill Tkhai @ 2019-05-29 8:14 UTC (permalink / raw)
To: Yang Shi, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm
Cc: linux-mm, linux-kernel
On 29.05.2019 05:43, Yang Shi wrote:
>
>
> On 5/28/19 10:42 PM, Kirill Tkhai wrote:
>> Hi, Yang,
>>
>> On 28.05.2019 15:44, Yang Shi wrote:
>>> Currently THP deferred split shrinker is not memcg aware, this may cause
>>> premature OOM with some configuration. For example the below test would
>>> run into premature OOM easily:
>>>
>>> $ cgcreate -g memory:thp
>>> $ echo 4G > /sys/fs/cgroup/memory/thp/memory/limit_in_bytes
>>> $ cgexec -g memory:thp transhuge-stress 4000
>>>
>>> transhuge-stress comes from kernel selftest.
>>>
>>> It is easy to hit OOM, but there are still a lot THP on the deferred
>>> split queue, memcg direct reclaim can't touch them since the deferred
>>> split shrinker is not memcg aware.
>>>
>>> Convert deferred split shrinker memcg aware by introducing per memcg
>>> deferred split queue. The THP should be on either per node or per memcg
>>> deferred split queue if it belongs to a memcg. When the page is
>>> immigrated to the other memcg, it will be immigrated to the target
>>> memcg's deferred split queue too.
>>>
>>> And, move deleting THP from deferred split queue in page free before
>>> memcg uncharge so that the page's memcg information is available.
>>>
>>> Reuse the second tail page's deferred_list for per memcg list since the
>>> same THP can't be on multiple deferred split queues.
>>>
>>> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
>>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
>>> Cc: Hugh Dickins <hughd@google.com>
>>> Cc: Shakeel Butt <shakeelb@google.com>
>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>>> ---
>>> include/linux/huge_mm.h | 24 ++++++
>>> include/linux/memcontrol.h | 6 ++
>>> include/linux/mm_types.h | 7 +-
>>> mm/huge_memory.c | 182 +++++++++++++++++++++++++++++++++------------
>>> mm/memcontrol.c | 20 +++++
>>> mm/swap.c | 4 +
>>> 6 files changed, 194 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>> index 7cd5c15..f6d1cde 100644
>>> --- a/include/linux/huge_mm.h
>>> +++ b/include/linux/huge_mm.h
>>> @@ -250,6 +250,26 @@ static inline bool thp_migration_supported(void)
>>> return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
>>> }
>>> +static inline struct list_head *page_deferred_list(struct page *page)
>>> +{
>>> + /*
>>> + * Global deferred list in the second tail pages is occupied by
>>> + * compound_head.
>>> + */
>>> + return &page[2].deferred_list;
>>> +}
>>> +
>>> +static inline struct list_head *page_memcg_deferred_list(struct page *page)
>>> +{
>>> + /*
>>> + * Memcg deferred list in the second tail pages is occupied by
>>> + * compound_head.
>>> + */
>>> + return &page[2].memcg_deferred_list;
>>> +}
>>> +
>>> +extern void del_thp_from_deferred_split_queue(struct page *);
>>> +
>>> #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>>> #define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })
>>> #define HPAGE_PMD_MASK ({ BUILD_BUG(); 0; })
>>> @@ -368,6 +388,10 @@ static inline bool thp_migration_supported(void)
>>> {
>>> return false;
>>> }
>>> +
>>> +static inline void del_thp_from_deferred_split_queue(struct page *page)
>>> +{
>>> +}
>>> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>> #endif /* _LINUX_HUGE_MM_H */
>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>> index bc74d6a..9ff5fab 100644
>>> --- a/include/linux/memcontrol.h
>>> +++ b/include/linux/memcontrol.h
>>> @@ -316,6 +316,12 @@ struct mem_cgroup {
>>> struct list_head event_list;
>>> spinlock_t event_list_lock;
>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>> + struct list_head split_queue;
>>> + unsigned long split_queue_len;
>>> + spinlock_t split_queue_lock;
>>> +#endif
>>> +
>>> struct mem_cgroup_per_node *nodeinfo[0];
>>> /* WARNING: nodeinfo must be the last member here */
>>> };
>>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>>> index 8ec38b1..405f5e6 100644
>>> --- a/include/linux/mm_types.h
>>> +++ b/include/linux/mm_types.h
>>> @@ -139,7 +139,12 @@ struct page {
>>> struct { /* Second tail page of compound page */
>>> unsigned long _compound_pad_1; /* compound_head */
>>> unsigned long _compound_pad_2;
>>> - struct list_head deferred_list;
>>> + union {
>>> + /* Global THP deferred split list */
>>> + struct list_head deferred_list;
>>> + /* Memcg THP deferred split list */
>>> + struct list_head memcg_deferred_list;
>> Why we need two namesakes for this list entry?
>>
>> For me it looks redundantly: it does not give additional information,
>> but it leads to duplication (and we have two helpers page_deferred_list()
>> and page_memcg_deferred_list() instead of one).
>
> Yes, kind of. Actually I was also wondering if this is worth or not. My point is this may improve the code readability. We can figure out what split queue (per node or per memcg) is being manipulated just by the name of the list.
>
> If the most people thought this is unnecessary, I'm definitely ok to just keep one name.
>
>>
>>> + };
>>> };
>>> struct { /* Page table pages */
>>> unsigned long _pt_pad_1; /* compound_head */
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index 9f8bce9..0b9cfe1 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -492,12 +492,6 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
>>> return pmd;
>>> }
>>> -static inline struct list_head *page_deferred_list(struct page *page)
>>> -{
>>> - /* ->lru in the tail pages is occupied by compound_head. */
>>> - return &page[2].deferred_list;
>>> -}
>>> -
>>> void prep_transhuge_page(struct page *page)
>>> {
>>> /*
>>> @@ -505,7 +499,10 @@ void prep_transhuge_page(struct page *page)
>>> * as list_head: assuming THP order >= 2
>>> */
>>> - INIT_LIST_HEAD(page_deferred_list(page));
>>> + if (mem_cgroup_disabled())
>>> + INIT_LIST_HEAD(page_deferred_list(page));
>>> + else
>>> + INIT_LIST_HEAD(page_memcg_deferred_list(page));
>>> set_compound_page_dtor(page, TRANSHUGE_PAGE_DTOR);
>>> }
>>> @@ -2664,6 +2661,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>> bool mlocked;
>>> unsigned long flags;
>>> pgoff_t end;
>>> + struct mem_cgroup *memcg = head->mem_cgroup;
>>> VM_BUG_ON_PAGE(is_huge_zero_page(page), page);
>>> VM_BUG_ON_PAGE(!PageLocked(page), page);
>>> @@ -2744,17 +2742,30 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>> }
>>> /* Prevent deferred_split_scan() touching ->_refcount */
>>> - spin_lock(&pgdata->split_queue_lock);
>>> + if (!memcg)
>>> + spin_lock(&pgdata->split_queue_lock);
>>> + else
>>> + spin_lock(&memcg->split_queue_lock);
>>> count = page_count(head);
>>> mapcount = total_mapcount(head);
>>> if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) {
>>> - if (!list_empty(page_deferred_list(head))) {
>>> - pgdata->split_queue_len--;
>>> - list_del(page_deferred_list(head));
>>> + if (!memcg) {
>>> + if (!list_empty(page_deferred_list(head))) {
>>> + pgdata->split_queue_len--;
>>> + list_del(page_deferred_list(head));
>>> + }
>>> + } else {
>>> + if (!list_empty(page_memcg_deferred_list(head))) {
>>> + memcg->split_queue_len--;
>>> + list_del(page_memcg_deferred_list(head));
>>> + }
>>> }
>>> if (mapping)
>>> __dec_node_page_state(page, NR_SHMEM_THPS);
>>> - spin_unlock(&pgdata->split_queue_lock);
>>> + if (!memcg)
>>> + spin_unlock(&pgdata->split_queue_lock);
>>> + else
>>> + spin_unlock(&memcg->split_queue_lock);
>>> __split_huge_page(page, list, end, flags);
>>> if (PageSwapCache(head)) {
>>> swp_entry_t entry = { .val = page_private(head) };
>>> @@ -2771,7 +2782,10 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>> dump_page(page, "total_mapcount(head) > 0");
>>> BUG();
>>> }
>>> - spin_unlock(&pgdata->split_queue_lock);
>>> + if (!memcg)
>>> + spin_unlock(&pgdata->split_queue_lock);
>>> + else
>>> + spin_unlock(&memcg->split_queue_lock);
>>> fail: if (mapping)
>>> xa_unlock(&mapping->i_pages);
>>> spin_unlock_irqrestore(&pgdata->lru_lock, flags);
>>> @@ -2791,17 +2805,40 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>> return ret;
>>> }
>>> -void free_transhuge_page(struct page *page)
>>> +void del_thp_from_deferred_split_queue(struct page *page)
>>> {
>>> struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
>>> unsigned long flags;
>>> + struct mem_cgroup *memcg = compound_head(page)->mem_cgroup;
>>> - spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>>> - if (!list_empty(page_deferred_list(page))) {
>>> - pgdata->split_queue_len--;
>>> - list_del(page_deferred_list(page));
>>> + /*
>>> + * The THP may be not on LRU at this point, e.g. the old page of
>>> + * NUMA migration. And PageTransHuge is not enough to distinguish
>>> + * with other compound page, e.g. skb, THP destructor is not used
>>> + * anymore and will be removed, so the compound order sounds like
>>> + * the only choice here.
>>> + */
>>> + if (PageTransHuge(page) && compound_order(page) == HPAGE_PMD_ORDER) {
>>> + if (!memcg) {
>>> + spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>>> + if (!list_empty(page_deferred_list(page))) {
>>> + pgdata->split_queue_len--;
>>> + list_del(page_deferred_list(page));
>>> + }
>>> + spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
>>> + } else {
>>> + spin_lock_irqsave(&memcg->split_queue_lock, flags);
>>> + if (!list_empty(page_memcg_deferred_list(page))) {
>>> + memcg->split_queue_len--;
>>> + list_del(page_memcg_deferred_list(page));
>>> + }
>>> + spin_unlock_irqrestore(&memcg->split_queue_lock, flags);
>> Such the patterns look like a duplication of functionality, we already have
>> in list_lru: it handles both root_mem_cgroup and all children memcg.
>
> Would you please point me to some example code?
I mean that we do almost the same in list_lru_add(): check for whether
item is already added, find the desired list, maintain the list's len.
It looks all the above we may replace with something like
list_lru_add(defered_thp_lru, page_deferred_list(page))
after necessary preparations (some rewriting of the rest of code is needed).
>>
>> Should we try to reuse that code, and to switch huge pages shrinker
>> into generic code?
>
> Definitely if it is feasible.
>
>>
>> (Yeah, currently we allocate memcg_cache_ida IDS only for kmem, but we may
>> consider to allocate them for any cases, since now we have new memcg shrinkers
>> like you introduce).
>
> The patch 3/3 removes the restriction.
It does that for shrinker code. but there is the code in mm/memcontrol.c,
which maintains list_lru slots for all memcgs. In case of switching to
list_lru base, we will need to teach the code to maintain the slots
for !CONFIG_MEMCG_KMEM case too.
Kirill
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware
2019-05-29 8:14 ` Kirill Tkhai
@ 2019-05-29 11:25 ` Yang Shi
2019-06-10 8:23 ` Kirill Tkhai
0 siblings, 1 reply; 20+ messages in thread
From: Yang Shi @ 2019-05-29 11:25 UTC (permalink / raw)
To: Kirill Tkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm
Cc: linux-mm, linux-kernel
On 5/29/19 4:14 PM, Kirill Tkhai wrote:
> On 29.05.2019 05:43, Yang Shi wrote:
>>
>> On 5/28/19 10:42 PM, Kirill Tkhai wrote:
>>> Hi, Yang,
>>>
>>> On 28.05.2019 15:44, Yang Shi wrote:
>>>> Currently THP deferred split shrinker is not memcg aware, this may cause
>>>> premature OOM with some configuration. For example the below test would
>>>> run into premature OOM easily:
>>>>
>>>> $ cgcreate -g memory:thp
>>>> $ echo 4G > /sys/fs/cgroup/memory/thp/memory/limit_in_bytes
>>>> $ cgexec -g memory:thp transhuge-stress 4000
>>>>
>>>> transhuge-stress comes from kernel selftest.
>>>>
>>>> It is easy to hit OOM, but there are still a lot THP on the deferred
>>>> split queue, memcg direct reclaim can't touch them since the deferred
>>>> split shrinker is not memcg aware.
>>>>
>>>> Convert deferred split shrinker memcg aware by introducing per memcg
>>>> deferred split queue. The THP should be on either per node or per memcg
>>>> deferred split queue if it belongs to a memcg. When the page is
>>>> immigrated to the other memcg, it will be immigrated to the target
>>>> memcg's deferred split queue too.
>>>>
>>>> And, move deleting THP from deferred split queue in page free before
>>>> memcg uncharge so that the page's memcg information is available.
>>>>
>>>> Reuse the second tail page's deferred_list for per memcg list since the
>>>> same THP can't be on multiple deferred split queues.
>>>>
>>>> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>>>> Cc: Michal Hocko <mhocko@suse.com>
>>>> Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
>>>> Cc: Hugh Dickins <hughd@google.com>
>>>> Cc: Shakeel Butt <shakeelb@google.com>
>>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>>>> ---
>>>> include/linux/huge_mm.h | 24 ++++++
>>>> include/linux/memcontrol.h | 6 ++
>>>> include/linux/mm_types.h | 7 +-
>>>> mm/huge_memory.c | 182 +++++++++++++++++++++++++++++++++------------
>>>> mm/memcontrol.c | 20 +++++
>>>> mm/swap.c | 4 +
>>>> 6 files changed, 194 insertions(+), 49 deletions(-)
>>>>
>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>> index 7cd5c15..f6d1cde 100644
>>>> --- a/include/linux/huge_mm.h
>>>> +++ b/include/linux/huge_mm.h
>>>> @@ -250,6 +250,26 @@ static inline bool thp_migration_supported(void)
>>>> return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
>>>> }
>>>> +static inline struct list_head *page_deferred_list(struct page *page)
>>>> +{
>>>> + /*
>>>> + * Global deferred list in the second tail pages is occupied by
>>>> + * compound_head.
>>>> + */
>>>> + return &page[2].deferred_list;
>>>> +}
>>>> +
>>>> +static inline struct list_head *page_memcg_deferred_list(struct page *page)
>>>> +{
>>>> + /*
>>>> + * Memcg deferred list in the second tail pages is occupied by
>>>> + * compound_head.
>>>> + */
>>>> + return &page[2].memcg_deferred_list;
>>>> +}
>>>> +
>>>> +extern void del_thp_from_deferred_split_queue(struct page *);
>>>> +
>>>> #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>> #define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })
>>>> #define HPAGE_PMD_MASK ({ BUILD_BUG(); 0; })
>>>> @@ -368,6 +388,10 @@ static inline bool thp_migration_supported(void)
>>>> {
>>>> return false;
>>>> }
>>>> +
>>>> +static inline void del_thp_from_deferred_split_queue(struct page *page)
>>>> +{
>>>> +}
>>>> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>> #endif /* _LINUX_HUGE_MM_H */
>>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>>> index bc74d6a..9ff5fab 100644
>>>> --- a/include/linux/memcontrol.h
>>>> +++ b/include/linux/memcontrol.h
>>>> @@ -316,6 +316,12 @@ struct mem_cgroup {
>>>> struct list_head event_list;
>>>> spinlock_t event_list_lock;
>>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>> + struct list_head split_queue;
>>>> + unsigned long split_queue_len;
>>>> + spinlock_t split_queue_lock;
>>>> +#endif
>>>> +
>>>> struct mem_cgroup_per_node *nodeinfo[0];
>>>> /* WARNING: nodeinfo must be the last member here */
>>>> };
>>>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>>>> index 8ec38b1..405f5e6 100644
>>>> --- a/include/linux/mm_types.h
>>>> +++ b/include/linux/mm_types.h
>>>> @@ -139,7 +139,12 @@ struct page {
>>>> struct { /* Second tail page of compound page */
>>>> unsigned long _compound_pad_1; /* compound_head */
>>>> unsigned long _compound_pad_2;
>>>> - struct list_head deferred_list;
>>>> + union {
>>>> + /* Global THP deferred split list */
>>>> + struct list_head deferred_list;
>>>> + /* Memcg THP deferred split list */
>>>> + struct list_head memcg_deferred_list;
>>> Why we need two namesakes for this list entry?
>>>
>>> For me it looks redundantly: it does not give additional information,
>>> but it leads to duplication (and we have two helpers page_deferred_list()
>>> and page_memcg_deferred_list() instead of one).
>> Yes, kind of. Actually I was also wondering if this is worth or not. My point is this may improve the code readability. We can figure out what split queue (per node or per memcg) is being manipulated just by the name of the list.
>>
>> If the most people thought this is unnecessary, I'm definitely ok to just keep one name.
>>
>>>> + };
>>>> };
>>>> struct { /* Page table pages */
>>>> unsigned long _pt_pad_1; /* compound_head */
>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>> index 9f8bce9..0b9cfe1 100644
>>>> --- a/mm/huge_memory.c
>>>> +++ b/mm/huge_memory.c
>>>> @@ -492,12 +492,6 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
>>>> return pmd;
>>>> }
>>>> -static inline struct list_head *page_deferred_list(struct page *page)
>>>> -{
>>>> - /* ->lru in the tail pages is occupied by compound_head. */
>>>> - return &page[2].deferred_list;
>>>> -}
>>>> -
>>>> void prep_transhuge_page(struct page *page)
>>>> {
>>>> /*
>>>> @@ -505,7 +499,10 @@ void prep_transhuge_page(struct page *page)
>>>> * as list_head: assuming THP order >= 2
>>>> */
>>>> - INIT_LIST_HEAD(page_deferred_list(page));
>>>> + if (mem_cgroup_disabled())
>>>> + INIT_LIST_HEAD(page_deferred_list(page));
>>>> + else
>>>> + INIT_LIST_HEAD(page_memcg_deferred_list(page));
>>>> set_compound_page_dtor(page, TRANSHUGE_PAGE_DTOR);
>>>> }
>>>> @@ -2664,6 +2661,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>> bool mlocked;
>>>> unsigned long flags;
>>>> pgoff_t end;
>>>> + struct mem_cgroup *memcg = head->mem_cgroup;
>>>> VM_BUG_ON_PAGE(is_huge_zero_page(page), page);
>>>> VM_BUG_ON_PAGE(!PageLocked(page), page);
>>>> @@ -2744,17 +2742,30 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>> }
>>>> /* Prevent deferred_split_scan() touching ->_refcount */
>>>> - spin_lock(&pgdata->split_queue_lock);
>>>> + if (!memcg)
>>>> + spin_lock(&pgdata->split_queue_lock);
>>>> + else
>>>> + spin_lock(&memcg->split_queue_lock);
>>>> count = page_count(head);
>>>> mapcount = total_mapcount(head);
>>>> if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) {
>>>> - if (!list_empty(page_deferred_list(head))) {
>>>> - pgdata->split_queue_len--;
>>>> - list_del(page_deferred_list(head));
>>>> + if (!memcg) {
>>>> + if (!list_empty(page_deferred_list(head))) {
>>>> + pgdata->split_queue_len--;
>>>> + list_del(page_deferred_list(head));
>>>> + }
>>>> + } else {
>>>> + if (!list_empty(page_memcg_deferred_list(head))) {
>>>> + memcg->split_queue_len--;
>>>> + list_del(page_memcg_deferred_list(head));
>>>> + }
>>>> }
>>>> if (mapping)
>>>> __dec_node_page_state(page, NR_SHMEM_THPS);
>>>> - spin_unlock(&pgdata->split_queue_lock);
>>>> + if (!memcg)
>>>> + spin_unlock(&pgdata->split_queue_lock);
>>>> + else
>>>> + spin_unlock(&memcg->split_queue_lock);
>>>> __split_huge_page(page, list, end, flags);
>>>> if (PageSwapCache(head)) {
>>>> swp_entry_t entry = { .val = page_private(head) };
>>>> @@ -2771,7 +2782,10 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>> dump_page(page, "total_mapcount(head) > 0");
>>>> BUG();
>>>> }
>>>> - spin_unlock(&pgdata->split_queue_lock);
>>>> + if (!memcg)
>>>> + spin_unlock(&pgdata->split_queue_lock);
>>>> + else
>>>> + spin_unlock(&memcg->split_queue_lock);
>>>> fail: if (mapping)
>>>> xa_unlock(&mapping->i_pages);
>>>> spin_unlock_irqrestore(&pgdata->lru_lock, flags);
>>>> @@ -2791,17 +2805,40 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>> return ret;
>>>> }
>>>> -void free_transhuge_page(struct page *page)
>>>> +void del_thp_from_deferred_split_queue(struct page *page)
>>>> {
>>>> struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
>>>> unsigned long flags;
>>>> + struct mem_cgroup *memcg = compound_head(page)->mem_cgroup;
>>>> - spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>>>> - if (!list_empty(page_deferred_list(page))) {
>>>> - pgdata->split_queue_len--;
>>>> - list_del(page_deferred_list(page));
>>>> + /*
>>>> + * The THP may be not on LRU at this point, e.g. the old page of
>>>> + * NUMA migration. And PageTransHuge is not enough to distinguish
>>>> + * with other compound page, e.g. skb, THP destructor is not used
>>>> + * anymore and will be removed, so the compound order sounds like
>>>> + * the only choice here.
>>>> + */
>>>> + if (PageTransHuge(page) && compound_order(page) == HPAGE_PMD_ORDER) {
>>>> + if (!memcg) {
>>>> + spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>>>> + if (!list_empty(page_deferred_list(page))) {
>>>> + pgdata->split_queue_len--;
>>>> + list_del(page_deferred_list(page));
>>>> + }
>>>> + spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
>>>> + } else {
>>>> + spin_lock_irqsave(&memcg->split_queue_lock, flags);
>>>> + if (!list_empty(page_memcg_deferred_list(page))) {
>>>> + memcg->split_queue_len--;
>>>> + list_del(page_memcg_deferred_list(page));
>>>> + }
>>>> + spin_unlock_irqrestore(&memcg->split_queue_lock, flags);
>>> Such the patterns look like a duplication of functionality, we already have
>>> in list_lru: it handles both root_mem_cgroup and all children memcg.
>> Would you please point me to some example code?
> I mean that we do almost the same in list_lru_add(): check for whether
> item is already added, find the desired list, maintain the list's len.
>
> It looks all the above we may replace with something like
>
> list_lru_add(defered_thp_lru, page_deferred_list(page))
>
> after necessary preparations (some rewriting of the rest of code is needed).
Aha, I got your point. I'm not quite familiar with that code. I took a
quick loot at it, it looks the current APIs are not good enough for
deferred split, which needs irqsave/irqrestore version list
add/del/move/walk and page refcount bumped version walk.
We could add those APIs, but deferred split may be the only user. So,
I'm not sure about whether it is worth the complexity or not.
>
>>> Should we try to reuse that code, and to switch huge pages shrinker
>>> into generic code?
>> Definitely if it is feasible.
>>
>>> (Yeah, currently we allocate memcg_cache_ida IDS only for kmem, but we may
>>> consider to allocate them for any cases, since now we have new memcg shrinkers
>>> like you introduce).
>> The patch 3/3 removes the restriction.
> It does that for shrinker code. but there is the code in mm/memcontrol.c,
> which maintains list_lru slots for all memcgs. In case of switching to
> list_lru base, we will need to teach the code to maintain the slots
> for !CONFIG_MEMCG_KMEM case too.
Yes, the extra complexity comes in. And, we may need distinguish
deferred split queue with other lrus since other lrus may be reparented
(they are slab), but deferred split should be not.
>
> Kirill
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 0/3] Make deferred split shrinker memcg aware
2019-05-29 2:34 ` Yang Shi
@ 2019-05-29 21:07 ` David Rientjes
2019-05-30 3:22 ` Yang Shi
0 siblings, 1 reply; 20+ messages in thread
From: David Rientjes @ 2019-05-29 21:07 UTC (permalink / raw)
To: Yang Shi
Cc: ktkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb,
Andrew Morton, linux-mm, linux-kernel
On Wed, 29 May 2019, Yang Shi wrote:
> > Right, we've also encountered this. I talked to Kirill about it a week or
> > so ago where the suggestion was to split all compound pages on the
> > deferred split queues under the presence of even memory pressure.
> >
> > That breaks cgroup isolation and perhaps unfairly penalizes workloads that
> > are running attached to other memcg hierarchies that are not under
> > pressure because their compound pages are now split as a side effect.
> > There is a benefit to keeping these compound pages around while not under
> > memory pressure if all pages are subsequently mapped again.
>
> Yes, I do agree. I tried other approaches too, it sounds making deferred split
> queue per memcg is the optimal one.
>
The approach we went with were to track the actual counts of compound
pages on the deferred split queue for each pgdat for each memcg and then
invoke the shrinker for memcg reclaim and iterate those not charged to the
hierarchy under reclaim. That's suboptimal and was a stop gap measure
under time pressure: it's refreshing to see the optimal method being
pursued, thanks!
> > I'm curious if your internal applications team is also asking for
> > statistics on how much memory can be freed if the deferred split queues
> > can be shrunk? We have applications that monitor their own memory usage
>
> No, but this reminds me. The THPs on deferred split queue should be accounted
> into available memory too.
>
Right, and we have also seen this for users of MADV_FREE that have both an
increased rss and memcg usage that don't realize that the memory is freed
under pressure. I'm thinking that we need some kind of MemAvailable for
memcg hierarchies to be the authoritative source of what can be reclaimed
under pressure.
> > through memcg stats or usage and proactively try to reduce that usage when
> > it is growing too large. The deferred split queues have significantly
> > increased both memcg usage and rss when they've upgraded kernels.
> >
> > How are your applications monitoring how much memory from deferred split
> > queues can be freed on memory pressure? Any thoughts on providing it as a
> > memcg stat?
>
> I don't think they have such monitor. I saw rss_huge is abormal in memcg stat
> even after the application is killed by oom, so I realized the deferred split
> queue may play a role here.
>
Exactly the same in my case :) We were likely looking at the exact same
issue at the same time.
> The memcg stat doesn't have counters for available memory as global vmstat. It
> may be better to have such statistics, or extending reclaimable "slab" to
> shrinkable/reclaimable "memory".
>
Have you considered following how NR_ANON_MAPPED is tracked for each pgdat
and using that as an indicator of when the modify a memcg stat to track
the amount of memory on a compound page? I think this would be necessary
for userspace to know what their true memory usage is.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 0/3] Make deferred split shrinker memcg aware
2019-05-29 21:07 ` David Rientjes
@ 2019-05-30 3:22 ` Yang Shi
0 siblings, 0 replies; 20+ messages in thread
From: Yang Shi @ 2019-05-30 3:22 UTC (permalink / raw)
To: David Rientjes
Cc: ktkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb,
Andrew Morton, linux-mm, linux-kernel
On 5/30/19 5:07 AM, David Rientjes wrote:
> On Wed, 29 May 2019, Yang Shi wrote:
>
>>> Right, we've also encountered this. I talked to Kirill about it a week or
>>> so ago where the suggestion was to split all compound pages on the
>>> deferred split queues under the presence of even memory pressure.
>>>
>>> That breaks cgroup isolation and perhaps unfairly penalizes workloads that
>>> are running attached to other memcg hierarchies that are not under
>>> pressure because their compound pages are now split as a side effect.
>>> There is a benefit to keeping these compound pages around while not under
>>> memory pressure if all pages are subsequently mapped again.
>> Yes, I do agree. I tried other approaches too, it sounds making deferred split
>> queue per memcg is the optimal one.
>>
> The approach we went with were to track the actual counts of compound
> pages on the deferred split queue for each pgdat for each memcg and then
> invoke the shrinker for memcg reclaim and iterate those not charged to the
> hierarchy under reclaim. That's suboptimal and was a stop gap measure
> under time pressure: it's refreshing to see the optimal method being
> pursued, thanks!
We did the exactly same thing for a temporary hotfix.
>
>>> I'm curious if your internal applications team is also asking for
>>> statistics on how much memory can be freed if the deferred split queues
>>> can be shrunk? We have applications that monitor their own memory usage
>> No, but this reminds me. The THPs on deferred split queue should be accounted
>> into available memory too.
>>
> Right, and we have also seen this for users of MADV_FREE that have both an
> increased rss and memcg usage that don't realize that the memory is freed
> under pressure. I'm thinking that we need some kind of MemAvailable for
> memcg hierarchies to be the authoritative source of what can be reclaimed
> under pressure.
It sounds useful. We also need know the available memory in memcg scope
in our containers.
>
>>> through memcg stats or usage and proactively try to reduce that usage when
>>> it is growing too large. The deferred split queues have significantly
>>> increased both memcg usage and rss when they've upgraded kernels.
>>>
>>> How are your applications monitoring how much memory from deferred split
>>> queues can be freed on memory pressure? Any thoughts on providing it as a
>>> memcg stat?
>> I don't think they have such monitor. I saw rss_huge is abormal in memcg stat
>> even after the application is killed by oom, so I realized the deferred split
>> queue may play a role here.
>>
> Exactly the same in my case :) We were likely looking at the exact same
> issue at the same time.
Yes, it seems so. :-)
>> The memcg stat doesn't have counters for available memory as global vmstat. It
>> may be better to have such statistics, or extending reclaimable "slab" to
>> shrinkable/reclaimable "memory".
>>
> Have you considered following how NR_ANON_MAPPED is tracked for each pgdat
> and using that as an indicator of when the modify a memcg stat to track
> the amount of memory on a compound page? I think this would be necessary
> for userspace to know what their true memory usage is.
No, I haven't. Do you mean minus MADV_FREE and deferred split THP from
NR_ANON_MAPPED? It looks they have been decreased from NR_ANON_MAPPED
when removing rmap.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware
2019-05-28 12:44 ` [PATCH 1/3] mm: thp: make " Yang Shi
2019-05-28 14:42 ` Kirill Tkhai
@ 2019-05-30 12:07 ` Kirill A. Shutemov
2019-05-30 13:29 ` Yang Shi
1 sibling, 1 reply; 20+ messages in thread
From: Kirill A. Shutemov @ 2019-05-30 12:07 UTC (permalink / raw)
To: Yang Shi
Cc: ktkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm,
linux-mm, linux-kernel
On Tue, May 28, 2019 at 08:44:22PM +0800, Yang Shi wrote:
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index bc74d6a..9ff5fab 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -316,6 +316,12 @@ struct mem_cgroup {
> struct list_head event_list;
> spinlock_t event_list_lock;
>
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> + struct list_head split_queue;
> + unsigned long split_queue_len;
> + spinlock_t split_queue_lock;
Maybe we should wrap there into a struct and have helper that would return
pointer to the struct which is right for the page: from pgdat or from
memcg, depending on the situation?
This way we will be able to kill most of code duplication, right?
--
Kirill A. Shutemov
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] mm: shrinker: make shrinker not depend on memcg kmem
2019-05-28 12:44 ` [PATCH 3/3] mm: shrinker: make shrinker not depend on memcg kmem Yang Shi
@ 2019-05-30 12:08 ` Kirill A. Shutemov
2019-05-30 13:20 ` Yang Shi
0 siblings, 1 reply; 20+ messages in thread
From: Kirill A. Shutemov @ 2019-05-30 12:08 UTC (permalink / raw)
To: Yang Shi
Cc: ktkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm,
linux-mm, linux-kernel
On Tue, May 28, 2019 at 08:44:24PM +0800, Yang Shi wrote:
> @@ -81,6 +79,7 @@ struct shrinker {
> /* Flags */
> #define SHRINKER_NUMA_AWARE (1 << 0)
> #define SHRINKER_MEMCG_AWARE (1 << 1)
> +#define SHRINKER_NONSLAB (1 << 3)
Why 3?
--
Kirill A. Shutemov
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] mm: shrinker: make shrinker not depend on memcg kmem
2019-05-30 12:08 ` Kirill A. Shutemov
@ 2019-05-30 13:20 ` Yang Shi
0 siblings, 0 replies; 20+ messages in thread
From: Yang Shi @ 2019-05-30 13:20 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: ktkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm,
linux-mm, linux-kernel
On 5/30/19 8:08 PM, Kirill A. Shutemov wrote:
> On Tue, May 28, 2019 at 08:44:24PM +0800, Yang Shi wrote:
>> @@ -81,6 +79,7 @@ struct shrinker {
>> /* Flags */
>> #define SHRINKER_NUMA_AWARE (1 << 0)
>> #define SHRINKER_MEMCG_AWARE (1 << 1)
>> +#define SHRINKER_NONSLAB (1 << 3)
> Why 3?
My fault, it is a typo. Should be 2.
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware
2019-05-30 12:07 ` Kirill A. Shutemov
@ 2019-05-30 13:29 ` Yang Shi
0 siblings, 0 replies; 20+ messages in thread
From: Yang Shi @ 2019-05-30 13:29 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: ktkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm,
linux-mm, linux-kernel
On 5/30/19 8:07 PM, Kirill A. Shutemov wrote:
> On Tue, May 28, 2019 at 08:44:22PM +0800, Yang Shi wrote:
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index bc74d6a..9ff5fab 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -316,6 +316,12 @@ struct mem_cgroup {
>> struct list_head event_list;
>> spinlock_t event_list_lock;
>>
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> + struct list_head split_queue;
>> + unsigned long split_queue_len;
>> + spinlock_t split_queue_lock;
> Maybe we should wrap there into a struct and have helper that would return
> pointer to the struct which is right for the page: from pgdat or from
> memcg, depending on the situation?
>
> This way we will be able to kill most of code duplication, right?
Yes, it sounds simpler than using list_lru.
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware
2019-05-29 11:25 ` Yang Shi
@ 2019-06-10 8:23 ` Kirill Tkhai
2019-06-10 17:25 ` Yang Shi
0 siblings, 1 reply; 20+ messages in thread
From: Kirill Tkhai @ 2019-06-10 8:23 UTC (permalink / raw)
To: Yang Shi, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm
Cc: linux-mm, linux-kernel
On 29.05.2019 14:25, Yang Shi wrote:
>
>
> On 5/29/19 4:14 PM, Kirill Tkhai wrote:
>> On 29.05.2019 05:43, Yang Shi wrote:
>>>
>>> On 5/28/19 10:42 PM, Kirill Tkhai wrote:
>>>> Hi, Yang,
>>>>
>>>> On 28.05.2019 15:44, Yang Shi wrote:
>>>>> Currently THP deferred split shrinker is not memcg aware, this may cause
>>>>> premature OOM with some configuration. For example the below test would
>>>>> run into premature OOM easily:
>>>>>
>>>>> $ cgcreate -g memory:thp
>>>>> $ echo 4G > /sys/fs/cgroup/memory/thp/memory/limit_in_bytes
>>>>> $ cgexec -g memory:thp transhuge-stress 4000
>>>>>
>>>>> transhuge-stress comes from kernel selftest.
>>>>>
>>>>> It is easy to hit OOM, but there are still a lot THP on the deferred
>>>>> split queue, memcg direct reclaim can't touch them since the deferred
>>>>> split shrinker is not memcg aware.
>>>>>
>>>>> Convert deferred split shrinker memcg aware by introducing per memcg
>>>>> deferred split queue. The THP should be on either per node or per memcg
>>>>> deferred split queue if it belongs to a memcg. When the page is
>>>>> immigrated to the other memcg, it will be immigrated to the target
>>>>> memcg's deferred split queue too.
>>>>>
>>>>> And, move deleting THP from deferred split queue in page free before
>>>>> memcg uncharge so that the page's memcg information is available.
>>>>>
>>>>> Reuse the second tail page's deferred_list for per memcg list since the
>>>>> same THP can't be on multiple deferred split queues.
>>>>>
>>>>> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>>>>> Cc: Michal Hocko <mhocko@suse.com>
>>>>> Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
>>>>> Cc: Hugh Dickins <hughd@google.com>
>>>>> Cc: Shakeel Butt <shakeelb@google.com>
>>>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>>>>> ---
>>>>> include/linux/huge_mm.h | 24 ++++++
>>>>> include/linux/memcontrol.h | 6 ++
>>>>> include/linux/mm_types.h | 7 +-
>>>>> mm/huge_memory.c | 182 +++++++++++++++++++++++++++++++++------------
>>>>> mm/memcontrol.c | 20 +++++
>>>>> mm/swap.c | 4 +
>>>>> 6 files changed, 194 insertions(+), 49 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>>> index 7cd5c15..f6d1cde 100644
>>>>> --- a/include/linux/huge_mm.h
>>>>> +++ b/include/linux/huge_mm.h
>>>>> @@ -250,6 +250,26 @@ static inline bool thp_migration_supported(void)
>>>>> return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
>>>>> }
>>>>> +static inline struct list_head *page_deferred_list(struct page *page)
>>>>> +{
>>>>> + /*
>>>>> + * Global deferred list in the second tail pages is occupied by
>>>>> + * compound_head.
>>>>> + */
>>>>> + return &page[2].deferred_list;
>>>>> +}
>>>>> +
>>>>> +static inline struct list_head *page_memcg_deferred_list(struct page *page)
>>>>> +{
>>>>> + /*
>>>>> + * Memcg deferred list in the second tail pages is occupied by
>>>>> + * compound_head.
>>>>> + */
>>>>> + return &page[2].memcg_deferred_list;
>>>>> +}
>>>>> +
>>>>> +extern void del_thp_from_deferred_split_queue(struct page *);
>>>>> +
>>>>> #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>>> #define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })
>>>>> #define HPAGE_PMD_MASK ({ BUILD_BUG(); 0; })
>>>>> @@ -368,6 +388,10 @@ static inline bool thp_migration_supported(void)
>>>>> {
>>>>> return false;
>>>>> }
>>>>> +
>>>>> +static inline void del_thp_from_deferred_split_queue(struct page *page)
>>>>> +{
>>>>> +}
>>>>> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>>> #endif /* _LINUX_HUGE_MM_H */
>>>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>>>> index bc74d6a..9ff5fab 100644
>>>>> --- a/include/linux/memcontrol.h
>>>>> +++ b/include/linux/memcontrol.h
>>>>> @@ -316,6 +316,12 @@ struct mem_cgroup {
>>>>> struct list_head event_list;
>>>>> spinlock_t event_list_lock;
>>>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>>> + struct list_head split_queue;
>>>>> + unsigned long split_queue_len;
>>>>> + spinlock_t split_queue_lock;
>>>>> +#endif
>>>>> +
>>>>> struct mem_cgroup_per_node *nodeinfo[0];
>>>>> /* WARNING: nodeinfo must be the last member here */
>>>>> };
>>>>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>>>>> index 8ec38b1..405f5e6 100644
>>>>> --- a/include/linux/mm_types.h
>>>>> +++ b/include/linux/mm_types.h
>>>>> @@ -139,7 +139,12 @@ struct page {
>>>>> struct { /* Second tail page of compound page */
>>>>> unsigned long _compound_pad_1; /* compound_head */
>>>>> unsigned long _compound_pad_2;
>>>>> - struct list_head deferred_list;
>>>>> + union {
>>>>> + /* Global THP deferred split list */
>>>>> + struct list_head deferred_list;
>>>>> + /* Memcg THP deferred split list */
>>>>> + struct list_head memcg_deferred_list;
>>>> Why we need two namesakes for this list entry?
>>>>
>>>> For me it looks redundantly: it does not give additional information,
>>>> but it leads to duplication (and we have two helpers page_deferred_list()
>>>> and page_memcg_deferred_list() instead of one).
>>> Yes, kind of. Actually I was also wondering if this is worth or not. My point is this may improve the code readability. We can figure out what split queue (per node or per memcg) is being manipulated just by the name of the list.
>>>
>>> If the most people thought this is unnecessary, I'm definitely ok to just keep one name.
>>>
>>>>> + };
>>>>> };
>>>>> struct { /* Page table pages */
>>>>> unsigned long _pt_pad_1; /* compound_head */
>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>> index 9f8bce9..0b9cfe1 100644
>>>>> --- a/mm/huge_memory.c
>>>>> +++ b/mm/huge_memory.c
>>>>> @@ -492,12 +492,6 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
>>>>> return pmd;
>>>>> }
>>>>> -static inline struct list_head *page_deferred_list(struct page *page)
>>>>> -{
>>>>> - /* ->lru in the tail pages is occupied by compound_head. */
>>>>> - return &page[2].deferred_list;
>>>>> -}
>>>>> -
>>>>> void prep_transhuge_page(struct page *page)
>>>>> {
>>>>> /*
>>>>> @@ -505,7 +499,10 @@ void prep_transhuge_page(struct page *page)
>>>>> * as list_head: assuming THP order >= 2
>>>>> */
>>>>> - INIT_LIST_HEAD(page_deferred_list(page));
>>>>> + if (mem_cgroup_disabled())
>>>>> + INIT_LIST_HEAD(page_deferred_list(page));
>>>>> + else
>>>>> + INIT_LIST_HEAD(page_memcg_deferred_list(page));
>>>>> set_compound_page_dtor(page, TRANSHUGE_PAGE_DTOR);
>>>>> }
>>>>> @@ -2664,6 +2661,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>> bool mlocked;
>>>>> unsigned long flags;
>>>>> pgoff_t end;
>>>>> + struct mem_cgroup *memcg = head->mem_cgroup;
>>>>> VM_BUG_ON_PAGE(is_huge_zero_page(page), page);
>>>>> VM_BUG_ON_PAGE(!PageLocked(page), page);
>>>>> @@ -2744,17 +2742,30 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>> }
>>>>> /* Prevent deferred_split_scan() touching ->_refcount */
>>>>> - spin_lock(&pgdata->split_queue_lock);
>>>>> + if (!memcg)
>>>>> + spin_lock(&pgdata->split_queue_lock);
>>>>> + else
>>>>> + spin_lock(&memcg->split_queue_lock);
>>>>> count = page_count(head);
>>>>> mapcount = total_mapcount(head);
>>>>> if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) {
>>>>> - if (!list_empty(page_deferred_list(head))) {
>>>>> - pgdata->split_queue_len--;
>>>>> - list_del(page_deferred_list(head));
>>>>> + if (!memcg) {
>>>>> + if (!list_empty(page_deferred_list(head))) {
>>>>> + pgdata->split_queue_len--;
>>>>> + list_del(page_deferred_list(head));
>>>>> + }
>>>>> + } else {
>>>>> + if (!list_empty(page_memcg_deferred_list(head))) {
>>>>> + memcg->split_queue_len--;
>>>>> + list_del(page_memcg_deferred_list(head));
>>>>> + }
>>>>> }
>>>>> if (mapping)
>>>>> __dec_node_page_state(page, NR_SHMEM_THPS);
>>>>> - spin_unlock(&pgdata->split_queue_lock);
>>>>> + if (!memcg)
>>>>> + spin_unlock(&pgdata->split_queue_lock);
>>>>> + else
>>>>> + spin_unlock(&memcg->split_queue_lock);
>>>>> __split_huge_page(page, list, end, flags);
>>>>> if (PageSwapCache(head)) {
>>>>> swp_entry_t entry = { .val = page_private(head) };
>>>>> @@ -2771,7 +2782,10 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>> dump_page(page, "total_mapcount(head) > 0");
>>>>> BUG();
>>>>> }
>>>>> - spin_unlock(&pgdata->split_queue_lock);
>>>>> + if (!memcg)
>>>>> + spin_unlock(&pgdata->split_queue_lock);
>>>>> + else
>>>>> + spin_unlock(&memcg->split_queue_lock);
>>>>> fail: if (mapping)
>>>>> xa_unlock(&mapping->i_pages);
>>>>> spin_unlock_irqrestore(&pgdata->lru_lock, flags);
>>>>> @@ -2791,17 +2805,40 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>> return ret;
>>>>> }
>>>>> -void free_transhuge_page(struct page *page)
>>>>> +void del_thp_from_deferred_split_queue(struct page *page)
>>>>> {
>>>>> struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
>>>>> unsigned long flags;
>>>>> + struct mem_cgroup *memcg = compound_head(page)->mem_cgroup;
>>>>> - spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>>>>> - if (!list_empty(page_deferred_list(page))) {
>>>>> - pgdata->split_queue_len--;
>>>>> - list_del(page_deferred_list(page));
>>>>> + /*
>>>>> + * The THP may be not on LRU at this point, e.g. the old page of
>>>>> + * NUMA migration. And PageTransHuge is not enough to distinguish
>>>>> + * with other compound page, e.g. skb, THP destructor is not used
>>>>> + * anymore and will be removed, so the compound order sounds like
>>>>> + * the only choice here.
>>>>> + */
>>>>> + if (PageTransHuge(page) && compound_order(page) == HPAGE_PMD_ORDER) {
>>>>> + if (!memcg) {
>>>>> + spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>>>>> + if (!list_empty(page_deferred_list(page))) {
>>>>> + pgdata->split_queue_len--;
>>>>> + list_del(page_deferred_list(page));
>>>>> + }
>>>>> + spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
>>>>> + } else {
>>>>> + spin_lock_irqsave(&memcg->split_queue_lock, flags);
>>>>> + if (!list_empty(page_memcg_deferred_list(page))) {
>>>>> + memcg->split_queue_len--;
>>>>> + list_del(page_memcg_deferred_list(page));
>>>>> + }
>>>>> + spin_unlock_irqrestore(&memcg->split_queue_lock, flags);
>>>> Such the patterns look like a duplication of functionality, we already have
>>>> in list_lru: it handles both root_mem_cgroup and all children memcg.
>>> Would you please point me to some example code?
>> I mean that we do almost the same in list_lru_add(): check for whether
>> item is already added, find the desired list, maintain the list's len.
>>
>> It looks all the above we may replace with something like
>>
>> list_lru_add(defered_thp_lru, page_deferred_list(page))
>>
>> after necessary preparations (some rewriting of the rest of code is needed).
>
> Aha, I got your point. I'm not quite familiar with that code. I took a quick loot at it, it looks the current APIs are not good enough for deferred split, which needs irqsave/irqrestore version list add/del/move/walk and page refcount bumped version walk.
I missed the point about refcount bumping, could you please clarify?
Thanks,
Kirill
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware
2019-06-10 8:23 ` Kirill Tkhai
@ 2019-06-10 17:25 ` Yang Shi
2019-06-13 8:19 ` Kirill Tkhai
0 siblings, 1 reply; 20+ messages in thread
From: Yang Shi @ 2019-06-10 17:25 UTC (permalink / raw)
To: Kirill Tkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm
Cc: linux-mm, linux-kernel
On 6/10/19 1:23 AM, Kirill Tkhai wrote:
> On 29.05.2019 14:25, Yang Shi wrote:
>>
>> On 5/29/19 4:14 PM, Kirill Tkhai wrote:
>>> On 29.05.2019 05:43, Yang Shi wrote:
>>>> On 5/28/19 10:42 PM, Kirill Tkhai wrote:
>>>>> Hi, Yang,
>>>>>
>>>>> On 28.05.2019 15:44, Yang Shi wrote:
>>>>>> Currently THP deferred split shrinker is not memcg aware, this may cause
>>>>>> premature OOM with some configuration. For example the below test would
>>>>>> run into premature OOM easily:
>>>>>>
>>>>>> $ cgcreate -g memory:thp
>>>>>> $ echo 4G > /sys/fs/cgroup/memory/thp/memory/limit_in_bytes
>>>>>> $ cgexec -g memory:thp transhuge-stress 4000
>>>>>>
>>>>>> transhuge-stress comes from kernel selftest.
>>>>>>
>>>>>> It is easy to hit OOM, but there are still a lot THP on the deferred
>>>>>> split queue, memcg direct reclaim can't touch them since the deferred
>>>>>> split shrinker is not memcg aware.
>>>>>>
>>>>>> Convert deferred split shrinker memcg aware by introducing per memcg
>>>>>> deferred split queue. The THP should be on either per node or per memcg
>>>>>> deferred split queue if it belongs to a memcg. When the page is
>>>>>> immigrated to the other memcg, it will be immigrated to the target
>>>>>> memcg's deferred split queue too.
>>>>>>
>>>>>> And, move deleting THP from deferred split queue in page free before
>>>>>> memcg uncharge so that the page's memcg information is available.
>>>>>>
>>>>>> Reuse the second tail page's deferred_list for per memcg list since the
>>>>>> same THP can't be on multiple deferred split queues.
>>>>>>
>>>>>> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>>>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>>>>>> Cc: Michal Hocko <mhocko@suse.com>
>>>>>> Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
>>>>>> Cc: Hugh Dickins <hughd@google.com>
>>>>>> Cc: Shakeel Butt <shakeelb@google.com>
>>>>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>>>>>> ---
>>>>>> include/linux/huge_mm.h | 24 ++++++
>>>>>> include/linux/memcontrol.h | 6 ++
>>>>>> include/linux/mm_types.h | 7 +-
>>>>>> mm/huge_memory.c | 182 +++++++++++++++++++++++++++++++++------------
>>>>>> mm/memcontrol.c | 20 +++++
>>>>>> mm/swap.c | 4 +
>>>>>> 6 files changed, 194 insertions(+), 49 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>>>> index 7cd5c15..f6d1cde 100644
>>>>>> --- a/include/linux/huge_mm.h
>>>>>> +++ b/include/linux/huge_mm.h
>>>>>> @@ -250,6 +250,26 @@ static inline bool thp_migration_supported(void)
>>>>>> return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
>>>>>> }
>>>>>> +static inline struct list_head *page_deferred_list(struct page *page)
>>>>>> +{
>>>>>> + /*
>>>>>> + * Global deferred list in the second tail pages is occupied by
>>>>>> + * compound_head.
>>>>>> + */
>>>>>> + return &page[2].deferred_list;
>>>>>> +}
>>>>>> +
>>>>>> +static inline struct list_head *page_memcg_deferred_list(struct page *page)
>>>>>> +{
>>>>>> + /*
>>>>>> + * Memcg deferred list in the second tail pages is occupied by
>>>>>> + * compound_head.
>>>>>> + */
>>>>>> + return &page[2].memcg_deferred_list;
>>>>>> +}
>>>>>> +
>>>>>> +extern void del_thp_from_deferred_split_queue(struct page *);
>>>>>> +
>>>>>> #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>>>> #define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })
>>>>>> #define HPAGE_PMD_MASK ({ BUILD_BUG(); 0; })
>>>>>> @@ -368,6 +388,10 @@ static inline bool thp_migration_supported(void)
>>>>>> {
>>>>>> return false;
>>>>>> }
>>>>>> +
>>>>>> +static inline void del_thp_from_deferred_split_queue(struct page *page)
>>>>>> +{
>>>>>> +}
>>>>>> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>>>> #endif /* _LINUX_HUGE_MM_H */
>>>>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>>>>> index bc74d6a..9ff5fab 100644
>>>>>> --- a/include/linux/memcontrol.h
>>>>>> +++ b/include/linux/memcontrol.h
>>>>>> @@ -316,6 +316,12 @@ struct mem_cgroup {
>>>>>> struct list_head event_list;
>>>>>> spinlock_t event_list_lock;
>>>>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>>>> + struct list_head split_queue;
>>>>>> + unsigned long split_queue_len;
>>>>>> + spinlock_t split_queue_lock;
>>>>>> +#endif
>>>>>> +
>>>>>> struct mem_cgroup_per_node *nodeinfo[0];
>>>>>> /* WARNING: nodeinfo must be the last member here */
>>>>>> };
>>>>>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>>>>>> index 8ec38b1..405f5e6 100644
>>>>>> --- a/include/linux/mm_types.h
>>>>>> +++ b/include/linux/mm_types.h
>>>>>> @@ -139,7 +139,12 @@ struct page {
>>>>>> struct { /* Second tail page of compound page */
>>>>>> unsigned long _compound_pad_1; /* compound_head */
>>>>>> unsigned long _compound_pad_2;
>>>>>> - struct list_head deferred_list;
>>>>>> + union {
>>>>>> + /* Global THP deferred split list */
>>>>>> + struct list_head deferred_list;
>>>>>> + /* Memcg THP deferred split list */
>>>>>> + struct list_head memcg_deferred_list;
>>>>> Why we need two namesakes for this list entry?
>>>>>
>>>>> For me it looks redundantly: it does not give additional information,
>>>>> but it leads to duplication (and we have two helpers page_deferred_list()
>>>>> and page_memcg_deferred_list() instead of one).
>>>> Yes, kind of. Actually I was also wondering if this is worth or not. My point is this may improve the code readability. We can figure out what split queue (per node or per memcg) is being manipulated just by the name of the list.
>>>>
>>>> If the most people thought this is unnecessary, I'm definitely ok to just keep one name.
>>>>
>>>>>> + };
>>>>>> };
>>>>>> struct { /* Page table pages */
>>>>>> unsigned long _pt_pad_1; /* compound_head */
>>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>>> index 9f8bce9..0b9cfe1 100644
>>>>>> --- a/mm/huge_memory.c
>>>>>> +++ b/mm/huge_memory.c
>>>>>> @@ -492,12 +492,6 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
>>>>>> return pmd;
>>>>>> }
>>>>>> -static inline struct list_head *page_deferred_list(struct page *page)
>>>>>> -{
>>>>>> - /* ->lru in the tail pages is occupied by compound_head. */
>>>>>> - return &page[2].deferred_list;
>>>>>> -}
>>>>>> -
>>>>>> void prep_transhuge_page(struct page *page)
>>>>>> {
>>>>>> /*
>>>>>> @@ -505,7 +499,10 @@ void prep_transhuge_page(struct page *page)
>>>>>> * as list_head: assuming THP order >= 2
>>>>>> */
>>>>>> - INIT_LIST_HEAD(page_deferred_list(page));
>>>>>> + if (mem_cgroup_disabled())
>>>>>> + INIT_LIST_HEAD(page_deferred_list(page));
>>>>>> + else
>>>>>> + INIT_LIST_HEAD(page_memcg_deferred_list(page));
>>>>>> set_compound_page_dtor(page, TRANSHUGE_PAGE_DTOR);
>>>>>> }
>>>>>> @@ -2664,6 +2661,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>>> bool mlocked;
>>>>>> unsigned long flags;
>>>>>> pgoff_t end;
>>>>>> + struct mem_cgroup *memcg = head->mem_cgroup;
>>>>>> VM_BUG_ON_PAGE(is_huge_zero_page(page), page);
>>>>>> VM_BUG_ON_PAGE(!PageLocked(page), page);
>>>>>> @@ -2744,17 +2742,30 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>>> }
>>>>>> /* Prevent deferred_split_scan() touching ->_refcount */
>>>>>> - spin_lock(&pgdata->split_queue_lock);
>>>>>> + if (!memcg)
>>>>>> + spin_lock(&pgdata->split_queue_lock);
>>>>>> + else
>>>>>> + spin_lock(&memcg->split_queue_lock);
>>>>>> count = page_count(head);
>>>>>> mapcount = total_mapcount(head);
>>>>>> if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) {
>>>>>> - if (!list_empty(page_deferred_list(head))) {
>>>>>> - pgdata->split_queue_len--;
>>>>>> - list_del(page_deferred_list(head));
>>>>>> + if (!memcg) {
>>>>>> + if (!list_empty(page_deferred_list(head))) {
>>>>>> + pgdata->split_queue_len--;
>>>>>> + list_del(page_deferred_list(head));
>>>>>> + }
>>>>>> + } else {
>>>>>> + if (!list_empty(page_memcg_deferred_list(head))) {
>>>>>> + memcg->split_queue_len--;
>>>>>> + list_del(page_memcg_deferred_list(head));
>>>>>> + }
>>>>>> }
>>>>>> if (mapping)
>>>>>> __dec_node_page_state(page, NR_SHMEM_THPS);
>>>>>> - spin_unlock(&pgdata->split_queue_lock);
>>>>>> + if (!memcg)
>>>>>> + spin_unlock(&pgdata->split_queue_lock);
>>>>>> + else
>>>>>> + spin_unlock(&memcg->split_queue_lock);
>>>>>> __split_huge_page(page, list, end, flags);
>>>>>> if (PageSwapCache(head)) {
>>>>>> swp_entry_t entry = { .val = page_private(head) };
>>>>>> @@ -2771,7 +2782,10 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>>> dump_page(page, "total_mapcount(head) > 0");
>>>>>> BUG();
>>>>>> }
>>>>>> - spin_unlock(&pgdata->split_queue_lock);
>>>>>> + if (!memcg)
>>>>>> + spin_unlock(&pgdata->split_queue_lock);
>>>>>> + else
>>>>>> + spin_unlock(&memcg->split_queue_lock);
>>>>>> fail: if (mapping)
>>>>>> xa_unlock(&mapping->i_pages);
>>>>>> spin_unlock_irqrestore(&pgdata->lru_lock, flags);
>>>>>> @@ -2791,17 +2805,40 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>>> return ret;
>>>>>> }
>>>>>> -void free_transhuge_page(struct page *page)
>>>>>> +void del_thp_from_deferred_split_queue(struct page *page)
>>>>>> {
>>>>>> struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
>>>>>> unsigned long flags;
>>>>>> + struct mem_cgroup *memcg = compound_head(page)->mem_cgroup;
>>>>>> - spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>>>>>> - if (!list_empty(page_deferred_list(page))) {
>>>>>> - pgdata->split_queue_len--;
>>>>>> - list_del(page_deferred_list(page));
>>>>>> + /*
>>>>>> + * The THP may be not on LRU at this point, e.g. the old page of
>>>>>> + * NUMA migration. And PageTransHuge is not enough to distinguish
>>>>>> + * with other compound page, e.g. skb, THP destructor is not used
>>>>>> + * anymore and will be removed, so the compound order sounds like
>>>>>> + * the only choice here.
>>>>>> + */
>>>>>> + if (PageTransHuge(page) && compound_order(page) == HPAGE_PMD_ORDER) {
>>>>>> + if (!memcg) {
>>>>>> + spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>>>>>> + if (!list_empty(page_deferred_list(page))) {
>>>>>> + pgdata->split_queue_len--;
>>>>>> + list_del(page_deferred_list(page));
>>>>>> + }
>>>>>> + spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
>>>>>> + } else {
>>>>>> + spin_lock_irqsave(&memcg->split_queue_lock, flags);
>>>>>> + if (!list_empty(page_memcg_deferred_list(page))) {
>>>>>> + memcg->split_queue_len--;
>>>>>> + list_del(page_memcg_deferred_list(page));
>>>>>> + }
>>>>>> + spin_unlock_irqrestore(&memcg->split_queue_lock, flags);
>>>>> Such the patterns look like a duplication of functionality, we already have
>>>>> in list_lru: it handles both root_mem_cgroup and all children memcg.
>>>> Would you please point me to some example code?
>>> I mean that we do almost the same in list_lru_add(): check for whether
>>> item is already added, find the desired list, maintain the list's len.
>>>
>>> It looks all the above we may replace with something like
>>>
>>> list_lru_add(defered_thp_lru, page_deferred_list(page))
>>>
>>> after necessary preparations (some rewriting of the rest of code is needed).
>> Aha, I got your point. I'm not quite familiar with that code. I took a quick loot at it, it looks the current APIs are not good enough for deferred split, which needs irqsave/irqrestore version list add/del/move/walk and page refcount bumped version walk.
> I missed the point about refcount bumping, could you please clarify?
The deferred_split_scan() need bump refcount for every head page when
scanning the deferred split queue.
>
> Thanks,
> Kirill
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware
2019-06-10 17:25 ` Yang Shi
@ 2019-06-13 8:19 ` Kirill Tkhai
2019-06-13 17:53 ` Yang Shi
0 siblings, 1 reply; 20+ messages in thread
From: Kirill Tkhai @ 2019-06-13 8:19 UTC (permalink / raw)
To: Yang Shi, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm
Cc: linux-mm, linux-kernel
On 10.06.2019 20:25, Yang Shi wrote:
>
>
> On 6/10/19 1:23 AM, Kirill Tkhai wrote:
>> On 29.05.2019 14:25, Yang Shi wrote:
>>>
>>> On 5/29/19 4:14 PM, Kirill Tkhai wrote:
>>>> On 29.05.2019 05:43, Yang Shi wrote:
>>>>> On 5/28/19 10:42 PM, Kirill Tkhai wrote:
>>>>>> Hi, Yang,
>>>>>>
>>>>>> On 28.05.2019 15:44, Yang Shi wrote:
>>>>>>> Currently THP deferred split shrinker is not memcg aware, this may cause
>>>>>>> premature OOM with some configuration. For example the below test would
>>>>>>> run into premature OOM easily:
>>>>>>>
>>>>>>> $ cgcreate -g memory:thp
>>>>>>> $ echo 4G > /sys/fs/cgroup/memory/thp/memory/limit_in_bytes
>>>>>>> $ cgexec -g memory:thp transhuge-stress 4000
>>>>>>>
>>>>>>> transhuge-stress comes from kernel selftest.
>>>>>>>
>>>>>>> It is easy to hit OOM, but there are still a lot THP on the deferred
>>>>>>> split queue, memcg direct reclaim can't touch them since the deferred
>>>>>>> split shrinker is not memcg aware.
>>>>>>>
>>>>>>> Convert deferred split shrinker memcg aware by introducing per memcg
>>>>>>> deferred split queue. The THP should be on either per node or per memcg
>>>>>>> deferred split queue if it belongs to a memcg. When the page is
>>>>>>> immigrated to the other memcg, it will be immigrated to the target
>>>>>>> memcg's deferred split queue too.
>>>>>>>
>>>>>>> And, move deleting THP from deferred split queue in page free before
>>>>>>> memcg uncharge so that the page's memcg information is available.
>>>>>>>
>>>>>>> Reuse the second tail page's deferred_list for per memcg list since the
>>>>>>> same THP can't be on multiple deferred split queues.
>>>>>>>
>>>>>>> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>>>>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>>>>>>> Cc: Michal Hocko <mhocko@suse.com>
>>>>>>> Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
>>>>>>> Cc: Hugh Dickins <hughd@google.com>
>>>>>>> Cc: Shakeel Butt <shakeelb@google.com>
>>>>>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>>>>>>> ---
>>>>>>> include/linux/huge_mm.h | 24 ++++++
>>>>>>> include/linux/memcontrol.h | 6 ++
>>>>>>> include/linux/mm_types.h | 7 +-
>>>>>>> mm/huge_memory.c | 182 +++++++++++++++++++++++++++++++++------------
>>>>>>> mm/memcontrol.c | 20 +++++
>>>>>>> mm/swap.c | 4 +
>>>>>>> 6 files changed, 194 insertions(+), 49 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>>>>> index 7cd5c15..f6d1cde 100644
>>>>>>> --- a/include/linux/huge_mm.h
>>>>>>> +++ b/include/linux/huge_mm.h
>>>>>>> @@ -250,6 +250,26 @@ static inline bool thp_migration_supported(void)
>>>>>>> return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
>>>>>>> }
>>>>>>> +static inline struct list_head *page_deferred_list(struct page *page)
>>>>>>> +{
>>>>>>> + /*
>>>>>>> + * Global deferred list in the second tail pages is occupied by
>>>>>>> + * compound_head.
>>>>>>> + */
>>>>>>> + return &page[2].deferred_list;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static inline struct list_head *page_memcg_deferred_list(struct page *page)
>>>>>>> +{
>>>>>>> + /*
>>>>>>> + * Memcg deferred list in the second tail pages is occupied by
>>>>>>> + * compound_head.
>>>>>>> + */
>>>>>>> + return &page[2].memcg_deferred_list;
>>>>>>> +}
>>>>>>> +
>>>>>>> +extern void del_thp_from_deferred_split_queue(struct page *);
>>>>>>> +
>>>>>>> #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>>>>> #define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })
>>>>>>> #define HPAGE_PMD_MASK ({ BUILD_BUG(); 0; })
>>>>>>> @@ -368,6 +388,10 @@ static inline bool thp_migration_supported(void)
>>>>>>> {
>>>>>>> return false;
>>>>>>> }
>>>>>>> +
>>>>>>> +static inline void del_thp_from_deferred_split_queue(struct page *page)
>>>>>>> +{
>>>>>>> +}
>>>>>>> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>>>>> #endif /* _LINUX_HUGE_MM_H */
>>>>>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>>>>>> index bc74d6a..9ff5fab 100644
>>>>>>> --- a/include/linux/memcontrol.h
>>>>>>> +++ b/include/linux/memcontrol.h
>>>>>>> @@ -316,6 +316,12 @@ struct mem_cgroup {
>>>>>>> struct list_head event_list;
>>>>>>> spinlock_t event_list_lock;
>>>>>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>>>>> + struct list_head split_queue;
>>>>>>> + unsigned long split_queue_len;
>>>>>>> + spinlock_t split_queue_lock;
>>>>>>> +#endif
>>>>>>> +
>>>>>>> struct mem_cgroup_per_node *nodeinfo[0];
>>>>>>> /* WARNING: nodeinfo must be the last member here */
>>>>>>> };
>>>>>>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>>>>>>> index 8ec38b1..405f5e6 100644
>>>>>>> --- a/include/linux/mm_types.h
>>>>>>> +++ b/include/linux/mm_types.h
>>>>>>> @@ -139,7 +139,12 @@ struct page {
>>>>>>> struct { /* Second tail page of compound page */
>>>>>>> unsigned long _compound_pad_1; /* compound_head */
>>>>>>> unsigned long _compound_pad_2;
>>>>>>> - struct list_head deferred_list;
>>>>>>> + union {
>>>>>>> + /* Global THP deferred split list */
>>>>>>> + struct list_head deferred_list;
>>>>>>> + /* Memcg THP deferred split list */
>>>>>>> + struct list_head memcg_deferred_list;
>>>>>> Why we need two namesakes for this list entry?
>>>>>>
>>>>>> For me it looks redundantly: it does not give additional information,
>>>>>> but it leads to duplication (and we have two helpers page_deferred_list()
>>>>>> and page_memcg_deferred_list() instead of one).
>>>>> Yes, kind of. Actually I was also wondering if this is worth or not. My point is this may improve the code readability. We can figure out what split queue (per node or per memcg) is being manipulated just by the name of the list.
>>>>>
>>>>> If the most people thought this is unnecessary, I'm definitely ok to just keep one name.
>>>>>
>>>>>>> + };
>>>>>>> };
>>>>>>> struct { /* Page table pages */
>>>>>>> unsigned long _pt_pad_1; /* compound_head */
>>>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>>>> index 9f8bce9..0b9cfe1 100644
>>>>>>> --- a/mm/huge_memory.c
>>>>>>> +++ b/mm/huge_memory.c
>>>>>>> @@ -492,12 +492,6 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
>>>>>>> return pmd;
>>>>>>> }
>>>>>>> -static inline struct list_head *page_deferred_list(struct page *page)
>>>>>>> -{
>>>>>>> - /* ->lru in the tail pages is occupied by compound_head. */
>>>>>>> - return &page[2].deferred_list;
>>>>>>> -}
>>>>>>> -
>>>>>>> void prep_transhuge_page(struct page *page)
>>>>>>> {
>>>>>>> /*
>>>>>>> @@ -505,7 +499,10 @@ void prep_transhuge_page(struct page *page)
>>>>>>> * as list_head: assuming THP order >= 2
>>>>>>> */
>>>>>>> - INIT_LIST_HEAD(page_deferred_list(page));
>>>>>>> + if (mem_cgroup_disabled())
>>>>>>> + INIT_LIST_HEAD(page_deferred_list(page));
>>>>>>> + else
>>>>>>> + INIT_LIST_HEAD(page_memcg_deferred_list(page));
>>>>>>> set_compound_page_dtor(page, TRANSHUGE_PAGE_DTOR);
>>>>>>> }
>>>>>>> @@ -2664,6 +2661,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>>>> bool mlocked;
>>>>>>> unsigned long flags;
>>>>>>> pgoff_t end;
>>>>>>> + struct mem_cgroup *memcg = head->mem_cgroup;
>>>>>>> VM_BUG_ON_PAGE(is_huge_zero_page(page), page);
>>>>>>> VM_BUG_ON_PAGE(!PageLocked(page), page);
>>>>>>> @@ -2744,17 +2742,30 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>>>> }
>>>>>>> /* Prevent deferred_split_scan() touching ->_refcount */
>>>>>>> - spin_lock(&pgdata->split_queue_lock);
>>>>>>> + if (!memcg)
>>>>>>> + spin_lock(&pgdata->split_queue_lock);
>>>>>>> + else
>>>>>>> + spin_lock(&memcg->split_queue_lock);
>>>>>>> count = page_count(head);
>>>>>>> mapcount = total_mapcount(head);
>>>>>>> if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) {
>>>>>>> - if (!list_empty(page_deferred_list(head))) {
>>>>>>> - pgdata->split_queue_len--;
>>>>>>> - list_del(page_deferred_list(head));
>>>>>>> + if (!memcg) {
>>>>>>> + if (!list_empty(page_deferred_list(head))) {
>>>>>>> + pgdata->split_queue_len--;
>>>>>>> + list_del(page_deferred_list(head));
>>>>>>> + }
>>>>>>> + } else {
>>>>>>> + if (!list_empty(page_memcg_deferred_list(head))) {
>>>>>>> + memcg->split_queue_len--;
>>>>>>> + list_del(page_memcg_deferred_list(head));
>>>>>>> + }
>>>>>>> }
>>>>>>> if (mapping)
>>>>>>> __dec_node_page_state(page, NR_SHMEM_THPS);
>>>>>>> - spin_unlock(&pgdata->split_queue_lock);
>>>>>>> + if (!memcg)
>>>>>>> + spin_unlock(&pgdata->split_queue_lock);
>>>>>>> + else
>>>>>>> + spin_unlock(&memcg->split_queue_lock);
>>>>>>> __split_huge_page(page, list, end, flags);
>>>>>>> if (PageSwapCache(head)) {
>>>>>>> swp_entry_t entry = { .val = page_private(head) };
>>>>>>> @@ -2771,7 +2782,10 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>>>> dump_page(page, "total_mapcount(head) > 0");
>>>>>>> BUG();
>>>>>>> }
>>>>>>> - spin_unlock(&pgdata->split_queue_lock);
>>>>>>> + if (!memcg)
>>>>>>> + spin_unlock(&pgdata->split_queue_lock);
>>>>>>> + else
>>>>>>> + spin_unlock(&memcg->split_queue_lock);
>>>>>>> fail: if (mapping)
>>>>>>> xa_unlock(&mapping->i_pages);
>>>>>>> spin_unlock_irqrestore(&pgdata->lru_lock, flags);
>>>>>>> @@ -2791,17 +2805,40 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>>>> return ret;
>>>>>>> }
>>>>>>> -void free_transhuge_page(struct page *page)
>>>>>>> +void del_thp_from_deferred_split_queue(struct page *page)
>>>>>>> {
>>>>>>> struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
>>>>>>> unsigned long flags;
>>>>>>> + struct mem_cgroup *memcg = compound_head(page)->mem_cgroup;
>>>>>>> - spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>>>>>>> - if (!list_empty(page_deferred_list(page))) {
>>>>>>> - pgdata->split_queue_len--;
>>>>>>> - list_del(page_deferred_list(page));
>>>>>>> + /*
>>>>>>> + * The THP may be not on LRU at this point, e.g. the old page of
>>>>>>> + * NUMA migration. And PageTransHuge is not enough to distinguish
>>>>>>> + * with other compound page, e.g. skb, THP destructor is not used
>>>>>>> + * anymore and will be removed, so the compound order sounds like
>>>>>>> + * the only choice here.
>>>>>>> + */
>>>>>>> + if (PageTransHuge(page) && compound_order(page) == HPAGE_PMD_ORDER) {
>>>>>>> + if (!memcg) {
>>>>>>> + spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>>>>>>> + if (!list_empty(page_deferred_list(page))) {
>>>>>>> + pgdata->split_queue_len--;
>>>>>>> + list_del(page_deferred_list(page));
>>>>>>> + }
>>>>>>> + spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
>>>>>>> + } else {
>>>>>>> + spin_lock_irqsave(&memcg->split_queue_lock, flags);
>>>>>>> + if (!list_empty(page_memcg_deferred_list(page))) {
>>>>>>> + memcg->split_queue_len--;
>>>>>>> + list_del(page_memcg_deferred_list(page));
>>>>>>> + }
>>>>>>> + spin_unlock_irqrestore(&memcg->split_queue_lock, flags);
>>>>>> Such the patterns look like a duplication of functionality, we already have
>>>>>> in list_lru: it handles both root_mem_cgroup and all children memcg.
>>>>> Would you please point me to some example code?
>>>> I mean that we do almost the same in list_lru_add(): check for whether
>>>> item is already added, find the desired list, maintain the list's len.
>>>>
>>>> It looks all the above we may replace with something like
>>>>
>>>> list_lru_add(defered_thp_lru, page_deferred_list(page))
>>>>
>>>> after necessary preparations (some rewriting of the rest of code is needed).
>>> Aha, I got your point. I'm not quite familiar with that code. I took a quick loot at it, it looks the current APIs are not good enough for deferred split, which needs irqsave/irqrestore version list add/del/move/walk and page refcount bumped version walk.
>> I missed the point about refcount bumping, could you please clarify?
>
> The deferred_split_scan() need bump refcount for every head page when scanning the deferred split queue.
Why can't we increment refcount in isolate callback of __list_lru_walk_one() function?
Kirill
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware
2019-06-13 8:19 ` Kirill Tkhai
@ 2019-06-13 17:53 ` Yang Shi
0 siblings, 0 replies; 20+ messages in thread
From: Yang Shi @ 2019-06-13 17:53 UTC (permalink / raw)
To: Kirill Tkhai, hannes, mhocko, kirill.shutemov, hughd, shakeelb, akpm
Cc: linux-mm, linux-kernel
On 6/13/19 1:19 AM, Kirill Tkhai wrote:
> On 10.06.2019 20:25, Yang Shi wrote:
>>
>> On 6/10/19 1:23 AM, Kirill Tkhai wrote:
>>> On 29.05.2019 14:25, Yang Shi wrote:
>>>> On 5/29/19 4:14 PM, Kirill Tkhai wrote:
>>>>> On 29.05.2019 05:43, Yang Shi wrote:
>>>>>> On 5/28/19 10:42 PM, Kirill Tkhai wrote:
>>>>>>> Hi, Yang,
>>>>>>>
>>>>>>> On 28.05.2019 15:44, Yang Shi wrote:
>>>>>>>> Currently THP deferred split shrinker is not memcg aware, this may cause
>>>>>>>> premature OOM with some configuration. For example the below test would
>>>>>>>> run into premature OOM easily:
>>>>>>>>
>>>>>>>> $ cgcreate -g memory:thp
>>>>>>>> $ echo 4G > /sys/fs/cgroup/memory/thp/memory/limit_in_bytes
>>>>>>>> $ cgexec -g memory:thp transhuge-stress 4000
>>>>>>>>
>>>>>>>> transhuge-stress comes from kernel selftest.
>>>>>>>>
>>>>>>>> It is easy to hit OOM, but there are still a lot THP on the deferred
>>>>>>>> split queue, memcg direct reclaim can't touch them since the deferred
>>>>>>>> split shrinker is not memcg aware.
>>>>>>>>
>>>>>>>> Convert deferred split shrinker memcg aware by introducing per memcg
>>>>>>>> deferred split queue. The THP should be on either per node or per memcg
>>>>>>>> deferred split queue if it belongs to a memcg. When the page is
>>>>>>>> immigrated to the other memcg, it will be immigrated to the target
>>>>>>>> memcg's deferred split queue too.
>>>>>>>>
>>>>>>>> And, move deleting THP from deferred split queue in page free before
>>>>>>>> memcg uncharge so that the page's memcg information is available.
>>>>>>>>
>>>>>>>> Reuse the second tail page's deferred_list for per memcg list since the
>>>>>>>> same THP can't be on multiple deferred split queues.
>>>>>>>>
>>>>>>>> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>>>>>> Cc: Johannes Weiner <hannes@cmpxchg.org>
>>>>>>>> Cc: Michal Hocko <mhocko@suse.com>
>>>>>>>> Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
>>>>>>>> Cc: Hugh Dickins <hughd@google.com>
>>>>>>>> Cc: Shakeel Butt <shakeelb@google.com>
>>>>>>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>>>>>>>> ---
>>>>>>>> include/linux/huge_mm.h | 24 ++++++
>>>>>>>> include/linux/memcontrol.h | 6 ++
>>>>>>>> include/linux/mm_types.h | 7 +-
>>>>>>>> mm/huge_memory.c | 182 +++++++++++++++++++++++++++++++++------------
>>>>>>>> mm/memcontrol.c | 20 +++++
>>>>>>>> mm/swap.c | 4 +
>>>>>>>> 6 files changed, 194 insertions(+), 49 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>>>>>> index 7cd5c15..f6d1cde 100644
>>>>>>>> --- a/include/linux/huge_mm.h
>>>>>>>> +++ b/include/linux/huge_mm.h
>>>>>>>> @@ -250,6 +250,26 @@ static inline bool thp_migration_supported(void)
>>>>>>>> return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
>>>>>>>> }
>>>>>>>> +static inline struct list_head *page_deferred_list(struct page *page)
>>>>>>>> +{
>>>>>>>> + /*
>>>>>>>> + * Global deferred list in the second tail pages is occupied by
>>>>>>>> + * compound_head.
>>>>>>>> + */
>>>>>>>> + return &page[2].deferred_list;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static inline struct list_head *page_memcg_deferred_list(struct page *page)
>>>>>>>> +{
>>>>>>>> + /*
>>>>>>>> + * Memcg deferred list in the second tail pages is occupied by
>>>>>>>> + * compound_head.
>>>>>>>> + */
>>>>>>>> + return &page[2].memcg_deferred_list;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +extern void del_thp_from_deferred_split_queue(struct page *);
>>>>>>>> +
>>>>>>>> #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>>>>>> #define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })
>>>>>>>> #define HPAGE_PMD_MASK ({ BUILD_BUG(); 0; })
>>>>>>>> @@ -368,6 +388,10 @@ static inline bool thp_migration_supported(void)
>>>>>>>> {
>>>>>>>> return false;
>>>>>>>> }
>>>>>>>> +
>>>>>>>> +static inline void del_thp_from_deferred_split_queue(struct page *page)
>>>>>>>> +{
>>>>>>>> +}
>>>>>>>> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>>>>>> #endif /* _LINUX_HUGE_MM_H */
>>>>>>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>>>>>>> index bc74d6a..9ff5fab 100644
>>>>>>>> --- a/include/linux/memcontrol.h
>>>>>>>> +++ b/include/linux/memcontrol.h
>>>>>>>> @@ -316,6 +316,12 @@ struct mem_cgroup {
>>>>>>>> struct list_head event_list;
>>>>>>>> spinlock_t event_list_lock;
>>>>>>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>>>>>> + struct list_head split_queue;
>>>>>>>> + unsigned long split_queue_len;
>>>>>>>> + spinlock_t split_queue_lock;
>>>>>>>> +#endif
>>>>>>>> +
>>>>>>>> struct mem_cgroup_per_node *nodeinfo[0];
>>>>>>>> /* WARNING: nodeinfo must be the last member here */
>>>>>>>> };
>>>>>>>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>>>>>>>> index 8ec38b1..405f5e6 100644
>>>>>>>> --- a/include/linux/mm_types.h
>>>>>>>> +++ b/include/linux/mm_types.h
>>>>>>>> @@ -139,7 +139,12 @@ struct page {
>>>>>>>> struct { /* Second tail page of compound page */
>>>>>>>> unsigned long _compound_pad_1; /* compound_head */
>>>>>>>> unsigned long _compound_pad_2;
>>>>>>>> - struct list_head deferred_list;
>>>>>>>> + union {
>>>>>>>> + /* Global THP deferred split list */
>>>>>>>> + struct list_head deferred_list;
>>>>>>>> + /* Memcg THP deferred split list */
>>>>>>>> + struct list_head memcg_deferred_list;
>>>>>>> Why we need two namesakes for this list entry?
>>>>>>>
>>>>>>> For me it looks redundantly: it does not give additional information,
>>>>>>> but it leads to duplication (and we have two helpers page_deferred_list()
>>>>>>> and page_memcg_deferred_list() instead of one).
>>>>>> Yes, kind of. Actually I was also wondering if this is worth or not. My point is this may improve the code readability. We can figure out what split queue (per node or per memcg) is being manipulated just by the name of the list.
>>>>>>
>>>>>> If the most people thought this is unnecessary, I'm definitely ok to just keep one name.
>>>>>>
>>>>>>>> + };
>>>>>>>> };
>>>>>>>> struct { /* Page table pages */
>>>>>>>> unsigned long _pt_pad_1; /* compound_head */
>>>>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>>>>> index 9f8bce9..0b9cfe1 100644
>>>>>>>> --- a/mm/huge_memory.c
>>>>>>>> +++ b/mm/huge_memory.c
>>>>>>>> @@ -492,12 +492,6 @@ pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
>>>>>>>> return pmd;
>>>>>>>> }
>>>>>>>> -static inline struct list_head *page_deferred_list(struct page *page)
>>>>>>>> -{
>>>>>>>> - /* ->lru in the tail pages is occupied by compound_head. */
>>>>>>>> - return &page[2].deferred_list;
>>>>>>>> -}
>>>>>>>> -
>>>>>>>> void prep_transhuge_page(struct page *page)
>>>>>>>> {
>>>>>>>> /*
>>>>>>>> @@ -505,7 +499,10 @@ void prep_transhuge_page(struct page *page)
>>>>>>>> * as list_head: assuming THP order >= 2
>>>>>>>> */
>>>>>>>> - INIT_LIST_HEAD(page_deferred_list(page));
>>>>>>>> + if (mem_cgroup_disabled())
>>>>>>>> + INIT_LIST_HEAD(page_deferred_list(page));
>>>>>>>> + else
>>>>>>>> + INIT_LIST_HEAD(page_memcg_deferred_list(page));
>>>>>>>> set_compound_page_dtor(page, TRANSHUGE_PAGE_DTOR);
>>>>>>>> }
>>>>>>>> @@ -2664,6 +2661,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>>>>> bool mlocked;
>>>>>>>> unsigned long flags;
>>>>>>>> pgoff_t end;
>>>>>>>> + struct mem_cgroup *memcg = head->mem_cgroup;
>>>>>>>> VM_BUG_ON_PAGE(is_huge_zero_page(page), page);
>>>>>>>> VM_BUG_ON_PAGE(!PageLocked(page), page);
>>>>>>>> @@ -2744,17 +2742,30 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>>>>> }
>>>>>>>> /* Prevent deferred_split_scan() touching ->_refcount */
>>>>>>>> - spin_lock(&pgdata->split_queue_lock);
>>>>>>>> + if (!memcg)
>>>>>>>> + spin_lock(&pgdata->split_queue_lock);
>>>>>>>> + else
>>>>>>>> + spin_lock(&memcg->split_queue_lock);
>>>>>>>> count = page_count(head);
>>>>>>>> mapcount = total_mapcount(head);
>>>>>>>> if (!mapcount && page_ref_freeze(head, 1 + extra_pins)) {
>>>>>>>> - if (!list_empty(page_deferred_list(head))) {
>>>>>>>> - pgdata->split_queue_len--;
>>>>>>>> - list_del(page_deferred_list(head));
>>>>>>>> + if (!memcg) {
>>>>>>>> + if (!list_empty(page_deferred_list(head))) {
>>>>>>>> + pgdata->split_queue_len--;
>>>>>>>> + list_del(page_deferred_list(head));
>>>>>>>> + }
>>>>>>>> + } else {
>>>>>>>> + if (!list_empty(page_memcg_deferred_list(head))) {
>>>>>>>> + memcg->split_queue_len--;
>>>>>>>> + list_del(page_memcg_deferred_list(head));
>>>>>>>> + }
>>>>>>>> }
>>>>>>>> if (mapping)
>>>>>>>> __dec_node_page_state(page, NR_SHMEM_THPS);
>>>>>>>> - spin_unlock(&pgdata->split_queue_lock);
>>>>>>>> + if (!memcg)
>>>>>>>> + spin_unlock(&pgdata->split_queue_lock);
>>>>>>>> + else
>>>>>>>> + spin_unlock(&memcg->split_queue_lock);
>>>>>>>> __split_huge_page(page, list, end, flags);
>>>>>>>> if (PageSwapCache(head)) {
>>>>>>>> swp_entry_t entry = { .val = page_private(head) };
>>>>>>>> @@ -2771,7 +2782,10 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>>>>> dump_page(page, "total_mapcount(head) > 0");
>>>>>>>> BUG();
>>>>>>>> }
>>>>>>>> - spin_unlock(&pgdata->split_queue_lock);
>>>>>>>> + if (!memcg)
>>>>>>>> + spin_unlock(&pgdata->split_queue_lock);
>>>>>>>> + else
>>>>>>>> + spin_unlock(&memcg->split_queue_lock);
>>>>>>>> fail: if (mapping)
>>>>>>>> xa_unlock(&mapping->i_pages);
>>>>>>>> spin_unlock_irqrestore(&pgdata->lru_lock, flags);
>>>>>>>> @@ -2791,17 +2805,40 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>>>>>>> return ret;
>>>>>>>> }
>>>>>>>> -void free_transhuge_page(struct page *page)
>>>>>>>> +void del_thp_from_deferred_split_queue(struct page *page)
>>>>>>>> {
>>>>>>>> struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
>>>>>>>> unsigned long flags;
>>>>>>>> + struct mem_cgroup *memcg = compound_head(page)->mem_cgroup;
>>>>>>>> - spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>>>>>>>> - if (!list_empty(page_deferred_list(page))) {
>>>>>>>> - pgdata->split_queue_len--;
>>>>>>>> - list_del(page_deferred_list(page));
>>>>>>>> + /*
>>>>>>>> + * The THP may be not on LRU at this point, e.g. the old page of
>>>>>>>> + * NUMA migration. And PageTransHuge is not enough to distinguish
>>>>>>>> + * with other compound page, e.g. skb, THP destructor is not used
>>>>>>>> + * anymore and will be removed, so the compound order sounds like
>>>>>>>> + * the only choice here.
>>>>>>>> + */
>>>>>>>> + if (PageTransHuge(page) && compound_order(page) == HPAGE_PMD_ORDER) {
>>>>>>>> + if (!memcg) {
>>>>>>>> + spin_lock_irqsave(&pgdata->split_queue_lock, flags);
>>>>>>>> + if (!list_empty(page_deferred_list(page))) {
>>>>>>>> + pgdata->split_queue_len--;
>>>>>>>> + list_del(page_deferred_list(page));
>>>>>>>> + }
>>>>>>>> + spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
>>>>>>>> + } else {
>>>>>>>> + spin_lock_irqsave(&memcg->split_queue_lock, flags);
>>>>>>>> + if (!list_empty(page_memcg_deferred_list(page))) {
>>>>>>>> + memcg->split_queue_len--;
>>>>>>>> + list_del(page_memcg_deferred_list(page));
>>>>>>>> + }
>>>>>>>> + spin_unlock_irqrestore(&memcg->split_queue_lock, flags);
>>>>>>> Such the patterns look like a duplication of functionality, we already have
>>>>>>> in list_lru: it handles both root_mem_cgroup and all children memcg.
>>>>>> Would you please point me to some example code?
>>>>> I mean that we do almost the same in list_lru_add(): check for whether
>>>>> item is already added, find the desired list, maintain the list's len.
>>>>>
>>>>> It looks all the above we may replace with something like
>>>>>
>>>>> list_lru_add(defered_thp_lru, page_deferred_list(page))
>>>>>
>>>>> after necessary preparations (some rewriting of the rest of code is needed).
>>>> Aha, I got your point. I'm not quite familiar with that code. I took a quick loot at it, it looks the current APIs are not good enough for deferred split, which needs irqsave/irqrestore version list add/del/move/walk and page refcount bumped version walk.
>>> I missed the point about refcount bumping, could you please clarify?
>> The deferred_split_scan() need bump refcount for every head page when scanning the deferred split queue.
> Why can't we increment refcount in isolate callback of __list_lru_walk_one() function?
I don't mean we can't do that. It is definitely feasible. I missed the
isolate callback part when I had first glance.
>
> Kirill
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2019-06-13 17:54 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-28 12:44 [RFC PATCH 0/3] Make deferred split shrinker memcg aware Yang Shi
2019-05-28 12:44 ` [PATCH 1/3] mm: thp: make " Yang Shi
2019-05-28 14:42 ` Kirill Tkhai
2019-05-29 2:43 ` Yang Shi
2019-05-29 8:14 ` Kirill Tkhai
2019-05-29 11:25 ` Yang Shi
2019-06-10 8:23 ` Kirill Tkhai
2019-06-10 17:25 ` Yang Shi
2019-06-13 8:19 ` Kirill Tkhai
2019-06-13 17:53 ` Yang Shi
2019-05-30 12:07 ` Kirill A. Shutemov
2019-05-30 13:29 ` Yang Shi
2019-05-28 12:44 ` [PATCH 2/3] mm: thp: remove THP destructor Yang Shi
2019-05-28 12:44 ` [PATCH 3/3] mm: shrinker: make shrinker not depend on memcg kmem Yang Shi
2019-05-30 12:08 ` Kirill A. Shutemov
2019-05-30 13:20 ` Yang Shi
2019-05-29 1:22 ` [RFC PATCH 0/3] Make deferred split shrinker memcg aware David Rientjes
2019-05-29 2:34 ` Yang Shi
2019-05-29 21:07 ` David Rientjes
2019-05-30 3:22 ` Yang Shi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).