Skip to contentSkip to conversation

s390: implement and optimize vmemmap_free()

PATCH Superseded David Hildenbrand 4 participants 20 messages Jul 3, 2020 – Jul 10, 2020

Subsystems (3)
Memory management s390 s390 MM

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

David Hildenbrand

author
#0 lore raw

cover s390: implement and optimize vmemmap_free()

This series is based on the latest s390/features branch [1]. It implements vmemmap_free(), consolidating it with vmem_add_range(), and optimizes it by
- Freeing empty page tables (now also done for idendity mapping).
- Handling cases where the vmemmap of a section does not fill huge pages completely.

vmemmap_free() is currently never used, unless adiing standby memory fails (unlikely). This is relevant for virtio-mem, which adds/removes memory in memory block/section granularity (always removes memory in the same granularity it added it).

I gave this a proper test with my virtio-mem prototype (which I will share once the basic QEMU implementation is upstream), both with 56 byte memmap per page and 64 byte memmap per page, with and without huge page support. In both cases, removing memory (routed through arch_remove_memory()) will result in
- all populated vmemmap pages to get removed/freed
- all applicable page tables for the vmemmap getting removed/freed
- all applicable page tables for the idendity mapping getting removed/freed Unfortunately, I don't have access to bigger and z/VM (esp. dcss) environments.

This is the basis for real memory hotunplug support for s390x and should complete my journey to s390x vmem/vmemmap code for now :)

What needs double-checking is tlb flushing. AFAIKS, as there are no valid accesses, doing a single range flush at the end is sufficient, both when removing vmemmap pages and the idendity mapping.

Along, some minor cleanups.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/commit/?h=features

David Hildenbrand (9):
s390/vmem: rename vmem_add_mem() to vmem_add_range() s390/vmem: recursive implementation of vmem_remove_range() s390/vmemmap: implement vmemmap_free()
s390/vmemmap: cleanup when vmemmap_populate() fails s390/vmemmap: take the vmem_mutex when populating/freeing s390/vmem: cleanup empty page tables
s390/vmemmap: fallback to PTEs if mapping large PMD fails s390/vmemmap: remember unused sub-pmd ranges s390/vmemmap: avoid memset(PAGE_UNUSED) when adding consecutive
sections

 arch/s390/mm/vmem.c | 400 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 338 insertions(+), 62 deletions(-)
Signature · 1 line
2.26.2
author
#1 lore raw

PATCH 1/9 s390/vmem: rename vmem_add_mem() to vmem_add_range()

Let's match the name to vmem_remove_range().

Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Gerald Schaefer <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 arch/s390/mm/vmem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
arch/s390/mm/vmem.c +3 -3
index 3b9e71654c37b..66c5333020ead 100644
@@ -57,7 +57,7 @@ pte_t __ref *vmem_pte_alloc(void)
 /*
  * Add a physical memory range to the 1:1 mapping.
  */
-static int vmem_add_mem(unsigned long start, unsigned long size)
+static int vmem_add_range(unsigned long start, unsigned long size)
 {
 	unsigned long pgt_prot, sgt_prot, r3_prot;
 	unsigned long pages4k, pages1m, pages2g;

@@ -308,7 +308,7 @@ int vmem_add_mapping(unsigned long start, unsigned long size)
 		return -ERANGE;
 
 	mutex_lock(&vmem_mutex);
-	ret = vmem_add_mem(start, size);
+	ret = vmem_add_range(start, size);
 	if (ret)
 		vmem_remove_range(start, size);
 	mutex_unlock(&vmem_mutex);

@@ -325,7 +325,7 @@ void __init vmem_map_init(void)
 	struct memblock_region *reg;
 
 	for_each_memblock(memory, reg)
-		vmem_add_mem(reg->base, reg->size);
+		vmem_add_range(reg->base, reg->size);
 	__set_memory((unsigned long)_stext,
 		     (unsigned long)(_etext - _stext) >> PAGE_SHIFT,
 		     SET_MEMORY_RO | SET_MEMORY_X);
Signature · 1 line
2.26.2
author
#2 lore raw

PATCH 2/9 s390/vmem: recursive implementation of vmem_remove_range()

In reply to David Hildenbrand

We want to reuse the same functionality in vmemmap_free(). Let's start by introducing recursive remove_pagetable(), inspired by x86. We'll extend it to cover the vmemmap similarly next.

A recursive implementation makes it easier to expand individual cases without harming readability. In addition, we minimize traversing the whole hierarchy over and over again.

One change is that we don't unmap large PMDs/PUDs when not completely covered by the request, something that should never happen with direct mappings, unless one would be removing in other granularity than added, which would be broken already.

Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Gerald Schaefer <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 arch/s390/mm/vmem.c | 153 +++++++++++++++++++++++++++++++-------------
 1 file changed, 107 insertions(+), 46 deletions(-)
arch/s390/mm/vmem.c +107 -46
index 66c5333020ead..6fe156c3f035c 100644
@@ -138,64 +138,125 @@ static int vmem_add_range(unsigned long start, unsigned long size)
 	return ret;
 }
 
