From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755821AbcEaHvx (ORCPT ); Tue, 31 May 2016 03:51:53 -0400 Received: from mx2.suse.de ([195.135.220.15]:59023 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755696AbcEaHvw (ORCPT ); Tue, 31 May 2016 03:51:52 -0400 Subject: Re: PATCH v6v2 02/12] mm: migrate: support non-lru movable page migration To: Minchan Kim References: <1463754225-31311-1-git-send-email-minchan@kernel.org> <1463754225-31311-3-git-send-email-minchan@kernel.org> <20160530013926.GB8683@bbox> <2bc277c4-4257-c6cb-2e37-ee5de985410b@suse.cz> <20160530162523.GA18314@bbox> Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Rik van Riel , Joonsoo Kim , Mel Gorman , Hugh Dickins , Rafael Aquini , virtualization@lists.linux-foundation.org, Jonathan Corbet , John Einar Reitan , dri-devel@lists.freedesktop.org, Sergey Senozhatsky , Gioh Kim From: Vlastimil Babka Message-ID: Date: Tue, 31 May 2016 09:51:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <20160530162523.GA18314@bbox> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/30/2016 06:25 PM, Minchan Kim wrote: >>> --- a/mm/compaction.c >>> +++ b/mm/compaction.c >>> @@ -81,6 +81,39 @@ static inline bool migrate_async_suitable(int migratetype) >>> >>> #ifdef CONFIG_COMPACTION >>> >>> +int PageMovable(struct page *page) >>> +{ >>> + struct address_space *mapping; >>> + >>> + VM_BUG_ON_PAGE(!PageLocked(page), page); >>> + if (!__PageMovable(page)) >>> + return 0; >>> + >>> + mapping = page_mapping(page); >>> + if (mapping && mapping->a_ops && mapping->a_ops->isolate_page) >>> + return 1; >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(PageMovable); >>> + >>> +void __SetPageMovable(struct page *page, struct address_space *mapping) >>> +{ >>> + VM_BUG_ON_PAGE(!PageLocked(page), page); >>> + VM_BUG_ON_PAGE((unsigned long)mapping & PAGE_MAPPING_MOVABLE, page); >>> + page->mapping = (void *)((unsigned long)mapping | PAGE_MAPPING_MOVABLE); >>> +} >>> +EXPORT_SYMBOL(__SetPageMovable); >>> + >>> +void __ClearPageMovable(struct page *page) >>> +{ >>> + VM_BUG_ON_PAGE(!PageLocked(page), page); >>> + VM_BUG_ON_PAGE(!PageMovable(page), page); >>> + page->mapping = (void *)((unsigned long)page->mapping & >>> + PAGE_MAPPING_MOVABLE); >>> +} >>> +EXPORT_SYMBOL(__ClearPageMovable); >> >> The second confusing thing is that the function is named >> __ClearPageMovable(), but what it really clears is the mapping >> pointer, >> which is not at all the opposite of what __SetPageMovable() does. >> >> I know it's explained in the documentation, but it also deserves a >> comment here so it doesn't confuse everyone who looks at it. >> Even better would be a less confusing name for the function, but I >> can't offer one right now. > > To me, __ClearPageMovable naming is suitable for user POV. > It effectively makes the page unmovable. The confusion is just caused > by the implementation and I don't prefer exported API depends on the > implementation. So I want to add just comment. > > I didn't add comment above the function because I don't want to export > internal implementation to the user. I think they don't need to know it. > > index a7df2ae71f2a..d1d2063b4fd9 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -108,6 +108,11 @@ void __ClearPageMovable(struct page *page) > { > VM_BUG_ON_PAGE(!PageLocked(page), page); > VM_BUG_ON_PAGE(!PageMovable(page), page); > + /* > + * Clear registered address_space val with keeping PAGE_MAPPING_MOVABLE > + * flag so that VM can catch up released page by driver after isolation. > + * With it, VM migration doesn't try to put it back. > + */ > page->mapping = (void *)((unsigned long)page->mapping & > PAGE_MAPPING_MOVABLE); OK, that's fine! >> >>> diff --git a/mm/util.c b/mm/util.c >>> index 917e0e3d0f8e..b756ee36f7f0 100644 >>> --- a/mm/util.c >>> +++ b/mm/util.c >>> @@ -399,10 +399,12 @@ struct address_space *page_mapping(struct page *page) >>> } >>> >>> mapping = page->mapping; >> >> I'd probably use READ_ONCE() here to be safe. Not all callers are >> under page lock? > > I don't understand. Yeah, all caller are not under page lock but at least, > new user of movable pages should call it under page_lock. > Yeah, I will write the rule down in document. > In this case, what kinds of problem do you see? After more thinking, probably none. It wouldn't prevent any extra races. Sorry for the noise.