On Tue, Jul 21, 2020 at 10:25:58PM +1000, Michael Ellerman wrote:
> Bharata B Rao <bhar...@linux.ibm.com> writes:
> > On Tue, Jul 21, 2020 at 11:45:20AM +1000, Michael Ellerman wrote:
> >> Nathan Lynch <nath...@linux.ibm.com> writes:
> >> > "Aneesh Kumar K.V" <aneesh.ku...@linux.ibm.com> writes:
> >> >> 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);
> >> > ...
> >>
> >> 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.
> >
> > 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 :)
>
> >> 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.
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?
diff --git a/arch/powerpc/include/asm/sparsemem.h
b/arch/powerpc/include/asm/sparsemem.h
index c89b32443cff..1e6fa371cc38 100644
--- a/arch/powerpc/include/asm/sparsemem.h
+++ b/arch/powerpc/include/asm/sparsemem.h
@@ -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
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c
b/arch/powerpc/mm/book3s64/hash_utils.c
index eec6f4e5e481..5daf53ec7600 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -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 */
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index c2c11eb8dcfc..9dafc636588f 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -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
--
2.26.2