-/*
- * Remove a physical memory range from the 1:1 mapping.
- * Currently only invalidates page table entries.
- */
-static void vmem_remove_range(unsigned long start, unsigned long size)
+static void remove_pte_table(pmd_t *pmd, unsigned long addr,
+			     unsigned long end)
 {
-	unsigned long pages4k, pages1m, pages2g;
-	unsigned long end = start + size;
-	unsigned long address = start;
-	pgd_t *pg_dir;
-	p4d_t *p4_dir;
-	pud_t *pu_dir;
-	pmd_t *pm_dir;
-	pte_t *pt_dir;
+	unsigned long pages = 0;
+	pte_t *pte;
 
-	pages4k = pages1m = pages2g = 0;
-	while (address < end) {
-		pg_dir = pgd_offset_k(address);
-		if (pgd_none(*pg_dir)) {
-			address += PGDIR_SIZE;
+	pte = pte_offset_kernel(pmd, addr);
+	for (; addr < end; addr += PAGE_SIZE, pte++) {
+		if (pte_none(*pte))
 			continue;
-		}
-		p4_dir = p4d_offset(pg_dir, address);
-		if (p4d_none(*p4_dir)) {
-			address += P4D_SIZE;
+
+		pte_clear(&init_mm, addr, pte);
+		pages++;
+	}
+
+	update_page_count(PG_DIRECT_MAP_4K, -pages);
+}
+
+static void remove_pmd_table(pud_t *pud, unsigned long addr,
+			     unsigned long end)
+{
+	unsigned long next, pages = 0;
+	pmd_t *pmd;
+
+	pmd = pmd_offset(pud, addr);
+	for (; addr < end; addr = next, pmd++) {
+		next = pmd_addr_end(addr, end);
+
+		if (pmd_none(*pmd))
 			continue;
-		}
-		pu_dir = pud_offset(p4_dir, address);
-		if (pud_none(*pu_dir)) {
-			address += PUD_SIZE;
+
+		if (pmd_large(*pmd)) {
+			if (IS_ALIGNED(addr, PMD_SIZE) &&
+			    IS_ALIGNED(next, PMD_SIZE)) {
+				pmd_clear(pmd);
+				pages++;
+			}
 			continue;
 		}
-		if (pud_large(*pu_dir)) {
-			pud_clear(pu_dir);
-			address += PUD_SIZE;
-			pages2g++;
+
+		remove_pte_table(pmd, addr, next);
+	}
+
+	update_page_count(PG_DIRECT_MAP_1M, -pages);
+}
+
+static void remove_pud_table(p4d_t *p4d, unsigned long addr,
+			     unsigned long end)
+{
+	unsigned long next, pages = 0;
+	pud_t *pud;
+
+	pud = pud_offset(p4d, addr);
+	for (; addr < end; addr = next, pud++) {
+		next = pud_addr_end(addr, end);
+
+		if (pud_none(*pud))
 			continue;
-		}
-		pm_dir = pmd_offset(pu_dir, address);
-		if (pmd_none(*pm_dir)) {
-			address += PMD_SIZE;
+
+		if (pud_large(*pud)) {
+			if (IS_ALIGNED(addr, PUD_SIZE) &&
+			    IS_ALIGNED(next, PUD_SIZE)) {
+				pud_clear(pud);
+				pages++;
+			}
 			continue;
 		}
-		if (pmd_large(*pm_dir)) {
-			pmd_clear(pm_dir);
-			address += PMD_SIZE;
-			pages1m++;
+
+		remove_pmd_table(pud, addr, next);
+	}
+
+	update_page_count(PG_DIRECT_MAP_2G, -pages);
+}
+
+static void remove_p4d_table(pgd_t *pgd, unsigned long addr,
+			     unsigned long end)
+{
+	unsigned long next;
+	p4d_t *p4d;
+
+	p4d = p4d_offset(pgd, addr);
+	for (; addr < end; addr = next, p4d++) {
+		next = p4d_addr_end(addr, end);
+
+		if (p4d_none(*p4d))
 			continue;
-		}
-		pt_dir = pte_offset_kernel(pm_dir, address);
-		pte_clear(&init_mm, address, pt_dir);
-		address += PAGE_SIZE;
-		pages4k++;
+
+		remove_pud_table(p4d, addr, next);
 	}
+}
+
+static void remove_pagetable(unsigned long start, unsigned long end)
+{
+	unsigned long addr, next;
+	pgd_t *pgd;
+
+	if (WARN_ON_ONCE(!PAGE_ALIGNED(start | end)))
+		return;
+
+	for (addr = start; addr < end; addr = next) {
+		next = pgd_addr_end(addr, end);
+		pgd = pgd_offset_k(addr);
+
+		if (pgd_none(*pgd))
+			continue;
+
+		remove_p4d_table(pgd, addr, next);
+	}
+
 	flush_tlb_kernel_range(start, end);
-	update_page_count(PG_DIRECT_MAP_4K, -pages4k);
-	update_page_count(PG_DIRECT_MAP_1M, -pages1m);
-	update_page_count(PG_DIRECT_MAP_2G, -pages2g);
+}
+
+/*
+ * Remove a physical memory range from the 1:1 mapping.
+ * Currently only invalidates page table entries.
+ */
+static void vmem_remove_range(unsigned long start, unsigned long size)
+{
+	remove_pagetable(start, start + size);
 }
 
 /*
Signature · 1 line
2.26.2
author
#3 lore raw

PATCH 3/9 s390/vmemmap: implement vmemmap_free()

In reply to David Hildenbrand

Reuse the shiny new remove_pagetable(), tweaking it to handle freeing of vmemmap pages, similar to the x86-64 variant (passing "bool direct" to distinguish).

Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Gerald Schaefer <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 arch/s390/mm/vmem.c | 46 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 13 deletions(-)
arch/s390/mm/vmem.c +33 -13
index 6fe156c3f035c..16e109c292bf5 100644
@@ -29,6 +29,15 @@ static void __ref *vmem_alloc_pages(unsigned int order)
 	return (void *) memblock_phys_alloc(size, size);
 }
 
+static void vmem_free_pages(unsigned long addr, int order)
+{
+	/* We don't expect boot memory to be removed ever. */
+	if (!slab_is_available() ||
+	    WARN_ON_ONCE(PageReserved(phys_to_page(addr))))
+		return;
+	free_pages(addr, order);
+}
+
 void *vmem_crst_alloc(unsigned long val)
 {
 	unsigned long *table;

@@ -139,7 +148,7 @@ static int vmem_add_range(unsigned long start, unsigned long size)
 }
 
 static void remove_pte_table(pmd_t *pmd, unsigned long addr,
-			     unsigned long end)
+			     unsigned long end, bool direct)
 {
 	unsigned long pages = 0;
 	pte_t *pte;

@@ -149,15 +158,18 @@ static void remove_pte_table(pmd_t *pmd, unsigned long addr,
 		if (pte_none(*pte))
 			continue;
 
+		if (!direct)
+			vmem_free_pages(pfn_to_phys(pte_pfn(*pte)), 0);
 		pte_clear(&init_mm, addr, pte);
 		pages++;
 	}
 
-	update_page_count(PG_DIRECT_MAP_4K, -pages);
+	if (direct)
+		update_page_count(PG_DIRECT_MAP_4K, -pages);
 }
 
 static void remove_pmd_table(pud_t *pud, unsigned long addr,
-			     unsigned long end)
+			     unsigned long end, bool direct)
 {
 	unsigned long next, pages = 0;
 	pmd_t *pmd;

@@ -172,20 +184,24 @@ static void remove_pmd_table(pud_t *pud, unsigned long addr,
 		if (pmd_large(*pmd)) {
 			if (IS_ALIGNED(addr, PMD_SIZE) &&
 			    IS_ALIGNED(next, PMD_SIZE)) {
+				if (!direct)
+					vmem_free_pages(pmd_deref(*pmd),
+							get_order(PMD_SIZE));
 				pmd_clear(pmd);
 				pages++;
 			}
 			continue;
 		}
 
-		remove_pte_table(pmd, addr, next);
+		remove_pte_table(pmd, addr, next, direct);
 	}
 
-	update_page_count(PG_DIRECT_MAP_1M, -pages);
+	if (direct)
+		update_page_count(PG_DIRECT_MAP_1M, -pages);
 }
 
 static void remove_pud_table(p4d_t *p4d, unsigned long addr,
-			     unsigned long end)
+			     unsigned long end, bool direct)
 {
 	unsigned long next, pages = 0;
 	pud_t *pud;

@@ -200,20 +216,22 @@ static void remove_pud_table(p4d_t *p4d, unsigned long addr,
 		if (pud_large(*pud)) {
 			if (IS_ALIGNED(addr, PUD_SIZE) &&
 			    IS_ALIGNED(next, PUD_SIZE)) {
+				WARN_ON_ONCE(!direct);
 				pud_clear(pud);
 				pages++;
 			}
 			continue;
 		}
 
-		remove_pmd_table(pud, addr, next);
+		remove_pmd_table(pud, addr, next, direct);
 	}
 
-	update_page_count(PG_DIRECT_MAP_2G, -pages);
+	if (direct)
+		update_page_count(PG_DIRECT_MAP_2G, -pages);
 }
 
 static void remove_p4d_table(pgd_t *pgd, unsigned long addr,
-			     unsigned long end)
+			     unsigned long end, bool direct)
 {
 	unsigned long next;
 	p4d_t *p4d;

@@ -225,11 +243,12 @@ static void remove_p4d_table(pgd_t *pgd, unsigned long addr,
 		if (p4d_none(*p4d))
 			continue;
 
-		remove_pud_table(p4d, addr, next);
+		remove_pud_table(p4d, addr, next, direct);
 	}
 }
 
