Re: [PATCH v6 5/8] mm: Device exclusive memory access

On Monday, 15 March 2021 6:42:45 PM AEDT Christoph Hellwig wrote:

quoted

+Not all devices support atomic access to system memory. To support atomic
+operations to a shared virtual memory page such a device needs access to 

that

quoted

+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

quoted

+inaccessible from userspace.

s/Not all devices/Some devices/ ?

I will reword this. What I was trying to convey is that devices may have 
features which allow for atomics to be implemented with SW assistance.

quoted

 static inline int mm_has_notifiers(struct mm_struct *mm)

@@ -528,7 +534,17 @@ static inline void mmu_notifier_range_init_migrate(
 {
 	mmu_notifier_range_init(range, MMU_NOTIFY_MIGRATE, flags, vma, mm,
 				start, end);
-	range->migrate_pgmap_owner = pgmap;
+	range->owner = pgmap;
+}
+
+static inline void mmu_notifier_range_init_exclusive(
+			struct mmu_notifier_range *range, unsigned int flags,
+			struct vm_area_struct *vma, struct mm_struct *mm,
+			unsigned long start, unsigned long end, void *owner)
+{
+	mmu_notifier_range_init(range, MMU_NOTIFY_EXCLUSIVE, flags, vma, mm,
+				start, end);
+	range->owner = owner;

Maybe just replace mmu_notifier_range_init_migrate with a
mmu_notifier_range_init_owner helper that takes the owner but does
not hard code a type?

Ok. That does result in a function which takes a fair number of arguments, but 
I guess that's no worse than multiple functions hard coding the different 
types and it does result in less code overall.

quoted

 		}
+	} else if (is_device_exclusive_entry(entry)) {
+		page = pfn_swap_entry_to_page(entry);
+
+		get_page(page);
+		rss[mm_counter(page)]++;
+
+		if (is_writable_device_exclusive_entry(entry) &&
+		    is_cow_mapping(vm_flags)) {
+			/*
+			 * COW mappings require pages in both
+			 * parent and child to be set to read.
+			 */
+			entry = make_readable_device_exclusive_entry(
+							swp_offset(entry));
+			pte = swp_entry_to_pte(entry);
+			if (pte_swp_soft_dirty(*src_pte))
+				pte = pte_swp_mksoft_dirty(pte);
+			if (pte_swp_uffd_wp(*src_pte))
+				pte = pte_swp_mkuffd_wp(pte);
+			set_pte_at(src_mm, addr, src_pte, pte);
+		}

Just cosmetic, but I wonder if should factor this code block into
a little helper.

In that case there are arguably are other bits of this function which should 
be refactored into helpers as well. Unless you feel strongly about it I would 
like to leave this as is and put together a future series to fix this and a 
couple of other areas I've noticed that could do with some refactoring/clean 
ups.

quoted

+
+static bool try_to_protect_one(struct page *page, struct vm_area_struct 

*vma,

quoted

+			unsigned long address, void *arg)
+{
+	struct mm_struct *mm = vma->vm_mm;
+	struct page_vma_mapped_walk pvmw = {
+		.page = page,
+		.vma = vma,
+		.address = address,
+	};
+	struct ttp_args *ttp = (struct ttp_args *) arg;

This cast should not be needed.

quoted

+	return ttp.valid && (!page_mapcount(page) ? true : false);

This can be simplified to:

	return ttp.valid && !page_mapcount(page);

quoted

+	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 (!try_to_protect(pages[i], mm, start, arg)) {
+			unlock_page(pages[i]);
+			put_page(pages[i]);
+			pages[i] = NULL;
+		}

Should the trylock_page go into try_to_protect to simplify the loop
a little?  Also I wonder if we need make_device_exclusive_range or
should just open code the get_user_pages_remote + try_to_protect
loop in the callers, as that might allow them to also deduct other
information about the found pages.

This function has evolved over time and putting the trylock_page into 
try_to_protect does simplify things nicely. I'm not sure what other 
information a caller could deduct through open coding though, but I guess in 
some circumstances it might be possible for callers to skip 
get_user_pages_remote() which might be a future improvement.

The main reason it looks like this was simply to keep it looking fairly 
similar to how hmm_range_fault() and migrate_vma() are used with an array of 
pages (or pfns) which are filled out from the given address range.
 

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks.