On Mon, Sep 9, 2019 at 5:06 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> On 09.09.19 13:53, Dan Williams wrote:
> > On Mon, Sep 9, 2019 at 1:11 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
> > [..]
> >>>> It seems that SECTION_IS_ONLINE and SECTION_MARKED_PRESENT can be used to
> >>>> distinguish uninitialized struct pages if we can apply them to ZONE_DEVICE,
> >>>> but that is no longer necessary with this approach.
> >>>
> >>> Let's take a step back here to understand the issues I am aware of. I
> >>> think we should solve this for good now:
> >>>
> >>> A PFN walker takes a look at a random PFN at a random point in time. It
> >>> finds a PFN with SECTION_MARKED_PRESENT && !SECTION_IS_ONLINE. The
> >>> options are:
> >>>
> >>> 1. It is buddy memory (add_memory()) that has not been online yet. The
> >>> memmap contains garbage. Don't access.
> >>>
> >>> 2. It is ZONE_DEVICE memory with a valid memmap. Access it.
> >>>
> >>> 3. It is ZONE_DEVICE memory with an invalid memmap, because the section
> >>> is only partially present: E.g., device starts at offset 64MB within a
> >>> section or the device ends at offset 64MB within a section. Don't access it.
> >>>
> >>> 4. It is ZONE_DEVICE memory with an invalid memmap, because the memmap
> >>> was not initialized yet. memmap_init_zone_device() did not yet succeed
> >>> after dropping the mem_hotplug lock in mm/memremap.c. Don't access it.
> >>>
> >>> 5. It is reserved ZONE_DEVICE memory ("pages mapped, but reserved for
> >>> driver") with an invalid memmap. Don't access it.
> >>>
> >>> I can see that your patch tries to make #5 vanish by initializing the
> >>> memmap, fair enough. #3 and #4 can't be detected. The PFN walker could
> >>> still stumble over uninitialized memmaps.
> >>>
> >>
> >> FWIW, I thinkg having something like pfn_zone_device(), similarly
> >> implemented like pfn_zone_device_reserved() could be one solution to
> >> most issues.
> >
> > I've been thinking of a replacement for PTE_DEVMAP with section-level,
> > or sub-section level flags. The section-level flag would still require
> > a call to get_dev_pagemap() to validate that the pfn is not section in
> > the subsection case which seems to be entirely too much overhead. If
> > ZONE_DEVICE is to be a first class citizen in pfn walkers I think it
> > would be worth the cost to double the size of subsection_map and to
> > identify whether a sub-section is ZONE_DEVICE, or not.
> >
> > Thoughts?
> >
>
> I thought about this last week and came up with something like
>
> 1. Convert SECTION_IS_ONLINE to SECTION IS_ACTIVE
>
> 2. Make pfn_to_online_page() also check that it's not ZONE_DEVICE.
> Online pfns are limited to !ZONE_DEVICE.
>
> 3. Extend subsection_map to an additional active_map
>
> 4. Set SECTION IS_ACTIVE *iff* the whole active_map is set. This keeps
> most accesses of pfn_to_online_page() fast. If !SECTION IS_ACTIVE, check
> the active_map.
>
> 5. Set sub-sections active/unactive in
> move_pfn_range_to_zone()/remove_pfn_range_from_zone() - see "[PATCH v4
> 0/8] mm/memory_hotplug: Shrink zones before removing memory" for the
> latter.
>
> 6. Set boot memory properly active (this is a tricky bit :/ ).
>
> However, it turned out too complex for my taste (and limited time to
> spend on this), so I abandoned that idea for now. If somebody wants to
> pick that up, fine.
>
That seems to solve the pfn walk case but it would not address the
need for PTE_DEVMAP or speed up the other places that want an
efficient way to determine if it's worthwhile to call
get_dev_pagemap().
