[PATCH] Revert "powerpc/powernv: Increase memory block size to 1GB on radix"

[PATCH] Revert "powerpc/powernv: Increase memory block size to 1GB on radix"

[Balbir Singh]

This reverts commit 53ecde0b9126ff140abe3aefd7f0ec64d6fa36b0.

The commit above changed the memblock size to 1GiB, which did some
nice things like create fewer TLB entries for mapping memory at
the time of hotplug. The downside is that it changes the granularity
at which memory can be hot-plugged and hot-unplugged. The implication
is that if we had less than a 1GiB to hot-plug/hot-unplug that
would not be possible.

The reason we had this fix was to resolve an issue where we did not
split mappings on hot-unplug, leaving a TLB entry that spanned the
region that was unplugged. This is now fixed by 4dd5f8a99e79 which
splits the page table, removing the MMU mappings for the hot-unplugged
region correctly.

This trade-offs performance as we increase the number of TLB's to
map 1GiB of memory, mapped in via hot-plug, but gives us the ability
to handle smaller memory sizes.

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 arch/powerpc/platforms/powernv/setup.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
index ef8c9ce53a61..fa63d3fff14c 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -356,15 +356,7 @@ static void pnv_kexec_cpu_down(int crash_shutdown, int secondary)
 #ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
 static unsigned long pnv_memory_block_size(void)
 {
-	/*
-	 * We map the kernel linear region with 1GB large pages on radix. For
-	 * memory hot unplug to work our memory block size must be at least
-	 * this size.
-	 */
-	if (radix_enabled())
-		return 1UL * 1024 * 1024 * 1024;
-	else
-		return 256UL * 1024 * 1024;
+	return 256UL * 1024 * 1024;
 }
 #endif
 
-- 
2.13.6

Re: [PATCH] Revert "powerpc/powernv: Increase memory block size to 1GB on radix"

[Nicholas Piggin]

On Mon, 30 Apr 2018 18:34:32 +1000
Balbir Singh <bsingharora@gmail.com> wrote:

> This reverts commit 53ecde0b9126ff140abe3aefd7f0ec64d6fa36b0.
> 
> The commit above changed the memblock size to 1GiB, which did some
> nice things like create fewer TLB entries for mapping memory at
> the time of hotplug. The downside is that it changes the granularity
> at which memory can be hot-plugged and hot-unplugged. The implication
> is that if we had less than a 1GiB to hot-plug/hot-unplug that
> would not be possible.
> 
> The reason we had this fix was to resolve an issue where we did not
> split mappings on hot-unplug, leaving a TLB entry that spanned the
> region that was unplugged. This is now fixed by 4dd5f8a99e79 which
> splits the page table, removing the MMU mappings for the hot-unplugged
> region correctly.
> 
> This trade-offs performance as we increase the number of TLB's to
> map 1GiB of memory, mapped in via hot-plug, but gives us the ability
> to handle smaller memory sizes.

Why does powernv have any memory block size override at all now?

> 
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> ---
>  arch/powerpc/platforms/powernv/setup.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
> index ef8c9ce53a61..fa63d3fff14c 100644
> --- a/arch/powerpc/platforms/powernv/setup.c
> +++ b/arch/powerpc/platforms/powernv/setup.c
> @@ -356,15 +356,7 @@ static void pnv_kexec_cpu_down(int crash_shutdown, int secondary)
>  #ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
>  static unsigned long pnv_memory_block_size(void)
>  {
> -	/*
> -	 * We map the kernel linear region with 1GB large pages on radix. For
> -	 * memory hot unplug to work our memory block size must be at least
> -	 * this size.
> -	 */
> -	if (radix_enabled())
> -		return 1UL * 1024 * 1024 * 1024;
> -	else
> -		return 256UL * 1024 * 1024;
> +	return 256UL * 1024 * 1024;
>  }
>  #endif
>  

Re: [PATCH] Revert "powerpc/powernv: Increase memory block size to 1GB on radix"

[Michael Ellerman]

Balbir Singh <bsingharora@gmail.com> writes:
> This reverts commit 53ecde0b9126ff140abe3aefd7f0ec64d6fa36b0.

Firstly everything here only applies to Radix, so we need to say that.

> The commit above changed the memblock size to 1GiB, which did some
> nice things like create fewer TLB entries for mapping memory at
> the time of hotplug.

You say TLB entry here and below, but I think that's misleading. We
don't create TLB entries for mappings at the time of hotplug. We create
entries in the page tables.

And is it true that changing the memory block size to 256MB necessarily
means we'll never create a 1GiB mapping? It looks like if you call
arch_add_memory() with a 1GiB block it will create 1GiB mappings.

I agree if we add 256MB blocks individually then we won't use a 1GiB
mapping.

But I'm not sure any of the above is all that relevant, because we
didn't make the change to 1GiB for any of those reasons, we did it to
prevent the kernel crashing.

If we *did* want to change the block size for 1GiB for performance
reasons then we need a commit that justifies that.

> The downside is that it changes the granularity
> at which memory can be hot-plugged and hot-unplugged. The implication
> is that if we had less than a 1GiB to hot-plug/hot-unplug that
> would not be possible.

That 2nd sentence is not strong enough, it should say something more
like: It forces hot(un)plug to operate at a 1GiB granularity.

> The reason we had this fix was to resolve an issue where we did not
> split mappings on hot-unplug, leaving a TLB entry that spanned the
> region that was unplugged. This is now fixed by 4dd5f8a99e79 which
> splits the page table, removing the MMU mappings for the hot-unplugged
> region correctly.

I'd say:

This commit was a stop-gap to prevent crashes on hotunplug, caused by
the mismatch between the 1G mappings used for the linear mapping and the
memory block size. Those issues are now resolved because we split the
linear mapping at hotunplug time if necessary, as implemented in commit
4dd5f8a99e79 ("powerpc/mm/radix: Split linear mapping on hot-unplug").

cheers

Re: [PATCH] Revert "powerpc/powernv: Increase memory block size to 1GB on radix"

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:

> On Mon, 30 Apr 2018 18:34:32 +1000
> Balbir Singh <bsingharora@gmail.com> wrote:
>
>> This reverts commit 53ecde0b9126ff140abe3aefd7f0ec64d6fa36b0.
>> 
>> The commit above changed the memblock size to 1GiB, which did some
>> nice things like create fewer TLB entries for mapping memory at
>> the time of hotplug. The downside is that it changes the granularity
>> at which memory can be hot-plugged and hot-unplugged. The implication
>> is that if we had less than a 1GiB to hot-plug/hot-unplug that
>> would not be possible.
>> 
>> The reason we had this fix was to resolve an issue where we did not
>> split mappings on hot-unplug, leaving a TLB entry that spanned the
>> region that was unplugged. This is now fixed by 4dd5f8a99e79 which
>> splits the page table, removing the MMU mappings for the hot-unplugged
>> region correctly.
>> 
>> This trade-offs performance as we increase the number of TLB's to
>> map 1GiB of memory, mapped in via hot-plug, but gives us the ability
>> to handle smaller memory sizes.
>
> Why does powernv have any memory block size override at all now?

I dunno, history says:

    powerpc/powernv: Set memory_block_size_bytes to 256MB
    
    powerpc sets a low SECTION_SIZE_BITS to accomodate small pseries
    boxes. We default to 16MB memory blocks, and boxes with a lot
    of memory end up with enormous numbers of sysfs memory nodes.
    
    Set a more reasonable default for powernv of 256MB.
    
    Signed-off-by: Anton Blanchard <anton@samba.org>


It might make more sense to go back to a single definition for powerpc
and have it return a sensible size based on the total available memory -
though I guess memory hotplugged after boot might make that difficult.

Because we definitely have pseries machines with a lot of memory these
days too.

cheers

Re: [PATCH] Revert "powerpc/powernv: Increase memory block size to 1GB on radix"

[Balbir Singh]

On Mon, Apr 30, 2018 at 8:43 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
>> This reverts commit 53ecde0b9126ff140abe3aefd7f0ec64d6fa36b0.
>
> Firstly everything here only applies to Radix, so we need to say that.

The subject mentions it :)

>
>> The commit above changed the memblock size to 1GiB, which did some
>> nice things like create fewer TLB entries for mapping memory at
>> the time of hotplug.
>
> You say TLB entry here and below, but I think that's misleading. We
> don't create TLB entries for mappings at the time of hotplug. We create
> entries in the page tables.

Agreed! I meant we'll end up with fewer TLB entries for the linear mapping.

>
> And is it true that changing the memory block size to 256MB necessarily
> means we'll never create a 1GiB mapping? It looks like if you call
> arch_add_memory() with a 1GiB block it will create 1GiB mappings.
>
> I agree if we add 256MB blocks individually then we won't use a 1GiB
> mapping.
>
> But I'm not sure any of the above is all that relevant, because we
> didn't make the change to 1GiB for any of those reasons, we did it to
> prevent the kernel crashing.
>
> If we *did* want to change the block size for 1GiB for performance
> reasons then we need a commit that justifies that.
>

I don't deny it, but I think it was a side-effect and we needed to document the
potential impact of the reversal

>> The downside is that it changes the granularity
>> at which memory can be hot-plugged and hot-unplugged. The implication
>> is that if we had less than a 1GiB to hot-plug/hot-unplug that
>> would not be possible.
>
> That 2nd sentence is not strong enough, it should say something more
> like: It forces hot(un)plug to operate at a 1GiB granularity.
>

OK, I think you mean anything modulo 1GiB would not work.

>> The reason we had this fix was to resolve an issue where we did not
>> split mappings on hot-unplug, leaving a TLB entry that spanned the
>> region that was unplugged. This is now fixed by 4dd5f8a99e79 which
>> splits the page table, removing the MMU mappings for the hot-unplugged
>> region correctly.
>
> I'd say:
>
> This commit was a stop-gap to prevent crashes on hotunplug, caused by
> the mismatch between the 1G mappings used for the linear mapping and the
> memory block size. Those issues are now resolved because we split the
> linear mapping at hotunplug time if necessary, as implemented in commit
> 4dd5f8a99e79 ("powerpc/mm/radix: Split linear mapping on hot-unplug").
>
> cheers

Looks good!
Cheers,
Balbir Singh.