[RFC PATCH 0/3] Make deferred split shrinker memcg aware

[RFC PATCH 0/3] Make deferred split shrinker memcg aware

[Yang Shi]

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(-)

[PATCH 1/3] mm: thp: make deferred split shrinker memcg aware

[Yang Shi]

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

Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware

[Kirill Tkhai]

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

Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware

[Yang Shi]

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

Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware

[Kirill Tkhai]

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

Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware

[Yang Shi]

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

Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware

[Kirill Tkhai]

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

Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware

[Yang Shi]

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

Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware

[Kirill Tkhai]

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

Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware

[Yang Shi]

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

Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware

[Kirill A. Shutemov]

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

Re: [PATCH 1/3] mm: thp: make deferred split shrinker memcg aware

[Yang Shi]

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.

>

[PATCH 2/3] mm: thp: remove THP destructor

[Yang Shi]

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

[PATCH 3/3] mm: shrinker: make shrinker not depend on memcg kmem

[Yang Shi]

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

Re: [PATCH 3/3] mm: shrinker: make shrinker not depend on memcg kmem

[Kirill A. Shutemov]

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

Re: [PATCH 3/3] mm: shrinker: make shrinker not depend on memcg kmem

[Yang Shi]

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.

>

Re: [RFC PATCH 0/3] Make deferred split shrinker memcg aware

[David Rientjes]

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!

Re: [RFC PATCH 0/3] Make deferred split shrinker memcg aware

[Yang Shi]

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!

Re: [RFC PATCH 0/3] Make deferred split shrinker memcg aware

[David Rientjes]

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.

Re: [RFC PATCH 0/3] Make deferred split shrinker memcg aware

[Yang Shi]

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.