On Monday, 15 March 2021 6:42:45 PM AEDT Christoph Hellwig wrote: > > +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 > > +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. > > 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. > > 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. > > } > > + } 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. > > + > > +static bool try_to_protect_one(struct page *page, struct vm_area_struct *vma, > > + 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. > > > + return ttp.valid && (!page_mapcount(page) ? true : false); > > This can be simplified to: > > return ttp.valid && !page_mapcount(page); > > > + 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 <h...@lst.de> > Thanks. _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau