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

[Michael Ellerman <mpe@ellerman.id.au>]

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