From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750916AbcDKE3H (ORCPT ); Mon, 11 Apr 2016 00:29:07 -0400 Received: from LGEAMRELO12.lge.com ([156.147.23.52]:45191 "EHLO lgeamrelo12.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750709AbcDKE3F (ORCPT ); Mon, 11 Apr 2016 00:29:05 -0400 X-Original-SENDERIP: 156.147.1.127 X-Original-MAILFROM: minchan@kernel.org X-Original-SENDERIP: 165.244.98.150 X-Original-MAILFROM: minchan@kernel.org X-Original-SENDERIP: 10.177.223.161 X-Original-MAILFROM: minchan@kernel.org Date: Mon, 11 Apr 2016 13:29:33 +0900 From: Minchan Kim To: Vlastimil Babka CC: Andrew Morton , , , , , Joonsoo Kim , , , , Mel Gorman , Hugh Dickins , Sergey Senozhatsky , Rik van Riel , , Gioh Kim , Sangseok Lee , Chan Gyun Jeong , Al Viro , YiPing Xu , Gioh Kim Subject: Re: [PATCH v3 04/16] mm/balloon: use general movable page feature into balloon Message-ID: <20160411042933.GB4804@bbox> References: <1459321935-3655-1-git-send-email-minchan@kernel.org> <1459321935-3655-5-git-send-email-minchan@kernel.org> <5703A979.402@suse.cz> MIME-Version: 1.0 In-Reply-To: <5703A979.402@suse.cz> User-Agent: Mutt/1.5.21 (2010-09-15) X-MIMETrack: Itemize by SMTP Server on LGEKRMHUB04/LGE/LG Group(Release 8.5.3FP6|November 21, 2013) at 2016/04/11 13:29:01, Serialize by Router on LGEKRMHUB04/LGE/LG Group(Release 8.5.3FP6|November 21, 2013) at 2016/04/11 13:29:01, Serialize complete at 2016/04/11 13:29:01 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >Cc: Konstantin Khlebnikov > >Signed-off-by: Gioh Kim > >Signed-off-by: Minchan Kim > > 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!