Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp538898imm; Wed, 18 Jul 2018 06:38:01 -0700 (PDT) X-Google-Smtp-Source: AAOMgpfU8vuFroQUqkp7OwBw4Zk6hqVe3AvJObUaDT5MFgl4fyzaDtnK8ioT3PwXN5Cjm5PlJWIn X-Received: by 2002:a63:4506:: with SMTP id s6-v6mr6027085pga.422.1531921081529; Wed, 18 Jul 2018 06:38:01 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1531921081; cv=none; d=google.com; s=arc-20160816; b=AeNdptvHYPS/cREZsbd74kcvpY3vzY83phTWE3CcJ/0p3BwJkgTX/ygyCfisNGwb0V psplxz+ibTAKTnSG7WZxf64YMJUEsJ2UGmW655vXXHsUnQ/wuAYcN0Zu6lGJPZ3hYA02 h/+bgT44ubkc3mT0BEHSiwofk+EBJVsyhiF2w3O8bFVwDjOMAs8aAg4JfdfIR1f/bo/3 RzeZmP6PJ9xCoKQd5N7YGHju9CHbMiT+lP9RFDrc3xpkTyZGAV9wL4XHutCj9XTFjN4Q WQFm1XefHfuwwwwhBybfOQ4Ii0hTLoosz5sUYHyjXtBErmJ/Yn0M6lr420U/vMIJGMNy Z5gw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:arc-authentication-results; bh=HRc6rKeYRUVnoR0BsUx+dzQHsp3oHh2VGxrSEZLxeMc=; b=oQ9FjikPndkDMRnoPbyjviR2dlu3VuB9DUG62+JOLin4YI/BPSLA4GfsL+X20whdyy fGT49cntPz/P8uP6d+AnQEThu050PEHYlbNKOw/I7KAmoK1gmT29I+eCXskzN1q/kkz3 T1znf+RXAlikMLeys5SnRnSvKz/WXYpKcZty7U2IjURHLdcDho7sPCQYP9KCsWZdL0Lw cXnF9JctpGK0Lv3UaeTDLzWlaRqrkZXSW2CYlZ3M3J543WbIIpeETxyqiAdhsFmIVCQa nN9UZ7VC3CjGgZ3nrK+tO1EKXUMneLBmePZLCmTEKtB/zmwCiG12OFNmjvB86ozShRTM Mnow== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id o20-v6si3492482pgh.319.2018.07.18.06.37.46; Wed, 18 Jul 2018 06:38:01 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731133AbeGROOt (ORCPT + 99 others); Wed, 18 Jul 2018 10:14:49 -0400 Received: from mx2.suse.de ([195.135.220.15]:38404 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726909AbeGROOs (ORCPT ); Wed, 18 Jul 2018 10:14:48 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 3AA9DAD86; Wed, 18 Jul 2018 13:36:48 +0000 (UTC) Date: Wed, 18 Jul 2018 15:36:47 +0200 From: Michal Hocko To: osalvador@techadventures.net Cc: akpm@linux-foundation.org, pasha.tatashin@oracle.com, vbabka@suse.cz, iamjoonsoo.kim@lge.com, aaron.lu@intel.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, Oscar Salvador Subject: Re: [PATCH 2/3] mm/page_alloc: Refactor free_area_init_core Message-ID: <20180718133647.GD7193@dhcp22.suse.cz> References: <20180718124722.9872-1-osalvador@techadventures.net> <20180718124722.9872-3-osalvador@techadventures.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180718124722.9872-3-osalvador@techadventures.net> User-Agent: Mutt/1.10.0 (2018-05-17) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed 18-07-18 14:47:21, osalvador@techadventures.net wrote: > From: Oscar Salvador > > When free_area_init_core gets called from the memhotplug code, > we only need to perform some of the operations in > there. Which ones? Or other way around. Which we do not want to do and why? > Since memhotplug code is the only place where free_area_init_core > gets called while node being still offline, we can better separate > the context from where it is called. I really do not like this if node is offline than only perform half of the function. This will generate more mess in the future. Why don't you simply. If we can split out this code into logical units then let's do that but no, please do not make random ifs for hotplug code paths. Sooner or later somebody will simply don't know what is needed and what is not. > This patch re-structures the code for that purpose. > > Signed-off-by: Oscar Salvador > --- > mm/page_alloc.c | 94 +++++++++++++++++++++++++++++++-------------------------- > 1 file changed, 52 insertions(+), 42 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 8a73305f7c55..d652a3ad720c 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -6237,6 +6237,40 @@ static void pgdat_init_kcompactd(struct pglist_data *pgdat) > static void pgdat_init_kcompactd(struct pglist_data *pgdat) {} > #endif > > +static unsigned long calc_remaining_pages(enum zone_type type, unsigned long freesize, > + unsigned long size) > +{ > + unsigned long memmap_pages = calc_memmap_size(size, freesize); > + > + if(!is_highmem_idx(type)) { > + if (freesize >= memmap_pages) { > + freesize -= memmap_pages; > + if (memmap_pages) > + printk(KERN_DEBUG > + " %s zone: %lu pages used for memmap\n", > + zone_names[type], memmap_pages); > + } else > + pr_warn(" %s zone: %lu pages exceeds freesize %lu\n", > + zone_names[type], memmap_pages, freesize); > + } > + > + /* Account for reserved pages */ > + if (type == 0 && freesize > dma_reserve) { > + freesize -= dma_reserve; > + printk(KERN_DEBUG " %s zone: %lu pages reserved\n", > + zone_names[0], dma_reserve); > + } > + > + if (!is_highmem_idx(type)) > + nr_kernel_pages += freesize; > + /* Charge for highmem memmap if there are enough kernel pages */ > + else if (nr_kernel_pages > memmap_pages * 2) > + nr_kernel_pages -= memmap_pages; > + nr_all_pages += freesize; > + > + return freesize; > +} > + > /* > * Set up the zone data structures: > * - mark all pages reserved > @@ -6249,6 +6283,7 @@ static void __paginginit free_area_init_core(struct pglist_data *pgdat) > { > enum zone_type j; > int nid = pgdat->node_id; > + bool no_hotplug_context; > > pgdat_resize_init(pgdat); > > @@ -6265,45 +6300,18 @@ static void __paginginit free_area_init_core(struct pglist_data *pgdat) > > pgdat->per_cpu_nodestats = &boot_nodestats; > > + /* Memhotplug is the only place where free_area_init_node gets called > + * with the node being still offline. > + */ > + no_hotplug_context = node_online(nid); > + > for (j = 0; j < MAX_NR_ZONES; j++) { > struct zone *zone = pgdat->node_zones + j; > - unsigned long size, freesize, memmap_pages; > - unsigned long zone_start_pfn = zone->zone_start_pfn; > + unsigned long size = zone->spanned_pages; > + unsigned long freesize = zone->present_pages; > > - size = zone->spanned_pages; > - freesize = zone->present_pages; > - > - /* > - * Adjust freesize so that it accounts for how much memory > - * is used by this zone for memmap. This affects the watermark > - * and per-cpu initialisations > - */ > - memmap_pages = calc_memmap_size(size, freesize); > - if (!is_highmem_idx(j)) { > - if (freesize >= memmap_pages) { > - freesize -= memmap_pages; > - if (memmap_pages) > - printk(KERN_DEBUG > - " %s zone: %lu pages used for memmap\n", > - zone_names[j], memmap_pages); > - } else > - pr_warn(" %s zone: %lu pages exceeds freesize %lu\n", > - zone_names[j], memmap_pages, freesize); > - } > - > - /* Account for reserved pages */ > - if (j == 0 && freesize > dma_reserve) { > - freesize -= dma_reserve; > - printk(KERN_DEBUG " %s zone: %lu pages reserved\n", > - zone_names[0], dma_reserve); > - } > - > - if (!is_highmem_idx(j)) > - nr_kernel_pages += freesize; > - /* Charge for highmem memmap if there are enough kernel pages */ > - else if (nr_kernel_pages > memmap_pages * 2) > - nr_kernel_pages -= memmap_pages; > - nr_all_pages += freesize; > + if (no_hotplug_context) > + freesize = calc_remaining_pages(j, freesize, size); > > /* > * Set an approximate value for lowmem here, it will be adjusted > @@ -6311,6 +6319,7 @@ static void __paginginit free_area_init_core(struct pglist_data *pgdat) > * And all highmem pages will be managed by the buddy system. > */ > zone->managed_pages = freesize; > + > #ifdef CONFIG_NUMA > zone->node = nid; > #endif > @@ -6320,13 +6329,14 @@ static void __paginginit free_area_init_core(struct pglist_data *pgdat) > zone_seqlock_init(zone); > zone_pcp_init(zone); > > - if (!size) > - continue; > + if (size && no_hotplug_context) { > + unsigned long zone_start_pfn = zone->zone_start_pfn; > > - set_pageblock_order(); > - setup_usemap(pgdat, zone, zone_start_pfn, size); > - init_currently_empty_zone(zone, zone_start_pfn, size); > - memmap_init(size, nid, j, zone_start_pfn); > + set_pageblock_order(); > + setup_usemap(pgdat, zone, zone_start_pfn, size); > + init_currently_empty_zone(zone, zone_start_pfn, size); > + memmap_init(size, nid, j, zone_start_pfn); > + } > } > } > > -- > 2.13.6 > -- Michal Hocko SUSE Labs