Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
On Fri, Jul 24, 2020 at 09:52:14PM +1000, Michael Ellerman wrote:
> Bharata B Rao writes:
> > On Tue, Jul 21, 2020 at 10:25:58PM +1000, Michael Ellerman wrote:
> >> Bharata B Rao writes:
> >> > On Tue, Jul 21, 2020 at 11:45:20AM +1000, Michael Ellerman wrote:
> >> >> Nathan Lynch writes:
> >> >> > "Aneesh Kumar K.V" 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.
Sounds about right; I don't remember for certain.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
On Thu, 9 Jul 2020 18:49:21 +0530, Aneesh Kumar K.V wrote: > 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. > > [...] Applied to powerpc/next. [1/4] powerpc/mm/radix: Fix PTE/PMD fragment count for early page table mappings https://git.kernel.org/powerpc/c/645d5ce2f7d6cb4dcf6a4e087fb550e238d24283 [2/4] powerpc/mm/radix: Free PUD table when freeing pagetable https://git.kernel.org/powerpc/c/9ce8853b4a735c8115f55ac0e9c2b27a4c8f80b5 [3/4] powerpc/mm/radix: Remove split_kernel_mapping() https://git.kernel.org/powerpc/c/d6d6ebfc5dbb4008be21baa4ec2ad45606578966 [4/4] powerpc/mm/radix: Create separate mappings for hot-plugged memory https://git.kernel.org/powerpc/c/af9d00e93a4f062c5f160325d7b8f6f6744e cheers
Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
On Fri, Jul 24, 2020 at 09:52:14PM +1000, Michael Ellerman wrote:
> Bharata B Rao writes:
> > On Tue, Jul 21, 2020 at 10:25:58PM +1000, Michael Ellerman wrote:
> >> Bharata B Rao writes:
> >> > On Tue, Jul 21, 2020 at 11:45:20AM +1000, Michael Ellerman wrote:
> >> >> Nathan Lynch writes:
> >> >> > "Aneesh Kumar K.V" 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.
Sure I will send it as a proper patch. I did test minimal hotplug/unplug
for hash guest with that patch, will do more extensive test and resend.
>
> > 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.
Right.
Regards,
Bharata.
Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
Bharata B Rao writes:
> On Tue, Jul 21, 2020 at 10:25:58PM +1000, Michael Ellerman wrote:
>> Bharata B Rao writes:
>> > On Tue, Jul 21, 2020 at 11:45:20AM +1000, Michael Ellerman wrote:
>> >> Nathan Lynch writes:
>> >> > "Aneesh Kumar K.V" 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
Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
On Wed, Jul 22, 2020 at 11:35:06AM +0530, Bharata B Rao wrote:
> On Tue, Jul 21, 2020 at 10:25:58PM +1000, Michael Ellerman wrote:
> > Bharata B Rao writes:
> > > On Tue, Jul 21, 2020 at 11:45:20AM +1000, Michael Ellerman wrote:
> > >> Nathan Lynch writes:
> > >> > "Aneesh Kumar K.V" 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?
I don't remember, sorry.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
On Tue, Jul 21, 2020 at 10:25:58PM +1000, Michael Ellerman wrote:
> Bharata B Rao writes:
> > On Tue, Jul 21, 2020 at 11:45:20AM +1000, Michael Ellerman wrote:
> >> Nathan Lynch writes:
> >> > "Aneesh Kumar K.V" 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
Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
Bharata B Rao writes:
> On Tue, Jul 21, 2020 at 11:45:20AM +1000, Michael Ellerman wrote:
>> Nathan Lynch writes:
>> > "Aneesh Kumar K.V" 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.
cheers
Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
On 7/21/20 7:15 AM, Michael Ellerman wrote:
Nathan Lynch writes:
"Aneesh Kumar K.V" 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.
We are good there because hpte_init_pseries is only called for hash
translation.
early_init_mmu()
-> hash__early_init_mmu
-> hpte_init_pseries
-> mmu_hash_ops.resize_hpt = pseries_lpar_resize_hpt;
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.
All the changes are tested with kvm.
-aneesh
Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
On Tue, Jul 21, 2020 at 11:45:20AM +1000, Michael Ellerman wrote:
> Nathan Lynch writes:
> > "Aneesh Kumar K.V" 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.
>
> 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.
However a separate patch to fix this will be good.
Regards,
Bharata.
Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
Nathan Lynch writes:
> "Aneesh Kumar K.V" 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.
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.
cheers
Re: [PATCH v3 0/4] powerpc/mm/radix: Memory unplug fixes
"Aneesh Kumar K.V" 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);
...
[PATCH v3 0/4] 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. 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. Changes from v0: - Rebased to latest kernel. - Took care of p4d changes. - Addressed Aneesh's review feedback: - Added comments. - Indentation fixed. - Dropped the 1st patch (setting DRCONF_MEM_HOTREMOVABLE lmb flags) as it is debatable if this flag should be set in the device tree by OS and not by platform in case of hotplug. This can be looked at separately. (The fixes in this patchset remain valid without the dropped patch) - Dropped the last patch that removed split_kernel_mapping() to ensure that spilitting code is available for any radix guest running on platforms that don't set DRCONF_MEM_HOTREMOVABLE. Aneesh Kumar K.V (2): powerpc/mm/radix: Fix PTE/PMD fragment count for early page table mappings powerpc/mm/radix: Create separate mappings for hot-plugged memory Bharata B Rao (2): powerpc/mm/radix: Free PUD table when freeing pagetable powerpc/mm/radix: Remove split_kernel_mapping() arch/powerpc/include/asm/book3s/64/mmu.h | 5 + arch/powerpc/include/asm/book3s/64/pgalloc.h | 16 +- arch/powerpc/mm/book3s64/pgtable.c | 5 +- arch/powerpc/mm/book3s64/radix_pgtable.c | 197 +++ arch/powerpc/mm/pgtable-frag.c | 3 + arch/powerpc/platforms/powernv/setup.c | 10 +- 6 files changed, 147 insertions(+), 89 deletions(-) -- 2.26.2