-static void remove_pagetable(unsigned long start, unsigned long end)
+static void remove_pagetable(unsigned long start, unsigned long end,
+			     bool direct)
 {
 	unsigned long addr, next;
 	pgd_t *pgd;

@@ -244,7 +263,7 @@ static void remove_pagetable(unsigned long start, unsigned long end)
 		if (pgd_none(*pgd))
 			continue;
 
-		remove_p4d_table(pgd, addr, next);
+		remove_p4d_table(pgd, addr, next, direct);
 	}
 
 	flush_tlb_kernel_range(start, end);

@@ -256,7 +275,7 @@ static void remove_pagetable(unsigned long start, unsigned long end)
  */
 static void vmem_remove_range(unsigned long start, unsigned long size)
 {
-	remove_pagetable(start, start + size);
+	remove_pagetable(start, start + size, true);
 }
 
 /*

@@ -351,6 +370,7 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 void vmemmap_free(unsigned long start, unsigned long end,
 		struct vmem_altmap *altmap)
 {
+	remove_pagetable(start, end, false);
 }
 
 void vmem_remove_mapping(unsigned long start, unsigned long size)
Signature · 1 line
2.26.2
author
#4 lore raw

PATCH 4/9 s390/vmemmap: cleanup when vmemmap_populate() fails

In reply to David Hildenbrand

Cleanup what we partially added in case vmemmap_populate() fails.

Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Gerald Schaefer <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 arch/s390/mm/vmem.c | 2 ++
 1 file changed, 2 insertions(+)
arch/s390/mm/vmem.c +2 -0
index 16e109c292bf5..bcddabd509da8 100644
@@ -364,6 +364,8 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 	}
 	ret = 0;
 out:
+	if (ret)
+		vmemmap_free(start, end, altmap);
 	return ret;
 }
Signature · 1 line
2.26.2
build bot
#5 lore raw

Re: [PATCH v1 4/9] s390/vmemmap: cleanup when vmemmap_populate() fails

I love your patch! Yet something to improve:

Hi David,

I love your patch! Yet something to improve:

[auto build test ERROR on s390/features]
[also build test ERROR on next-20200703]
[cannot apply to linux/master kvms390/next linus/master v5.8-rc3] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/David-Hildenbrand/s390-implement-and-optimize-vmemmap_free/20200703-214348
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: s390-alldefconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=s390

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All error/warnings (new ones prefixed by >>):

arch/s390/mm/vmem.c: In function 'vmemmap_populate':

>> arch/s390/mm/vmem.c:368:3: error: implicit declaration of function 'vmemmap_free'; did you mean 'vmem_altmap_free'? [-Werror=implicit-function-declaration]
     368 |   vmemmap_free(start, end, altmap);
         |   ^~~~~~~~~~~~
         |   vmem_altmap_free
   arch/s390/mm/vmem.c: At top level:
   arch/s390/mm/vmem.c:372:6: warning: no previous prototype for 'vmemmap_free' [-Wmissing-prototypes]
     372 | void vmemmap_free(unsigned long start, unsigned long end,
         |      ^~~~~~~~~~~~
>> arch/s390/mm/vmem.c:372:6: warning: conflicting types for 'vmemmap_free'
   arch/s390/mm/vmem.c:368:3: note: previous implicit declaration of 'vmemmap_free' was here
     368 |   vmemmap_free(start, end, altmap);
         |   ^~~~~~~~~~~~

cc1: some warnings being treated as errors

vim +368 arch/s390/mm/vmem.c

280
281 /*
282 * Add a backed mem_map array to the virtual mem_map array.
283 */
284 int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
285 struct vmem_altmap *altmap)
286 {
287 unsigned long pgt_prot, sgt_prot;
288 unsigned long address = start;
289 pgd_t *pg_dir;
290 p4d_t *p4_dir;
291 pud_t *pu_dir;
292 pmd_t *pm_dir;
293 pte_t *pt_dir;
294 int ret = -ENOMEM;
295
296 pgt_prot = pgprot_val(PAGE_KERNEL);
297 sgt_prot = pgprot_val(SEGMENT_KERNEL);
298 if (!MACHINE_HAS_NX) {
299 pgt_prot &= ~_PAGE_NOEXEC;
300 sgt_prot &= ~_SEGMENT_ENTRY_NOEXEC;
301 }
302 for (address = start; address < end;) {
303 pg_dir = pgd_offset_k(address);
304 if (pgd_none(*pg_dir)) {
305 p4_dir = vmem_crst_alloc(_REGION2_ENTRY_EMPTY);
306 if (!p4_dir)
307 goto out;
308 pgd_populate(&init_mm, pg_dir, p4_dir);
309 }
310
311 p4_dir = p4d_offset(pg_dir, address);
312 if (p4d_none(*p4_dir)) {
313 pu_dir = vmem_crst_alloc(_REGION3_ENTRY_EMPTY);
314 if (!pu_dir)
315 goto out;
316 p4d_populate(&init_mm, p4_dir, pu_dir);
317 }
318
319 pu_dir = pud_offset(p4_dir, address);
320 if (pud_none(*pu_dir)) {
321 pm_dir = vmem_crst_alloc(_SEGMENT_ENTRY_EMPTY);
322 if (!pm_dir)
323 goto out;
324 pud_populate(&init_mm, pu_dir, pm_dir);
325 }
326
327 pm_dir = pmd_offset(pu_dir, address);
328 if (pmd_none(*pm_dir)) {
329 /* Use 1MB frames for vmemmap if available. We always
330 * use large frames even if they are only partially
331 * used.
332 * Otherwise we would have also page tables since
333 * vmemmap_populate gets called for each section
334 * separately. */
335 if (MACHINE_HAS_EDAT1) {
336 void *new_page;
337
338 new_page = vmemmap_alloc_block(PMD_SIZE, node);
339 if (!new_page)
340 goto out;
341 pmd_val(*pm_dir) = __pa(new_page) | sgt_prot;
342 address = (address + PMD_SIZE) & PMD_MASK;
343 continue;
344 }
345 pt_dir = vmem_pte_alloc();
346 if (!pt_dir)
347 goto out;
348 pmd_populate(&init_mm, pm_dir, pt_dir);
349 } else if (pmd_large(*pm_dir)) {
350 address = (address + PMD_SIZE) & PMD_MASK;
351 continue;
352 }
353
354 pt_dir = pte_offset_kernel(pm_dir, address);
355 if (pte_none(*pt_dir)) {
356 void *new_page;
357
358 new_page = vmemmap_alloc_block(PAGE_SIZE, node);
359 if (!new_page)
360 goto out;
361 pte_val(*pt_dir) = __pa(new_page) | pgt_prot;
362 }
363 address += PAGE_SIZE;
364 }
365 ret = 0;
366 out:
367 if (ret)

 > 368			vmemmap_free(start, end, altmap);
   369		return ret;
   370	}
   371	
 > 372	void vmemmap_free(unsigned long start, unsigned long end,

373 struct vmem_altmap *altmap)
374 {
375 remove_pagetable(start, end, false);
376 }
377

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

author
#6 lore raw

Re: [PATCH v1 4/9] s390/vmemmap: cleanup when vmemmap_populate() fails

Quoted 35 lines · kernel test robot

Hi David,

I love your patch! Yet something to improve:

