Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757904Ab3ETTiu (ORCPT ); Mon, 20 May 2013 15:38:50 -0400 Received: from hydra.sisk.pl ([212.160.235.94]:40932 "EHLO hydra.sisk.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756441Ab3ETTir (ORCPT ); Mon, 20 May 2013 15:38:47 -0400 From: "Rafael J. Wysocki" To: Toshi Kani Cc: ACPI Devel Maling List , LKML , Greg Kroah-Hartman , Wen Congyang , Tang Chen , Yasuaki Ishimatsu , Andrew Morton , Jiang Liu , Vasilis Liaskovitis , linux-mm@kvack.org Subject: Re: [PATCH 5/5] ACPI / memhotplug: Drop unnecessary code Date: Mon, 20 May 2013 21:47:25 +0200 Message-ID: <1726699.Z30ifEcQDQ@vostro.rjw.lan> User-Agent: KMail/4.9.5 (Linux/3.9.0+; KDE/4.9.5; x86_64; ; ) In-Reply-To: <1369070876.5673.51.camel@misato.fc.hp.com> References: <2250271.rGYN6WlBxf@vostro.rjw.lan> <2228904.eTtCrFuSan@vostro.rjw.lan> <1369070876.5673.51.camel@misato.fc.hp.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5942 Lines: 171 On Monday, May 20, 2013 11:27:56 AM Toshi Kani wrote: > On Sun, 2013-05-19 at 01:34 +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki > > > > Now that the memory offlining should be taken care of by the > > companion device offlining code in acpi_scan_hot_remove(), the > > ACPI memory hotplug driver doesn't need to offline it in > > acpi_memory_remove_memory() any more. Consequently, it doesn't > > need to call remove_memory() any more, which means that that > > funtion may be dropped entirely, because acpi_memory_remove_memory() > > is the only user of it. > > The off-lining part of remove_memory() can be removed, but not the > hot-delete part. Please see my comments below. > > > Make the changes described above to get rid of the dead code. > > > > Signed-off-by: Rafael J. Wysocki > > --- > > drivers/acpi/acpi_memhotplug.c | 15 ------ > > include/linux/memory_hotplug.h | 1 > > mm/memory_hotplug.c | 102 ----------------------------------------- > > 3 files changed, 2 insertions(+), 116 deletions(-) > > > > Index: linux-pm/drivers/acpi/acpi_memhotplug.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/acpi_memhotplug.c > > +++ linux-pm/drivers/acpi/acpi_memhotplug.c > > @@ -271,31 +271,20 @@ static int acpi_memory_enable_device(str > > return 0; > > } > > > > -static int acpi_memory_remove_memory(struct acpi_memory_device *mem_device) > > +static void acpi_memory_remove_memory(struct acpi_memory_device *mem_device) > > { > > acpi_handle handle = mem_device->device->handle; > > - int result = 0, nid; > > struct acpi_memory_info *info, *n; > > > > - nid = acpi_get_node(handle); > > - > > list_for_each_entry_safe(info, n, &mem_device->res_list, list) { > > if (!info->enabled) > > continue; > > > > - if (nid < 0) > > - nid = memory_add_physaddr_to_nid(info->start_addr); > > - > > + /* All of the memory blocks are offline at this point. */ > > acpi_unbind_memory_blocks(info, handle); > > - result = remove_memory(nid, info->start_addr, info->length); > > We still need to call remove_memory(). > > > - if (result) > > - return result; > > - > > list_del(&info->list); > > kfree(info); > > } > > - > > - return result; > > } > > > > static void acpi_memory_device_free(struct acpi_memory_device *mem_device) > > Index: linux-pm/include/linux/memory_hotplug.h > > =================================================================== > > --- linux-pm.orig/include/linux/memory_hotplug.h > > +++ linux-pm/include/linux/memory_hotplug.h > > @@ -252,7 +252,6 @@ extern int add_memory(int nid, u64 start > > extern int arch_add_memory(int nid, u64 start, u64 size); > > extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages); > > extern bool is_memblock_offlined(struct memory_block *mem); > > -extern int remove_memory(int nid, u64 start, u64 size); > > extern int sparse_add_one_section(struct zone *zone, unsigned long start_pfn, > > int nr_pages); > > extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms); > > Index: linux-pm/mm/memory_hotplug.c > > =================================================================== > > --- linux-pm.orig/mm/memory_hotplug.c > > +++ linux-pm/mm/memory_hotplug.c > > @@ -1670,41 +1670,6 @@ int walk_memory_range(unsigned long star > > } > > : > > > - > > -int __ref remove_memory(int nid, u64 start, u64 size) > > -{ > > - unsigned long start_pfn, end_pfn; > > - int ret = 0; > > - int retry = 1; > > - > > - start_pfn = PFN_DOWN(start); > > - end_pfn = PFN_UP(start + size - 1); > > - > > - /* > > - * When CONFIG_MEMCG is on, one memory block may be used by other > > - * blocks to store page cgroup when onlining pages. But we don't know > > - * in what order pages are onlined. So we iterate twice to offline > > - * memory: > > - * 1st iterate: offline every non primary memory block. > > - * 2nd iterate: offline primary (i.e. first added) memory block. > > - */ > > -repeat: > > - walk_memory_range(start_pfn, end_pfn, &ret, > > - offline_memory_block_cb); > > - if (ret) { > > - if (!retry) > > - return ret; > > - > > - retry = 0; > > - ret = 0; > > - goto repeat; > > - } > > The above procedure can be removed as it is for off-lining. > > > - lock_memory_hotplug(); > > - > > - /* > > - * we have offlined all memory blocks like this: > > - * 1. lock memory hotplug > > - * 2. offline a memory block > > - * 3. unlock memory hotplug > > - * > > - * repeat step1-3 to offline the memory block. All memory blocks > > - * must be offlined before removing memory. But we don't hold the > > - * lock in the whole operation. So we should check whether all > > - * memory blocks are offlined. > > - */ > > - > > - ret = walk_memory_range(start_pfn, end_pfn, NULL, > > - is_memblock_offlined_cb); > > - if (ret) { > > - unlock_memory_hotplug(); > > - return ret; > > - } > > - > > I think the above procedure is still useful for safe guard. But then it shoud to BUG_ON() instead of returning an error (which isn't very useful for anything now). > > - /* remove memmap entry */ > > - firmware_map_remove(start, start + size, "System RAM"); > > - > > - arch_remove_memory(start, size); > > - > > - try_offline_node(nid); > > The above procedure performs memory hot-delete specific operations and > is necessary. OK, I see. I'll replace this patch with something simpler, then. What about the other patches in the series? Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/