| Message ID | 20210524132725.12697-8-apopple@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for SVM atomics in Nouveau | expand |
On Mon, 24 May 2021 23:27:22 +1000 Alistair Popple <apopple@nvidia.com> wrote: > Some devices require exclusive write access to shared virtual > memory (SVM) ranges to perform atomic operations on that memory. This > requires CPU page tables to be updated to deny access whilst atomic > operations are occurring. > > In order to do this introduce a new swap entry > type (SWP_DEVICE_EXCLUSIVE). When a SVM range needs to be marked for > exclusive access by a device all page table mappings for the particular > range are replaced with device exclusive swap entries. This causes any > CPU access to the page to result in a fault. > > Faults are resovled by replacing the faulting entry with the original > mapping. This results in MMU notifiers being called which a driver uses > to update access permissions such as revoking atomic access. After > notifiers have been called the device will no longer have exclusive > access to the region. > > Walking of the page tables to find the target pages is handled by > get_user_pages() rather than a direct page table walk. A direct page > table walk similar to what migrate_vma_collect()/unmap() does could also > have been utilised. However this resulted in more code similar in > functionality to what get_user_pages() provides as page faulting is > required to make the PTEs present and to break COW. > > ... > > Documentation/vm/hmm.rst | 17 ++++ > include/linux/mmu_notifier.h | 6 ++ > include/linux/rmap.h | 4 + > include/linux/swap.h | 7 +- > include/linux/swapops.h | 44 ++++++++- > mm/hmm.c | 5 + > mm/memory.c | 128 +++++++++++++++++++++++- > mm/mprotect.c | 8 ++ > mm/page_vma_mapped.c | 9 +- > mm/rmap.c | 186 +++++++++++++++++++++++++++++++++++ > 10 files changed, 405 insertions(+), 9 deletions(-) > This is quite a lot of code added to core MM for a single driver. Is there any expectation that other drivers will use this code? Is there a way of reducing the impact (code size, at least) for systems which don't need this code? How beneficial is this code to nouveau users? I see that it permits a part of OpenCL to be implemented, but how useful/important is this in the real world? Thanks.
On 5/24/21 3:11 PM, Andrew Morton wrote: >> ... >> >> Documentation/vm/hmm.rst | 17 ++++ >> include/linux/mmu_notifier.h | 6 ++ >> include/linux/rmap.h | 4 + >> include/linux/swap.h | 7 +- >> include/linux/swapops.h | 44 ++++++++- >> mm/hmm.c | 5 + >> mm/memory.c | 128 +++++++++++++++++++++++- >> mm/mprotect.c | 8 ++ >> mm/page_vma_mapped.c | 9 +- >> mm/rmap.c | 186 +++++++++++++++++++++++++++++++++++ >> 10 files changed, 405 insertions(+), 9 deletions(-) >> > > This is quite a lot of code added to core MM for a single driver. > > Is there any expectation that other drivers will use this code? Yes! This should work for GPUs (and potentially, other devices) that support OpenCL SVM atomic accesses on the device. I haven't looked into how amdgpu works in any detail, but that's certainly at the top of the list of likely additional callers. > > Is there a way of reducing the impact (code size, at least) for systems > which don't need this code? I'll leave this question to others for the moment, in order to answer the "do we need it at all" points. > > How beneficial is this code to nouveau users? I see that it permits a > part of OpenCL to be implemented, but how useful/important is this in > the real world? > So this is interesting. Right now, OpenCL support in Nouveau is rather new and so probably not a huge impact yet. However, we've built up enough experience with CUDA and OpenCL to learn that atomic operations, as part of the user space programming model, are a super big deal. Atomic operations are so useful and important that I'd expect many OpenCL SVM users to be uninterested in programming models that lack atomic operations for GPU compute programs. Again, this doesn't rule out future, non-GPU accelerator devices that may come along. Atomic ops are just a really important piece of high-end multi-threaded programming, it turns out. So this is the beginning of support for an important building block for general purpose programming on devices that have GPU-like memory models. thanks,
On Tuesday, 25 May 2021 11:31:17 AM AEST John Hubbard wrote: > On 5/24/21 3:11 PM, Andrew Morton wrote: > >> ... > >> > >> Documentation/vm/hmm.rst | 17 ++++ > >> include/linux/mmu_notifier.h | 6 ++ > >> include/linux/rmap.h | 4 + > >> include/linux/swap.h | 7 +- > >> include/linux/swapops.h | 44 ++++++++- > >> mm/hmm.c | 5 + > >> mm/memory.c | 128 +++++++++++++++++++++++- > >> mm/mprotect.c | 8 ++ > >> mm/page_vma_mapped.c | 9 +- > >> mm/rmap.c | 186 +++++++++++++++++++++++++++++++++++ > >> 10 files changed, 405 insertions(+), 9 deletions(-) > > > > This is quite a lot of code added to core MM for a single driver. > > > > Is there any expectation that other drivers will use this code? > > Yes! This should work for GPUs (and potentially, other devices) that support > OpenCL SVM atomic accesses on the device. I haven't looked into how amdgpu > works in any detail, but that's certainly at the top of the list of likely > additional callers. > > > Is there a way of reducing the impact (code size, at least) for systems > > which don't need this code? All of the code added to mm/rmap.c is specific to implementing this feature and not depended on by other core MM code so could be put behind something like CONFIG_DEVICE_PRIVATE to reduce the code size impact (I realise now it currently isn't but should be). The impact on compiled code size in mm/memory.c also ends up being minimised by the compiler because all of it is of the form: if (is_device_exclusive_entry(...)) { [...] } Meaning it should get thrown away when the feature is not configured given is_device_exclusive_entry() is a static inline always returning false in that case. > I'll leave this question to others for the moment, in order to answer > the "do we need it at all" points. > > > How beneficial is this code to nouveau users? I see that it permits a > > part of OpenCL to be implemented, but how useful/important is this in > > the real world? > > So this is interesting. Right now, OpenCL support in Nouveau is rather new > and so probably not a huge impact yet. However, we've built up enough > experience with CUDA and OpenCL to learn that atomic operations, as part of > the user space programming model, are a super big deal. Atomic operations > are so useful and important that I'd expect many OpenCL SVM users to be > uninterested in programming models that lack atomic operations for GPU > compute programs. > > Again, this doesn't rule out future, non-GPU accelerator devices that may > come along. > > Atomic ops are just a really important piece of high-end multi-threaded > programming, it turns out. So this is the beginning of support for an > important building block for general purpose programming on devices that > have GPU-like memory models. > > > thanks,
On Mon, May 24, 2021 at 03:11:57PM -0700, Andrew Morton wrote: > On Mon, 24 May 2021 23:27:22 +1000 Alistair Popple <apopple@nvidia.com> wrote: > > > Some devices require exclusive write access to shared virtual > > memory (SVM) ranges to perform atomic operations on that memory. This > > requires CPU page tables to be updated to deny access whilst atomic > > operations are occurring. > > > > In order to do this introduce a new swap entry > > type (SWP_DEVICE_EXCLUSIVE). When a SVM range needs to be marked for > > exclusive access by a device all page table mappings for the particular > > range are replaced with device exclusive swap entries. This causes any > > CPU access to the page to result in a fault. > > > > Faults are resovled by replacing the faulting entry with the original > > mapping. This results in MMU notifiers being called which a driver uses > > to update access permissions such as revoking atomic access. After > > notifiers have been called the device will no longer have exclusive > > access to the region. > > > > Walking of the page tables to find the target pages is handled by > > get_user_pages() rather than a direct page table walk. A direct page > > table walk similar to what migrate_vma_collect()/unmap() does could also > > have been utilised. However this resulted in more code similar in > > functionality to what get_user_pages() provides as page faulting is > > required to make the PTEs present and to break COW. > > > > ... > > > > Documentation/vm/hmm.rst | 17 ++++ > > include/linux/mmu_notifier.h | 6 ++ > > include/linux/rmap.h | 4 + > > include/linux/swap.h | 7 +- > > include/linux/swapops.h | 44 ++++++++- > > mm/hmm.c | 5 + > > mm/memory.c | 128 +++++++++++++++++++++++- > > mm/mprotect.c | 8 ++ > > mm/page_vma_mapped.c | 9 +- > > mm/rmap.c | 186 +++++++++++++++++++++++++++++++++++ > > 10 files changed, 405 insertions(+), 9 deletions(-) > > > > This is quite a lot of code added to core MM for a single driver. > > Is there any expectation that other drivers will use this code? > > Is there a way of reducing the impact (code size, at least) for systems > which don't need this code? > > How beneficial is this code to nouveau users? I see that it permits a > part of OpenCL to be implemented, but how useful/important is this in > the real world? That is a very good question! I've not reviewed the code, but a sample program with the described use case would make things easy to parse. I suspect that is not easy to build at the moment? I wonder how we co-ordinate all the work the mm is doing, page migration, reclaim with device exclusive access? Do we have any numbers for the worst case page fault latency when something is marked away for exclusive access? I presume for now this is anonymous memory only? SWP_DEVICE_EXCLUSIVE would only impact the address space of programs using the GPU. Should the exclusively marked range live in the unreclaimable list and recycled back to active/in-active to account for the fact that 1. It is not reclaimable and reclaim will only hurt via page faults? 2. It ages the page correctly or at-least allows for that possibility when the page is used by the GPU. Balbir Singh.
On 5/25/21 4:51 AM, Balbir Singh wrote: ... >> How beneficial is this code to nouveau users? I see that it permits a >> part of OpenCL to be implemented, but how useful/important is this in >> the real world? > > That is a very good question! I've not reviewed the code, but a sample > program with the described use case would make things easy to parse. > I suspect that is not easy to build at the moment? > The cover letter says this: This has been tested with upstream Mesa 21.1.0 and a simple OpenCL program which checks that GPU atomic accesses to system memory are atomic. Without this series the test fails as there is no way of write-protecting the page mapping which results in the device clobbering CPU writes. For reference the test is available at https://ozlabs.org/~apopple/opencl_svm_atomics/ Further testing has been performed by adding support for testing exclusive access to the hmm-tests kselftests. ...so that seems to cover the "sample program" request, at least. > I wonder how we co-ordinate all the work the mm is doing, page migration, > reclaim with device exclusive access? Do we have any numbers for the worst > case page fault latency when something is marked away for exclusive access? CPU page fault latency is approximately "terrible", if a page is resident on the GPU. We have to spin up a DMA engine on the GPU and have it copy the page over the PCIe bus, after all. > I presume for now this is anonymous memory only? SWP_DEVICE_EXCLUSIVE would Yes, for now. > only impact the address space of programs using the GPU. Should the exclusively > marked range live in the unreclaimable list and recycled back to active/in-active > to account for the fact that > > 1. It is not reclaimable and reclaim will only hurt via page faults? > 2. It ages the page correctly or at-least allows for that possibility when the > page is used by the GPU. I'm not sure that that is *necessarily* something we can conclude. It depends upon access patterns of each program. For example, a "reduction" parallel program sends over lots of data to the GPU, and only a tiny bit of (reduced!) data comes back to the CPU. In that case, freeing the physical page on the CPU is actually the best decision for the OS to make (if the OS is sufficiently prescient). thanks,
On Wednesday, 26 May 2021 5:17:18 PM AEST John Hubbard wrote: > On 5/25/21 4:51 AM, Balbir Singh wrote: > ... > > >> How beneficial is this code to nouveau users? I see that it permits a > >> part of OpenCL to be implemented, but how useful/important is this in > >> the real world? > > > > That is a very good question! I've not reviewed the code, but a sample > > program with the described use case would make things easy to parse. > > I suspect that is not easy to build at the moment? > > The cover letter says this: > > This has been tested with upstream Mesa 21.1.0 and a simple OpenCL program > which checks that GPU atomic accesses to system memory are atomic. Without > this series the test fails as there is no way of write-protecting the page > mapping which results in the device clobbering CPU writes. For reference > the test is available at https://ozlabs.org/~apopple/opencl_svm_atomics/ > > Further testing has been performed by adding support for testing exclusive > access to the hmm-tests kselftests. > > ...so that seems to cover the "sample program" request, at least. It is also sufficiently easy to build, assuming of course you have the appropriate Mesa/LLVM/OpenCL libraries installed :-) If you are interested I have some scripts which may help with building Mesa, etc. Not that that is especially hard either, it's just there are a couple of different dependencies required. > > I wonder how we co-ordinate all the work the mm is doing, page migration, > > reclaim with device exclusive access? Do we have any numbers for the worst > > case page fault latency when something is marked away for exclusive > > access? > > CPU page fault latency is approximately "terrible", if a page is resident on > the GPU. We have to spin up a DMA engine on the GPU and have it copy the > page over the PCIe bus, after all. Although for clarity that describes latency for CPU faults to device private pages which are always resident on the GPU. A CPU fault to a page being exclusively accessed will be slightly less terrible as it only requires the GPU MMU/TLB mappings to be taken down in much the same as for any other MMU notifier callback as the page is mapped by the GPU rather than resident there. > > I presume for now this is anonymous memory only? SWP_DEVICE_EXCLUSIVE > > would > > Yes, for now. > > > only impact the address space of programs using the GPU. Should the > > exclusively marked range live in the unreclaimable list and recycled back > > to active/in-active to account for the fact that > > > > 1. It is not reclaimable and reclaim will only hurt via page faults? > > 2. It ages the page correctly or at-least allows for that possibility when > > the> > > page is used by the GPU. > > I'm not sure that that is *necessarily* something we can conclude. It > depends upon access patterns of each program. For example, a "reduction" > parallel program sends over lots of data to the GPU, and only a tiny bit of > (reduced!) data comes back to the CPU. In that case, freeing the physical > page on the CPU is actually the best decision for the OS to make (if the OS > is sufficiently prescient). > > thanks,
On Mon, May 24, 2021 at 11:27:22PM +1000, Alistair Popple wrote: > Some devices require exclusive write access to shared virtual > memory (SVM) ranges to perform atomic operations on that memory. This > requires CPU page tables to be updated to deny access whilst atomic > operations are occurring. > > In order to do this introduce a new swap entry > type (SWP_DEVICE_EXCLUSIVE). When a SVM range needs to be marked for > exclusive access by a device all page table mappings for the particular > range are replaced with device exclusive swap entries. This causes any > CPU access to the page to result in a fault. > > Faults are resovled by replacing the faulting entry with the original > mapping. This results in MMU notifiers being called which a driver uses > to update access permissions such as revoking atomic access. After > notifiers have been called the device will no longer have exclusive > access to the region. > > Walking of the page tables to find the target pages is handled by > get_user_pages() rather than a direct page table walk. A direct page > table walk similar to what migrate_vma_collect()/unmap() does could also > have been utilised. However this resulted in more code similar in > functionality to what get_user_pages() provides as page faulting is > required to make the PTEs present and to break COW. > > Signed-off-by: Alistair Popple <apopple@nvidia.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > > --- > > v9: > * Split rename of migrate_pgmap_owner into a separate patch. > * Added comments explaining SWP_DEVICE_EXCLUSIVE_* entries. > * Renamed try_to_protect{_one} to page_make_device_exclusive{_one} based > somewhat on a suggestion from Peter Xu. I was never particularly happy > with try_to_protect() as a name so think this is better. > * Removed unneccesary code and reworded some comments based on feedback > from Peter Xu. > * Removed the VMA walk when restoring PTEs for device-exclusive entries. > * Simplified implementation of copy_pte_range() to fail if the page > cannot be locked. This might lead to occasional fork() failures but at > this stage we don't think that will be an issue. > > v8: > * Remove device exclusive entries on fork rather than copy them. > > v7: > * Added Christoph's Reviewed-by. > * Minor cosmetic cleanups suggested by Christoph. > * Replace mmu_notifier_range_init_migrate/exclusive with > mmu_notifier_range_init_owner as suggested by Christoph. > * Replaced lock_page() with lock_page_retry() when handling faults. > * Restrict to anonymous pages for now. > > v6: > * Fixed a bisectablity issue due to incorrectly applying the rename of > migrate_pgmap_owner to the wrong patches for Nouveau and hmm_test. > > v5: > * Renamed range->migrate_pgmap_owner to range->owner. > * Added MMU_NOTIFY_EXCLUSIVE to allow passing of a driver cookie which > allows notifiers called as a result of make_device_exclusive_range() to > be ignored. > * Added a check to try_to_protect_one() to detect if the pages originally > returned from get_user_pages() have been unmapped or not. > * Removed check_device_exclusive_range() as it is no longer required with > the other changes. > * Documentation update. > > v4: > * Add function to check that mappings are still valid and exclusive. > * s/long/unsigned long/ in make_device_exclusive_entry(). > --- > Documentation/vm/hmm.rst | 17 ++++ > include/linux/mmu_notifier.h | 6 ++ > include/linux/rmap.h | 4 + > include/linux/swap.h | 7 +- > include/linux/swapops.h | 44 ++++++++- > mm/hmm.c | 5 + > mm/memory.c | 128 +++++++++++++++++++++++- > mm/mprotect.c | 8 ++ > mm/page_vma_mapped.c | 9 +- > mm/rmap.c | 186 +++++++++++++++++++++++++++++++++++ > 10 files changed, 405 insertions(+), 9 deletions(-) > > diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst > index 3df79307a797..a14c2938e7af 100644 > --- a/Documentation/vm/hmm.rst > +++ b/Documentation/vm/hmm.rst > @@ -405,6 +405,23 @@ between device driver specific code and shared common code: > > The lock can now be released. > > +Exclusive access memory > +======================= > + > +Some devices have features such as atomic PTE bits that can be used to implement > +atomic access to system memory. To support atomic operations to a shared virtual > +memory page such a device needs access to that page which is exclusive of any > +userspace access from the CPU. The ``make_device_exclusive_range()`` function > +can be used to make a memory range inaccessible from userspace. > + > +This replaces all mappings for pages in the given range with special swap > +entries. Any attempt to access the swap entry results in a fault which is > +resovled by replacing the entry with the original mapping. A driver gets > +notified that the mapping has been changed by MMU notifiers, after which point > +it will no longer have exclusive access to the page. Exclusive access is > +guranteed to last until the driver drops the page lock and page reference, at > +which point any CPU faults on the page may proceed as described. > + > Memory cgroup (memcg) and rss accounting > ======================================== > > diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h > index 8e428eb813b8..d049e0f6f756 100644 > --- a/include/linux/mmu_notifier.h > +++ b/include/linux/mmu_notifier.h > @@ -42,6 +42,11 @@ struct mmu_interval_notifier; > * @MMU_NOTIFY_MIGRATE: used during migrate_vma_collect() invalidate to signal > * a device driver to possibly ignore the invalidation if the > * owner field matches the driver's device private pgmap owner. > + * > + * @MMU_NOTIFY_EXCLUSIVE: to signal a device driver that the device will no > + * longer have exclusive access to the page. May ignore the invalidation that's > + * part of make_device_exclusive_range() if the owner field > + * matches the value passed to make_device_exclusive_range(). Perhaps s/matches/does not match/? > */ > enum mmu_notifier_event { > MMU_NOTIFY_UNMAP = 0, > @@ -51,6 +56,7 @@ enum mmu_notifier_event { > MMU_NOTIFY_SOFT_DIRTY, > MMU_NOTIFY_RELEASE, > MMU_NOTIFY_MIGRATE, > + MMU_NOTIFY_EXCLUSIVE, > }; > > #define MMU_NOTIFIER_RANGE_BLOCKABLE (1 << 0) > diff --git a/include/linux/rmap.h b/include/linux/rmap.h > index 0e25d829f742..3a1ce4ef9276 100644 > --- a/include/linux/rmap.h > +++ b/include/linux/rmap.h > @@ -193,6 +193,10 @@ int page_referenced(struct page *, int is_locked, > bool try_to_migrate(struct page *page, enum ttu_flags flags); > bool try_to_unmap(struct page *, enum ttu_flags flags); > > +int make_device_exclusive_range(struct mm_struct *mm, unsigned long start, > + unsigned long end, struct page **pages, > + void *arg); > + > /* Avoid racy checks */ > #define PVMW_SYNC (1 << 0) > /* Look for migarion entries rather than present PTEs */ > diff --git a/include/linux/swap.h b/include/linux/swap.h > index a6d4505ecf73..306df39d7c67 100644 > --- a/include/linux/swap.h > +++ b/include/linux/swap.h > @@ -63,11 +63,16 @@ static inline int current_is_kswapd(void) > * > * When a page is migrated from CPU to device, we set the CPU page table entry > * to a special SWP_DEVICE_* entry. s/SWP_DEVICE_*/SWP_DEVICE_{READ|WRITE}/? Since SWP_DEVICE_* covers all four too. > + * > + * When a page is mapped by the device for exclusive access we set the CPU page > + * table entries to special SWP_DEVICE_EXCLUSIVE_* entries. > */ > #ifdef CONFIG_DEVICE_PRIVATE > -#define SWP_DEVICE_NUM 2 > +#define SWP_DEVICE_NUM 4 > #define SWP_DEVICE_WRITE (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM) > #define SWP_DEVICE_READ (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM+1) > +#define SWP_DEVICE_EXCLUSIVE_WRITE (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM+2) > +#define SWP_DEVICE_EXCLUSIVE_READ (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM+3) > #else > #define SWP_DEVICE_NUM 0 > #endif > diff --git a/include/linux/swapops.h b/include/linux/swapops.h > index 4dfd807ae52a..4129bd2ff9d6 100644 > --- a/include/linux/swapops.h > +++ b/include/linux/swapops.h > @@ -120,6 +120,27 @@ static inline bool is_writable_device_private_entry(swp_entry_t entry) > { > return unlikely(swp_type(entry) == SWP_DEVICE_WRITE); > } > + > +static inline swp_entry_t make_readable_device_exclusive_entry(pgoff_t offset) > +{ > + return swp_entry(SWP_DEVICE_EXCLUSIVE_READ, offset); > +} > + > +static inline swp_entry_t make_writable_device_exclusive_entry(pgoff_t offset) > +{ > + return swp_entry(SWP_DEVICE_EXCLUSIVE_WRITE, offset); > +} > + > +static inline bool is_device_exclusive_entry(swp_entry_t entry) > +{ > + return swp_type(entry) == SWP_DEVICE_EXCLUSIVE_READ || > + swp_type(entry) == SWP_DEVICE_EXCLUSIVE_WRITE; > +} > + > +static inline bool is_writable_device_exclusive_entry(swp_entry_t entry) > +{ > + return unlikely(swp_type(entry) == SWP_DEVICE_EXCLUSIVE_WRITE); > +} > #else /* CONFIG_DEVICE_PRIVATE */ > static inline swp_entry_t make_readable_device_private_entry(pgoff_t offset) > { > @@ -140,6 +161,26 @@ static inline bool is_writable_device_private_entry(swp_entry_t entry) > { > return false; > } > + > +static inline swp_entry_t make_readable_device_exclusive_entry(pgoff_t offset) > +{ > + return swp_entry(0, 0); > +} > + > +static inline swp_entry_t make_writable_device_exclusive_entry(pgoff_t offset) > +{ > + return swp_entry(0, 0); > +} > + > +static inline bool is_device_exclusive_entry(swp_entry_t entry) > +{ > + return false; > +} > + > +static inline bool is_writable_device_exclusive_entry(swp_entry_t entry) > +{ > + return false; > +} > #endif /* CONFIG_DEVICE_PRIVATE */ > > #ifdef CONFIG_MIGRATION > @@ -219,7 +260,8 @@ static inline struct page *pfn_swap_entry_to_page(swp_entry_t entry) > */ > static inline bool is_pfn_swap_entry(swp_entry_t entry) > { > - return is_migration_entry(entry) || is_device_private_entry(entry); > + return is_migration_entry(entry) || is_device_private_entry(entry) || > + is_device_exclusive_entry(entry); > } > > struct page_vma_mapped_walk; > diff --git a/mm/hmm.c b/mm/hmm.c > index 11df3ca30b82..fad6be2bf072 100644 > --- a/mm/hmm.c > +++ b/mm/hmm.c > @@ -26,6 +26,8 @@ > #include <linux/mmu_notifier.h> > #include <linux/memory_hotplug.h> > > +#include "internal.h" > + > struct hmm_vma_walk { > struct hmm_range *range; > unsigned long last; > @@ -271,6 +273,9 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr, > if (!non_swap_entry(entry)) > goto fault; > > + if (is_device_exclusive_entry(entry)) > + goto fault; > + > if (is_migration_entry(entry)) { > pte_unmap(ptep); > hmm_vma_walk->last = addr; > diff --git a/mm/memory.c b/mm/memory.c > index e061cfa18c11..c1d2d732f189 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -700,6 +700,68 @@ struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr, > } > #endif > > +static void restore_exclusive_pte(struct vm_area_struct *vma, > + struct page *page, unsigned long address, > + pte_t *ptep) > +{ > + pte_t pte; > + swp_entry_t entry; > + > + pte = pte_mkold(mk_pte(page, READ_ONCE(vma->vm_page_prot))); > + if (pte_swp_soft_dirty(*ptep)) > + pte = pte_mksoft_dirty(pte); > + > + entry = pte_to_swp_entry(*ptep); > + if (pte_swp_uffd_wp(*ptep)) > + pte = pte_mkuffd_wp(pte); > + else if (is_writable_device_exclusive_entry(entry)) > + pte = maybe_mkwrite(pte_mkdirty(pte), vma); > + > + set_pte_at(vma->vm_mm, address, ptep, pte); > + > + /* > + * No need to take a page reference as one was already > + * created when the swap entry was made. > + */ > + if (PageAnon(page)) > + page_add_anon_rmap(page, vma, address, false); > + else > + /* > + * Currently device exclusive access only supports anonymous > + * memory so the entry shouldn't point to a filebacked page. > + */ > + WARN_ON_ONCE(!PageAnon(page)); > + > + if (vma->vm_flags & VM_LOCKED) > + mlock_vma_page(page); > + > + /* > + * No need to invalidate - it was non-present before. However > + * secondary CPUs may have mappings that need invalidating. > + */ > + update_mmu_cache(vma, address, ptep); > +} > + > +/* > + * Tries to restore an exclusive pte if the page lock can be acquired without > + * sleeping. > + */ > +static unsigned long Better return a int? > +try_restore_exclusive_pte(struct mm_struct *src_mm, pte_t *src_pte, > + struct vm_area_struct *vma, unsigned long addr) Raised in the other thread too: src_mm can be dropped. > +{ > + swp_entry_t entry = pte_to_swp_entry(*src_pte); > + struct page *page = pfn_swap_entry_to_page(entry); > + > + if (trylock_page(page)) { > + restore_exclusive_pte(vma, page, addr, src_pte); > + unlock_page(page); > + return 0; > + } > + > + return -EBUSY; > +} > + > /* > * copy one vm_area from one task to the other. Assumes the page tables > * already present in the new task to be cleared in the whole range > @@ -781,6 +843,17 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, > pte = pte_swp_mkuffd_wp(pte); > set_pte_at(src_mm, addr, src_pte, pte); > } > + } else if (is_device_exclusive_entry(entry)) { > + /* > + * Make device exclusive entries present by restoring the > + * original entry then copying as for a present pte. Device > + * exclusive entries currently only support private writable > + * (ie. COW) mappings. > + */ > + VM_BUG_ON(!is_cow_mapping(vma->vm_flags)); > + if (try_restore_exclusive_pte(src_mm, src_pte, vma, addr)) > + return -EBUSY; > + return -ENOENT; > } > set_pte_at(dst_mm, addr, dst_pte, pte); > return 0; > @@ -980,9 +1053,18 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma, > if (ret == -EAGAIN) { > entry = pte_to_swp_entry(*src_pte); > break; > + } else if (ret == -EBUSY) { > + break; > + } else if (!ret) { > + progress += 8; > + continue; > } > - progress += 8; > - continue; > + > + /* > + * Device exclusive entry restored, continue by copying > + * the now present pte. > + */ > + WARN_ON_ONCE(ret != -ENOENT); The change looks right, thanks. It's just that we should start to consider document all these err code now in copy_pte_range() some day (perhaps on top of this patch).. > } > /* copy_present_pte() will clear `*prealloc' if consumed */ > ret = copy_present_pte(dst_vma, src_vma, dst_pte, src_pte, > @@ -1019,6 +1101,8 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma, > goto out; > } > entry.val = 0; > + } else if (ret == -EBUSY) { > + return -EBUSY; > } else if (ret) { > WARN_ON_ONCE(ret != -EAGAIN); > prealloc = page_copy_prealloc(src_mm, src_vma, addr); > @@ -1283,7 +1367,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, > } > > entry = pte_to_swp_entry(ptent); > - if (is_device_private_entry(entry)) { > + if (is_device_private_entry(entry) || > + is_device_exclusive_entry(entry)) { > struct page *page = pfn_swap_entry_to_page(entry); > > if (unlikely(details && details->check_mapping)) { > @@ -1299,7 +1384,10 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, > > pte_clear_not_present_full(mm, addr, pte, tlb->fullmm); > rss[mm_counter(page)]--; > - page_remove_rmap(page, false); > + > + if (is_device_private_entry(entry)) > + page_remove_rmap(page, false); > + > put_page(page); > continue; > } > @@ -3303,6 +3391,35 @@ void unmap_mapping_range(struct address_space *mapping, > } > EXPORT_SYMBOL(unmap_mapping_range); > > +/* > + * Restore a potential device exclusive pte to a working pte entry > + */ > +static vm_fault_t remove_device_exclusive_entry(struct vm_fault *vmf) > +{ > + struct page *page = vmf->page; > + struct vm_area_struct *vma = vmf->vma; > + vm_fault_t ret = 0; > + struct mmu_notifier_range range; > + > + if (!lock_page_or_retry(page, vma->vm_mm, vmf->flags)) > + return VM_FAULT_RETRY; > + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm, > + vmf->address & PAGE_MASK, > + (vmf->address & PAGE_MASK) + PAGE_SIZE); @MMU_NOTIFY_EXCLUSIVE: to signal a device driver that the device will no longer have exclusive access to the page. Shouldn't this be the place to use new MMU_NOTIFY_EXCLUSIVE? > + mmu_notifier_invalidate_range_start(&range); > + > + vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address, > + &vmf->ptl); > + if (likely(pte_same(*vmf->pte, vmf->orig_pte))) > + restore_exclusive_pte(vma, page, vmf->address, vmf->pte); > + > + pte_unmap_unlock(vmf->pte, vmf->ptl); > + unlock_page(page); > + > + mmu_notifier_invalidate_range_end(&range); > + return ret; We can drop "ret" and return 0 here directly. > +} > + > /* > * We enter with non-exclusive mmap_lock (to exclude vma changes, > * but allow concurrent faults), and pte mapped but not yet locked. > @@ -3330,6 +3447,9 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > if (is_migration_entry(entry)) { > migration_entry_wait(vma->vm_mm, vmf->pmd, > vmf->address); > + } else if (is_device_exclusive_entry(entry)) { > + vmf->page = pfn_swap_entry_to_page(entry); > + ret = remove_device_exclusive_entry(vmf); > } else if (is_device_private_entry(entry)) { > vmf->page = pfn_swap_entry_to_page(entry); > ret = vmf->page->pgmap->ops->migrate_to_ram(vmf); > diff --git a/mm/mprotect.c b/mm/mprotect.c > index ee5961888e70..883e2cc85cad 100644 > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -165,6 +165,14 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd, > newpte = swp_entry_to_pte(entry); > if (pte_swp_uffd_wp(oldpte)) > newpte = pte_swp_mkuffd_wp(newpte); > + } else if (is_writable_device_exclusive_entry(entry)) { > + entry = make_readable_device_exclusive_entry( > + swp_offset(entry)); > + newpte = swp_entry_to_pte(entry); > + if (pte_swp_soft_dirty(oldpte)) > + newpte = pte_swp_mksoft_dirty(newpte); > + if (pte_swp_uffd_wp(oldpte)) > + newpte = pte_swp_mkuffd_wp(newpte); > } else { > newpte = oldpte; > } > diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c > index a6a7febb4d93..f535bcb4950c 100644 > --- a/mm/page_vma_mapped.c > +++ b/mm/page_vma_mapped.c > @@ -41,7 +41,8 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw) > > /* Handle un-addressable ZONE_DEVICE memory */ > entry = pte_to_swp_entry(*pvmw->pte); > - if (!is_device_private_entry(entry)) > + if (!is_device_private_entry(entry) && > + !is_device_exclusive_entry(entry)) > return false; > } else if (!pte_present(*pvmw->pte)) > return false; > @@ -93,7 +94,8 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw) > return false; > entry = pte_to_swp_entry(*pvmw->pte); > > - if (!is_migration_entry(entry)) > + if (!is_migration_entry(entry) && > + !is_device_exclusive_entry(entry)) > return false; > > pfn = swp_offset(entry); > @@ -102,7 +104,8 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw) > > /* Handle un-addressable ZONE_DEVICE memory */ > entry = pte_to_swp_entry(*pvmw->pte); > - if (!is_device_private_entry(entry)) > + if (!is_device_private_entry(entry) && > + !is_device_exclusive_entry(entry)) > return false; > > pfn = swp_offset(entry); > diff --git a/mm/rmap.c b/mm/rmap.c > index 8ed1853060cf..fe062f63ef4d 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -2008,6 +2008,192 @@ void page_mlock(struct page *page) > rmap_walk(page, &rwc); > } > > +struct make_exclusive_args { > + struct mm_struct *mm; > + unsigned long address; > + void *owner; > + bool valid; > +}; > + > +static bool page_make_device_exclusive_one(struct page *page, > + struct vm_area_struct *vma, unsigned long address, void *priv) > +{ > + struct mm_struct *mm = vma->vm_mm; > + struct page_vma_mapped_walk pvmw = { > + .page = page, > + .vma = vma, > + .address = address, > + }; > + struct make_exclusive_args *args = priv; > + pte_t pteval; > + struct page *subpage; > + bool ret = true; > + struct mmu_notifier_range range; > + swp_entry_t entry; > + pte_t swp_pte; > + > + mmu_notifier_range_init_owner(&range, MMU_NOTIFY_EXCLUSIVE, 0, vma, Similar question here, EXCLUSIVE comment says it gets notified when the device does not have exclusive access. If you prefer to keep using EXCLUSIVE for both mark/restore, then we need to change the comment above MMU_NOTIFY_EXCLUSIVE? > + vma->vm_mm, address, min(vma->vm_end, > + address + page_size(page)), args->owner); > + mmu_notifier_invalidate_range_start(&range); > + > + while (page_vma_mapped_walk(&pvmw)) { > + /* Unexpected PMD-mapped THP? */ > + VM_BUG_ON_PAGE(!pvmw.pte, page); > + > + if (!pte_present(*pvmw.pte)) { > + ret = false; > + page_vma_mapped_walk_done(&pvmw); > + break; > + } > + > + subpage = page - page_to_pfn(page) + pte_pfn(*pvmw.pte); I see that all pages passed in should be done after FOLL_SPLIT_PMD, so is this needed? Or say, should subpage==page always be true? > + address = pvmw.address; > + > + /* Nuke the page table entry. */ > + flush_cache_page(vma, address, pte_pfn(*pvmw.pte)); > + pteval = ptep_clear_flush(vma, address, pvmw.pte); > + > + /* Move the dirty bit to the page. Now the pte is gone. */ > + if (pte_dirty(pteval)) > + set_page_dirty(page); > + > + if (arch_unmap_one(mm, vma, address, pteval) < 0) { > + set_pte_at(mm, address, pvmw.pte, pteval); > + ret = false; > + page_vma_mapped_walk_done(&pvmw); > + break; > + } Didn't notice this previously, but also suggest to drop this. Two reasons: 1. It's introduced in ca827d55ebaa ("mm, swap: Add infrastructure for saving page metadata on swap", 2018-03-18) for sparc-only use so far. If we really want this, we'll also want to call arch_do_swap_page() when restoring the pte just like what we do in do_swap_page(); NOTE: current code path of SWP_DEVICE_EXCLUSIVE will skip the arch_do_swap_page() in do_swap_page() so it's not even paired with the above arch_unmap_one(), so I believe this won't even work for sparc at all. 2. I highly doubt whether sparc is also on the list of platforms to support for device atomic ops even in the future. IMHO we'd better not copy-paste code clips if never used at all, because once merged, removing it would need more justifications. > + > + /* > + * Check that our target page is still mapped at the expected > + * address. > + */ > + if (args->mm == mm && args->address == address && > + pte_write(pteval)) > + args->valid = true; > + > + /* > + * Store the pfn of the page in a special migration > + * pte. do_swap_page() will wait until the migration > + * pte is removed and then restart fault handling. > + */ > + if (pte_write(pteval)) > + entry = make_writable_device_exclusive_entry( > + page_to_pfn(subpage)); > + else > + entry = make_readable_device_exclusive_entry( > + page_to_pfn(subpage)); > + swp_pte = swp_entry_to_pte(entry); > + if (pte_soft_dirty(pteval)) > + swp_pte = pte_swp_mksoft_dirty(swp_pte); > + if (pte_uffd_wp(pteval)) > + swp_pte = pte_swp_mkuffd_wp(swp_pte); > + > + /* Take a reference for the swap entry */ > + get_page(page); > + set_pte_at(mm, address, pvmw.pte, swp_pte); > + > + page_remove_rmap(subpage, PageHuge(page)); Why PageHuge()? Should it be a constant "false"? > + put_page(page); Should we drop this put_page() along with get_page() above? page_count() should be >0 anyway as we've got a mapcount before at least when dropping the pte. Then IMHO we can simply keep the old page reference. > + } > + > + mmu_notifier_invalidate_range_end(&range); > + > + return ret; > +} > + > +/** > + * page_make_device_exclusive - replace page table mappings with swap entries "with swap entries" looks a bit blurred to me (although below longer comment explains much better). How about below (or something similar): page_make_device_exclusive - Mark the page exclusively owned by the device ? It'll also match with comment above make_device_exclusive_range(). No strong opinion. The rest looks good. Thanks, > + * @page: the page to replace page table entries for > + * @mm: the mm_struct where the page is expected to be mapped > + * @address: address where the page is expected to be mapped > + * @owner: passed to MMU_NOTIFY_EXCLUSIVE range notifier callbacks > + * > + * Tries to remove all the page table entries which are mapping this page and > + * replace them with special device exclusive swap entries to grant a device > + * exclusive access to the page. Caller must hold the page lock. > + * > + * Returns false if the page is still mapped, or if it could not be unmapped > + * from the expected address. Otherwise returns true (success). > + */ > +static bool page_make_device_exclusive(struct page *page, struct mm_struct *mm, > + unsigned long address, void *owner) > +{ > + struct make_exclusive_args args = { > + .mm = mm, > + .address = address, > + .owner = owner, > + .valid = false, > + }; > + struct rmap_walk_control rwc = { > + .rmap_one = page_make_device_exclusive_one, > + .done = page_not_mapped, > + .anon_lock = page_lock_anon_vma_read, > + .arg = &args, > + }; > + > + /* > + * Restrict to anonymous pages for now to avoid potential writeback > + * issues. > + */ > + if (!PageAnon(page)) > + return false; > + > + rmap_walk(page, &rwc); > + > + return args.valid && !page_mapcount(page); > +} > + > +/** > + * make_device_exclusive_range() - Mark a range for exclusive use by a device > + * @mm: mm_struct of assoicated target process > + * @start: start of the region to mark for exclusive device access > + * @end: end address of region > + * @pages: returns the pages which were successfully marked for exclusive access > + * @owner: passed to MMU_NOTIFY_EXCLUSIVE range notifier to allow filtering > + * > + * Returns: number of pages found in the range by GUP. A page is marked for > + * exclusive access only if the page pointer is non-NULL. > + * > + * This function finds ptes mapping page(s) to the given address range, locks > + * them and replaces mappings with special swap entries preventing userspace CPU > + * access. On fault these entries are replaced with the original mapping after > + * calling MMU notifiers. > + * > + * A driver using this to program access from a device must use a mmu notifier > + * critical section to hold a device specific lock during programming. Once > + * programming is complete it should drop the page lock and reference after > + * which point CPU access to the page will revoke the exclusive access. > + */ > +int make_device_exclusive_range(struct mm_struct *mm, unsigned long start, > + unsigned long end, struct page **pages, > + void *owner) > +{ > + unsigned long npages = (end - start) >> PAGE_SHIFT; > + unsigned long i; > + > + npages = get_user_pages_remote(mm, start, npages, > + FOLL_GET | FOLL_WRITE | FOLL_SPLIT_PMD, > + pages, NULL, NULL); > + for (i = 0; i < npages; i++, start += PAGE_SIZE) { > + if (!trylock_page(pages[i])) { > + put_page(pages[i]); > + pages[i] = NULL; > + continue; > + } > + > + if (!page_make_device_exclusive(pages[i], mm, start, owner)) { > + unlock_page(pages[i]); > + put_page(pages[i]); > + pages[i] = NULL; > + } > + } > + > + return npages; > +} > +EXPORT_SYMBOL_GPL(make_device_exclusive_range); > + > void __put_anon_vma(struct anon_vma *anon_vma) > { > struct anon_vma *root = anon_vma->root; > -- > 2.20.1 >
On Thursday, 27 May 2021 5:28:32 AM AEST Peter Xu wrote: > On Mon, May 24, 2021 at 11:27:22PM +1000, Alistair Popple wrote: > > Some devices require exclusive write access to shared virtual > > memory (SVM) ranges to perform atomic operations on that memory. This > > requires CPU page tables to be updated to deny access whilst atomic > > operations are occurring. > > > > In order to do this introduce a new swap entry > > type (SWP_DEVICE_EXCLUSIVE). When a SVM range needs to be marked for > > exclusive access by a device all page table mappings for the particular > > range are replaced with device exclusive swap entries. This causes any > > CPU access to the page to result in a fault. > > > > Faults are resovled by replacing the faulting entry with the original > > mapping. This results in MMU notifiers being called which a driver uses > > to update access permissions such as revoking atomic access. After > > notifiers have been called the device will no longer have exclusive > > access to the region. > > > > Walking of the page tables to find the target pages is handled by > > get_user_pages() rather than a direct page table walk. A direct page > > table walk similar to what migrate_vma_collect()/unmap() does could also > > have been utilised. However this resulted in more code similar in > > functionality to what get_user_pages() provides as page faulting is > > required to make the PTEs present and to break COW. > > > > Signed-off-by: Alistair Popple <apopple@nvidia.com> > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > --- > > > > v9: > > * Split rename of migrate_pgmap_owner into a separate patch. > > * Added comments explaining SWP_DEVICE_EXCLUSIVE_* entries. > > * Renamed try_to_protect{_one} to page_make_device_exclusive{_one} based > > > > somewhat on a suggestion from Peter Xu. I was never particularly happy > > with try_to_protect() as a name so think this is better. > > > > * Removed unneccesary code and reworded some comments based on feedback > > > > from Peter Xu. > > > > * Removed the VMA walk when restoring PTEs for device-exclusive entries. > > * Simplified implementation of copy_pte_range() to fail if the page > > > > cannot be locked. This might lead to occasional fork() failures but at > > this stage we don't think that will be an issue. > > > > v8: > > * Remove device exclusive entries on fork rather than copy them. > > > > v7: > > * Added Christoph's Reviewed-by. > > * Minor cosmetic cleanups suggested by Christoph. > > * Replace mmu_notifier_range_init_migrate/exclusive with > > > > mmu_notifier_range_init_owner as suggested by Christoph. > > > > * Replaced lock_page() with lock_page_retry() when handling faults. > > * Restrict to anonymous pages for now. > > > > v6: > > * Fixed a bisectablity issue due to incorrectly applying the rename of > > > > migrate_pgmap_owner to the wrong patches for Nouveau and hmm_test. > > > > v5: > > * Renamed range->migrate_pgmap_owner to range->owner. > > * Added MMU_NOTIFY_EXCLUSIVE to allow passing of a driver cookie which > > > > allows notifiers called as a result of make_device_exclusive_range() to > > be ignored. > > > > * Added a check to try_to_protect_one() to detect if the pages originally > > > > returned from get_user_pages() have been unmapped or not. > > > > * Removed check_device_exclusive_range() as it is no longer required with > > > > the other changes. > > > > * Documentation update. > > > > v4: > > * Add function to check that mappings are still valid and exclusive. > > * s/long/unsigned long/ in make_device_exclusive_entry(). > > --- > > > > Documentation/vm/hmm.rst | 17 ++++ > > include/linux/mmu_notifier.h | 6 ++ > > include/linux/rmap.h | 4 + > > include/linux/swap.h | 7 +- > > include/linux/swapops.h | 44 ++++++++- > > mm/hmm.c | 5 + > > mm/memory.c | 128 +++++++++++++++++++++++- > > mm/mprotect.c | 8 ++ > > mm/page_vma_mapped.c | 9 +- > > mm/rmap.c | 186 +++++++++++++++++++++++++++++++++++ > > 10 files changed, 405 insertions(+), 9 deletions(-) > > > > diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst > > index 3df79307a797..a14c2938e7af 100644 > > --- a/Documentation/vm/hmm.rst > > +++ b/Documentation/vm/hmm.rst > > > > @@ -405,6 +405,23 @@ between device driver specific code and shared common code: > > The lock can now be released. > > > > +Exclusive access memory > > +======================= > > + > > +Some devices have features such as atomic PTE bits that can be used to > > implement +atomic access to system memory. To support atomic operations > > to a shared virtual +memory page such a device needs access to that page > > which is exclusive of any +userspace access from the CPU. The > > ``make_device_exclusive_range()`` function +can be used to make a memory > > range inaccessible from userspace. > > + > > +This replaces all mappings for pages in the given range with special swap > > +entries. Any attempt to access the swap entry results in a fault which is > > +resovled by replacing the entry with the original mapping. A driver gets > > +notified that the mapping has been changed by MMU notifiers, after which > > point +it will no longer have exclusive access to the page. Exclusive > > access is +guranteed to last until the driver drops the page lock and > > page reference, at +which point any CPU faults on the page may proceed as > > described. > > + > > > > Memory cgroup (memcg) and rss accounting > > ======================================== > > > > diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h > > index 8e428eb813b8..d049e0f6f756 100644 > > --- a/include/linux/mmu_notifier.h > > +++ b/include/linux/mmu_notifier.h > > @@ -42,6 +42,11 @@ struct mmu_interval_notifier; > > > > * @MMU_NOTIFY_MIGRATE: used during migrate_vma_collect() invalidate to > > signal * a device driver to possibly ignore the invalidation if the > > * owner field matches the driver's device private pgmap owner. > > > > + * > > + * @MMU_NOTIFY_EXCLUSIVE: to signal a device driver that the device will > > no + * longer have exclusive access to the page. May ignore the > > invalidation that's + * part of make_device_exclusive_range() if the > > owner field > > + * matches the value passed to make_device_exclusive_range(). > > Perhaps s/matches/does not match/? No, "matches" is correct. The MMU_NOTIFY_EXCLUSIVE notifier is to notify a listener that a range is being invalidated for the purpose of making the range available for some device to have exclusive access to. Which does also mean a device getting the notification no longer has exclusive access if it already did. A unique type is needed because when creating the range a driver needs to form a mmu critical section (with mmu_interval_read_begin()/ mmu_interval_read_end()) to ensure the entry remains valid long enough to program the device pte and hasn't been invalidated. However without a way of filtering any invalidations will result in a retry, but make_device_exclusive_range() needs to do an invalidation during installation of the entry. To avoid this causing infinite retries the driver ignores specific invalidation events that it knows don't apply, ie. the invalidations that are a result of that driver asking for device exclusive entries. Agree the comment could be improved though. > > */ > > > > enum mmu_notifier_event { > > > > MMU_NOTIFY_UNMAP = 0, > > > > @@ -51,6 +56,7 @@ enum mmu_notifier_event { > > > > MMU_NOTIFY_SOFT_DIRTY, > > MMU_NOTIFY_RELEASE, > > MMU_NOTIFY_MIGRATE, > > > > + MMU_NOTIFY_EXCLUSIVE, > > > > }; > > > > #define MMU_NOTIFIER_RANGE_BLOCKABLE (1 << 0) > > > > diff --git a/include/linux/rmap.h b/include/linux/rmap.h > > index 0e25d829f742..3a1ce4ef9276 100644 > > --- a/include/linux/rmap.h > > +++ b/include/linux/rmap.h > > @@ -193,6 +193,10 @@ int page_referenced(struct page *, int is_locked, > > > > bool try_to_migrate(struct page *page, enum ttu_flags flags); > > bool try_to_unmap(struct page *, enum ttu_flags flags); > > > > +int make_device_exclusive_range(struct mm_struct *mm, unsigned long > > start, > > + unsigned long end, struct page **pages, > > + void *arg); > > + > > > > /* Avoid racy checks */ > > #define PVMW_SYNC (1 << 0) > > /* Look for migarion entries rather than present PTEs */ > > > > diff --git a/include/linux/swap.h b/include/linux/swap.h > > index a6d4505ecf73..306df39d7c67 100644 > > --- a/include/linux/swap.h > > +++ b/include/linux/swap.h > > @@ -63,11 +63,16 @@ static inline int current_is_kswapd(void) > > > > * > > * When a page is migrated from CPU to device, we set the CPU page table > > entry * to a special SWP_DEVICE_* entry. > > s/SWP_DEVICE_*/SWP_DEVICE_{READ|WRITE}/? Since SWP_DEVICE_* covers all four > too. Sure. > > + * > > + * When a page is mapped by the device for exclusive access we set the > > CPU page + * table entries to special SWP_DEVICE_EXCLUSIVE_* entries. > > > > */ > > > > #ifdef CONFIG_DEVICE_PRIVATE > > > > -#define SWP_DEVICE_NUM 2 > > +#define SWP_DEVICE_NUM 4 > > > > #define SWP_DEVICE_WRITE > > (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM) > > #define SWP_DEVICE_READ > > (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM+1)> > > +#define SWP_DEVICE_EXCLUSIVE_WRITE > > (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM+2) +#define > > SWP_DEVICE_EXCLUSIVE_READ > > (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM+3)> > > #else > > #define SWP_DEVICE_NUM 0 > > #endif > > > > diff --git a/include/linux/swapops.h b/include/linux/swapops.h > > index 4dfd807ae52a..4129bd2ff9d6 100644 > > --- a/include/linux/swapops.h > > +++ b/include/linux/swapops.h > > @@ -120,6 +120,27 @@ static inline bool > > is_writable_device_private_entry(swp_entry_t entry)> > > { > > > > return unlikely(swp_type(entry) == SWP_DEVICE_WRITE); > > > > } > > > > + > > +static inline swp_entry_t make_readable_device_exclusive_entry(pgoff_t > > offset) +{ > > + return swp_entry(SWP_DEVICE_EXCLUSIVE_READ, offset); > > +} > > + > > +static inline swp_entry_t make_writable_device_exclusive_entry(pgoff_t > > offset) +{ > > + return swp_entry(SWP_DEVICE_EXCLUSIVE_WRITE, offset); > > +} > > + > > +static inline bool is_device_exclusive_entry(swp_entry_t entry) > > +{ > > + return swp_type(entry) == SWP_DEVICE_EXCLUSIVE_READ || > > + swp_type(entry) == SWP_DEVICE_EXCLUSIVE_WRITE; > > +} > > + > > +static inline bool is_writable_device_exclusive_entry(swp_entry_t entry) > > +{ > > + return unlikely(swp_type(entry) == SWP_DEVICE_EXCLUSIVE_WRITE); > > +} > > > > #else /* CONFIG_DEVICE_PRIVATE */ > > static inline swp_entry_t make_readable_device_private_entry(pgoff_t > > offset) { > > > > @@ -140,6 +161,26 @@ static inline bool > > is_writable_device_private_entry(swp_entry_t entry)> > > { > > > > return false; > > > > } > > > > + > > +static inline swp_entry_t make_readable_device_exclusive_entry(pgoff_t > > offset) +{ > > + return swp_entry(0, 0); > > +} > > + > > +static inline swp_entry_t make_writable_device_exclusive_entry(pgoff_t > > offset) +{ > > + return swp_entry(0, 0); > > +} > > + > > +static inline bool is_device_exclusive_entry(swp_entry_t entry) > > +{ > > + return false; > > +} > > + > > +static inline bool is_writable_device_exclusive_entry(swp_entry_t entry) > > +{ > > + return false; > > +} > > > > #endif /* CONFIG_DEVICE_PRIVATE */ > > > > #ifdef CONFIG_MIGRATION > > > > @@ -219,7 +260,8 @@ static inline struct page > > *pfn_swap_entry_to_page(swp_entry_t entry)> > > */ > > > > static inline bool is_pfn_swap_entry(swp_entry_t entry) > > { > > > > - return is_migration_entry(entry) || is_device_private_entry(entry); > > + return is_migration_entry(entry) || is_device_private_entry(entry) > > || > > + is_device_exclusive_entry(entry); > > > > } > > > > struct page_vma_mapped_walk; > > > > diff --git a/mm/hmm.c b/mm/hmm.c > > index 11df3ca30b82..fad6be2bf072 100644 > > --- a/mm/hmm.c > > +++ b/mm/hmm.c > > @@ -26,6 +26,8 @@ > > > > #include <linux/mmu_notifier.h> > > #include <linux/memory_hotplug.h> > > > > +#include "internal.h" > > + > > > > struct hmm_vma_walk { > > > > struct hmm_range *range; > > unsigned long last; > > > > @@ -271,6 +273,9 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, > > unsigned long addr,> > > if (!non_swap_entry(entry)) > > > > goto fault; > > > > + if (is_device_exclusive_entry(entry)) > > + goto fault; > > + > > > > if (is_migration_entry(entry)) { > > > > pte_unmap(ptep); > > hmm_vma_walk->last = addr; > > > > diff --git a/mm/memory.c b/mm/memory.c > > index e061cfa18c11..c1d2d732f189 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -700,6 +700,68 @@ struct page *vm_normal_page_pmd(struct vm_area_struct > > *vma, unsigned long addr,> > > } > > #endif > > > > +static void restore_exclusive_pte(struct vm_area_struct *vma, > > + struct page *page, unsigned long address, > > + pte_t *ptep) > > +{ > > + pte_t pte; > > + swp_entry_t entry; > > + > > + pte = pte_mkold(mk_pte(page, READ_ONCE(vma->vm_page_prot))); > > + if (pte_swp_soft_dirty(*ptep)) > > + pte = pte_mksoft_dirty(pte); > > + > > + entry = pte_to_swp_entry(*ptep); > > + if (pte_swp_uffd_wp(*ptep)) > > + pte = pte_mkuffd_wp(pte); > > + else if (is_writable_device_exclusive_entry(entry)) > > + pte = maybe_mkwrite(pte_mkdirty(pte), vma); > > + > > + set_pte_at(vma->vm_mm, address, ptep, pte); > > + > > + /* > > + * No need to take a page reference as one was already > > + * created when the swap entry was made. > > + */ > > + if (PageAnon(page)) > > + page_add_anon_rmap(page, vma, address, false); > > + else > > + /* > > + * Currently device exclusive access only supports anonymous > > + * memory so the entry shouldn't point to a filebacked page. > > + */ > > + WARN_ON_ONCE(!PageAnon(page)); > > + > > + if (vma->vm_flags & VM_LOCKED) > > + mlock_vma_page(page); > > + > > + /* > > + * No need to invalidate - it was non-present before. However > > + * secondary CPUs may have mappings that need invalidating. > > + */ > > + update_mmu_cache(vma, address, ptep); > > +} > > + > > +/* > > + * Tries to restore an exclusive pte if the page lock can be acquired > > without + * sleeping. > > + */ > > +static unsigned long > > Better return a int? Ok. > > +try_restore_exclusive_pte(struct mm_struct *src_mm, pte_t *src_pte, > > + struct vm_area_struct *vma, unsigned long addr) > > Raised in the other thread too: src_mm can be dropped. Ack, sorry I must have missed that. > > +{ > > + swp_entry_t entry = pte_to_swp_entry(*src_pte); > > + struct page *page = pfn_swap_entry_to_page(entry); > > + > > + if (trylock_page(page)) { > > + restore_exclusive_pte(vma, page, addr, src_pte); > > + unlock_page(page); > > + return 0; > > + } > > + > > + return -EBUSY; > > +} > > + > > > > /* > > > > * copy one vm_area from one task to the other. Assumes the page tables > > * already present in the new task to be cleared in the whole range > > > > @@ -781,6 +843,17 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct > > mm_struct *src_mm,> > > pte = pte_swp_mkuffd_wp(pte); > > > > set_pte_at(src_mm, addr, src_pte, pte); > > > > } > > > > + } else if (is_device_exclusive_entry(entry)) { > > + /* > > + * Make device exclusive entries present by restoring the > > + * original entry then copying as for a present pte. Device > > + * exclusive entries currently only support private writable > > + * (ie. COW) mappings. > > + */ > > + VM_BUG_ON(!is_cow_mapping(vma->vm_flags)); > > + if (try_restore_exclusive_pte(src_mm, src_pte, vma, addr)) > > + return -EBUSY; > > + return -ENOENT; > > > > } > > set_pte_at(dst_mm, addr, dst_pte, pte); > > return 0; > > > > @@ -980,9 +1053,18 @@ copy_pte_range(struct vm_area_struct *dst_vma, > > struct vm_area_struct *src_vma,> > > if (ret == -EAGAIN) { > > > > entry = pte_to_swp_entry(*src_pte); > > break; > > > > + } else if (ret == -EBUSY) { > > + break; > > + } else if (!ret) { > > + progress += 8; > > + continue; > > > > } > > > > - progress += 8; > > - continue; > > + > > + /* > > + * Device exclusive entry restored, continue by > > copying + * the now present pte. > > + */ > > + WARN_ON_ONCE(ret != -ENOENT); > > The change looks right, thanks. It's just that we should start to consider > document all these err code now in copy_pte_range() some day (perhaps on top > of this patch).. I tried to write the documentation but with the new clean-up patch using a unique return code for each case the code ends up being rather self documenting IMHO. It seems reasonably obvious what function returns what due to the "if (ret == ...) break;" statements after each so the comments ended up repeating the code (ie. copy_present_pte() returns this for this case, etc.), but lets see what we think once I've updated. Of course the whole thing is still a bit clunky, so it's still on my list of things to look at reworking/cleaning up in future. > > } > > /* copy_present_pte() will clear `*prealloc' if consumed */ > > ret = copy_present_pte(dst_vma, src_vma, dst_pte, src_pte, > > > > @@ -1019,6 +1101,8 @@ copy_pte_range(struct vm_area_struct *dst_vma, > > struct vm_area_struct *src_vma,> > > goto out; > > > > } > > entry.val = 0; > > > > + } else if (ret == -EBUSY) { > > + return -EBUSY; > > > > } else if (ret) { > > > > WARN_ON_ONCE(ret != -EAGAIN); > > prealloc = page_copy_prealloc(src_mm, src_vma, addr); > > > > @@ -1283,7 +1367,8 @@ static unsigned long zap_pte_range(struct mmu_gather > > *tlb,> > > } > > > > entry = pte_to_swp_entry(ptent); > > > > - if (is_device_private_entry(entry)) { > > + if (is_device_private_entry(entry) || > > + is_device_exclusive_entry(entry)) { > > > > struct page *page = pfn_swap_entry_to_page(entry); > > > > if (unlikely(details && details->check_mapping)) { > > > > @@ -1299,7 +1384,10 @@ static unsigned long zap_pte_range(struct > > mmu_gather *tlb,> > > pte_clear_not_present_full(mm, addr, pte, > > tlb->fullmm); > > rss[mm_counter(page)]--; > > > > - page_remove_rmap(page, false); > > + > > + if (is_device_private_entry(entry)) > > + page_remove_rmap(page, false); > > + > > > > put_page(page); > > continue; > > > > } > > > > @@ -3303,6 +3391,35 @@ void unmap_mapping_range(struct address_space > > *mapping,> > > } > > EXPORT_SYMBOL(unmap_mapping_range); > > > > +/* > > + * Restore a potential device exclusive pte to a working pte entry > > + */ > > +static vm_fault_t remove_device_exclusive_entry(struct vm_fault *vmf) > > +{ > > + struct page *page = vmf->page; > > + struct vm_area_struct *vma = vmf->vma; > > + vm_fault_t ret = 0; > > + struct mmu_notifier_range range; > > + > > + if (!lock_page_or_retry(page, vma->vm_mm, vmf->flags)) > > + return VM_FAULT_RETRY; > > + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, > > vma->vm_mm, > > + vmf->address & PAGE_MASK, > > + (vmf->address & PAGE_MASK) + PAGE_SIZE); > > @MMU_NOTIFY_EXCLUSIVE: to signal a device driver that the device will no > longer have exclusive access to the page. > > Shouldn't this be the place to use new MMU_NOTIFY_EXCLUSIVE? No. We could introduce another type to notify the range is going away due to fault but as mentioned in the other thread I didn't think that was necessary as the only sensible thing a driver can do is invalidate the entry anyway. MMU_NOTIFY_EXCLUSIVE is to signal the invalidation is occurring because the range is being marked for exclusive access (hopefully the explanation earlier makes sense). > > + mmu_notifier_invalidate_range_start(&range); > > + > > + vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address, > > + &vmf->ptl); > > + if (likely(pte_same(*vmf->pte, vmf->orig_pte))) > > + restore_exclusive_pte(vma, page, vmf->address, vmf->pte); > > + > > + pte_unmap_unlock(vmf->pte, vmf->ptl); > > + unlock_page(page); > > + > > + mmu_notifier_invalidate_range_end(&range); > > + return ret; > > We can drop "ret" and return 0 here directly. Agreed, was left over from cleaning this function up in the last version. > > +} > > + > > > > /* > > > > * We enter with non-exclusive mmap_lock (to exclude vma changes, > > * but allow concurrent faults), and pte mapped but not yet locked. > > > > @@ -3330,6 +3447,9 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > > > if (is_migration_entry(entry)) { > > > > migration_entry_wait(vma->vm_mm, vmf->pmd, > > > > vmf->address); > > > > + } else if (is_device_exclusive_entry(entry)) { > > + vmf->page = pfn_swap_entry_to_page(entry); > > + ret = remove_device_exclusive_entry(vmf); > > > > } else if (is_device_private_entry(entry)) { > > > > vmf->page = pfn_swap_entry_to_page(entry); > > ret = vmf->page->pgmap->ops->migrate_to_ram(vmf); > > > > diff --git a/mm/mprotect.c b/mm/mprotect.c > > index ee5961888e70..883e2cc85cad 100644 > > --- a/mm/mprotect.c > > +++ b/mm/mprotect.c > > @@ -165,6 +165,14 @@ static unsigned long change_pte_range(struct > > vm_area_struct *vma, pmd_t *pmd,> > > newpte = swp_entry_to_pte(entry); > > if (pte_swp_uffd_wp(oldpte)) > > > > newpte = pte_swp_mkuffd_wp(newpte); > > > > + } else if > > (is_writable_device_exclusive_entry(entry)) { + > > entry = make_readable_device_exclusive_entry( + > > swp_offset(entry)); + > > newpte = swp_entry_to_pte(entry); > > + if (pte_swp_soft_dirty(oldpte)) > > + newpte = > > pte_swp_mksoft_dirty(newpte); + if > > (pte_swp_uffd_wp(oldpte)) > > + newpte = pte_swp_mkuffd_wp(newpte); > > > > } else { > > > > newpte = oldpte; > > > > } > > > > diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c > > index a6a7febb4d93..f535bcb4950c 100644 > > --- a/mm/page_vma_mapped.c > > +++ b/mm/page_vma_mapped.c > > @@ -41,7 +41,8 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw) > > > > /* Handle un-addressable ZONE_DEVICE memory > > */ > > entry = pte_to_swp_entry(*pvmw->pte); > > > > - if (!is_device_private_entry(entry)) > > + if (!is_device_private_entry(entry) && > > + !is_device_exclusive_entry(entry)) > > > > return false; > > > > } else if (!pte_present(*pvmw->pte)) > > > > return false; > > > > @@ -93,7 +94,8 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw) > > > > return false; > > > > entry = pte_to_swp_entry(*pvmw->pte); > > > > - if (!is_migration_entry(entry)) > > + if (!is_migration_entry(entry) && > > + !is_device_exclusive_entry(entry)) > > > > return false; > > > > pfn = swp_offset(entry); > > > > @@ -102,7 +104,8 @@ static bool check_pte(struct page_vma_mapped_walk > > *pvmw)> > > /* Handle un-addressable ZONE_DEVICE memory */ > > entry = pte_to_swp_entry(*pvmw->pte); > > > > - if (!is_device_private_entry(entry)) > > + if (!is_device_private_entry(entry) && > > + !is_device_exclusive_entry(entry)) > > > > return false; > > > > pfn = swp_offset(entry); > > > > diff --git a/mm/rmap.c b/mm/rmap.c > > index 8ed1853060cf..fe062f63ef4d 100644 > > --- a/mm/rmap.c > > +++ b/mm/rmap.c > > @@ -2008,6 +2008,192 @@ void page_mlock(struct page *page) > > > > rmap_walk(page, &rwc); > > > > } > > > > +struct make_exclusive_args { > > + struct mm_struct *mm; > > + unsigned long address; > > + void *owner; > > + bool valid; > > +}; > > + > > +static bool page_make_device_exclusive_one(struct page *page, > > + struct vm_area_struct *vma, unsigned long address, void > > *priv) +{ > > + struct mm_struct *mm = vma->vm_mm; > > + struct page_vma_mapped_walk pvmw = { > > + .page = page, > > + .vma = vma, > > + .address = address, > > + }; > > + struct make_exclusive_args *args = priv; > > + pte_t pteval; > > + struct page *subpage; > > + bool ret = true; > > + struct mmu_notifier_range range; > > + swp_entry_t entry; > > + pte_t swp_pte; > > + > > + mmu_notifier_range_init_owner(&range, MMU_NOTIFY_EXCLUSIVE, 0, vma, > > Similar question here, EXCLUSIVE comment says it gets notified when the > device does not have exclusive access. > > If you prefer to keep using EXCLUSIVE for both mark/restore, then we need to > change the comment above MMU_NOTIFY_EXCLUSIVE? Yeah, sorry for the confusion that comment was stating the somewhat obvious (any invalidation notifier means a device no longer has exclusive access) but not enough detail about why a driver might treat this specific invalidation reason differently. > > + vma->vm_mm, address, min(vma->vm_end, > > + address + page_size(page)), > > args->owner); + mmu_notifier_invalidate_range_start(&range); > > + > > + while (page_vma_mapped_walk(&pvmw)) { > > + /* Unexpected PMD-mapped THP? */ > > + VM_BUG_ON_PAGE(!pvmw.pte, page); > > + > > + if (!pte_present(*pvmw.pte)) { > > + ret = false; > > + page_vma_mapped_walk_done(&pvmw); > > + break; > > + } > > + > > + subpage = page - page_to_pfn(page) + pte_pfn(*pvmw.pte); > > I see that all pages passed in should be done after FOLL_SPLIT_PMD, so is > this needed? Or say, should subpage==page always be true? Not always, in the case of a thp there are small ptes which will get device exclusive entries. > > + address = pvmw.address; > > + > > + /* Nuke the page table entry. */ > > + flush_cache_page(vma, address, pte_pfn(*pvmw.pte)); > > + pteval = ptep_clear_flush(vma, address, pvmw.pte); > > + > > + /* Move the dirty bit to the page. Now the pte is gone. */ > > + if (pte_dirty(pteval)) > > + set_page_dirty(page); > > + > > + if (arch_unmap_one(mm, vma, address, pteval) < 0) { > > + set_pte_at(mm, address, pvmw.pte, pteval); > > + ret = false; > > + page_vma_mapped_walk_done(&pvmw); > > + break; > > + } > > Didn't notice this previously, but also suggest to drop this. > > Two reasons: > > 1. It's introduced in ca827d55ebaa ("mm, swap: Add infrastructure for saving > page metadata on swap", 2018-03-18) for sparc-only use so far. If we > really want this, we'll also want to call arch_do_swap_page() when > restoring the pte just like what we do in do_swap_page(); NOTE: current > code path of SWP_DEVICE_EXCLUSIVE will skip the arch_do_swap_page() in > do_swap_page() so it's not even paired with the above arch_unmap_one(), so > I believe this won't even work for sparc at all. > > 2. I highly doubt whether sparc is also on the list of platforms to support > for device atomic ops even in the future. IMHO we'd better not copy-paste > code clips if never used at all, because once merged, removing it would > need more justifications. That seems reasonable, I am not aware of any need to support this on sparc now or in the future and we can always add it then. And as you say I had missed the need to pair it with arch_do_swap_page() anyway. > > + > > + /* > > + * Check that our target page is still mapped at the > > expected > > + * address. > > + */ > > + if (args->mm == mm && args->address == address && > > + pte_write(pteval)) > > + args->valid = true; > > + > > + /* > > + * Store the pfn of the page in a special migration > > + * pte. do_swap_page() will wait until the migration > > + * pte is removed and then restart fault handling. > > + */ > > + if (pte_write(pteval)) > > + entry = make_writable_device_exclusive_entry( > > + > > page_to_pfn(subpage)); + else > > + entry = make_readable_device_exclusive_entry( > > + > > page_to_pfn(subpage)); + swp_pte = swp_entry_to_pte(entry); > > + if (pte_soft_dirty(pteval)) > > + swp_pte = pte_swp_mksoft_dirty(swp_pte); > > + if (pte_uffd_wp(pteval)) > > + swp_pte = pte_swp_mkuffd_wp(swp_pte); > > + > > + /* Take a reference for the swap entry */ > > + get_page(page); > > + set_pte_at(mm, address, pvmw.pte, swp_pte); > > + > > + page_remove_rmap(subpage, PageHuge(page)); > > Why PageHuge()? Should it be a constant "false"? Yes. > > + put_page(page); > > Should we drop this put_page() along with get_page() above? > > page_count() should be >0 anyway as we've got a mapcount before at least > when dropping the pte. Then IMHO we can simply keep the old page > reference. I had debated doing that when I wrote it but left it there to keep things obvious whilst checking the refcounting. However a comment here works just as well so have done that. > > + } > > + > > + mmu_notifier_invalidate_range_end(&range); > > + > > + return ret; > > +} > > + > > +/** > > + * page_make_device_exclusive - replace page table mappings with swap > > entries > "with swap entries" looks a bit blurred to me (although below longer comment > explains much better). How about below (or something similar): > > page_make_device_exclusive - Mark the page exclusively owned by the device > > ? Seems good, will do. > It'll also match with comment above make_device_exclusive_range(). > > No strong opinion. > > The rest looks good. Thanks, Thanks again for looking. > > + * @page: the page to replace page table entries for > > + * @mm: the mm_struct where the page is expected to be mapped > > + * @address: address where the page is expected to be mapped > > + * @owner: passed to MMU_NOTIFY_EXCLUSIVE range notifier callbacks > > + * > > + * Tries to remove all the page table entries which are mapping this page > > and + * replace them with special device exclusive swap entries to grant > > a device + * exclusive access to the page. Caller must hold the page > > lock. > > + * > > + * Returns false if the page is still mapped, or if it could not be > > unmapped + * from the expected address. Otherwise returns true (success). > > + */ > > +static bool page_make_device_exclusive(struct page *page, struct > > mm_struct *mm, + unsigned long address, void > > *owner) > > +{ > > + struct make_exclusive_args args = { > > + .mm = mm, > > + .address = address, > > + .owner = owner, > > + .valid = false, > > + }; > > + struct rmap_walk_control rwc = { > > + .rmap_one = page_make_device_exclusive_one, > > + .done = page_not_mapped, > > + .anon_lock = page_lock_anon_vma_read, > > + .arg = &args, > > + }; > > + > > + /* > > + * Restrict to anonymous pages for now to avoid potential writeback > > + * issues. > > + */ > > + if (!PageAnon(page)) > > + return false; > > + > > + rmap_walk(page, &rwc); > > + > > + return args.valid && !page_mapcount(page); > > +} > > + > > +/** > > + * make_device_exclusive_range() - Mark a range for exclusive use by a > > device + * @mm: mm_struct of assoicated target process > > + * @start: start of the region to mark for exclusive device access > > + * @end: end address of region > > + * @pages: returns the pages which were successfully marked for exclusive > > access + * @owner: passed to MMU_NOTIFY_EXCLUSIVE range notifier to allow > > filtering + * > > + * Returns: number of pages found in the range by GUP. A page is marked > > for + * exclusive access only if the page pointer is non-NULL. > > + * > > + * This function finds ptes mapping page(s) to the given address range, > > locks + * them and replaces mappings with special swap entries preventing > > userspace CPU + * access. On fault these entries are replaced with the > > original mapping after + * calling MMU notifiers. > > + * > > + * A driver using this to program access from a device must use a mmu > > notifier + * critical section to hold a device specific lock during > > programming. Once + * programming is complete it should drop the page > > lock and reference after + * which point CPU access to the page will > > revoke the exclusive access. + */ > > +int make_device_exclusive_range(struct mm_struct *mm, unsigned long > > start, > > + unsigned long end, struct page **pages, > > + void *owner) > > +{ > > + unsigned long npages = (end - start) >> PAGE_SHIFT; > > + unsigned long i; > > + > > + npages = get_user_pages_remote(mm, start, npages, > > + FOLL_GET | FOLL_WRITE | > > FOLL_SPLIT_PMD, + pages, NULL, NULL); > > + for (i = 0; i < npages; i++, start += PAGE_SIZE) { > > + if (!trylock_page(pages[i])) { > > + put_page(pages[i]); > > + pages[i] = NULL; > > + continue; > > + } > > + > > + if (!page_make_device_exclusive(pages[i], mm, start, owner)) > > { + unlock_page(pages[i]); > > + put_page(pages[i]); > > + pages[i] = NULL; > > + } > > + } > > + > > + return npages; > > +} > > +EXPORT_SYMBOL_GPL(make_device_exclusive_range); > > + > > > > void __put_anon_vma(struct anon_vma *anon_vma) > > { > > > > struct anon_vma *root = anon_vma->root; > > > > -- > > 2.20.1 > > -- > Peter Xu
On Thu, May 27, 2021 at 01:35:39PM +1000, Alistair Popple wrote: > > > + * > > > + * @MMU_NOTIFY_EXCLUSIVE: to signal a device driver that the device will > > > no + * longer have exclusive access to the page. May ignore the > > > invalidation that's + * part of make_device_exclusive_range() if the > > > owner field > > > + * matches the value passed to make_device_exclusive_range(). > > > > Perhaps s/matches/does not match/? > > No, "matches" is correct. The MMU_NOTIFY_EXCLUSIVE notifier is to notify a > listener that a range is being invalidated for the purpose of making the range > available for some device to have exclusive access to. Which does also mean a > device getting the notification no longer has exclusive access if it already > did. > > A unique type is needed because when creating the range a driver needs to form > a mmu critical section (with mmu_interval_read_begin()/ > mmu_interval_read_end()) to ensure the entry remains valid long enough to > program the device pte and hasn't been invalidated. > > However without a way of filtering any invalidations will result in a retry, > but make_device_exclusive_range() needs to do an invalidation during > installation of the entry. To avoid this causing infinite retries the driver > ignores specific invalidation events that it knows don't apply, ie. the > invalidations that are a result of that driver asking for device exclusive > entries. OK I think I get it now.. so the driver checks both EXCLUSIVE and owner, if all match it skips the notify, otherwise it's treated like all the rest. Thanks. However then it's still confusing (as I raised it too in previous comment) that we use CLEAR when re-installing the valid pte. It's merely against what CLEAR means. How about sending EXCLUSIVE for both mark/restore? Just that when restore we notify with owner==NULL telling that no one is owning it anymore so driver needs to drop the ownership. I assume your driver patch does not need change too. Would that be much cleaner than CLEAR? I bet it also makes commenting the new notify easier. What do you think? [...] > > > + vma->vm_mm, address, min(vma->vm_end, > > > + address + page_size(page)), > > > args->owner); + mmu_notifier_invalidate_range_start(&range); > > > + > > > + while (page_vma_mapped_walk(&pvmw)) { > > > + /* Unexpected PMD-mapped THP? */ > > > + VM_BUG_ON_PAGE(!pvmw.pte, page); > > > + > > > + if (!pte_present(*pvmw.pte)) { > > > + ret = false; > > > + page_vma_mapped_walk_done(&pvmw); > > > + break; > > > + } > > > + > > > + subpage = page - page_to_pfn(page) + pte_pfn(*pvmw.pte); > > > > I see that all pages passed in should be done after FOLL_SPLIT_PMD, so is > > this needed? Or say, should subpage==page always be true? > > Not always, in the case of a thp there are small ptes which will get device > exclusive entries. FOLL_SPLIT_PMD will first split the huge thp into smaller pages, then do follow_page_pte() on them (in follow_pmd_mask): if (flags & FOLL_SPLIT_PMD) { int ret; page = pmd_page(*pmd); if (is_huge_zero_page(page)) { spin_unlock(ptl); ret = 0; split_huge_pmd(vma, pmd, address); if (pmd_trans_unstable(pmd)) ret = -EBUSY; } else { spin_unlock(ptl); split_huge_pmd(vma, pmd, address); ret = pte_alloc(mm, pmd) ? -ENOMEM : 0; } return ret ? ERR_PTR(ret) : follow_page_pte(vma, address, pmd, flags, &ctx->pgmap); } So I thought all pages are small pages?
On Thursday, 27 May 2021 11:04:57 PM AEST Peter Xu wrote: > On Thu, May 27, 2021 at 01:35:39PM +1000, Alistair Popple wrote: > > > > + * > > > > + * @MMU_NOTIFY_EXCLUSIVE: to signal a device driver that the device > > > > will > > > > no + * longer have exclusive access to the page. May ignore the > > > > invalidation that's + * part of make_device_exclusive_range() if the > > > > owner field > > > > + * matches the value passed to make_device_exclusive_range(). > > > > > > Perhaps s/matches/does not match/? > > > > No, "matches" is correct. The MMU_NOTIFY_EXCLUSIVE notifier is to notify a > > listener that a range is being invalidated for the purpose of making the > > range available for some device to have exclusive access to. Which does > > also mean a device getting the notification no longer has exclusive > > access if it already did. > > > > A unique type is needed because when creating the range a driver needs to > > form a mmu critical section (with mmu_interval_read_begin()/ > > mmu_interval_read_end()) to ensure the entry remains valid long enough to > > program the device pte and hasn't been invalidated. > > > > However without a way of filtering any invalidations will result in a > > retry, but make_device_exclusive_range() needs to do an invalidation > > during installation of the entry. To avoid this causing infinite retries > > the driver ignores specific invalidation events that it knows don't > > apply, ie. the invalidations that are a result of that driver asking for > > device exclusive entries. > > OK I think I get it now.. so the driver checks both EXCLUSIVE and owner, if > all match it skips the notify, otherwise it's treated like all the rest. > Thanks. > > However then it's still confusing (as I raised it too in previous comment) > that we use CLEAR when re-installing the valid pte. It's merely against > what CLEAR means. Oh, thanks. I understand where you are coming from now - the pte is already invalid so ordinarily wouldn't need clearing. > How about sending EXCLUSIVE for both mark/restore? Just that when restore > we notify with owner==NULL telling that no one is owning it anymore so > driver needs to drop the ownership. I assume your driver patch does not > need change too. Would that be much cleaner than CLEAR? I bet it also > makes commenting the new notify easier. > > What do you think? That seems like a good and avoids adding another type. And as you say they driver patch shouldn't need changing either (will need to confirm though). > [...] > > > > > + vma->vm_mm, address, > > > > min(vma->vm_end, > > > > + address + page_size(page)), > > > > args->owner); + mmu_notifier_invalidate_range_start(&range); > > > > + > > > > + while (page_vma_mapped_walk(&pvmw)) { > > > > + /* Unexpected PMD-mapped THP? */ > > > > + VM_BUG_ON_PAGE(!pvmw.pte, page); > > > > + > > > > + if (!pte_present(*pvmw.pte)) { > > > > + ret = false; > > > > + page_vma_mapped_walk_done(&pvmw); > > > > + break; > > > > + } > > > > + > > > > + subpage = page - page_to_pfn(page) + pte_pfn(*pvmw.pte); > > > > > > I see that all pages passed in should be done after FOLL_SPLIT_PMD, so > > > is > > > this needed? Or say, should subpage==page always be true? > > > > Not always, in the case of a thp there are small ptes which will get > > device > > exclusive entries. > > FOLL_SPLIT_PMD will first split the huge thp into smaller pages, then do > follow_page_pte() on them (in follow_pmd_mask): > > if (flags & FOLL_SPLIT_PMD) { > int ret; > page = pmd_page(*pmd); > if (is_huge_zero_page(page)) { > spin_unlock(ptl); > ret = 0; > split_huge_pmd(vma, pmd, address); > if (pmd_trans_unstable(pmd)) > ret = -EBUSY; > } else { > spin_unlock(ptl); > split_huge_pmd(vma, pmd, address); > ret = pte_alloc(mm, pmd) ? -ENOMEM : 0; > } > > return ret ? ERR_PTR(ret) : > follow_page_pte(vma, address, pmd, flags, > &ctx->pgmap); } > > So I thought all pages are small pages? The page will remain as a transparent huge page though (at least as I understand things). FOLL_SPLIT_PMD turns it into a pte mapped thp by splitting the pmd and creating pte's mapping the subpages but doesn't split the page itself. For comparison FOLL_SPLIT (which has been removed in v5.13 due to lack of use) is what would be used to split the page in the above GUP code by calling split_huge_page() rather than split_huge_pmd(). This was done to avoid adding code for handling device exclusive entries at the pmd level as well which would have made the changes more complicated and seems unnecessary at least for now. > -- > Peter Xu
On Fri, May 28, 2021 at 11:48:40AM +1000, Alistair Popple wrote: [...] > > > > > + while (page_vma_mapped_walk(&pvmw)) { > > > > > + /* Unexpected PMD-mapped THP? */ > > > > > + VM_BUG_ON_PAGE(!pvmw.pte, page); > > > > > + > > > > > + if (!pte_present(*pvmw.pte)) { > > > > > + ret = false; > > > > > + page_vma_mapped_walk_done(&pvmw); > > > > > + break; > > > > > + } > > > > > + > > > > > + subpage = page - page_to_pfn(page) + pte_pfn(*pvmw.pte); > > > > > > > > I see that all pages passed in should be done after FOLL_SPLIT_PMD, so > > > > is > > > > this needed? Or say, should subpage==page always be true? > > > > > > Not always, in the case of a thp there are small ptes which will get > > > device > > > exclusive entries. > > > > FOLL_SPLIT_PMD will first split the huge thp into smaller pages, then do > > follow_page_pte() on them (in follow_pmd_mask): > > > > if (flags & FOLL_SPLIT_PMD) { > > int ret; > > page = pmd_page(*pmd); > > if (is_huge_zero_page(page)) { > > spin_unlock(ptl); > > ret = 0; > > split_huge_pmd(vma, pmd, address); > > if (pmd_trans_unstable(pmd)) > > ret = -EBUSY; > > } else { > > spin_unlock(ptl); > > split_huge_pmd(vma, pmd, address); > > ret = pte_alloc(mm, pmd) ? -ENOMEM : 0; > > } > > > > return ret ? ERR_PTR(ret) : > > follow_page_pte(vma, address, pmd, flags, > > &ctx->pgmap); } > > > > So I thought all pages are small pages? > > The page will remain as a transparent huge page though (at least as I > understand things). FOLL_SPLIT_PMD turns it into a pte mapped thp by splitting > the pmd and creating pte's mapping the subpages but doesn't split the page > itself. For comparison FOLL_SPLIT (which has been removed in v5.13 due to lack > of use) is what would be used to split the page in the above GUP code by > calling split_huge_page() rather than split_huge_pmd(). But shouldn't FOLL_SPLIT_PMD filled in small pfns for each pte? See __split_huge_pmd_locked(): for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) { ... } else { entry = mk_pte(page + i, READ_ONCE(vma->vm_page_prot)); ... } ... set_pte_at(mm, addr, pte, entry); } Then iiuc the coming follow_page_pte() will directly fetch the small pages?
On Wed, May 26, 2021 at 12:17:18AM -0700, John Hubbard wrote: > On 5/25/21 4:51 AM, Balbir Singh wrote: > ... > > > How beneficial is this code to nouveau users? I see that it permits a > > > part of OpenCL to be implemented, but how useful/important is this in > > > the real world? > > > > That is a very good question! I've not reviewed the code, but a sample > > program with the described use case would make things easy to parse. > > I suspect that is not easy to build at the moment? > > > > The cover letter says this: > > This has been tested with upstream Mesa 21.1.0 and a simple OpenCL program > which checks that GPU atomic accesses to system memory are atomic. Without > this series the test fails as there is no way of write-protecting the page > mapping which results in the device clobbering CPU writes. For reference > the test is available at https://ozlabs.org/~apopple/opencl_svm_atomics/ > > Further testing has been performed by adding support for testing exclusive > access to the hmm-tests kselftests. > > ...so that seems to cover the "sample program" request, at least. Thanks, I'll take a look > > > I wonder how we co-ordinate all the work the mm is doing, page migration, > > reclaim with device exclusive access? Do we have any numbers for the worst > > case page fault latency when something is marked away for exclusive access? > > CPU page fault latency is approximately "terrible", if a page is resident on > the GPU. We have to spin up a DMA engine on the GPU and have it copy the page > over the PCIe bus, after all. > > > I presume for now this is anonymous memory only? SWP_DEVICE_EXCLUSIVE would > > Yes, for now. > > > only impact the address space of programs using the GPU. Should the exclusively > > marked range live in the unreclaimable list and recycled back to active/in-active > > to account for the fact that > > > > 1. It is not reclaimable and reclaim will only hurt via page faults? > > 2. It ages the page correctly or at-least allows for that possibility when the > > page is used by the GPU. > > I'm not sure that that is *necessarily* something we can conclude. It depends upon > access patterns of each program. For example, a "reduction" parallel program sends > over lots of data to the GPU, and only a tiny bit of (reduced!) data comes back > to the CPU. In that case, freeing the physical page on the CPU is actually the > best decision for the OS to make (if the OS is sufficiently prescient). > With a shared device or a device exclusive range, it would be good to get the device usage pattern and update the mm with that knowledge, so that the LRU can be better maintained. With your comment you seem to suggest that a page used by the GPU might be a good candidate for reclaim based on the CPU's understanding of the age of the page should not account for use by the device (are GPU workloads - access once and discard?) Balbir Singh.
On Wed, Jun 02, 2021 at 06:50:37PM +1000, Balbir Singh wrote: > On Wed, May 26, 2021 at 12:17:18AM -0700, John Hubbard wrote: > > On 5/25/21 4:51 AM, Balbir Singh wrote: > > ... > > > > How beneficial is this code to nouveau users? I see that it permits a > > > > part of OpenCL to be implemented, but how useful/important is this in > > > > the real world? > > > > > > That is a very good question! I've not reviewed the code, but a sample > > > program with the described use case would make things easy to parse. > > > I suspect that is not easy to build at the moment? > > > > > > > The cover letter says this: > > > > This has been tested with upstream Mesa 21.1.0 and a simple OpenCL program > > which checks that GPU atomic accesses to system memory are atomic. Without > > this series the test fails as there is no way of write-protecting the page > > mapping which results in the device clobbering CPU writes. For reference > > the test is available at https://ozlabs.org/~apopple/opencl_svm_atomics/ > > > > Further testing has been performed by adding support for testing exclusive > > access to the hmm-tests kselftests. > > > > ...so that seems to cover the "sample program" request, at least. > > Thanks, I'll take a look > > > > > > I wonder how we co-ordinate all the work the mm is doing, page migration, > > > reclaim with device exclusive access? Do we have any numbers for the worst > > > case page fault latency when something is marked away for exclusive access? > > > > CPU page fault latency is approximately "terrible", if a page is resident on > > the GPU. We have to spin up a DMA engine on the GPU and have it copy the page > > over the PCIe bus, after all. > > > > > I presume for now this is anonymous memory only? SWP_DEVICE_EXCLUSIVE would > > > > Yes, for now. > > > > > only impact the address space of programs using the GPU. Should the exclusively > > > marked range live in the unreclaimable list and recycled back to active/in-active > > > to account for the fact that > > > > > > 1. It is not reclaimable and reclaim will only hurt via page faults? > > > 2. It ages the page correctly or at-least allows for that possibility when the > > > page is used by the GPU. > > > > I'm not sure that that is *necessarily* something we can conclude. It depends upon > > access patterns of each program. For example, a "reduction" parallel program sends > > over lots of data to the GPU, and only a tiny bit of (reduced!) data comes back > > to the CPU. In that case, freeing the physical page on the CPU is actually the > > best decision for the OS to make (if the OS is sufficiently prescient). > > > > With a shared device or a device exclusive range, it would be good to get the device > usage pattern and update the mm with that knowledge, so that the LRU can be better > maintained. With your comment you seem to suggest that a page used by the GPU might > be a good candidate for reclaim based on the CPU's understanding of the age of > the page should not account for use by the device > (are GPU workloads - access once and discard?) Hmm, besides the aging info, this reminded me: do we need to isolate the page from lru too when marking device exclusive access? Afaict the current patch didn't do that so I think it's reclaimable. If we still have the rmap then we'll get a mmu notify CLEAR when unmapping that special pte, so device driver should be able to drop the ownership. However we dropped the rmap when marking exclusive. Now I don't know whether and how it'll work if page reclaim runs with the page being exclusively owned if without isolating the page..
On 6/2/21 1:50 AM, Balbir Singh wrote: ... >>> only impact the address space of programs using the GPU. Should the exclusively >>> marked range live in the unreclaimable list and recycled back to active/in-active >>> to account for the fact that >>> >>> 1. It is not reclaimable and reclaim will only hurt via page faults? >>> 2. It ages the page correctly or at-least allows for that possibility when the >>> page is used by the GPU. >> >> I'm not sure that that is *necessarily* something we can conclude. It depends upon >> access patterns of each program. For example, a "reduction" parallel program sends >> over lots of data to the GPU, and only a tiny bit of (reduced!) data comes back >> to the CPU. In that case, freeing the physical page on the CPU is actually the >> best decision for the OS to make (if the OS is sufficiently prescient). >> > > With a shared device or a device exclusive range, it would be good to get the device > usage pattern and update the mm with that knowledge, so that the LRU can be better Integrating a GPU (or "device") processor and it's mm behavior with the Linux kernel is always an interesting concept. Certainly worth exploring, although it's probably not a small project by any means. > maintained. With your comment you seem to suggest that a page used by the GPU might > be a good candidate for reclaim based on the CPU's understanding of the age of > the page should not account for use by the device > (are GPU workloads - access once and discard?) > Well, that's a little too narrow of an interpretation. The GPU is a fairly general purpose processor, and so it has all kinds of workloads. I'm trying to discourage any hopes that one can know, in advance, precisely how the GPU's pages need to be managed. It's similar to the the CPU, in that regard. My example was just one, out of a vast pool of possible behaviors. thanks,
On Thursday, 3 June 2021 12:37:30 AM AEST Peter Xu wrote: > External email: Use caution opening links or attachments > > On Wed, Jun 02, 2021 at 06:50:37PM +1000, Balbir Singh wrote: > > On Wed, May 26, 2021 at 12:17:18AM -0700, John Hubbard wrote: > > > On 5/25/21 4:51 AM, Balbir Singh wrote: > > > ... > > > > > > > > How beneficial is this code to nouveau users? I see that it permits > > > > > a > > > > > part of OpenCL to be implemented, but how useful/important is this > > > > > in > > > > > the real world? > > > > > > > > That is a very good question! I've not reviewed the code, but a sample > > > > program with the described use case would make things easy to parse. > > > > I suspect that is not easy to build at the moment? > > > > > > The cover letter says this: > > > > > > This has been tested with upstream Mesa 21.1.0 and a simple OpenCL > > > program > > > which checks that GPU atomic accesses to system memory are atomic. > > > Without > > > this series the test fails as there is no way of write-protecting the > > > page > > > mapping which results in the device clobbering CPU writes. For reference > > > the test is available at https://ozlabs.org/~apopple/opencl_svm_atomics/ > > > > > > Further testing has been performed by adding support for testing > > > exclusive > > > access to the hmm-tests kselftests. > > > > > > ...so that seems to cover the "sample program" request, at least. > > > > Thanks, I'll take a look > > > > > > I wonder how we co-ordinate all the work the mm is doing, page > > > > migration, > > > > reclaim with device exclusive access? Do we have any numbers for the > > > > worst > > > > case page fault latency when something is marked away for exclusive > > > > access? > > > > > > CPU page fault latency is approximately "terrible", if a page is > > > resident on the GPU. We have to spin up a DMA engine on the GPU and > > > have it copy the page over the PCIe bus, after all. > > > > > > > I presume for now this is anonymous memory only? SWP_DEVICE_EXCLUSIVE > > > > would > > > > > > Yes, for now. > > > > > > > only impact the address space of programs using the GPU. Should the > > > > exclusively marked range live in the unreclaimable list and recycled > > > > back to active/in-active to account for the fact that > > > > > > > > 1. It is not reclaimable and reclaim will only hurt via page faults? > > > > 2. It ages the page correctly or at-least allows for that possibility > > > > when the> > > > > > > page is used by the GPU. > > > > > > I'm not sure that that is *necessarily* something we can conclude. It > > > depends upon access patterns of each program. For example, a > > > "reduction" parallel program sends over lots of data to the GPU, and > > > only a tiny bit of (reduced!) data comes back to the CPU. In that case, > > > freeing the physical page on the CPU is actually the best decision for > > > the OS to make (if the OS is sufficiently prescient).> > > With a shared device or a device exclusive range, it would be good to get > > the device usage pattern and update the mm with that knowledge, so that > > the LRU can be better maintained. With your comment you seem to suggest > > that a page used by the GPU might be a good candidate for reclaim based > > on the CPU's understanding of the age of the page should not account for > > use by the device > > (are GPU workloads - access once and discard?) > > Hmm, besides the aging info, this reminded me: do we need to isolate the > page from lru too when marking device exclusive access? > > Afaict the current patch didn't do that so I think it's reclaimable. If we > still have the rmap then we'll get a mmu notify CLEAR when unmapping that > special pte, so device driver should be able to drop the ownership. However > we dropped the rmap when marking exclusive. Now I don't know whether and > how it'll work if page reclaim runs with the page being exclusively owned > if without isolating the page.. Reclaim won't run on the page due to the extra references from the special swap entries. > -- > Peter Xu
On Thu, Jun 03, 2021 at 09:39:32PM +1000, Alistair Popple wrote: > Reclaim won't run on the page due to the extra references from the special > swap entries. That sounds reasonable, but I didn't find the point that stops it, probably due to my limited knowledge on the reclaim code. Could you elaborate?
On Friday, 4 June 2021 12:47:40 AM AEST Peter Xu wrote: > External email: Use caution opening links or attachments > > On Thu, Jun 03, 2021 at 09:39:32PM +1000, Alistair Popple wrote: > > Reclaim won't run on the page due to the extra references from the special > > swap entries. > > That sounds reasonable, but I didn't find the point that stops it, probably > due to my limited knowledge on the reclaim code. Could you elaborate? Sure, it isn't immediately obvious but it ends up being detected at the start of is_page_cache_freeable() in the pageout code: static pageout_t pageout(struct page *page, struct address_space *mapping) { [...] if (!is_page_cache_freeable(page)) return PAGE_KEEP; - Alistair > -- > Peter Xu
On Fri, Jun 04, 2021 at 11:07:42AM +1000, Alistair Popple wrote: > On Friday, 4 June 2021 12:47:40 AM AEST Peter Xu wrote: > > External email: Use caution opening links or attachments > > > > On Thu, Jun 03, 2021 at 09:39:32PM +1000, Alistair Popple wrote: > > > Reclaim won't run on the page due to the extra references from the special > > > swap entries. > > > > That sounds reasonable, but I didn't find the point that stops it, probably > > due to my limited knowledge on the reclaim code. Could you elaborate? > > Sure, it isn't immediately obvious but it ends up being detected at the start > of is_page_cache_freeable() in the pageout code: > > > static pageout_t pageout(struct page *page, struct address_space *mapping) > { > > [...] > > if (!is_page_cache_freeable(page)) > return PAGE_KEEP; I did look at pageout() but still missed this small helper indeed (while it's so important to know..), thanks!
diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst index 3df79307a797..a14c2938e7af 100644 --- a/Documentation/vm/hmm.rst +++ b/Documentation/vm/hmm.rst @@ -405,6 +405,23 @@ between device driver specific code and shared common code: The lock can now be released. +Exclusive access memory +======================= + +Some devices have features such as atomic PTE bits that can be used to implement +atomic access to system memory. To support atomic operations to a shared virtual +memory page such a device needs access to that page which is exclusive of any +userspace access from the CPU. The ``make_device_exclusive_range()`` function +can be used to make a memory range inaccessible from userspace. + +This replaces all mappings for pages in the given range with special swap +entries. Any attempt to access the swap entry results in a fault which is +resovled by replacing the entry with the original mapping. A driver gets +notified that the mapping has been changed by MMU notifiers, after which point +it will no longer have exclusive access to the page. Exclusive access is +guranteed to last until the driver drops the page lock and page reference, at +which point any CPU faults on the page may proceed as described. + Memory cgroup (memcg) and rss accounting ======================================== diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h index 8e428eb813b8..d049e0f6f756 100644 --- a/include/linux/mmu_notifier.h +++ b/include/linux/mmu_notifier.h @@ -42,6 +42,11 @@ struct mmu_interval_notifier; * @MMU_NOTIFY_MIGRATE: used during migrate_vma_collect() invalidate to signal * a device driver to possibly ignore the invalidation if the * owner field matches the driver's device private pgmap owner. + * + * @MMU_NOTIFY_EXCLUSIVE: to signal a device driver that the device will no + * longer have exclusive access to the page. May ignore the invalidation that's + * part of make_device_exclusive_range() if the owner field + * matches the value passed to make_device_exclusive_range(). */ enum mmu_notifier_event { MMU_NOTIFY_UNMAP = 0, @@ -51,6 +56,7 @@ enum mmu_notifier_event { MMU_NOTIFY_SOFT_DIRTY, MMU_NOTIFY_RELEASE, MMU_NOTIFY_MIGRATE, + MMU_NOTIFY_EXCLUSIVE, }; #define MMU_NOTIFIER_RANGE_BLOCKABLE (1 << 0) diff --git a/include/linux/rmap.h b/include/linux/rmap.h index 0e25d829f742..3a1ce4ef9276 100644 --- a/include/linux/rmap.h +++ b/include/linux/rmap.h @@ -193,6 +193,10 @@ int page_referenced(struct page *, int is_locked, bool try_to_migrate(struct page *page, enum ttu_flags flags); bool try_to_unmap(struct page *, enum ttu_flags flags); +int make_device_exclusive_range(struct mm_struct *mm, unsigned long start, + unsigned long end, struct page **pages, + void *arg); + /* Avoid racy checks */ #define PVMW_SYNC (1 << 0) /* Look for migarion entries rather than present PTEs */ diff --git a/include/linux/swap.h b/include/linux/swap.h index a6d4505ecf73..306df39d7c67 100644 --- a/include/linux/swap.h +++ b/include/linux/swap.h @@ -63,11 +63,16 @@ static inline int current_is_kswapd(void) * * When a page is migrated from CPU to device, we set the CPU page table entry * to a special SWP_DEVICE_* entry. + * + * When a page is mapped by the device for exclusive access we set the CPU page + * table entries to special SWP_DEVICE_EXCLUSIVE_* entries. */ #ifdef CONFIG_DEVICE_PRIVATE -#define SWP_DEVICE_NUM 2 +#define SWP_DEVICE_NUM 4 #define SWP_DEVICE_WRITE (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM) #define SWP_DEVICE_READ (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM+1) +#define SWP_DEVICE_EXCLUSIVE_WRITE (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM+2) +#define SWP_DEVICE_EXCLUSIVE_READ (MAX_SWAPFILES+SWP_HWPOISON_NUM+SWP_MIGRATION_NUM+3) #else #define SWP_DEVICE_NUM 0 #endif diff --git a/include/linux/swapops.h b/include/linux/swapops.h index 4dfd807ae52a..4129bd2ff9d6 100644 --- a/include/linux/swapops.h +++ b/include/linux/swapops.h @@ -120,6 +120,27 @@ static inline bool is_writable_device_private_entry(swp_entry_t entry) { return unlikely(swp_type(entry) == SWP_DEVICE_WRITE); } + +static inline swp_entry_t make_readable_device_exclusive_entry(pgoff_t offset) +{ + return swp_entry(SWP_DEVICE_EXCLUSIVE_READ, offset); +} + +static inline swp_entry_t make_writable_device_exclusive_entry(pgoff_t offset) +{ + return swp_entry(SWP_DEVICE_EXCLUSIVE_WRITE, offset); +} + +static inline bool is_device_exclusive_entry(swp_entry_t entry) +{ + return swp_type(entry) == SWP_DEVICE_EXCLUSIVE_READ || + swp_type(entry) == SWP_DEVICE_EXCLUSIVE_WRITE; +} + +static inline bool is_writable_device_exclusive_entry(swp_entry_t entry) +{ + return unlikely(swp_type(entry) == SWP_DEVICE_EXCLUSIVE_WRITE); +} #else /* CONFIG_DEVICE_PRIVATE */ static inline swp_entry_t make_readable_device_private_entry(pgoff_t offset) { @@ -140,6 +161,26 @@ static inline bool is_writable_device_private_entry(swp_entry_t entry) { return false; } + +static inline swp_entry_t make_readable_device_exclusive_entry(pgoff_t offset) +{ + return swp_entry(0, 0); +} + +static inline swp_entry_t make_writable_device_exclusive_entry(pgoff_t offset) +{ + return swp_entry(0, 0); +} + +static inline bool is_device_exclusive_entry(swp_entry_t entry) +{ + return false; +} + +static inline bool is_writable_device_exclusive_entry(swp_entry_t entry) +{ + return false; +} #endif /* CONFIG_DEVICE_PRIVATE */ #ifdef CONFIG_MIGRATION @@ -219,7 +260,8 @@ static inline struct page *pfn_swap_entry_to_page(swp_entry_t entry) */ static inline bool is_pfn_swap_entry(swp_entry_t entry) { - return is_migration_entry(entry) || is_device_private_entry(entry); + return is_migration_entry(entry) || is_device_private_entry(entry) || + is_device_exclusive_entry(entry); } struct page_vma_mapped_walk; diff --git a/mm/hmm.c b/mm/hmm.c index 11df3ca30b82..fad6be2bf072 100644 --- a/mm/hmm.c +++ b/mm/hmm.c @@ -26,6 +26,8 @@ #include <linux/mmu_notifier.h> #include <linux/memory_hotplug.h> +#include "internal.h" + struct hmm_vma_walk { struct hmm_range *range; unsigned long last; @@ -271,6 +273,9 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr, if (!non_swap_entry(entry)) goto fault; + if (is_device_exclusive_entry(entry)) + goto fault; + if (is_migration_entry(entry)) { pte_unmap(ptep); hmm_vma_walk->last = addr; diff --git a/mm/memory.c b/mm/memory.c index e061cfa18c11..c1d2d732f189 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -700,6 +700,68 @@ struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr, } #endif +static void restore_exclusive_pte(struct vm_area_struct *vma, + struct page *page, unsigned long address, + pte_t *ptep) +{ + pte_t pte; + swp_entry_t entry; + + pte = pte_mkold(mk_pte(page, READ_ONCE(vma->vm_page_prot))); + if (pte_swp_soft_dirty(*ptep)) + pte = pte_mksoft_dirty(pte); + + entry = pte_to_swp_entry(*ptep); + if (pte_swp_uffd_wp(*ptep)) + pte = pte_mkuffd_wp(pte); + else if (is_writable_device_exclusive_entry(entry)) + pte = maybe_mkwrite(pte_mkdirty(pte), vma); + + set_pte_at(vma->vm_mm, address, ptep, pte); + + /* + * No need to take a page reference as one was already + * created when the swap entry was made. + */ + if (PageAnon(page)) + page_add_anon_rmap(page, vma, address, false); + else + /* + * Currently device exclusive access only supports anonymous + * memory so the entry shouldn't point to a filebacked page. + */ + WARN_ON_ONCE(!PageAnon(page)); + + if (vma->vm_flags & VM_LOCKED) + mlock_vma_page(page); + + /* + * No need to invalidate - it was non-present before. However + * secondary CPUs may have mappings that need invalidating. + */ + update_mmu_cache(vma, address, ptep); +} + +/* + * Tries to restore an exclusive pte if the page lock can be acquired without + * sleeping. + */ +static unsigned long +try_restore_exclusive_pte(struct mm_struct *src_mm, pte_t *src_pte, + struct vm_area_struct *vma, unsigned long addr) +{ + swp_entry_t entry = pte_to_swp_entry(*src_pte); + struct page *page = pfn_swap_entry_to_page(entry); + + if (trylock_page(page)) { + restore_exclusive_pte(vma, page, addr, src_pte); + unlock_page(page); + return 0; + } + + return -EBUSY; +} + /* * copy one vm_area from one task to the other. Assumes the page tables * already present in the new task to be cleared in the whole range @@ -781,6 +843,17 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm, pte = pte_swp_mkuffd_wp(pte); set_pte_at(src_mm, addr, src_pte, pte); } + } else if (is_device_exclusive_entry(entry)) { + /* + * Make device exclusive entries present by restoring the + * original entry then copying as for a present pte. Device + * exclusive entries currently only support private writable + * (ie. COW) mappings. + */ + VM_BUG_ON(!is_cow_mapping(vma->vm_flags)); + if (try_restore_exclusive_pte(src_mm, src_pte, vma, addr)) + return -EBUSY; + return -ENOENT; } set_pte_at(dst_mm, addr, dst_pte, pte); return 0; @@ -980,9 +1053,18 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma, if (ret == -EAGAIN) { entry = pte_to_swp_entry(*src_pte); break; + } else if (ret == -EBUSY) { + break; + } else if (!ret) { + progress += 8; + continue; } - progress += 8; - continue; + + /* + * Device exclusive entry restored, continue by copying + * the now present pte. + */ + WARN_ON_ONCE(ret != -ENOENT); } /* copy_present_pte() will clear `*prealloc' if consumed */ ret = copy_present_pte(dst_vma, src_vma, dst_pte, src_pte, @@ -1019,6 +1101,8 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma, goto out; } entry.val = 0; + } else if (ret == -EBUSY) { + return -EBUSY; } else if (ret) { WARN_ON_ONCE(ret != -EAGAIN); prealloc = page_copy_prealloc(src_mm, src_vma, addr); @@ -1283,7 +1367,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, } entry = pte_to_swp_entry(ptent); - if (is_device_private_entry(entry)) { + if (is_device_private_entry(entry) || + is_device_exclusive_entry(entry)) { struct page *page = pfn_swap_entry_to_page(entry); if (unlikely(details && details->check_mapping)) { @@ -1299,7 +1384,10 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, pte_clear_not_present_full(mm, addr, pte, tlb->fullmm); rss[mm_counter(page)]--; - page_remove_rmap(page, false); + + if (is_device_private_entry(entry)) + page_remove_rmap(page, false); + put_page(page); continue; } @@ -3303,6 +3391,35 @@ void unmap_mapping_range(struct address_space *mapping, } EXPORT_SYMBOL(unmap_mapping_range); +/* + * Restore a potential device exclusive pte to a working pte entry + */ +static vm_fault_t remove_device_exclusive_entry(struct vm_fault *vmf) +{ + struct page *page = vmf->page; + struct vm_area_struct *vma = vmf->vma; + vm_fault_t ret = 0; + struct mmu_notifier_range range; + + if (!lock_page_or_retry(page, vma->vm_mm, vmf->flags)) + return VM_FAULT_RETRY; + mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm, + vmf->address & PAGE_MASK, + (vmf->address & PAGE_MASK) + PAGE_SIZE); + mmu_notifier_invalidate_range_start(&range); + + vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address, + &vmf->ptl); + if (likely(pte_same(*vmf->pte, vmf->orig_pte))) + restore_exclusive_pte(vma, page, vmf->address, vmf->pte); + + pte_unmap_unlock(vmf->pte, vmf->ptl); + unlock_page(page); + + mmu_notifier_invalidate_range_end(&range); + return ret; +} + /* * We enter with non-exclusive mmap_lock (to exclude vma changes, * but allow concurrent faults), and pte mapped but not yet locked. @@ -3330,6 +3447,9 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) if (is_migration_entry(entry)) { migration_entry_wait(vma->vm_mm, vmf->pmd, vmf->address); + } else if (is_device_exclusive_entry(entry)) { + vmf->page = pfn_swap_entry_to_page(entry); + ret = remove_device_exclusive_entry(vmf); } else if (is_device_private_entry(entry)) { vmf->page = pfn_swap_entry_to_page(entry); ret = vmf->page->pgmap->ops->migrate_to_ram(vmf); diff --git a/mm/mprotect.c b/mm/mprotect.c index ee5961888e70..883e2cc85cad 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -165,6 +165,14 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd, newpte = swp_entry_to_pte(entry); if (pte_swp_uffd_wp(oldpte)) newpte = pte_swp_mkuffd_wp(newpte); + } else if (is_writable_device_exclusive_entry(entry)) { + entry = make_readable_device_exclusive_entry( + swp_offset(entry)); + newpte = swp_entry_to_pte(entry); + if (pte_swp_soft_dirty(oldpte)) + newpte = pte_swp_mksoft_dirty(newpte); + if (pte_swp_uffd_wp(oldpte)) + newpte = pte_swp_mkuffd_wp(newpte); } else { newpte = oldpte; } diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c index a6a7febb4d93..f535bcb4950c 100644 --- a/mm/page_vma_mapped.c +++ b/mm/page_vma_mapped.c @@ -41,7 +41,8 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw) /* Handle un-addressable ZONE_DEVICE memory */ entry = pte_to_swp_entry(*pvmw->pte); - if (!is_device_private_entry(entry)) + if (!is_device_private_entry(entry) && + !is_device_exclusive_entry(entry)) return false; } else if (!pte_present(*pvmw->pte)) return false; @@ -93,7 +94,8 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw) return false; entry = pte_to_swp_entry(*pvmw->pte); - if (!is_migration_entry(entry)) + if (!is_migration_entry(entry) && + !is_device_exclusive_entry(entry)) return false; pfn = swp_offset(entry); @@ -102,7 +104,8 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw) /* Handle un-addressable ZONE_DEVICE memory */ entry = pte_to_swp_entry(*pvmw->pte); - if (!is_device_private_entry(entry)) + if (!is_device_private_entry(entry) && + !is_device_exclusive_entry(entry)) return false; pfn = swp_offset(entry); diff --git a/mm/rmap.c b/mm/rmap.c index 8ed1853060cf..fe062f63ef4d 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -2008,6 +2008,192 @@ void page_mlock(struct page *page) rmap_walk(page, &rwc); } +struct make_exclusive_args { + struct mm_struct *mm; + unsigned long address; + void *owner; + bool valid; +}; + +static bool page_make_device_exclusive_one(struct page *page, + struct vm_area_struct *vma, unsigned long address, void *priv) +{ + struct mm_struct *mm = vma->vm_mm; + struct page_vma_mapped_walk pvmw = { + .page = page, + .vma = vma, + .address = address, + }; + struct make_exclusive_args *args = priv; + pte_t pteval; + struct page *subpage; + bool ret = true; + struct mmu_notifier_range range; + swp_entry_t entry; + pte_t swp_pte; + + mmu_notifier_range_init_owner(&range, MMU_NOTIFY_EXCLUSIVE, 0, vma, + vma->vm_mm, address, min(vma->vm_end, + address + page_size(page)), args->owner); + mmu_notifier_invalidate_range_start(&range); + + while (page_vma_mapped_walk(&pvmw)) { + /* Unexpected PMD-mapped THP? */ + VM_BUG_ON_PAGE(!pvmw.pte, page); + + if (!pte_present(*pvmw.pte)) { + ret = false; + page_vma_mapped_walk_done(&pvmw); + break; + } + + subpage = page - page_to_pfn(page) + pte_pfn(*pvmw.pte); + address = pvmw.address; + + /* Nuke the page table entry. */ + flush_cache_page(vma, address, pte_pfn(*pvmw.pte)); + pteval = ptep_clear_flush(vma, address, pvmw.pte); + + /* Move the dirty bit to the page. Now the pte is gone. */ + if (pte_dirty(pteval)) + set_page_dirty(page); + + if (arch_unmap_one(mm, vma, address, pteval) < 0) { + set_pte_at(mm, address, pvmw.pte, pteval); + ret = false; + page_vma_mapped_walk_done(&pvmw); + break; + } + + /* + * Check that our target page is still mapped at the expected + * address. + */ + if (args->mm == mm && args->address == address && + pte_write(pteval)) + args->valid = true; + + /* + * Store the pfn of the page in a special migration + * pte. do_swap_page() will wait until the migration + * pte is removed and then restart fault handling. + */ + if (pte_write(pteval)) + entry = make_writable_device_exclusive_entry( + page_to_pfn(subpage)); + else + entry = make_readable_device_exclusive_entry( + page_to_pfn(subpage)); + swp_pte = swp_entry_to_pte(entry); + if (pte_soft_dirty(pteval)) + swp_pte = pte_swp_mksoft_dirty(swp_pte); + if (pte_uffd_wp(pteval)) + swp_pte = pte_swp_mkuffd_wp(swp_pte); + + /* Take a reference for the swap entry */ + get_page(page); + set_pte_at(mm, address, pvmw.pte, swp_pte); + + page_remove_rmap(subpage, PageHuge(page)); + put_page(page); + } + + mmu_notifier_invalidate_range_end(&range); + + return ret; +} + +/** + * page_make_device_exclusive - replace page table mappings with swap entries + * @page: the page to replace page table entries for + * @mm: the mm_struct where the page is expected to be mapped + * @address: address where the page is expected to be mapped + * @owner: passed to MMU_NOTIFY_EXCLUSIVE range notifier callbacks + * + * Tries to remove all the page table entries which are mapping this page and + * replace them with special device exclusive swap entries to grant a device + * exclusive access to the page. Caller must hold the page lock. + * + * Returns false if the page is still mapped, or if it could not be unmapped + * from the expected address. Otherwise returns true (success). + */ +static bool page_make_device_exclusive(struct page *page, struct mm_struct *mm, + unsigned long address, void *owner) +{ + struct make_exclusive_args args = { + .mm = mm, + .address = address, + .owner = owner, + .valid = false, + }; + struct rmap_walk_control rwc = { + .rmap_one = page_make_device_exclusive_one, + .done = page_not_mapped, + .anon_lock = page_lock_anon_vma_read, + .arg = &args, + }; + + /* + * Restrict to anonymous pages for now to avoid potential writeback + * issues. + */ + if (!PageAnon(page)) + return false; + + rmap_walk(page, &rwc); + + return args.valid && !page_mapcount(page); +} + +/** + * make_device_exclusive_range() - Mark a range for exclusive use by a device + * @mm: mm_struct of assoicated target process + * @start: start of the region to mark for exclusive device access + * @end: end address of region + * @pages: returns the pages which were successfully marked for exclusive access + * @owner: passed to MMU_NOTIFY_EXCLUSIVE range notifier to allow filtering + * + * Returns: number of pages found in the range by GUP. A page is marked for + * exclusive access only if the page pointer is non-NULL. + * + * This function finds ptes mapping page(s) to the given address range, locks + * them and replaces mappings with special swap entries preventing userspace CPU + * access. On fault these entries are replaced with the original mapping after + * calling MMU notifiers. + * + * A driver using this to program access from a device must use a mmu notifier + * critical section to hold a device specific lock during programming. Once + * programming is complete it should drop the page lock and reference after + * which point CPU access to the page will revoke the exclusive access. + */ +int make_device_exclusive_range(struct mm_struct *mm, unsigned long start, + unsigned long end, struct page **pages, + void *owner) +{ + unsigned long npages = (end - start) >> PAGE_SHIFT; + unsigned long i; + + npages = get_user_pages_remote(mm, start, npages, + FOLL_GET | FOLL_WRITE | FOLL_SPLIT_PMD, + pages, NULL, NULL); + for (i = 0; i < npages; i++, start += PAGE_SIZE) { + if (!trylock_page(pages[i])) { + put_page(pages[i]); + pages[i] = NULL; + continue; + } + + if (!page_make_device_exclusive(pages[i], mm, start, owner)) { + unlock_page(pages[i]); + put_page(pages[i]); + pages[i] = NULL; + } + } + + return npages; +} +EXPORT_SYMBOL_GPL(make_device_exclusive_range); + void __put_anon_vma(struct anon_vma *anon_vma) { struct anon_vma *root = anon_vma->root;