Messages in this thread | | | Date | Wed, 12 Feb 2014 18:17:51 -0800 | From | Laura Abbott <> | Subject | Re: [PATCHv3 2/2] arm: Get rid of meminfo |
| |
On 2/12/2014 7:09 AM, Grygorii Strashko wrote: > Hi Laura, > > On 02/11/2014 11:14 PM, Laura Abbott wrote: >> memblock is now fully integrated into the kernel and is the prefered >> method for tracking memory. Rather than reinvent the wheel with >> meminfo, migrate to using memblock directly instead of meminfo as >> an intermediate. >> >> Acked-by: Jason Cooper <jason@lakedaemon.net> >> Acked-by: Catalin Marinas <catalin.marinas@arm.com> >> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> Tested-by: Leif Lindholm <leif.lindholm@linaro.org> >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org> >> --- >> arch/arm/include/asm/mach/arch.h | 4 +- >> arch/arm/include/asm/memblock.h | 3 +- >> arch/arm/include/asm/setup.h | 23 ------ >> arch/arm/kernel/atags_parse.c | 5 +- >> arch/arm/kernel/devtree.c | 5 -- >> arch/arm/kernel/setup.c | 30 ++------ >> arch/arm/mach-clps711x/board-clep7312.c | 7 +- >> arch/arm/mach-clps711x/board-edb7211.c | 10 +-- >> arch/arm/mach-clps711x/board-p720t.c | 2 +- >> arch/arm/mach-footbridge/cats-hw.c | 2 +- >> arch/arm/mach-footbridge/netwinder-hw.c | 2 +- >> arch/arm/mach-msm/board-halibut.c | 6 -- >> arch/arm/mach-msm/board-mahimahi.c | 13 +--- >> arch/arm/mach-msm/board-msm7x30.c | 3 +- >> arch/arm/mach-msm/board-sapphire.c | 13 ++-- >> arch/arm/mach-msm/board-trout.c | 8 +-- >> arch/arm/mach-orion5x/common.c | 3 +- >> arch/arm/mach-orion5x/common.h | 3 +- >> arch/arm/mach-pxa/cm-x300.c | 3 +- >> arch/arm/mach-pxa/corgi.c | 10 +-- >> arch/arm/mach-pxa/eseries.c | 9 +-- >> arch/arm/mach-pxa/poodle.c | 8 +-- >> arch/arm/mach-pxa/spitz.c | 8 +-- >> arch/arm/mach-pxa/tosa.c | 8 +-- >> arch/arm/mach-realview/core.c | 11 +-- >> arch/arm/mach-realview/core.h | 3 +- >> arch/arm/mach-realview/realview_pb1176.c | 8 +-- >> arch/arm/mach-realview/realview_pbx.c | 17 ++--- >> arch/arm/mach-s3c24xx/mach-smdk2413.c | 8 +-- >> arch/arm/mach-s3c24xx/mach-vstms.c | 8 +-- >> arch/arm/mach-sa1100/assabet.c | 2 +- >> arch/arm/mm/init.c | 67 +++++++----------- >> arch/arm/mm/mmu.c | 115 +++++++++--------------------- > > The arch/arm/mm/nommu.c has to be updated too :) >
Sure does.
> [...] > > I've tested your change on keystone (with some additional printouts in sanity_check_meminfo()) > and got following results: > > - without your change + HIGHMEM=ON > [ 0.000000] ==== memblock_limit0x00000000af800000, arm_lowmem_limit0x00000000af800000 high_memoryef800000 vmalloc_limit0x00000000af800000 > > - without your change + HIGHMEM=OFF > [ 0.000000] Truncating RAM at 80000000-bfffffff to -af7fffff (vmalloc region overlap). > [ 0.000000] ==== memblock_limit0x00000000af800000, arm_lowmem_limit0x00000000af800000 high_memoryef800000 vmalloc_limit0x00000000af800000 > > - with your change + HIGHMEM=ON > [ 0.000000] ==== memblock_limit0x00000000af800000, arm_lowmem_limit0x00000000af800000 high_memoryef800000 vmalloc_limit0x00000000af800000 > > - with your change + HIGHMEM=OFF > [ 0.000000] Truncating RAM at 0x0000000080000000-0x00000000c0000000 to -0x0000000010800000 > ^printout changed > [ 0.000000] ==== memblock_limit0x00000000af800000, arm_lowmem_limit0x00000000af800000 high_memoryef800000 vmalloc_limit0x00000000af800000 > > Keystone mem defined as: from@0x80000000 size@0x40000000 (LPAE=OFF) > > As result, i have few comments regarding sanity_check_meminfo() changes as I think there are > some issues &side effects changes at least in printouts - see below. > >> memblock_reserve(__pa(_sdata), _end - _sdata); >> @@ -413,54 +397,53 @@ free_memmap(unsigned long start_pfn, unsigned long end_pfn) >> /* >> * The mem_map array can get very big. Free the unused area of the memory map. >> */ >> -static void __init free_unused_memmap(struct meminfo *mi) >> +static void __init free_unused_memmap(void) >> { >> - unsigned long bank_start, prev_bank_end = 0; >> - unsigned int i; >> + unsigned long start, prev_end = 0; >> + struct memblock_region *reg; >> >> /* >> * This relies on each bank being in address order. >> * The banks are sorted previously in bootmem_init(). >> */ >> - for_each_bank(i, mi) { >> - struct membank *bank = &mi->bank[i]; >> - >> - bank_start = bank_pfn_start(bank); >> + for_each_memblock(memory, reg) { >> + start = __phys_to_pfn(reg->base); > > memblock_region_memory_base_pfn() can be used here. >
Okay
>> >> #ifdef CONFIG_SPARSEMEM >> /* >> * Take care not to free memmap entries that don't exist >> * due to SPARSEMEM sections which aren't present. >> */ >> - bank_start = min(bank_start, >> - ALIGN(prev_bank_end, PAGES_PER_SECTION)); >> + start = min(start, >> + ALIGN(prev_end, PAGES_PER_SECTION)); >> #else >> /* >> * Align down here since the VM subsystem insists that the >> * memmap entries are valid from the bank start aligned to >> * MAX_ORDER_NR_PAGES. >> */ >> - bank_start = round_down(bank_start, MAX_ORDER_NR_PAGES); >> + start = round_down(start, MAX_ORDER_NR_PAGES); >> #endif >> /* >> * If we had a previous bank, and there is a space >> * between the current bank and the previous, free it. >> */ >> - if (prev_bank_end && prev_bank_end < bank_start) >> - free_memmap(prev_bank_end, bank_start); >> + if (prev_end && prev_end < start) >> + free_memmap(prev_end, start); >> >> /* >> * Align up here since the VM subsystem insists that the >> * memmap entries are valid from the bank end aligned to >> * MAX_ORDER_NR_PAGES. >> */ >> - prev_bank_end = ALIGN(bank_pfn_end(bank), MAX_ORDER_NR_PAGES); >> + prev_end = ALIGN(start + __phys_to_pfn(reg->size), > > I think, start + __phys_to_pfn(reg->size) can be replaced by > memblock_region_memory_end_pfn(). >
Okay
>> + MAX_ORDER_NR_PAGES); >> } >> >> #ifdef CONFIG_SPARSEMEM >> - if (!IS_ALIGNED(prev_bank_end, PAGES_PER_SECTION)) >> - free_memmap(prev_bank_end, >> - ALIGN(prev_bank_end, PAGES_PER_SECTION)); >> + if (!IS_ALIGNED(prev_end, PAGES_PER_SECTION)) >> + free_memmap(prev_end, >> + ALIGN(prev_end, PAGES_PER_SECTION)); >> #endif >> } >> >> @@ -536,7 +519,7 @@ void __init mem_init(void) >> set_max_mapnr(pfn_to_page(max_pfn) - mem_map); >> >> /* this will put all unused low memory onto the freelists */ >> - free_unused_memmap(&meminfo); >> + free_unused_memmap(); >> free_all_bootmem(); >> >> #ifdef CONFIG_SA1111 >> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c >> index 4f08c13..23433ef 100644 >> --- a/arch/arm/mm/mmu.c >> +++ b/arch/arm/mm/mmu.c >> @@ -1046,74 +1046,44 @@ phys_addr_t arm_lowmem_limit __initdata = 0; >> void __init sanity_check_meminfo(void) >> { >> phys_addr_t memblock_limit = 0; >> - int i, j, highmem = 0; >> + int highmem = 0; >> phys_addr_t vmalloc_limit = __pa(vmalloc_min - 1) + 1; >> + struct memblock_region *reg; >> >> - for (i = 0, j = 0; i < meminfo.nr_banks; i++) { >> - struct membank *bank = &meminfo.bank[j]; >> - phys_addr_t size_limit; >> - >> - *bank = meminfo.bank[i]; >> - size_limit = bank->size; >> + for_each_memblock(memory, reg) { >> + phys_addr_t block_start = reg->base; >> + phys_addr_t block_end = reg->base + reg->size; >> + phys_addr_t size_limit = reg->size; >> >> - if (bank->start >= vmalloc_limit) >> + if (reg->base >= vmalloc_limit) >> highmem = 1; >> else >> - size_limit = vmalloc_limit - bank->start; >> + size_limit = vmalloc_limit - reg->base; >> >> - bank->highmem = highmem; >> >> -#ifdef CONFIG_HIGHMEM >> - /* >> - * Split those memory banks which are partially overlapping >> - * the vmalloc area greatly simplifying things later. >> - */ >> - if (!highmem && bank->size > size_limit) { >> - if (meminfo.nr_banks >= NR_BANKS) { >> - printk(KERN_CRIT "NR_BANKS too low, " >> - "ignoring high memory\n"); >> - } else { >> - memmove(bank + 1, bank, >> - (meminfo.nr_banks - i) * sizeof(*bank)); >> - meminfo.nr_banks++; >> - i++; >> - bank[1].size -= size_limit; >> - bank[1].start = vmalloc_limit; >> - bank[1].highmem = highmem = 1; >> - j++; >> + if (!IS_ENABLED(CONFIG_HIGHMEM) || cache_is_vipt_aliasing()) { >> + >> + if (highmem) { >> + pr_notice("Ignoring ram at %pa-%pa (!CONFIG_HIGHMEM)\n", >> + &block_start, &block_end); >> + memblock_remove(block_start, block_end); > > The wrong size is used here, should be => memblock_remove(block_start, reg->size); > or => memblock_remove(block_start, size_limit); >
Yes, you are correct. I'm guessing I meant to do block_end-block_start and missed the last part.
>> + continue; >> } >> - bank->size = size_limit; >> - } >> -#else >> - /* >> - * Highmem banks not allowed with !CONFIG_HIGHMEM. >> - */ >> - if (highmem) { >> - printk(KERN_NOTICE "Ignoring RAM at %.8llx-%.8llx " >> - "(!CONFIG_HIGHMEM).\n", >> - (unsigned long long)bank->start, >> - (unsigned long long)bank->start + bank->size - 1); >> - continue; >> - } >> >> - /* >> - * Check whether this memory bank would partially overlap >> - * the vmalloc area. >> - */ >> - if (bank->size > size_limit) { >> - printk(KERN_NOTICE "Truncating RAM at %.8llx-%.8llx " >> - "to -%.8llx (vmalloc region overlap).\n", >> - (unsigned long long)bank->start, >> - (unsigned long long)bank->start + bank->size - 1, >> - (unsigned long long)bank->start + size_limit - 1); >> - bank->size = size_limit; >> + if (reg->size > size_limit) { >> + phys_addr_t overlap_size = reg->size - size_limit; >> + >> + pr_notice("Truncating RAM at %pa-%pa to -%pa", >> + &block_start, &block_end, &overlap_size); > > Pls, change it back to show new RAM limit instead of size. > pr_notice("Truncating RAM at %pa-%pa to -%pa", > &block_start, &block_end, &vmalloc_limit); > > >> + memblock_remove(vmalloc_limit, overlap_size); >> + block_end = vmalloc_limit; >> + } >> } >> -#endif >> - if (!bank->highmem) { >> - phys_addr_t bank_end = bank->start + bank->size; >> >> - if (bank_end > arm_lowmem_limit) >> - arm_lowmem_limit = bank_end; >> + if (!highmem) { >> + if (block_end > arm_lowmem_limit) >> + arm_lowmem_limit = reg->base + size_limit; >> + > > if !highmem then size_limit will be calculated as vmalloc_limit - reg->base > which in turn can be greater than reg->size. So, arm_lowmem_limit can point on > non existent memory address. > > Seems, it should be: > arm_lowmem_limit = block_end; >
Yes, I believe so. I will review it.
>> >> /* >> * Find the first non-section-aligned page, and point >> @@ -1129,35 +1099,16 @@ void __init sanity_check_meminfo(void) >> * occurs before any free memory is mapped. >> */ >> if (!memblock_limit) { >> - if (!IS_ALIGNED(bank->start, SECTION_SIZE)) >> - memblock_limit = bank->start; >> - else if (!IS_ALIGNED(bank_end, SECTION_SIZE)) >> - memblock_limit = bank_end; > [...] > > Thanks for your patience :) >
Thanks for the review and debugging!
> Regards, > -grygorii >
Thanks, Laura
-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
| |