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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
