From: Michal Hocko <mhocko@suse.cz> To: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> Cc: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>, Mel Gorman <mel@csn.ul.ie>, Hugh Dickins <hughd@google.com>, KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>, Andi Kleen <andi@firstfloor.org>, Hillf Danton <dhillf@gmail.com>, linux-kernel@vger.kernel.org Subject: Re: [PATCH 06/10] migrate: add hugepage migration code to move_pages() Date: Tue, 26 Mar 2013 11:02:21 +0100 [thread overview] Message-ID: <20130326100221.GN2295@dhcp22.suse.cz> (raw) In-Reply-To: <1364281578-4bs50rjv-mutt-n-horiguchi@ah.jp.nec.com> On Tue 26-03-13 03:06:18, Naoya Horiguchi wrote: > On Mon, Mar 25, 2013 at 02:36:44PM +0100, Michal Hocko wrote: > > On Fri 22-03-13 16:23:51, Naoya Horiguchi wrote: [...] > > > @@ -1514,8 +1515,9 @@ struct page *follow_page_mask(struct vm_area_struct *vma, > > > if (pmd_none(*pmd)) > > > goto no_page_table; > > > if (pmd_huge(*pmd) && vma->vm_flags & VM_HUGETLB) { > > > - BUG_ON(flags & FOLL_GET); > > > page = follow_huge_pmd(mm, address, pmd, flags & FOLL_WRITE); > > > + if (flags & FOLL_GET && PageHead(page)) > > > + get_page_foll(page); > > > > Hmm, so the caller gets a non-null page without elevated ref counted > > even when he asked for it. This means that all callers have to check > > PageTail && hugetlb and put_page according to that. That is _really_ > > fragile. > > I agree. And refcounting of tail pages are already very fragile, > because get_page_foll() does something very tricky on tail pages, > where we use page->_mapcount for refcount. > This seems to be to handle some thp splitting problem, > and is never intended to be used for hugepage. yes this is THP thingy. > So I just avoid calling it for tail pages of hugepage in caller's side. > > > I think that returning NULL would make more sense in this case. > > Sounds nice. I'll do this with some comment. > > > > goto out; > > > } > > > if ((flags & FOLL_NUMA) && pmd_numa(*pmd)) > > > @@ -1164,6 +1175,12 @@ static int do_move_page_to_node_array(struct mm_struct *mm, > > [...] > > > !migrate_all) > > > goto put_and_set; > > > > > > + if (PageHuge(page)) { > > > + get_page(page); > > > + list_move_tail(&page->lru, &pagelist); > > > + goto put_and_set; > > > + } > > > > Why do you take an additional reference here? You have one from > > follow_page already. > > For normal pages, follow_page(FOLL_GET) takes a refcount and > isolate_lru_page() takes another one, so I think the same should > be done for hugepages. Refcounting of this function looks tricky, > and I'm not sure why existing code does like that. Ohh, I see. But the whole reference is taken just to release it in goto put_and_set because isolate_lru_page elevates reference count because other users require that. I think you do not have to mimic this behavior here and you can drop get_page and use goto set_status. -- Michal Hocko SUSE Labs
WARNING: multiple messages have this Message-ID (diff)
From: Michal Hocko <mhocko@suse.cz> To: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> Cc: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>, Mel Gorman <mel@csn.ul.ie>, Hugh Dickins <hughd@google.com>, KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>, Andi Kleen <andi@firstfloor.org>, Hillf Danton <dhillf@gmail.com>, linux-kernel@vger.kernel.org Subject: Re: [PATCH 06/10] migrate: add hugepage migration code to move_pages() Date: Tue, 26 Mar 2013 11:02:21 +0100 [thread overview] Message-ID: <20130326100221.GN2295@dhcp22.suse.cz> (raw) In-Reply-To: <1364281578-4bs50rjv-mutt-n-horiguchi@ah.jp.nec.com> On Tue 26-03-13 03:06:18, Naoya Horiguchi wrote: > On Mon, Mar 25, 2013 at 02:36:44PM +0100, Michal Hocko wrote: > > On Fri 22-03-13 16:23:51, Naoya Horiguchi wrote: [...] > > > @@ -1514,8 +1515,9 @@ struct page *follow_page_mask(struct vm_area_struct *vma, > > > if (pmd_none(*pmd)) > > > goto no_page_table; > > > if (pmd_huge(*pmd) && vma->vm_flags & VM_HUGETLB) { > > > - BUG_ON(flags & FOLL_GET); > > > page = follow_huge_pmd(mm, address, pmd, flags & FOLL_WRITE); > > > + if (flags & FOLL_GET && PageHead(page)) > > > + get_page_foll(page); > > > > Hmm, so the caller gets a non-null page without elevated ref counted > > even when he asked for it. This means that all callers have to check > > PageTail && hugetlb and put_page according to that. That is _really_ > > fragile. > > I agree. And refcounting of tail pages are already very fragile, > because get_page_foll() does something very tricky on tail pages, > where we use page->_mapcount for refcount. > This seems to be to handle some thp splitting problem, > and is never intended to be used for hugepage. yes this is THP thingy. > So I just avoid calling it for tail pages of hugepage in caller's side. > > > I think that returning NULL would make more sense in this case. > > Sounds nice. I'll do this with some comment. > > > > goto out; > > > } > > > if ((flags & FOLL_NUMA) && pmd_numa(*pmd)) > > > @@ -1164,6 +1175,12 @@ static int do_move_page_to_node_array(struct mm_struct *mm, > > [...] > > > !migrate_all) > > > goto put_and_set; > > > > > > + if (PageHuge(page)) { > > > + get_page(page); > > > + list_move_tail(&page->lru, &pagelist); > > > + goto put_and_set; > > > + } > > > > Why do you take an additional reference here? You have one from > > follow_page already. > > For normal pages, follow_page(FOLL_GET) takes a refcount and > isolate_lru_page() takes another one, so I think the same should > be done for hugepages. Refcounting of this function looks tricky, > and I'm not sure why existing code does like that. Ohh, I see. But the whole reference is taken just to release it in goto put_and_set because isolate_lru_page elevates reference count because other users require that. I think you do not have to mimic this behavior here and you can drop get_page and use goto set_status. -- 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:[~2013-03-26 10:02 UTC|newest] Thread overview: 132+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-03-22 20:23 [PATCH v2 0/10] extend hugepage migration Naoya Horiguchi 2013-03-22 20:23 ` Naoya Horiguchi 2013-03-22 20:23 ` [PATCH 01/10] migrate: add migrate_entry_wait_huge() Naoya Horiguchi 2013-03-22 20:23 ` Naoya Horiguchi 2013-03-23 15:55 ` Rik van Riel 2013-03-23 15:55 ` Rik van Riel 2013-03-25 10:13 ` Michal Hocko 2013-03-25 10:13 ` Michal Hocko 2013-03-26 4:25 ` Naoya Horiguchi 2013-03-26 4:25 ` Naoya Horiguchi 2013-04-05 20:33 ` KOSAKI Motohiro 2013-04-05 20:33 ` KOSAKI Motohiro 2013-04-08 20:00 ` Naoya Horiguchi 2013-04-08 20:00 ` Naoya Horiguchi 2013-04-05 20:33 ` KOSAKI Motohiro 2013-04-05 20:33 ` KOSAKI Motohiro 2013-03-22 20:23 ` [PATCH 02/10] migrate: make core migration code aware of hugepage Naoya Horiguchi 2013-03-22 20:23 ` Naoya Horiguchi 2013-03-25 10:57 ` Michal Hocko 2013-03-25 10:57 ` Michal Hocko 2013-03-26 4:33 ` Naoya Horiguchi 2013-03-26 4:33 ` Naoya Horiguchi 2013-03-26 8:49 ` Michal Hocko 2013-03-26 8:49 ` Michal Hocko 2013-04-05 20:41 ` KOSAKI Motohiro 2013-04-05 20:41 ` KOSAKI Motohiro 2013-03-22 20:23 ` [PATCH 03/10] soft-offline: use migrate_pages() instead of migrate_huge_page() Naoya Horiguchi 2013-03-22 20:23 ` Naoya Horiguchi 2013-03-25 12:31 ` Michal Hocko 2013-03-25 12:31 ` Michal Hocko 2013-03-26 4:34 ` Naoya Horiguchi 2013-03-26 4:34 ` Naoya Horiguchi 2013-03-26 9:49 ` Michal Hocko 2013-03-26 9:49 ` Michal Hocko 2013-03-26 20:35 ` Naoya Horiguchi 2013-03-26 20:35 ` Naoya Horiguchi 2013-03-27 13:00 ` Michal Hocko 2013-03-27 13:00 ` Michal Hocko 2013-04-05 21:11 ` KOSAKI Motohiro 2013-04-05 21:11 ` KOSAKI Motohiro 2013-03-26 11:29 ` Aneesh Kumar K.V 2013-03-26 11:29 ` Aneesh Kumar K.V 2013-03-27 13:52 ` Michal Hocko 2013-03-27 13:52 ` Michal Hocko 2013-03-27 19:19 ` Naoya Horiguchi 2013-03-27 19:19 ` Naoya Horiguchi 2013-03-28 8:53 ` Michal Hocko 2013-03-28 8:53 ` Michal Hocko 2013-03-29 5:26 ` Aneesh Kumar K.V 2013-03-29 5:26 ` Aneesh Kumar K.V 2013-03-29 9:36 ` Michal Hocko 2013-03-29 9:36 ` Michal Hocko 2013-04-01 5:13 ` Aneesh Kumar K.V 2013-04-01 5:13 ` Aneesh Kumar K.V 2013-04-02 9:45 ` Michal Hocko 2013-04-02 9:45 ` Michal Hocko 2013-03-22 20:23 ` [PATCH 04/10] migrate: clean up migrate_huge_page() Naoya Horiguchi 2013-03-22 20:23 ` Naoya Horiguchi 2013-04-05 21:13 ` KOSAKI Motohiro 2013-04-05 21:13 ` KOSAKI Motohiro 2013-03-22 20:23 ` [PATCH 05/10] migrate: add hugepage migration code to migrate_pages() Naoya Horiguchi 2013-03-22 20:23 ` Naoya Horiguchi 2013-03-25 13:04 ` Michal Hocko 2013-03-25 13:04 ` Michal Hocko 2013-03-26 5:13 ` Naoya Horiguchi 2013-03-26 5:13 ` Naoya Horiguchi 2013-03-26 8:55 ` Michal Hocko 2013-03-26 8:55 ` Michal Hocko 2013-04-05 21:17 ` KOSAKI Motohiro 2013-04-05 21:17 ` KOSAKI Motohiro 2013-04-08 20:21 ` Naoya Horiguchi 2013-04-08 20:21 ` Naoya Horiguchi 2013-03-22 20:23 ` [PATCH 06/10] migrate: add hugepage migration code to move_pages() Naoya Horiguchi 2013-03-22 20:23 ` Naoya Horiguchi 2013-03-25 13:36 ` Michal Hocko 2013-03-25 13:36 ` Michal Hocko 2013-03-26 7:06 ` Naoya Horiguchi 2013-03-26 7:06 ` Naoya Horiguchi 2013-03-26 10:02 ` Michal Hocko [this message] 2013-03-26 10:02 ` Michal Hocko 2013-03-26 20:37 ` Naoya Horiguchi 2013-03-26 20:37 ` Naoya Horiguchi 2013-03-22 20:23 ` [PATCH 07/10] mbind: add hugepage migration code to mbind() Naoya Horiguchi 2013-03-22 20:23 ` Naoya Horiguchi 2013-03-25 13:49 ` Michal Hocko 2013-03-25 13:49 ` Michal Hocko 2013-04-05 22:23 ` KOSAKI Motohiro 2013-04-05 22:23 ` KOSAKI Motohiro 2013-04-06 7:04 ` Michal Hocko 2013-04-06 7:04 ` Michal Hocko 2013-04-05 22:18 ` KOSAKI Motohiro 2013-04-05 22:18 ` KOSAKI Motohiro 2013-04-08 20:25 ` Naoya Horiguchi 2013-04-08 20:25 ` Naoya Horiguchi 2013-03-22 20:23 ` [PATCH 08/10] migrate: remove VM_HUGETLB from vma flag check in vma_migratable() Naoya Horiguchi 2013-03-22 20:23 ` Naoya Horiguchi 2013-03-22 20:23 ` [PATCH 09/10] memory-hotplug: enable memory hotplug to handle hugepage Naoya Horiguchi 2013-03-22 20:23 ` Naoya Horiguchi 2013-03-25 15:09 ` Michal Hocko 2013-03-25 15:09 ` Michal Hocko 2013-03-26 18:23 ` Naoya Horiguchi 2013-03-26 18:23 ` Naoya Horiguchi 2013-03-27 14:19 ` Michal Hocko 2013-03-27 14:19 ` Michal Hocko 2013-03-27 21:29 ` Naoya Horiguchi 2013-03-27 21:29 ` Naoya Horiguchi 2013-03-27 21:58 ` Naoya Horiguchi 2013-03-27 21:58 ` Naoya Horiguchi 2013-03-27 22:55 ` Michal Hocko 2013-03-27 22:55 ` Michal Hocko 2013-03-26 12:01 ` Aneesh Kumar K.V 2013-03-26 12:01 ` Aneesh Kumar K.V 2013-03-27 19:28 ` Naoya Horiguchi 2013-03-27 19:28 ` Naoya Horiguchi 2013-04-06 0:13 ` KOSAKI Motohiro 2013-04-06 0:13 ` KOSAKI Motohiro 2013-04-09 20:07 ` Naoya Horiguchi 2013-04-09 20:07 ` Naoya Horiguchi 2013-04-09 21:27 ` KOSAKI Motohiro 2013-04-09 21:27 ` KOSAKI Motohiro 2013-04-09 22:43 ` Naoya Horiguchi 2013-04-09 22:43 ` Naoya Horiguchi 2013-04-10 1:56 ` KOSAKI Motohiro 2013-04-10 1:56 ` KOSAKI Motohiro 2013-04-10 2:24 ` Naoya Horiguchi 2013-04-10 2:24 ` Naoya Horiguchi 2013-03-22 20:23 ` [PATCH 10/10] prepare to remove /proc/sys/vm/hugepages_treat_as_movable Naoya Horiguchi 2013-03-22 20:23 ` Naoya Horiguchi 2013-03-25 15:12 ` Michal Hocko 2013-03-25 15:12 ` Michal Hocko 2013-04-06 0:15 ` KOSAKI Motohiro 2013-04-06 0:15 ` KOSAKI Motohiro
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=20130326100221.GN2295@dhcp22.suse.cz \ --to=mhocko@suse.cz \ --cc=akpm@linux-foundation.org \ --cc=andi@firstfloor.org \ --cc=dhillf@gmail.com \ --cc=hughd@google.com \ --cc=kosaki.motohiro@jp.fujitsu.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mel@csn.ul.ie \ --cc=n-horiguchi@ah.jp.nec.com \ /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.