Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1035970AbdDUHQY (ORCPT ); Fri, 21 Apr 2017 03:16:24 -0400 Received: from mx2.suse.de ([195.135.220.15]:53138 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1035691AbdDUHQW (ORCPT ); Fri, 21 Apr 2017 03:16:22 -0400 Date: Fri, 21 Apr 2017 09:16:16 +0200 From: Michal Hocko To: Joonsoo Kim Cc: linux-mm@kvack.org, Andrew Morton , Mel Gorman , Vlastimil Babka , Andrea Arcangeli , Jerome Glisse , Reza Arbab , Yasuaki Ishimatsu , qiuxishi@huawei.com, Kani Toshimitsu , slaoub@gmail.com, Andi Kleen , David Rientjes , Daniel Kiper , Igor Mammedov , Vitaly Kuznetsov , LKML Subject: Re: your mail Message-ID: <20170421071616.GC14154@dhcp22.suse.cz> References: <20170410110351.12215-1-mhocko@kernel.org> <20170415121734.6692-1-mhocko@kernel.org> <20170417054718.GD1351@js1304-desktop> <20170417081513.GA12511@dhcp22.suse.cz> <20170420012753.GA22054@js1304-desktop> <20170420072820.GB15781@dhcp22.suse.cz> <20170421043826.GC13966@js1304-desktop> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170421043826.GC13966@js1304-desktop> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3585 Lines: 82 On Fri 21-04-17 13:38:28, Joonsoo Kim wrote: > On Thu, Apr 20, 2017 at 09:28:20AM +0200, Michal Hocko wrote: > > On Thu 20-04-17 10:27:55, Joonsoo Kim wrote: > > > On Mon, Apr 17, 2017 at 10:15:15AM +0200, Michal Hocko wrote: > > [...] > > > > Which pfn walkers you have in mind? > > > > > > For example, kpagecount_read() in fs/proc/page.c. I searched it by > > > using pfn_valid(). > > > > Yeah, I've checked that one and in fact this is a good example of the > > case where you do not really care about holes. It just checks the page > > count which is a valid information under any circumstances. > > I don't think so. First, it checks the page *map* count. Is it still valid > even if PageReserved() is set? I do not know about any user which would manipulate page map count for referenced pages. The core MM code doesn't. > What I'd like to ask in this example is > that what information is valid if PageReserved() is set. Is there any > design document on this? I think that we need to define/document it first. NO, it is not AFAIK. [...] > > OK, fair enough. I did't consider memblock allocations. I will rethink > > this patch but there are essentially 3 options > > - use a different criterion for the offline holes dection. I > > have just realized we might do it by storing the online > > information into the mem sections > > - drop this patch > > - move the PageReferenced check down the chain into > > isolate_freepages_block resp. isolate_migratepages_block > > > > I would prefer 3 over 2 over 1. I definitely want to make this more > > robust so 1 is preferable long term but I do not want this to be a > > roadblock to the rest of the rework. Does that sound acceptable to you? > > I like #1 among of above options and I already see your patch for #1. > It's much better than your first attempt but I'm still not happy due > to the semantic of pfn_valid(). You are trying to change a semantic of something that has a well defined meaning. I disagree that we should change it. It might sound like a simpler thing to do because pfn walkers will have to be checked but what you are proposing is conflating two different things together. > > [..] > > > Let me clarify my desire(?) for this issue. > > > > > > 1. If pfn_valid() returns true, struct page has valid information, at > > > least, in flags (zone id, node id, flags, etc...). So, we can use them > > > without checking PageResereved(). > > > > This is no longer true after my rework. Pages are associated with the > > zone during _onlining_ rather than when they are physically hotpluged. > > If your rework make information valid during _onlining_, my > suggestion is making pfn_valid() return false until onlining. > > Caller of pfn_valid() expects that they can get valid information from > the struct page. There is no reason to access the struct page if they > can't get valid information from it. So, passing pfn_valid() should > guarantee that, at least, some kind of information is valid. > > If pfn_valid() doesn't guarantee it, most of the pfn walker should > check PageResereved() to make sure that validity of information from > the struct page. This is true only for those walkers which really depend on the full initialization. This is not the case for all of them. I do not see any reason to introduce another _pfn_valid to just check whether there is a struct page... So please do not conflate those two different concepts together. I believe that the most prominent pfn walkers should be covered now and others can be evaluated later. -- Michal Hocko SUSE Labs