| Message ID | 20210309174113.5597-4-osalvador@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup and fixups for vmemmap handling | expand |
On 09.03.21 18:41, Oscar Salvador wrote: > When sizeof(struct page) is not a power of 2, 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_UNUSED, 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/ > > Signed-off-by: Oscar Salvador <osalvador@suse.de> > Reviewed-by: David Hildenbrand <david@redhat.com> > --- > arch/x86/mm/init_64.c | 63 ++++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 52 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c > index 9ecb3c488ac8..3bb3988c7681 100644 > --- a/arch/x86/mm/init_64.c > +++ b/arch/x86/mm/init_64.c > @@ -871,7 +871,50 @@ int arch_add_memory(int nid, u64 start, u64 size, > return add_pages(nid, start_pfn, nr_pages, params); > } > > -#define PAGE_INUSE 0xFD > +#ifdef CONFIG_SPARSEMEM_VMEMMAP > +#define PAGE_UNUSED 0xFD > + > +/* Returns true if the PMD is completely unused and thus it can be freed */ > +static bool __meminit vmemmap_pmd_is_unused(unsigned long addr, unsigned long end) > +{ I don't think the new name is any better. It implies that all it does is a check - yet it actually clears the given range. (I prefer the old name, but well, I came up with that, so what do I know :D )
On 3/9/21 9:41 AM, Oscar Salvador wrote: > When sizeof(struct page) is not a power of 2, 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_UNUSED, 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/ Looks good now. It's much easier to read without the optimization. Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
On Tue, Mar 09, 2021 at 06:52:38PM +0100, David Hildenbrand wrote: > > -#define PAGE_INUSE 0xFD > > +#ifdef CONFIG_SPARSEMEM_VMEMMAP > > +#define PAGE_UNUSED 0xFD > > + > > +/* Returns true if the PMD is completely unused and thus it can be freed */ > > +static bool __meminit vmemmap_pmd_is_unused(unsigned long addr, unsigned long end) > > +{ > > I don't think the new name is any better. It implies that all it does is a > check - yet it actually clears the given range. (I prefer the old name, but > well, I came up with that, so what do I know :D ) Sorry, I did not mean to offend here. Something like: vmemmap_is_pmd_unused_after_clearing_it would be a bit better I guess. Tbh, both this and previous one looked fine to me, but I understand where Dave confusion was coming from, that is why I decided to rename it. Maybe a middle-ground would have been to expand the comment above.
On 10.03.21 18:49, Oscar Salvador wrote: > On Tue, Mar 09, 2021 at 06:52:38PM +0100, David Hildenbrand wrote: >>> -#define PAGE_INUSE 0xFD >>> +#ifdef CONFIG_SPARSEMEM_VMEMMAP >>> +#define PAGE_UNUSED 0xFD >>> + >>> +/* Returns true if the PMD is completely unused and thus it can be freed */ >>> +static bool __meminit vmemmap_pmd_is_unused(unsigned long addr, unsigned long end) >>> +{ >> >> I don't think the new name is any better. It implies that all it does is a >> check - yet it actually clears the given range. (I prefer the old name, but >> well, I came up with that, so what do I know :D ) > > Sorry, I did not mean to offend here. Oh, I didn't feel offended - I was rather expressing that my opinion might be biased because I came up with these names in the s390x variant ;) > > Something like: vmemmap_is_pmd_unused_after_clearing_it would be a bit better > I guess. > Tbh, both this and previous one looked fine to me, but I understand where Dave > confusion was coming from, that is why I decided to rename it. > > Maybe a middle-ground would have been to expand the comment above. Thinking again, I guess it might be a good idea to factor out the core functions into common code. For the optimization part, it might make sense too pass some "state" structure that contains e.g., "unused_pmd_start". Then we don't have diverging implementations of essentially the same thing. Of course, we can do that on top of this series - unifying both implementations.
On Wed, Mar 10, 2021 at 06:58:26PM +0100, David Hildenbrand wrote: > Thinking again, I guess it might be a good idea to factor out the core > functions into common code. For the optimization part, it might make sense > too pass some "state" structure that contains e.g., "unused_pmd_start". Yeah, that really sounds like a good thing to do. > > Then we don't have diverging implementations of essentially the same thing. > > Of course, we can do that on top of this series - unifying both > implementations. I would rather do it on top of this series, not because I am lazy, but rather fairly busy and I will not be able to spend much time on it anytime soon. Once this series gets merged, I commit to have a look into that. Thanks!
On 10.03.21 22:58, Oscar Salvador wrote: > On Wed, Mar 10, 2021 at 06:58:26PM +0100, David Hildenbrand wrote: >> Thinking again, I guess it might be a good idea to factor out the core >> functions into common code. For the optimization part, it might make sense >> too pass some "state" structure that contains e.g., "unused_pmd_start". > > Yeah, that really sounds like a good thing to do. > >> >> Then we don't have diverging implementations of essentially the same thing. >> >> Of course, we can do that on top of this series - unifying both >> implementations. > > I would rather do it on top of this series, not because I am lazy, but > rather fairly busy and I will not be able to spend much time on it > anytime soon. > > Once this series gets merged, I commit to have a look into that. > Sure, makes sense - thanks!
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c index 9ecb3c488ac8..3bb3988c7681 100644 --- a/arch/x86/mm/init_64.c +++ b/arch/x86/mm/init_64.c @@ -871,7 +871,50 @@ int arch_add_memory(int nid, u64 start, u64 size, return add_pages(nid, start_pfn, nr_pages, params); } -#define PAGE_INUSE 0xFD +#ifdef CONFIG_SPARSEMEM_VMEMMAP +#define PAGE_UNUSED 0xFD + +/* Returns true if the PMD is completely unused and thus it can be freed */ +static bool __meminit vmemmap_pmd_is_unused(unsigned long addr, unsigned long end) +{ + unsigned long start = ALIGN_DOWN(addr, PMD_SIZE); + + 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) +{ + /* + * As we expect to add in the same granularity as we remove, it's + * sufficient to mark only some piece used to block the memmap page from + * getting removed when removing some other adjacent memmap (just in + * case the first memmap never gets initialized e.g., because the memory + * block never gets onlined). + */ + memset((void *)start, 0, sizeof(struct page)); +} + +static void __meminit vmemmap_use_new_sub_pmd(unsigned long start, unsigned long end) +{ + /* + * Could be our memmap page is filled with PAGE_UNUSED already from a + * previous remove. Make sure to reset it. + */ + vmemmap_use_sub_pmd(start); + + /* + * Mark with PAGE_UNUSED 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)); + if (!IS_ALIGNED(end, PMD_SIZE)) + memset((void *)end, PAGE_UNUSED, + ALIGN(end, PMD_SIZE) - end); +} +#endif static void __meminit free_pagetable(struct page *page, int order) { @@ -1006,7 +1049,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++) { @@ -1026,20 +1068,13 @@ remove_pmd_table(pmd_t *pmd_start, unsigned long addr, unsigned long end, pmd_clear(pmd); 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)) { + } else if (IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP) && + vmemmap_pmd_is_unused(addr, next)) { free_hugepage_table(pmd_page(*pmd), altmap); - spin_lock(&init_mm.page_table_lock); pmd_clear(pmd); spin_unlock(&init_mm.page_table_lock); - } } continue; @@ -1492,11 +1527,17 @@ 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); continue; } if (vmemmap_populate_basepages(addr, next, node, NULL))