lkml.org 
[lkml]   [2021]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v7 4/6] userfaultfd: add UFFDIO_CONTINUE ioctl
On Tue, Feb 23, 2021 at 10:05:49AM -0800, Axel Rasmussen wrote:
> On Tue, Feb 23, 2021 at 7:38 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Thu, Feb 18, 2021 at 04:48:22PM -0800, Axel Rasmussen wrote:
> > > @@ -4645,8 +4646,18 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> > > spinlock_t *ptl;
> > > int ret;
> > > struct page *page;
> > > + int writable;
> > >
> > > - if (!*pagep) {
> > > + mapping = dst_vma->vm_file->f_mapping;
> > > + idx = vma_hugecache_offset(h, dst_vma, dst_addr);
> > > +
> > > + if (is_continue) {
> > > + ret = -EFAULT;
> > > + page = find_lock_page(mapping, idx);
> > > + *pagep = NULL;
> >
> > Why set *pagep to NULL? Shouldn't it be NULL always?.. If that's the case,
> > maybe WARN_ON_ONCE(*pagep) suite more.
>
> Right, the caller should be passing in NULL in the
> MCOPY_ATOMIC_CONTINUE case. Looking more closely at the caller
> (__mcopy_atomic_hugetlb), it already has a BUG_ON(page), so at best
> this assignment is redundant, and at worst it might actually cover up
> a real bug (say the caller mistakenly *did* pass in some page, we'd
> set it to NULL and the BUG_ON wouldn't trigger).
>
> So, I'll just remove this - I don't think an additional WARN_ON_ONCE
> is needed given the existing BUG_ON.

It's still okay to have the WARN_ON_ONCE; it gives a direct hint that *pagep
should never be set for uffdio_continue. No strong opinion.

>
> >
> > Otherwise the patch looks good to me.
>
> Shall I add a R-B? :)

Yes, as long as "*pagep = NULL" dropped, please feel free to. :)

Thanks,

--
Peter Xu

\
 
 \ /
  Last update: 2021-02-23 19:32    [W:0.038 / U:20.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site