linux-kernel - Re: [PATCH 4/5] mm: simplify reclaim path for MADV

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20151027070903.GD26803@bbox>
Date:	Tue, 27 Oct 2015 16:09:03 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	yalin wang <yalin.wang2010@...il.com>
Cc:	Hugh Dickins <hughd@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"open list:MEMORY MANAGEMENT" <linux-mm@...ck.org>,
	lkml <linux-kernel@...r.kernel.org>,
	Rik van Riel <riel@...hat.com>, Mel Gorman <mgorman@...e.de>,
	Michal Hocko <mhocko@...e.cz>,
	Johannes Weiner <hannes@...xchg.org>,
	"Kirill A. Shutemov" <kirill@...temov.name>,
	Vlastimil Babka <vbabka@...e.cz>
Subject: Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE

Hello Yalin,

Sorry for missing you in Cc list.
IIRC, mails to send your previous mail address(Yalin.Wang@...ymobile.com)
were returned.

On Tue, Oct 27, 2015 at 11:44:09AM +0800, yalin wang wrote:
> 
> > On Oct 27, 2015, at 10:09, Hugh Dickins <hughd@...gle.com> wrote:
> > 
> > On Mon, 19 Oct 2015, Minchan Kim wrote:
> > 
> >> I made reclaim path mess to check and free MADV_FREEed page.
> >> This patch simplify it with tweaking add_to_swap.
> >> 
> >> So far, we mark page as PG_dirty when we add the page into
> >> swap cache(ie, add_to_swap) to page out to swap device but
> >> this patch moves PG_dirty marking under try_to_unmap_one
> >> when we decide to change pte from anon to swapent so if
> >> any process's pte has swapent for the page, the page must
> >> be swapped out. IOW, there should be no funcional behavior
> >> change. It makes relcaim path really simple for MADV_FREE
> >> because we just need to check PG_dirty of page to decide
> >> discarding the page or not.
> >> 
> >> Other thing this patch does is to pass TTU_BATCH_FLUSH to
> >> try_to_unmap when we handle freeable page because I don't
> >> see any reason to prevent it.
> >> 
> >> Cc: Hugh Dickins <hughd@...gle.com>
> >> Cc: Mel Gorman <mgorman@...e.de>
> >> Signed-off-by: Minchan Kim <minchan@...nel.org>
> > 
> > Acked-by: Hugh Dickins <hughd@...gle.com>
> > 
> > This is sooooooo much nicer than the code it replaces!  Really good.
> > Kudos also to Hannes for suggesting this approach originally, I think.
> > 
> > I hope this implementation satisfies a good proportion of the people
> > who have been wanting MADV_FREE: I'm not among them, and have long
> > lost touch with those discussions, so won't judge how usable it is.
> > 
> > I assume you'll refactor the series again before it goes to Linus,
> > so the previous messier implementations vanish?  I notice Andrew
> > has this "mm: simplify reclaim path for MADV_FREE" in mmotm as
> > mm-dont-split-thp-page-when-syscall-is-called-fix-6.patch:
> > I guess it all got much too messy to divide up in a hurry.
> > 
> > I've noticed no problems in testing (unlike the first time you moved
> > to working with pte_dirty); though of course I've not been using
> > MADV_FREE itself at all.
> > 
> > One aspect has worried me for a while, but I think I've reached the
> > conclusion that it doesn't matter at all.  The swap that's allocated
> > in add_to_swap() would normally get freed again (after try_to_unmap
> > found it was a MADV_FREE !pte_dirty !PageDirty case) at the bottom
> > of shrink_page_list(), in __remove_mapping(), yes?
> > 
> > The bit that worried me is that on rare occasions, something unknown
> > might take a speculative reference to the page, and __remove_mapping()
> > fail to freeze refs for that reason.  Much too rare to worry over not
> > freeing that page immediately, but it leaves us with a PageUptodate
> > PageSwapCache !PageDirty page, yet its contents are not the contents
> > of that location on swap.
> > 
> > But since this can only happen when you have *not* inserted the
> > corresponding swapent anywhere, I cannot think of anything that would
> > have a legitimate interest in its contents matching that location on swap.
> > So I don't think it's worth looking for somewhere to add a SetPageDirty
> > (or a delete_from_swap_cache) just to regularize that case.
> > 
> >> ---
> >> include/linux/rmap.h |  6 +----
> >> mm/huge_memory.c     |  5 ----
> >> mm/rmap.c            | 42 ++++++----------------------------
> >> mm/swap_state.c      |  5 ++--
> >> mm/vmscan.c          | 64 ++++++++++++++++------------------------------------
> >> 5 files changed, 30 insertions(+), 92 deletions(-)
> >> 

<snip>

You added comment bottom line so I'm not sure what PageDirty you meant.

> it is wrong here if you only check PageDirty() to decide if the page is freezable or not .
> The Anon page are shared by multiple process, _mapcount > 1 ,
> so you must check all pt_dirty bit during page_referenced() function,
> see this mail thread:
> http://ns1.ske-art.com/lists/kernel/msg1934021.html

If one of pte among process sharing the page was dirty, the dirtiness should
be propagated from pte to PG_dirty by try_to_unmap_one.
IOW, if the page doesn't have PG_dirty flag, it means all of process did
MADV_FREE.

Am I missing something from you question?
If so, could you show exact scenario I am missing?

Thanks for the interest.


> Thanks
> 
> 
> 
> 
> 
> 
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/