| Message ID | 20210129064045.18471-1-osalvador@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] x86/vmemmap: Handle unpopulated sub-pmd ranges | expand |
On 29.01.21 07:40, Oscar Salvador wrote: > When the size of a struct page is not multiple of 2MB, sections do > not span a PMD anymore and so when populating them some parts of the > PMD will remain unused. > Because of this, PMDs will be left behind when depopulating sections > since remove_pmd_table() thinks that those unused parts are still in > use. > > Fix this by marking the unused parts with PAGE_INUSE, so memchr_inv() will > do the right thing and will let us free the PMD when the last user of it > is gone. > > This patch is based on a similar patch by David Hildenbrand: > > https://lore.kernel.org/linux-mm/20200722094558.9828-9-david@redhat.com/ > https://lore.kernel.org/linux-mm/20200722094558.9828-10-david@redhat.com/ > > Signed-off-by: Oscar Salvador <osalvador@suse.de> > --- > > v1 -> v2: > - Rename PAGE_INUSE to PAGE_UNUSED as it better describes what we do > > --- > arch/x86/mm/init_64.c | 91 +++++++++++++++++++++++++++++++++++++------ > 1 file changed, 79 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c > index b5a3fa4033d3..dbb76160ed52 100644 > --- a/arch/x86/mm/init_64.c > +++ b/arch/x86/mm/init_64.c > @@ -871,7 +871,72 @@ int arch_add_memory(int nid, u64 start, u64 size, > return add_pages(nid, start_pfn, nr_pages, params); > } > > -#define PAGE_INUSE 0xFD > +#define PAGE_UNUSED 0xFD > + > +/* > + * The unused vmemmap range, which was not yet memset(PAGE_UNUSED) ranges > + * from unused_pmd_start to next PMD_SIZE boundary. > + */ > +static unsigned long unused_pmd_start __meminitdata; > + > +static void __meminit vmemmap_flush_unused_pmd(void) > +{ > + if (!unused_pmd_start) > + return; > + /* > + * Clears (unused_pmd_start, PMD_END] > + */ > + memset((void *)unused_pmd_start, PAGE_UNUSED, > + ALIGN(unused_pmd_start, PMD_SIZE) - unused_pmd_start); > + unused_pmd_start = 0; > +} > + > +/* Returns true if the PMD is completely unused and thus it can be freed */ > +static bool __meminit vmemmap_unuse_sub_pmd(unsigned long addr, unsigned long end) > +{ > + unsigned long start = ALIGN_DOWN(addr, PMD_SIZE); > + > + vmemmap_flush_unused_pmd(); > + memset((void *)addr, PAGE_UNUSED, end - addr); > + > + return !memchr_inv((void *)start, PAGE_UNUSED, PMD_SIZE); > +} > + > +static void __meminit vmemmap_use_sub_pmd(unsigned long start, unsigned long end) > +{ > + /* > + * We only optimize if the new used range directly follows the > + * previously unused range (esp., when populating consecutive sections). > + */ > + if (unused_pmd_start == start) { > + if (likely(IS_ALIGNED(end, PMD_SIZE))) > + unused_pmd_start = 0; > + else > + unused_pmd_start = end; > + return; > + } > + > + vmemmap_flush_unused_pmd(); > +} > + > +static void __meminit vmemmap_use_new_sub_pmd(unsigned long start, unsigned long end) > +{ > + vmemmap_flush_unused_pmd(); > + > + /* > + * Mark the unused parts of the new memmap range > + */ > + if (!IS_ALIGNED(start, PMD_SIZE)) > + memset((void *)start, PAGE_UNUSED, > + start - ALIGN_DOWN(start, PMD_SIZE)); > + /* > + * We want to avoid memset(PAGE_UNUSED) when populating the vmemmap of > + * consecutive sections. Remember for the last added PMD the last > + * unused range in the populated PMD. > + */ > + if (!IS_ALIGNED(end, PMD_SIZE)) > + unused_pmd_start = end; > +} > > static void __meminit free_pagetable(struct page *page, int order) > { > @@ -1008,10 +1073,10 @@ remove_pte_table(pte_t *pte_start, unsigned long addr, unsigned long end, > * with 0xFD, and remove the page when it is wholly > * filled with 0xFD. > */ > - memset((void *)addr, PAGE_INUSE, next - addr); > + memset((void *)addr, PAGE_UNUSED, next - addr); > > page_addr = page_address(pte_page(*pte)); > - if (!memchr_inv(page_addr, PAGE_INUSE, PAGE_SIZE)) { > + if (!memchr_inv(page_addr, PAGE_UNUSED, PAGE_SIZE)) { > free_pagetable(pte_page(*pte), 0); > I remember already raising this, in the context of other cleanups, but let's start anew: How could we ever even end up in "!PAGE_ALIGNED(addr) && PAGE_ALIGNED(next)"? As the comment correctly indicates, it would only make sense for "freeing vmemmap pages". This would mean we are removing parts of a vmemmap page (4k), calling vmemmap_free()->remove_pagetable() on sub-page granularity. Even sub-sections (2MB - 512 pages) have a memmap size with base pages: - 56 bytes: 7 pages - 64 bytes: 8 pages - 72 bytes: 9 pages sizeof(struct page) is always multiples of 8 bytes, so that will hold. E.g., in __populate_section_memmap(), we already enforce proper subsection alignment. IMHO, we should rip out that code here and enforce page alignment in vmemmap_populate()/vmemmap_free(). Am I missing something?
On Fri, Jan 29, 2021 at 01:46:33PM +0100, David Hildenbrand wrote: > > static void __meminit free_pagetable(struct page *page, int order) > > { > > @@ -1008,10 +1073,10 @@ remove_pte_table(pte_t *pte_start, unsigned long addr, unsigned long end, > > * with 0xFD, and remove the page when it is wholly > > * filled with 0xFD. > > */ > > - memset((void *)addr, PAGE_INUSE, next - addr); > > + memset((void *)addr, PAGE_UNUSED, next - addr); > > page_addr = page_address(pte_page(*pte)); > > - if (!memchr_inv(page_addr, PAGE_INUSE, PAGE_SIZE)) { > > + if (!memchr_inv(page_addr, PAGE_UNUSED, PAGE_SIZE)) { > > free_pagetable(pte_page(*pte), 0); > > I remember already raising this, in the context of other cleanups, but let's > start anew: > > How could we ever even end up in "!PAGE_ALIGNED(addr) && > PAGE_ALIGNED(next)"? As the comment correctly indicates, it would only make > sense for "freeing vmemmap pages". > > This would mean we are removing parts of a vmemmap page (4k), calling > vmemmap_free()->remove_pagetable() on sub-page granularity. > > Even sub-sections (2MB - 512 pages) have a memmap size with base pages: > - 56 bytes: 7 pages > - 64 bytes: 8 pages > - 72 bytes: 9 pages > > sizeof(struct page) is always multiples of 8 bytes, so that will hold. > > E.g., in __populate_section_memmap(), we already enforce proper subsection > alignment. > > IMHO, we should rip out that code here and enforce page alignment in > vmemmap_populate()/vmemmap_free(). > > Am I missing something? Thanks David for bringing this up, I must say I was not aware that this topic was ever discussed. Ok, I've been having a look into this. At first I was concerced because of a pure SPARSEMEM configuration, but I see that those allocations are done in a very diferent way so it does not bother us. So we have the following enforcements during hotplug: add_memory_resource check_hotplug_memory_range : Checks range aligned to memory_block_size_bytes, : which means it must be section-size aligned populate_section_memmap __populate_section_memmap : Checks range aligned to sub-section size So, IIRC we have two cases during hotplug: 1) the ones that want memory blocks 2) the ones that do not want them (pmem stuff) For #1, we always enforce section alignment in add_memory_resource, and for #2 we always make sure the range is at least sub-section aligned. And the important stuff is that boot memory is no longer to be hot-removed (boot memory had some strange layout sometimes). So, given the above, I think it should be safe to drop that check in remote_pte_table. But do we really need to force page alignment in vmemmap_populate/vmemmap_free? vmemmap_populate should already receive a page-aligned chunk because __populate_section_memmap made sure of that, and vmemmap_free() should be ok as we already filtered out at hot-adding stage. Of course, this will hold as long as struct page size of multiple of 8. Should that change we might get trouble, but I do not think that can ever happened (tm). But anyway, I am fine with placing a couple of checks in vmemmap_{populate,free} just to double check. What do you think?
>> IMHO, we should rip out that code here and enforce page alignment in >> vmemmap_populate()/vmemmap_free(). >> >> Am I missing something? > > Thanks David for bringing this up, I must say I was not aware that this > topic was ever discussed. Yeah, last time I raised it was in https://lkml.kernel.org/r/20200703013435.GA11340@L-31X9LVDL-1304.local but I never got to clean it up myself. > > Ok, I've been having a look into this. > At first I was concerced because of a pure SPARSEMEM configuration, but I > see that those allocations are done in a very diferent way so it does not > bother us. > > So we have the following enforcements during hotplug: > > add_memory_resource > check_hotplug_memory_range : Checks range aligned to memory_block_size_bytes, > : which means it must be section-size aligned > > populate_section_memmap > __populate_section_memmap : Checks range aligned to sub-section size > > So, IIRC we have two cases during hotplug: > 1) the ones that want memory blocks > 2) the ones that do not want them (pmem stuff) > > For #1, we always enforce section alignment in add_memory_resource, and for > #2 we always make sure the range is at least sub-section aligned. > > And the important stuff is that boot memory is no longer to be hot-removed > (boot memory had some strange layout sometimes). The vmemmap of boot mem sections is always fully populated, even with strange memory layouts (e.g., see comment in pfn_valid()). In addition, we can only offline+remove whole sections, so that should be fine. > > So, given the above, I think it should be safe to drop that check in > remote_pte_table. > But do we really need to force page alignment in vmemmap_populate/vmemmap_free? > vmemmap_populate should already receive a page-aligned chunk because > __populate_section_memmap made sure of that, and vmemmap_free() should be ok > as we already filtered out at hot-adding stage. > > Of course, this will hold as long as struct page size of multiple of 8. > Should that change we might get trouble, but I do not think that can ever > happened (tm). > > But anyway, I am fine with placing a couple of checks in vmemmap_{populate,free} > just to double check. > > What do you think? I'd just throw in 1 or 2 VM_BUG_ON() to self-document what we expect and that we thought about these conditions. It's then easy to identify the relevant commit where we explain the rationale. I don't have a strong opinion, the other archs also don't seem to care about documenting/enforcing it.
On Tue, Feb 02, 2021 at 09:35:09AM +0100, David Hildenbrand wrote: > Yeah, last time I raised it was in > > https://lkml.kernel.org/r/20200703013435.GA11340@L-31X9LVDL-1304.local > > but I never got to clean it up myself. I see. > > So, IIRC we have two cases during hotplug: > > 1) the ones that want memory blocks > > 2) the ones that do not want them (pmem stuff) > > > > For #1, we always enforce section alignment in add_memory_resource, and for > > #2 we always make sure the range is at least sub-section aligned. > > > > And the important stuff is that boot memory is no longer to be hot-removed > > (boot memory had some strange layout sometimes). > > The vmemmap of boot mem sections is always fully populated, even with > strange memory layouts (e.g., see comment in pfn_valid()). In addition, we > can only offline+remove whole sections, so that should be fine. You are right. > > > > > So, given the above, I think it should be safe to drop that check in > > remote_pte_table. > > But do we really need to force page alignment in vmemmap_populate/vmemmap_free? > > vmemmap_populate should already receive a page-aligned chunk because > > __populate_section_memmap made sure of that, and vmemmap_free() should be ok > > as we already filtered out at hot-adding stage. > > > > Of course, this will hold as long as struct page size of multiple of 8. > > Should that change we might get trouble, but I do not think that can ever > > happened (tm). > > > > But anyway, I am fine with placing a couple of checks in vmemmap_{populate,free} > > just to double check. > > > > What do you think? > > I'd just throw in 1 or 2 VM_BUG_ON() to self-document what we expect and > that we thought about these conditions. It's then easy to identify the > relevant commit where we explain the rationale. Fine by me, also on a second thought it is good to have some sort of clue when looking at the code. I will add that cleanup before the actual "fix" of the sub-pmd stuff. thanks!
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c index b5a3fa4033d3..dbb76160ed52 100644 --- a/arch/x86/mm/init_64.c +++ b/arch/x86/mm/init_64.c @@ -871,7 +871,72 @@ int arch_add_memory(int nid, u64 start, u64 size, return add_pages(nid, start_pfn, nr_pages, params); } -#define PAGE_INUSE 0xFD +#define PAGE_UNUSED 0xFD + +/* + * The unused vmemmap range, which was not yet memset(PAGE_UNUSED) ranges + * from unused_pmd_start to next PMD_SIZE boundary. + */ +static unsigned long unused_pmd_start __meminitdata; + +static void __meminit vmemmap_flush_unused_pmd(void) +{ + if (!unused_pmd_start) + return; + /* + * Clears (unused_pmd_start, PMD_END] + */ + memset((void *)unused_pmd_start, PAGE_UNUSED, + ALIGN(unused_pmd_start, PMD_SIZE) - unused_pmd_start); + unused_pmd_start = 0; +} + +/* Returns true if the PMD is completely unused and thus it can be freed */ +static bool __meminit vmemmap_unuse_sub_pmd(unsigned long addr, unsigned long end) +{ + unsigned long start = ALIGN_DOWN(addr, PMD_SIZE); + + vmemmap_flush_unused_pmd(); + memset((void *)addr, PAGE_UNUSED, end - addr); + + return !memchr_inv((void *)start, PAGE_UNUSED, PMD_SIZE); +} + +static void __meminit vmemmap_use_sub_pmd(unsigned long start, unsigned long end) +{ + /* + * We only optimize if the new used range directly follows the + * previously unused range (esp., when populating consecutive sections). + */ + if (unused_pmd_start == start) { + if (likely(IS_ALIGNED(end, PMD_SIZE))) + unused_pmd_start = 0; + else + unused_pmd_start = end; + return; + } + + vmemmap_flush_unused_pmd(); +} + +static void __meminit vmemmap_use_new_sub_pmd(unsigned long start, unsigned long end) +{ + vmemmap_flush_unused_pmd(); + + /* + * Mark the unused parts of the new memmap range + */ + if (!IS_ALIGNED(start, PMD_SIZE)) + memset((void *)start, PAGE_UNUSED, + start - ALIGN_DOWN(start, PMD_SIZE)); + /* + * We want to avoid memset(PAGE_UNUSED) when populating the vmemmap of + * consecutive sections. Remember for the last added PMD the last + * unused range in the populated PMD. + */ + if (!IS_ALIGNED(end, PMD_SIZE)) + unused_pmd_start = end; +} static void __meminit free_pagetable(struct page *page, int order) { @@ -1008,10 +1073,10 @@ remove_pte_table(pte_t *pte_start, unsigned long addr, unsigned long end, * with 0xFD, and remove the page when it is wholly * filled with 0xFD. */ - memset((void *)addr, PAGE_INUSE, next - addr); + memset((void *)addr, PAGE_UNUSED, next - addr); page_addr = page_address(pte_page(*pte)); - if (!memchr_inv(page_addr, PAGE_INUSE, PAGE_SIZE)) { + if (!memchr_inv(page_addr, PAGE_UNUSED, PAGE_SIZE)) { free_pagetable(pte_page(*pte), 0); spin_lock(&init_mm.page_table_lock); @@ -1034,7 +1099,6 @@ remove_pmd_table(pmd_t *pmd_start, unsigned long addr, unsigned long end, unsigned long next, pages = 0; pte_t *pte_base; pmd_t *pmd; - void *page_addr; pmd = pmd_start + pmd_index(addr); for (; addr < end; addr = next, pmd++) { @@ -1055,12 +1119,10 @@ remove_pmd_table(pmd_t *pmd_start, unsigned long addr, unsigned long end, spin_unlock(&init_mm.page_table_lock); pages++; } else { - /* If here, we are freeing vmemmap pages. */ - memset((void *)addr, PAGE_INUSE, next - addr); - - page_addr = page_address(pmd_page(*pmd)); - if (!memchr_inv(page_addr, PAGE_INUSE, - PMD_SIZE)) { + /* + * Free the PMD if the whole range is unused. + */ + if (vmemmap_unuse_sub_pmd(addr, next)) { free_hugepage_table(pmd_page(*pmd), altmap); @@ -1112,10 +1174,10 @@ remove_pud_table(pud_t *pud_start, unsigned long addr, unsigned long end, pages++; } else { /* If here, we are freeing vmemmap pages. */ - memset((void *)addr, PAGE_INUSE, next - addr); + memset((void *)addr, PAGE_UNUSED, next - addr); page_addr = page_address(pud_page(*pud)); - if (!memchr_inv(page_addr, PAGE_INUSE, + if (!memchr_inv(page_addr, PAGE_UNUSED, PUD_SIZE)) { free_pagetable(pud_page(*pud), get_order(PUD_SIZE)); @@ -1538,11 +1600,16 @@ static int __meminit vmemmap_populate_hugepages(unsigned long start, addr_end = addr + PMD_SIZE; p_end = p + PMD_SIZE; + + if (!IS_ALIGNED(addr, PMD_SIZE) || + !IS_ALIGNED(next, PMD_SIZE)) + vmemmap_use_new_sub_pmd(addr, next); continue; } else if (altmap) return -ENOMEM; /* no fallback */ } else if (pmd_large(*pmd)) { vmemmap_verify((pte_t *)pmd, node, addr, next); + vmemmap_use_sub_pmd(addr, next); continue; } if (vmemmap_populate_basepages(addr, next, node, NULL))
When the size of a struct page is not multiple of 2MB, sections do not span a PMD anymore and so when populating them some parts of the PMD will remain unused. Because of this, PMDs will be left behind when depopulating sections since remove_pmd_table() thinks that those unused parts are still in use. Fix this by marking the unused parts with PAGE_INUSE, so memchr_inv() will do the right thing and will let us free the PMD when the last user of it is gone. This patch is based on a similar patch by David Hildenbrand: https://lore.kernel.org/linux-mm/20200722094558.9828-9-david@redhat.com/ https://lore.kernel.org/linux-mm/20200722094558.9828-10-david@redhat.com/ Signed-off-by: Oscar Salvador <osalvador@suse.de> --- v1 -> v2: - Rename PAGE_INUSE to PAGE_UNUSED as it better describes what we do --- arch/x86/mm/init_64.c | 91 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 79 insertions(+), 12 deletions(-)