powerpc/mm/radix: Memory unplug fixes

This is the next version of the fixes for memory unplug on radix. The issues and the fix are described in the actual patches.

Quoted 2 lines · Aneesh Kumar K.V

This is the next version of the fixes for memory unplug on radix. The issues and the fix are described in the actual patches.

I guess this isn't actually causing problems at runtime right now, but I notice calls to resize_hpt_for_hotplug() from arch_add_memory() and arch_remove_memory(), which ought to be mmu-agnostic:

int __ref arch_add_memory(int nid, u64 start, u64 size,
struct mhp_params *params)

{
	unsigned long start_pfn = start >> PAGE_SHIFT;
	unsigned long nr_pages = size >> PAGE_SHIFT;
	int rc;

	resize_hpt_for_hotplug(memblock_phys_mem_size());

	start = (unsigned long)__va(start);
	rc = create_section_mapping(start, start + size, nid,
				    params->pgprot);

...

Quoted 20 lines · Nathan Lynch

Quoted 2 lines · Aneesh Kumar K.V

This is the next version of the fixes for memory unplug on radix. The issues and the fix are described in the actual patches.

I guess this isn't actually causing problems at runtime right now, but I notice calls to resize_hpt_for_hotplug() from arch_add_memory() and arch_remove_memory(), which ought to be mmu-agnostic:

int __ref arch_add_memory(int nid, u64 start, u64 size,
struct mhp_params *params)

{
	unsigned long start_pfn = start >> PAGE_SHIFT;
	unsigned long nr_pages = size >> PAGE_SHIFT;
	int rc;

	resize_hpt_for_hotplug(memblock_phys_mem_size());

