From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932454AbdDRIqq (ORCPT ); Tue, 18 Apr 2017 04:46:46 -0400 Received: from mx2.suse.de ([195.135.220.15]:47973 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932095AbdDRIp0 (ORCPT ); Tue, 18 Apr 2017 04:45:26 -0400 Subject: Re: [PATCH 1/3] mm: consider zone which is not fully populated to have holes To: Michal Hocko , linux-mm@kvack.org References: <20170410110351.12215-1-mhocko@kernel.org> <20170415121734.6692-1-mhocko@kernel.org> <20170415121734.6692-2-mhocko@kernel.org> Cc: Andrew Morton , 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 , Michal Hocko From: Vlastimil Babka Message-ID: <97a658cd-e656-6efa-7725-150063d276f1@suse.cz> Date: Tue, 18 Apr 2017 10:45:23 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170415121734.6692-2-mhocko@kernel.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/15/2017 02:17 PM, Michal Hocko wrote: > From: Michal Hocko > > __pageblock_pfn_to_page has two users currently, set_zone_contiguous > which checks whether the given zone contains holes and > pageblock_pfn_to_page which then carefully returns a first valid > page from the given pfn range for the given zone. This doesn't handle > zones which are not fully populated though. Memory pageblocks can be > offlined or might not have been onlined yet. In such a case the zone > should be considered to have holes otherwise pfn walkers can touch > and play with offline pages. > > Current callers of pageblock_pfn_to_page in compaction seem to work > properly right now because they only isolate PageBuddy > (isolate_freepages_block) or PageLRU resp. __PageMovable > (isolate_migratepages_block) which will be always false for these pages. > It would be safer to skip these pages altogether, though. In order > to do that let's check PageReserved in __pageblock_pfn_to_page because > offline pages are reserved. My issue with this is that PageReserved can be also set for other reasons than offlined block, e.g. by a random driver. So there are two suboptimal scenarios: - PageReserved is set on some page in the middle of pageblock. It won't be detected by this patch. This violates the "it would be safer" argument. - PageReserved is set on just the first (few) page(s) and because of this patch, we skip it completely and won't compact the rest of it. So if we decide we really need to check PageReserved to ensure safety, then we have to check it on each page. But I hope the existing criteria in compaction scanners are sufficient. Unless the semantic is that if somebody sets PageReserved, he's free to repurpose the rest of flags at his will (IMHO that's not the case). The pageblock-level check them becomes a performance optimization so when there's an "offline hole", compaction won't iterate it page by page. But the downside is the false positive resulting in skipping whole pageblock due to single page. I guess it's uncommon for a longlived offline holes to exist, so we could simply just drop this? > Signed-off-by: Michal Hocko > --- > mm/page_alloc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 0cacba69ab04..dcbbcfdda60e 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -1351,6 +1351,8 @@ struct page *__pageblock_pfn_to_page(unsigned long start_pfn, > return NULL; > > start_page = pfn_to_page(start_pfn); > + if (PageReserved(start_page)) > + return NULL; > > if (page_zone(start_page) != zone) > return NULL; > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f200.google.com (mail-wr0-f200.google.com [209.85.128.200]) by kanga.kvack.org (Postfix) with ESMTP id 57E046B0038 for ; Tue, 18 Apr 2017 04:45:27 -0400 (EDT) Received: by mail-wr0-f200.google.com with SMTP id 28so15741043wrw.13 for ; Tue, 18 Apr 2017 01:45:27 -0700 (PDT) Received: from mx1.suse.de (mx2.suse.de. [195.135.220.15]) by mx.google.com with ESMTPS id d3si15064919wmf.12.2017.04.18.01.45.25 for (version=TLS1 cipher=AES128-SHA bits=128/128); Tue, 18 Apr 2017 01:45:25 -0700 (PDT) Subject: Re: [PATCH 1/3] mm: consider zone which is not fully populated to have holes References: <20170410110351.12215-1-mhocko@kernel.org> <20170415121734.6692-1-mhocko@kernel.org> <20170415121734.6692-2-mhocko@kernel.org> From: Vlastimil Babka Message-ID: <97a658cd-e656-6efa-7725-150063d276f1@suse.cz> Date: Tue, 18 Apr 2017 10:45:23 +0200 MIME-Version: 1.0 In-Reply-To: <20170415121734.6692-2-mhocko@kernel.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko , linux-mm@kvack.org Cc: Andrew Morton , 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 , Michal Hocko On 04/15/2017 02:17 PM, Michal Hocko wrote: > From: Michal Hocko > > __pageblock_pfn_to_page has two users currently, set_zone_contiguous > which checks whether the given zone contains holes and > pageblock_pfn_to_page which then carefully returns a first valid > page from the given pfn range for the given zone. This doesn't handle > zones which are not fully populated though. Memory pageblocks can be > offlined or might not have been onlined yet. In such a case the zone > should be considered to have holes otherwise pfn walkers can touch > and play with offline pages. > > Current callers of pageblock_pfn_to_page in compaction seem to work > properly right now because they only isolate PageBuddy > (isolate_freepages_block) or PageLRU resp. __PageMovable > (isolate_migratepages_block) which will be always false for these pages. > It would be safer to skip these pages altogether, though. In order > to do that let's check PageReserved in __pageblock_pfn_to_page because > offline pages are reserved. My issue with this is that PageReserved can be also set for other reasons than offlined block, e.g. by a random driver. So there are two suboptimal scenarios: - PageReserved is set on some page in the middle of pageblock. It won't be detected by this patch. This violates the "it would be safer" argument. - PageReserved is set on just the first (few) page(s) and because of this patch, we skip it completely and won't compact the rest of it. So if we decide we really need to check PageReserved to ensure safety, then we have to check it on each page. But I hope the existing criteria in compaction scanners are sufficient. Unless the semantic is that if somebody sets PageReserved, he's free to repurpose the rest of flags at his will (IMHO that's not the case). The pageblock-level check them becomes a performance optimization so when there's an "offline hole", compaction won't iterate it page by page. But the downside is the false positive resulting in skipping whole pageblock due to single page. I guess it's uncommon for a longlived offline holes to exist, so we could simply just drop this? > Signed-off-by: Michal Hocko > --- > mm/page_alloc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 0cacba69ab04..dcbbcfdda60e 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -1351,6 +1351,8 @@ struct page *__pageblock_pfn_to_page(unsigned long start_pfn, > return NULL; > > start_page = pfn_to_page(start_pfn); > + if (PageReserved(start_page)) > + return NULL; > > if (page_zone(start_page) != zone) > return NULL; > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org