On Fri, Jul 10, 2026 at 01:38:35PM -0300, Jason Gunthorpe wrote: > On Tue, Jul 07, 2026 at 12:47:01PM -0700, Stanislav Kinsburskii wrote: > > hmm_range_fault() requires the caller to hold the mmap read lock for the > > duration of the call. This is incompatible with mappings whose fault > > handler may release the mmap lock, notably userfaultfd-managed regions, > > where handle_mm_fault() can return VM_FAULT_RETRY or VM_FAULT_COMPLETED > > after dropping the lock. Drivers that need to populate device page tables > > for such mappings have no way to do so today. > > sashiko could not apply v7 for some reason but the remarks on v6 > seemed meaningful, did you see them were they delt with? >
Yes, I dealt with them. > https://sashiko.dev/#/patchset/178336023903.504354.7500950448226027718.stgit%40skinsburskii > > > diff --git a/Documentation/mm/hmm.rst b/Documentation/mm/hmm.rst > > index 7d61b7a8b65b..70885f153d03 100644 > > --- a/Documentation/mm/hmm.rst > > +++ b/Documentation/mm/hmm.rst > > @@ -208,6 +208,69 @@ invalidate() callback. That lock must be held before > > calling > > mmu_interval_read_retry() to avoid any race with a concurrent CPU page > > table > > update. > > > > +Dropping the mmap lock during page faults > > +========================================= > > + > > +Some VMAs have fault handlers that need to release the mmap lock while > > +servicing a fault (for example, regions managed by ``userfaultfd``). > > +``hmm_range_fault()`` cannot be used on such mappings because it must hold > > the > > +mmap lock for the duration of the call. Drivers that need to support them > > +should call:: > > Given the majority of callers use this API it should probably be the > focus of the documentation and example, regulate the existing API to a > 'BTW if you really need the mmap lock, and you really shouldn't, this > exists too' > Sure, I'll update the doc to reflect it this way. > > @@ -32,6 +32,7 @@ > > > > struct hmm_vma_walk { > > struct hmm_range *range; > > + int *locked; > > Let's use bool if you have to respin this > Sure. > > @@ -651,37 +663,33 @@ static int hmm_do_fault(struct mm_struct *mm, > > fault_flags |= FAULT_FLAG_WRITE; > > } > > > > - for (; addr < end; addr += PAGE_SIZE) > > - if (handle_mm_fault(vma, addr, fault_flags, NULL) & > > - VM_FAULT_ERROR) > > - return -EFAULT; > > + for (; addr < end; addr += PAGE_SIZE) { > > + vm_fault_t ret; > > + > > + ret = handle_mm_fault(vma, addr, fault_flags, NULL); > > + > > + if (ret & (VM_FAULT_COMPLETED | VM_FAULT_RETRY)) { > > + *hmm_vma_walk->locked = 0; > > + return HMM_FAULT_UNLOCKED; > > + } > > + > > + if (ret & VM_FAULT_ERROR) { > > + int err = vm_fault_to_errno(ret, 0); > > + > > + if (err) > > + return err; > > + BUG(); > > Linux will be upset if he sees this. > > if (WARN_ON(!err)) > err = -EINVAL > It will. I copied it from GUP. I'll change it the way you propose it. > > +/** > > + * hmm_range_fault - try to fault some address in a virtual address range > > + * @range: argument structure > > + * > > + * Returns 0 on success or one of the following error codes: > > + * > > + * -EINVAL: Invalid arguments or mm or virtual address is in an > > invalid vma > > + * (e.g., device file vma). > > + * -ENOMEM: Out of memory. > > + * -EPERM: Invalid permission (e.g., asking for write and range is read > > + * only). > > + * -EBUSY: The range has been invalidated and the caller needs to wait for > > + * the invalidation to finish. > > + * -EFAULT: A page was requested to be valid and could not be made > > valid > > + * ie it has no backing VMA or it is illegal to access > > + * > > + * This is similar to get_user_pages(), except that it can read the page > > tables > > + * without mutating them (ie causing faults). > > + * > > + * The mmap lock must be held by the caller and will remain held on return. > > + * For a variant that allows the mmap lock to be dropped during faults > > (e.g., > > + * for userfaultfd support), see hmm_range_fault_unlocked_timeout(). > > + */ > > Add a comment discourging anyone from using this function and prefer > hmm_range_fault_unlocked_timeout() > Will do. Thanks, Stanislav > Other than the concern about the timeout and minor nits this looks > fine > > Jason
