From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933145AbdERQmQ (ORCPT ); Thu, 18 May 2017 12:42:16 -0400 Received: from mx2.suse.de ([195.135.220.15]:38593 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932939AbdERQmP (ORCPT ); Thu, 18 May 2017 12:42:15 -0400 Date: Thu, 18 May 2017 18:42:10 +0200 From: Michal Hocko To: Vlastimil Babka Cc: Andrew Morton , linux-mm@kvack.org, Mel Gorman , Andrea Arcangeli , Jerome Glisse , Reza Arbab , Yasuaki Ishimatsu , qiuxishi@huawei.com, Kani Toshimitsu , slaoub@gmail.com, Joonsoo Kim , Andi Kleen , David Rientjes , Daniel Kiper , Igor Mammedov , Vitaly Kuznetsov , LKML Subject: Re: [PATCH 07/14] mm: consider zone which is not fully populated to have holes Message-ID: <20170518164210.GD18333@dhcp22.suse.cz> References: <20170515085827.16474-1-mhocko@kernel.org> <20170515085827.16474-8-mhocko@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 On Thu 18-05-17 18:14:39, Vlastimil Babka wrote: > On 05/15/2017 10:58 AM, Michal Hocko wrote: [...] > > #ifdef CONFIG_MEMORY_HOTPLUG > > +/* > > + * Return page for the valid pfn only if the page is online. All pfn > > + * walkers which rely on the fully initialized page->flags and others > > + * should use this rather than pfn_valid && pfn_to_page > > + */ > > +#define pfn_to_online_page(pfn) \ > > +({ \ > > + struct page *___page = NULL; \ > > + \ > > + if (online_section_nr(pfn_to_section_nr(pfn))) \ > > + ___page = pfn_to_page(pfn); \ > > + ___page; \ > > +}) > > This seems to be already assuming pfn_valid() to be true. There's no > "pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS" check and the comment > suggests as such, but... Yes, we should check the validity of the section number. We do not have to check whether the section is valid because online sections are a subset of those that are valid. > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > > index 05796ee974f7..c3a146028ba6 100644 > > --- a/mm/memory_hotplug.c > > +++ b/mm/memory_hotplug.c > > @@ -929,6 +929,9 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages, > > unsigned long i; > > unsigned long onlined_pages = *(unsigned long *)arg; > > struct page *page; > > + > > + online_mem_sections(start_pfn, start_pfn + nr_pages); > > Shouldn't this be moved *below* the loop that initializes struct pages? > In the offline case you do mark sections offline before "tearing" struct > pages, so that should be symmetric. You are right! Andrew, could you fold the following intot the patch? --- >>From 0550b61203d6970b47fd79f5e6372dccd143cbec Mon Sep 17 00:00:00 2001 From: Michal Hocko Date: Thu, 18 May 2017 18:38:24 +0200 Subject: [PATCH] fold me "mm: consider zone which is not fully populated to have holes" - check valid section number in pfn_to_online_page - Vlastimil - mark sections online after all struct pages are initialized in online_pages_range - Vlastimil Signed-off-by: Michal Hocko --- include/linux/memory_hotplug.h | 3 ++- mm/memory_hotplug.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h index aec88657ec49..9e0249d0f5e4 100644 --- a/include/linux/memory_hotplug.h +++ b/include/linux/memory_hotplug.h @@ -22,8 +22,9 @@ struct resource; #define pfn_to_online_page(pfn) \ ({ \ struct page *___page = NULL; \ + unsigned long ___nr = pfn_to_section_nr(pfn); \ \ - if (online_section_nr(pfn_to_section_nr(pfn))) \ + if (___nr < NR_MEM_SECTIONS && online_section_nr(___nr))\ ___page = pfn_to_page(pfn); \ ___page; \ }) diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index de30822642c6..a31b547c11f0 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -746,14 +746,15 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages, unsigned long onlined_pages = *(unsigned long *)arg; struct page *page; - online_mem_sections(start_pfn, start_pfn + nr_pages); - if (PageReserved(pfn_to_page(start_pfn))) for (i = 0; i < nr_pages; i++) { page = pfn_to_page(start_pfn + i); (*online_page_callback)(page); onlined_pages++; } + + online_mem_sections(start_pfn, start_pfn + nr_pages); + *(unsigned long *)arg = onlined_pages; return 0; } -- 2.11.0 -- Michal Hocko SUSE Labs From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f197.google.com (mail-wr0-f197.google.com [209.85.128.197]) by kanga.kvack.org (Postfix) with ESMTP id A0C98831F4 for ; Thu, 18 May 2017 12:42:15 -0400 (EDT) Received: by mail-wr0-f197.google.com with SMTP id u96so10369024wrc.7 for ; Thu, 18 May 2017 09:42:15 -0700 (PDT) Received: from mx1.suse.de (mx2.suse.de. [195.135.220.15]) by mx.google.com with ESMTPS id 94si5982968edp.312.2017.05.18.09.42.13 for (version=TLS1 cipher=AES128-SHA bits=128/128); Thu, 18 May 2017 09:42:14 -0700 (PDT) Date: Thu, 18 May 2017 18:42:10 +0200 From: Michal Hocko Subject: Re: [PATCH 07/14] mm: consider zone which is not fully populated to have holes Message-ID: <20170518164210.GD18333@dhcp22.suse.cz> References: <20170515085827.16474-1-mhocko@kernel.org> <20170515085827.16474-8-mhocko@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: owner-linux-mm@kvack.org List-ID: To: Vlastimil Babka Cc: Andrew Morton , linux-mm@kvack.org, Mel Gorman , Andrea Arcangeli , Jerome Glisse , Reza Arbab , Yasuaki Ishimatsu , qiuxishi@huawei.com, Kani Toshimitsu , slaoub@gmail.com, Joonsoo Kim , Andi Kleen , David Rientjes , Daniel Kiper , Igor Mammedov , Vitaly Kuznetsov , LKML On Thu 18-05-17 18:14:39, Vlastimil Babka wrote: > On 05/15/2017 10:58 AM, Michal Hocko wrote: [...] > > #ifdef CONFIG_MEMORY_HOTPLUG > > +/* > > + * Return page for the valid pfn only if the page is online. All pfn > > + * walkers which rely on the fully initialized page->flags and others > > + * should use this rather than pfn_valid && pfn_to_page > > + */ > > +#define pfn_to_online_page(pfn) \ > > +({ \ > > + struct page *___page = NULL; \ > > + \ > > + if (online_section_nr(pfn_to_section_nr(pfn))) \ > > + ___page = pfn_to_page(pfn); \ > > + ___page; \ > > +}) > > This seems to be already assuming pfn_valid() to be true. There's no > "pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS" check and the comment > suggests as such, but... Yes, we should check the validity of the section number. We do not have to check whether the section is valid because online sections are a subset of those that are valid. > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > > index 05796ee974f7..c3a146028ba6 100644 > > --- a/mm/memory_hotplug.c > > +++ b/mm/memory_hotplug.c > > @@ -929,6 +929,9 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages, > > unsigned long i; > > unsigned long onlined_pages = *(unsigned long *)arg; > > struct page *page; > > + > > + online_mem_sections(start_pfn, start_pfn + nr_pages); > > Shouldn't this be moved *below* the loop that initializes struct pages? > In the offline case you do mark sections offline before "tearing" struct > pages, so that should be symmetric. You are right! Andrew, could you fold the following intot the patch? ---