Bharata B Rao <bhar...@linux.ibm.com> writes:
> 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?
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.
> 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.
cheers
> 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
