* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
@ 2025-08-07 22:24 kernel test robot
0 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2025-08-07 22:24 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp, Dan Carpenter
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20250807103902.2242717-1-lokeshgidra@google.com>
References: <20250807103902.2242717-1-lokeshgidra@google.com>
TO: Lokesh Gidra <lokeshgidra@google.com>
Hi Lokesh,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 6e64f4580381e32c06ee146ca807c555b8f73e24]
url: https://github.com/intel-lab-lkp/linux/commits/Lokesh-Gidra/userfaultfd-opportunistic-TLB-flush-batching-for-present-pages-in-MOVE/20250807-184025
base: 6e64f4580381e32c06ee146ca807c555b8f73e24
patch link: https://lore.kernel.org/r/20250807103902.2242717-1-lokeshgidra%40google.com
patch subject: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
:::::: branch date: 12 hours ago
:::::: commit date: 12 hours ago
config: openrisc-randconfig-r071-20250808 (https://download.01.org/0day-ci/archive/20250808/202508080639.Cf7SyRwQ-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 10.5.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202508080639.Cf7SyRwQ-lkp@intel.com/
smatch warnings:
mm/userfaultfd.c:1959 move_pages() error: uninitialized symbol 'step_size'.
vim +/step_size +1959 mm/userfaultfd.c
adef440691bab82 Andrea Arcangeli 2023-12-06 1725
adef440691bab82 Andrea Arcangeli 2023-12-06 1726 /**
adef440691bab82 Andrea Arcangeli 2023-12-06 1727 * move_pages - move arbitrary anonymous pages of an existing vma
adef440691bab82 Andrea Arcangeli 2023-12-06 1728 * @ctx: pointer to the userfaultfd context
adef440691bab82 Andrea Arcangeli 2023-12-06 1729 * @dst_start: start of the destination virtual memory range
adef440691bab82 Andrea Arcangeli 2023-12-06 1730 * @src_start: start of the source virtual memory range
adef440691bab82 Andrea Arcangeli 2023-12-06 1731 * @len: length of the virtual memory range
adef440691bab82 Andrea Arcangeli 2023-12-06 1732 * @mode: flags from uffdio_move.mode
adef440691bab82 Andrea Arcangeli 2023-12-06 1733 *
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1734 * It will either use the mmap_lock in read mode or per-vma locks
adef440691bab82 Andrea Arcangeli 2023-12-06 1735 *
adef440691bab82 Andrea Arcangeli 2023-12-06 1736 * move_pages() remaps arbitrary anonymous pages atomically in zero
adef440691bab82 Andrea Arcangeli 2023-12-06 1737 * copy. It only works on non shared anonymous pages because those can
adef440691bab82 Andrea Arcangeli 2023-12-06 1738 * be relocated without generating non linear anon_vmas in the rmap
adef440691bab82 Andrea Arcangeli 2023-12-06 1739 * code.
adef440691bab82 Andrea Arcangeli 2023-12-06 1740 *
adef440691bab82 Andrea Arcangeli 2023-12-06 1741 * It provides a zero copy mechanism to handle userspace page faults.
adef440691bab82 Andrea Arcangeli 2023-12-06 1742 * The source vma pages should have mapcount == 1, which can be
adef440691bab82 Andrea Arcangeli 2023-12-06 1743 * enforced by using madvise(MADV_DONTFORK) on src vma.
adef440691bab82 Andrea Arcangeli 2023-12-06 1744 *
adef440691bab82 Andrea Arcangeli 2023-12-06 1745 * The thread receiving the page during the userland page fault
adef440691bab82 Andrea Arcangeli 2023-12-06 1746 * will receive the faulting page in the source vma through the network,
adef440691bab82 Andrea Arcangeli 2023-12-06 1747 * storage or any other I/O device (MADV_DONTFORK in the source vma
adef440691bab82 Andrea Arcangeli 2023-12-06 1748 * avoids move_pages() to fail with -EBUSY if the process forks before
adef440691bab82 Andrea Arcangeli 2023-12-06 1749 * move_pages() is called), then it will call move_pages() to map the
adef440691bab82 Andrea Arcangeli 2023-12-06 1750 * page in the faulting address in the destination vma.
adef440691bab82 Andrea Arcangeli 2023-12-06 1751 *
adef440691bab82 Andrea Arcangeli 2023-12-06 1752 * This userfaultfd command works purely via pagetables, so it's the
adef440691bab82 Andrea Arcangeli 2023-12-06 1753 * most efficient way to move physical non shared anonymous pages
adef440691bab82 Andrea Arcangeli 2023-12-06 1754 * across different virtual addresses. Unlike mremap()/mmap()/munmap()
adef440691bab82 Andrea Arcangeli 2023-12-06 1755 * it does not create any new vmas. The mapping in the destination
adef440691bab82 Andrea Arcangeli 2023-12-06 1756 * address is atomic.
adef440691bab82 Andrea Arcangeli 2023-12-06 1757 *
adef440691bab82 Andrea Arcangeli 2023-12-06 1758 * It only works if the vma protection bits are identical from the
adef440691bab82 Andrea Arcangeli 2023-12-06 1759 * source and destination vma.
adef440691bab82 Andrea Arcangeli 2023-12-06 1760 *
adef440691bab82 Andrea Arcangeli 2023-12-06 1761 * It can remap non shared anonymous pages within the same vma too.
adef440691bab82 Andrea Arcangeli 2023-12-06 1762 *
adef440691bab82 Andrea Arcangeli 2023-12-06 1763 * If the source virtual memory range has any unmapped holes, or if
adef440691bab82 Andrea Arcangeli 2023-12-06 1764 * the destination virtual memory range is not a whole unmapped hole,
adef440691bab82 Andrea Arcangeli 2023-12-06 1765 * move_pages() will fail respectively with -ENOENT or -EEXIST. This
adef440691bab82 Andrea Arcangeli 2023-12-06 1766 * provides a very strict behavior to avoid any chance of memory
adef440691bab82 Andrea Arcangeli 2023-12-06 1767 * corruption going unnoticed if there are userland race conditions.
adef440691bab82 Andrea Arcangeli 2023-12-06 1768 * Only one thread should resolve the userland page fault at any given
adef440691bab82 Andrea Arcangeli 2023-12-06 1769 * time for any given faulting address. This means that if two threads
adef440691bab82 Andrea Arcangeli 2023-12-06 1770 * try to both call move_pages() on the same destination address at the
adef440691bab82 Andrea Arcangeli 2023-12-06 1771 * same time, the second thread will get an explicit error from this
adef440691bab82 Andrea Arcangeli 2023-12-06 1772 * command.
adef440691bab82 Andrea Arcangeli 2023-12-06 1773 *
adef440691bab82 Andrea Arcangeli 2023-12-06 1774 * The command retval will return "len" is successful. The command
adef440691bab82 Andrea Arcangeli 2023-12-06 1775 * however can be interrupted by fatal signals or errors. If
adef440691bab82 Andrea Arcangeli 2023-12-06 1776 * interrupted it will return the number of bytes successfully
adef440691bab82 Andrea Arcangeli 2023-12-06 1777 * remapped before the interruption if any, or the negative error if
adef440691bab82 Andrea Arcangeli 2023-12-06 1778 * none. It will never return zero. Either it will return an error or
adef440691bab82 Andrea Arcangeli 2023-12-06 1779 * an amount of bytes successfully moved. If the retval reports a
adef440691bab82 Andrea Arcangeli 2023-12-06 1780 * "short" remap, the move_pages() command should be repeated by
adef440691bab82 Andrea Arcangeli 2023-12-06 1781 * userland with src+retval, dst+reval, len-retval if it wants to know
adef440691bab82 Andrea Arcangeli 2023-12-06 1782 * about the error that interrupted it.
adef440691bab82 Andrea Arcangeli 2023-12-06 1783 *
adef440691bab82 Andrea Arcangeli 2023-12-06 1784 * The UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES flag can be specified to
adef440691bab82 Andrea Arcangeli 2023-12-06 1785 * prevent -ENOENT errors to materialize if there are holes in the
adef440691bab82 Andrea Arcangeli 2023-12-06 1786 * source virtual range that is being remapped. The holes will be
adef440691bab82 Andrea Arcangeli 2023-12-06 1787 * accounted as successfully remapped in the retval of the
adef440691bab82 Andrea Arcangeli 2023-12-06 1788 * command. This is mostly useful to remap hugepage naturally aligned
adef440691bab82 Andrea Arcangeli 2023-12-06 1789 * virtual regions without knowing if there are transparent hugepage
adef440691bab82 Andrea Arcangeli 2023-12-06 1790 * in the regions or not, but preventing the risk of having to split
adef440691bab82 Andrea Arcangeli 2023-12-06 1791 * the hugepmd during the remap.
adef440691bab82 Andrea Arcangeli 2023-12-06 1792 *
adef440691bab82 Andrea Arcangeli 2023-12-06 1793 * If there's any rmap walk that is taking the anon_vma locks without
adef440691bab82 Andrea Arcangeli 2023-12-06 1794 * first obtaining the folio lock (the only current instance is
adef440691bab82 Andrea Arcangeli 2023-12-06 1795 * folio_referenced), they will have to verify if the folio->mapping
adef440691bab82 Andrea Arcangeli 2023-12-06 1796 * has changed after taking the anon_vma lock. If it changed they
adef440691bab82 Andrea Arcangeli 2023-12-06 1797 * should release the lock and retry obtaining a new anon_vma, because
adef440691bab82 Andrea Arcangeli 2023-12-06 1798 * it means the anon_vma was changed by move_pages() before the lock
adef440691bab82 Andrea Arcangeli 2023-12-06 1799 * could be obtained. This is the only additional complexity added to
adef440691bab82 Andrea Arcangeli 2023-12-06 1800 * the rmap code to provide this anonymous page remapping functionality.
adef440691bab82 Andrea Arcangeli 2023-12-06 1801 */
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1802 ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1803 unsigned long src_start, unsigned long len, __u64 mode)
adef440691bab82 Andrea Arcangeli 2023-12-06 1804 {
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1805 struct mm_struct *mm = ctx->mm;
adef440691bab82 Andrea Arcangeli 2023-12-06 1806 struct vm_area_struct *src_vma, *dst_vma;
3abf06e0c484867 Lokesh Gidra 2025-08-07 1807 unsigned long src_addr, dst_addr, src_end;
adef440691bab82 Andrea Arcangeli 2023-12-06 1808 pmd_t *src_pmd, *dst_pmd;
adef440691bab82 Andrea Arcangeli 2023-12-06 1809 long err = -EINVAL;
adef440691bab82 Andrea Arcangeli 2023-12-06 1810 ssize_t moved = 0;
adef440691bab82 Andrea Arcangeli 2023-12-06 1811
adef440691bab82 Andrea Arcangeli 2023-12-06 1812 /* Sanitize the command parameters. */
31defc3b01d907e Tal Zussman 2025-06-19 1813 VM_WARN_ON_ONCE(src_start & ~PAGE_MASK);
31defc3b01d907e Tal Zussman 2025-06-19 1814 VM_WARN_ON_ONCE(dst_start & ~PAGE_MASK);
31defc3b01d907e Tal Zussman 2025-06-19 1815 VM_WARN_ON_ONCE(len & ~PAGE_MASK);
adef440691bab82 Andrea Arcangeli 2023-12-06 1816
adef440691bab82 Andrea Arcangeli 2023-12-06 1817 /* Does the address range wrap, or is the span zero-sized? */
31defc3b01d907e Tal Zussman 2025-06-19 1818 VM_WARN_ON_ONCE(src_start + len < src_start);
31defc3b01d907e Tal Zussman 2025-06-19 1819 VM_WARN_ON_ONCE(dst_start + len < dst_start);
adef440691bab82 Andrea Arcangeli 2023-12-06 1820
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1821 err = uffd_move_lock(mm, dst_start, src_start, &dst_vma, &src_vma);
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1822 if (err)
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1823 goto out;
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1824
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1825 /* Re-check after taking map_changing_lock */
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1826 err = -EAGAIN;
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1827 down_read(&ctx->map_changing_lock);
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1828 if (likely(atomic_read(&ctx->mmap_changing)))
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1829 goto out_unlock;
adef440691bab82 Andrea Arcangeli 2023-12-06 1830 /*
adef440691bab82 Andrea Arcangeli 2023-12-06 1831 * Make sure the vma is not shared, that the src and dst remap
adef440691bab82 Andrea Arcangeli 2023-12-06 1832 * ranges are both valid and fully within a single existing
adef440691bab82 Andrea Arcangeli 2023-12-06 1833 * vma.
adef440691bab82 Andrea Arcangeli 2023-12-06 1834 */
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1835 err = -EINVAL;
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1836 if (src_vma->vm_flags & VM_SHARED)
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1837 goto out_unlock;
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1838 if (src_start + len > src_vma->vm_end)
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1839 goto out_unlock;
adef440691bab82 Andrea Arcangeli 2023-12-06 1840
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1841 if (dst_vma->vm_flags & VM_SHARED)
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1842 goto out_unlock;
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1843 if (dst_start + len > dst_vma->vm_end)
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1844 goto out_unlock;
adef440691bab82 Andrea Arcangeli 2023-12-06 1845
adef440691bab82 Andrea Arcangeli 2023-12-06 1846 err = validate_move_areas(ctx, src_vma, dst_vma);
adef440691bab82 Andrea Arcangeli 2023-12-06 1847 if (err)
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1848 goto out_unlock;
adef440691bab82 Andrea Arcangeli 2023-12-06 1849
3abf06e0c484867 Lokesh Gidra 2025-08-07 1850 for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
3abf06e0c484867 Lokesh Gidra 2025-08-07 1851 src_addr < src_end;) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1852 spinlock_t *ptl;
adef440691bab82 Andrea Arcangeli 2023-12-06 1853 pmd_t dst_pmdval;
adef440691bab82 Andrea Arcangeli 2023-12-06 1854 unsigned long step_size;
adef440691bab82 Andrea Arcangeli 2023-12-06 1855
adef440691bab82 Andrea Arcangeli 2023-12-06 1856 /*
adef440691bab82 Andrea Arcangeli 2023-12-06 1857 * Below works because anonymous area would not have a
adef440691bab82 Andrea Arcangeli 2023-12-06 1858 * transparent huge PUD. If file-backed support is added,
adef440691bab82 Andrea Arcangeli 2023-12-06 1859 * that case would need to be handled here.
adef440691bab82 Andrea Arcangeli 2023-12-06 1860 */
adef440691bab82 Andrea Arcangeli 2023-12-06 1861 src_pmd = mm_find_pmd(mm, src_addr);
adef440691bab82 Andrea Arcangeli 2023-12-06 1862 if (unlikely(!src_pmd)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1863 if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1864 err = -ENOENT;
adef440691bab82 Andrea Arcangeli 2023-12-06 1865 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1866 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1867 src_pmd = mm_alloc_pmd(mm, src_addr);
adef440691bab82 Andrea Arcangeli 2023-12-06 1868 if (unlikely(!src_pmd)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1869 err = -ENOMEM;
adef440691bab82 Andrea Arcangeli 2023-12-06 1870 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1871 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1872 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1873 dst_pmd = mm_alloc_pmd(mm, dst_addr);
adef440691bab82 Andrea Arcangeli 2023-12-06 1874 if (unlikely(!dst_pmd)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1875 err = -ENOMEM;
adef440691bab82 Andrea Arcangeli 2023-12-06 1876 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1877 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1878
adef440691bab82 Andrea Arcangeli 2023-12-06 1879 dst_pmdval = pmdp_get_lockless(dst_pmd);
adef440691bab82 Andrea Arcangeli 2023-12-06 1880 /*
adef440691bab82 Andrea Arcangeli 2023-12-06 1881 * If the dst_pmd is mapped as THP don't override it and just
adef440691bab82 Andrea Arcangeli 2023-12-06 1882 * be strict. If dst_pmd changes into TPH after this check, the
adef440691bab82 Andrea Arcangeli 2023-12-06 1883 * move_pages_huge_pmd() will detect the change and retry
adef440691bab82 Andrea Arcangeli 2023-12-06 1884 * while move_pages_pte() will detect the change and fail.
adef440691bab82 Andrea Arcangeli 2023-12-06 1885 */
adef440691bab82 Andrea Arcangeli 2023-12-06 1886 if (unlikely(pmd_trans_huge(dst_pmdval))) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1887 err = -EEXIST;
adef440691bab82 Andrea Arcangeli 2023-12-06 1888 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1889 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1890
adef440691bab82 Andrea Arcangeli 2023-12-06 1891 ptl = pmd_trans_huge_lock(src_pmd, src_vma);
adef440691bab82 Andrea Arcangeli 2023-12-06 1892 if (ptl) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1893 /* Check if we can move the pmd without splitting it. */
adef440691bab82 Andrea Arcangeli 2023-12-06 1894 if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
adef440691bab82 Andrea Arcangeli 2023-12-06 1895 !pmd_none(dst_pmdval)) {
e06d03d5590ae1c Matthew Wilcox (Oracle 2024-03-26 1896) struct folio *folio = pmd_folio(*src_pmd);
adef440691bab82 Andrea Arcangeli 2023-12-06 1897
5beaee54a324ba1 Matthew Wilcox (Oracle 2024-03-26 1898) if (!folio || (!is_huge_zero_folio(folio) &&
eb1521dad8f391d Suren Baghdasaryan 2024-01-31 1899 !PageAnonExclusive(&folio->page))) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1900 spin_unlock(ptl);
adef440691bab82 Andrea Arcangeli 2023-12-06 1901 err = -EBUSY;
adef440691bab82 Andrea Arcangeli 2023-12-06 1902 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1903 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1904
adef440691bab82 Andrea Arcangeli 2023-12-06 1905 spin_unlock(ptl);
adef440691bab82 Andrea Arcangeli 2023-12-06 1906 split_huge_pmd(src_vma, src_pmd, src_addr);
adef440691bab82 Andrea Arcangeli 2023-12-06 1907 /* The folio will be split by move_pages_pte() */
adef440691bab82 Andrea Arcangeli 2023-12-06 1908 continue;
adef440691bab82 Andrea Arcangeli 2023-12-06 1909 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1910
adef440691bab82 Andrea Arcangeli 2023-12-06 1911 err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
adef440691bab82 Andrea Arcangeli 2023-12-06 1912 dst_pmdval, dst_vma, src_vma,
adef440691bab82 Andrea Arcangeli 2023-12-06 1913 dst_addr, src_addr);
adef440691bab82 Andrea Arcangeli 2023-12-06 1914 step_size = HPAGE_PMD_SIZE;
adef440691bab82 Andrea Arcangeli 2023-12-06 1915 } else {
3abf06e0c484867 Lokesh Gidra 2025-08-07 1916 long ret;
3abf06e0c484867 Lokesh Gidra 2025-08-07 1917
adef440691bab82 Andrea Arcangeli 2023-12-06 1918 if (pmd_none(*src_pmd)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1919 if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1920 err = -ENOENT;
adef440691bab82 Andrea Arcangeli 2023-12-06 1921 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1922 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1923 if (unlikely(__pte_alloc(mm, src_pmd))) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1924 err = -ENOMEM;
adef440691bab82 Andrea Arcangeli 2023-12-06 1925 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1926 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1927 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1928
adef440691bab82 Andrea Arcangeli 2023-12-06 1929 if (unlikely(pte_alloc(mm, dst_pmd))) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1930 err = -ENOMEM;
adef440691bab82 Andrea Arcangeli 2023-12-06 1931 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1932 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1933
3abf06e0c484867 Lokesh Gidra 2025-08-07 1934 ret = move_pages_ptes(mm, dst_pmd, src_pmd,
3abf06e0c484867 Lokesh Gidra 2025-08-07 1935 dst_vma, src_vma, dst_addr,
3abf06e0c484867 Lokesh Gidra 2025-08-07 1936 src_addr, src_end - src_addr, mode);
3abf06e0c484867 Lokesh Gidra 2025-08-07 1937 if (ret > 0)
3abf06e0c484867 Lokesh Gidra 2025-08-07 1938 step_size = ret;
3abf06e0c484867 Lokesh Gidra 2025-08-07 1939 else
3abf06e0c484867 Lokesh Gidra 2025-08-07 1940 err = ret;
adef440691bab82 Andrea Arcangeli 2023-12-06 1941 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1942
adef440691bab82 Andrea Arcangeli 2023-12-06 1943 cond_resched();
adef440691bab82 Andrea Arcangeli 2023-12-06 1944
adef440691bab82 Andrea Arcangeli 2023-12-06 1945 if (fatal_signal_pending(current)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1946 /* Do not override an error */
adef440691bab82 Andrea Arcangeli 2023-12-06 1947 if (!err || err == -EAGAIN)
adef440691bab82 Andrea Arcangeli 2023-12-06 1948 err = -EINTR;
adef440691bab82 Andrea Arcangeli 2023-12-06 1949 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1950 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1951
adef440691bab82 Andrea Arcangeli 2023-12-06 1952 if (err) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1953 if (err == -EAGAIN)
adef440691bab82 Andrea Arcangeli 2023-12-06 1954 continue;
adef440691bab82 Andrea Arcangeli 2023-12-06 1955 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1956 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1957
adef440691bab82 Andrea Arcangeli 2023-12-06 1958 /* Proceed to the next page */
adef440691bab82 Andrea Arcangeli 2023-12-06 @1959 dst_addr += step_size;
adef440691bab82 Andrea Arcangeli 2023-12-06 1960 src_addr += step_size;
adef440691bab82 Andrea Arcangeli 2023-12-06 1961 moved += step_size;
adef440691bab82 Andrea Arcangeli 2023-12-06 1962 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1963
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1964 out_unlock:
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1965 up_read(&ctx->map_changing_lock);
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1966 uffd_move_unlock(dst_vma, src_vma);
adef440691bab82 Andrea Arcangeli 2023-12-06 1967 out:
31defc3b01d907e Tal Zussman 2025-06-19 1968 VM_WARN_ON_ONCE(moved < 0);
31defc3b01d907e Tal Zussman 2025-06-19 1969 VM_WARN_ON_ONCE(err > 0);
31defc3b01d907e Tal Zussman 2025-06-19 1970 VM_WARN_ON_ONCE(!moved && !err);
adef440691bab82 Andrea Arcangeli 2023-12-06 1971 return moved ? moved : err;
adef440691bab82 Andrea Arcangeli 2023-12-06 1972 }
a17c7d8fd2b097a Lorenzo Stoakes 2024-07-29 1973
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
@ 2025-08-07 10:39 Lokesh Gidra
2025-08-07 19:16 ` Peter Xu
2025-08-08 6:18 ` Dan Carpenter
0 siblings, 2 replies; 11+ messages in thread
From: Lokesh Gidra @ 2025-08-07 10:39 UTC (permalink / raw)
To: akpm
Cc: aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
Lokesh Gidra, Suren Baghdasaryan, Kalesh Singh, Barry Song,
David Hildenbrand, Peter Xu
MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
for moving present pages. Mitigate this cost by opportunistically
batching present contiguous pages for TLB flushing.
Without batching, in our testing on an arm64 Android device with UFFD GC,
which uses MOVE ioctl for compaction, we observed that out of the total
time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
~20% in vm_normal_folio().
With batching, the proportion of vm_normal_folio() increases to over
70% of move_pages_pte() without any changes to vm_normal_folio().
Furthermore, time spent within move_pages_pte() is only ~20%, which
includes TLB-flush overhead.
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Kalesh Singh <kaleshsingh@google.com>
Cc: Barry Song <v-songbaohua@oppo.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
---
Changes since v2 [1]
- Addressed VM_WARN_ON failure, per Lorenzo Stoakes
- Added check to ensure all batched pages share the same anon_vma
Changes since v1 [2]
- Removed flush_tlb_batched_pending(), per Barry Song
- Unified single and multi page case, per Barry Song
[1] https://lore.kernel.org/all/20250805121410.1658418-1-lokeshgidra@google.com/
[2] https://lore.kernel.org/all/20250731104726.103071-1-lokeshgidra@google.com/
mm/userfaultfd.c | 179 +++++++++++++++++++++++++++++++++--------------
1 file changed, 128 insertions(+), 51 deletions(-)
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index cbed91b09640..78c732100aec 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -1026,18 +1026,64 @@ static inline bool is_pte_pages_stable(pte_t *dst_pte, pte_t *src_pte,
pmd_same(dst_pmdval, pmdp_get_lockless(dst_pmd));
}
-static int move_present_pte(struct mm_struct *mm,
- struct vm_area_struct *dst_vma,
- struct vm_area_struct *src_vma,
- unsigned long dst_addr, unsigned long src_addr,
- pte_t *dst_pte, pte_t *src_pte,
- pte_t orig_dst_pte, pte_t orig_src_pte,
- pmd_t *dst_pmd, pmd_t dst_pmdval,
- spinlock_t *dst_ptl, spinlock_t *src_ptl,
- struct folio *src_folio)
+/*
+ * Checks if the two ptes and the corresponding folio are eligible for batched
+ * move. If so, then returns pointer to the folio, after locking it. Otherwise,
+ * returns NULL.
+ */
+static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
+ unsigned long src_addr,
+ pte_t *src_pte, pte_t *dst_pte,
+ struct anon_vma *src_anon_vma)
+{
+ pte_t orig_dst_pte, orig_src_pte;
+ struct folio *folio;
+
+ orig_dst_pte = ptep_get(dst_pte);
+ if (!pte_none(orig_dst_pte))
+ return NULL;
+
+ orig_src_pte = ptep_get(src_pte);
+ if (pte_none(orig_src_pte) || !pte_present(orig_src_pte) ||
+ is_zero_pfn(pte_pfn(orig_src_pte)))
+ return NULL;
+
+ folio = vm_normal_folio(src_vma, src_addr, orig_src_pte);
+ if (!folio || !folio_trylock(folio))
+ return NULL;
+ if (!PageAnonExclusive(&folio->page) || folio_test_large(folio) ||
+ folio_anon_vma(folio) != src_anon_vma) {
+ folio_unlock(folio);
+ return NULL;
+ }
+ return folio;
+}
+
+static long move_present_ptes(struct mm_struct *mm,
+ struct vm_area_struct *dst_vma,
+ struct vm_area_struct *src_vma,
+ unsigned long dst_addr, unsigned long src_addr,
+ pte_t *dst_pte, pte_t *src_pte,
+ pte_t orig_dst_pte, pte_t orig_src_pte,
+ pmd_t *dst_pmd, pmd_t dst_pmdval,
+ spinlock_t *dst_ptl, spinlock_t *src_ptl,
+ struct folio *src_folio, unsigned long len,
+ struct anon_vma *src_anon_vma)
{
int err = 0;
+ unsigned long src_start = src_addr;
+ unsigned long addr_end;
+ if (len > PAGE_SIZE) {
+ addr_end = (dst_addr + PMD_SIZE) & PMD_MASK;
+ if (dst_addr + len > addr_end)
+ len = addr_end - dst_addr;
+
+ addr_end = (src_addr + PMD_SIZE) & PMD_MASK;
+ if (src_addr + len > addr_end)
+ len = addr_end - src_addr;
+ }
+ flush_cache_range(src_vma, src_addr, src_addr + len);
double_pt_lock(dst_ptl, src_ptl);
if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
@@ -1051,31 +1097,54 @@ static int move_present_pte(struct mm_struct *mm,
err = -EBUSY;
goto out;
}
+ arch_enter_lazy_mmu_mode();
+
+ addr_end = src_start + len;
+ while (true) {
+ orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
+ /* Folio got pinned from under us. Put it back and fail the move. */
+ if (folio_maybe_dma_pinned(src_folio)) {
+ set_pte_at(mm, src_addr, src_pte, orig_src_pte);
+ err = -EBUSY;
+ break;
+ }
- orig_src_pte = ptep_clear_flush(src_vma, src_addr, src_pte);
- /* Folio got pinned from under us. Put it back and fail the move. */
- if (folio_maybe_dma_pinned(src_folio)) {
- set_pte_at(mm, src_addr, src_pte, orig_src_pte);
- err = -EBUSY;
- goto out;
- }
-
- folio_move_anon_rmap(src_folio, dst_vma);
- src_folio->index = linear_page_index(dst_vma, dst_addr);
+ folio_move_anon_rmap(src_folio, dst_vma);
+ src_folio->index = linear_page_index(dst_vma, dst_addr);
- orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
- /* Set soft dirty bit so userspace can notice the pte was moved */
+ orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
+ /* Set soft dirty bit so userspace can notice the pte was moved */
#ifdef CONFIG_MEM_SOFT_DIRTY
- orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
+ orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
#endif
- if (pte_dirty(orig_src_pte))
- orig_dst_pte = pte_mkdirty(orig_dst_pte);
- orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
+ if (pte_dirty(orig_src_pte))
+ orig_dst_pte = pte_mkdirty(orig_dst_pte);
+ orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
+ set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
+
+ src_addr += PAGE_SIZE;
+ if (src_addr == addr_end)
+ break;
+ src_pte++;
+ dst_pte++;
+
+ folio_unlock(src_folio);
+ src_folio = check_ptes_for_batched_move(src_vma, src_addr, src_pte,
+ dst_pte, src_anon_vma);
+ if (!src_folio)
+ break;
+ dst_addr += PAGE_SIZE;
+ }
+
+ arch_leave_lazy_mmu_mode();
+ if (src_addr > src_start)
+ flush_tlb_range(src_vma, src_start, src_addr);
- set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
out:
double_pt_unlock(dst_ptl, src_ptl);
- return err;
+ if (src_folio)
+ folio_unlock(src_folio);
+ return src_addr > src_start ? src_addr - src_start : err;
}
static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
@@ -1140,7 +1209,7 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
set_pte_at(mm, dst_addr, dst_pte, orig_src_pte);
double_pt_unlock(dst_ptl, src_ptl);
- return 0;
+ return PAGE_SIZE;
}
static int move_zeropage_pte(struct mm_struct *mm,
@@ -1154,6 +1223,7 @@ static int move_zeropage_pte(struct mm_struct *mm,
{
pte_t zero_pte;
+ flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
double_pt_lock(dst_ptl, src_ptl);
if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
dst_pmd, dst_pmdval)) {
@@ -1167,20 +1237,19 @@ static int move_zeropage_pte(struct mm_struct *mm,
set_pte_at(mm, dst_addr, dst_pte, zero_pte);
double_pt_unlock(dst_ptl, src_ptl);
- return 0;
+ return PAGE_SIZE;
}
/*
- * The mmap_lock for reading is held by the caller. Just move the page
- * from src_pmd to dst_pmd if possible, and return true if succeeded
- * in moving the page.
+ * The mmap_lock for reading is held by the caller. Just move the page(s)
+ * from src_pmd to dst_pmd if possible, and return number of bytes moved.
*/
-static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
- struct vm_area_struct *dst_vma,
- struct vm_area_struct *src_vma,
- unsigned long dst_addr, unsigned long src_addr,
- __u64 mode)
+static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
+ struct vm_area_struct *dst_vma,
+ struct vm_area_struct *src_vma,
+ unsigned long dst_addr, unsigned long src_addr,
+ unsigned long len, __u64 mode)
{
swp_entry_t entry;
struct swap_info_struct *si = NULL;
@@ -1196,9 +1265,8 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
struct mmu_notifier_range range;
int err = 0;
- flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm,
- src_addr, src_addr + PAGE_SIZE);
+ src_addr, src_addr + len);
mmu_notifier_invalidate_range_start(&range);
retry:
/*
@@ -1257,7 +1325,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES))
err = -ENOENT;
else /* nothing to do to move a hole */
- err = 0;
+ err = PAGE_SIZE;
goto out;
}
@@ -1375,10 +1443,14 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
}
}
- err = move_present_pte(mm, dst_vma, src_vma,
- dst_addr, src_addr, dst_pte, src_pte,
- orig_dst_pte, orig_src_pte, dst_pmd,
- dst_pmdval, dst_ptl, src_ptl, src_folio);
+ err = move_present_ptes(mm, dst_vma, src_vma,
+ dst_addr, src_addr, dst_pte, src_pte,
+ orig_dst_pte, orig_src_pte, dst_pmd,
+ dst_pmdval, dst_ptl, src_ptl, src_folio,
+ len, src_anon_vma);
+ /* folio is already unlocked by move_present_ptes() */
+ folio_put(src_folio);
+ src_folio = NULL;
} else {
struct folio *folio = NULL;
@@ -1732,7 +1804,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
{
struct mm_struct *mm = ctx->mm;
struct vm_area_struct *src_vma, *dst_vma;
- unsigned long src_addr, dst_addr;
+ unsigned long src_addr, dst_addr, src_end;
pmd_t *src_pmd, *dst_pmd;
long err = -EINVAL;
ssize_t moved = 0;
@@ -1775,8 +1847,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
if (err)
goto out_unlock;
- for (src_addr = src_start, dst_addr = dst_start;
- src_addr < src_start + len;) {
+ for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
+ src_addr < src_end;) {
spinlock_t *ptl;
pmd_t dst_pmdval;
unsigned long step_size;
@@ -1841,6 +1913,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
dst_addr, src_addr);
step_size = HPAGE_PMD_SIZE;
} else {
+ long ret;
+
if (pmd_none(*src_pmd)) {
if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
err = -ENOENT;
@@ -1857,10 +1931,13 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
break;
}
- err = move_pages_pte(mm, dst_pmd, src_pmd,
- dst_vma, src_vma,
- dst_addr, src_addr, mode);
- step_size = PAGE_SIZE;
+ ret = move_pages_ptes(mm, dst_pmd, src_pmd,
+ dst_vma, src_vma, dst_addr,
+ src_addr, src_end - src_addr, mode);
+ if (ret > 0)
+ step_size = ret;
+ else
+ err = ret;
}
cond_resched();
base-commit: 6e64f4580381e32c06ee146ca807c555b8f73e24
--
2.50.1.565.gc32cd1483b-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
2025-08-07 10:39 Lokesh Gidra
@ 2025-08-07 19:16 ` Peter Xu
2025-08-07 22:54 ` Andrew Morton
2025-08-08 16:29 ` Lokesh Gidra
2025-08-08 6:18 ` Dan Carpenter
1 sibling, 2 replies; 11+ messages in thread
From: Peter Xu @ 2025-08-07 19:16 UTC (permalink / raw)
To: Lokesh Gidra
Cc: akpm, aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
Suren Baghdasaryan, Kalesh Singh, Barry Song, David Hildenbrand
Hi, Lokesh,
On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> for moving present pages. Mitigate this cost by opportunistically
> batching present contiguous pages for TLB flushing.
>
> Without batching, in our testing on an arm64 Android device with UFFD GC,
> which uses MOVE ioctl for compaction, we observed that out of the total
> time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> ~20% in vm_normal_folio().
>
> With batching, the proportion of vm_normal_folio() increases to over
> 70% of move_pages_pte() without any changes to vm_normal_folio().
Do you know why vm_normal_folio() could be expensive? I still see quite
some other things this path needs to do.
> Furthermore, time spent within move_pages_pte() is only ~20%, which
> includes TLB-flush overhead.
Indeed this should already prove the optimization, I'm just curious whether
you've run some benchmark on the GC app to show the real world benefit.
>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Kalesh Singh <kaleshsingh@google.com>
> Cc: Barry Song <v-songbaohua@oppo.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> ---
> Changes since v2 [1]
> - Addressed VM_WARN_ON failure, per Lorenzo Stoakes
> - Added check to ensure all batched pages share the same anon_vma
>
> Changes since v1 [2]
> - Removed flush_tlb_batched_pending(), per Barry Song
> - Unified single and multi page case, per Barry Song
>
> [1] https://lore.kernel.org/all/20250805121410.1658418-1-lokeshgidra@google.com/
> [2] https://lore.kernel.org/all/20250731104726.103071-1-lokeshgidra@google.com/
>
> mm/userfaultfd.c | 179 +++++++++++++++++++++++++++++++++--------------
> 1 file changed, 128 insertions(+), 51 deletions(-)
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index cbed91b09640..78c732100aec 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1026,18 +1026,64 @@ static inline bool is_pte_pages_stable(pte_t *dst_pte, pte_t *src_pte,
> pmd_same(dst_pmdval, pmdp_get_lockless(dst_pmd));
> }
>
> -static int move_present_pte(struct mm_struct *mm,
> - struct vm_area_struct *dst_vma,
> - struct vm_area_struct *src_vma,
> - unsigned long dst_addr, unsigned long src_addr,
> - pte_t *dst_pte, pte_t *src_pte,
> - pte_t orig_dst_pte, pte_t orig_src_pte,
> - pmd_t *dst_pmd, pmd_t dst_pmdval,
> - spinlock_t *dst_ptl, spinlock_t *src_ptl,
> - struct folio *src_folio)
> +/*
> + * Checks if the two ptes and the corresponding folio are eligible for batched
> + * move. If so, then returns pointer to the folio, after locking it. Otherwise,
> + * returns NULL.
> + */
> +static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
> + unsigned long src_addr,
> + pte_t *src_pte, pte_t *dst_pte,
> + struct anon_vma *src_anon_vma)
> +{
> + pte_t orig_dst_pte, orig_src_pte;
> + struct folio *folio;
> +
> + orig_dst_pte = ptep_get(dst_pte);
> + if (!pte_none(orig_dst_pte))
> + return NULL;
> +
> + orig_src_pte = ptep_get(src_pte);
> + if (pte_none(orig_src_pte) || !pte_present(orig_src_pte) ||
pte_none() check could be removed - the pte_present() check should make
sure it's !none.
> + is_zero_pfn(pte_pfn(orig_src_pte)))
> + return NULL;
> +
> + folio = vm_normal_folio(src_vma, src_addr, orig_src_pte);
> + if (!folio || !folio_trylock(folio))
> + return NULL;
So here we don't take a refcount anymore, while the 1st folio that got
passed in will still has the refcount boosted. IMHO it would still be
better to keep the behavior the same on the 1st and continuous folios..
Or if this is intentional, maybe worth some comment. More below on this..
> + if (!PageAnonExclusive(&folio->page) || folio_test_large(folio) ||
> + folio_anon_vma(folio) != src_anon_vma) {
> + folio_unlock(folio);
> + return NULL;
> + }
> + return folio;
> +}
> +
> +static long move_present_ptes(struct mm_struct *mm,
> + struct vm_area_struct *dst_vma,
> + struct vm_area_struct *src_vma,
> + unsigned long dst_addr, unsigned long src_addr,
> + pte_t *dst_pte, pte_t *src_pte,
> + pte_t orig_dst_pte, pte_t orig_src_pte,
> + pmd_t *dst_pmd, pmd_t dst_pmdval,
> + spinlock_t *dst_ptl, spinlock_t *src_ptl,
> + struct folio *src_folio, unsigned long len,
> + struct anon_vma *src_anon_vma)
(Not an immediate concern, but this function has potential to win the
max-num-of-parameters kernel function.. :)
> {
> int err = 0;
> + unsigned long src_start = src_addr;
> + unsigned long addr_end;
>
> + if (len > PAGE_SIZE) {
> + addr_end = (dst_addr + PMD_SIZE) & PMD_MASK;
> + if (dst_addr + len > addr_end)
> + len = addr_end - dst_addr;
Use something like ALIGN() and MIN()?
> +
> + addr_end = (src_addr + PMD_SIZE) & PMD_MASK;
> + if (src_addr + len > addr_end)
> + len = addr_end - src_addr;
Same here.
> + }
> + flush_cache_range(src_vma, src_addr, src_addr + len);
> double_pt_lock(dst_ptl, src_ptl);
>
> if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> @@ -1051,31 +1097,54 @@ static int move_present_pte(struct mm_struct *mm,
> err = -EBUSY;
> goto out;
> }
> + arch_enter_lazy_mmu_mode();
> +
> + addr_end = src_start + len;
> + while (true) {
> + orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
> + /* Folio got pinned from under us. Put it back and fail the move. */
> + if (folio_maybe_dma_pinned(src_folio)) {
> + set_pte_at(mm, src_addr, src_pte, orig_src_pte);
> + err = -EBUSY;
> + break;
> + }
>
> - orig_src_pte = ptep_clear_flush(src_vma, src_addr, src_pte);
> - /* Folio got pinned from under us. Put it back and fail the move. */
> - if (folio_maybe_dma_pinned(src_folio)) {
> - set_pte_at(mm, src_addr, src_pte, orig_src_pte);
> - err = -EBUSY;
> - goto out;
> - }
> -
> - folio_move_anon_rmap(src_folio, dst_vma);
> - src_folio->index = linear_page_index(dst_vma, dst_addr);
> + folio_move_anon_rmap(src_folio, dst_vma);
> + src_folio->index = linear_page_index(dst_vma, dst_addr);
>
> - orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
> - /* Set soft dirty bit so userspace can notice the pte was moved */
> + orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
> + /* Set soft dirty bit so userspace can notice the pte was moved */
> #ifdef CONFIG_MEM_SOFT_DIRTY
> - orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
> + orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
> #endif
> - if (pte_dirty(orig_src_pte))
> - orig_dst_pte = pte_mkdirty(orig_dst_pte);
> - orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> + if (pte_dirty(orig_src_pte))
> + orig_dst_pte = pte_mkdirty(orig_dst_pte);
> + orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> + set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
> +
> + src_addr += PAGE_SIZE;
> + if (src_addr == addr_end)
> + break;
> + src_pte++;
> + dst_pte++;
> +
> + folio_unlock(src_folio);
> + src_folio = check_ptes_for_batched_move(src_vma, src_addr, src_pte,
> + dst_pte, src_anon_vma);
> + if (!src_folio)
> + break;
> + dst_addr += PAGE_SIZE;
> + }
> +
> + arch_leave_lazy_mmu_mode();
> + if (src_addr > src_start)
> + flush_tlb_range(src_vma, src_start, src_addr);
>
> - set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
> out:
> double_pt_unlock(dst_ptl, src_ptl);
> - return err;
> + if (src_folio)
> + folio_unlock(src_folio);
> + return src_addr > src_start ? src_addr - src_start : err;
> }
>
> static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> @@ -1140,7 +1209,7 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> set_pte_at(mm, dst_addr, dst_pte, orig_src_pte);
> double_pt_unlock(dst_ptl, src_ptl);
>
> - return 0;
> + return PAGE_SIZE;
> }
>
> static int move_zeropage_pte(struct mm_struct *mm,
> @@ -1154,6 +1223,7 @@ static int move_zeropage_pte(struct mm_struct *mm,
> {
> pte_t zero_pte;
>
> + flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
If it's a zero page hence not writtable, do we still need to flush cache at
all? Looks harmless, but looks like not needed either.
> double_pt_lock(dst_ptl, src_ptl);
> if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> dst_pmd, dst_pmdval)) {
> @@ -1167,20 +1237,19 @@ static int move_zeropage_pte(struct mm_struct *mm,
> set_pte_at(mm, dst_addr, dst_pte, zero_pte);
> double_pt_unlock(dst_ptl, src_ptl);
>
> - return 0;
> + return PAGE_SIZE;
> }
>
>
> /*
> - * The mmap_lock for reading is held by the caller. Just move the page
> - * from src_pmd to dst_pmd if possible, and return true if succeeded
> - * in moving the page.
> + * The mmap_lock for reading is held by the caller. Just move the page(s)
> + * from src_pmd to dst_pmd if possible, and return number of bytes moved.
> */
> -static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> - struct vm_area_struct *dst_vma,
> - struct vm_area_struct *src_vma,
> - unsigned long dst_addr, unsigned long src_addr,
> - __u64 mode)
> +static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> + struct vm_area_struct *dst_vma,
> + struct vm_area_struct *src_vma,
> + unsigned long dst_addr, unsigned long src_addr,
> + unsigned long len, __u64 mode)
> {
> swp_entry_t entry;
> struct swap_info_struct *si = NULL;
> @@ -1196,9 +1265,8 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> struct mmu_notifier_range range;
> int err = 0;
>
> - flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
> mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm,
> - src_addr, src_addr + PAGE_SIZE);
> + src_addr, src_addr + len);
> mmu_notifier_invalidate_range_start(&range);
> retry:
> /*
> @@ -1257,7 +1325,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES))
> err = -ENOENT;
> else /* nothing to do to move a hole */
> - err = 0;
> + err = PAGE_SIZE;
> goto out;
> }
>
> @@ -1375,10 +1443,14 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> }
> }
>
> - err = move_present_pte(mm, dst_vma, src_vma,
> - dst_addr, src_addr, dst_pte, src_pte,
> - orig_dst_pte, orig_src_pte, dst_pmd,
> - dst_pmdval, dst_ptl, src_ptl, src_folio);
> + err = move_present_ptes(mm, dst_vma, src_vma,
> + dst_addr, src_addr, dst_pte, src_pte,
> + orig_dst_pte, orig_src_pte, dst_pmd,
> + dst_pmdval, dst_ptl, src_ptl, src_folio,
> + len, src_anon_vma);
> + /* folio is already unlocked by move_present_ptes() */
> + folio_put(src_folio);
> + src_folio = NULL;
So the function above now can move multiple folios but keep holding the
1st's refcount.. This still smells error prone, sooner or later.
Would it be slightly better if we take a folio pointer in
move_present_ptes(), and releae everything there (including reset the
pointer)?
Thanks,
> } else {
> struct folio *folio = NULL;
>
> @@ -1732,7 +1804,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> {
> struct mm_struct *mm = ctx->mm;
> struct vm_area_struct *src_vma, *dst_vma;
> - unsigned long src_addr, dst_addr;
> + unsigned long src_addr, dst_addr, src_end;
> pmd_t *src_pmd, *dst_pmd;
> long err = -EINVAL;
> ssize_t moved = 0;
> @@ -1775,8 +1847,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> if (err)
> goto out_unlock;
>
> - for (src_addr = src_start, dst_addr = dst_start;
> - src_addr < src_start + len;) {
> + for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
> + src_addr < src_end;) {
> spinlock_t *ptl;
> pmd_t dst_pmdval;
> unsigned long step_size;
> @@ -1841,6 +1913,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> dst_addr, src_addr);
> step_size = HPAGE_PMD_SIZE;
> } else {
> + long ret;
> +
> if (pmd_none(*src_pmd)) {
> if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> err = -ENOENT;
> @@ -1857,10 +1931,13 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> break;
> }
>
> - err = move_pages_pte(mm, dst_pmd, src_pmd,
> - dst_vma, src_vma,
> - dst_addr, src_addr, mode);
> - step_size = PAGE_SIZE;
> + ret = move_pages_ptes(mm, dst_pmd, src_pmd,
> + dst_vma, src_vma, dst_addr,
> + src_addr, src_end - src_addr, mode);
> + if (ret > 0)
> + step_size = ret;
> + else
> + err = ret;
> }
>
> cond_resched();
>
> base-commit: 6e64f4580381e32c06ee146ca807c555b8f73e24
> --
> 2.50.1.565.gc32cd1483b-goog
>
--
Peter Xu
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
2025-08-07 19:16 ` Peter Xu
@ 2025-08-07 22:54 ` Andrew Morton
2025-08-08 16:41 ` Lokesh Gidra
2025-08-08 16:29 ` Lokesh Gidra
1 sibling, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2025-08-07 22:54 UTC (permalink / raw)
To: Peter Xu
Cc: Lokesh Gidra, aarcange, linux-mm, linux-kernel, 21cnbao,
ngeoffray, Suren Baghdasaryan, Kalesh Singh, Barry Song,
David Hildenbrand
On Thu, 7 Aug 2025 15:16:57 -0400 Peter Xu <peterx@redhat.com> wrote:
> Hi, Lokesh,
>
> On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> > MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> > for moving present pages. Mitigate this cost by opportunistically
> > batching present contiguous pages for TLB flushing.
> >
> > Without batching, in our testing on an arm64 Android device with UFFD GC,
> > which uses MOVE ioctl for compaction, we observed that out of the total
> > time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> > ~20% in vm_normal_folio().
> >
> > With batching, the proportion of vm_normal_folio() increases to over
> > 70% of move_pages_pte() without any changes to vm_normal_folio().
>
> Do you know why vm_normal_folio() could be expensive? I still see quite
> some other things this path needs to do.
Maybe as explained here?
https://lkml.kernel.org/r/20250807185819.199865-1-lorenzo.stoakes@oracle.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
2025-08-07 22:54 ` Andrew Morton
@ 2025-08-08 16:41 ` Lokesh Gidra
0 siblings, 0 replies; 11+ messages in thread
From: Lokesh Gidra @ 2025-08-08 16:41 UTC (permalink / raw)
To: Andrew Morton
Cc: Peter Xu, aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
Suren Baghdasaryan, Kalesh Singh, Barry Song, David Hildenbrand
On Thu, Aug 7, 2025 at 3:54 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 7 Aug 2025 15:16:57 -0400 Peter Xu <peterx@redhat.com> wrote:
>
> > Hi, Lokesh,
> >
> > On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> > > MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> > > for moving present pages. Mitigate this cost by opportunistically
> > > batching present contiguous pages for TLB flushing.
> > >
> > > Without batching, in our testing on an arm64 Android device with UFFD GC,
> > > which uses MOVE ioctl for compaction, we observed that out of the total
> > > time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> > > ~20% in vm_normal_folio().
> > >
> > > With batching, the proportion of vm_normal_folio() increases to over
> > > 70% of move_pages_pte() without any changes to vm_normal_folio().
> >
> > Do you know why vm_normal_folio() could be expensive? I still see quite
> > some other things this path needs to do.
>
> Maybe as explained here?
> https://lkml.kernel.org/r/20250807185819.199865-1-lorenzo.stoakes@oracle.com
>
Thanks for sharing this, Andrew. IMHO, this seems like the most likely
reason to me. There is nothing there other than a cold access to the
page struct.
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
2025-08-07 19:16 ` Peter Xu
2025-08-07 22:54 ` Andrew Morton
@ 2025-08-08 16:29 ` Lokesh Gidra
2025-08-10 6:31 ` Lokesh Gidra
2025-08-11 14:00 ` Peter Xu
1 sibling, 2 replies; 11+ messages in thread
From: Lokesh Gidra @ 2025-08-08 16:29 UTC (permalink / raw)
To: Peter Xu
Cc: akpm, aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
Suren Baghdasaryan, Kalesh Singh, Barry Song, David Hildenbrand
On Thu, Aug 7, 2025 at 12:17 PM Peter Xu <peterx@redhat.com> wrote:
>
> Hi, Lokesh,
>
> On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> > MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> > for moving present pages. Mitigate this cost by opportunistically
> > batching present contiguous pages for TLB flushing.
> >
> > Without batching, in our testing on an arm64 Android device with UFFD GC,
> > which uses MOVE ioctl for compaction, we observed that out of the total
> > time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> > ~20% in vm_normal_folio().
> >
> > With batching, the proportion of vm_normal_folio() increases to over
> > 70% of move_pages_pte() without any changes to vm_normal_folio().
>
> Do you know why vm_normal_folio() could be expensive? I still see quite
> some other things this path needs to do.
>
Let's discuss this in Andrew's reply thread.
> > Furthermore, time spent within move_pages_pte() is only ~20%, which
> > includes TLB-flush overhead.
>
> Indeed this should already prove the optimization, I'm just curious whether
> you've run some benchmark on the GC app to show the real world benefit.
>
I did! The same benchmark through which I gathered these numbers, when
run on cuttlefish (qemu android instance on x86_64), the completion
time of the benchmark went down from ~45mins to ~20mins. The benchmark
is very GC intensive and the overhead of IPI on vCPUs seems to be
enormous leading to this drastic improvement.
In another instance, system_server, one of the most critical system
processes on android, saw over 50% reduction in GC compaction time on
an arm64 android device.
> >
> > Cc: Suren Baghdasaryan <surenb@google.com>
> > Cc: Kalesh Singh <kaleshsingh@google.com>
> > Cc: Barry Song <v-songbaohua@oppo.com>
> > Cc: David Hildenbrand <david@redhat.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > ---
> > Changes since v2 [1]
> > - Addressed VM_WARN_ON failure, per Lorenzo Stoakes
> > - Added check to ensure all batched pages share the same anon_vma
> >
> > Changes since v1 [2]
> > - Removed flush_tlb_batched_pending(), per Barry Song
> > - Unified single and multi page case, per Barry Song
> >
> > [1] https://lore.kernel.org/all/20250805121410.1658418-1-lokeshgidra@google.com/
> > [2] https://lore.kernel.org/all/20250731104726.103071-1-lokeshgidra@google.com/
> >
> > mm/userfaultfd.c | 179 +++++++++++++++++++++++++++++++++--------------
> > 1 file changed, 128 insertions(+), 51 deletions(-)
> >
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index cbed91b09640..78c732100aec 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -1026,18 +1026,64 @@ static inline bool is_pte_pages_stable(pte_t *dst_pte, pte_t *src_pte,
> > pmd_same(dst_pmdval, pmdp_get_lockless(dst_pmd));
> > }
> >
> > -static int move_present_pte(struct mm_struct *mm,
> > - struct vm_area_struct *dst_vma,
> > - struct vm_area_struct *src_vma,
> > - unsigned long dst_addr, unsigned long src_addr,
> > - pte_t *dst_pte, pte_t *src_pte,
> > - pte_t orig_dst_pte, pte_t orig_src_pte,
> > - pmd_t *dst_pmd, pmd_t dst_pmdval,
> > - spinlock_t *dst_ptl, spinlock_t *src_ptl,
> > - struct folio *src_folio)
> > +/*
> > + * Checks if the two ptes and the corresponding folio are eligible for batched
> > + * move. If so, then returns pointer to the folio, after locking it. Otherwise,
> > + * returns NULL.
> > + */
> > +static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
> > + unsigned long src_addr,
> > + pte_t *src_pte, pte_t *dst_pte,
> > + struct anon_vma *src_anon_vma)
> > +{
> > + pte_t orig_dst_pte, orig_src_pte;
> > + struct folio *folio;
> > +
> > + orig_dst_pte = ptep_get(dst_pte);
> > + if (!pte_none(orig_dst_pte))
> > + return NULL;
> > +
> > + orig_src_pte = ptep_get(src_pte);
> > + if (pte_none(orig_src_pte) || !pte_present(orig_src_pte) ||
>
> pte_none() check could be removed - the pte_present() check should make
> sure it's !none.
>
Makes sense. I'll make the change in the next version of the patch.
> > + is_zero_pfn(pte_pfn(orig_src_pte)))
> > + return NULL;
> > +
> > + folio = vm_normal_folio(src_vma, src_addr, orig_src_pte);
> > + if (!folio || !folio_trylock(folio))
> > + return NULL;
>
> So here we don't take a refcount anymore, while the 1st folio that got
> passed in will still has the refcount boosted. IMHO it would still be
> better to keep the behavior the same on the 1st and continuous folios..
>
> Or if this is intentional, maybe worth some comment. More below on this..
>
This is indeed intentional, and I'll add a comment in the next
version. But let me explain:
The first folio needed the refcount as we need to pin the page before
releasing the src ptl. Also, because split_folio(), if called, expects
the caller to hold the lock as well as reference on the folio.
The subsequent folios in the batch are always within the ptl critical
section and neither the splits are required. Therefore, I didn't want
to unnecessarily increase the work done within the critical section.
But, please correct me if I'm misunderstanding something.
> > + if (!PageAnonExclusive(&folio->page) || folio_test_large(folio) ||
> > + folio_anon_vma(folio) != src_anon_vma) {
> > + folio_unlock(folio);
> > + return NULL;
> > + }
> > + return folio;
> > +}
> > +
> > +static long move_present_ptes(struct mm_struct *mm,
> > + struct vm_area_struct *dst_vma,
> > + struct vm_area_struct *src_vma,
> > + unsigned long dst_addr, unsigned long src_addr,
> > + pte_t *dst_pte, pte_t *src_pte,
> > + pte_t orig_dst_pte, pte_t orig_src_pte,
> > + pmd_t *dst_pmd, pmd_t dst_pmdval,
> > + spinlock_t *dst_ptl, spinlock_t *src_ptl,
> > + struct folio *src_folio, unsigned long len,
> > + struct anon_vma *src_anon_vma)
>
> (Not an immediate concern, but this function has potential to win the
> max-num-of-parameters kernel function.. :)
I noticed the same when I made the change :) Maybe in a subsequent
patch if we inline is_pte_pages_stable() and PTL acquire/release in
move_pages_ptes(), then quite a few parameters can be reduced. But
then, IMO, even move_pages_ptes() refactoring is required as well.
>
> > {
> > int err = 0;
> > + unsigned long src_start = src_addr;
> > + unsigned long addr_end;
> >
> > + if (len > PAGE_SIZE) {
> > + addr_end = (dst_addr + PMD_SIZE) & PMD_MASK;
> > + if (dst_addr + len > addr_end)
> > + len = addr_end - dst_addr;
>
> Use something like ALIGN() and MIN()?
Will do in the next version.
>
> > +
> > + addr_end = (src_addr + PMD_SIZE) & PMD_MASK;
> > + if (src_addr + len > addr_end)
> > + len = addr_end - src_addr;
>
> Same here.
>
Will do.
> > + }
> > + flush_cache_range(src_vma, src_addr, src_addr + len);
> > double_pt_lock(dst_ptl, src_ptl);
> >
> > if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> > @@ -1051,31 +1097,54 @@ static int move_present_pte(struct mm_struct *mm,
> > err = -EBUSY;
> > goto out;
> > }
> > + arch_enter_lazy_mmu_mode();
> > +
> > + addr_end = src_start + len;
> > + while (true) {
> > + orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
> > + /* Folio got pinned from under us. Put it back and fail the move. */
> > + if (folio_maybe_dma_pinned(src_folio)) {
> > + set_pte_at(mm, src_addr, src_pte, orig_src_pte);
> > + err = -EBUSY;
> > + break;
> > + }
> >
> > - orig_src_pte = ptep_clear_flush(src_vma, src_addr, src_pte);
> > - /* Folio got pinned from under us. Put it back and fail the move. */
> > - if (folio_maybe_dma_pinned(src_folio)) {
> > - set_pte_at(mm, src_addr, src_pte, orig_src_pte);
> > - err = -EBUSY;
> > - goto out;
> > - }
> > -
> > - folio_move_anon_rmap(src_folio, dst_vma);
> > - src_folio->index = linear_page_index(dst_vma, dst_addr);
> > + folio_move_anon_rmap(src_folio, dst_vma);
> > + src_folio->index = linear_page_index(dst_vma, dst_addr);
> >
> > - orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
> > - /* Set soft dirty bit so userspace can notice the pte was moved */
> > + orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
> > + /* Set soft dirty bit so userspace can notice the pte was moved */
> > #ifdef CONFIG_MEM_SOFT_DIRTY
> > - orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
> > + orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
> > #endif
> > - if (pte_dirty(orig_src_pte))
> > - orig_dst_pte = pte_mkdirty(orig_dst_pte);
> > - orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> > + if (pte_dirty(orig_src_pte))
> > + orig_dst_pte = pte_mkdirty(orig_dst_pte);
> > + orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> > + set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
> > +
> > + src_addr += PAGE_SIZE;
> > + if (src_addr == addr_end)
> > + break;
> > + src_pte++;
> > + dst_pte++;
> > +
> > + folio_unlock(src_folio);
> > + src_folio = check_ptes_for_batched_move(src_vma, src_addr, src_pte,
> > + dst_pte, src_anon_vma);
> > + if (!src_folio)
> > + break;
> > + dst_addr += PAGE_SIZE;
> > + }
> > +
> > + arch_leave_lazy_mmu_mode();
> > + if (src_addr > src_start)
> > + flush_tlb_range(src_vma, src_start, src_addr);
> >
> > - set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
> > out:
> > double_pt_unlock(dst_ptl, src_ptl);
> > - return err;
> > + if (src_folio)
> > + folio_unlock(src_folio);
> > + return src_addr > src_start ? src_addr - src_start : err;
> > }
> >
> > static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> > @@ -1140,7 +1209,7 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> > set_pte_at(mm, dst_addr, dst_pte, orig_src_pte);
> > double_pt_unlock(dst_ptl, src_ptl);
> >
> > - return 0;
> > + return PAGE_SIZE;
> > }
> >
> > static int move_zeropage_pte(struct mm_struct *mm,
> > @@ -1154,6 +1223,7 @@ static int move_zeropage_pte(struct mm_struct *mm,
> > {
> > pte_t zero_pte;
> >
> > + flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
>
> If it's a zero page hence not writtable, do we still need to flush cache at
> all? Looks harmless, but looks like not needed either.
>
I just realized when reading your comment that it is indeed not
required. There is no cacheline to be flushed for the zero-page :)
> > double_pt_lock(dst_ptl, src_ptl);
> > if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> > dst_pmd, dst_pmdval)) {
> > @@ -1167,20 +1237,19 @@ static int move_zeropage_pte(struct mm_struct *mm,
> > set_pte_at(mm, dst_addr, dst_pte, zero_pte);
> > double_pt_unlock(dst_ptl, src_ptl);
> >
> > - return 0;
> > + return PAGE_SIZE;
> > }
> >
> >
> > /*
> > - * The mmap_lock for reading is held by the caller. Just move the page
> > - * from src_pmd to dst_pmd if possible, and return true if succeeded
> > - * in moving the page.
> > + * The mmap_lock for reading is held by the caller. Just move the page(s)
> > + * from src_pmd to dst_pmd if possible, and return number of bytes moved.
> > */
> > -static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > - struct vm_area_struct *dst_vma,
> > - struct vm_area_struct *src_vma,
> > - unsigned long dst_addr, unsigned long src_addr,
> > - __u64 mode)
> > +static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > + struct vm_area_struct *dst_vma,
> > + struct vm_area_struct *src_vma,
> > + unsigned long dst_addr, unsigned long src_addr,
> > + unsigned long len, __u64 mode)
> > {
> > swp_entry_t entry;
> > struct swap_info_struct *si = NULL;
> > @@ -1196,9 +1265,8 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > struct mmu_notifier_range range;
> > int err = 0;
> >
> > - flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
> > mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm,
> > - src_addr, src_addr + PAGE_SIZE);
> > + src_addr, src_addr + len);
> > mmu_notifier_invalidate_range_start(&range);
> > retry:
> > /*
> > @@ -1257,7 +1325,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES))
> > err = -ENOENT;
> > else /* nothing to do to move a hole */
> > - err = 0;
> > + err = PAGE_SIZE;
> > goto out;
> > }
> >
> > @@ -1375,10 +1443,14 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > }
> > }
> >
> > - err = move_present_pte(mm, dst_vma, src_vma,
> > - dst_addr, src_addr, dst_pte, src_pte,
> > - orig_dst_pte, orig_src_pte, dst_pmd,
> > - dst_pmdval, dst_ptl, src_ptl, src_folio);
> > + err = move_present_ptes(mm, dst_vma, src_vma,
> > + dst_addr, src_addr, dst_pte, src_pte,
> > + orig_dst_pte, orig_src_pte, dst_pmd,
> > + dst_pmdval, dst_ptl, src_ptl, src_folio,
> > + len, src_anon_vma);
> > + /* folio is already unlocked by move_present_ptes() */
> > + folio_put(src_folio);
> > + src_folio = NULL;
>
> So the function above now can move multiple folios but keep holding the
> 1st's refcount.. This still smells error prone, sooner or later.
>
> Would it be slightly better if we take a folio pointer in
> move_present_ptes(), and releae everything there (including reset the
> pointer)?
>
Yeah this seems cleaner. I'll do so in the next version.
> Thanks,
>
> > } else {
> > struct folio *folio = NULL;
> >
> > @@ -1732,7 +1804,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > {
> > struct mm_struct *mm = ctx->mm;
> > struct vm_area_struct *src_vma, *dst_vma;
> > - unsigned long src_addr, dst_addr;
> > + unsigned long src_addr, dst_addr, src_end;
> > pmd_t *src_pmd, *dst_pmd;
> > long err = -EINVAL;
> > ssize_t moved = 0;
> > @@ -1775,8 +1847,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > if (err)
> > goto out_unlock;
> >
> > - for (src_addr = src_start, dst_addr = dst_start;
> > - src_addr < src_start + len;) {
> > + for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
> > + src_addr < src_end;) {
> > spinlock_t *ptl;
> > pmd_t dst_pmdval;
> > unsigned long step_size;
> > @@ -1841,6 +1913,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > dst_addr, src_addr);
> > step_size = HPAGE_PMD_SIZE;
> > } else {
> > + long ret;
> > +
> > if (pmd_none(*src_pmd)) {
> > if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> > err = -ENOENT;
> > @@ -1857,10 +1931,13 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > break;
> > }
> >
> > - err = move_pages_pte(mm, dst_pmd, src_pmd,
> > - dst_vma, src_vma,
> > - dst_addr, src_addr, mode);
> > - step_size = PAGE_SIZE;
> > + ret = move_pages_ptes(mm, dst_pmd, src_pmd,
> > + dst_vma, src_vma, dst_addr,
> > + src_addr, src_end - src_addr, mode);
> > + if (ret > 0)
> > + step_size = ret;
> > + else
> > + err = ret;
> > }
> >
> > cond_resched();
> >
> > base-commit: 6e64f4580381e32c06ee146ca807c555b8f73e24
> > --
> > 2.50.1.565.gc32cd1483b-goog
> >
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
2025-08-08 16:29 ` Lokesh Gidra
@ 2025-08-10 6:31 ` Lokesh Gidra
2025-08-11 14:00 ` Peter Xu
1 sibling, 0 replies; 11+ messages in thread
From: Lokesh Gidra @ 2025-08-10 6:31 UTC (permalink / raw)
To: Peter Xu
Cc: akpm, aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
Suren Baghdasaryan, Kalesh Singh, Barry Song, David Hildenbrand
On Fri, Aug 8, 2025 at 9:29 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
>
> On Thu, Aug 7, 2025 at 12:17 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > Hi, Lokesh,
> >
> > On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> > > MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> > > for moving present pages. Mitigate this cost by opportunistically
> > > batching present contiguous pages for TLB flushing.
> > >
> > > Without batching, in our testing on an arm64 Android device with UFFD GC,
> > > which uses MOVE ioctl for compaction, we observed that out of the total
> > > time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> > > ~20% in vm_normal_folio().
> > >
> > > With batching, the proportion of vm_normal_folio() increases to over
> > > 70% of move_pages_pte() without any changes to vm_normal_folio().
> >
> > Do you know why vm_normal_folio() could be expensive? I still see quite
> > some other things this path needs to do.
> >
> Let's discuss this in Andrew's reply thread.
>
> > > Furthermore, time spent within move_pages_pte() is only ~20%, which
> > > includes TLB-flush overhead.
> >
> > Indeed this should already prove the optimization, I'm just curious whether
> > you've run some benchmark on the GC app to show the real world benefit.
> >
> I did! The same benchmark through which I gathered these numbers, when
> run on cuttlefish (qemu android instance on x86_64), the completion
> time of the benchmark went down from ~45mins to ~20mins. The benchmark
> is very GC intensive and the overhead of IPI on vCPUs seems to be
> enormous leading to this drastic improvement.
>
> In another instance, system_server, one of the most critical system
> processes on android, saw over 50% reduction in GC compaction time on
> an arm64 android device.
>
> > >
> > > Cc: Suren Baghdasaryan <surenb@google.com>
> > > Cc: Kalesh Singh <kaleshsingh@google.com>
> > > Cc: Barry Song <v-songbaohua@oppo.com>
> > > Cc: David Hildenbrand <david@redhat.com>
> > > Cc: Peter Xu <peterx@redhat.com>
> > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > > ---
> > > Changes since v2 [1]
> > > - Addressed VM_WARN_ON failure, per Lorenzo Stoakes
> > > - Added check to ensure all batched pages share the same anon_vma
> > >
> > > Changes since v1 [2]
> > > - Removed flush_tlb_batched_pending(), per Barry Song
> > > - Unified single and multi page case, per Barry Song
> > >
> > > [1] https://lore.kernel.org/all/20250805121410.1658418-1-lokeshgidra@google.com/
> > > [2] https://lore.kernel.org/all/20250731104726.103071-1-lokeshgidra@google.com/
> > >
> > > mm/userfaultfd.c | 179 +++++++++++++++++++++++++++++++++--------------
> > > 1 file changed, 128 insertions(+), 51 deletions(-)
> > >
> > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > index cbed91b09640..78c732100aec 100644
> > > --- a/mm/userfaultfd.c
> > > +++ b/mm/userfaultfd.c
> > > @@ -1026,18 +1026,64 @@ static inline bool is_pte_pages_stable(pte_t *dst_pte, pte_t *src_pte,
> > > pmd_same(dst_pmdval, pmdp_get_lockless(dst_pmd));
> > > }
> > >
> > > -static int move_present_pte(struct mm_struct *mm,
> > > - struct vm_area_struct *dst_vma,
> > > - struct vm_area_struct *src_vma,
> > > - unsigned long dst_addr, unsigned long src_addr,
> > > - pte_t *dst_pte, pte_t *src_pte,
> > > - pte_t orig_dst_pte, pte_t orig_src_pte,
> > > - pmd_t *dst_pmd, pmd_t dst_pmdval,
> > > - spinlock_t *dst_ptl, spinlock_t *src_ptl,
> > > - struct folio *src_folio)
> > > +/*
> > > + * Checks if the two ptes and the corresponding folio are eligible for batched
> > > + * move. If so, then returns pointer to the folio, after locking it. Otherwise,
> > > + * returns NULL.
> > > + */
> > > +static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
> > > + unsigned long src_addr,
> > > + pte_t *src_pte, pte_t *dst_pte,
> > > + struct anon_vma *src_anon_vma)
> > > +{
> > > + pte_t orig_dst_pte, orig_src_pte;
> > > + struct folio *folio;
> > > +
> > > + orig_dst_pte = ptep_get(dst_pte);
> > > + if (!pte_none(orig_dst_pte))
> > > + return NULL;
> > > +
> > > + orig_src_pte = ptep_get(src_pte);
> > > + if (pte_none(orig_src_pte) || !pte_present(orig_src_pte) ||
> >
> > pte_none() check could be removed - the pte_present() check should make
> > sure it's !none.
> >
> Makes sense. I'll make the change in the next version of the patch.
>
> > > + is_zero_pfn(pte_pfn(orig_src_pte)))
> > > + return NULL;
> > > +
> > > + folio = vm_normal_folio(src_vma, src_addr, orig_src_pte);
> > > + if (!folio || !folio_trylock(folio))
> > > + return NULL;
> >
> > So here we don't take a refcount anymore, while the 1st folio that got
> > passed in will still has the refcount boosted. IMHO it would still be
> > better to keep the behavior the same on the 1st and continuous folios..
> >
> > Or if this is intentional, maybe worth some comment. More below on this..
> >
> This is indeed intentional, and I'll add a comment in the next
> version. But let me explain:
>
> The first folio needed the refcount as we need to pin the page before
> releasing the src ptl. Also, because split_folio(), if called, expects
> the caller to hold the lock as well as reference on the folio.
>
> The subsequent folios in the batch are always within the ptl critical
> section and neither the splits are required. Therefore, I didn't want
> to unnecessarily increase the work done within the critical section.
> But, please correct me if I'm misunderstanding something.
>
> > > + if (!PageAnonExclusive(&folio->page) || folio_test_large(folio) ||
> > > + folio_anon_vma(folio) != src_anon_vma) {
> > > + folio_unlock(folio);
> > > + return NULL;
> > > + }
> > > + return folio;
> > > +}
> > > +
> > > +static long move_present_ptes(struct mm_struct *mm,
> > > + struct vm_area_struct *dst_vma,
> > > + struct vm_area_struct *src_vma,
> > > + unsigned long dst_addr, unsigned long src_addr,
> > > + pte_t *dst_pte, pte_t *src_pte,
> > > + pte_t orig_dst_pte, pte_t orig_src_pte,
> > > + pmd_t *dst_pmd, pmd_t dst_pmdval,
> > > + spinlock_t *dst_ptl, spinlock_t *src_ptl,
> > > + struct folio *src_folio, unsigned long len,
> > > + struct anon_vma *src_anon_vma)
> >
> > (Not an immediate concern, but this function has potential to win the
> > max-num-of-parameters kernel function.. :)
>
> I noticed the same when I made the change :) Maybe in a subsequent
> patch if we inline is_pte_pages_stable() and PTL acquire/release in
> move_pages_ptes(), then quite a few parameters can be reduced. But
> then, IMO, even move_pages_ptes() refactoring is required as well.
> >
> > > {
> > > int err = 0;
> > > + unsigned long src_start = src_addr;
> > > + unsigned long addr_end;
> > >
> > > + if (len > PAGE_SIZE) {
> > > + addr_end = (dst_addr + PMD_SIZE) & PMD_MASK;
> > > + if (dst_addr + len > addr_end)
> > > + len = addr_end - dst_addr;
> >
> > Use something like ALIGN() and MIN()?
>
> Will do in the next version.
> >
> > > +
> > > + addr_end = (src_addr + PMD_SIZE) & PMD_MASK;
> > > + if (src_addr + len > addr_end)
> > > + len = addr_end - src_addr;
> >
> > Same here.
> >
> Will do.
>
> > > + }
> > > + flush_cache_range(src_vma, src_addr, src_addr + len);
> > > double_pt_lock(dst_ptl, src_ptl);
> > >
> > > if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> > > @@ -1051,31 +1097,54 @@ static int move_present_pte(struct mm_struct *mm,
> > > err = -EBUSY;
> > > goto out;
> > > }
> > > + arch_enter_lazy_mmu_mode();
> > > +
> > > + addr_end = src_start + len;
> > > + while (true) {
> > > + orig_src_pte = ptep_get_and_clear(mm, src_addr, src_pte);
> > > + /* Folio got pinned from under us. Put it back and fail the move. */
> > > + if (folio_maybe_dma_pinned(src_folio)) {
> > > + set_pte_at(mm, src_addr, src_pte, orig_src_pte);
> > > + err = -EBUSY;
> > > + break;
> > > + }
> > >
> > > - orig_src_pte = ptep_clear_flush(src_vma, src_addr, src_pte);
> > > - /* Folio got pinned from under us. Put it back and fail the move. */
> > > - if (folio_maybe_dma_pinned(src_folio)) {
> > > - set_pte_at(mm, src_addr, src_pte, orig_src_pte);
> > > - err = -EBUSY;
> > > - goto out;
> > > - }
> > > -
> > > - folio_move_anon_rmap(src_folio, dst_vma);
> > > - src_folio->index = linear_page_index(dst_vma, dst_addr);
> > > + folio_move_anon_rmap(src_folio, dst_vma);
> > > + src_folio->index = linear_page_index(dst_vma, dst_addr);
> > >
> > > - orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
> > > - /* Set soft dirty bit so userspace can notice the pte was moved */
> > > + orig_dst_pte = folio_mk_pte(src_folio, dst_vma->vm_page_prot);
> > > + /* Set soft dirty bit so userspace can notice the pte was moved */
> > > #ifdef CONFIG_MEM_SOFT_DIRTY
> > > - orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
> > > + orig_dst_pte = pte_mksoft_dirty(orig_dst_pte);
> > > #endif
> > > - if (pte_dirty(orig_src_pte))
> > > - orig_dst_pte = pte_mkdirty(orig_dst_pte);
> > > - orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> > > + if (pte_dirty(orig_src_pte))
> > > + orig_dst_pte = pte_mkdirty(orig_dst_pte);
> > > + orig_dst_pte = pte_mkwrite(orig_dst_pte, dst_vma);
> > > + set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
> > > +
> > > + src_addr += PAGE_SIZE;
> > > + if (src_addr == addr_end)
> > > + break;
> > > + src_pte++;
> > > + dst_pte++;
> > > +
> > > + folio_unlock(src_folio);
> > > + src_folio = check_ptes_for_batched_move(src_vma, src_addr, src_pte,
> > > + dst_pte, src_anon_vma);
> > > + if (!src_folio)
> > > + break;
> > > + dst_addr += PAGE_SIZE;
> > > + }
> > > +
> > > + arch_leave_lazy_mmu_mode();
> > > + if (src_addr > src_start)
> > > + flush_tlb_range(src_vma, src_start, src_addr);
> > >
> > > - set_pte_at(mm, dst_addr, dst_pte, orig_dst_pte);
> > > out:
> > > double_pt_unlock(dst_ptl, src_ptl);
> > > - return err;
> > > + if (src_folio)
> > > + folio_unlock(src_folio);
> > > + return src_addr > src_start ? src_addr - src_start : err;
> > > }
> > >
> > > static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> > > @@ -1140,7 +1209,7 @@ static int move_swap_pte(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> > > set_pte_at(mm, dst_addr, dst_pte, orig_src_pte);
> > > double_pt_unlock(dst_ptl, src_ptl);
> > >
> > > - return 0;
> > > + return PAGE_SIZE;
> > > }
> > >
> > > static int move_zeropage_pte(struct mm_struct *mm,
> > > @@ -1154,6 +1223,7 @@ static int move_zeropage_pte(struct mm_struct *mm,
> > > {
> > > pte_t zero_pte;
> > >
> > > + flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
> >
> > If it's a zero page hence not writtable, do we still need to flush cache at
> > all? Looks harmless, but looks like not needed either.
> >
> I just realized when reading your comment that it is indeed not
> required. There is no cacheline to be flushed for the zero-page :)
>
> > > double_pt_lock(dst_ptl, src_ptl);
> > > if (!is_pte_pages_stable(dst_pte, src_pte, orig_dst_pte, orig_src_pte,
> > > dst_pmd, dst_pmdval)) {
> > > @@ -1167,20 +1237,19 @@ static int move_zeropage_pte(struct mm_struct *mm,
> > > set_pte_at(mm, dst_addr, dst_pte, zero_pte);
> > > double_pt_unlock(dst_ptl, src_ptl);
> > >
> > > - return 0;
> > > + return PAGE_SIZE;
> > > }
> > >
> > >
> > > /*
> > > - * The mmap_lock for reading is held by the caller. Just move the page
> > > - * from src_pmd to dst_pmd if possible, and return true if succeeded
> > > - * in moving the page.
> > > + * The mmap_lock for reading is held by the caller. Just move the page(s)
> > > + * from src_pmd to dst_pmd if possible, and return number of bytes moved.
> > > */
> > > -static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > > - struct vm_area_struct *dst_vma,
> > > - struct vm_area_struct *src_vma,
> > > - unsigned long dst_addr, unsigned long src_addr,
> > > - __u64 mode)
> > > +static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > > + struct vm_area_struct *dst_vma,
> > > + struct vm_area_struct *src_vma,
> > > + unsigned long dst_addr, unsigned long src_addr,
> > > + unsigned long len, __u64 mode)
> > > {
> > > swp_entry_t entry;
> > > struct swap_info_struct *si = NULL;
> > > @@ -1196,9 +1265,8 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > > struct mmu_notifier_range range;
> > > int err = 0;
> > >
> > > - flush_cache_range(src_vma, src_addr, src_addr + PAGE_SIZE);
> > > mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm,
> > > - src_addr, src_addr + PAGE_SIZE);
> > > + src_addr, src_addr + len);
> > > mmu_notifier_invalidate_range_start(&range);
> > > retry:
> > > /*
> > > @@ -1257,7 +1325,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > > if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES))
> > > err = -ENOENT;
> > > else /* nothing to do to move a hole */
> > > - err = 0;
> > > + err = PAGE_SIZE;
> > > goto out;
> > > }
> > >
> > > @@ -1375,10 +1443,14 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
> > > }
> > > }
> > >
> > > - err = move_present_pte(mm, dst_vma, src_vma,
> > > - dst_addr, src_addr, dst_pte, src_pte,
> > > - orig_dst_pte, orig_src_pte, dst_pmd,
> > > - dst_pmdval, dst_ptl, src_ptl, src_folio);
> > > + err = move_present_ptes(mm, dst_vma, src_vma,
> > > + dst_addr, src_addr, dst_pte, src_pte,
> > > + orig_dst_pte, orig_src_pte, dst_pmd,
> > > + dst_pmdval, dst_ptl, src_ptl, src_folio,
> > > + len, src_anon_vma);
> > > + /* folio is already unlocked by move_present_ptes() */
> > > + folio_put(src_folio);
> > > + src_folio = NULL;
> >
> > So the function above now can move multiple folios but keep holding the
> > 1st's refcount.. This still smells error prone, sooner or later.
> >
> > Would it be slightly better if we take a folio pointer in
> > move_present_ptes(), and releae everything there (including reset the
> > pointer)?
> >
> Yeah this seems cleaner. I'll do so in the next version.
>
Uploaded v4 addressing all the comments here:
https://lore.kernel.org/all/20250810062912.1096815-1-lokeshgidra@google.com/
Thanks.
> > Thanks,
> >
> > > } else {
> > > struct folio *folio = NULL;
> > >
> > > @@ -1732,7 +1804,7 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > {
> > > struct mm_struct *mm = ctx->mm;
> > > struct vm_area_struct *src_vma, *dst_vma;
> > > - unsigned long src_addr, dst_addr;
> > > + unsigned long src_addr, dst_addr, src_end;
> > > pmd_t *src_pmd, *dst_pmd;
> > > long err = -EINVAL;
> > > ssize_t moved = 0;
> > > @@ -1775,8 +1847,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > if (err)
> > > goto out_unlock;
> > >
> > > - for (src_addr = src_start, dst_addr = dst_start;
> > > - src_addr < src_start + len;) {
> > > + for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
> > > + src_addr < src_end;) {
> > > spinlock_t *ptl;
> > > pmd_t dst_pmdval;
> > > unsigned long step_size;
> > > @@ -1841,6 +1913,8 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > dst_addr, src_addr);
> > > step_size = HPAGE_PMD_SIZE;
> > > } else {
> > > + long ret;
> > > +
> > > if (pmd_none(*src_pmd)) {
> > > if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> > > err = -ENOENT;
> > > @@ -1857,10 +1931,13 @@ ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> > > break;
> > > }
> > >
> > > - err = move_pages_pte(mm, dst_pmd, src_pmd,
> > > - dst_vma, src_vma,
> > > - dst_addr, src_addr, mode);
> > > - step_size = PAGE_SIZE;
> > > + ret = move_pages_ptes(mm, dst_pmd, src_pmd,
> > > + dst_vma, src_vma, dst_addr,
> > > + src_addr, src_end - src_addr, mode);
> > > + if (ret > 0)
> > > + step_size = ret;
> > > + else
> > > + err = ret;
> > > }
> > >
> > > cond_resched();
> > >
> > > base-commit: 6e64f4580381e32c06ee146ca807c555b8f73e24
> > > --
> > > 2.50.1.565.gc32cd1483b-goog
> > >
> >
> > --
> > Peter Xu
> >
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
2025-08-08 16:29 ` Lokesh Gidra
2025-08-10 6:31 ` Lokesh Gidra
@ 2025-08-11 14:00 ` Peter Xu
2025-08-12 14:01 ` Lokesh Gidra
1 sibling, 1 reply; 11+ messages in thread
From: Peter Xu @ 2025-08-11 14:00 UTC (permalink / raw)
To: Lokesh Gidra
Cc: akpm, aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
Suren Baghdasaryan, Kalesh Singh, Barry Song, David Hildenbrand
On Fri, Aug 08, 2025 at 09:29:58AM -0700, Lokesh Gidra wrote:
> On Thu, Aug 7, 2025 at 12:17 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > Hi, Lokesh,
> >
> > On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> > > MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> > > for moving present pages. Mitigate this cost by opportunistically
> > > batching present contiguous pages for TLB flushing.
> > >
> > > Without batching, in our testing on an arm64 Android device with UFFD GC,
> > > which uses MOVE ioctl for compaction, we observed that out of the total
> > > time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> > > ~20% in vm_normal_folio().
> > >
> > > With batching, the proportion of vm_normal_folio() increases to over
> > > 70% of move_pages_pte() without any changes to vm_normal_folio().
> >
> > Do you know why vm_normal_folio() could be expensive? I still see quite
> > some other things this path needs to do.
> >
> Let's discuss this in Andrew's reply thread.
Sorry to get back to this late. Thanks for the link, Andrew!
>
> > > Furthermore, time spent within move_pages_pte() is only ~20%, which
> > > includes TLB-flush overhead.
> >
> > Indeed this should already prove the optimization, I'm just curious whether
> > you've run some benchmark on the GC app to show the real world benefit.
> >
> I did! The same benchmark through which I gathered these numbers, when
> run on cuttlefish (qemu android instance on x86_64), the completion
> time of the benchmark went down from ~45mins to ~20mins. The benchmark
> is very GC intensive and the overhead of IPI on vCPUs seems to be
> enormous leading to this drastic improvement.
>
> In another instance, system_server, one of the most critical system
> processes on android, saw over 50% reduction in GC compaction time on
> an arm64 android device.
Would you mind add some of these numbers into the commit message when you
repost?
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
2025-08-11 14:00 ` Peter Xu
@ 2025-08-12 14:01 ` Lokesh Gidra
0 siblings, 0 replies; 11+ messages in thread
From: Lokesh Gidra @ 2025-08-12 14:01 UTC (permalink / raw)
To: Peter Xu
Cc: akpm, aarcange, linux-mm, linux-kernel, 21cnbao, ngeoffray,
Suren Baghdasaryan, Kalesh Singh, Barry Song, David Hildenbrand
On Mon, Aug 11, 2025 at 7:00 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Aug 08, 2025 at 09:29:58AM -0700, Lokesh Gidra wrote:
> > On Thu, Aug 7, 2025 at 12:17 PM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > Hi, Lokesh,
> > >
> > > On Thu, Aug 07, 2025 at 03:39:02AM -0700, Lokesh Gidra wrote:
> > > > MOVE ioctl's runtime is dominated by TLB-flush cost, which is required
> > > > for moving present pages. Mitigate this cost by opportunistically
> > > > batching present contiguous pages for TLB flushing.
> > > >
> > > > Without batching, in our testing on an arm64 Android device with UFFD GC,
> > > > which uses MOVE ioctl for compaction, we observed that out of the total
> > > > time spent in move_pages_pte(), over 40% is in ptep_clear_flush(), and
> > > > ~20% in vm_normal_folio().
> > > >
> > > > With batching, the proportion of vm_normal_folio() increases to over
> > > > 70% of move_pages_pte() without any changes to vm_normal_folio().
> > >
> > > Do you know why vm_normal_folio() could be expensive? I still see quite
> > > some other things this path needs to do.
> > >
> > Let's discuss this in Andrew's reply thread.
>
> Sorry to get back to this late. Thanks for the link, Andrew!
>
> >
> > > > Furthermore, time spent within move_pages_pte() is only ~20%, which
> > > > includes TLB-flush overhead.
> > >
> > > Indeed this should already prove the optimization, I'm just curious whether
> > > you've run some benchmark on the GC app to show the real world benefit.
> > >
> > I did! The same benchmark through which I gathered these numbers, when
> > run on cuttlefish (qemu android instance on x86_64), the completion
> > time of the benchmark went down from ~45mins to ~20mins. The benchmark
> > is very GC intensive and the overhead of IPI on vCPUs seems to be
> > enormous leading to this drastic improvement.
> >
> > In another instance, system_server, one of the most critical system
> > processes on android, saw over 50% reduction in GC compaction time on
> > an arm64 android device.
>
> Would you mind add some of these numbers into the commit message when you
> repost?
I can certainly do that in v5. But I already sent v4 addressing all
your other feedback. I'll incorporate these numbers as well as any
additional changes suggested in v4 in v5.
>
> Thanks,
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
2025-08-07 10:39 Lokesh Gidra
2025-08-07 19:16 ` Peter Xu
@ 2025-08-08 6:18 ` Dan Carpenter
2025-08-08 15:27 ` Lokesh Gidra
1 sibling, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2025-08-08 6:18 UTC (permalink / raw)
To: oe-kbuild, Lokesh Gidra; +Cc: lkp, oe-kbuild-all
Hi Lokesh,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/Lokesh-Gidra/userfaultfd-opportunistic-TLB-flush-batching-for-present-pages-in-MOVE/20250807-184025
base: 6e64f4580381e32c06ee146ca807c555b8f73e24
patch link: https://lore.kernel.org/r/20250807103902.2242717-1-lokeshgidra%40google.com
patch subject: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
config: openrisc-randconfig-r071-20250808 (https://download.01.org/0day-ci/archive/20250808/202508080639.Cf7SyRwQ-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 10.5.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202508080639.Cf7SyRwQ-lkp@intel.com/
smatch warnings:
mm/userfaultfd.c:1959 move_pages() error: uninitialized symbol 'step_size'.
vim +/step_size +1959 mm/userfaultfd.c
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1802 ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1803 unsigned long src_start, unsigned long len, __u64 mode)
adef440691bab82 Andrea Arcangeli 2023-12-06 1804 {
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1805 struct mm_struct *mm = ctx->mm;
adef440691bab82 Andrea Arcangeli 2023-12-06 1806 struct vm_area_struct *src_vma, *dst_vma;
3abf06e0c484867 Lokesh Gidra 2025-08-07 1807 unsigned long src_addr, dst_addr, src_end;
adef440691bab82 Andrea Arcangeli 2023-12-06 1808 pmd_t *src_pmd, *dst_pmd;
adef440691bab82 Andrea Arcangeli 2023-12-06 1809 long err = -EINVAL;
adef440691bab82 Andrea Arcangeli 2023-12-06 1810 ssize_t moved = 0;
adef440691bab82 Andrea Arcangeli 2023-12-06 1811
adef440691bab82 Andrea Arcangeli 2023-12-06 1812 /* Sanitize the command parameters. */
31defc3b01d907e Tal Zussman 2025-06-19 1813 VM_WARN_ON_ONCE(src_start & ~PAGE_MASK);
31defc3b01d907e Tal Zussman 2025-06-19 1814 VM_WARN_ON_ONCE(dst_start & ~PAGE_MASK);
31defc3b01d907e Tal Zussman 2025-06-19 1815 VM_WARN_ON_ONCE(len & ~PAGE_MASK);
adef440691bab82 Andrea Arcangeli 2023-12-06 1816
adef440691bab82 Andrea Arcangeli 2023-12-06 1817 /* Does the address range wrap, or is the span zero-sized? */
31defc3b01d907e Tal Zussman 2025-06-19 1818 VM_WARN_ON_ONCE(src_start + len < src_start);
31defc3b01d907e Tal Zussman 2025-06-19 1819 VM_WARN_ON_ONCE(dst_start + len < dst_start);
adef440691bab82 Andrea Arcangeli 2023-12-06 1820
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1821 err = uffd_move_lock(mm, dst_start, src_start, &dst_vma, &src_vma);
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1822 if (err)
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1823 goto out;
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1824
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1825 /* Re-check after taking map_changing_lock */
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1826 err = -EAGAIN;
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1827 down_read(&ctx->map_changing_lock);
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1828 if (likely(atomic_read(&ctx->mmap_changing)))
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1829 goto out_unlock;
adef440691bab82 Andrea Arcangeli 2023-12-06 1830 /*
adef440691bab82 Andrea Arcangeli 2023-12-06 1831 * Make sure the vma is not shared, that the src and dst remap
adef440691bab82 Andrea Arcangeli 2023-12-06 1832 * ranges are both valid and fully within a single existing
adef440691bab82 Andrea Arcangeli 2023-12-06 1833 * vma.
adef440691bab82 Andrea Arcangeli 2023-12-06 1834 */
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1835 err = -EINVAL;
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1836 if (src_vma->vm_flags & VM_SHARED)
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1837 goto out_unlock;
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1838 if (src_start + len > src_vma->vm_end)
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1839 goto out_unlock;
adef440691bab82 Andrea Arcangeli 2023-12-06 1840
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1841 if (dst_vma->vm_flags & VM_SHARED)
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1842 goto out_unlock;
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1843 if (dst_start + len > dst_vma->vm_end)
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1844 goto out_unlock;
adef440691bab82 Andrea Arcangeli 2023-12-06 1845
adef440691bab82 Andrea Arcangeli 2023-12-06 1846 err = validate_move_areas(ctx, src_vma, dst_vma);
adef440691bab82 Andrea Arcangeli 2023-12-06 1847 if (err)
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1848 goto out_unlock;
adef440691bab82 Andrea Arcangeli 2023-12-06 1849
3abf06e0c484867 Lokesh Gidra 2025-08-07 1850 for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
3abf06e0c484867 Lokesh Gidra 2025-08-07 1851 src_addr < src_end;) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1852 spinlock_t *ptl;
adef440691bab82 Andrea Arcangeli 2023-12-06 1853 pmd_t dst_pmdval;
adef440691bab82 Andrea Arcangeli 2023-12-06 1854 unsigned long step_size;
adef440691bab82 Andrea Arcangeli 2023-12-06 1855
adef440691bab82 Andrea Arcangeli 2023-12-06 1856 /*
adef440691bab82 Andrea Arcangeli 2023-12-06 1857 * Below works because anonymous area would not have a
adef440691bab82 Andrea Arcangeli 2023-12-06 1858 * transparent huge PUD. If file-backed support is added,
adef440691bab82 Andrea Arcangeli 2023-12-06 1859 * that case would need to be handled here.
adef440691bab82 Andrea Arcangeli 2023-12-06 1860 */
adef440691bab82 Andrea Arcangeli 2023-12-06 1861 src_pmd = mm_find_pmd(mm, src_addr);
adef440691bab82 Andrea Arcangeli 2023-12-06 1862 if (unlikely(!src_pmd)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1863 if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1864 err = -ENOENT;
adef440691bab82 Andrea Arcangeli 2023-12-06 1865 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1866 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1867 src_pmd = mm_alloc_pmd(mm, src_addr);
adef440691bab82 Andrea Arcangeli 2023-12-06 1868 if (unlikely(!src_pmd)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1869 err = -ENOMEM;
adef440691bab82 Andrea Arcangeli 2023-12-06 1870 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1871 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1872 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1873 dst_pmd = mm_alloc_pmd(mm, dst_addr);
adef440691bab82 Andrea Arcangeli 2023-12-06 1874 if (unlikely(!dst_pmd)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1875 err = -ENOMEM;
adef440691bab82 Andrea Arcangeli 2023-12-06 1876 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1877 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1878
adef440691bab82 Andrea Arcangeli 2023-12-06 1879 dst_pmdval = pmdp_get_lockless(dst_pmd);
adef440691bab82 Andrea Arcangeli 2023-12-06 1880 /*
adef440691bab82 Andrea Arcangeli 2023-12-06 1881 * If the dst_pmd is mapped as THP don't override it and just
adef440691bab82 Andrea Arcangeli 2023-12-06 1882 * be strict. If dst_pmd changes into TPH after this check, the
adef440691bab82 Andrea Arcangeli 2023-12-06 1883 * move_pages_huge_pmd() will detect the change and retry
adef440691bab82 Andrea Arcangeli 2023-12-06 1884 * while move_pages_pte() will detect the change and fail.
adef440691bab82 Andrea Arcangeli 2023-12-06 1885 */
adef440691bab82 Andrea Arcangeli 2023-12-06 1886 if (unlikely(pmd_trans_huge(dst_pmdval))) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1887 err = -EEXIST;
adef440691bab82 Andrea Arcangeli 2023-12-06 1888 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1889 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1890
adef440691bab82 Andrea Arcangeli 2023-12-06 1891 ptl = pmd_trans_huge_lock(src_pmd, src_vma);
adef440691bab82 Andrea Arcangeli 2023-12-06 1892 if (ptl) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1893 /* Check if we can move the pmd without splitting it. */
adef440691bab82 Andrea Arcangeli 2023-12-06 1894 if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
adef440691bab82 Andrea Arcangeli 2023-12-06 1895 !pmd_none(dst_pmdval)) {
e06d03d5590ae1c Matthew Wilcox (Oracle 2024-03-26 1896) struct folio *folio = pmd_folio(*src_pmd);
adef440691bab82 Andrea Arcangeli 2023-12-06 1897
5beaee54a324ba1 Matthew Wilcox (Oracle 2024-03-26 1898) if (!folio || (!is_huge_zero_folio(folio) &&
eb1521dad8f391d Suren Baghdasaryan 2024-01-31 1899 !PageAnonExclusive(&folio->page))) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1900 spin_unlock(ptl);
adef440691bab82 Andrea Arcangeli 2023-12-06 1901 err = -EBUSY;
adef440691bab82 Andrea Arcangeli 2023-12-06 1902 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1903 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1904
adef440691bab82 Andrea Arcangeli 2023-12-06 1905 spin_unlock(ptl);
adef440691bab82 Andrea Arcangeli 2023-12-06 1906 split_huge_pmd(src_vma, src_pmd, src_addr);
adef440691bab82 Andrea Arcangeli 2023-12-06 1907 /* The folio will be split by move_pages_pte() */
adef440691bab82 Andrea Arcangeli 2023-12-06 1908 continue;
adef440691bab82 Andrea Arcangeli 2023-12-06 1909 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1910
adef440691bab82 Andrea Arcangeli 2023-12-06 1911 err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
adef440691bab82 Andrea Arcangeli 2023-12-06 1912 dst_pmdval, dst_vma, src_vma,
adef440691bab82 Andrea Arcangeli 2023-12-06 1913 dst_addr, src_addr);
adef440691bab82 Andrea Arcangeli 2023-12-06 1914 step_size = HPAGE_PMD_SIZE;
adef440691bab82 Andrea Arcangeli 2023-12-06 1915 } else {
3abf06e0c484867 Lokesh Gidra 2025-08-07 1916 long ret;
3abf06e0c484867 Lokesh Gidra 2025-08-07 1917
adef440691bab82 Andrea Arcangeli 2023-12-06 1918 if (pmd_none(*src_pmd)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1919 if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1920 err = -ENOENT;
adef440691bab82 Andrea Arcangeli 2023-12-06 1921 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1922 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1923 if (unlikely(__pte_alloc(mm, src_pmd))) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1924 err = -ENOMEM;
adef440691bab82 Andrea Arcangeli 2023-12-06 1925 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1926 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1927 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1928
adef440691bab82 Andrea Arcangeli 2023-12-06 1929 if (unlikely(pte_alloc(mm, dst_pmd))) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1930 err = -ENOMEM;
adef440691bab82 Andrea Arcangeli 2023-12-06 1931 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1932 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1933
3abf06e0c484867 Lokesh Gidra 2025-08-07 1934 ret = move_pages_ptes(mm, dst_pmd, src_pmd,
3abf06e0c484867 Lokesh Gidra 2025-08-07 1935 dst_vma, src_vma, dst_addr,
3abf06e0c484867 Lokesh Gidra 2025-08-07 1936 src_addr, src_end - src_addr, mode);
3abf06e0c484867 Lokesh Gidra 2025-08-07 1937 if (ret > 0)
3abf06e0c484867 Lokesh Gidra 2025-08-07 1938 step_size = ret;
3abf06e0c484867 Lokesh Gidra 2025-08-07 1939 else
3abf06e0c484867 Lokesh Gidra 2025-08-07 1940 err = ret;
Imagine "ret == 0".
adef440691bab82 Andrea Arcangeli 2023-12-06 1941 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1942
adef440691bab82 Andrea Arcangeli 2023-12-06 1943 cond_resched();
adef440691bab82 Andrea Arcangeli 2023-12-06 1944
adef440691bab82 Andrea Arcangeli 2023-12-06 1945 if (fatal_signal_pending(current)) {
adef440691bab82 Andrea Arcangeli 2023-12-06 1946 /* Do not override an error */
adef440691bab82 Andrea Arcangeli 2023-12-06 1947 if (!err || err == -EAGAIN)
adef440691bab82 Andrea Arcangeli 2023-12-06 1948 err = -EINTR;
adef440691bab82 Andrea Arcangeli 2023-12-06 1949 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1950 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1951
adef440691bab82 Andrea Arcangeli 2023-12-06 1952 if (err) {
err is zero so we don't break here.
adef440691bab82 Andrea Arcangeli 2023-12-06 1953 if (err == -EAGAIN)
adef440691bab82 Andrea Arcangeli 2023-12-06 1954 continue;
adef440691bab82 Andrea Arcangeli 2023-12-06 1955 break;
adef440691bab82 Andrea Arcangeli 2023-12-06 1956 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1957
adef440691bab82 Andrea Arcangeli 2023-12-06 1958 /* Proceed to the next page */
adef440691bab82 Andrea Arcangeli 2023-12-06 @1959 dst_addr += step_size;
^^^^^^^^^
Uninitialized or from the previous iteration.
adef440691bab82 Andrea Arcangeli 2023-12-06 1960 src_addr += step_size;
adef440691bab82 Andrea Arcangeli 2023-12-06 1961 moved += step_size;
adef440691bab82 Andrea Arcangeli 2023-12-06 1962 }
adef440691bab82 Andrea Arcangeli 2023-12-06 1963
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1964 out_unlock:
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1965 up_read(&ctx->map_changing_lock);
867a43a34ff8a38 Lokesh Gidra 2024-02-15 1966 uffd_move_unlock(dst_vma, src_vma);
adef440691bab82 Andrea Arcangeli 2023-12-06 1967 out:
31defc3b01d907e Tal Zussman 2025-06-19 1968 VM_WARN_ON_ONCE(moved < 0);
31defc3b01d907e Tal Zussman 2025-06-19 1969 VM_WARN_ON_ONCE(err > 0);
31defc3b01d907e Tal Zussman 2025-06-19 1970 VM_WARN_ON_ONCE(!moved && !err);
adef440691bab82 Andrea Arcangeli 2023-12-06 1971 return moved ? moved : err;
adef440691bab82 Andrea Arcangeli 2023-12-06 1972 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
2025-08-08 6:18 ` Dan Carpenter
@ 2025-08-08 15:27 ` Lokesh Gidra
0 siblings, 0 replies; 11+ messages in thread
From: Lokesh Gidra @ 2025-08-08 15:27 UTC (permalink / raw)
To: Dan Carpenter; +Cc: oe-kbuild, lkp, oe-kbuild-all
On Thu, Aug 7, 2025 at 11:18 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Hi Lokesh,
>
> kernel test robot noticed the following build warnings:
>
> url: https://github.com/intel-lab-lkp/linux/commits/Lokesh-Gidra/userfaultfd-opportunistic-TLB-flush-batching-for-present-pages-in-MOVE/20250807-184025
> base: 6e64f4580381e32c06ee146ca807c555b8f73e24
> patch link: https://lore.kernel.org/r/20250807103902.2242717-1-lokeshgidra%40google.com
> patch subject: [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE
> config: openrisc-randconfig-r071-20250808 (https://download.01.org/0day-ci/archive/20250808/202508080639.Cf7SyRwQ-lkp@intel.com/config)
> compiler: or1k-linux-gcc (GCC) 10.5.0
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202508080639.Cf7SyRwQ-lkp@intel.com/
>
Thanks Dan. I will fix the warning in the next version of the patch.
> smatch warnings:
> mm/userfaultfd.c:1959 move_pages() error: uninitialized symbol 'step_size'.
>
> vim +/step_size +1959 mm/userfaultfd.c
>
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1802 ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1803 unsigned long src_start, unsigned long len, __u64 mode)
> adef440691bab82 Andrea Arcangeli 2023-12-06 1804 {
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1805 struct mm_struct *mm = ctx->mm;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1806 struct vm_area_struct *src_vma, *dst_vma;
> 3abf06e0c484867 Lokesh Gidra 2025-08-07 1807 unsigned long src_addr, dst_addr, src_end;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1808 pmd_t *src_pmd, *dst_pmd;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1809 long err = -EINVAL;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1810 ssize_t moved = 0;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1811
> adef440691bab82 Andrea Arcangeli 2023-12-06 1812 /* Sanitize the command parameters. */
> 31defc3b01d907e Tal Zussman 2025-06-19 1813 VM_WARN_ON_ONCE(src_start & ~PAGE_MASK);
> 31defc3b01d907e Tal Zussman 2025-06-19 1814 VM_WARN_ON_ONCE(dst_start & ~PAGE_MASK);
> 31defc3b01d907e Tal Zussman 2025-06-19 1815 VM_WARN_ON_ONCE(len & ~PAGE_MASK);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1816
> adef440691bab82 Andrea Arcangeli 2023-12-06 1817 /* Does the address range wrap, or is the span zero-sized? */
> 31defc3b01d907e Tal Zussman 2025-06-19 1818 VM_WARN_ON_ONCE(src_start + len < src_start);
> 31defc3b01d907e Tal Zussman 2025-06-19 1819 VM_WARN_ON_ONCE(dst_start + len < dst_start);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1820
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1821 err = uffd_move_lock(mm, dst_start, src_start, &dst_vma, &src_vma);
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1822 if (err)
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1823 goto out;
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1824
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1825 /* Re-check after taking map_changing_lock */
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1826 err = -EAGAIN;
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1827 down_read(&ctx->map_changing_lock);
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1828 if (likely(atomic_read(&ctx->mmap_changing)))
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1829 goto out_unlock;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1830 /*
> adef440691bab82 Andrea Arcangeli 2023-12-06 1831 * Make sure the vma is not shared, that the src and dst remap
> adef440691bab82 Andrea Arcangeli 2023-12-06 1832 * ranges are both valid and fully within a single existing
> adef440691bab82 Andrea Arcangeli 2023-12-06 1833 * vma.
> adef440691bab82 Andrea Arcangeli 2023-12-06 1834 */
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1835 err = -EINVAL;
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1836 if (src_vma->vm_flags & VM_SHARED)
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1837 goto out_unlock;
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1838 if (src_start + len > src_vma->vm_end)
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1839 goto out_unlock;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1840
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1841 if (dst_vma->vm_flags & VM_SHARED)
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1842 goto out_unlock;
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1843 if (dst_start + len > dst_vma->vm_end)
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1844 goto out_unlock;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1845
> adef440691bab82 Andrea Arcangeli 2023-12-06 1846 err = validate_move_areas(ctx, src_vma, dst_vma);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1847 if (err)
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1848 goto out_unlock;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1849
> 3abf06e0c484867 Lokesh Gidra 2025-08-07 1850 for (src_addr = src_start, dst_addr = dst_start, src_end = src_start + len;
> 3abf06e0c484867 Lokesh Gidra 2025-08-07 1851 src_addr < src_end;) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1852 spinlock_t *ptl;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1853 pmd_t dst_pmdval;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1854 unsigned long step_size;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1855
> adef440691bab82 Andrea Arcangeli 2023-12-06 1856 /*
> adef440691bab82 Andrea Arcangeli 2023-12-06 1857 * Below works because anonymous area would not have a
> adef440691bab82 Andrea Arcangeli 2023-12-06 1858 * transparent huge PUD. If file-backed support is added,
> adef440691bab82 Andrea Arcangeli 2023-12-06 1859 * that case would need to be handled here.
> adef440691bab82 Andrea Arcangeli 2023-12-06 1860 */
> adef440691bab82 Andrea Arcangeli 2023-12-06 1861 src_pmd = mm_find_pmd(mm, src_addr);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1862 if (unlikely(!src_pmd)) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1863 if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1864 err = -ENOENT;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1865 break;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1866 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1867 src_pmd = mm_alloc_pmd(mm, src_addr);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1868 if (unlikely(!src_pmd)) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1869 err = -ENOMEM;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1870 break;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1871 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1872 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1873 dst_pmd = mm_alloc_pmd(mm, dst_addr);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1874 if (unlikely(!dst_pmd)) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1875 err = -ENOMEM;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1876 break;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1877 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1878
> adef440691bab82 Andrea Arcangeli 2023-12-06 1879 dst_pmdval = pmdp_get_lockless(dst_pmd);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1880 /*
> adef440691bab82 Andrea Arcangeli 2023-12-06 1881 * If the dst_pmd is mapped as THP don't override it and just
> adef440691bab82 Andrea Arcangeli 2023-12-06 1882 * be strict. If dst_pmd changes into TPH after this check, the
> adef440691bab82 Andrea Arcangeli 2023-12-06 1883 * move_pages_huge_pmd() will detect the change and retry
> adef440691bab82 Andrea Arcangeli 2023-12-06 1884 * while move_pages_pte() will detect the change and fail.
> adef440691bab82 Andrea Arcangeli 2023-12-06 1885 */
> adef440691bab82 Andrea Arcangeli 2023-12-06 1886 if (unlikely(pmd_trans_huge(dst_pmdval))) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1887 err = -EEXIST;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1888 break;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1889 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1890
> adef440691bab82 Andrea Arcangeli 2023-12-06 1891 ptl = pmd_trans_huge_lock(src_pmd, src_vma);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1892 if (ptl) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1893 /* Check if we can move the pmd without splitting it. */
> adef440691bab82 Andrea Arcangeli 2023-12-06 1894 if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
> adef440691bab82 Andrea Arcangeli 2023-12-06 1895 !pmd_none(dst_pmdval)) {
> e06d03d5590ae1c Matthew Wilcox (Oracle 2024-03-26 1896) struct folio *folio = pmd_folio(*src_pmd);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1897
> 5beaee54a324ba1 Matthew Wilcox (Oracle 2024-03-26 1898) if (!folio || (!is_huge_zero_folio(folio) &&
> eb1521dad8f391d Suren Baghdasaryan 2024-01-31 1899 !PageAnonExclusive(&folio->page))) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1900 spin_unlock(ptl);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1901 err = -EBUSY;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1902 break;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1903 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1904
> adef440691bab82 Andrea Arcangeli 2023-12-06 1905 spin_unlock(ptl);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1906 split_huge_pmd(src_vma, src_pmd, src_addr);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1907 /* The folio will be split by move_pages_pte() */
> adef440691bab82 Andrea Arcangeli 2023-12-06 1908 continue;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1909 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1910
> adef440691bab82 Andrea Arcangeli 2023-12-06 1911 err = move_pages_huge_pmd(mm, dst_pmd, src_pmd,
> adef440691bab82 Andrea Arcangeli 2023-12-06 1912 dst_pmdval, dst_vma, src_vma,
> adef440691bab82 Andrea Arcangeli 2023-12-06 1913 dst_addr, src_addr);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1914 step_size = HPAGE_PMD_SIZE;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1915 } else {
> 3abf06e0c484867 Lokesh Gidra 2025-08-07 1916 long ret;
> 3abf06e0c484867 Lokesh Gidra 2025-08-07 1917
> adef440691bab82 Andrea Arcangeli 2023-12-06 1918 if (pmd_none(*src_pmd)) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1919 if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES)) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1920 err = -ENOENT;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1921 break;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1922 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1923 if (unlikely(__pte_alloc(mm, src_pmd))) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1924 err = -ENOMEM;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1925 break;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1926 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1927 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1928
> adef440691bab82 Andrea Arcangeli 2023-12-06 1929 if (unlikely(pte_alloc(mm, dst_pmd))) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1930 err = -ENOMEM;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1931 break;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1932 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1933
> 3abf06e0c484867 Lokesh Gidra 2025-08-07 1934 ret = move_pages_ptes(mm, dst_pmd, src_pmd,
> 3abf06e0c484867 Lokesh Gidra 2025-08-07 1935 dst_vma, src_vma, dst_addr,
> 3abf06e0c484867 Lokesh Gidra 2025-08-07 1936 src_addr, src_end - src_addr, mode);
> 3abf06e0c484867 Lokesh Gidra 2025-08-07 1937 if (ret > 0)
> 3abf06e0c484867 Lokesh Gidra 2025-08-07 1938 step_size = ret;
> 3abf06e0c484867 Lokesh Gidra 2025-08-07 1939 else
> 3abf06e0c484867 Lokesh Gidra 2025-08-07 1940 err = ret;
>
> Imagine "ret == 0".
>
> adef440691bab82 Andrea Arcangeli 2023-12-06 1941 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1942
> adef440691bab82 Andrea Arcangeli 2023-12-06 1943 cond_resched();
> adef440691bab82 Andrea Arcangeli 2023-12-06 1944
> adef440691bab82 Andrea Arcangeli 2023-12-06 1945 if (fatal_signal_pending(current)) {
> adef440691bab82 Andrea Arcangeli 2023-12-06 1946 /* Do not override an error */
> adef440691bab82 Andrea Arcangeli 2023-12-06 1947 if (!err || err == -EAGAIN)
> adef440691bab82 Andrea Arcangeli 2023-12-06 1948 err = -EINTR;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1949 break;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1950 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1951
> adef440691bab82 Andrea Arcangeli 2023-12-06 1952 if (err) {
>
> err is zero so we don't break here.
>
> adef440691bab82 Andrea Arcangeli 2023-12-06 1953 if (err == -EAGAIN)
> adef440691bab82 Andrea Arcangeli 2023-12-06 1954 continue;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1955 break;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1956 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1957
> adef440691bab82 Andrea Arcangeli 2023-12-06 1958 /* Proceed to the next page */
> adef440691bab82 Andrea Arcangeli 2023-12-06 @1959 dst_addr += step_size;
> ^^^^^^^^^
> Uninitialized or from the previous iteration.
>
> adef440691bab82 Andrea Arcangeli 2023-12-06 1960 src_addr += step_size;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1961 moved += step_size;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1962 }
> adef440691bab82 Andrea Arcangeli 2023-12-06 1963
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1964 out_unlock:
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1965 up_read(&ctx->map_changing_lock);
> 867a43a34ff8a38 Lokesh Gidra 2024-02-15 1966 uffd_move_unlock(dst_vma, src_vma);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1967 out:
> 31defc3b01d907e Tal Zussman 2025-06-19 1968 VM_WARN_ON_ONCE(moved < 0);
> 31defc3b01d907e Tal Zussman 2025-06-19 1969 VM_WARN_ON_ONCE(err > 0);
> 31defc3b01d907e Tal Zussman 2025-06-19 1970 VM_WARN_ON_ONCE(!moved && !err);
> adef440691bab82 Andrea Arcangeli 2023-12-06 1971 return moved ? moved : err;
> adef440691bab82 Andrea Arcangeli 2023-12-06 1972 }
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-08-12 14:02 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-07 22:24 [PATCH v3] userfaultfd: opportunistic TLB-flush batching for present pages in MOVE kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2025-08-07 10:39 Lokesh Gidra
2025-08-07 19:16 ` Peter Xu
2025-08-07 22:54 ` Andrew Morton
2025-08-08 16:41 ` Lokesh Gidra
2025-08-08 16:29 ` Lokesh Gidra
2025-08-10 6:31 ` Lokesh Gidra
2025-08-11 14:00 ` Peter Xu
2025-08-12 14:01 ` Lokesh Gidra
2025-08-08 6:18 ` Dan Carpenter
2025-08-08 15:27 ` Lokesh Gidra
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.