[auto build test ERROR on s390/features]
[also build test ERROR on next-20200703]
[cannot apply to linux/master kvms390/next linus/master v5.8-rc3] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/David-Hildenbrand/s390-implement-and-optimize-vmemmap_free/20200703-214348
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: s390-alldefconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=s390

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All error/warnings (new ones prefixed by >>):

arch/s390/mm/vmem.c: In function 'vmemmap_populate':

Quoted 1 line
Quoted 1 line

arch/s390/mm/vmem.c:368:3: error: implicit declaration of function 'vmemmap_free'; did you mean 'vmem_altmap_free'? [-Werror=implicit-function-declaration]

     368 |   vmemmap_free(start, end, altmap);
         |   ^~~~~~~~~~~~
         |   vmem_altmap_free

arch/s390/mm/vmem.c: At top level:
arch/s390/mm/vmem.c:372:6: warning: no previous prototype for 'vmemmap_free' [-Wmissing-prototypes]
372 | void vmemmap_free(unsigned long start, unsigned long end,
| ^~~~~~~~~~~~

Quoted 5 lines
Quoted 1 line
Quoted 1 line

arch/s390/mm/vmem.c:372:6: warning: conflicting types for 'vmemmap_free'

arch/s390/mm/vmem.c:368:3: note: previous implicit declaration of 'vmemmap_free' was here

     368 |   vmemmap_free(start, end, altmap);
         |   ^~~~~~~~~~~~

cc1: some warnings being treated as errors

Ah, vmemmap_free() is only defined with CONFIG_MEMORY_HOTPLUG. Easy to fix.

Signature · 3 lines
Thanks,

David / dhildenb
build bot
#7 lore raw

Re: [PATCH v1 4/9] s390/vmemmap: cleanup when vmemmap_populate() fails

In reply to David Hildenbrand
I love your patch! Yet something to improve:

Hi David,

I love your patch! Yet something to improve:

[auto build test ERROR on s390/features]
[also build test ERROR on next-20200703]
[cannot apply to linux/master kvms390/next linus/master v5.8-rc3] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/David-Hildenbrand/s390-implement-and-optimize-vmemmap_free/20200703-214348
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: s390-randconfig-r036-20200701 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project ca464639a1c9dd3944eb055ffd2796e8c2e7639f)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:19:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x000000ffUL) << 24) |            \
                     ^
   In file included from arch/s390/mm/vmem.c:7:
   In file included from include/linux/memblock.h:14:
   In file included from arch/s390/include/asm/dma.h:5:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:20:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x0000ff00UL) <<  8) |            \
                     ^
   In file included from arch/s390/mm/vmem.c:7:
   In file included from include/linux/memblock.h:14:
   In file included from arch/s390/include/asm/dma.h:5:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |            \
                     ^
   In file included from arch/s390/mm/vmem.c:7:
   In file included from include/linux/memblock.h:14:
   In file included from arch/s390/include/asm/dma.h:5:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0xff000000UL) >> 24)))
                     ^
   In file included from arch/s390/mm/vmem.c:7:
   In file included from include/linux/memblock.h:14:
   In file included from arch/s390/include/asm/dma.h:5:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:120:12: note: expanded from macro '__swab32'
           __fswab32(x))
                     ^
   In file included from arch/s390/mm/vmem.c:7:
   In file included from include/linux/memblock.h:14:
   In file included from arch/s390/include/asm/dma.h:5:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:46: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew(cpu_to_le16(value), PCI_IOBASE + addr);
                                            ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:46: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel(cpu_to_le32(value), PCI_IOBASE + addr);
                                            ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> arch/s390/mm/vmem.c:368:3: error: implicit declaration of function 'vmemmap_free' [-Werror,-Wimplicit-function-declaration]
                   vmemmap_free(start, end, altmap);
                   ^
>> arch/s390/mm/vmem.c:372:6: error: conflicting types for 'vmemmap_free'
   void vmemmap_free(unsigned long start, unsigned long end,
        ^
   arch/s390/mm/vmem.c:368:3: note: previous implicit declaration is here
                   vmemmap_free(start, end, altmap);
                   ^

20 warnings and 2 errors generated.

vim +/vmemmap_free +368 arch/s390/mm/vmem.c

280
281 /*
282 * Add a backed mem_map array to the virtual mem_map array.
283 */
284 int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
285 struct vmem_altmap *altmap)
286 {
287 unsigned long pgt_prot, sgt_prot;
288 unsigned long address = start;
289 pgd_t *pg_dir;
290 p4d_t *p4_dir;
291 pud_t *pu_dir;
292 pmd_t *pm_dir;
293 pte_t *pt_dir;
294 int ret = -ENOMEM;
295
296 pgt_prot = pgprot_val(PAGE_KERNEL);
297 sgt_prot = pgprot_val(SEGMENT_KERNEL);
298 if (!MACHINE_HAS_NX) {
299 pgt_prot &= ~_PAGE_NOEXEC;
300 sgt_prot &= ~_SEGMENT_ENTRY_NOEXEC;
301 }
302 for (address = start; address < end;) {
303 pg_dir = pgd_offset_k(address);
304 if (pgd_none(*pg_dir)) {
305 p4_dir = vmem_crst_alloc(_REGION2_ENTRY_EMPTY);
306 if (!p4_dir)
307 goto out;
308 pgd_populate(&init_mm, pg_dir, p4_dir);
309 }
310
311 p4_dir = p4d_offset(pg_dir, address);
312 if (p4d_none(*p4_dir)) {
313 pu_dir = vmem_crst_alloc(_REGION3_ENTRY_EMPTY);
314 if (!pu_dir)
315 goto out;
316 p4d_populate(&init_mm, p4_dir, pu_dir);
317 }
318
319 pu_dir = pud_offset(p4_dir, address);
320 if (pud_none(*pu_dir)) {
321 pm_dir = vmem_crst_alloc(_SEGMENT_ENTRY_EMPTY);
322 if (!pm_dir)
323 goto out;
324 pud_populate(&init_mm, pu_dir, pm_dir);
325 }
326
327 pm_dir = pmd_offset(pu_dir, address);
328 if (pmd_none(*pm_dir)) {
329 /* Use 1MB frames for vmemmap if available. We always
330 * use large frames even if they are only partially
331 * used.
332 * Otherwise we would have also page tables since
333 * vmemmap_populate gets called for each section
334 * separately. */
335 if (MACHINE_HAS_EDAT1) {
336 void *new_page;
337
338 new_page = vmemmap_alloc_block(PMD_SIZE, node);
339 if (!new_page)
340 goto out;
341 pmd_val(*pm_dir) = __pa(new_page) | sgt_prot;
342 address = (address + PMD_SIZE) & PMD_MASK;
343 continue;
344 }
345 pt_dir = vmem_pte_alloc();
346 if (!pt_dir)
347 goto out;
348 pmd_populate(&init_mm, pm_dir, pt_dir);
349 } else if (pmd_large(*pm_dir)) {
350 address = (address + PMD_SIZE) & PMD_MASK;
351 continue;
352 }
353
354 pt_dir = pte_offset_kernel(pm_dir, address);
355 if (pte_none(*pt_dir)) {
356 void *new_page;
357
358 new_page = vmemmap_alloc_block(PAGE_SIZE, node);
359 if (!new_page)
360 goto out;
361 pte_val(*pt_dir) = __pa(new_page) | pgt_prot;
362 }
363 address += PAGE_SIZE;
364 }
365 ret = 0;
366 out:
367 if (ret)

 > 368			vmemmap_free(start, end, altmap);
   369		return ret;
   370	}
   371	
 > 372	void vmemmap_free(unsigned long start, unsigned long end,

373 struct vmem_altmap *altmap)
374 {
375 remove_pagetable(start, end, false);
376 }
377

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

