Messages in this thread | | | Subject | Re: [RFC 1/2] arm64/mm: Fix pfn_valid() for ZONE_DEVICE based memory | From | Anshuman Khandual <> | Date | Thu, 28 Jan 2021 13:12:28 +0530 |
| |
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.marinas@arm.com> >>>>> Cc: Will Deacon <will@kernel.org> >>>>> Cc: Ard Biesheuvel <ardb@kernel.org> >>>>> Cc: Robin Murphy <robin.murphy@arm.com> >>>>> Cc: linux-arm-kernel@lists.infradead.org >>>>> Cc: linux-kernel@vger.kernel.org >>>>> Fixes: 73b20c84d42d ("arm64: mm: implement pte_devmap support") >>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@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.
| |