Re: memory_hotplug: zone_can_shift() returns boolean value

[Reza Arbab]

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