Re: [PATCH] kvm: Faults which trigger IO release the mmap_sem

[Andres Lagar-Cavilla <andreslc@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2651 bytes --]

On Tue, Sep 16, 2014 at 6:51 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 15/09/2014 22:11, Andres Lagar-Cavilla ha scritto:
> > +     if (!locked) {
> > +             BUG_ON(npages != -EBUSY);
>
> VM_BUG_ON perhaps?
>
Sure.


> > @@ -1177,9 +1210,15 @@ static int hva_to_pfn_slow(unsigned long addr,
> bool *async, bool write_fault,
> >               npages = get_user_page_nowait(current, current->mm,
> >                                             addr, write_fault, page);
> >               up_read(&current->mm->mmap_sem);
> > -     } else
> > -             npages = get_user_pages_fast(addr, 1, write_fault,
> > -                                          page);
> > +     } else {
> > +             /*
> > +              * By now we have tried gup_fast, and possible async_pf,
> and we
> > +              * are certainly not atomic. Time to retry the gup,
> allowing
> > +              * mmap semaphore to be relinquished in the case of IO.
> > +              */
> > +             npages = kvm_get_user_page_retry(current, current->mm,
> addr,
> > +                                              write_fault, page);
>
> This is a separate logical change.  Was this:
>
>         down_read(&mm->mmap_sem);
>         npages = get_user_pages(NULL, mm, addr, 1, 1, 0, NULL, NULL);
>         up_read(&mm->mmap_sem);
>
> the intention rather than get_user_pages_fast?
>

Nope. The intention was to pass FAULT_FLAG_RETRY to the vma fault handler
(without _NOWAIT). And once you do that, if you come back without holding
the mmap sem, you need to call yet again.

By that point in the call chain I felt comfortable dropping the _fast. All
paths that get there have already tried _fast (and some have tried _NOWAIT).


> I think a first patch should introduce kvm_get_user_page_retry ("Retry a
> fault after a gup with FOLL_NOWAIT.") and the second would add
> FOLL_TRIED ("This properly relinquishes mmap semaphore if the
> filemap/swap has to wait on page lock (and retries the gup to completion
> after that").
>

That's not what FOLL_TRIED does. The relinquishing of mmap semaphore is
done by this patch minus the FOLL_TRIED bits. FOLL_TRIED will let the fault
handler (e.g. filemap) know that we've been there and waited on the IO
already, so in the common case we won't need to redo the IO.

Have a look at how FAULT_FLAG_TRIED is used in e.g. arch/x86/mm/fault.c.


>
> Apart from this, the patch looks good.  The mm/ parts are minimal, so I
> think it's best to merge it through the KVM tree with someone's Acked-by.
>

Thanks!
Andres


>
> Paolo
>



-- 
Andres Lagar-Cavilla | Google Cloud Platform | andreslc@google.com |
 647-778-4380

[-- Attachment #2: Type: text/html, Size: 5021 bytes --]