Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758789Ab2JSOQE (ORCPT ); Fri, 19 Oct 2012 10:16:04 -0400 Received: from mail-pb0-f46.google.com ([209.85.160.46]:54657 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756940Ab2JSOP7 (ORCPT ); Fri, 19 Oct 2012 10:15:59 -0400 Message-ID: <5081609C.9080702@gmail.com> Date: Fri, 19 Oct 2012 22:15:56 +0800 From: Wen Congyang User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0.1) Gecko/20110929 Thunderbird/7.0.1 MIME-Version: 1.0 To: Wen Congyang CC: KOSAKI Motohiro , Yasuaki Ishimatsu , x86@kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-acpi@vger.kernel.org, linux-s390@vger.kernel.org, linux-sh@vger.kernel.org, linux-ia64@vger.kernel.org, cmetcalf@tilera.com, sparclinux@vger.kernel.org, rientjes@google.com, liuj97@gmail.com, len.brown@intel.com, cl@linux.com, minchan.kim@gmail.com, akpm@linux-foundation.org Subject: Re: [PATCH 1/10] memory-hotplug : check whether memory is offline or not when removing memory References: <506E43E0.70507@jp.fujitsu.com> <506E451E.1050403@jp.fujitsu.com> <50812F13.20503@cn.fujitsu.com> In-Reply-To: <50812F13.20503@cn.fujitsu.com> Content-Type: text/plain; charset=ISO-8859-1; 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: 7245 Lines: 201 At 2012/10/19 18:44, Wen Congyang Wrote: > At 10/06/2012 03:27 AM, KOSAKI Motohiro Wrote: >> On Thu, Oct 4, 2012 at 10:25 PM, Yasuaki Ishimatsu >> wrote: >>> When calling remove_memory(), the memory should be offline. If the function >>> is used to online memory, kernel panic may occur. >>> >>> So the patch checks whether memory is offline or not. >> >> You don't explain WHY we need the check. > > This patch is no necessary now, because the newest kernel has checked > it. I think it again, and found that this check is necessary. Because we only lock memory hotplug when offlining pages. Here is the steps to offline and remove memory: 1. lock memory hotplug 2. offline a memory section 3. unlock memory hotplug 4. repeat 1-3 to offline all memory sections 5. lock memory hotplug 6. remove memory 7. unlock memory hotplug All memory sections must be offlined before removing memory. But we don't hold the lock in the whole operation. So we should check whether all memory sections are offlined before step6. > > Thanks > Wen Congyang > >> >> >>> CC: David Rientjes >>> CC: Jiang Liu >>> CC: Len Brown >>> CC: Christoph Lameter >>> Cc: Minchan Kim >>> CC: Andrew Morton >>> CC: KOSAKI Motohiro >>> Signed-off-by: Wen Congyang >>> Signed-off-by: Yasuaki Ishimatsu >>> >>> --- >>> drivers/base/memory.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> include/linux/memory.h | 5 +++++ >>> mm/memory_hotplug.c | 17 +++++++++++++++-- >>> 3 files changed, 59 insertions(+), 2 deletions(-) >>> >>> Index: linux-3.6/drivers/base/memory.c >>> =================================================================== >>> --- linux-3.6.orig/drivers/base/memory.c 2012-10-04 14:22:57.000000000 +0900 >>> +++ linux-3.6/drivers/base/memory.c 2012-10-04 14:45:46.653585860 +0900 >>> @@ -70,6 +70,45 @@ void unregister_memory_isolate_notifier( >>> } >>> EXPORT_SYMBOL(unregister_memory_isolate_notifier); >>> >>> +bool is_memblk_offline(unsigned long start, unsigned long size) >> >> Don't use memblk. Usually memblk mean struct numa_meminfo for x86/numa. >> Maybe memory_range_offlined() is better. >> >> And, this function don't take struct memory_block, then this file may be no good >> place. >> >> And you need to write down function comment. >> >> >>> +{ >>> + struct memory_block *mem = NULL; >>> + struct mem_section *section; >>> + unsigned long start_pfn, end_pfn; >>> + unsigned long pfn, section_nr; >>> + >>> + start_pfn = PFN_DOWN(start); >>> + end_pfn = PFN_UP(start + size); >>> + >>> + for (pfn = start_pfn; pfn< end_pfn; pfn += PAGES_PER_SECTION) { >>> + section_nr = pfn_to_section_nr(pfn); >>> + if (!present_section_nr(section_nr)) >>> + continue; >>> + >>> + section = __nr_to_section(section_nr); >>> + /* same memblock? */ >>> + if (mem) >>> + if ((section_nr>= mem->start_section_nr)&& >>> + (section_nr<= mem->end_section_nr)) >>> + continue; >>> + >>> + mem = find_memory_block_hinted(section, mem); >>> + if (!mem) >>> + continue; >>> + if (mem->state == MEM_OFFLINE) >>> + continue; >>> + >>> + kobject_put(&mem->dev.kobj); >>> + return false; >>> + } >>> + >>> + if (mem) >>> + kobject_put(&mem->dev.kobj); >>> + >>> + return true; >>> +} >>> +EXPORT_SYMBOL(is_memblk_offline); >>> + >>> /* >>> * register_memory - Setup a sysfs device for a memory block >>> */ >>> Index: linux-3.6/include/linux/memory.h >>> =================================================================== >>> --- linux-3.6.orig/include/linux/memory.h 2012-10-02 18:00:22.000000000 +0900 >>> +++ linux-3.6/include/linux/memory.h 2012-10-04 14:44:40.902581028 +0900 >>> @@ -106,6 +106,10 @@ static inline int memory_isolate_notify( >>> { >>> return 0; >>> } >>> +static inline bool is_memblk_offline(unsigned long start, unsigned long size) >>> +{ >>> + return false; >>> +} >>> #else >>> extern int register_memory_notifier(struct notifier_block *nb); >>> extern void unregister_memory_notifier(struct notifier_block *nb); >>> @@ -120,6 +124,7 @@ extern int memory_isolate_notify(unsigne >>> extern struct memory_block *find_memory_block_hinted(struct mem_section *, >>> struct memory_block *); >>> extern struct memory_block *find_memory_block(struct mem_section *); >>> +extern bool is_memblk_offline(unsigned long start, unsigned long size); >>> #define CONFIG_MEM_BLOCK_SIZE (PAGES_PER_SECTION<>> enum mem_add_context { BOOT, HOTPLUG }; >>> #endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */ >>> Index: linux-3.6/mm/memory_hotplug.c >>> =================================================================== >>> --- linux-3.6.orig/mm/memory_hotplug.c 2012-10-04 14:31:08.000000000 +0900 >>> +++ linux-3.6/mm/memory_hotplug.c 2012-10-04 14:58:22.449687986 +0900 >>> @@ -1045,8 +1045,21 @@ int offline_memory(u64 start, u64 size) >>> >>> int remove_memory(int nid, u64 start, u64 size) >>> { >> >> Your remove_memory() don't remove anything. that's strange. IIUC, this batch is based on another patchset. >> >> >>> - /* It is not implemented yet*/ >>> - return 0; >>> + int ret = 0; >>> + lock_memory_hotplug(); >>> + /* >>> + * The memory might become online by other task, even if you offine it. >>> + * So we check whether the memory has been onlined or not. >>> + */ >>> + if (!is_memblk_offline(start, size)) { >>> + pr_warn("memory removing [mem %#010llx-%#010llx] failed, " >>> + "because the memmory range is online\n", >>> + start, start + size); >> >> No good warning. You should output which memory block can't be >> offlined, I think. OK. I'll update it. Thanks Wen Congyang >> >> >>> + ret = -EAGAIN; >>> + } >>> + >>> + unlock_memory_hotplug(); >>> + return ret; >>> } >>> EXPORT_SYMBOL_GPL(remove_memory); >>> #else >>> >>> -- >>> To unsubscribe, send a message with 'unsubscribe linux-mm' in >>> the body to majordomo@kvack.org. For more info on Linux MM, >>> see: http://www.linux-mm.org/ . >>> Don't email: email@kvack.org >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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/