Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20230914152620.2743033-1-surenb@google.com> <20230914152620.2743033-3-surenb@google.com>
 <ZQNMze6SXdIm13CW@casper.infradead.org> <e77b75f9-ab9e-f20b-6484-22f73524c159@redhat.com>
 <f6e350f4-1bf3-ca10-93f8-c11db44ce62b@redhat.com> <CAJuCfpGqt1V5puRMhLkjG6F2T4xtsDY8qy--ZfBPNL9kxPyWtg@mail.gmail.com>
 <354f2508-74d5-2723-502c-32d009f77a3e@redhat.com>
In-Reply-To: <354f2508-74d5-2723-502c-32d009f77a3e@redhat.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Thu, 21 Sep 2023 18:57:42 -0700
Message-ID: <CAJuCfpFk6tfP=nJng4G1dSsSEy-piQUSAShrVdBJzXBH=YF3aQ@mail.gmail.com>
Subject: Re: [PATCH 2/3] userfaultfd: UFFDIO_REMAP uABI
To:     David Hildenbrand <david@redhat.com>
Cc:     Matthew Wilcox <willy@infradead.org>, akpm@linux-foundation.org,
        viro@zeniv.linux.org.uk, brauner@kernel.org, shuah@kernel.org,
        aarcange@redhat.com, lokeshgidra@google.com, peterx@redhat.com,
        hughd@google.com, mhocko@suse.com, axelrasmussen@google.com,
        rppt@kernel.org, Liam.Howlett@oracle.com, jannh@google.com,
        zhangpeng362@huawei.com, bgeffon@google.com,
        kaleshsingh@google.com, ngeoffray@google.com, jdduke@google.com,
        linux-mm@kvack.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Sep 21, 2023 at 11:17=E2=80=AFAM David Hildenbrand <david@redhat.co=
m> wrote:
>
> On 21.09.23 20:04, Suren Baghdasaryan wrote:
> > On Thu, Sep 14, 2023 at 6:45=E2=80=AFPM David Hildenbrand <david@redhat=
.com> wrote:
> >>
> >> On 14.09.23 20:43, David Hildenbrand wrote:
> >>> On 14.09.23 20:11, Matthew Wilcox wrote:
> >>>> On Thu, Sep 14, 2023 at 08:26:12AM -0700, Suren Baghdasaryan wrote:
> >>>>> +++ b/include/linux/userfaultfd_k.h
> >>>>> @@ -93,6 +93,23 @@ extern int mwriteprotect_range(struct mm_struct =
*dst_mm,
> >>>>>     extern long uffd_wp_range(struct vm_area_struct *vma,
> >>>>>                        unsigned long start, unsigned long len, bool=
 enable_wp);
> >>>>>
> >>>>> +/* remap_pages */
> >>>>> +extern void double_pt_lock(spinlock_t *ptl1, spinlock_t *ptl2);
> >>>>> +extern void double_pt_unlock(spinlock_t *ptl1, spinlock_t *ptl2);
> >>>>> +extern ssize_t remap_pages(struct mm_struct *dst_mm,
> >>>>> +                      struct mm_struct *src_mm,
> >>>>> +                      unsigned long dst_start,
> >>>>> +                      unsigned long src_start,
> >>>>> +                      unsigned long len, __u64 flags);
> >>>>> +extern int remap_pages_huge_pmd(struct mm_struct *dst_mm,
> >>>>> +                           struct mm_struct *src_mm,
> >>>>> +                           pmd_t *dst_pmd, pmd_t *src_pmd,
> >>>>> +                           pmd_t dst_pmdval,
> >>>>> +                           struct vm_area_struct *dst_vma,
> >>>>> +                           struct vm_area_struct *src_vma,
> >>>>> +                           unsigned long dst_addr,
> >>>>> +                           unsigned long src_addr);
> >>>>
> >>>> Drop the 'extern' markers from function declarations.
> >>>>
> >>>>> +int remap_pages_huge_pmd(struct mm_struct *dst_mm,
> >>>>> +                    struct mm_struct *src_mm,
> >>>>> +                    pmd_t *dst_pmd, pmd_t *src_pmd,
> >>>>> +                    pmd_t dst_pmdval,
> >>>>> +                    struct vm_area_struct *dst_vma,
> >>>>> +                    struct vm_area_struct *src_vma,
> >>>>> +                    unsigned long dst_addr,
> >>>>> +                    unsigned long src_addr)
> >>>>> +{
> >>>>> +   pmd_t _dst_pmd, src_pmdval;
> >>>>> +   struct page *src_page;
> >>>>> +   struct anon_vma *src_anon_vma, *dst_anon_vma;
> >>>>> +   spinlock_t *src_ptl, *dst_ptl;
> >>>>> +   pgtable_t pgtable;
> >>>>> +   struct mmu_notifier_range range;
> >>>>> +
> >>>>> +   src_pmdval =3D *src_pmd;
> >>>>> +   src_ptl =3D pmd_lockptr(src_mm, src_pmd);
> >>>>> +
> >>>>> +   BUG_ON(!pmd_trans_huge(src_pmdval));
> >>>>> +   BUG_ON(!pmd_none(dst_pmdval));
> >>>>> +   BUG_ON(!spin_is_locked(src_ptl));
> >>>>> +   mmap_assert_locked(src_mm);
> >>>>> +   mmap_assert_locked(dst_mm);
> >>>>> +   BUG_ON(src_addr & ~HPAGE_PMD_MASK);
> >>>>> +   BUG_ON(dst_addr & ~HPAGE_PMD_MASK);
> >>>>> +
> >>>>> +   src_page =3D pmd_page(src_pmdval);
> >>>>> +   BUG_ON(!PageHead(src_page));
> >>>>> +   BUG_ON(!PageAnon(src_page));
> >>>>
> >>>> Better to add a src_folio =3D page_folio(src_page);
> >>>> and then folio_test_anon() here.
> >>>>
> >>>>> +   if (unlikely(page_mapcount(src_page) !=3D 1)) {
> >>>>
> >>>> Brr, this is going to miss PTE mappings of this folio.  I think you
> >>>> actually want folio_mapcount() instead, although it'd be more effici=
ent
> >>>> to look at folio->_entire_mapcount =3D=3D 1 and _nr_pages_mapped =3D=
=3D 0.
> >>>> Not wure what a good name for that predicate would be.
> >>>
> >>> We have
> >>>
> >>>     * It only works on non shared anonymous pages because those can
> >>>     * be relocated without generating non linear anon_vmas in the rma=
p
> >>>     * code.
> >>>     *
> >>>     * It provides a zero copy mechanism to handle userspace page faul=
ts.
> >>>     * The source vma pages should have mapcount =3D=3D 1, which can b=
e
> >>>     * enforced by using madvise(MADV_DONTFORK) on src vma.
> >>>
> >>> Use PageAnonExclusive(). As long as KSM is not involved and you don't
> >>> use fork(), that flag should be good enough for that use case here.
> >>>
> >> ... and similarly don't do any of that swapcount stuff and only check =
if
> >> the swap pte is anon exclusive.
> >
> > I'm preparing v2 and this is the only part left for me to address but
> > I'm not clear how. David, could you please clarify how I should be
> > checking swap pte to be exclusive without swapcount?
>
> If you have a real swp pte (not a non-swap pte like migration entries)
> you should be able to just use pte_swp_exclusive().

Got it. Thanks!

>
> --
> Cheers,
>
> David / dhildenb
>