Hi, On Thu, 19 Oct 2006 16:16:12 +0100 Andy Whitcroft <apw@xxxxxxxxxxxx> wrote: > KAMEZAWA Hiroyuki wrote: > > #elif defined(CONFIG_SPARSEMEM) > > +#ifndef CONFIG_VMEMMAP_SPARSEMEM > > Ok, this is a sub-type of sparsemem, we already have one called extreme > and that is called CONFIG_SPARSMEM_EXTREME so it seems sensible to stay > with this namespace, and call this CONFIG_SPARSEMEM_VMEMMAP. > looks better. I'll rename. > > +#else /* CONFIG_VMEMMAP_SPARSEMEM */ > > + > > +#define __pfn_to_page(pfn) (virt_memmap_start + (pfn)) > > +#define __page_to_pfn(pg) ((unsigned long)((pg) - virt_memmap_start)) > > + > > +#endif /* CONFIG_VMEMMAP_SPARSEMEM */ > > #endif /* CONFIG_FLATMEM/DISCONTIGMEM/SPARSEMEM */ > > Could we not leverage the standard infrastructure here. It almost feels > like if __section_mem_map_addr just returned virt_memmap_start then > things would just come out the same with the compiler able to optimse > things away. It would stop us having to change this above section which > would perhaps seem nicer? I've not looked at all the other users of it > to see if that would defeat the rest of sparsemem, so I may be talking > out of my hat. > Hm, Okay. I'll try it in the next time and check how it looks. > > + > > +#ifdef CONFIG_VMEMMAP_SPARSEMEM > > +extern struct page *virt_memmap_start; > > +extern void init_vmemmap_sparsemem(void *addr); > > +#else > > +#define init_vmemmap_sparsemem(addr) do{}while(0) > > +#endif > > + > > The existing initialisation function for sparsemem is sparse_init(). It > seems that this one should follow the same scheme if we are part of > sparsemem. sparse_vmemmap_init() perhaps, though as this is defining > the address of it perhaps, sparse_vmemmap_base() or > sparse_vmemmap_setbase(). > Okay. I have another idea, which Chiristoph mentioned, to make start address of vmemmap to be constant value. If doing so, this call can be removed. > > +void init_vmemmap_sparsemem(void *start_addr) > > +{ > > + virt_memmap_start = start_addr; > > +} > > + > > +void *pte_alloc_vmemmap(int node) > > +{ > > + void *ret; > > + if (system_state == SYSTEM_BOOTING) { > > + ret = alloc_bootmem_pages_node(NODE_DATA(node), PAGE_SIZE); > > + } else { > > + ret = kmalloc_node(PAGE_SIZE, GFP_KERNEL, node); > > + memset(ret, 0 , PAGE_SIZE); > > + } > > + BUG_ON(!ret); > > + return ret; > > +} > > Hmmm, this routine is not __init, but is calling an __init function. I > assume its safe under the system_state switcheroo, but the tools will > barf about the difference. Is there a way to mark this up as ok > (assuming it is). Maybe my mistake is to handle booting case and memory-hot-add case in a patch. And I'll add __init or __meminit to suitable place in the next time. > > > +/* > > + * At Hot-add, vmalloc'ed memmap will never call this. > > + * They have been already in suitable address. > > + * Called only when map is allocated by alloc_bootmem()/alloc_pages() > > They will? By who? > If they alloc one it has to be placed in the real > virtual map in VMEMAP mode else it won't be found by pfn_to_page and > family. I assume I am missing the point of this comment. Could you > explain more fully ... Or perhaps this is a bit which is not right yet > as you do say in the heading that hotplug is not right? > Sorry...What I wanted to say here was that vmalloced memmap by memory-hotplug cannot be handled by this routine. I'll divide memory-hotplug case from this patch to make things clearer. > > + */ > > +static void map_virtual_memmap(unsigned long section, void *map, int node) > > +{ > > + unsigned long vmap_start, vmap_end, vmap; > > + unsigned long pfn; > > + void *pg; > > + pgd_t *pgd; > > + pud_t *pud; > > + pmd_t *pmd; > > + pte_t *pte; > > + > > + BUG_ON (!virt_memmap_start); > > + > > + pfn = section_nr_to_pfn(section); > > + vmap_start = (unsigned long)(virt_memmap_start + pfn); > > + vmap_end = (unsigned long)(vmap_start + sizeof(struct page) * PAGES_PER_SECTION); > > + > > + for (vmap = vmap_start; vmap < vmap_end; vmap += PAGE_SIZE, map += PAGE_SIZE) > > + { > > + pgd = pgd_offset_k(vmap); > > + if (pgd_none(*pgd)) { > > + pg = pte_alloc_vmemmap(node); > > + pgd_populate(&init_mm, pgd, pg); > > + } > > + pud = pud_offset(pgd, vmap); > > + if (pud_none(*pud)) { > > + pg = pte_alloc_vmemmap(node); > > + pud_populate(&init_mm, pud, pg); > > + } > > + pmd = pmd_offset(pud, vmap); > > + if (pmd_none(*pmd)) { > > + pg = pte_alloc_vmemmap(node); > > + pmd_populate_kernel(&init_mm, pmd, pg); > > + } > > + pte = pte_offset_kernel(pmd, vmap); > > + if (pte_none(*pte)) > > + set_pte(pte, pfn_pte(__pa(map) >> PAGE_SHIFT, PAGE_KERNEL)); > > + } > > + return; > > +} > > Its nice to see that this is generic as we can then add large page > support for instance where applicable. Are there really no helpers in > the world to make this less 'wordy'. > > We use this in the fault handler, are we using the above because we > want to assure numa locality of the allocations? (Which would be valid.) > yes, for NUMA allocation. and for using alloc_bootmem(). > > ms->section_mem_map &= ~SECTION_MAP_MASK; > > ms->section_mem_map |= sparse_encode_mem_map(mem_map, pnum); > > + map_virtual_memmap(pnum, mem_map, nid); > > We seem to be using mem_map in sparse.c for the mem map, so perhaps this > should be map_virtual_mem_map(), or map_vmap_mem_map() or something? Okay, will rename. > > > > return 1; > > } > > @@ -214,10 +289,11 @@ > > page = alloc_pages(GFP_KERNEL, get_order(memmap_size)); > > if (page) > > goto got_map_page; > > - > > +#ifndef CONFIG_VMEMMAP_SPARSEMEM > > ret = vmalloc(memmap_size); > > if (ret) > > goto got_map_ptr; > > +#endif > > I assume we need this because its not really a good thing to have pages > allocated which are already mapped as you are going to map them > elsewhere? Yes? T Yes. > his only seems to be used from hotplug, so I'll defer to Dave. I'll add hotplug handling later. > > - ret = sparse_init_one_section(ms, section_nr, memmap); > > + ret = sparse_init_one_section(ms, section_nr, memmap, zone->zone_pgdat->node_id); > > In sparse_add_one_section() we already have the pgdat in a local, so > this would better be pgdat->node_id. > Okay, thanks. Thank you for comments. I'll refresh this patch. Regards, -Kame - To unsubscribe from this list: send the line "unsubscribe linux-ia64" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
![]() |