	start = (unsigned long)__va(start);
	rc = create_section_mapping(start, start + size, nid,
				    params->pgprot);

Quoted 1 line

...

Hmm well spotted.

That does return early if the ops are not setup:

int resize_hpt_for_hotplug(unsigned long new_mem_size)

{
	unsigned target_hpt_shift;

	if (!mmu_hash_ops.resize_hpt)
		return 0;

And:

void __init hpte_init_pseries(void)

{
	...
	if (firmware_has_feature(FW_FEATURE_HPT_RESIZE))
		mmu_hash_ops.resize_hpt = pseries_lpar_resize_hpt;

And that comes in via ibm,hypertas-functions:

	{FW_FEATURE_HPT_RESIZE,		"hcall-hpt-resize"},

But firmware is not necessarily going to add/remove that call based on whether we're using hash/radix.

So I think a follow-up patch is needed to make this more robust.

Aneesh/Bharata what platform did you test this series on? I'm curious how this didn't break.

Quoted 45 lines · Michael Ellerman

Quoted 20 lines · Nathan Lynch

Quoted 2 lines · Aneesh Kumar K.V

This is the next version of the fixes for memory unplug on radix. The issues and the fix are described in the actual patches.

I guess this isn't actually causing problems at runtime right now, but I notice calls to resize_hpt_for_hotplug() from arch_add_memory() and arch_remove_memory(), which ought to be mmu-agnostic:

int __ref arch_add_memory(int nid, u64 start, u64 size,
struct mhp_params *params)

{
	unsigned long start_pfn = start >> PAGE_SHIFT;
	unsigned long nr_pages = size >> PAGE_SHIFT;
	int rc;

	resize_hpt_for_hotplug(memblock_phys_mem_size());

	start = (unsigned long)__va(start);
	rc = create_section_mapping(start, start + size, nid,
				    params->pgprot);

Quoted 1 line

...

Hmm well spotted.

That does return early if the ops are not setup:

int resize_hpt_for_hotplug(unsigned long new_mem_size)

{
	unsigned target_hpt_shift;

	if (!mmu_hash_ops.resize_hpt)
		return 0;

And:

void __init hpte_init_pseries(void)

{
	...
	if (firmware_has_feature(FW_FEATURE_HPT_RESIZE))
		mmu_hash_ops.resize_hpt = pseries_lpar_resize_hpt;

And that comes in via ibm,hypertas-functions:

Quoted 5 lines

	{FW_FEATURE_HPT_RESIZE,		"hcall-hpt-resize"},

But firmware is not necessarily going to add/remove that call based on whether we're using hash/radix.

Correct but hpte_init_pseries() will not be called for radix guests.

Quoted 4 lines

So I think a follow-up patch is needed to make this more robust.

Aneesh/Bharata what platform did you test this series on? I'm curious how this didn't break.

I have tested memory hotplug/unplug for radix guest on zz platform and sanity-tested this for hash guest on P8.

As noted above, mmu_hash_ops.resize_hpt will not be set for radix guest and hence we won't see any breakage.

Quoted 46 lines · Bharata B Rao

Quoted 45 lines · Michael Ellerman

Quoted 20 lines · Nathan Lynch

Quoted 2 lines · Aneesh Kumar K.V

This is the next version of the fixes for memory unplug on radix. The issues and the fix are described in the actual patches.

I guess this isn't actually causing problems at runtime right now, but I notice calls to resize_hpt_for_hotplug() from arch_add_memory() and arch_remove_memory(), which ought to be mmu-agnostic:

int __ref arch_add_memory(int nid, u64 start, u64 size,
struct mhp_params *params)

{
	unsigned long start_pfn = start >> PAGE_SHIFT;
	unsigned long nr_pages = size >> PAGE_SHIFT;
	int rc;

	resize_hpt_for_hotplug(memblock_phys_mem_size());

	start = (unsigned long)__va(start);
	rc = create_section_mapping(start, start + size, nid,
				    params->pgprot);

Quoted 1 line

...

Hmm well spotted.

That does return early if the ops are not setup:

int resize_hpt_for_hotplug(unsigned long new_mem_size)

{
	unsigned target_hpt_shift;

	if (!mmu_hash_ops.resize_hpt)
		return 0;

And:

void __init hpte_init_pseries(void)

{
	...
	if (firmware_has_feature(FW_FEATURE_HPT_RESIZE))
		mmu_hash_ops.resize_hpt = pseries_lpar_resize_hpt;

And that comes in via ibm,hypertas-functions:

Quoted 7 lines

Quoted 5 lines

	{FW_FEATURE_HPT_RESIZE,		"hcall-hpt-resize"},

But firmware is not necessarily going to add/remove that call based on whether we're using hash/radix.

Correct but hpte_init_pseries() will not be called for radix guests.

Yeah, duh. You'd think the function name would have been a sufficient clue for me :)

Quoted 8 lines

Quoted 4 lines

So I think a follow-up patch is needed to make this more robust.

Aneesh/Bharata what platform did you test this series on? I'm curious how this didn't break.

I have tested memory hotplug/unplug for radix guest on zz platform and sanity-tested this for hash guest on P8.

Quoted 2 lines

As noted above, mmu_hash_ops.resize_hpt will not be set for radix guest and hence we won't see any breakage.

OK.

That's probably fine as it is then. Or maybe just a comment in resize_hpt_for_hotplug() pointing out that resize_hpt will be NULL if we're using radix.

Quoted 71 lines · Michael Ellerman

Quoted 46 lines · Bharata B Rao

Quoted 45 lines · Michael Ellerman

Quoted 20 lines · Nathan Lynch

Quoted 2 lines · Aneesh Kumar K.V

This is the next version of the fixes for memory unplug on radix. The issues and the fix are described in the actual patches.

I guess this isn't actually causing problems at runtime right now, but I notice calls to resize_hpt_for_hotplug() from arch_add_memory() and arch_remove_memory(), which ought to be mmu-agnostic:

int __ref arch_add_memory(int nid, u64 start, u64 size,
struct mhp_params *params)

{
	unsigned long start_pfn = start >> PAGE_SHIFT;
	unsigned long nr_pages = size >> PAGE_SHIFT;
	int rc;

	resize_hpt_for_hotplug(memblock_phys_mem_size());

	start = (unsigned long)__va(start);
	rc = create_section_mapping(start, start + size, nid,
				    params->pgprot);

Quoted 1 line

...

Hmm well spotted.

That does return early if the ops are not setup:

int resize_hpt_for_hotplug(unsigned long new_mem_size)

{
	unsigned target_hpt_shift;

	if (!mmu_hash_ops.resize_hpt)
		return 0;

And:

void __init hpte_init_pseries(void)

{
	...
	if (firmware_has_feature(FW_FEATURE_HPT_RESIZE))
		mmu_hash_ops.resize_hpt = pseries_lpar_resize_hpt;

And that comes in via ibm,hypertas-functions:

Quoted 7 lines

Quoted 5 lines

	{FW_FEATURE_HPT_RESIZE,		"hcall-hpt-resize"},

But firmware is not necessarily going to add/remove that call based on whether we're using hash/radix.

Correct but hpte_init_pseries() will not be called for radix guests.

Yeah, duh. You'd think the function name would have been a sufficient clue for me :)

Quoted 10 lines

Quoted 4 lines

So I think a follow-up patch is needed to make this more robust.

Aneesh/Bharata what platform did you test this series on? I'm curious how this didn't break.

I have tested memory hotplug/unplug for radix guest on zz platform and sanity-tested this for hash guest on P8.

As noted above, mmu_hash_ops.resize_hpt will not be set for radix guest and hence we won't see any breakage.

OK.

Quoted 3 lines

That's probably fine as it is then. Or maybe just a comment in resize_hpt_for_hotplug() pointing out that resize_hpt will be NULL if we're using radix.

Or we could move these calls to hpt-only routines like below?

David - Do you remember if there was any particular reason to have these two hpt-resize calls within powerpc-generic memory hotplug code?

Quoted 69 lines · Michael Ellerman

Quoted 46 lines · Bharata B Rao

Quoted 45 lines · Michael Ellerman

Quoted 20 lines · Nathan Lynch

Quoted 2 lines · Aneesh Kumar K.V

This is the next version of the fixes for memory unplug on radix. The issues and the fix are described in the actual patches.

I guess this isn't actually causing problems at runtime right now, but I notice calls to resize_hpt_for_hotplug() from arch_add_memory() and arch_remove_memory(), which ought to be mmu-agnostic:

int __ref arch_add_memory(int nid, u64 start, u64 size,
struct mhp_params *params)

{
	unsigned long start_pfn = start >> PAGE_SHIFT;
	unsigned long nr_pages = size >> PAGE_SHIFT;
	int rc;

	resize_hpt_for_hotplug(memblock_phys_mem_size());

	start = (unsigned long)__va(start);
	rc = create_section_mapping(start, start + size, nid,
				    params->pgprot);

Quoted 1 line

...

Hmm well spotted.

That does return early if the ops are not setup:

int resize_hpt_for_hotplug(unsigned long new_mem_size)

{
	unsigned target_hpt_shift;

	if (!mmu_hash_ops.resize_hpt)
		return 0;

And:

void __init hpte_init_pseries(void)

{
	...
	if (firmware_has_feature(FW_FEATURE_HPT_RESIZE))
		mmu_hash_ops.resize_hpt = pseries_lpar_resize_hpt;

And that comes in via ibm,hypertas-functions:

Quoted 7 lines

Quoted 5 lines

	{FW_FEATURE_HPT_RESIZE,		"hcall-hpt-resize"},

But firmware is not necessarily going to add/remove that call based on whether we're using hash/radix.

Correct but hpte_init_pseries() will not be called for radix guests.

Yeah, duh. You'd think the function name would have been a sufficient clue for me :)

Quoted 10 lines

Quoted 4 lines

So I think a follow-up patch is needed to make this more robust.

Aneesh/Bharata what platform did you test this series on? I'm curious how this didn't break.

I have tested memory hotplug/unplug for radix guest on zz platform and sanity-tested this for hash guest on P8.

As noted above, mmu_hash_ops.resize_hpt will not be set for radix guest and hence we won't see any breakage.

Quoted 5 lines

OK.

That's probably fine as it is then. Or maybe just a comment in resize_hpt_for_hotplug() pointing out that resize_hpt will be NULL if we're using radix.

Or we could move these calls to hpt-only routines like below?

David - Do you remember if there was any particular reason to have these two hpt-resize calls within powerpc-generic memory hotplug code?

arch/powerpc/include/asm/sparsemem.h +0 -6

index c89b32443cff..1e6fa371cc38 100644
@@ -17,12 +17,6 @@ extern int create_section_mapping(unsigned long start, unsigned long end,
 				  int nid, pgprot_t prot);
 extern int remove_section_mapping(unsigned long start, unsigned long end);
 
-#ifdef CONFIG_PPC_BOOK3S_64
-extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
-#else
-static inline int resize_hpt_for_hotplug(unsigned long new_mem_size) { return 0; }
-#endif
-
 #ifdef CONFIG_NUMA
 extern int hot_add_scn_to_nid(unsigned long scn_addr);
 #else

arch/powerpc/mm/book3s64/hash_utils.c +7 -1

index eec6f4e5e481..5daf53ec7600 100644
@@ -787,7 +787,7 @@ static unsigned long __init htab_get_table_size(void)
 }
 
 #ifdef CONFIG_MEMORY_HOTPLUG
-int resize_hpt_for_hotplug(unsigned long new_mem_size)
+static int resize_hpt_for_hotplug(unsigned long new_mem_size)
 {
 	unsigned target_hpt_shift;
 

@@ -821,6 +821,8 @@ int hash__create_section_mapping(unsigned long start, unsigned long end,
 		return -1;
 	}
 
+	resize_hpt_for_hotplug(memblock_phys_mem_size());
+
 	rc = htab_bolt_mapping(start, end, __pa(start),
 			       pgprot_val(prot), mmu_linear_psize,
 			       mmu_kernel_ssize);

@@ -838,6 +840,10 @@ int hash__remove_section_mapping(unsigned long start, unsigned long end)
 	int rc = htab_remove_mapping(start, end, mmu_linear_psize,
 				     mmu_kernel_ssize);
 	WARN_ON(rc < 0);
+
+	if (resize_hpt_for_hotplug(memblock_phys_mem_size()) == -ENOSPC)
+		pr_warn("Hash collision while resizing HPT\n");
+
 	return rc;
 }
 #endif /* CONFIG_MEMORY_HOTPLUG */

arch/powerpc/mm/mem.c +0 -5

index c2c11eb8dcfc..9dafc636588f 100644
@@ -127,8 +127,6 @@ int __ref arch_add_memory(int nid, u64 start, u64 size,
 	unsigned long nr_pages = size >> PAGE_SHIFT;
 	int rc;
 
-	resize_hpt_for_hotplug(memblock_phys_mem_size());
-
 	start = (unsigned long)__va(start);
 	rc = create_section_mapping(start, start + size, nid,
 				    params->pgprot);

@@ -161,9 +159,6 @@ void __ref arch_remove_memory(int nid, u64 start, u64 size,
 	 * hit that section of memory
 	 */
 	vm_unmap_aliases();
-
-	if (resize_hpt_for_hotplug(memblock_phys_mem_size()) == -ENOSPC)
-		pr_warn("Hash collision while resizing HPT\n");
 }
 #endif
 

Signature · 1 line

2.26.2

Quoted 76 lines · Bharata B Rao

Quoted 74 lines · Michael Ellerman

Quoted 46 lines · Bharata B Rao

Quoted 45 lines · Michael Ellerman

Quoted 20 lines · Nathan Lynch

Quoted 2 lines · Aneesh Kumar K.V

This is the next version of the fixes for memory unplug on radix. The issues and the fix are described in the actual patches.

I guess this isn't actually causing problems at runtime right now, but I notice calls to resize_hpt_for_hotplug() from arch_add_memory() and arch_remove_memory(), which ought to be mmu-agnostic:

int __ref arch_add_memory(int nid, u64 start, u64 size,
struct mhp_params *params)

{
	unsigned long start_pfn = start >> PAGE_SHIFT;
	unsigned long nr_pages = size >> PAGE_SHIFT;
	int rc;

	resize_hpt_for_hotplug(memblock_phys_mem_size());

	start = (unsigned long)__va(start);
	rc = create_section_mapping(start, start + size, nid,
				    params->pgprot);

Quoted 1 line

...

Hmm well spotted.

That does return early if the ops are not setup:

int resize_hpt_for_hotplug(unsigned long new_mem_size)

{
	unsigned target_hpt_shift;

	if (!mmu_hash_ops.resize_hpt)
		return 0;

And:

void __init hpte_init_pseries(void)

{
	...
	if (firmware_has_feature(FW_FEATURE_HPT_RESIZE))
		mmu_hash_ops.resize_hpt = pseries_lpar_resize_hpt;

And that comes in via ibm,hypertas-functions:

Quoted 7 lines

Quoted 5 lines

	{FW_FEATURE_HPT_RESIZE,		"hcall-hpt-resize"},

But firmware is not necessarily going to add/remove that call based on whether we're using hash/radix.

Correct but hpte_init_pseries() will not be called for radix guests.

Yeah, duh. You'd think the function name would have been a sufficient clue for me :)

Quoted 10 lines

Quoted 4 lines

So I think a follow-up patch is needed to make this more robust.

Aneesh/Bharata what platform did you test this series on? I'm curious how this didn't break.

I have tested memory hotplug/unplug for radix guest on zz platform and sanity-tested this for hash guest on P8.

As noted above, mmu_hash_ops.resize_hpt will not be set for radix guest and hence we won't see any breakage.

OK.

That's probably fine as it is then. Or maybe just a comment in resize_hpt_for_hotplug() pointing out that resize_hpt will be NULL if we're using radix.

Quoted 1 line

Or we could move these calls to hpt-only routines like below?

That looks like it would be equivalent, and would nicely isolate those calls in hash specific code. So yeah I think that's worth sending as a proper patch, even better if you can test it.

Quoted 3 lines

David - Do you remember if there was any particular reason to have these two hpt-resize calls within powerpc-generic memory hotplug code?

I think the HPT resizing was developed before or concurrently with the radix support, so I would guess it was just not something we thought about at the time.

Quoted 70 lines · Bharata B Rao

Quoted 69 lines · Michael Ellerman

Quoted 46 lines · Bharata B Rao

Quoted 45 lines · Michael Ellerman

Quoted 20 lines · Nathan Lynch

Quoted 2 lines · Aneesh Kumar K.V

This is the next version of the fixes for memory unplug on radix. The issues and the fix are described in the actual patches.

I guess this isn't actually causing problems at runtime right now, but I notice calls to resize_hpt_for_hotplug() from arch_add_memory() and arch_remove_memory(), which ought to be mmu-agnostic:

int __ref arch_add_memory(int nid, u64 start, u64 size,
struct mhp_params *params)

{
	unsigned long start_pfn = start >> PAGE_SHIFT;
	unsigned long nr_pages = size >> PAGE_SHIFT;
	int rc;

	resize_hpt_for_hotplug(memblock_phys_mem_size());

	start = (unsigned long)__va(start);
	rc = create_section_mapping(start, start + size, nid,
				    params->pgprot);

Quoted 1 line

...

Hmm well spotted.

That does return early if the ops are not setup:

int resize_hpt_for_hotplug(unsigned long new_mem_size)

{
	unsigned target_hpt_shift;

	if (!mmu_hash_ops.resize_hpt)
		return 0;

And:

void __init hpte_init_pseries(void)

{
	...
	if (firmware_has_feature(FW_FEATURE_HPT_RESIZE))
		mmu_hash_ops.resize_hpt = pseries_lpar_resize_hpt;

And that comes in via ibm,hypertas-functions:

Quoted 7 lines

Quoted 5 lines

	{FW_FEATURE_HPT_RESIZE,		"hcall-hpt-resize"},

But firmware is not necessarily going to add/remove that call based on whether we're using hash/radix.

Correct but hpte_init_pseries() will not be called for radix guests.

Yeah, duh. You'd think the function name would have been a sufficient clue for me :)

Quoted 10 lines

Quoted 4 lines

So I think a follow-up patch is needed to make this more robust.

Aneesh/Bharata what platform did you test this series on? I'm curious how this didn't break.

I have tested memory hotplug/unplug for radix guest on zz platform and sanity-tested this for hash guest on P8.

As noted above, mmu_hash_ops.resize_hpt will not be set for radix guest and hence we won't see any breakage.

Quoted 7 lines

Quoted 5 lines

OK.

That's probably fine as it is then. Or maybe just a comment in resize_hpt_for_hotplug() pointing out that resize_hpt will be NULL if we're using radix.

Or we could move these calls to hpt-only routines like below?

That looks like it would be equivalent, and would nicely isolate those calls in hash specific code. So yeah I think that's worth sending as a proper patch, even better if you can test it.

Quoted 2 lines

David - Do you remember if there was any particular reason to have these two hpt-resize calls within powerpc-generic memory hotplug code?

I think the HPT resizing was developed before or concurrently with the radix support, so I would guess it was just not something we thought about at the time.

Quoted 20 lines · Nathan Lynch

Quoted 2 lines · Aneesh Kumar K.V

This is the next version of the fixes for memory unplug on radix. The issues and the fix are described in the actual patches.

I guess this isn't actually causing problems at runtime right now, but I notice calls to resize_hpt_for_hotplug() from arch_add_memory() and arch_remove_memory(), which ought to be mmu-agnostic:

int __ref arch_add_memory(int nid, u64 start, u64 size,
struct mhp_params *params)

{
	unsigned long start_pfn = start >> PAGE_SHIFT;
	unsigned long nr_pages = size >> PAGE_SHIFT;
	int rc;

	resize_hpt_for_hotplug(memblock_phys_mem_size());

	start = (unsigned long)__va(start);
	rc = create_section_mapping(start, start + size, nid,
				    params->pgprot);

Quoted 1 line

...

Hmm well spotted.

That does return early if the ops are not setup:

int resize_hpt_for_hotplug(unsigned long new_mem_size)

{
	unsigned target_hpt_shift;

	if (!mmu_hash_ops.resize_hpt)
		return 0;

And:

void __init hpte_init_pseries(void)

{
	...
	if (firmware_has_feature(FW_FEATURE_HPT_RESIZE))
		mmu_hash_ops.resize_hpt = pseries_lpar_resize_hpt;

And that comes in via ibm,hypertas-functions:

	{FW_FEATURE_HPT_RESIZE,		"hcall-hpt-resize"},

But firmware is not necessarily going to add/remove that call based on whether we're using hash/radix.

So I think a follow-up patch is needed to make this more robust.

Aneesh/Bharata what platform did you test this series on? I'm curious how this didn't break.

This is the next version of the fixes for memory unplug on radix. The issues and the fix are described in the actual patches.

Changes from v2:
- Address review feedback

Changes from v1:
- Added back patch to drop split_kernel_mapping
- Most of the split_kernel_mapping related issues are now described in the removal patch
- drop pte fragment change
- use lmb size as the max mapping size.
- Radix baremetal now use memory block size of 1G.

[...]