Message-ID: <73117498-9e99-7a2d-2243-5dff1ff4c4b2@linux.alibaba.com>
Date:   Mon, 10 Jun 2019 10:23:06 -0700
From:   Yang Shi <yang.shi@...ux.alibaba.com>
To:     Kirill Tkhai <ktkhai@...tuozzo.com>,
        kirill.shutemov@...ux.intel.com, hannes@...xchg.org,
        mhocko@...e.com, hughd@...gle.com, shakeelb@...gle.com,
        rientjes@...gle.com, akpm@...ux-foundation.org
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] mm: thp: make deferred split shrinker memcg aware



On 6/10/19 1:21 AM, Kirill Tkhai wrote:
> Hi, Yang,
>
> On 07.06.2019 09:07, 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@...tuozzo.com>
>> Cc: Johannes Weiner <hannes@...xchg.org>
>> Cc: Michal Hocko <mhocko@...e.com>
>> Cc: "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>
>> Cc: Hugh Dickins <hughd@...gle.com>
>> Cc: Shakeel Butt <shakeelb@...gle.com>
>> Cc: David Rientjes <rientjes@...gle.com>
>> Signed-off-by: Yang Shi <yang.shi@...ux.alibaba.com>
>> ---
>>   include/linux/huge_mm.h    | 15 ++++++++++
>>   include/linux/memcontrol.h |  4 +++
>>   include/linux/mm_types.h   |  1 +
>>   mm/huge_memory.c           | 71 +++++++++++++++++++++++++++++++++-------------
>>   mm/memcontrol.c            | 19 +++++++++++++
>>   mm/swap.c                  |  4 +++
>>   6 files changed, 94 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>> index 7cd5c15..8137c3a 100644
>> --- a/include/linux/huge_mm.h
>> +++ b/include/linux/huge_mm.h
>> @@ -250,6 +250,17 @@ 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 or memcg deferred list in the second tail pages is
>> +	 * occupied by compound_head.
>> +	 */
>> +	return &page[2].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 +379,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..5d3c10c 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -316,6 +316,10 @@ struct mem_cgroup {
>>   	struct list_head event_list;
>>   	spinlock_t event_list_lock;
>>   
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +	struct deferred_split deferred_split_queue;
>> +#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..4eabf80 100644
>> --- a/include/linux/mm_types.h
>> +++ b/include/linux/mm_types.h
>> @@ -139,6 +139,7 @@ struct page {
>>   		struct {	/* Second tail page of compound page */
>>   			unsigned long _compound_pad_1;	/* compound_head */
>>   			unsigned long _compound_pad_2;
>> +			/* For both global and memcg */
>>   			struct list_head deferred_list;
>>   		};
>>   		struct {	/* Page table pages */
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 81cf759..3307697 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -492,10 +492,15 @@ 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)
>> +static inline struct deferred_split *get_deferred_split_queue(struct page *page)
>>   {
>> -	/* ->lru in the tail pages is occupied by compound_head. */
>> -	return &page[2].deferred_list;
>> +	struct mem_cgroup *memcg = compound_head(page)->mem_cgroup;
>> +	struct pglist_data *pgdat = NODE_DATA(page_to_nid(page));
>> +
>> +	if (memcg)
>> +		return &memcg->deferred_split_queue;
>> +	else
>> +		return &pgdat->deferred_split_queue;
> memory_cgrp_subsys is not early initialized, so at the beginning of boot
> root_mem_cgroup is NULL, and pages will use &pgdat->deferred_split_queue
> list head. But after root_mem_cgroup is initialized, another list head
> will be used, won't it?! So there will be two different list heads used
> for same cgroup.
>
> This may be a reason of a problem (I won't say you, where the problem will
> occur).

Actually I was wondering this too. But, it looks impossible. The memcg 
is initialized by subsys_initcall, there are still fs, device and late 
initcalls after it. The init process is run much later, so I can't think 
of any THP could be put on deferred split queue before memcg is 
initialized. There may be compound page allocation, i.e. skb, but we 
don't care them.

>
>>   }
>>   
>>   void prep_transhuge_page(struct page *page)
>> @@ -2658,7 +2663,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
>>   {
>>   	struct page *head = compound_head(page);
>>   	struct pglist_data *pgdata = NODE_DATA(page_to_nid(head));
>> -	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
>> +	struct deferred_split *ds_queue = get_deferred_split_queue(page);
>>   	struct anon_vma *anon_vma = NULL;
>>   	struct address_space *mapping = NULL;
>>   	int count, mapcount, extra_pins, ret;
>> @@ -2792,25 +2797,36 @@ 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));
>> -	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
>> -	unsigned long flags;
>> -
>> -	spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
>> -	if (!list_empty(page_deferred_list(page))) {
>> -		ds_queue->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) {
>> +		struct deferred_split *ds_queue = get_deferred_split_queue(page);
>> +		unsigned long flags;
>> +		spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
>> +			if (!list_empty(page_deferred_list(page))) {
>> +				ds_queue->split_queue_len--;
>> +				list_del(page_deferred_list(page));
>> +			}
>> +		spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
>>   	}
>> -	spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
>> +}
>> +
>> +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));
>> -	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
>> +	struct deferred_split *ds_queue = get_deferred_split_queue(page);
>> +	struct mem_cgroup *memcg = compound_head(page)->mem_cgroup;
>>   	unsigned long flags;
>>   
>>   	VM_BUG_ON_PAGE(!PageTransHuge(page), page);
>> @@ -2820,6 +2836,9 @@ void deferred_split_huge_page(struct page *page)
>>   		count_vm_event(THP_DEFERRED_SPLIT_PAGE);
>>   		list_add_tail(page_deferred_list(page), &ds_queue->split_queue);
>>   		ds_queue->split_queue_len++;
>> +		if (memcg)
>> +			memcg_set_shrinker_bit(memcg, page_to_nid(page),
>> +					       deferred_split_shrinker.id);
>>   	}
>>   	spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags);
>>   }
>> @@ -2827,8 +2846,15 @@ void deferred_split_huge_page(struct page *page)
>>   static unsigned long deferred_split_count(struct shrinker *shrink,
>>   		struct shrink_control *sc)
>>   {
>> -	struct pglist_data *pgdata = NODE_DATA(sc->nid);
>> -	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
>> +	struct deferred_split *ds_queue;
>> +
>> +	if (!sc->memcg) {
>> +		struct pglist_data *pgdata = NODE_DATA(sc->nid);
>> +		ds_queue = &pgdata->deferred_split_queue;
>> +		return READ_ONCE(ds_queue->split_queue_len);
>> +	}
>> +
>> +	ds_queue = &sc->memcg->deferred_split_queue;
>>   	return READ_ONCE(ds_queue->split_queue_len);
>>   }
>>   
>> @@ -2836,12 +2862,17 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
>>   		struct shrink_control *sc)
>>   {
>>   	struct pglist_data *pgdata = NODE_DATA(sc->nid);
>> -	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
>> +	struct deferred_split *ds_queue;
>>   	unsigned long flags;
>>   	LIST_HEAD(list), *pos, *next;
>>   	struct page *page;
>>   	int split = 0;
>>   
>> +	if (sc->memcg)
>> +		ds_queue = &sc->memcg->deferred_split_queue;
>> +	else
>> +		ds_queue = &pgdata->deferred_split_queue;
>> +
>>   	spin_lock_irqsave(&ds_queue->split_queue_lock, flags);
>>   	/* Take pin on all head pages to avoid freeing them under us */
>>   	list_for_each_safe(pos, next, &ds_queue->split_queue) {
>> @@ -2888,7 +2919,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..fe7e544 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->deferred_split_queue.split_queue_lock);
>> +	INIT_LIST_HEAD(&memcg->deferred_split_queue.split_queue);
>> +	memcg->deferred_split_queue.split_queue_len = 0;
>> +#endif
>>   	idr_replace(&mem_cgroup_idr, memcg, memcg->id.id);
>>   	return memcg;
>>   fail:
>> @@ -4949,6 +4954,20 @@ 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_deferred_list(page))) {
>> +		spin_lock(&from->deferred_split_queue.split_queue_lock);
>> +		list_del(page_deferred_list(page));
>> +		from->deferred_split_queue.split_queue_len--;
>> +		spin_unlock(&from->deferred_split_queue.split_queue_lock);
> Won't be better to assign
>
> page->mem_cgroup = to;
>
> after removing from one list and before linking to another list?
> There is possible no a problem, but another people writing code
> on top of this may not expect such the behavior.

It should be fine too.

>> +		spin_lock(&to->deferred_split_queue.split_queue_lock);
>> +		list_add_tail(page_deferred_list(page),
>> +			      &to->deferred_split_queue.split_queue);
>> +		to->deferred_split_queue.split_queue_len++;
>> +		spin_unlock(&to->deferred_split_queue.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);
>>   }

Powered by blists - more mailing lists