On Thu, Mar 09, 2023 at 06:57:15PM +0500, Muhammad Usama Anjum wrote:
- for (addr = start; !ret && addr < end; pte++, addr += PAGE_SIZE) {
pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);is_writ = !is_pte_uffd_wp(*pte);is_file = vma->vm_file;is_pres = pte_present(*pte);is_swap = is_swap_pte(*pte);pte_unmap_unlock(pte, ptl);ret = pagemap_scan_output(is_writ, is_file, is_pres, is_swap,p, addr, 1);if (ret)break;if (PM_SCAN_OP_IS_WP(p) && is_writ &&uffd_wp_range(walk->mm, vma, addr, PAGE_SIZE, true) < 0)ret = -EINVAL;- }
This is not real atomic..
Taking the spinlock for eacy pte is not only overkill but wrong in atomicity because the pte can change right after spinlock unlocked.
Unfortunately you also cannot reuse uffd_wp_range() because that's not atomic either, my fault here. Probably I was thinking mostly from soft-dirty pov on batching the collect+reset.
You need to take the spin lock, collect whatever bits, set/clear whatever bits, only until then release the spin lock.
"Not atomic" means you can have some page got dirtied but you could miss it. Depending on how strict you want, I think it'll break apps like CRIU if strict atomicity needed for migrating a process. If we want to have a new interface anyway, IMHO we'd better do that in the strict way.
Same comment applies to the THP handling (where I cut from the context).