author
#8 lore raw

PATCH 5/9 s390/vmemmap: take the vmem_mutex when populating/freeing

In reply to David Hildenbrand

Let's synchronize all accesses to the 1:1 and vmemmap mappings. This will be especially relevant when wanting to cleanup empty page tables that could be shared by both. Avoid races when removing tables that might be just about to get reused.

Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Gerald Schaefer <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 arch/s390/mm/vmem.c | 4 ++++
 1 file changed, 4 insertions(+)
arch/s390/mm/vmem.c +4 -0
index bcddabd509da8..aa968f67d7f9f 100644
@@ -293,6 +293,7 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 	pte_t *pt_dir;
 	int ret = -ENOMEM;
 
+	mutex_lock(&vmem_mutex);
 	pgt_prot = pgprot_val(PAGE_KERNEL);
 	sgt_prot = pgprot_val(SEGMENT_KERNEL);
 	if (!MACHINE_HAS_NX) {

@@ -364,6 +365,7 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 	}
 	ret = 0;
 out:
+	mutex_unlock(&vmem_mutex);
 	if (ret)
 		vmemmap_free(start, end, altmap);
 	return ret;

@@ -372,7 +374,9 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 void vmemmap_free(unsigned long start, unsigned long end,
 		struct vmem_altmap *altmap)
 {
+	mutex_lock(&vmem_mutex);
 	remove_pagetable(start, end, false);
+	mutex_unlock(&vmem_mutex);
 }
 
 void vmem_remove_mapping(unsigned long start, unsigned long size)
Signature · 1 line
2.26.2
author
#9 lore raw

PATCH 6/9 s390/vmem: cleanup empty page tables

In reply to David Hildenbrand

Let's cleanup empty page tables. Consider only page tables that fully fall into the idendity mapping and the vmemmap range.

As there are no valid accesses to vmem/vmemmap within non-populated ranges, the single tlb flush at the end should be sufficient.

Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Gerald Schaefer <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 arch/s390/mm/vmem.c | 98 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 97 insertions(+), 1 deletion(-)
arch/s390/mm/vmem.c +97 -1
index aa968f67d7f9f..5239130770b7b 100644
@@ -63,6 +63,15 @@ pte_t __ref *vmem_pte_alloc(void)
 	return pte;
 }
 
+static void vmem_pte_free(unsigned long *table)
+{
+	/* We don't expect boot memory to be removed ever. */
+	if (!slab_is_available() ||
+	    WARN_ON_ONCE(PageReserved(virt_to_page(table))))
+		return;
+	page_table_free(&init_mm, table);
+}
+
 /*
  * Add a physical memory range to the 1:1 mapping.
  */

@@ -168,6 +177,21 @@ static void remove_pte_table(pmd_t *pmd, unsigned long addr,
 		update_page_count(PG_DIRECT_MAP_4K, -pages);
 }
 
+static void try_free_pte_table(pmd_t *pmd, unsigned long start)
+{
+	pte_t *pte;
+	int i;
+
+	/* We can safely assume this is fully in 1:1 mapping & vmemmap area */
+	pte = pte_offset_kernel(pmd, start);
+	for (i = 0; i < PTRS_PER_PTE; i++, pte++)
+		if (!pte_none(*pte))
+			return;
+
+	vmem_pte_free(__va(pmd_deref(*pmd)));
+	pmd_clear(pmd);
+}
+
 static void remove_pmd_table(pud_t *pud, unsigned long addr,
 			     unsigned long end, bool direct)
 {

@@ -194,12 +218,36 @@ static void remove_pmd_table(pud_t *pud, unsigned long addr,
 		}
 
 		remove_pte_table(pmd, addr, next, direct);
+		try_free_pte_table(pmd, addr & PMD_MASK);
 	}
 
 	if (direct)
 		update_page_count(PG_DIRECT_MAP_1M, -pages);
 }
 
+static void try_free_pmd_table(pud_t *pud, unsigned long start)
+{
+	const unsigned long end = start + PUD_SIZE;
+	pmd_t *pmd;
+	int i;
+
+	/* Don't mess with any tables not fully in 1:1 mapping & vmemmap area */
+	if (end > VMALLOC_START)
+		return;
+#ifdef CONFIG_KASAN
+	if (start < KASAN_SHADOW_END && KASAN_SHADOW_START > end)
+		return;
+#endif
+
+	pmd = pmd_offset(pud, start);
+	for (i = 0; i < PTRS_PER_PMD; i++, pmd++)
+		if (!pmd_none(*pmd))
+			return;
+
+	vmem_free_pages(pud_deref(*pud), CRST_ALLOC_ORDER);
+	pud_clear(pud);
+}
+
 static void remove_pud_table(p4d_t *p4d, unsigned long addr,
 			     unsigned long end, bool direct)
 {

@@ -224,12 +272,36 @@ static void remove_pud_table(p4d_t *p4d, unsigned long addr,
 		}
 
 		remove_pmd_table(pud, addr, next, direct);
+		try_free_pmd_table(pud, addr & PUD_MASK);
 	}
 
 	if (direct)
 		update_page_count(PG_DIRECT_MAP_2G, -pages);
 }
 
+static void try_free_pud_table(p4d_t *p4d, unsigned long start)
+{
+	const unsigned long end = start + P4D_SIZE;
+	pud_t *pud;
+	int i;
+
+	/* Don't mess with any tables not fully in 1:1 mapping & vmemmap area */
+	if (end > VMALLOC_START)
+		return;
+#ifdef CONFIG_KASAN
+	if (start < KASAN_SHADOW_END && KASAN_SHADOW_START > end)
+		return;
+#endif
+
+	pud = pud_offset(p4d, start);
+	for (i = 0; i < PTRS_PER_PUD; i++, pud++)
+		if (!pud_none(*pud))
+			return;
+
+	vmem_free_pages(p4d_deref(*p4d), CRST_ALLOC_ORDER);
+	p4d_clear(p4d);
+}
+
 static void remove_p4d_table(pgd_t *pgd, unsigned long addr,
 			     unsigned long end, bool direct)
 {

@@ -244,9 +316,33 @@ static void remove_p4d_table(pgd_t *pgd, unsigned long addr,
 			continue;
 
 		remove_pud_table(p4d, addr, next, direct);
+		try_free_pud_table(p4d, addr & P4D_MASK);
 	}
 }
 
