lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 27 Dec 2012 11:09:19 +0800 From: Tang Chen <tangchen@...fujitsu.com> To: Kamezawa Hiroyuki <kamezawa.hiroyu@...fujitsu.com> CC: akpm@...ux-foundation.org, rientjes@...gle.com, liuj97@...il.com, len.brown@...el.com, benh@...nel.crashing.org, paulus@...ba.org, cl@...ux.com, minchan.kim@...il.com, kosaki.motohiro@...fujitsu.com, isimatu.yasuaki@...fujitsu.com, wujianguo@...wei.com, wency@...fujitsu.com, hpa@...or.com, linfeng@...fujitsu.com, laijs@...fujitsu.com, mgorman@...e.de, yinghai@...nel.org, x86@...nel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org, linux-acpi@...r.kernel.org, linux-s390@...r.kernel.org, linux-sh@...r.kernel.org, linux-ia64@...r.kernel.org, cmetcalf@...era.com, sparclinux@...r.kernel.org Subject: Re: [PATCH v5 03/14] memory-hotplug: remove redundant codes Hi Kamezawa-san, Thanks for the reviewing. Please see below. :) On 12/26/2012 11:20 AM, Kamezawa Hiroyuki wrote: > (2012/12/24 21:09), Tang Chen wrote: >> From: Wen Congyang<wency@...fujitsu.com> >> >> offlining memory blocks and checking whether memory blocks are offlined >> are very similar. This patch introduces a new function to remove >> redundant codes. >> >> Signed-off-by: Wen Congyang<wency@...fujitsu.com> >> --- >> mm/memory_hotplug.c | 101 ++++++++++++++++++++++++++++----------------------- >> 1 files changed, 55 insertions(+), 46 deletions(-) >> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >> index d43d97b..dbb04d8 100644 >> --- a/mm/memory_hotplug.c >> +++ b/mm/memory_hotplug.c >> @@ -1381,20 +1381,14 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages) >> return __offline_pages(start_pfn, start_pfn + nr_pages, 120 * HZ); >> } >> >> -int remove_memory(u64 start, u64 size) > > please add explanation of this function here. If (*func) returns val other than 0, > this function will fail and returns callback's return value...right ? > Yes, it will always return the func()'s return value. I'll add the comment here. :) > >> +static int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn, >> + void *arg, int (*func)(struct memory_block *, void *)) >> { >> struct memory_block *mem = NULL; >> struct mem_section *section; >> - unsigned long start_pfn, end_pfn; >> unsigned long pfn, section_nr; >> int ret; >> - int return_on_error = 0; >> - int retry = 0; >> - >> - start_pfn = PFN_DOWN(start); >> - end_pfn = start_pfn + PFN_DOWN(size); >> >> -repeat: > > Shouldn't we check lock is held here ? (VM_BUG_ON(!mutex_is_locked(&mem_hotplug_mutex); Well, I think, after applying this patch, walk_memory_range() will be a separated function. And it can be used somewhere else where we don't hold this lock. But for now, we can do this check. :) > > >> for (pfn = start_pfn; pfn< end_pfn; pfn += PAGES_PER_SECTION) { >> section_nr = pfn_to_section_nr(pfn); >> if (!present_section_nr(section_nr)) >> @@ -1411,22 +1405,61 @@ repeat: >> if (!mem) >> continue; >> >> - ret = offline_memory_block(mem); >> + ret = func(mem, arg); >> if (ret) { >> - if (return_on_error) { >> - kobject_put(&mem->dev.kobj); >> - return ret; >> - } else { >> - retry = 1; >> - } >> + kobject_put(&mem->dev.kobj); >> + return ret; >> } >> } >> >> if (mem) >> kobject_put(&mem->dev.kobj); >> >> - if (retry) { >> - return_on_error = 1; >> + return 0; >> +} >> + >> +static int offline_memory_block_cb(struct memory_block *mem, void *arg) >> +{ >> + int *ret = arg; >> + int error = offline_memory_block(mem); >> + >> + if (error != 0&& *ret == 0) >> + *ret = error; >> + >> + return 0; > > Always returns 0 and run through all mem blocks for scan-and-retry, right ? > You need explanation here ! Yes, I'll add the comment. :) > > >> +} >> + >> +static int is_memblock_offlined_cb(struct memory_block *mem, void *arg) >> +{ >> + int ret = !is_memblock_offlined(mem); >> + >> + if (unlikely(ret)) >> + pr_warn("removing memory fails, because memory " >> + "[%#010llx-%#010llx] is onlined\n", >> + PFN_PHYS(section_nr_to_pfn(mem->start_section_nr)), >> + PFN_PHYS(section_nr_to_pfn(mem->end_section_nr + 1))-1); >> + >> + return ret; >> +} >> + >> +int remove_memory(u64 start, u64 size) >> +{ >> + unsigned long start_pfn, end_pfn; >> + int ret = 0; >> + int retry = 1; >> + >> + start_pfn = PFN_DOWN(start); >> + end_pfn = start_pfn + PFN_DOWN(size); >> + >> +repeat: > > please explan why you repeat here . This repeat is add in patch1. It aims to solve the problem we were talking about in patch1. I'll add the comment here. :) > >> + walk_memory_range(start_pfn, end_pfn,&ret, >> + offline_memory_block_cb); >> + if (ret) { >> + if (!retry) >> + return ret; >> + >> + retry = 0; >> + ret = 0; >> goto repeat; >> } >> >> @@ -1444,37 +1477,13 @@ repeat: >> * memory blocks are offlined. >> */ >> >> - 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; >> - >> - ret = is_memblock_offlined(mem); >> - if (!ret) { >> - pr_warn("removing memory fails, because memory " >> - "[%#010llx-%#010llx] is onlined\n", >> - PFN_PHYS(section_nr_to_pfn(mem->start_section_nr)), >> - PFN_PHYS(section_nr_to_pfn(mem->end_section_nr + 1)) - 1); >> - >> - kobject_put(&mem->dev.kobj); >> - unlock_memory_hotplug(); >> - return ret; >> - } > > please explain what you do here. confirming all memory blocks are offlined > before returning 0 ....right ? Will be added. :) Thanks. :) > >> + ret = walk_memory_range(start_pfn, end_pfn, NULL, >> + is_memblock_offlined_cb); >> + if (ret) { >> + unlock_memory_hotplug(); >> + return ret; >> } >> >> - if (mem) >> - kobject_put(&mem->dev.kobj); >> unlock_memory_hotplug(); >> >> return 0; >> > > Thanks, > -Kame > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists