From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S934076AbcJRI1V (ORCPT <rfc822;w@1wt.eu>);
        Tue, 18 Oct 2016 04:27:21 -0400
Received: from LGEAMRELO11.lge.com ([156.147.23.51]:57477 "EHLO
        lgeamrelo11.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S934016AbcJRI0u (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 18 Oct 2016 04:26:50 -0400
X-Original-SENDERIP: 156.147.1.126
X-Original-MAILFROM: iamjoonsoo.kim@lge.com
X-Original-SENDERIP: 10.177.222.138
X-Original-MAILFROM: iamjoonsoo.kim@lge.com
Date: Tue, 18 Oct 2016 17:27:30 +0900
From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        Rik van Riel <riel@redhat.com>, Johannes Weiner <hannes@cmpxchg.org>,
        mgorman@techsingularity.net, Laura Abbott <lauraa@codeaurora.org>,
        Minchan Kim <minchan@kernel.org>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Michal Nazarewicz <mina86@mina86.com>,
        "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 3/6] mm/cma: populate ZONE_CMA
Message-ID: <20161018082730.GA20442@js1304-P5Q-DELUXE>
References: <1476414196-3514-1-git-send-email-iamjoonsoo.kim@lge.com>
 <1476414196-3514-4-git-send-email-iamjoonsoo.kim@lge.com>
 <33f0a8f3-38d1-e527-f71f-839afe0b2ed9@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <33f0a8f3-38d1-e527-f71f-839afe0b2ed9@suse.cz>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Oct 18, 2016 at 09:42:57AM +0200, Vlastimil Babka wrote:
> On 10/14/2016 05:03 AM, js1304@gmail.com wrote:
> >@@ -145,6 +145,35 @@ static int __init cma_activate_area(struct cma *cma)
> > static int __init cma_init_reserved_areas(void)
> > {
> > 	int i;
> >+	struct zone *zone;
> >+	pg_data_t *pgdat;
> >+
> >+	if (!cma_area_count)
> >+		return 0;
> >+
> >+	for_each_online_pgdat(pgdat) {
> >+		unsigned long start_pfn = UINT_MAX, end_pfn = 0;
> >+
> >+		for (i = 0; i < cma_area_count; i++) {
> >+			if (pfn_to_nid(cma_areas[i].base_pfn) !=
> >+				pgdat->node_id)
> >+				continue;
> >+
> >+			start_pfn = min(start_pfn, cma_areas[i].base_pfn);
> >+			end_pfn = max(end_pfn, cma_areas[i].base_pfn +
> >+						cma_areas[i].count);
> >+		}
> >+
> >+		if (!end_pfn)
> >+			continue;
> >+
> >+		zone = &pgdat->node_zones[ZONE_CMA];
> >+
> >+		/* ZONE_CMA doesn't need to exceed CMA region */
> >+		zone->zone_start_pfn = max(zone->zone_start_pfn, start_pfn);
> >+		zone->spanned_pages = min(zone_end_pfn(zone), end_pfn) -
> >+					zone->zone_start_pfn;
> 
> Hmm, do the max/min here work as intended? IIUC the initial

Yeap.

> zone_start_pfn is UINT_MAX and zone->spanned_pages is 1? So at least
> the max/min should be swapped?

No. CMA zone's start/end pfn are updated as node's start/end pfn.

> Also the zone_end_pfn(zone) on the second line already sees the
> changes to zone->zone_start_pfn in the first line, so it's kind of a
> mess. You should probably cache zone_end_pfn() to a temporary
> variable before changing zone_start_pfn.

You're right although it doesn't cause any problem. I look at the code
again and find that max/min isn't needed. Calculated start/end pfn
should be inbetween node's start/end pfn so max(zone->zone_start_pfn,
start_pfn) will return start_pfn and messed up min(zone_end_pfn(zone),
end_pfn) will return end_pfn in all the cases.

Anyway, I will fix it as following.

zone->zone_start_pfn = start_pfn
zone->spanned_pages = end_pfn - start_pfn

Thanks.