+static void try_free_p4d_table(pgd_t *pgd, unsigned long start)
+{
+	const unsigned long end = start + PGDIR_SIZE;
+	p4d_t *p4d;
+	int i;
+
+	/* Don't mess with any tables not fully in 1:1 mapping & vmemmap area */
+	if (end > VMALLOC_START)
+		return;
+#ifdef CONFIG_KASAN
+	if (start < KASAN_SHADOW_END && KASAN_SHADOW_START > end)
+		return;
+#endif
+
+	p4d = p4d_offset(pgd, start);
+	for (i = 0; i < PTRS_PER_P4D; i++, p4d++)
+		if (!p4d_none(*p4d))
+			return;
+
+	vmem_free_pages(pgd_deref(*pgd), CRST_ALLOC_ORDER);
+	pgd_clear(pgd);
+}
+
 static void remove_pagetable(unsigned long start, unsigned long end,
 			     bool direct)
 {

@@ -264,6 +360,7 @@ static void remove_pagetable(unsigned long start, unsigned long end,
 			continue;
 
 		remove_p4d_table(pgd, addr, next, direct);
+		try_free_p4d_table(pgd, addr & PGDIR_MASK);
 	}
 
 	flush_tlb_kernel_range(start, end);

@@ -271,7 +368,6 @@ static void remove_pagetable(unsigned long start, unsigned long end,
 
 /*
  * Remove a physical memory range from the 1:1 mapping.
- * Currently only invalidates page table entries.
  */
 static void vmem_remove_range(unsigned long start, unsigned long size)
 {
Signature · 1 line
2.26.2
author
#10 lore raw

PATCH 7/9 s390/vmemmap: fallback to PTEs if mapping large PMD fails

In reply to David Hildenbrand

Let's fallback to single pages if short on huge pages. No need to stop memory hotplug.

Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Gerald Schaefer <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 arch/s390/mm/vmem.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
arch/s390/mm/vmem.c +7 -7
index 5239130770b7b..b7fdb9536707f 100644
@@ -422,23 +422,23 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 		}
 
 		pm_dir = pmd_offset(pu_dir, address);
-		if (pmd_none(*pm_dir)) {
+		if (pmd_none(*pm_dir) && MACHINE_HAS_EDAT1) {
+			void *new_page;
+
 			/* Use 1MB frames for vmemmap if available. We always
 			 * use large frames even if they are only partially
 			 * used.
 			 * Otherwise we would have also page tables since
 			 * vmemmap_populate gets called for each section
 			 * separately. */
-			if (MACHINE_HAS_EDAT1) {
-				void *new_page;
-
-				new_page = vmemmap_alloc_block(PMD_SIZE, node);
-				if (!new_page)
-					goto out;
+			new_page = vmemmap_alloc_block(PMD_SIZE, node);
+			if (new_page) {
 				pmd_val(*pm_dir) = __pa(new_page) | sgt_prot;
 				address = (address + PMD_SIZE) & PMD_MASK;
 				continue;
 			}
+		}
+		if (pmd_none(*pm_dir)) {
 			pt_dir = vmem_pte_alloc();
 			if (!pt_dir)
 				goto out;
Signature · 1 line
2.26.2
author
#11 lore raw

PATCH 8/9 s390/vmemmap: remember unused sub-pmd ranges

In reply to David Hildenbrand

With a memmap size of 56 bytes or 72 bytes per page, the memmap for a 256 MB section won't span full PMDs. As we populate single sections and depopulate single sections, the depopulation step would not be able to free all vmemmap pmds anymore.

Do it similarly to x86, marking the unused memmap ranges in a special way (pad it with 0xFD).

This allows us to add/remove sections, cleaning up all allocated vmemmap pages even if the memmap size is not multiple of 16 bytes per page.

A 56 byte memmap can, for example, be created with !CONFIG_MEMCG and !CONFIG_SLUB.

Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Gerald Schaefer <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 arch/s390/mm/vmem.c | 66 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 60 insertions(+), 6 deletions(-)
arch/s390/mm/vmem.c +60 -6
index b7fdb9536707f..a981ff5d47223 100644
@@ -72,6 +72,42 @@ static void vmem_pte_free(unsigned long *table)
 	page_table_free(&init_mm, table);
 }
 
+#define PAGE_UNUSED 0xFD
+
+static void vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
+{
+	/*
+	 * 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 (just in case the memmap never gets initialized,
+	 * e.g., because the memory block never gets onlined).
+	 */
+	memset(__va(start), 0, sizeof(struct page));
+}
+
+static void vmemmap_use_new_sub_pmd(unsigned long start, unsigned long end)
+{
+	void *page = __va(ALIGN_DOWN(start, PMD_SIZE));
+
+	/* Could be our memmap page is filled with PAGE_UNUSED already ... */
+	vmemmap_use_sub_pmd(start, end);
+
+	/* Mark the unused parts of the new memmap page PAGE_UNUSED. */
+	if (!IS_ALIGNED(start, PMD_SIZE))
+		memset(page, PAGE_UNUSED, start - __pa(page));
+	if (!IS_ALIGNED(end, PMD_SIZE))
+		memset(__va(end), PAGE_UNUSED, __pa(page) + PMD_SIZE - end);
+}
+
+/* Returns true if the PMD is completely unused and can be freed. */
+static bool vmemmap_unuse_sub_pmd(unsigned long start, unsigned long end)
+{
+	void *page = __va(ALIGN_DOWN(start, PMD_SIZE));
+
+	memset(__va(start), PAGE_UNUSED, end - start);
+	return !memchr_inv(page, PAGE_UNUSED, PMD_SIZE);
+}
+
 /*
  * Add a physical memory range to the 1:1 mapping.
  */

@@ -213,6 +249,11 @@ static void remove_pmd_table(pud_t *pud, unsigned long addr,
 							get_order(PMD_SIZE));
 				pmd_clear(pmd);
 				pages++;
+			} else if (!direct &&
+				   vmemmap_unuse_sub_pmd(addr, next)) {
+				vmem_free_pages(pmd_deref(*pmd),
+						get_order(PMD_SIZE));
+				pmd_clear(pmd);
 			}
 			continue;
 		}

@@ -381,7 +422,7 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 		struct vmem_altmap *altmap)
 {
 	unsigned long pgt_prot, sgt_prot;
-	unsigned long address = start;
+	unsigned long next, address = start;
 	pgd_t *pg_dir;
 	p4d_t *p4_dir;
 	pud_t *pu_dir;

@@ -425,16 +466,27 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 		if (pmd_none(*pm_dir) && MACHINE_HAS_EDAT1) {
 			void *new_page;
 
-			/* Use 1MB frames for vmemmap if available. We always
+			/*
+			 * Use 1MB frames for vmemmap if available. We always
 			 * use large frames even if they are only partially
-			 * used.
+			 * used, and mark the unused parts using PAGE_UNUSED.
+			 *
+			 * This is only an issue in some setups. E.g.,
+			 * a full sections with 64 byte memmap per page need
+			 * 4 MB in total. However, with 56 byte, it's 3.5 MB.
+			 *
 			 * Otherwise we would have also page tables since
 			 * vmemmap_populate gets called for each section
-			 * separately. */
+			 * separately.
+			 */
 			new_page = vmemmap_alloc_block(PMD_SIZE, node);
 			if (new_page) {
 				pmd_val(*pm_dir) = __pa(new_page) | sgt_prot;
-				address = (address + PMD_SIZE) & PMD_MASK;
+				next = pmd_addr_end(address, end);
+				if (!IS_ALIGNED(next, PMD_SIZE) ||
+				    !IS_ALIGNED(address, PMD_SIZE))
+					vmemmap_use_new_sub_pmd(address, next);
+				address = next;
 				continue;
 			}
 		}

@@ -444,7 +496,9 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
 				goto out;
 			pmd_populate(&init_mm, pm_dir, pt_dir);
 		} else if (pmd_large(*pm_dir)) {
-			address = (address + PMD_SIZE) & PMD_MASK;
+			next = pmd_addr_end(address, end);
+			vmemmap_use_sub_pmd(address, next);
+			address = next;
 			continue;
 		}
Signature · 1 line
2.26.2
author
#12 lore raw

PATCH 9/9 s390/vmemmap: avoid memset(PAGE_UNUSED) when adding consecutive sections

In reply to David Hildenbrand

Let's avoid memset(PAGE_UNUSED) when adding consecutive sections, whereby the vmemmap of a single section does not span full PMDs.

Cc Heiko Carstens <[email protected]>
Cc Vasily Gorbik <[email protected]>
Cc Christian Borntraeger <[email protected]>
Cc Gerald Schaefer <[email protected]>
Signed-off-by David Hildenbrand <[email protected]>
---
 arch/s390/mm/vmem.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)
