From: Minchan Kim <minchan@kernel.org>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>,
<linux-kernel@vger.kernel.org>, <linux-mm@kvack.org>,
<jlayton@poochiereds.net>, <bfields@fieldses.org>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>, <koct9i@gmail.com>,
<aquini@redhat.com>, <virtualization@lists.linux-foundation.org>,
Mel Gorman <mgorman@suse.de>, Hugh Dickins <hughd@google.com>,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
Rik van Riel <riel@redhat.com>, <rknize@motorola.com>,
Gioh Kim <gi-oh.kim@profitbricks.com>,
Sangseok Lee <sangseok.lee@lge.com>,
Chan Gyun Jeong <chan.jeong@lge.com>,
Al Viro <viro@ZenIV.linux.org.uk>,
YiPing Xu <xuyiping@hisilicon.com>,
Gioh Kim <gurugio@hanmail.net>
Subject: Re: [PATCH v3 04/16] mm/balloon: use general movable page feature into balloon
Date: Mon, 11 Apr 2016 13:29:33 +0900 [thread overview]
Message-ID: <20160411042933.GB4804@bbox> (raw)
In-Reply-To: <5703A979.402@suse.cz>
On Tue, Apr 05, 2016 at 02:03:05PM +0200, Vlastimil Babka wrote:
> On 03/30/2016 09:12 AM, Minchan Kim wrote:
> >Now, VM has a feature to migrate non-lru movable pages so
> >balloon doesn't need custom migration hooks in migrate.c
> >and compact.c. Instead, this patch implements page->mapping
> >->{isolate|migrate|putback} functions.
> >
> >With that, we could remove hooks for ballooning in general
> >migration functions and make balloon compaction simple.
> >
> >Cc: virtualization@lists.linux-foundation.org
> >Cc: Rafael Aquini <aquini@redhat.com>
> >Cc: Konstantin Khlebnikov <koct9i@gmail.com>
> >Signed-off-by: Gioh Kim <gurugio@hanmail.net>
> >Signed-off-by: Minchan Kim <minchan@kernel.org>
>
> I'm not familiar with the inode and pseudofs stuff, so just some
> things I noticed:
>
> >-#define PAGE_MOVABLE_MAPCOUNT_VALUE (-255)
> >+#define PAGE_MOVABLE_MAPCOUNT_VALUE (-256)
> >+#define PAGE_BALLOON_MAPCOUNT_VALUE PAGE_MOVABLE_MAPCOUNT_VALUE
> >
> > static inline int PageMovable(struct page *page)
> > {
> >- return ((test_bit(PG_movable, &(page)->flags) &&
> >- atomic_read(&page->_mapcount) == PAGE_MOVABLE_MAPCOUNT_VALUE)
> >- || PageBalloon(page));
> >+ return (test_bit(PG_movable, &(page)->flags) &&
> >+ atomic_read(&page->_mapcount) == PAGE_MOVABLE_MAPCOUNT_VALUE);
> > }
> >
> > /* Caller should hold a PG_lock */
> >@@ -645,6 +626,35 @@ static inline void __ClearPageMovable(struct page *page)
> >
> > PAGEFLAG(Isolated, isolated, PF_ANY);
> >
> >+static inline int PageBalloon(struct page *page)
> >+{
> >+ return atomic_read(&page->_mapcount) == PAGE_BALLOON_MAPCOUNT_VALUE
> >+ && PagePrivate2(page);
> >+}
>
> Hmm so you are now using PG_private_2 flag here, but it's not
> documented. Also the only caller of PageBalloon() seems to be
> stable_page_flags(). Which will now report all movable pages with
> PG_private_2 as KPF_BALOON. Seems like an overkill and also not
> reliable. Could it test e.g. page->mapping instead?
Thanks for pointing out.
I will not use page->_mapcount in next version so it should be okay.
>
> Or maybe if we manage to get rid of PAGE_MOVABLE_MAPCOUNT_VALUE, we
> can keep PAGE_BALLOON_MAPCOUNT_VALUE to simply distinguish balloon
> pages for stable_page_flags().
Yeb.
>
> >@@ -1033,7 +1019,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
> > out:
> > /* If migration is successful, move newpage to right list */
> > if (rc == MIGRATEPAGE_SUCCESS) {
> >- if (unlikely(__is_movable_balloon_page(newpage)))
> >+ if (unlikely(PageMovable(newpage)))
> > put_page(newpage);
> > else
> > putback_lru_page(newpage);
>
> Hmm shouldn't the condition have been changed to
>
> if (unlikely(__is_movable_balloon_page(newpage)) || PageMovable(newpage)
>
> by patch 02/16? And this patch should be just removing the
> balloon-specific check? Otherwise it seems like between patches 02
> and 04, other kinds of PageMovable pages were unnecessarily/wrongly
> routed through putback_lru_page()?
Fixed.
>
> >diff --git a/mm/vmscan.c b/mm/vmscan.c
> >index d82196244340..c7696a2e11c7 100644
> >--- a/mm/vmscan.c
> >+++ b/mm/vmscan.c
> >@@ -1254,7 +1254,7 @@ unsigned long reclaim_clean_pages_from_list(struct zone *zone,
> >
> > list_for_each_entry_safe(page, next, page_list, lru) {
> > if (page_is_file_cache(page) && !PageDirty(page) &&
> >- !isolated_balloon_page(page)) {
> >+ !PageIsolated(page)) {
> > ClearPageActive(page);
> > list_move(&page->lru, &clean_pages);
> > }
>
> This looks like the same comment as above at first glance. But
> looking closer, it's even weirder. isolated_balloon_page() was
> simply PageBalloon() after d6d86c0a7f8dd... weird already. You
> replace it with check for !PageIsolated() which looks like a more
> correct check, so ok. Except the potential false positive with
> PG_owner_priv_1.
I will change it next version so it shouldn't be a problem.
Thanks for the review!
next prev parent reply other threads:[~2016-04-11 4:29 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-30 7:11 [PATCH v3 00/16] Support non-lru page migration Minchan Kim
2016-03-30 7:12 ` [PATCH v3 01/16] mm: use put_page to free page instead of putback_lru_page Minchan Kim
2016-04-01 12:58 ` Vlastimil Babka
2016-04-04 1:39 ` Minchan Kim
2016-04-04 4:45 ` Naoya Horiguchi
2016-04-04 14:46 ` Vlastimil Babka
2016-04-05 1:54 ` Naoya Horiguchi
2016-04-05 8:20 ` Vlastimil Babka
2016-04-06 0:54 ` Naoya Horiguchi
2016-04-06 7:57 ` Vlastimil Babka
2016-04-04 5:53 ` Balbir Singh
2016-04-04 6:01 ` Minchan Kim
2016-04-05 3:10 ` Balbir Singh
2016-03-30 7:12 ` [PATCH v3 02/16] mm/compaction: support non-lru movable page migration Minchan Kim
2016-04-01 21:29 ` Vlastimil Babka
2016-04-04 5:12 ` Minchan Kim
2016-04-04 13:24 ` Vlastimil Babka
2016-04-07 2:35 ` Minchan Kim
2016-04-12 8:00 ` Chulmin Kim
2016-04-12 14:25 ` Minchan Kim
2016-03-30 7:12 ` [PATCH v3 03/16] mm: add non-lru movable page support document Minchan Kim
2016-04-01 14:38 ` Vlastimil Babka
2016-04-04 2:25 ` Minchan Kim
2016-04-04 13:09 ` Vlastimil Babka
2016-04-07 2:27 ` Minchan Kim
2016-03-30 7:12 ` [PATCH v3 04/16] mm/balloon: use general movable page feature into balloon Minchan Kim
2016-04-05 12:03 ` Vlastimil Babka
2016-04-11 4:29 ` Minchan Kim [this message]
2016-03-30 7:12 ` [PATCH v3 05/16] zsmalloc: keep max_object in size_class Minchan Kim
2016-04-17 15:08 ` Sergey Senozhatsky
2016-03-30 7:12 ` [PATCH v3 06/16] zsmalloc: squeeze inuse into page->mapping Minchan Kim
2016-04-17 15:08 ` Sergey Senozhatsky
2016-04-19 7:40 ` Minchan Kim
2016-03-30 7:12 ` [PATCH v3 07/16] zsmalloc: remove page_mapcount_reset Minchan Kim
2016-04-17 15:11 ` Sergey Senozhatsky
2016-03-30 7:12 ` [PATCH v3 08/16] zsmalloc: squeeze freelist into page->mapping Minchan Kim
2016-04-17 15:56 ` Sergey Senozhatsky
2016-04-19 7:42 ` Minchan Kim
2016-03-30 7:12 ` [PATCH v3 09/16] zsmalloc: move struct zs_meta from mapping to freelist Minchan Kim
2016-04-17 15:22 ` Sergey Senozhatsky
2016-03-30 7:12 ` [PATCH v3 10/16] zsmalloc: factor page chain functionality out Minchan Kim
2016-04-18 0:33 ` Sergey Senozhatsky
2016-04-19 7:46 ` Minchan Kim
2016-03-30 7:12 ` [PATCH v3 11/16] zsmalloc: separate free_zspage from putback_zspage Minchan Kim
2016-04-18 1:04 ` Sergey Senozhatsky
2016-04-19 7:51 ` Minchan Kim
2016-04-19 7:53 ` Sergey Senozhatsky
2016-03-30 7:12 ` [PATCH v3 12/16] zsmalloc: zs_compact refactoring Minchan Kim
2016-04-04 8:04 ` Chulmin Kim
2016-04-04 9:01 ` Minchan Kim
2016-03-30 7:12 ` [PATCH v3 13/16] zsmalloc: migrate head page of zspage Minchan Kim
2016-04-06 13:01 ` Chulmin Kim
2016-04-07 0:34 ` Chulmin Kim
2016-04-07 0:43 ` Minchan Kim
2016-04-19 6:08 ` Chulmin Kim
2016-04-19 6:15 ` Minchan Kim
2016-03-30 7:12 ` [PATCH v3 14/16] zsmalloc: use single linked list for page chain Minchan Kim
2016-03-30 7:12 ` [PATCH v3 15/16] zsmalloc: migrate tail pages in zspage Minchan Kim
2016-03-30 7:12 ` [PATCH v3 16/16] zram: use __GFP_MOVABLE for memory allocation Minchan Kim
2016-03-30 23:11 ` [PATCH v3 00/16] Support non-lru page migration Andrew Morton
2016-03-31 0:29 ` Sergey Senozhatsky
2016-03-31 0:57 ` Minchan Kim
2016-03-31 0:57 ` Minchan Kim
2016-04-04 13:17 ` John Einar Reitan
2016-04-11 4:35 ` Minchan Kim
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=20160411042933.GB4804@bbox \
--to=minchan@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=aquini@redhat.com \
--cc=bfields@fieldses.org \
--cc=chan.jeong@lge.com \
--cc=gi-oh.kim@profitbricks.com \
--cc=gurugio@hanmail.net \
--cc=hughd@google.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=jlayton@poochiereds.net \
--cc=koct9i@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@suse.de \
--cc=riel@redhat.com \
--cc=rknize@motorola.com \
--cc=sangseok.lee@lge.com \
--cc=sergey.senozhatsky@gmail.com \
--cc=vbabka@suse.cz \
--cc=viro@ZenIV.linux.org.uk \
--cc=virtualization@lists.linux-foundation.org \
--cc=xuyiping@hisilicon.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).