Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934522AbcLMQrz (ORCPT ); Tue, 13 Dec 2016 11:47:55 -0500 Received: from mail-qk0-f194.google.com ([209.85.220.194]:33064 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933681AbcLMQrJ (ORCPT ); Tue, 13 Dec 2016 11:47:09 -0500 Subject: Re: memory_hotplug: zone_can_shift() returns boolean value To: Reza Arbab References: <8f85e530-4cc9-164b-ab44-6ebd78389c7b@gmail.com> <20161213155435.fs4n44gt6g2u2f2e@arbab-laptop> Cc: linux-mm@kvack.org, isimatu.yasuaki@jp.fujitsu.com, linux-kernel@vger.kernel.org From: Yasuaki Ishimatsu Message-ID: <598c13a6-81d8-e591-0239-ab403306e382@gmail.com> Date: Tue, 13 Dec 2016 11:47:04 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161213155435.fs4n44gt6g2u2f2e@arbab-laptop> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3853 Lines: 115 Hi Reza, Thank you for your review. On 12/13/2016 10:54 AM, Reza Arbab wrote: > 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 I'll update it. > >> --- 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; I'll update it. Thanks, Yasuaki Ishimatsu >> @@ -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 >> >