On 23.10.19 09:26, David Hildenbrand wrote:
On 22.10.19 23:54, Dan Williams wrote:
Hi David,
Thanks for tackling this!
Thanks for having a look :)
[...]
I am probably a little bit too careful (but I don't want to break things).
In most places (besides KVM and vfio that are nuts), the
pfn_to_online_page() check could most probably be avoided by a
is_zone_device_page() check. However, I usually get suspicious when I see
a pfn_valid() check (especially after I learned that people mmap parts of
/dev/mem into user space, including memory without memmaps. Also, people
could memmap offline memory blocks this way :/). As long as this does not
hurt performance, I think we should rather do it the clean way.
I'm concerned about using is_zone_device_page() in places that are not
known to already have a reference to the page. Here's an audit of
current usages, and the ones I think need to cleaned up. The "unsafe"
ones do not appear to have any protections against the device page
being removed (get_dev_pagemap()). Yes, some of these were added by
me. The "unsafe? HMM" ones need HMM eyes because HMM leaks device
pages into anonymous memory paths and I'm not up to speed on how it
guarantees 'struct page' validity vs device shutdown without using
get_dev_pagemap().
smaps_pmd_entry(): unsafe
put_devmap_managed_page(): safe, page reference is held
is_device_private_page(): safe? gpu driver manages private page lifetime
is_pci_p2pdma_page(): safe, page reference is held
uncharge_page(): unsafe? HMM
add_to_kill(): safe, protected by get_dev_pagemap() and dax_lock_page()
soft_offline_page(): unsafe
remove_migration_pte(): unsafe? HMM
move_to_new_page(): unsafe? HMM
migrate_vma_pages() and helpers: unsafe? HMM
try_to_unmap_one(): unsafe? HMM
__put_page(): safe
release_pages(): safe
I'm hoping all the HMM ones can be converted to
is_device_private_page() directlly and have that routine grow a nice
comment about how it knows it can always safely de-reference its @page
argument.
For the rest I'd like to propose that we add a facility to determine
ZONE_DEVICE by pfn rather than page. The most straightforward why I
can think of would be to just add another bitmap to mem_section_usage
to indicate if a subsection is ZONE_DEVICE or not.
(it's a somewhat unrelated bigger discussion, but we can start discussing it in this thread)
I dislike this for three reasons
a) It does not protect against any races, really, it does not improve things.
b) We do have the exact same problem with pfn_to_online_page(). As long as we
don't hold the memory hotplug lock, memory can get offlined and remove any time. Racy.
c) We mix in ZONE specific stuff into the core. It should be "just another zone"
What I propose instead (already discussed in https://lkml.org/lkml/2019/10/10/87)
1. Convert SECTION_IS_ONLINE to SECTION_IS_ACTIVE
2. Convert SECTION_IS_ACTIVE to a subsection bitmap
3. Introduce pfn_active() that checks against the subsection bitmap
4. Once the memmap was initialized / prepared, set the subsection active
(similar to SECTION_IS_ONLINE in the buddy right now)
5. Before the memmap gets invalidated, set the subsection inactive
(similar to SECTION_IS_ONLINE in the buddy right now)
5. pfn_to_online_page() = pfn_active() && zone != ZONE_DEVICE
6. pfn_to_device_page() = pfn_active() && zone == ZONE_DEVICE
Dan, I am suspecting that you want a pfn_to_zone() that will not touch
the memmap, because it could potentially (altmap) lie on slow memory, right?
A modification might make this possible (but I am not yet sure if we
want a less generic MM implementation just to fine tune slow memmap
access here)
1. Keep SECTION_IS_ONLINE as it is with the same semantics
2. Introduce a subsection bitmap to record active ("initialized memmap")
PFNs. E.g., also set it when setting sections online.
3. Introduce pfn_active() that checks against the subsection bitmap
4. Once the memmap was initialized / prepared, set the subsection active
(similar to SECTION_IS_ONLINE in the buddy right now)
5. Before the memmap gets invalidated, set the subsection inactive
(similar to SECTION_IS_ONLINE in the buddy right now)
5. pfn_to_online_page() = pfn_active() && section == SECTION_IS_ONLINE
(or keep it as is, depends on the RCU locking we eventually
implement)
6. pfn_to_device_page() = pfn_active() && section != SECTION_IS_ONLINE
7. use pfn_active() whenever we don't care about the zone.
Again, not really a friend of that, it hardcodes ZONE_DEVICE vs.
!ZONE_DEVICE. When we do a random "pfn_to_page()" (e.g., a pfn walker)
we really want to touch the memmap right away either way. So we can also
directly read the zone from it. I really do prefer right now a more
generic implementation.
--
Thanks,
David / dhildenb