arch/s390/mm/vmem.c +42 -3
index a981ff5d47223..9db15fc864fc8 100644
@@ -74,7 +74,22 @@ static void vmem_pte_free(unsigned long *table)
 
 #define PAGE_UNUSED 0xFD
 
-static void vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
+/*
+ * The unused vmemmap range, which was not yet memset(PAGE_UNUSED) ranges
+ * from unused_pmd_start to next PMD_SIZE boundary.
+ */
+unsigned long unused_pmd_start;
+
+static void vmemmap_flush_unused_pmd(void)
+{
+	if (!unused_pmd_start)
+		return;
+	memset(__va(unused_pmd_start), PAGE_UNUSED,
+	       ALIGN(unused_pmd_start, PMD_SIZE) - unused_pmd_start);
+	unused_pmd_start = 0;
+}
+
+static void __vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
 {
 	/*
 	 * As we expect to add in the same granularity as we remove, it's

@@ -85,18 +100,41 @@ static void vmemmap_use_sub_pmd(unsigned long start, unsigned long end)
 	memset(__va(start), 0, sizeof(struct page));
 }
 
+static void 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) {
+		unused_pmd_start = end;
+		if (likely(IS_ALIGNED(unused_pmd_start, PMD_SIZE)))
+			unused_pmd_start = 0;
+		return;
+	}
+	vmemmap_flush_unused_pmd();
+	__vmemmap_use_sub_pmd(start, end);
+}
+
 static void vmemmap_use_new_sub_pmd(unsigned long start, unsigned long end)
 {
 	void *page = __va(ALIGN_DOWN(start, PMD_SIZE));
 
+	vmemmap_flush_unused_pmd();
+
 	/* Could be our memmap page is filled with PAGE_UNUSED already ... */
-	vmemmap_use_sub_pmd(start, end);
+	__vmemmap_use_sub_pmd(start, end);
 
 	/* Mark the unused parts of the new memmap page PAGE_UNUSED. */
 	if (!IS_ALIGNED(start, PMD_SIZE))
 		memset(page, PAGE_UNUSED, start - __pa(page));
+	/*
+	 * 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))
-		memset(__va(end), PAGE_UNUSED, __pa(page) + PMD_SIZE - end);
+		unused_pmd_start = end;
 }
 
 /* Returns true if the PMD is completely unused and can be freed. */

@@ -104,6 +142,7 @@ static bool vmemmap_unuse_sub_pmd(unsigned long start, unsigned long end)
 {
 	void *page = __va(ALIGN_DOWN(start, PMD_SIZE));
 
+	vmemmap_flush_unused_pmd();
 	memset(__va(start), PAGE_UNUSED, end - start);
 	return !memchr_inv(page, PAGE_UNUSED, PMD_SIZE);
 }
Signature · 1 line
2.26.2
#13 lore raw
In reply to David Hildenbrand
Quoted 5 lines · David Hildenbrand

This series is based on the latest s390/features branch [1]. It implements vmemmap_free(), consolidating it with vmem_add_range(), and optimizes it by
- Freeing empty page tables (now also done for idendity mapping).
- Handling cases where the vmemmap of a section does not fill huge pages completely.

Nice! You implemented some things I "always wanted to do". Maybe I should just do nothing and wait until patches appear ;)

Will take a look at the series next week. Thanks!

#14 lore raw
In reply to David Hildenbrand
Quoted 22 lines · David Hildenbrand

This series is based on the latest s390/features branch [1]. It implements vmemmap_free(), consolidating it with vmem_add_range(), and optimizes it by
- Freeing empty page tables (now also done for idendity mapping).
- Handling cases where the vmemmap of a section does not fill huge pages completely.

vmemmap_free() is currently never used, unless adiing standby memory fails (unlikely). This is relevant for virtio-mem, which adds/removes memory in memory block/section granularity (always removes memory in the same granularity it added it).

I gave this a proper test with my virtio-mem prototype (which I will share once the basic QEMU implementation is upstream), both with 56 byte memmap per page and 64 byte memmap per page, with and without huge page support. In both cases, removing memory (routed through arch_remove_memory()) will result in
- all populated vmemmap pages to get removed/freed
- all applicable page tables for the vmemmap getting removed/freed
- all applicable page tables for the idendity mapping getting removed/freed Unfortunately, I don't have access to bigger and z/VM (esp. dcss) environments.

Quoted 8 lines

This is the basis for real memory hotunplug support for s390x and should complete my journey to s390x vmem/vmemmap code for now :)

What needs double-checking is tlb flushing. AFAIKS, as there are no valid accesses, doing a single range flush at the end is sufficient, both when removing vmemmap pages and the idendity mapping.

Along, some minor cleanups.

Hmm.. I really would like to see if there would be only a single page table walker left in vmem.c, which handles both adding and removing things.
Now we end up with two different page table walk implementations within the same file. However not sure if it is worth the effort to unify them though.

author
#15 lore raw
Quoted 30 lines · Heiko Carstens
Quoted 29 lines · David Hildenbrand

This series is based on the latest s390/features branch [1]. It implements vmemmap_free(), consolidating it with vmem_add_range(), and optimizes it by
- Freeing empty page tables (now also done for idendity mapping).
- Handling cases where the vmemmap of a section does not fill huge pages completely.

vmemmap_free() is currently never used, unless adiing standby memory fails (unlikely). This is relevant for virtio-mem, which adds/removes memory in memory block/section granularity (always removes memory in the same granularity it added it).

I gave this a proper test with my virtio-mem prototype (which I will share once the basic QEMU implementation is upstream), both with 56 byte memmap per page and 64 byte memmap per page, with and without huge page support. In both cases, removing memory (routed through arch_remove_memory()) will result in
- all populated vmemmap pages to get removed/freed
- all applicable page tables for the vmemmap getting removed/freed
- all applicable page tables for the idendity mapping getting removed/freed Unfortunately, I don't have access to bigger and z/VM (esp. dcss) environments.

This is the basis for real memory hotunplug support for s390x and should complete my journey to s390x vmem/vmemmap code for now :)

What needs double-checking is tlb flushing. AFAIKS, as there are no valid accesses, doing a single range flush at the end is sufficient, both when removing vmemmap pages and the idendity mapping.

Quoted 8 lines
Quoted 1 line

Along, some minor cleanups.

Hmm.. I really would like to see if there would be only a single page table walker left in vmem.c, which handles both adding and removing things.
Now we end up with two different page table walk implementations within the same file. However not sure if it is worth the effort to unify them though.

I tried to unify vmemmap_populate() and vmem_add_range() already and didn't like the end result ... so, unifying these along with the removal part won't be any better - most probably. Open for suggestions :)

