Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware
From: Yang Shi
Date: Thu Jun 13 2019 - 13:58:33 EST
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@xxxxxxxxxxxxx>
Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
Cc: Michal Hocko <mhocko@xxxxxxxx>
Cc: "Kirill A . Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
Cc: Hugh Dickins <hughd@xxxxxxxxxx>
Cc: Shakeel Butt <shakeelb@xxxxxxxxxx>
Signed-off-by: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
---
ÂÂÂ 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