Date: Tue, 13 Dec 2016 09:54:35 -0600
From: Reza Arbab <arbab@linux.vnet.ibm.com>
To: Yasuaki Ishimatsu <yasu.isimatu@gmail.com>
Cc: linux-mm@kvack.org, isimatu.yasuaki@jp.fujitsu.com,
        linux-kernel@vger.kernel.org
Subject: Re: memory_hotplug: zone_can_shift() returns boolean value
References: <8f85e530-4cc9-164b-ab44-6ebd78389c7b@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <8f85e530-4cc9-164b-ab44-6ebd78389c7b@gmail.com>
Organization: IBM Linux Technology Center
User-Agent: NeoMutt/20161126 (1.7.1)
Message-Id: <20161213155435.fs4n44gt6g2u2f2e@arbab-laptop>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3254
Lines: 106

On Mon, Dec 12, 2016 at 03:29:04PM -0500, Yasuaki Ishimatsu wrote:
>--- a/drivers/base/memory.c
>+++ b/drivers/base/memory.c
>@@ -410,15 +410,13 @@ static ssize_t show_valid_zones(struct device *dev,
> 	sprintf(buf, "%s", zone->name);
>
> 	/* MMOP_ONLINE_KERNEL */
>-	zone_shift = zone_can_shift(start_pfn, nr_pages, ZONE_NORMAL);
>-	if (zone_shift) {
>+	if (zone_can_shift(start_pfn, nr_pages, ZONE_NORMAL, &zone_shift)) {
> 		strcat(buf, " ");
> 		strcat(buf, (zone + zone_shift)->name);
> 	}
>
> 	/* MMOP_ONLINE_MOVABLE */
>-	zone_shift = zone_can_shift(start_pfn, nr_pages, ZONE_MOVABLE);
>-	if (zone_shift) {
>+	if (zone_can_shift(start_pfn, nr_pages, ZONE_MOVABLE, &zone_shift)) {
> 		strcat(buf, " ");
> 		strcat(buf, (zone + zone_shift)->name);
> 	}

You still need to check zone_shift != 0, otherwise you may get duplicate 
output:

$ cat /sys/devices/system/node/node1/memory256/valid_zones
Movable Normal Movable
$ cat /sys/devices/system/node/node1/memory257/valid_zones
Movable Movable

>--- a/mm/memory_hotplug.c
>+++ b/mm/memory_hotplug.c
>@@ -1033,8 +1033,8 @@ static void node_states_set_node(int node, struct memory_notify *arg)
> 	node_set_state(node, N_MEMORY);
> }
>
>-int zone_can_shift(unsigned long pfn, unsigned long nr_pages,
>-		   enum zone_type target)
>+bool zone_can_shift(unsigned long pfn, unsigned long nr_pages,
>+		   enum zone_type target, int *zone_shift)
> {
> 	struct zone *zone = page_zone(pfn_to_page(pfn));
> 	enum zone_type idx = zone_idx(zone);

I think you should initialize zone_shift here. It should be 0 if the 
function returns false.

	*zone_shift = 0;

>@@ -1043,26 +1043,27 @@ int zone_can_shift(unsigned long pfn, unsigned long nr_pages,
> 	if (idx < target) {
> 		/* pages must be at end of current zone */
> 		if (pfn + nr_pages != zone_end_pfn(zone))
>-			return 0;
>+			return false;
>
> 		/* no zones in use between current zone and target */
> 		for (i = idx + 1; i < target; i++)
> 			if (zone_is_initialized(zone - idx + i))
>-				return 0;
>+				return false;
> 	}
>
> 	if (target < idx) {
> 		/* pages must be at beginning of current zone */
> 		if (pfn != zone->zone_start_pfn)
>-			return 0;
>+			return false;
>
> 		/* no zones in use between current zone and target */
> 		for (i = target + 1; i < idx; i++)
> 			if (zone_is_initialized(zone - idx + i))
>-				return 0;
>+				return false;
> 	}
>
>-	return target - idx;
>+	*zone_shift = target - idx;
>+	return true;
> }
>
> /* Must be protected by mem_hotplug_begin() */
>@@ -1089,10 +1090,13 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages, int online_typ
> 	    !can_online_high_movable(zone))
> 		return -EINVAL;
>
>-	if (online_type == MMOP_ONLINE_KERNEL)
>-		zone_shift = zone_can_shift(pfn, nr_pages, ZONE_NORMAL);
>-	else if (online_type == MMOP_ONLINE_MOVABLE)
>-		zone_shift = zone_can_shift(pfn, nr_pages, ZONE_MOVABLE);
>+	if (online_type == MMOP_ONLINE_KERNEL) {
>+		if (!zone_can_shift(pfn, nr_pages, ZONE_NORMAL, &zone_shift))
>+			return -EINVAL;
>+	} else if (online_type == MMOP_ONLINE_MOVABLE) {
>+		if (!zone_can_shift(pfn, nr_pages, ZONE_MOVABLE, &zone_shift))
>+			return -EINVAL;
>+	}
>
> 	zone = move_pfn_range(zone_shift, pfn, pfn + nr_pages);
> 	if (!zone)
>-- 
>1.8.3.1
>

-- 
Reza Arbab