From: Michal Hocko <mhocko@kernel.org> To: Zi Yan <zi.yan@cs.rutgers.edu> Cc: linux-mm@kvack.org, Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>, "Kirill A. Shutemov" <kirill@shutemov.name>, Vlastimil Babka <vbabka@suse.cz>, Andrew Morton <akpm@linux-foundation.org>, Andrea Reale <ar@linux.vnet.ibm.com>, LKML <linux-kernel@vger.kernel.org> Subject: Re: [RFC PATCH] mm: unclutter THP migration Date: Thu, 7 Dec 2017 15:34:01 +0100 [thread overview] Message-ID: <20171207143401.GK20234@dhcp22.suse.cz> (raw) In-Reply-To: <5A294BE7.4010904@cs.rutgers.edu> On Thu 07-12-17 22:10:47, Zi Yan wrote: > Hi Michal, > > Thanks for sending this out. > > Michal Hocko wrote: > > From: Michal Hocko <mhocko@suse.com> > > > > THP migration is hacked into the generic migration with rather > > surprising semantic. The migration allocation callback is supposed to > > check whether the THP can be migrated at once and if that is not the > > case then it allocates a simple page to migrate. unmap_and_move then > > fixes that up by spliting the THP into small pages while moving the > > head page to the newly allocated order-0 page. Remaning pages are moved > > to the LRU list by split_huge_page. The same happens if the THP > > allocation fails. This is really ugly and error prone [1]. > > > > I also believe that split_huge_page to the LRU lists is inherently > > wrong because all tail pages are not migrated. Some callers will just > > I agree with you that we should try to migrate all tail pages if the THP > needs to be split. But this might not be compatible with "getting > migration results" in unmap_and_move(), since a caller of > migrate_pages() may want to know the status of each page in the > migration list via int **result in get_new_page() (e.g. > new_page_node()). The caller has no idea whether a THP in its migration > list will be split or not, thus, storing migration results might be > quite tricky if tail pages are added into the migration list. Ouch. I wasn't aware of this "beauty". I will try to wrap my head around this code and think about what to do about it. Thanks for point me to it. > We need to consider this when we clean up migrate_pages(). > [...] > > diff --git a/include/linux/migrate.h b/include/linux/migrate.h > > index a2246cf670ba..ec9503e5f2c2 100644 > > --- a/include/linux/migrate.h > > +++ b/include/linux/migrate.h > > @@ -43,9 +43,11 @@ static inline struct page *new_page_nodemask(struct page *page, > > return alloc_huge_page_nodemask(page_hstate(compound_head(page)), > > preferred_nid, nodemask); > > > > - if (thp_migration_supported() && PageTransHuge(page)) { > > - order = HPAGE_PMD_ORDER; > > + if (PageTransHuge(page)) { > > + if (!thp_migration_supported()) > > + return NULL; > We may not need these two lines, since if thp_migration_supported() is > false, unmap_and_move() returns -ENOMEM in your code below, which has > the same result of returning NULL here. yes, this is a left over after rebase. Originally I used to have thp_migration_supported in allocation callbacks but then moved it to unmap_and_move to reduce the code duplication and also it makes much more sense to have this up in the migration layer. I've fixed this up in my local copy now. -- Michal Hocko SUSE Labs
WARNING: multiple messages have this Message-ID (diff)
From: Michal Hocko <mhocko@kernel.org> To: Zi Yan <zi.yan@cs.rutgers.edu> Cc: linux-mm@kvack.org, Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>, "Kirill A. Shutemov" <kirill@shutemov.name>, Vlastimil Babka <vbabka@suse.cz>, Andrew Morton <akpm@linux-foundation.org>, Andrea Reale <ar@linux.vnet.ibm.com>, LKML <linux-kernel@vger.kernel.org> Subject: Re: [RFC PATCH] mm: unclutter THP migration Date: Thu, 7 Dec 2017 15:34:01 +0100 [thread overview] Message-ID: <20171207143401.GK20234@dhcp22.suse.cz> (raw) In-Reply-To: <5A294BE7.4010904@cs.rutgers.edu> On Thu 07-12-17 22:10:47, Zi Yan wrote: > Hi Michal, > > Thanks for sending this out. > > Michal Hocko wrote: > > From: Michal Hocko <mhocko@suse.com> > > > > THP migration is hacked into the generic migration with rather > > surprising semantic. The migration allocation callback is supposed to > > check whether the THP can be migrated at once and if that is not the > > case then it allocates a simple page to migrate. unmap_and_move then > > fixes that up by spliting the THP into small pages while moving the > > head page to the newly allocated order-0 page. Remaning pages are moved > > to the LRU list by split_huge_page. The same happens if the THP > > allocation fails. This is really ugly and error prone [1]. > > > > I also believe that split_huge_page to the LRU lists is inherently > > wrong because all tail pages are not migrated. Some callers will just > > I agree with you that we should try to migrate all tail pages if the THP > needs to be split. But this might not be compatible with "getting > migration results" in unmap_and_move(), since a caller of > migrate_pages() may want to know the status of each page in the > migration list via int **result in get_new_page() (e.g. > new_page_node()). The caller has no idea whether a THP in its migration > list will be split or not, thus, storing migration results might be > quite tricky if tail pages are added into the migration list. Ouch. I wasn't aware of this "beauty". I will try to wrap my head around this code and think about what to do about it. Thanks for point me to it. > We need to consider this when we clean up migrate_pages(). > [...] > > diff --git a/include/linux/migrate.h b/include/linux/migrate.h > > index a2246cf670ba..ec9503e5f2c2 100644 > > --- a/include/linux/migrate.h > > +++ b/include/linux/migrate.h > > @@ -43,9 +43,11 @@ static inline struct page *new_page_nodemask(struct page *page, > > return alloc_huge_page_nodemask(page_hstate(compound_head(page)), > > preferred_nid, nodemask); > > > > - if (thp_migration_supported() && PageTransHuge(page)) { > > - order = HPAGE_PMD_ORDER; > > + if (PageTransHuge(page)) { > > + if (!thp_migration_supported()) > > + return NULL; > We may not need these two lines, since if thp_migration_supported() is > false, unmap_and_move() returns -ENOMEM in your code below, which has > the same result of returning NULL here. yes, this is a left over after rebase. Originally I used to have thp_migration_supported in allocation callbacks but then moved it to unmap_and_move to reduce the code duplication and also it makes much more sense to have this up in the migration layer. I've fixed this up in my local copy now. -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2017-12-07 14:34 UTC|newest] Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-12-07 12:48 [RFC PATCH] mm: unclutter THP migration Michal Hocko 2017-12-07 12:48 ` Michal Hocko 2017-12-07 14:10 ` Zi Yan 2017-12-07 14:34 ` Michal Hocko [this message] 2017-12-07 14:34 ` Michal Hocko 2017-12-08 16:15 ` [RFC PATCH 0/3] " Michal Hocko 2017-12-08 16:15 ` Michal Hocko 2017-12-08 16:15 ` [RFC PATCH 1/3] mm, numa: rework do_pages_move Michal Hocko 2017-12-08 16:15 ` Michal Hocko 2017-12-13 12:07 ` Kirill A. Shutemov 2017-12-13 12:07 ` Kirill A. Shutemov 2017-12-13 12:17 ` Michal Hocko 2017-12-13 12:17 ` Michal Hocko 2017-12-13 12:47 ` Kirill A. Shutemov 2017-12-13 12:47 ` Kirill A. Shutemov 2017-12-13 14:10 ` Michal Hocko 2017-12-13 14:10 ` Michal Hocko 2017-12-13 14:27 ` Kirill A. Shutemov 2017-12-13 14:27 ` Kirill A. Shutemov 2017-12-13 14:39 ` Michal Hocko 2017-12-13 14:39 ` Michal Hocko 2017-12-14 15:35 ` Kirill A. Shutemov 2017-12-14 15:35 ` Kirill A. Shutemov 2017-12-15 9:28 ` Michal Hocko 2017-12-15 9:28 ` Michal Hocko 2017-12-15 9:51 ` Kirill A. Shutemov 2017-12-15 9:51 ` Kirill A. Shutemov 2017-12-15 9:57 ` Michal Hocko 2017-12-15 9:57 ` Michal Hocko 2018-01-02 11:25 ` Anshuman Khandual 2018-01-02 11:25 ` Anshuman Khandual 2018-01-02 12:12 ` Michal Hocko 2018-01-02 12:12 ` Michal Hocko 2018-01-03 3:11 ` Anshuman Khandual 2018-01-03 3:11 ` Anshuman Khandual 2018-01-03 8:42 ` Anshuman Khandual 2018-01-03 8:42 ` Anshuman Khandual 2018-01-03 8:58 ` Michal Hocko 2018-01-03 8:58 ` Michal Hocko 2018-01-03 9:36 ` Anshuman Khandual 2018-01-03 9:36 ` Anshuman Khandual 2018-01-03 9:52 ` Michal Hocko 2018-01-03 9:52 ` Michal Hocko 2017-12-08 16:15 ` [RFC PATCH 2/3] mm, migrate: remove reason argument from new_page_t Michal Hocko 2017-12-08 16:15 ` Michal Hocko 2017-12-27 2:12 ` Zi Yan 2017-12-29 11:32 ` Michal Hocko 2017-12-29 11:32 ` Michal Hocko 2017-12-08 16:15 ` [RFC PATCH 3/3] mm: unclutter THP migration Michal Hocko 2017-12-08 16:15 ` Michal Hocko 2017-12-13 12:20 ` Kirill A. Shutemov 2017-12-13 12:20 ` Kirill A. Shutemov 2017-12-27 2:19 ` Zi Yan 2017-12-29 11:36 ` Michal Hocko 2017-12-29 11:36 ` Michal Hocko 2017-12-29 15:45 ` Zi Yan 2017-12-31 9:07 ` Michal Hocko 2017-12-31 9:07 ` Michal Hocko 2017-12-31 13:09 ` Zi Yan 2017-12-19 12:07 ` [RFC PATCH 0/3] " Michal Hocko 2017-12-19 12:07 ` Michal Hocko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20171207143401.GK20234@dhcp22.suse.cz \ --to=mhocko@kernel.org \ --cc=akpm@linux-foundation.org \ --cc=ar@linux.vnet.ibm.com \ --cc=kirill@shutemov.name \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=n-horiguchi@ah.jp.nec.com \ --cc=vbabka@suse.cz \ --cc=zi.yan@cs.rutgers.edu \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.