From: Pavel Tatashin <hidden> Date: 2017-09-20 20:18:45
Changelog:
v9 - v8
- Addressed comments raised by Mark Rutland and Ard Biesheuvel: changed
kasan implementation. Added a new function: kasan_map_populate() that
zeroes the allocated and mapped memory
v8 - v7
- Added Acked-by's from Dave Miller for SPARC changes
- Fixed a minor compiling issue on tile architecture reported by kbuild
v7 - v6
- Addressed comments from Michal Hocko
- memblock_discard() patch was removed from this series and integrated
separately
- Fixed bug reported by kbuild test robot new patch:
mm: zero reserved and unavailable struct pages
- Removed patch
x86/mm: reserve only exiting low pages
As, it is not needed anymore, because of the previous fix
- Re-wrote deferred_init_memmap(), found and fixed an existing bug, where
page variable is not reset when zone holes present.
- Merged several patches together per Michal request
- Added performance data including raw logs
v6 - v5
- Fixed ARM64 + kasan code, as reported by Ard Biesheuvel
- Tested ARM64 code in qemu and found few more issues, that I fixed in this
iteration
- Added page roundup/rounddown to x86 and arm zeroing routines to zero the
whole allocated range, instead of only provided address range.
- Addressed SPARC related comment from Sam Ravnborg
- Fixed section mismatch warnings related to memblock_discard().
v5 - v4
- Fixed build issues reported by kbuild on various configurations
v4 - v3
- Rewrote code to zero sturct pages in __init_single_page() as
suggested by Michal Hocko
- Added code to handle issues related to accessing struct page
memory before they are initialized.
v3 - v2
- Addressed David Miller comments about one change per patch:
* Splited changes to platforms into 4 patches
* Made "do not zero vmemmap_buf" as a separate patch
v2 - v1
- Per request, added s390 to deferred "struct page" zeroing
- Collected performance data on x86 which proofs the importance to
keep memset() as prefetch (see below).
SMP machines can benefit from the DEFERRED_STRUCT_PAGE_INIT config option,
which defers initializing struct pages until all cpus have been started so
it can be done in parallel.
However, this feature is sub-optimal, because the deferred page
initialization code expects that the struct pages have already been zeroed,
and the zeroing is done early in boot with a single thread only. Also, we
access that memory and set flags before struct pages are initialized. All
of this is fixed in this patchset.
In this work we do the following:
- Never read access struct page until it was initialized
- Never set any fields in struct pages before they are initialized
- Zero struct page at the beginning of struct page initialization
==========================================================================
Performance improvements on x86 machine with 8 nodes:
Intel(R) Xeon(R) CPU E7-8895 v3 @ 2.60GHz and 1T of memory:
TIME SPEED UP
base no deferred: 95.796233s
fix no deferred: 79.978956s 19.77%
base deferred: 77.254713s
fix deferred: 55.050509s 40.34%
==========================================================================
SPARC M6 3600 MHz with 15T of memory
TIME SPEED UP
base no deferred: 358.335727s
fix no deferred: 302.320936s 18.52%
base deferred: 237.534603s
fix deferred: 182.103003s 30.44%
==========================================================================
Raw dmesg output with timestamps:
x86 base no deferred: https://hastebin.com/ofunepurit.scala
x86 base deferred: https://hastebin.com/ifazegeyas.scala
x86 fix no deferred: https://hastebin.com/pegocohevo.scala
x86 fix deferred: https://hastebin.com/ofupevikuk.scala
sparc base no deferred: https://hastebin.com/ibobeteken.go
sparc base deferred: https://hastebin.com/fariqimiyu.go
sparc fix no deferred: https://hastebin.com/muhegoheyi.go
sparc fix deferred: https://hastebin.com/xadinobutu.go
Pavel Tatashin (12):
x86/mm: setting fields in deferred pages
sparc64/mm: setting fields in deferred pages
mm: deferred_init_memmap improvements
sparc64: simplify vmemmap_populate
mm: defining memblock_virt_alloc_try_nid_raw
mm: zero struct pages during initialization
sparc64: optimized struct page zeroing
mm: zero reserved and unavailable struct pages
mm/kasan: kasan specific map populate function
x86/kasan: use kasan_map_populate()
arm64/kasan: use kasan_map_populate()
mm: stop zeroing memory during allocation in vmemmap
arch/arm64/include/asm/pgtable.h | 3 +
arch/arm64/mm/kasan_init.c | 12 +--
arch/sparc/include/asm/pgtable_64.h | 30 ++++++
arch/sparc/mm/init_64.c | 31 +++---
arch/x86/mm/init_64.c | 9 +-
arch/x86/mm/kasan_init_64.c | 8 +-
include/linux/bootmem.h | 27 +++++
include/linux/kasan.h | 2 +
include/linux/memblock.h | 16 +++
include/linux/mm.h | 26 +++++
mm/kasan/kasan_init.c | 67 ++++++++++++
mm/memblock.c | 60 +++++++++--
mm/page_alloc.c | 207 ++++++++++++++++++++----------------
mm/sparse-vmemmap.c | 15 ++-
mm/sparse.c | 6 +-
15 files changed, 380 insertions(+), 139 deletions(-)
--
2.14.1
From: Pavel Tatashin <hidden> Date: 2017-09-20 20:18:44
Remove duplicating code by using common functions
vmemmap_pud_populate and vmemmap_pgd_populate.
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
Acked-by: David S. Miller <davem@davemloft.net>
---
arch/sparc/mm/init_64.c | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)
@@ -2651,30 +2651,19 @@ int __meminit vmemmap_populate(unsigned long vstart, unsigned long vend,vstart=vstart&PMD_MASK;vend=ALIGN(vend,PMD_SIZE);for(;vstart<vend;vstart+=PMD_SIZE){-pgd_t*pgd=pgd_offset_k(vstart);+pgd_t*pgd=vmemmap_pgd_populate(vstart,node);unsignedlongpte;pud_t*pud;pmd_t*pmd;-if(pgd_none(*pgd)){-pud_t*new=vmemmap_alloc_block(PAGE_SIZE,node);+if(!pgd)+return-ENOMEM;-if(!new)-return-ENOMEM;-pgd_populate(&init_mm,pgd,new);-}--pud=pud_offset(pgd,vstart);-if(pud_none(*pud)){-pmd_t*new=vmemmap_alloc_block(PAGE_SIZE,node);--if(!new)-return-ENOMEM;-pud_populate(&init_mm,pud,new);-}+pud=vmemmap_pud_populate(pgd,vstart,node);+if(!pud)+return-ENOMEM;pmd=pmd_offset(pud,vstart);-pte=pmd_val(*pmd);if(!(pte&_PAGE_VALID)){void*block=vmemmap_alloc_block(PMD_SIZE,node);
From: Pavel Tatashin <hidden> Date: 2017-09-20 20:18:48
Without deferred struct page feature (CONFIG_DEFERRED_STRUCT_PAGE_INIT),
flags and other fields in "struct page"es are never changed prior to first
initializing struct pages by going through __init_single_page().
With deferred struct page feature enabled, however, we set fields in
register_page_bootmem_info that are subsequently clobbered right after in
free_all_bootmem:
mem_init() {
register_page_bootmem_info();
free_all_bootmem();
...
}
When register_page_bootmem_info() is called only non-deferred struct pages
are initialized. But, this function goes through some reserved pages which
might be part of the deferred, and thus are not yet initialized.
mem_init
register_page_bootmem_info
register_page_bootmem_info_node
get_page_bootmem
.. setting fields here ..
such as: page->freelist = (void *)type;
free_all_bootmem()
free_low_memory_core_early()
for_each_reserved_mem_region()
reserve_bootmem_region()
init_reserved_page() <- Only if this is deferred reserved page
__init_single_pfn()
__init_single_page()
memset(0) <-- Loose the set fields here
We end-up with issue where, currently we do not observe problem as memory
is explicitly zeroed. But, if flag asserts are changed we can start hitting
issues.
Also, because in this patch series we will stop zeroing struct page memory
during allocation, we must make sure that struct pages are properly
initialized prior to using them.
The deferred-reserved pages are initialized in free_all_bootmem().
Therefore, the fix is to switch the above calls.
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
---
arch/x86/mm/init_64.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
@@ -1182,12 +1182,17 @@ void __init mem_init(void)/* clear_bss() already clear the empty_zero_page */-register_page_bootmem_info();-/* this will put all memory onto the freelists */free_all_bootmem();after_bootmem=1;+/* Must be done after boot memory is put on freelist, because here we+*mightsetfieldsindeferredstructpagesthathavenotyetbeen+*initialized,andfree_all_bootmem()initializesallthereserved+*deferredpagesforus.+*/+register_page_bootmem_info();+/* Register memory areas for /proc/kcore */kclist_add(&kcore_vsyscall,(void*)VSYSCALL_ADDR,PAGE_SIZE,KCORE_OTHER);
From: Pavel Tatashin <hidden> Date: 2017-09-20 20:18:53
* A new variant of memblock_virt_alloc_* allocations:
memblock_virt_alloc_try_nid_raw()
- Does not zero the allocated memory
- Does not panic if request cannot be satisfied
* optimize early system hash allocations
Clients can call alloc_large_system_hash() with flag: HASH_ZERO to specify
that memory that was allocated for system hash needs to be zeroed,
otherwise the memory does not need to be zeroed, and client will initialize
it.
If memory does not need to be zero'd, call the new
memblock_virt_alloc_raw() interface, and thus improve the boot performance.
* debug for raw alloctor
When CONFIG_DEBUG_VM is enabled, this patch sets all the memory that is
returned by memblock_virt_alloc_try_nid_raw() to ones to ensure that no
places excpect zeroed memory.
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
Acked-by: Michal Hocko <mhocko@suse.com>
---
include/linux/bootmem.h | 27 ++++++++++++++++++++++
mm/memblock.c | 60 +++++++++++++++++++++++++++++++++++++++++++------
mm/page_alloc.c | 15 ++++++-------
3 files changed, 87 insertions(+), 15 deletions(-)
@@ -160,6 +160,9 @@ extern void *__alloc_bootmem_low_node(pg_data_t *pgdat,#define BOOTMEM_ALLOC_ANYWHERE (~(phys_addr_t)0)/* FIXME: Move to memblock.h at a point where we remove nobootmem.c */+void*memblock_virt_alloc_try_nid_raw(phys_addr_tsize,phys_addr_talign,+phys_addr_tmin_addr,+phys_addr_tmax_addr,intnid);void*memblock_virt_alloc_try_nid_nopanic(phys_addr_tsize,phys_addr_talign,phys_addr_tmin_addr,phys_addr_tmax_addr,intnid);
From: Pavel Tatashin <hidden> Date: 2017-09-20 20:18:54
vmemmap_alloc_block() will no longer zero the block, so zero memory
at its call sites for everything except struct pages. Struct page memory
is zero'd by struct page initialization.
Replace allocators in sprase-vmemmap to use the non-zeroing version. So,
we will get the performance improvement by zeroing the memory in parallel
when struct pages are zeroed.
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
---
include/linux/mm.h | 11 +++++++++++
mm/sparse-vmemmap.c | 15 +++++++--------
mm/sparse.c | 6 +++---
3 files changed, 21 insertions(+), 11 deletions(-)
From: Pavel Tatashin <hidden> Date: 2017-09-20 20:18:55
To optimize the performance of struct page initialization,
vmemmap_populate() will no longer zero memory.
Therefore, we must use a new interface to allocate and map kasan shadow
memory, that also zeroes memory for us.
Signed-off-by: Pavel Tatashin <redacted>
---
arch/arm64/mm/kasan_init.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Pavel Tatashin <hidden> Date: 2017-09-20 20:19:20
Add struct page zeroing as a part of initialization of other fields in
__init_single_page().
This single thread performance collected on: Intel(R) Xeon(R) CPU E7-8895
v3 @ 2.60GHz with 1T of memory (268400646 pages in 8 nodes):
BASE FIX
sparse_init 11.244671836s 0.007199623s
zone_sizes_init 4.879775891s 8.355182299s
--------------------------
Total 16.124447727s 8.362381922s
sparse_init is where memory for struct pages is zeroed, and the zeroing
part is moved later in this patch into __init_single_page(), which is
called from zone_sizes_init().
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
Acked-by: Michal Hocko <mhocko@suse.com>
---
include/linux/mm.h | 9 +++++++++
mm/page_alloc.c | 1 +
2 files changed, 10 insertions(+)
From: Pavel Tatashin <hidden> Date: 2017-09-20 20:19:52
Some memory is reserved but unavailable: not present in memblock.memory
(because not backed by physical pages), but present in memblock.reserved.
Such memory has backing struct pages, but they are not initialized by going
through __init_single_page().
In some cases these struct pages are accessed even if they do not contain
any data. One example is page_to_pfn() might access page->flags if this is
where section information is stored (CONFIG_SPARSEMEM,
SECTION_IN_PAGE_FLAGS).
Since, struct pages are zeroed in __init_single_page(), and not during
allocation time, we must zero such struct pages explicitly.
The patch involves adding a new memblock iterator:
for_each_resv_unavail_range(i, p_start, p_end)
Which iterates through reserved && !memory lists, and we zero struct pages
explicitly by calling mm_zero_struct_page().
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
---
include/linux/memblock.h | 16 ++++++++++++++++
include/linux/mm.h | 6 ++++++
mm/page_alloc.c | 30 ++++++++++++++++++++++++++++++
3 files changed, 52 insertions(+)
@@ -6202,6 +6202,34 @@ void __paginginit free_area_init_node(int nid, unsigned long *zones_size,free_area_init_core(pgdat);}+#ifdef CONFIG_HAVE_MEMBLOCK+/*+*Onlystructpagesthatarebackedbyphysicalmemoryarezeroedand+*initializedbygoingthrough__init_single_page().But,therearesome+*structpageswhicharereservedinmemblockallocatorandtheirfields+*maybeaccessed(forexamplepage_to_pfn()onsomeconfigurationaccesses+*flags).Wemustexplicitlyzerothosestructpages.+*/+void__paginginitzero_resv_unavail(void)+{+phys_addr_tstart,end;+unsignedlongpfn;+u64i,pgcnt;++/* Loop through ranges that are reserved, but do not have reported+*physicalmemorybacking.+*/+pgcnt=0;+for_each_resv_unavail_range(i,&start,&end){+for(pfn=PFN_DOWN(start);pfn<PFN_UP(end);pfn++){+mm_zero_struct_page(pfn_to_page(pfn));+pgcnt++;+}+}+pr_info("Reserved but unavailable: %lld pages",pgcnt);+}+#endif /* CONFIG_HAVE_MEMBLOCK */+#ifdef CONFIG_HAVE_MEMBLOCK_NODE_MAP#if MAX_NUMNODES > 1
@@ -6625,6 +6653,7 @@ void __init free_area_init_nodes(unsigned long *max_zone_pfn)node_set_state(nid,N_MEMORY);check_for_memory(pgdat,nid);}+zero_resv_unavail();}staticint__initcmdline_parse_core(char*p,unsignedlong*core)
@@ -6788,6 +6817,7 @@ void __init free_area_init(unsigned long *zones_size){free_area_init_node(0,zones_size,__pa(PAGE_OFFSET)>>PAGE_SHIFT,NULL);+zero_resv_unavail();}staticintpage_alloc_cpu_dead(unsignedintcpu)
From: Pavel Tatashin <hidden> Date: 2017-09-20 20:20:27
This patch fixes two issues in deferred_init_memmap
=====
In deferred_init_memmap() where all deferred struct pages are initialized
we have a check like this:
if (page->flags) {
VM_BUG_ON(page_zone(page) != zone);
goto free_range;
}
This way we are checking if the current deferred page has already been
initialized. It works, because memory for struct pages has been zeroed, and
the only way flags are not zero if it went through __init_single_page()
before. But, once we change the current behavior and won't zero the memory
in memblock allocator, we cannot trust anything inside "struct page"es
until they are initialized. This patch fixes this.
The deferred_init_memmap() is re-written to loop through only free memory
ranges provided by memblock.
=====
This patch fixes another existing issue on systems that have holes in
zones i.e CONFIG_HOLES_IN_ZONE is defined.
In for_each_mem_pfn_range() we have code like this:
if (!pfn_valid_within(pfn)
goto free_range;
Note: 'page' is not set to NULL and is not incremented but 'pfn' advances.
Thus means if deferred struct pages are enabled on systems with these kind
of holes, linux would get memory corruptions. I have fixed this issue by
defining a new macro that performs all the necessary operations when we
free the current set of pages.
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
---
mm/page_alloc.c | 161 +++++++++++++++++++++++++++-----------------------------
1 file changed, 78 insertions(+), 83 deletions(-)
@@ -1410,14 +1410,17 @@ void clear_zone_contiguous(struct zone *zone)}#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT-staticvoid__initdeferred_free_range(structpage*page,-unsignedlongpfn,intnr_pages)+staticvoid__initdeferred_free_range(unsignedlongpfn,+unsignedlongnr_pages){-inti;+structpage*page;+unsignedlongi;-if(!page)+if(!nr_pages)return;+page=pfn_to_page(pfn);+/* Free a large naturally-aligned chunk if possible */if(nr_pages==pageblock_nr_pages&&(pfn&(pageblock_nr_pages-1))==0){
@@ -1443,19 +1446,82 @@ static inline void __init pgdat_init_report_one_done(void)complete(&pgdat_init_all_done_comp);}+#define DEFERRED_FREE(nr_free, free_base_pfn, page) \+({\+unsignedlongnr=(nr_free);\+\+deferred_free_range((free_base_pfn),(nr));\+(free_base_pfn)=0;\+(nr_free)=0;\+page=NULL;\+nr;\+})++staticunsignedlongdeferred_init_range(intnid,intzid,unsignedlongpfn,+unsignedlongend_pfn)+{+structmminit_pfnnid_cachenid_init_state={};+unsignedlongnr_pgmask=pageblock_nr_pages-1;+unsignedlongfree_base_pfn=0;+unsignedlongnr_pages=0;+unsignedlongnr_free=0;+structpage*page=NULL;++for(;pfn<end_pfn;pfn++){+/*+*Firstwecheckifpfnisvalidonarchitectureswhereitis+*possibletohaveholeswithinpageblock_nr_pages.Onsystems+*whereitisnotpossible,thisfunctionisoptimizedout.+*+*Then,wecheckifacurrentlargepageisvalidbyonly+*checkingthevalidityoftheheadpfn.+*+*meminit_pfn_in_nidischeckedonsystemswherepfnscan+*interleavewithinanode:apfnisbetweenstartandend+*ofanode,butdoesnotbelongtothismemorynode.+*+*Finally,weminimizepfnpagelookupsandschedulerchecksby+*performingitonlyonceeverypageblock_nr_pages.+*/+if(!pfn_valid_within(pfn)){+nr_pages+=DEFERRED_FREE(nr_free,free_base_pfn,page);+}elseif(!(pfn&nr_pgmask)&&!pfn_valid(pfn)){+nr_pages+=DEFERRED_FREE(nr_free,free_base_pfn,page);+}elseif(!meminit_pfn_in_nid(pfn,nid,&nid_init_state)){+nr_pages+=DEFERRED_FREE(nr_free,free_base_pfn,page);+}elseif(page&&(pfn&nr_pgmask)){+page++;+__init_single_page(page,pfn,zid,nid);+nr_free++;+}else{+nr_pages+=DEFERRED_FREE(nr_free,free_base_pfn,page);+page=pfn_to_page(pfn);+__init_single_page(page,pfn,zid,nid);+free_base_pfn=pfn;+nr_free=1;+cond_resched();+}+}+/* Free the last block of pages to allocator */+nr_pages+=DEFERRED_FREE(nr_free,free_base_pfn,page);++returnnr_pages;+}+/* Initialise remaining memory on a node */staticint__initdeferred_init_memmap(void*data){pg_data_t*pgdat=data;intnid=pgdat->node_id;-structmminit_pfnnid_cachenid_init_state={};unsignedlongstart=jiffies;unsignedlongnr_pages=0;-unsignedlongwalk_start,walk_end;-inti,zid;+unsignedlongspfn,epfn;+phys_addr_tspa,epa;+intzid;structzone*zone;unsignedlongfirst_init_pfn=pgdat->first_deferred_pfn;conststructcpumask*cpumask=cpumask_of_node(pgdat->node_id);+u64i;if(first_init_pfn==ULONG_MAX){pgdat_init_report_one_done();
@@ -1477,83 +1543,12 @@ static int __init deferred_init_memmap(void *data)if(first_init_pfn<zone_end_pfn(zone))break;}+first_init_pfn=max(zone->zone_start_pfn,first_init_pfn);-for_each_mem_pfn_range(i,nid,&walk_start,&walk_end,NULL){-unsignedlongpfn,end_pfn;-structpage*page=NULL;-structpage*free_base_page=NULL;-unsignedlongfree_base_pfn=0;-intnr_to_free=0;--end_pfn=min(walk_end,zone_end_pfn(zone));-pfn=first_init_pfn;-if(pfn<walk_start)-pfn=walk_start;-if(pfn<zone->zone_start_pfn)-pfn=zone->zone_start_pfn;--for(;pfn<end_pfn;pfn++){-if(!pfn_valid_within(pfn))-gotofree_range;--/*-*Ensurepfn_validischeckedevery-*pageblock_nr_pagesformemoryholes-*/-if((pfn&(pageblock_nr_pages-1))==0){-if(!pfn_valid(pfn)){-page=NULL;-gotofree_range;-}-}--if(!meminit_pfn_in_nid(pfn,nid,&nid_init_state)){-page=NULL;-gotofree_range;-}--/* Minimise pfn page lookups and scheduler checks */-if(page&&(pfn&(pageblock_nr_pages-1))!=0){-page++;-}else{-nr_pages+=nr_to_free;-deferred_free_range(free_base_page,-free_base_pfn,nr_to_free);-free_base_page=NULL;-free_base_pfn=nr_to_free=0;--page=pfn_to_page(pfn);-cond_resched();-}--if(page->flags){-VM_BUG_ON(page_zone(page)!=zone);-gotofree_range;-}--__init_single_page(page,pfn,zid,nid);-if(!free_base_page){-free_base_page=page;-free_base_pfn=pfn;-nr_to_free=0;-}-nr_to_free++;--/* Where possible, batch up pages for a single free */-continue;-free_range:-/* Free the current block of pages to allocator */-nr_pages+=nr_to_free;-deferred_free_range(free_base_page,free_base_pfn,-nr_to_free);-free_base_page=NULL;-free_base_pfn=nr_to_free=0;-}-/* Free the last block of pages to allocator */-nr_pages+=nr_to_free;-deferred_free_range(free_base_page,free_base_pfn,nr_to_free);--first_init_pfn=max(end_pfn,first_init_pfn);+for_each_free_mem_range(i,nid,MEMBLOCK_NONE,&spa,&epa,NULL){+spfn=max_t(unsignedlong,first_init_pfn,PFN_UP(spa));+epfn=min_t(unsignedlong,zone_end_pfn(zone),PFN_DOWN(epa));+nr_pages+=deferred_init_range(nid,zid,spfn,epfn);}/* Sanity check that the next zone really is unpopulated */
From: Pavel Tatashin <hidden> Date: 2017-09-20 20:20:55
To optimize the performance of struct page initialization,
vmemmap_populate() will no longer zero memory.
Therefore, we must use a new interface to allocate and map kasan shadow
memory, that also zeroes memory for us.
Signed-off-by: Pavel Tatashin <redacted>
---
arch/x86/mm/kasan_init_64.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -23,7 +23,7 @@ static int __init map_range(struct range *range)start=(unsignedlong)kasan_mem_to_shadow(pfn_to_kaddr(range->start));end=(unsignedlong)kasan_mem_to_shadow(pfn_to_kaddr(range->end));-returnvmemmap_populate(start,end,NUMA_NO_NODE);+returnkasan_map_populate(start,end,NUMA_NO_NODE);}staticvoid__initclear_pgds(unsignedlongstart,
From: Pavel Tatashin <hidden> Date: 2017-09-20 20:21:11
During early boot, kasan uses vmemmap_populate() to establish its shadow
memory. But, that interface is intended for struct pages use.
Because of the current project, vmemmap won't be zeroed during allocation,
but kasan expects that memory to be zeroed. We are adding a new
kasan_map_populate() function to resolve this difference.
Signed-off-by: Pavel Tatashin <redacted>
---
arch/arm64/include/asm/pgtable.h | 3 ++
include/linux/kasan.h | 2 ++
mm/kasan/kasan_init.c | 67 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 72 insertions(+)
@@ -197,3 +197,70 @@ void __init kasan_populate_zero_shadow(const void *shadow_start,zero_p4d_populate(pgd,addr,next);}while(pgd++,addr=next,addr!=end);}++/* Creates mappings for kasan during early boot. The mapped memory is zeroed */+int__meminitkasan_map_populate(unsignedlongstart,unsignedlongend,+intnode)+{+unsignedlongaddr,pfn,next;+unsignedlonglongsize;+pgd_t*pgd;+p4d_t*p4d;+pud_t*pud;+pmd_t*pmd;+pte_t*pte;+intret;++ret=vmemmap_populate(start,end,node);+/*+*Wemighthavepartiallypopulatedmemory,socheckfornoentries,+*andzeroonlythosethatactuallyexist.+*/+for(addr=start;addr<end;addr=next){+pgd=pgd_offset_k(addr);+if(pgd_none(*pgd)){+next=pgd_addr_end(addr,end);+continue;+}++p4d=p4d_offset(pgd,addr);+if(p4d_none(*p4d)){+next=p4d_addr_end(addr,end);+continue;+}++pud=pud_offset(p4d,addr);+if(pud_none(*pud)){+next=pud_addr_end(addr,end);+continue;+}+if(pud_large(*pud)){+/* This is PUD size page */+next=pud_addr_end(addr,end);+size=PUD_SIZE;+pfn=pud_pfn(*pud);+}else{+pmd=pmd_offset(pud,addr);+if(pmd_none(*pmd)){+next=pmd_addr_end(addr,end);+continue;+}+if(pmd_large(*pmd)){+/* This is PMD size page */+next=pmd_addr_end(addr,end);+size=PMD_SIZE;+pfn=pmd_pfn(*pmd);+}else{+pte=pte_offset_kernel(pmd,addr);+next=addr+PAGE_SIZE;+if(pte_none(*pte))+continue;+/* This is base size page */+size=PAGE_SIZE;+pfn=pte_pfn(*pte);+}+}+memset(phys_to_virt(PFN_PHYS(pfn)),0,size);+}+returnret;+}
From: Pavel Tatashin <hidden> Date: 2017-09-20 20:21:27
Add an optimized mm_zero_struct_page(), so struct page's are zeroed without
calling memset(). We do eight to ten regular stores based on the size of
struct page. Compiler optimizes out the conditions of switch() statement.
SPARC-M6 with 15T of memory, single thread performance:
BASE FIX OPTIMIZED_FIX
bootmem_init 28.440467985s 2.305674818s 2.305161615s
free_area_init_nodes 202.845901673s 225.343084508s 172.556506560s
--------------------------------------------
Total 231.286369658s 227.648759326s 174.861668175s
BASE: current linux
FIX: This patch series without "optimized struct page zeroing"
OPTIMIZED_FIX: This patch series including the current patch.
bootmem_init() is where memory for struct pages is zeroed during
allocation. Note, about two seconds in this function is a fixed time: it
does not increase as memory is increased.
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
Acked-by: David S. Miller <davem@davemloft.net>
---
arch/sparc/include/asm/pgtable_64.h | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
@@ -230,6 +230,36 @@ extern unsigned long _PAGE_ALL_SZ_BITS;externstructpage*mem_map_zero;#define ZERO_PAGE(vaddr) (mem_map_zero)+/* This macro must be updated when the size of struct page grows above 80+*orreducesbelow64.+*Theideathatcompileroptimizesoutswitch()statement,andonly+*leavesclrxinstructions+*/+#define mm_zero_struct_page(pp) do { \+unsignedlong*_pp=(void*)(pp);\+\+/* Check that struct page is either 64, 72, or 80 bytes */\+BUILD_BUG_ON(sizeof(structpage)&7);\+BUILD_BUG_ON(sizeof(structpage)<64);\+BUILD_BUG_ON(sizeof(structpage)>80);\+\+switch(sizeof(structpage)){\+case80:\+_pp[9]=0;/* fallthrough */\+case72:\+_pp[8]=0;/* fallthrough */\+default:\+_pp[7]=0;\+_pp[6]=0;\+_pp[5]=0;\+_pp[4]=0;\+_pp[3]=0;\+_pp[2]=0;\+_pp[1]=0;\+_pp[0]=0;\+}\+}while(0)+/* PFNs are real physical page numbers. However, mem_map only begins to record*per-pageinformationstartingatpfn_base.Thisistohandlesystemswhere*thefirstphysicalpageinthemachineisatsomehugephysicaladdress,
From: Pavel Tatashin <hidden> Date: 2017-09-20 20:21:41
Without deferred struct page feature (CONFIG_DEFERRED_STRUCT_PAGE_INIT),
flags and other fields in "struct page"es are never changed prior to first
initializing struct pages by going through __init_single_page().
With deferred struct page feature enabled there is a case where we set some
fields prior to initializing:
mem_init() {
register_page_bootmem_info();
free_all_bootmem();
...
}
When register_page_bootmem_info() is called only non-deferred struct pages
are initialized. But, this function goes through some reserved pages which
might be part of the deferred, and thus are not yet initialized.
mem_init
register_page_bootmem_info
register_page_bootmem_info_node
get_page_bootmem
.. setting fields here ..
such as: page->freelist = (void *)type;
free_all_bootmem()
free_low_memory_core_early()
for_each_reserved_mem_region()
reserve_bootmem_region()
init_reserved_page() <- Only if this is deferred reserved page
__init_single_pfn()
__init_single_page()
memset(0) <-- Loose the set fields here
We end-up with similar issue as in the previous patch, where currently we
do not observe problem as memory is zeroed. But, if flag asserts are
changed we can start hitting issues.
Also, because in this patch series we will stop zeroing struct page memory
during allocation, we must make sure that struct pages are properly
initialized prior to using them.
The deferred-reserved pages are initialized in free_all_bootmem().
Therefore, the fix is to switch the above calls.
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
Acked-by: David S. Miller <davem@davemloft.net>
---
arch/sparc/mm/init_64.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
@@ -2548,9 +2548,15 @@ void __init mem_init(void){high_memory=__va(last_valid_pfn<<PAGE_SHIFT);-register_page_bootmem_info();free_all_bootmem();+/* Must be done after boot memory is put on freelist, because here we+*mightsetfieldsindeferredstructpagesthathavenotyetbeen+*initialized,andfree_all_bootmem()initializesallthereserved+*deferredpagesforus.+*/+register_page_bootmem_info();+/**Setupthezeropage,markitreserved,sothatpagecount*isnotmanipulatedwhenfreeingthepagefromuserptes.
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-03 12:27:07
On Wed 20-09-17 16:17:03, Pavel Tatashin wrote:
Without deferred struct page feature (CONFIG_DEFERRED_STRUCT_PAGE_INIT),
flags and other fields in "struct page"es are never changed prior to first
initializing struct pages by going through __init_single_page().
With deferred struct page feature enabled, however, we set fields in
register_page_bootmem_info that are subsequently clobbered right after in
free_all_bootmem:
mem_init() {
register_page_bootmem_info();
free_all_bootmem();
...
}
When register_page_bootmem_info() is called only non-deferred struct pages
are initialized. But, this function goes through some reserved pages which
might be part of the deferred, and thus are not yet initialized.
mem_init
register_page_bootmem_info
register_page_bootmem_info_node
get_page_bootmem
.. setting fields here ..
such as: page->freelist = (void *)type;
free_all_bootmem()
free_low_memory_core_early()
for_each_reserved_mem_region()
reserve_bootmem_region()
init_reserved_page() <- Only if this is deferred reserved page
__init_single_pfn()
__init_single_page()
memset(0) <-- Loose the set fields here
We end-up with issue where, currently we do not observe problem as memory
is explicitly zeroed. But, if flag asserts are changed we can start hitting
issues.
Also, because in this patch series we will stop zeroing struct page memory
during allocation, we must make sure that struct pages are properly
initialized prior to using them.
The deferred-reserved pages are initialized in free_all_bootmem().
Therefore, the fix is to switch the above calls.
Thanks for extending the changelog. This is more informative now.
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
I hope I haven't missed anything but it looks good to me.
Acked-by: Michal Hocko <mhocko@suse.com>
one nit below
@@ -1182,12 +1182,17 @@ void __init mem_init(void)/* clear_bss() already clear the empty_zero_page */-register_page_bootmem_info();-/* this will put all memory onto the freelists */free_all_bootmem();after_bootmem=1;+/* Must be done after boot memory is put on freelist, because here we
standard code style is to do
/*
* text starts here
+ * might set fields in deferred struct pages that have not yet been
+ * initialized, and free_all_bootmem() initializes all the reserved
+ * deferred pages for us.
+ */
+ register_page_bootmem_info();
+
/* Register memory areas for /proc/kcore */
kclist_add(&kcore_vsyscall, (void *)VSYSCALL_ADDR,
PAGE_SIZE, KCORE_OTHER);
--
2.14.1
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-03 12:28:28
On Wed 20-09-17 16:17:04, Pavel Tatashin wrote:
Without deferred struct page feature (CONFIG_DEFERRED_STRUCT_PAGE_INIT),
flags and other fields in "struct page"es are never changed prior to first
initializing struct pages by going through __init_single_page().
With deferred struct page feature enabled there is a case where we set some
fields prior to initializing:
mem_init() {
register_page_bootmem_info();
free_all_bootmem();
...
}
When register_page_bootmem_info() is called only non-deferred struct pages
are initialized. But, this function goes through some reserved pages which
might be part of the deferred, and thus are not yet initialized.
mem_init
register_page_bootmem_info
register_page_bootmem_info_node
get_page_bootmem
.. setting fields here ..
such as: page->freelist = (void *)type;
free_all_bootmem()
free_low_memory_core_early()
for_each_reserved_mem_region()
reserve_bootmem_region()
init_reserved_page() <- Only if this is deferred reserved page
__init_single_pfn()
__init_single_page()
memset(0) <-- Loose the set fields here
We end-up with similar issue as in the previous patch, where currently we
do not observe problem as memory is zeroed. But, if flag asserts are
changed we can start hitting issues.
Also, because in this patch series we will stop zeroing struct page memory
during allocation, we must make sure that struct pages are properly
initialized prior to using them.
The deferred-reserved pages are initialized in free_all_bootmem().
Therefore, the fix is to switch the above calls.
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
Acked-by: David S. Miller <davem@davemloft.net>
As you separated x86 and sparc patches doing essentially the same I
assume David is going to take this patch?
Acked-by: Michal Hocko <mhocko@suse.com>
@@ -2548,9 +2548,15 @@ void __init mem_init(void){high_memory=__va(last_valid_pfn<<PAGE_SHIFT);-register_page_bootmem_info();free_all_bootmem();+/* Must be done after boot memory is put on freelist, because here we+*mightsetfieldsindeferredstructpagesthathavenotyetbeen+*initialized,andfree_all_bootmem()initializesallthereserved+*deferredpagesforus.+*/+register_page_bootmem_info();+/**Setupthezeropage,markitreserved,sothatpagecount*isnotmanipulatedwhenfreeingthepagefromuserptes.
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-03 12:58:01
On Wed 20-09-17 16:17:05, Pavel Tatashin wrote:
This patch fixes two issues in deferred_init_memmap
=====
In deferred_init_memmap() where all deferred struct pages are initialized
we have a check like this:
if (page->flags) {
VM_BUG_ON(page_zone(page) != zone);
goto free_range;
}
This way we are checking if the current deferred page has already been
initialized. It works, because memory for struct pages has been zeroed, and
the only way flags are not zero if it went through __init_single_page()
before. But, once we change the current behavior and won't zero the memory
in memblock allocator, we cannot trust anything inside "struct page"es
until they are initialized. This patch fixes this.
The deferred_init_memmap() is re-written to loop through only free memory
ranges provided by memblock.
Please be explicit that this is possible only because we discard
memblock data later after 3010f876500f ("mm: discard memblock data
later"). Also be more explicit how the new code works.
I like how the resulting code is more compact and smaller.
for_each_free_mem_range also looks more appropriate but I really detest
the DEFERRED_FREE thingy. Maybe we can handle all that in a single goto
section. I know this is not an art but manipulating variables from
macros is more error prone and much more ugly IMHO.
=====
This patch fixes another existing issue on systems that have holes in
zones i.e CONFIG_HOLES_IN_ZONE is defined.
In for_each_mem_pfn_range() we have code like this:
if (!pfn_valid_within(pfn)
goto free_range;
Note: 'page' is not set to NULL and is not incremented but 'pfn' advances.
Thus means if deferred struct pages are enabled on systems with these kind
of holes, linux would get memory corruptions. I have fixed this issue by
defining a new macro that performs all the necessary operations when we
free the current set of pages.
please do not use macros. Btw. this deserves its own fix. I suspect that
no CONFIG_HOLES_IN_ZONE arch enables DEFERRED_STRUCT_PAGE_INIT but
purely from the review point of view it should be its own patch.
quoted hunk
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
---
mm/page_alloc.c | 161 +++++++++++++++++++++++++++-----------------------------
1 file changed, 78 insertions(+), 83 deletions(-)
@@ -1410,14 +1410,17 @@ void clear_zone_contiguous(struct zone *zone)}#ifdef CONFIG_DEFERRED_STRUCT_PAGE_INIT-staticvoid__initdeferred_free_range(structpage*page,-unsignedlongpfn,intnr_pages)+staticvoid__initdeferred_free_range(unsignedlongpfn,+unsignedlongnr_pages){-inti;+structpage*page;+unsignedlongi;-if(!page)+if(!nr_pages)return;+page=pfn_to_page(pfn);+/* Free a large naturally-aligned chunk if possible */if(nr_pages==pageblock_nr_pages&&(pfn&(pageblock_nr_pages-1))==0){
@@ -1443,19 +1446,82 @@ static inline void __init pgdat_init_report_one_done(void)complete(&pgdat_init_all_done_comp);}+#define DEFERRED_FREE(nr_free, free_base_pfn, page) \+({\+unsignedlongnr=(nr_free);\+\+deferred_free_range((free_base_pfn),(nr));\+(free_base_pfn)=0;\+(nr_free)=0;\+page=NULL;\+nr;\+})++staticunsignedlongdeferred_init_range(intnid,intzid,unsignedlongpfn,+unsignedlongend_pfn)+{+structmminit_pfnnid_cachenid_init_state={};+unsignedlongnr_pgmask=pageblock_nr_pages-1;+unsignedlongfree_base_pfn=0;+unsignedlongnr_pages=0;+unsignedlongnr_free=0;+structpage*page=NULL;++for(;pfn<end_pfn;pfn++){+/*+*Firstwecheckifpfnisvalidonarchitectureswhereitis+*possibletohaveholeswithinpageblock_nr_pages.Onsystems+*whereitisnotpossible,thisfunctionisoptimizedout.+*+*Then,wecheckifacurrentlargepageisvalidbyonly+*checkingthevalidityoftheheadpfn.+*+*meminit_pfn_in_nidischeckedonsystemswherepfnscan+*interleavewithinanode:apfnisbetweenstartandend+*ofanode,butdoesnotbelongtothismemorynode.+*+*Finally,weminimizepfnpagelookupsandschedulerchecksby+*performingitonlyonceeverypageblock_nr_pages.+*/+if(!pfn_valid_within(pfn)){+nr_pages+=DEFERRED_FREE(nr_free,free_base_pfn,page);+}elseif(!(pfn&nr_pgmask)&&!pfn_valid(pfn)){+nr_pages+=DEFERRED_FREE(nr_free,free_base_pfn,page);+}elseif(!meminit_pfn_in_nid(pfn,nid,&nid_init_state)){+nr_pages+=DEFERRED_FREE(nr_free,free_base_pfn,page);+}elseif(page&&(pfn&nr_pgmask)){+page++;+__init_single_page(page,pfn,zid,nid);+nr_free++;+}else{+nr_pages+=DEFERRED_FREE(nr_free,free_base_pfn,page);+page=pfn_to_page(pfn);+__init_single_page(page,pfn,zid,nid);+free_base_pfn=pfn;+nr_free=1;+cond_resched();+}+}+/* Free the last block of pages to allocator */+nr_pages+=DEFERRED_FREE(nr_free,free_base_pfn,page);++returnnr_pages;+}+/* Initialise remaining memory on a node */staticint__initdeferred_init_memmap(void*data){pg_data_t*pgdat=data;intnid=pgdat->node_id;-structmminit_pfnnid_cachenid_init_state={};unsignedlongstart=jiffies;unsignedlongnr_pages=0;-unsignedlongwalk_start,walk_end;-inti,zid;+unsignedlongspfn,epfn;+phys_addr_tspa,epa;+intzid;structzone*zone;unsignedlongfirst_init_pfn=pgdat->first_deferred_pfn;conststructcpumask*cpumask=cpumask_of_node(pgdat->node_id);+u64i;if(first_init_pfn==ULONG_MAX){pgdat_init_report_one_done();
@@ -1477,83 +1543,12 @@ static int __init deferred_init_memmap(void *data)if(first_init_pfn<zone_end_pfn(zone))break;}+first_init_pfn=max(zone->zone_start_pfn,first_init_pfn);-for_each_mem_pfn_range(i,nid,&walk_start,&walk_end,NULL){-unsignedlongpfn,end_pfn;-structpage*page=NULL;-structpage*free_base_page=NULL;-unsignedlongfree_base_pfn=0;-intnr_to_free=0;--end_pfn=min(walk_end,zone_end_pfn(zone));-pfn=first_init_pfn;-if(pfn<walk_start)-pfn=walk_start;-if(pfn<zone->zone_start_pfn)-pfn=zone->zone_start_pfn;--for(;pfn<end_pfn;pfn++){-if(!pfn_valid_within(pfn))-gotofree_range;--/*-*Ensurepfn_validischeckedevery-*pageblock_nr_pagesformemoryholes-*/-if((pfn&(pageblock_nr_pages-1))==0){-if(!pfn_valid(pfn)){-page=NULL;-gotofree_range;-}-}--if(!meminit_pfn_in_nid(pfn,nid,&nid_init_state)){-page=NULL;-gotofree_range;-}--/* Minimise pfn page lookups and scheduler checks */-if(page&&(pfn&(pageblock_nr_pages-1))!=0){-page++;-}else{-nr_pages+=nr_to_free;-deferred_free_range(free_base_page,-free_base_pfn,nr_to_free);-free_base_page=NULL;-free_base_pfn=nr_to_free=0;--page=pfn_to_page(pfn);-cond_resched();-}--if(page->flags){-VM_BUG_ON(page_zone(page)!=zone);-gotofree_range;-}--__init_single_page(page,pfn,zid,nid);-if(!free_base_page){-free_base_page=page;-free_base_pfn=pfn;-nr_to_free=0;-}-nr_to_free++;--/* Where possible, batch up pages for a single free */-continue;-free_range:-/* Free the current block of pages to allocator */-nr_pages+=nr_to_free;-deferred_free_range(free_base_page,free_base_pfn,-nr_to_free);-free_base_page=NULL;-free_base_pfn=nr_to_free=0;-}-/* Free the last block of pages to allocator */-nr_pages+=nr_to_free;-deferred_free_range(free_base_page,free_base_pfn,nr_to_free);--first_init_pfn=max(end_pfn,first_init_pfn);+for_each_free_mem_range(i,nid,MEMBLOCK_NONE,&spa,&epa,NULL){+spfn=max_t(unsignedlong,first_init_pfn,PFN_UP(spa));+epfn=min_t(unsignedlong,zone_end_pfn(zone),PFN_DOWN(epa));+nr_pages+=deferred_init_range(nid,zid,spfn,epfn);}/* Sanity check that the next zone really is unpopulated */
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-03 12:59:45
On Wed 20-09-17 16:17:06, Pavel Tatashin wrote:
Remove duplicating code by using common functions
vmemmap_pud_populate and vmemmap_pgd_populate.
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
Acked-by: David S. Miller <davem@davemloft.net>
@@ -2651,30 +2651,19 @@ int __meminit vmemmap_populate(unsigned long vstart, unsigned long vend,vstart=vstart&PMD_MASK;vend=ALIGN(vend,PMD_SIZE);for(;vstart<vend;vstart+=PMD_SIZE){-pgd_t*pgd=pgd_offset_k(vstart);+pgd_t*pgd=vmemmap_pgd_populate(vstart,node);unsignedlongpte;pud_t*pud;pmd_t*pmd;-if(pgd_none(*pgd)){-pud_t*new=vmemmap_alloc_block(PAGE_SIZE,node);+if(!pgd)+return-ENOMEM;-if(!new)-return-ENOMEM;-pgd_populate(&init_mm,pgd,new);-}--pud=pud_offset(pgd,vstart);-if(pud_none(*pud)){-pmd_t*new=vmemmap_alloc_block(PAGE_SIZE,node);--if(!new)-return-ENOMEM;-pud_populate(&init_mm,pud,new);-}+pud=vmemmap_pud_populate(pgd,vstart,node);+if(!pud)+return-ENOMEM;pmd=pmd_offset(pud,vstart);-pte=pmd_val(*pmd);if(!(pte&_PAGE_VALID)){void*block=vmemmap_alloc_block(PMD_SIZE,node);
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-03 13:09:02
On Wed 20-09-17 16:17:08, Pavel Tatashin wrote:
Add struct page zeroing as a part of initialization of other fields in
__init_single_page().
This single thread performance collected on: Intel(R) Xeon(R) CPU E7-8895
v3 @ 2.60GHz with 1T of memory (268400646 pages in 8 nodes):
BASE FIX
sparse_init 11.244671836s 0.007199623s
zone_sizes_init 4.879775891s 8.355182299s
--------------------------
Total 16.124447727s 8.362381922s
Hmm, this is confusing. This assumes that sparse_init doesn't zero pages
anymore, right? So these number depend on the last patch in the series?
quoted hunk
sparse_init is where memory for struct pages is zeroed, and the zeroing
part is moved later in this patch into __init_single_page(), which is
called from zone_sizes_init().
Signed-off-by: Pavel Tatashin <redacted>
Reviewed-by: Steven Sistare <redacted>
Reviewed-by: Daniel Jordan <daniel.m.jordan@oracle.com>
Reviewed-by: Bob Picco <redacted>
Acked-by: Michal Hocko <mhocko@suse.com>
---
include/linux/mm.h | 9 +++++++++
mm/page_alloc.c | 1 +
2 files changed, 10 insertions(+)
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-03 13:18:22
On Wed 20-09-17 16:17:10, Pavel Tatashin wrote:
Some memory is reserved but unavailable: not present in memblock.memory
(because not backed by physical pages), but present in memblock.reserved.
Such memory has backing struct pages, but they are not initialized by going
through __init_single_page().
Could you be more specific where is such a memory reserved?
--
Michal Hocko
SUSE Labs
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-03 13:19:58
On Wed 20-09-17 16:17:14, Pavel Tatashin wrote:
vmemmap_alloc_block() will no longer zero the block, so zero memory
at its call sites for everything except struct pages. Struct page memory
is zero'd by struct page initialization.
Replace allocators in sprase-vmemmap to use the non-zeroing version. So,
we will get the performance improvement by zeroing the memory in parallel
when struct pages are zeroed.
From: Mark Rutland <mark.rutland@arm.com> Date: 2017-10-03 14:50:20
Hi Pavel,
On Wed, Sep 20, 2017 at 04:17:11PM -0400, Pavel Tatashin wrote:
During early boot, kasan uses vmemmap_populate() to establish its shadow
memory. But, that interface is intended for struct pages use.
Because of the current project, vmemmap won't be zeroed during allocation,
but kasan expects that memory to be zeroed. We are adding a new
kasan_map_populate() function to resolve this difference.
Thanks for putting this together.
I've given this a spin on arm64, and can confirm that it works.
Given that this involes redundant walking of page tables, I still think
it'd be preferable to have some common *_populate() helper that took a
gfp argument, but I guess it's not the end of the world.
I'll leave it to Will and Catalin to say whether they're happy with the
page table walking and the new p{u,m}d_large() helpers added to arm64.
Thanks,
Mark.
@@ -197,3 +197,70 @@ void __init kasan_populate_zero_shadow(const void *shadow_start,zero_p4d_populate(pgd,addr,next);}while(pgd++,addr=next,addr!=end);}++/* Creates mappings for kasan during early boot. The mapped memory is zeroed */+int__meminitkasan_map_populate(unsignedlongstart,unsignedlongend,+intnode)+{+unsignedlongaddr,pfn,next;+unsignedlonglongsize;+pgd_t*pgd;+p4d_t*p4d;+pud_t*pud;+pmd_t*pmd;+pte_t*pte;+intret;++ret=vmemmap_populate(start,end,node);+/*+*Wemighthavepartiallypopulatedmemory,socheckfornoentries,+*andzeroonlythosethatactuallyexist.+*/+for(addr=start;addr<end;addr=next){+pgd=pgd_offset_k(addr);+if(pgd_none(*pgd)){+next=pgd_addr_end(addr,end);+continue;+}++p4d=p4d_offset(pgd,addr);+if(p4d_none(*p4d)){+next=p4d_addr_end(addr,end);+continue;+}++pud=pud_offset(p4d,addr);+if(pud_none(*pud)){+next=pud_addr_end(addr,end);+continue;+}+if(pud_large(*pud)){+/* This is PUD size page */+next=pud_addr_end(addr,end);+size=PUD_SIZE;+pfn=pud_pfn(*pud);+}else{+pmd=pmd_offset(pud,addr);+if(pmd_none(*pmd)){+next=pmd_addr_end(addr,end);+continue;+}+if(pmd_large(*pmd)){+/* This is PMD size page */+next=pmd_addr_end(addr,end);+size=PMD_SIZE;+pfn=pmd_pfn(*pmd);+}else{+pte=pte_offset_kernel(pmd,addr);+next=addr+PAGE_SIZE;+if(pte_none(*pte))+continue;+/* This is base size page */+size=PAGE_SIZE;+pfn=pte_pfn(*pte);+}+}+memset(phys_to_virt(PFN_PHYS(pfn)),0,size);+}+returnret;+}
Hi Mark,
I considered using a new *populate() function for shadow without using
vmemmap_populate(), but that makes things unnecessary complicated:
vmemmap_populate() has builtin:
1. large page support
2. device memory support
3. node locality support
4. several config based variants on different platforms
All of that will cause the code simply be duplicated on each platform
if we want to support that in kasan.
We could limit ourselves to only supporting base pages in memory by
using something like vmemmap_populate_basepages(), but that is a step
backward. Kasan benefits from using large pages now, why remove it?
So, the solution I provide is walking page table right after memory is
mapped. Since, we are using the actual page table, it is guaranteed that
we are not going to miss any mapped memory, and also it is in common
code, which makes things smaller and nicer.
Thank you,
Pasha
On 10/03/2017 10:48 AM, Mark Rutland wrote:
I've given this a spin on arm64, and can confirm that it works.
Given that this involes redundant walking of page tables, I still think
it'd be preferable to have some common *_populate() helper that took a
gfp argument, but I guess it's not the end of the world.
I'll leave it to Will and Catalin to say whether they're happy with the
page table walking and the new p{u,m}d_large() helpers added to arm64.
@@ -1182,12 +1182,17 @@ void __init mem_init(void)/* clear_bss() already clear the empty_zero_page */-register_page_bootmem_info();-/* this will put all memory onto the freelists */free_all_bootmem();after_bootmem=1;+/* Must be done after boot memory is put on freelist, because here we
standard code style is to do
/*
* text starts here
As you separated x86 and sparc patches doing essentially the same I
assume David is going to take this patch?
Correct, I noticed that usually platform specific changes are done in
separate patches even if they are small. Dave already Acked this patch.
So, I do not think it should be separated from the rest of the patches
when this projects goes into mm-tree.
Please be explicit that this is possible only because we discard
memblock data later after 3010f876500f ("mm: discard memblock data
later"). Also be more explicit how the new code works.
OK
I like how the resulting code is more compact and smaller.
That was the goal :)
for_each_free_mem_range also looks more appropriate but I really detest
the DEFERRED_FREE thingy. Maybe we can handle all that in a single goto
section. I know this is not an art but manipulating variables from
macros is more error prone and much more ugly IMHO.
Sure, I can re-arrange to have a goto place. Function won't be as small,
and if compiler is not smart enough we might end up with having more
branches than what my current code has.
please do not use macros. Btw. this deserves its own fix. I suspect that
no CONFIG_HOLES_IN_ZONE arch enables DEFERRED_STRUCT_PAGE_INIT but
purely from the review point of view it should be its own patch.
Sure, I will submit this patch separately from the rest of the project.
In my opinion DEFERRED_STRUCT_PAGE_INIT is the way of the future, so we
should make sure it is working with as many configs as possible.
Thank you,
Pasha
Add struct page zeroing as a part of initialization of other fields in
__init_single_page().
This single thread performance collected on: Intel(R) Xeon(R) CPU E7-8895
v3 @ 2.60GHz with 1T of memory (268400646 pages in 8 nodes):
BASE FIX
sparse_init 11.244671836s 0.007199623s
zone_sizes_init 4.879775891s 8.355182299s
--------------------------
Total 16.124447727s 8.362381922s
Hmm, this is confusing. This assumes that sparse_init doesn't zero pages
anymore, right? So these number depend on the last patch in the series?
Correct, without the last patch sparse_init time won't change.
Pasha
Some memory is reserved but unavailable: not present in memblock.memory
(because not backed by physical pages), but present in memblock.reserved.
Such memory has backing struct pages, but they are not initialized by going
through __init_single_page().
Could you be more specific where is such a memory reserved?
I know of one example: trim_low_memory_range() unconditionally reserves
from pfn 0, but e820__memblock_setup() might provide the exiting memory
from pfn 1 (i.e. KVM).
But, there could be more based on this comment from linux/page-flags.h:
19 * PG_reserved is set for special pages, which can never be swapped
out. Some
20 * of them might not even exist (eg empty_bad_page)...
Pasha
vmemmap_alloc_block() will no longer zero the block, so zero memory
at its call sites for everything except struct pages. Struct page memory
is zero'd by struct page initialization.
Replace allocators in sprase-vmemmap to use the non-zeroing version. So,
we will get the performance improvement by zeroing the memory in parallel
when struct pages are zeroed.
Yes, I will do that. It would also require re-arranging
[PATCH v9 07/12] sparc64: optimized struct page zeroing
optimization to come after this patch.
Pasha
Hi Michal,
Are you OK, if I replace DEFERRED_FREE() macro with a function like this:
/*
* Helper for deferred_init_range, free the given range, and reset the
* counters
*/
static inline unsigned long __def_free(unsigned long *nr_free,
unsigned long *free_base_pfn,
struct page **page)
{
unsigned long nr = *nr_free;
deferred_free_range(*free_base_pfn, nr);
*free_base_pfn = 0;
*nr_free = 0;
*page = NULL;
return nr;
}
Since it is inline, and we operate with non-volatile counters, compiler
will be smart enough to remove all the unnecessary de-references. As a
plus, we won't be adding any new branches, and the code is still going
to stay compact.
Pasha
On 10/03/2017 11:15 AM, Pasha Tatashin wrote:
Hi Michal,
quoted
Please be explicit that this is possible only because we discard
memblock data later after 3010f876500f ("mm: discard memblock data
later"). Also be more explicit how the new code works.
OK
quoted
I like how the resulting code is more compact and smaller.
That was the goal :)
quoted
for_each_free_mem_range also looks more appropriate but I really detest
the DEFERRED_FREE thingy. Maybe we can handle all that in a single goto
section. I know this is not an art but manipulating variables from
macros is more error prone and much more ugly IMHO.
Sure, I can re-arrange to have a goto place. Function won't be as small,
and if compiler is not smart enough we might end up with having more
branches than what my current code has.
quoted
please do not use macros. Btw. this deserves its own fix. I suspect that
no CONFIG_HOLES_IN_ZONE arch enables DEFERRED_STRUCT_PAGE_INIT but
purely from the review point of view it should be its own patch.
Sure, I will submit this patch separately from the rest of the project.
In my opinion DEFERRED_STRUCT_PAGE_INIT is the way of the future, so we
should make sure it is working with as many configs as possible.
Thank you,
Pasha
Hi Michal,
I decided not to merge these two patches, because in addition to sparc
optimization move, we have this dependancies:
mm: zero reserved and unavailable struct pages
must be before
mm: stop zeroing memory during allocation in vmemmap.
Otherwise, we can end-up with struct pages that are not zeroed properly.
However, the first patch depends on
mm: zero struct pages during initialization
As it uses mm_zero_struct_page().
Pasha
On 10/03/2017 11:34 AM, Pasha Tatashin wrote:
On 10/03/2017 09:19 AM, Michal Hocko wrote:
quoted
On Wed 20-09-17 16:17:14, Pavel Tatashin wrote:
quoted
vmemmap_alloc_block() will no longer zero the block, so zero memory
at its call sites for everything except struct pages. Struct page
memory
is zero'd by struct page initialization.
Replace allocators in sprase-vmemmap to use the non-zeroing version. So,
we will get the performance improvement by zeroing the memory in
parallel
when struct pages are zeroed.
Yes, I will do that. It would also require re-arranging
[PATCH v9 07/12] sparc64: optimized struct page zeroing
optimization to come after this patch.
Pasha
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-04 08:45:32
On Tue 03-10-17 16:26:51, Pasha Tatashin wrote:
Hi Michal,
I decided not to merge these two patches, because in addition to sparc
optimization move, we have this dependancies:
optimizations can and should go on top of the core patch.
mm: zero reserved and unavailable struct pages
must be before
mm: stop zeroing memory during allocation in vmemmap.
Otherwise, we can end-up with struct pages that are not zeroed properly.
Right and you can deal with it easily. Just introduce the
mm_zero_struct_page earlier along with its user in "stop zeroing ..."
I think that moving the zeroying in one go is more reasonable than
adding it to __init_single_page with misleading numbers and later
dropping the zeroying from the memmap path.
--
Michal Hocko
SUSE Labs
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-04 08:45:59
On Tue 03-10-17 11:22:35, Pasha Tatashin wrote:
On 10/03/2017 09:08 AM, Michal Hocko wrote:
quoted
On Wed 20-09-17 16:17:08, Pavel Tatashin wrote:
quoted
Add struct page zeroing as a part of initialization of other fields in
__init_single_page().
This single thread performance collected on: Intel(R) Xeon(R) CPU E7-8895
v3 @ 2.60GHz with 1T of memory (268400646 pages in 8 nodes):
BASE FIX
sparse_init 11.244671836s 0.007199623s
zone_sizes_init 4.879775891s 8.355182299s
--------------------------
Total 16.124447727s 8.362381922s
Hmm, this is confusing. This assumes that sparse_init doesn't zero pages
anymore, right? So these number depend on the last patch in the series?
Correct, without the last patch sparse_init time won't change.
THen this is just misleading.
--
Michal Hocko
SUSE Labs
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-04 08:48:22
On Tue 03-10-17 12:01:08, Pasha Tatashin wrote:
Hi Michal,
Are you OK, if I replace DEFERRED_FREE() macro with a function like this:
/*
* Helper for deferred_init_range, free the given range, and reset the
* counters
*/
static inline unsigned long __def_free(unsigned long *nr_free,
unsigned long *free_base_pfn,
struct page **page)
{
unsigned long nr = *nr_free;
deferred_free_range(*free_base_pfn, nr);
*free_base_pfn = 0;
*nr_free = 0;
*page = NULL;
return nr;
}
Since it is inline, and we operate with non-volatile counters, compiler will
be smart enough to remove all the unnecessary de-references. As a plus, we
won't be adding any new branches, and the code is still going to stay
compact.
OK. It is a bit clunky but we are holding too much state there. I
haven't checked whether that can be simplified but this can be always
done later.
--
Michal Hocko
SUSE Labs
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-04 08:56:41
On Tue 03-10-17 11:29:16, Pasha Tatashin wrote:
On 10/03/2017 09:18 AM, Michal Hocko wrote:
quoted
On Wed 20-09-17 16:17:10, Pavel Tatashin wrote:
quoted
Some memory is reserved but unavailable: not present in memblock.memory
(because not backed by physical pages), but present in memblock.reserved.
Such memory has backing struct pages, but they are not initialized by going
through __init_single_page().
Could you be more specific where is such a memory reserved?
I know of one example: trim_low_memory_range() unconditionally reserves from
pfn 0, but e820__memblock_setup() might provide the exiting memory from pfn
1 (i.e. KVM).
Then just initialize struct pages for that mapping rigth there where a
special API is used.
But, there could be more based on this comment from linux/page-flags.h:
19 * PG_reserved is set for special pages, which can never be swapped out.
Some
20 * of them might not even exist (eg empty_bad_page)...
I have no idea wht empty_bad_page is but a quick grep shows that this is
never used. I might be wrong here but if somebody is reserving a memory
in a special way then we should handle the initialization right there.
E.g. create an API for special memblock reservations.
--
Michal Hocko
SUSE Labs
Add struct page zeroing as a part of initialization of other fields in
__init_single_page().
This single thread performance collected on: Intel(R) Xeon(R) CPU E7-8895
v3 @ 2.60GHz with 1T of memory (268400646 pages in 8 nodes):
BASE FIX
sparse_init 11.244671836s 0.007199623s
zone_sizes_init 4.879775891s 8.355182299s
--------------------------
Total 16.124447727s 8.362381922s
Hmm, this is confusing. This assumes that sparse_init doesn't zero pages
anymore, right? So these number depend on the last patch in the series?
Correct, without the last patch sparse_init time won't change.
THen this is just misleading.
OK, I will re-arrange patches the way you suggested earlier.
Pasha
Could you be more specific where is such a memory reserved?
I know of one example: trim_low_memory_range() unconditionally reserves from
pfn 0, but e820__memblock_setup() might provide the exiting memory from pfn
1 (i.e. KVM).
Then just initialize struct pages for that mapping rigth there where a
special API is used.
quoted
But, there could be more based on this comment from linux/page-flags.h:
19 * PG_reserved is set for special pages, which can never be swapped out.
Some
20 * of them might not even exist (eg empty_bad_page)...
I have no idea wht empty_bad_page is but a quick grep shows that this is
never used. I might be wrong here but if somebody is reserving a memory
in a special way then we should handle the initialization right there.
E.g. create an API for special memblock reservations.
Hi Michal,
The reservations happen before struct pages are allocated and mapped.
So, it is not always possible to do it at call sites.
Previously, I have solved this problem like this:
https://patchwork.kernel.org/patch/9886163
But, I was not too happy with that approach, so I replaced it with the
current approach as it is more generic, and solves similar issues if
they happen in other places. Also, the comment in page-flags got me
scared that there are probably other places perhaps on other
architectures that can have the similar issue.
In addition, I did not like my solution, I was simply shrinking the low
reservation from:
[0 - reserve_low) to [min_pfn - reserve_low), but if min_pfn >
reserve_low can we skip low reservation entirely? I was not sure.
The current approach notifies us if there are such pages, and we can
fix/remove them in the future without crashing kernel in the meantime.
Pasha
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-04 12:57:55
On Wed 04-10-17 08:40:11, Pasha Tatashin wrote:
quoted
quoted
quoted
Could you be more specific where is such a memory reserved?
I know of one example: trim_low_memory_range() unconditionally reserves from
pfn 0, but e820__memblock_setup() might provide the exiting memory from pfn
1 (i.e. KVM).
Then just initialize struct pages for that mapping rigth there where a
special API is used.
quoted
But, there could be more based on this comment from linux/page-flags.h:
19 * PG_reserved is set for special pages, which can never be swapped out.
Some
20 * of them might not even exist (eg empty_bad_page)...
I have no idea wht empty_bad_page is but a quick grep shows that this is
never used. I might be wrong here but if somebody is reserving a memory
in a special way then we should handle the initialization right there.
E.g. create an API for special memblock reservations.
Hi Michal,
The reservations happen before struct pages are allocated and mapped. So, it
is not always possible to do it at call sites.
OK, I didn't realize that.
Previously, I have solved this problem like this:
https://patchwork.kernel.org/patch/9886163
But, I was not too happy with that approach, so I replaced it with the
current approach as it is more generic, and solves similar issues if they
happen in other places. Also, the comment in page-flags got me scared that
there are probably other places perhaps on other architectures that can have
the similar issue.
I believe the comment is just stale. I have looked into empty_bad_page
and it is just a relict. I plan to post a patch soon.
In addition, I did not like my solution, I was simply shrinking the low
reservation from:
[0 - reserve_low) to [min_pfn - reserve_low), but if min_pfn > reserve_low
can we skip low reservation entirely? I was not sure.
The current approach notifies us if there are such pages, and we can
fix/remove them in the future without crashing kernel in the meantime.
I am not really familiar with the trim_low_memory_range code path. I am
not even sure we have to care about it because nobody should be walking
pfns outside of any zone. I am worried that this patch adds a code which
is not really used and it will just stay that way for ever because
nobody will dare to change it as it is too obscure and not explained
very well. trim_low_memory_range is a good example of this. Why do we
even reserve this range from the memory block allocator? The memory
shouldn't be backed by any real memory and thus not in the allocator in
the first place, no?
--
Michal Hocko
SUSE Labs
I am not really familiar with the trim_low_memory_range code path. I am
not even sure we have to care about it because nobody should be walking
pfns outside of any zone.
According to commit comments first 4K belongs to BIOS, so I think the
memory exists but BIOS may or may not report it to Linux. So, reserve it
to make sure we never touch it.
I am worried that this patch adds a code which
is not really used and it will just stay that way for ever because
nobody will dare to change it as it is too obscure and not explained
very well.
I could explain mine code better. Perhaps add more comments, and explain
when it can be removed?
trim_low_memory_range is a good example of this. Why do we
even reserve this range from the memory block allocator? The memory
shouldn't be backed by any real memory and thus not in the allocator in
the first place, no?
Since it is not enforced in memblock that everything in reserved list
must be part of memory list, we can have it, and we need to make sure
kernel does not panic. Otherwise, it is very hard to detect such bugs.
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-04 14:04:16
On Wed 04-10-17 09:28:55, Pasha Tatashin wrote:
quoted
I am not really familiar with the trim_low_memory_range code path. I am
not even sure we have to care about it because nobody should be walking
pfns outside of any zone.
According to commit comments first 4K belongs to BIOS, so I think the memory
exists but BIOS may or may not report it to Linux. So, reserve it to make
sure we never touch it.
Yes and that memory should be outside of any zones, no?
quoted
I am worried that this patch adds a code which
is not really used and it will just stay that way for ever because
nobody will dare to change it as it is too obscure and not explained
very well.
I could explain mine code better. Perhaps add more comments, and explain
when it can be removed?
More explanation would be definitely helpful
quoted
trim_low_memory_range is a good example of this. Why do we
even reserve this range from the memory block allocator? The memory
shouldn't be backed by any real memory and thus not in the allocator in
the first place, no?
Since it is not enforced in memblock that everything in reserved list must
be part of memory list, we can have it, and we need to make sure kernel does
not panic. Otherwise, it is very hard to detect such bugs.
So, should we report such a memblock reservation API (ab)use to the log?
Are you actually sure that trim_low_memory_range is doing a sane and
really needed thing? In other words do we have a zone which contains
this no-memory backed pfns?
--
Michal Hocko
SUSE Labs
I am not really familiar with the trim_low_memory_range code path. I am
not even sure we have to care about it because nobody should be walking
pfns outside of any zone.
According to commit comments first 4K belongs to BIOS, so I think the memory
exists but BIOS may or may not report it to Linux. So, reserve it to make
sure we never touch it.
Yes and that memory should be outside of any zones, no?
I am not totally sure, I think some x86 expert could help us here. But,
in either case this issue can be fixed separately from the rest of the
series.
quoted
quoted
I am worried that this patch adds a code which
is not really used and it will just stay that way for ever because
nobody will dare to change it as it is too obscure and not explained
very well.
I could explain mine code better. Perhaps add more comments, and explain
when it can be removed?
More explanation would be definitely helpful
quoted
quoted
trim_low_memory_range is a good example of this. Why do we
even reserve this range from the memory block allocator? The memory
shouldn't be backed by any real memory and thus not in the allocator in
the first place, no?
Since it is not enforced in memblock that everything in reserved list must
be part of memory list, we can have it, and we need to make sure kernel does
not panic. Otherwise, it is very hard to detect such bugs.
So, should we report such a memblock reservation API (ab)use to the log?
Are you actually sure that trim_low_memory_range is doing a sane and
really needed thing? In other words do we have a zone which contains
this no-memory backed pfns?
And, this patch reports it already:
+ pr_info("Reserved but unavailable: %lld pages", pgcnt);
I could add a comment above this print call, explain that such memory is
probably bogus and must be studied/fixed. Also, add that this code can
be removed once memblock is changed to allow reserve only memory that is
backed by physical memory i.e. in "memory" list.
Pasha
From: Will Deacon <hidden> Date: 2017-10-09 17:13:37
On Tue, Oct 03, 2017 at 03:48:46PM +0100, Mark Rutland wrote:
On Wed, Sep 20, 2017 at 04:17:11PM -0400, Pavel Tatashin wrote:
quoted
During early boot, kasan uses vmemmap_populate() to establish its shadow
memory. But, that interface is intended for struct pages use.
Because of the current project, vmemmap won't be zeroed during allocation,
but kasan expects that memory to be zeroed. We are adding a new
kasan_map_populate() function to resolve this difference.
Thanks for putting this together.
I've given this a spin on arm64, and can confirm that it works.
Given that this involes redundant walking of page tables, I still think
it'd be preferable to have some common *_populate() helper that took a
gfp argument, but I guess it's not the end of the world.
I'll leave it to Will and Catalin to say whether they're happy with the
page table walking and the new p{u,m}d_large() helpers added to arm64.
To be honest, it just looks completely backwards to me; we're walking the
page tables we created earlier on so that we can figure out what needs to
be zeroed for KASAN. We already had that information before, hence my
preference to allow propagation of GFP_FLAGs to vmemmap_alloc_block when
it's needed. I know that's not popular for some reason, but is walking the
page tables really better?
Will
From: Pavel Tatashin <hidden> Date: 2017-10-09 17:51:53
Hi Will,
I can go back to that approach, if Michal OK with it. But, that would
mean that I would need to touch every single architecture that
implements vmemmap_populate(), and also pass flags at least through
these functions on every architectures (some have more than one
decided by configs).:
vmemmap_populate()
vmemmap_populate_basepages()
vmemmap_populate_hugepages()
vmemmap_pte_populate()
__vmemmap_alloc_block_buf()
alloc_block_buf()
vmemmap_alloc_block()
IMO, while I understand that it looks strange that we must walk page
table after creating it, it is a better approach: more enclosed as it
effects kasan only, and more universal as it is in common code. We are
also somewhat late in the review process, means we will need again to
get ACKs from the maintainers of other arches.
Pavel
On Mon, Oct 9, 2017 at 1:13 PM, Will Deacon [off-list ref] wrote:
On Tue, Oct 03, 2017 at 03:48:46PM +0100, Mark Rutland wrote:
quoted
On Wed, Sep 20, 2017 at 04:17:11PM -0400, Pavel Tatashin wrote:
quoted
During early boot, kasan uses vmemmap_populate() to establish its shadow
memory. But, that interface is intended for struct pages use.
Because of the current project, vmemmap won't be zeroed during allocation,
but kasan expects that memory to be zeroed. We are adding a new
kasan_map_populate() function to resolve this difference.
Thanks for putting this together.
I've given this a spin on arm64, and can confirm that it works.
Given that this involes redundant walking of page tables, I still think
it'd be preferable to have some common *_populate() helper that took a
gfp argument, but I guess it's not the end of the world.
I'll leave it to Will and Catalin to say whether they're happy with the
page table walking and the new p{u,m}d_large() helpers added to arm64.
To be honest, it just looks completely backwards to me; we're walking the
page tables we created earlier on so that we can figure out what needs to
be zeroed for KASAN. We already had that information before, hence my
preference to allow propagation of GFP_FLAGs to vmemmap_alloc_block when
it's needed. I know that's not popular for some reason, but is walking the
page tables really better?
Will
--
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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
From: Michal Hocko <mhocko@kernel.org> Date: 2017-10-09 18:14:39
On Mon 09-10-17 13:51:47, Pavel Tatashin wrote:
Hi Will,
I can go back to that approach, if Michal OK with it. But, that would
mean that I would need to touch every single architecture that
implements vmemmap_populate(), and also pass flags at least through
these functions on every architectures (some have more than one
decided by configs).:
vmemmap_populate()
vmemmap_populate_basepages()
vmemmap_populate_hugepages()
vmemmap_pte_populate()
__vmemmap_alloc_block_buf()
alloc_block_buf()
vmemmap_alloc_block()
IMO, while I understand that it looks strange that we must walk page
table after creating it, it is a better approach: more enclosed as it
effects kasan only, and more universal as it is in common code.
While I understand that gfp mask approach might look better at first
sight this is by no means a general purpose API so I would rather be
pragmatic and have a smaller code footprint than a more general
interface. Kasan is pretty much a special case and doing a one time
initialization 2 pass thing is imho acceptable. If this turns out to be
impractical in future then let's fix it up but right now I would rather
go a simpler path.
--
Michal Hocko
SUSE Labs
From: Will Deacon <hidden> Date: 2017-10-09 18:22:17
Hi Pavel,
On Mon, Oct 09, 2017 at 01:51:47PM -0400, Pavel Tatashin wrote:
I can go back to that approach, if Michal OK with it. But, that would
mean that I would need to touch every single architecture that
implements vmemmap_populate(), and also pass flags at least through
these functions on every architectures (some have more than one
decided by configs).:
vmemmap_populate()
vmemmap_populate_basepages()
vmemmap_populate_hugepages()
vmemmap_pte_populate()
__vmemmap_alloc_block_buf()
alloc_block_buf()
vmemmap_alloc_block()
As an interim step, why not introduce something like
vmemmap_alloc_block_flags and make the page-table walking opt-out for
architectures that don't want it? Then we can just pass __GFP_ZERO from
our vmemmap_populate where necessary and other architectures can do the
page-table walking dance if they prefer.
IMO, while I understand that it looks strange that we must walk page
table after creating it, it is a better approach: more enclosed as it
effects kasan only, and more universal as it is in common code.
I don't buy the more universal aspect, but I appreciate it's subjective.
Frankly, I'd just sooner not have core code walking early page tables if
it can be avoided, and it doesn't look hard to avoid it in this case.
The fact that you're having to add pmd_large and pud_large, which are
otherwise unused in mm/, is an indication that this isn't quite right imo.
Will
From: Pavel Tatashin <hidden> Date: 2017-10-09 18:42:40
Hi Will,
In addition to what Michal wrote:
As an interim step, why not introduce something like
vmemmap_alloc_block_flags and make the page-table walking opt-out for
architectures that don't want it? Then we can just pass __GFP_ZERO from
our vmemmap_populate where necessary and other architectures can do the
page-table walking dance if they prefer.
I do not see the benefit, implementing this approach means that we
would need to implement two table walks instead of one: one for x86,
another for ARM, as these two architectures support kasan. Also, this
would become a requirement for any future architecture that want to
add kasan support to add this page table walk implementation.
quoted
IMO, while I understand that it looks strange that we must walk page
table after creating it, it is a better approach: more enclosed as it
effects kasan only, and more universal as it is in common code.
I don't buy the more universal aspect, but I appreciate it's subjective.
Frankly, I'd just sooner not have core code walking early page tables if
it can be avoided, and it doesn't look hard to avoid it in this case.
The fact that you're having to add pmd_large and pud_large, which are
otherwise unused in mm/, is an indication that this isn't quite right imo.
28 +#define pmd_large(pmd) pmd_sect(pmd)
29 +#define pud_large(pud) pud_sect(pud)
it is just naming difference, ARM64 calls them pmd_sect, common mm and
other arches call them
pmd_large/pud_large. Even the ARM has these defines in
arm/include/asm/pgtable-3level.h
arm/include/asm/pgtable-2level.h
Pavel
From: Will Deacon <hidden> Date: 2017-10-09 18:48:29
On Mon, Oct 09, 2017 at 08:14:33PM +0200, Michal Hocko wrote:
On Mon 09-10-17 13:51:47, Pavel Tatashin wrote:
quoted
I can go back to that approach, if Michal OK with it. But, that would
mean that I would need to touch every single architecture that
implements vmemmap_populate(), and also pass flags at least through
these functions on every architectures (some have more than one
decided by configs).:
vmemmap_populate()
vmemmap_populate_basepages()
vmemmap_populate_hugepages()
vmemmap_pte_populate()
__vmemmap_alloc_block_buf()
alloc_block_buf()
vmemmap_alloc_block()
IMO, while I understand that it looks strange that we must walk page
table after creating it, it is a better approach: more enclosed as it
effects kasan only, and more universal as it is in common code.
While I understand that gfp mask approach might look better at first
sight this is by no means a general purpose API so I would rather be
pragmatic and have a smaller code footprint than a more general
interface. Kasan is pretty much a special case and doing a one time
initialization 2 pass thing is imho acceptable. If this turns out to be
impractical in future then let's fix it up but right now I would rather
go a simpler path.
I think the simpler path for arm64 is really to say when we want the memory
zeroing as opposed to exposing pmd_large/pud_large macros. Those are likely
to grow more users too, but are difficult to use correctly as we have things
like contiguous ptes that map to a granule smaller than a pmd.
I proposed an alternative solution to Pavel already, but it could be made
less general purpose by marking the function __meminit and only having it
do anything if KASAN is compiled in.
Will
From: Will Deacon <hidden> Date: 2017-10-09 18:48:35
On Mon, Oct 09, 2017 at 02:42:32PM -0400, Pavel Tatashin wrote:
Hi Will,
In addition to what Michal wrote:
quoted
As an interim step, why not introduce something like
vmemmap_alloc_block_flags and make the page-table walking opt-out for
architectures that don't want it? Then we can just pass __GFP_ZERO from
our vmemmap_populate where necessary and other architectures can do the
page-table walking dance if they prefer.
I do not see the benefit, implementing this approach means that we
would need to implement two table walks instead of one: one for x86,
another for ARM, as these two architectures support kasan. Also, this
would become a requirement for any future architecture that want to
add kasan support to add this page table walk implementation.
We have two table walks even with your patch series applied afaict: one in
our definition of vmemmap_populate (arch/arm64/mm/mmu.c) and this one
in the core code.
quoted
quoted
IMO, while I understand that it looks strange that we must walk page
table after creating it, it is a better approach: more enclosed as it
effects kasan only, and more universal as it is in common code.
I don't buy the more universal aspect, but I appreciate it's subjective.
Frankly, I'd just sooner not have core code walking early page tables if
it can be avoided, and it doesn't look hard to avoid it in this case.
The fact that you're having to add pmd_large and pud_large, which are
otherwise unused in mm/, is an indication that this isn't quite right imo.
28 +#define pmd_large(pmd) pmd_sect(pmd)
29 +#define pud_large(pud) pud_sect(pud)
it is just naming difference, ARM64 calls them pmd_sect, common mm and
other arches call them
pmd_large/pud_large. Even the ARM has these defines in
arm/include/asm/pgtable-3level.h
arm/include/asm/pgtable-2level.h
My worry is that these are actually highly arch-specific, but will likely
grow more users in mm/ that assume things for all architectures that aren't
necessarily valid.
Will
From: Pavel Tatashin <hidden> Date: 2017-10-09 18:59:16
Hi Will,
We have two table walks even with your patch series applied afaict: one in
our definition of vmemmap_populate (arch/arm64/mm/mmu.c) and this one
in the core code.
I meant to say implementing two new page table walkers, not at runtime.
My worry is that these are actually highly arch-specific, but will likely
grow more users in mm/ that assume things for all architectures that aren't
necessarily valid.
I see, how about moving new kasan_map_populate() implementation into
arch dependent code:
arch/x86/mm/kasan_init_64.c
arch/arm64/mm/kasan_init.c
This way we won't need to add pmd_large()/pud_large() macros for arm64?
Pavel
From: Will Deacon <hidden> Date: 2017-10-09 19:02:13
Hi Pavel,
On Mon, Oct 09, 2017 at 02:59:09PM -0400, Pavel Tatashin wrote:
quoted
We have two table walks even with your patch series applied afaict: one in
our definition of vmemmap_populate (arch/arm64/mm/mmu.c) and this one
in the core code.
I meant to say implementing two new page table walkers, not at runtime.
Ok, but I'm still missing why you think that is needed. What would be the
second page table walker that needs implementing?
quoted
My worry is that these are actually highly arch-specific, but will likely
grow more users in mm/ that assume things for all architectures that aren't
necessarily valid.
I see, how about moving new kasan_map_populate() implementation into
arch dependent code:
arch/x86/mm/kasan_init_64.c
arch/arm64/mm/kasan_init.c
This way we won't need to add pmd_large()/pud_large() macros for arm64?
I guess we could implement that on arm64 using our current vmemmap_populate
logic and an explicit memset.
Will
From: Pavel Tatashin <hidden> Date: 2017-10-09 19:07:07
Ok, but I'm still missing why you think that is needed. What would be the
second page table walker that needs implementing?
I guess we could implement that on arm64 using our current vmemmap_populate
logic and an explicit memset.
Hi Will,
What do you mean by explicit memset()? We can't simply memset() from
start to end without doing the page table walk, because at the time
kasan is calling vmemmap_populate() we have a tmp_pg_dir instead of
swapper_pg_dir.
We could do the explicit memset() after
cpu_replace_ttbr1(lm_alias(swapper_pg_dir)); but again, this was in
one of my previous implementations, and I was asked to replace that.
Pavel
From: Pavel Tatashin <hidden> Date: 2017-10-09 19:57:44
quoted
I guess we could implement that on arm64 using our current vmemmap_populate
logic and an explicit memset.
Hi Will,
I will send out a new patch series with x86/arm64 versions of
kasan_map_populate(), so you could take a look if this is something
that is acceptable.
Thank you,
Pavel