(at least arm64 and x86-64 handle it similarly)

Signature · 3 lines
Thanks,

David / dhildenb
author
#16 lore raw
Quoted 40 lines · David Hildenbrand
Quoted 30 lines · Heiko Carstens
Quoted 29 lines · David Hildenbrand

This series is based on the latest s390/features branch [1]. It implements vmemmap_free(), consolidating it with vmem_add_range(), and optimizes it by
- Freeing empty page tables (now also done for idendity mapping).
- Handling cases where the vmemmap of a section does not fill huge pages completely.

vmemmap_free() is currently never used, unless adiing standby memory fails (unlikely). This is relevant for virtio-mem, which adds/removes memory in memory block/section granularity (always removes memory in the same granularity it added it).

I gave this a proper test with my virtio-mem prototype (which I will share once the basic QEMU implementation is upstream), both with 56 byte memmap per page and 64 byte memmap per page, with and without huge page support. In both cases, removing memory (routed through arch_remove_memory()) will result in
- all populated vmemmap pages to get removed/freed
- all applicable page tables for the vmemmap getting removed/freed
- all applicable page tables for the idendity mapping getting removed/freed Unfortunately, I don't have access to bigger and z/VM (esp. dcss) environments.

This is the basis for real memory hotunplug support for s390x and should complete my journey to s390x vmem/vmemmap code for now :)

What needs double-checking is tlb flushing. AFAIKS, as there are no valid accesses, doing a single range flush at the end is sufficient, both when removing vmemmap pages and the idendity mapping.

Quoted 8 lines
Quoted 1 line

Along, some minor cleanups.

Hmm.. I really would like to see if there would be only a single page table walker left in vmem.c, which handles both adding and removing things.
Now we end up with two different page table walk implementations within the same file. However not sure if it is worth the effort to unify them though.

Quoted 6 lines

I tried to unify vmemmap_populate() and vmem_add_range() already and didn't like the end result ... so, unifying these along with the removal part won't be any better - most probably. Open for suggestions :)

(at least arm64 and x86-64 handle it similarly)

I'll play with something like

static void modify_pagetable(unsigned long start, unsigned long end,
bool direct, bool add)

and see how it turns out.

Signature · 3 lines
Thanks,

David / dhildenb
author
#17 lore raw
Quoted 48 lines · David Hildenbrand
Quoted 40 lines · David Hildenbrand
Quoted 30 lines · Heiko Carstens
Quoted 29 lines · David Hildenbrand

This series is based on the latest s390/features branch [1]. It implements vmemmap_free(), consolidating it with vmem_add_range(), and optimizes it by
- Freeing empty page tables (now also done for idendity mapping).
- Handling cases where the vmemmap of a section does not fill huge pages completely.

vmemmap_free() is currently never used, unless adiing standby memory fails (unlikely). This is relevant for virtio-mem, which adds/removes memory in memory block/section granularity (always removes memory in the same granularity it added it).

I gave this a proper test with my virtio-mem prototype (which I will share once the basic QEMU implementation is upstream), both with 56 byte memmap per page and 64 byte memmap per page, with and without huge page support. In both cases, removing memory (routed through arch_remove_memory()) will result in
- all populated vmemmap pages to get removed/freed
- all applicable page tables for the vmemmap getting removed/freed
- all applicable page tables for the idendity mapping getting removed/freed Unfortunately, I don't have access to bigger and z/VM (esp. dcss) environments.

This is the basis for real memory hotunplug support for s390x and should complete my journey to s390x vmem/vmemmap code for now :)

What needs double-checking is tlb flushing. AFAIKS, as there are no valid accesses, doing a single range flush at the end is sufficient, both when removing vmemmap pages and the idendity mapping.

Quoted 8 lines
Quoted 1 line

Along, some minor cleanups.

Hmm.. I really would like to see if there would be only a single page table walker left in vmem.c, which handles both adding and removing things.
Now we end up with two different page table walk implementations within the same file. However not sure if it is worth the effort to unify them though.

Quoted 6 lines

I tried to unify vmemmap_populate() and vmem_add_range() already and didn't like the end result ... so, unifying these along with the removal part won't be any better - most probably. Open for suggestions :)

(at least arm64 and x86-64 handle it similarly)

Quoted 7 lines

I'll play with something like

static void modify_pagetable(unsigned long start, unsigned long end,
bool direct, bool add)

and see how it turns out.

Did a quick hack. With a single walker (modify_pagetable) I get

 arch/s390/mm/vmem.c | 628 ++++++++++++++++++++++++++++++--------------
 1 file changed, 434 insertions(+), 194 deletions(-)

Overall looks cleaner, only modify_pte_table() and modify_pmd_table() are a little more involved ...

Signature · 3 lines
Thanks,

David / dhildenb
maintainer
#18 lore raw
Quoted 22 lines · David Hildenbrand
Quoted 14 lines
Quoted 7 lines
Quoted 6 lines

Hmm.. I really would like to see if there would be only a single page table walker left in vmem.c, which handles both adding and removing things.
Now we end up with two different page table walk implementations within the same file. However not sure if it is worth the effort to unify them though.

Quoted 6 lines

I tried to unify vmemmap_populate() and vmem_add_range() already and didn't like the end result ... so, unifying these along with the removal part won't be any better - most probably. Open for suggestions :)

(at least arm64 and x86-64 handle it similarly)

Quoted 7 lines

I'll play with something like

static void modify_pagetable(unsigned long start, unsigned long end,
bool direct, bool add)

and see how it turns out.

Quoted 7 lines

Did a quick hack. With a single walker (modify_pagetable) I get

 arch/s390/mm/vmem.c | 628 ++++++++++++++++++++++++++++++--------------
 1 file changed, 434 insertions(+), 194 deletions(-)

Overall looks cleaner, only modify_pte_table() and modify_pmd_table() are a little more involved ...

Would you mind to resend the series with this integrated?

author
#19 lore raw
Quoted 25 lines · Heiko Carstens
Quoted 24 lines · David Hildenbrand
Quoted 14 lines
Quoted 7 lines
Quoted 6 lines

Hmm.. I really would like to see if there would be only a single page table walker left in vmem.c, which handles both adding and removing things.
Now we end up with two different page table walk implementations within the same file. However not sure if it is worth the effort to unify them though.

Quoted 6 lines

I tried to unify vmemmap_populate() and vmem_add_range() already and didn't like the end result ... so, unifying these along with the removal part won't be any better - most probably. Open for suggestions :)

(at least arm64 and x86-64 handle it similarly)

Quoted 7 lines

I'll play with something like

static void modify_pagetable(unsigned long start, unsigned long end,
bool direct, bool add)

and see how it turns out.

Did a quick hack. With a single walker (modify_pagetable) I get

Quoted 8 lines
Quoted 5 lines
 arch/s390/mm/vmem.c | 628 ++++++++++++++++++++++++++++++--------------
 1 file changed, 434 insertions(+), 194 deletions(-)

Overall looks cleaner, only modify_pte_table() and modify_pmd_table() are a little more involved ...

Would you mind to resend the series with this integrated?

Yes, did some testing yesterday. Want to reshuffle/cleanup some things before posting.

Will be out of office next week, so expect a v2 some-when in 1-2 weeks.

Cheers!

Signature · 3 lines
Thanks,

David / dhildenb