On Thu, Apr 11, 2024 at 06:14:41PM +0200, David Hildenbrand wrote: David, could you please clarify the below questions? > +static int __s390_unshare_zeropages(struct mm_struct *mm) > +{ > + struct vm_area_struct *vma; > + VMA_ITERATOR(vmi, mm, 0); > + unsigned long addr; > + int rc; > + > + for_each_vma(vmi, vma) { > + /* > + * We could only look at COW mappings, but it's more future > + * proof to catch unexpected zeropages in other mappings and > + * fail. > + */ > + if ((vma->vm_flags & VM_PFNMAP) || is_vm_hugetlb_page(vma)) > + continue; > + addr = vma->vm_start; > + > +retry: > + rc = walk_page_range_vma(vma, addr, vma->vm_end, > + &find_zeropage_ops, &addr); > + if (rc <= 0) > + continue; So in case an error is returned for the last vma, __s390_unshare_zeropage() finishes with that error. By contrast, the error for a non-last vma would be ignored? > + > + /* addr was updated by find_zeropage_pte_entry() */ > + rc = handle_mm_fault(vma, addr, > + FAULT_FLAG_UNSHARE | FAULT_FLAG_REMOTE, > + NULL); > + if (rc & VM_FAULT_OOM) > + return -ENOMEM; Heiko pointed out that rc type is inconsistent vs vm_fault_t returned by handle_mm_fault(). While fixing it up, I've got concerned whether is it fine to continue in case any other error is met (including possible future VM_FAULT_xxxx)? > + /* > + * See break_ksm(): even after handle_mm_fault() returned 0, we > + * must start the lookup from the current address, because > + * handle_mm_fault() may back out if there's any difficulty. > + * > + * VM_FAULT_SIGBUS and VM_FAULT_SIGSEGV are unexpected but > + * maybe they could trigger in the future on concurrent > + * truncation. In that case, the shared zeropage would be gone > + * and we can simply retry and make progress. > + */ > + cond_resched(); > + goto retry; > + } > + > + return rc; > +} Thanks!