On 1/27/21 2:59 PM, David Hildenbrand wrote:
> On 27.01.21 05:06, Anshuman Khandual wrote:
>>
>>
>> On 1/25/21 2:43 PM, David Hildenbrand wrote:
>>> On 25.01.21 07:22, Anshuman Khandual wrote:
>>>>
>>>> On 12/22/20 12:42 PM, Anshuman Khandual wrote:
>>>>> pfn_valid() asserts that there is a memblock entry for a given pfn without
>>>>> MEMBLOCK_NOMAP flag being set. The problem with ZONE_DEVICE based memory
>>>>> is
>>>>> that they do not have memblock entries. Hence memblock_is_map_memory()
>>>>> will
>>>>> invariably fail via memblock_search() for a ZONE_DEVICE based address.
>>>>> This
>>>>> eventually fails pfn_valid() which is wrong. memblock_is_map_memory()
>>>>> needs
>>>>> to be skipped for such memory ranges. As ZONE_DEVICE memory gets
>>>>> hotplugged
>>>>> into the system via memremap_pages() called from a driver, their
>>>>> respective
>>>>> memory sections will not have SECTION_IS_EARLY set.
>>>>>
>>>>> Normal hotplug memory will never have MEMBLOCK_NOMAP set in their memblock
>>>>> regions. Because the flag MEMBLOCK_NOMAP was specifically designed and set
>>>>> for firmware reserved memory regions. memblock_is_map_memory() can just be
>>>>> skipped as its always going to be positive and that will be an
>>>>> optimization
>>>>> for the normal hotplug memory. Like ZONE_DEVIE based memory, all
>>>>> hotplugged
>>>>> normal memory too will not have SECTION_IS_EARLY set for their sections.
>>>>>
>>>>> Skipping memblock_is_map_memory() for all non early memory sections would
>>>>> fix pfn_valid() problem for ZONE_DEVICE based memory and also improve its
>>>>> performance for normal hotplug memory as well.
>>>>>
>>>>> Cc: Catalin Marinas <catalin.mari...@arm.com>
>>>>> Cc: Will Deacon <w...@kernel.org>
>>>>> Cc: Ard Biesheuvel <a...@kernel.org>
>>>>> Cc: Robin Murphy <robin.mur...@arm.com>
>>>>> Cc: linux-arm-ker...@lists.infradead.org
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> Fixes: 73b20c84d42d ("arm64: mm: implement pte_devmap support")
>>>>> Signed-off-by: Anshuman Khandual <anshuman.khand...@arm.com>
>>>>
>>>> Hello David/Mike,
>>>>
>>>> Given that we would need to rework early sections, memblock semantics via a
>>>> new config i.e EARLY_SECTION_MEMMAP_HOLES and also some possible changes to
>>>> ARCH_KEEP_MEMBLOCK and HAVE_ARCH_PFN_VALID, wondering if these patches here
>>>> which fixes a problem (and improves performance) can be merged first. After
>>>> that, I could start working on the proposed rework. Could you please let me
>>>> know your thoughts on this. Thank you.
>>>
>>> As I said, we might have to throw in an pfn_section_valid() check, to
>>> catch not-section-aligned ZONE_DEVICE ranges (I assume this is possible
>>> on arm64 as well, no?).
>>
>> pfn_section_valid() should be called only for !early_section() i.e normal
>> hotplug and ZONE_DEVICE memory ? Because early boot memory should always
>> be section aligned.
>
> Well, at least not on x86-64 you can have early sections intersect with
> ZONE_DEVICE memory.
>
> E.g., have 64MB boot memory in a section. Later, we add ZONE_DEVICE memory
> which might cover the remaining 64MB. For pfn_valid() on x86-64, we always
> return "true" for such sections, because we always have the memmap for the
> whole early section allocated during boot. So, there it's "simple".
This is the generic pfn_valid() used on X86. As you mentioned this
does not test pfn_section_valid() if the section is early assuming
that vmemmap coverage is complete.
#ifndef CONFIG_HAVE_ARCH_PFN_VALID
static inline int pfn_valid(unsigned long pfn)
{
struct mem_section *ms;
if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
return 0;
ms = __nr_to_section(pfn_to_section_nr(pfn));
if (!valid_section(ms))
return 0;
/*
* Traditionally early sections always returned pfn_valid() for
* the entire section-sized span.
*/
return early_section(ms) || pfn_section_valid(ms, pfn);
}
#endif
Looking at the code, seems like early sections get initialized via
sparse_init() only in section granularity but then please correct
me otherwise.
>
> Now, arm64 seems to discard some parts of the vmemmap, so the remaining 64MB
> in such an early section might not have a memmap anymore? TBH, I don't know.
Did not get that. Could you please be more specific on how arm64 discards
parts of the vmemmap.
>
> Most probably only performing the check for
> !early_section() is sufficient on arm64, but I really can't tell as I don't
> know what we're actually discarding and if something as described for x86-64
> is even possible on arm64.
Seems like direct users for arch_add_memory() and __add_pages() like
pagemap_range() can cause subsection hotplug and vmemmap mapping. So
pfn_section_valid() should be applicable only for !early_sections().
Although a simple test on arm64 shows that both boot memory and
traditional memory hotplug gets entire subsection_map populated. But
that might not be always true for ZONE_DEVICE memory.
>
> We should really try to take the magic out of arm64 vmemmap handling.
I would really like to understand more about this.
>
>>
>>>
>>> Apart from that, I'm fine with a simple fix upfront, that can be more
>>> easily backported if needed. (Q: do we? is this stable material?)
>>>
>>
>> Right, an upfront fix here would help in backporting. AFAICS it should be
>> backported to the stable as pte_devmap and ZONE_DEVICE have been around
>> for some time now. Do you have a particular stable version which needs to
>> be tagged in the patch ?
>
> I haven't looked yet TBH. I guess it is broken since ZONE_DEVICE was enabled
> on arm64?
>
Sure, will figure this out.
