On Tue, Apr 11, 2017 at 12:50:11PM -0600, Alex Williamson wrote:
> On Tue, 11 Apr 2017 12:27:55 -0600
> Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:
>
> > On Tue, 11 Apr 2017 19:03:14 +0800
> > Peter Xu <peterx@xxxxxxxxxx> wrote:
> >
> > > On Thu, Apr 06, 2017 at 08:53:43AM -0600, Alex Williamson wrote:
[...]
> > > > -static void vfio_lock_acct(struct task_struct *task, long npage)
> > > > -{
> > > > - struct vwork *vwork;
> > > > struct mm_struct *mm;
> > > > bool is_current;
> > > > + int ret;
> > > >
> > > > if (!npage)
> > > > - return;
> > > > + return 0;
> > > >
> > > > is_current = (task->mm == current->mm);
> > > >
> > > > mm = is_current ? task->mm : get_task_mm(task);
> > >
> > > A question besides current patch: could I ask why we need to take
> > > special care for is_current? I see that is only used to only try avoid
> > > get_task_mm() when proper, but is get_task_mm() a heavy operation?
> >
> > Yes, it's slower, performance was significantly degraded when mdev
> > support was introduced and imposed get_task_mm() on all calling paths.
I see. Thanks.
> >
> > > > if (!mm)
> > > > - return; /* process exited */
> > > > + return -ESRCH; /* process exited */
> > > >
> > > > - if (down_write_trylock(&mm->mmap_sem)) {
> > > > - mm->locked_vm += npage;
> > > > - up_write(&mm->mmap_sem);
> > > > - if (!is_current)
> > > > - mmput(mm);
> > > > - return;
> > > > - }
> > > > + ret = down_write_killable(&mm->mmap_sem);
> > > > + if (!ret) {
> > > > + if (npage < 0) {
> > > > + mm->locked_vm += npage;
> > > > + } else {
> > > > + unsigned long limit;
> > > > +
> > > > + limit = is_current ?
> > > > + rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT :
> > > > + task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> > >
> > > Maybe we can directly use task_rlimit() here? Since looks like
> > > rlimit() is calling it as well, with "current".
> >
> > We could, but does it actually change anything? rlimit() is static
> > inline, so using task_rlimit() for both just moves the is_current
> > ternary into the task_rlimit() argument. Is this:
> >
> > limit = task_rlimit(is_current ? current : task,
> > RLIMIT_MEMLOCK) >> PAGE_SHIFT);
> >
> > notably cleaner than above?
>
> Ah, maybe you were suggesting that we can just use "task" here for
> both since it's always correct. Thanks,
Yes it is.
[...]
> > > > out:
> > > > - vfio_lock_acct(current, lock_acct);
> > > > + ret = vfio_lock_acct(current, lock_acct);
> > > > +
> > > > +unpin_out:
> > > > + if (ret) {
> > > > + if (!rsvd) {
> > > > + for (pfn = *pfn_base ; pinned ; pfn++, pinned--)
> > > > + put_pfn(pfn, dma->prot);
> > > > + }
> > > > +
> > > > + return ret;
> > > > + }
> > >
> > > The change in vfio_pin_pages_remote() seems to contain a functional
> > > change totally not related to the subject (IIUC, we are going to unpin
> > > those pages if the huge page can only be pinned partially, and we are
> > > not doing that before)? If so, would it be nice to split current patch
> > > into two, or at least mention this behavior change in commit log of
> > > this patch?
> >
> >
> > This is only tangentially about hugepages, this loop is looking for
> > contiguous pages regardless of the processor or IOMMU page size
> > support.
It should somewhat related to huge pages? At least we have
disable_hugepages parameter, and as well in vfio_pin_pages_remote() we
have:
if (unlikely(disable_hugepages))
goto out;
So the loop will be skipped if that is specified.
> > We're trying to make as few calls to iommu_map() as we can
> > and thus we want the maximum range of IOVA to physical address we can
> > pump into the IOMMU driver. It's up to the IOMMU driver to figure out
> > how it can optimize that range with hugepages or superpages in its page
> > tables. So the caller of this function is looping over a range of
> > memory and each time this function returns, it maps that many pages
> > through the iommu. On success we return <=npage.
> >
> > The unpin_out loop here is absolutely related to the change proposed
> > here, vfio_lock_acct() can fail, we cannot return both an error and pin
> > pages, therefore we need to undo anything we've pinned this round.
Yes you are right. It's related.
> >
> > Are you perhaps only referring to the exit path above going straight to
> > this loop rather than attempting to do the accounting for the pages
> > pinned so far? Previously that was our only option because the unwind
> > path was to return a short count, invoking the page accounting and
> > iommu_mapping, while fully expecting the caller to again loop over the
> > excess page, return -ENOMEM, and teardown the entire mapping request.
> > So because we now require an unwind path for the vfio_lock_acct()
> > change, we can now do the teardown w/o the additional pinning here and
> > mapping by the caller. In a very strict sense, we could consider that
> > a separate change and move those 3 lines to a follow-on patch but
> > ultimately the caller did request pinned pages beyond what we believe
> > their limit to be and making use of this new exit path here saves us a
> > useless accounting and mapping iteration. I can note that in the
> > commit log. Thanks,
I agree that in all cases it's a corner case and trivial, and the
userspace caller should anyway do something to release its memory
accounting.
Thanks for addressing my comments and replied with full detail!
--
Peter Xu
