On Mon, Apr 30, 2018 at 8:43 PM, Michael Ellerman <m...@ellerman.id.au> wrote:
> Balbir Singh <bsinghar...@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.
