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 <ktk...@virtuozzo.com>
>>>>>>> Cc: Johannes Weiner <han...@cmpxchg.org>
>>>>>>> Cc: Michal Hocko <mho...@suse.com>
>>>>>>> Cc: "Kirill A . Shutemov" <kirill.shute...@linux.intel.com>
>>>>>>> Cc: Hugh Dickins <hu...@google.com>
>>>>>>> Cc: Shakeel Butt <shake...@google.com>
>>>>>>> Signed-off-by: Yang Shi <yang....@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
