Re: [PATCH v3 3/3] vfio-pci: Invalidate mmaps and block MMIO access on disabled memory

John Hubbard
2020-05-26T00:46:41Z

On 2020-05-25 17:37, Jason Gunthorpe wrote:
...
>> commit 318b275fbca1ab9ec0862de71420e0e92c3d1aa7
>> Author: Gleb Natapov <gleb@redhat.com>
>> Date: Tue Mar 22 16:30:51 2011 -0700
>>
>> mm: allow GUP to fail instead of waiting on a page
>>
>> GUP user may want to try to acquire a reference to a page if it is already
>> in memory, but not if IO, to bring it in, is needed. For example KVM may
>> tell vcpu to schedule another guest process if current one is trying to
>> access swapped out page. Meanwhile, the page will be swapped in and the
>> guest process, that depends on it, will be able to run again.
>>
>> This patch adds FAULT_FLAG_RETRY_NOWAIT (suggested by Linus) and
>> FOLL_NOWAIT follow_page flags. FAULT_FLAG_RETRY_NOWAIT, when used in
>> conjunction with VM_FAULT_ALLOW_RETRY, indicates to handle_mm_fault that
>> it shouldn't drop mmap_sem and wait on a page, but return VM_FAULT_RETRY
>> instead.
>
> So, from kvm's perspective it was to avoid excessively long blocking in
> common paths when it could rejoin the completed IO by somehow waiting
> on a page itself?

Or perhaps some variation on that, such as just retrying with an intervening
schedule() call. It's not clear just from that commit.

>
> It all seems like it should not be used unless the page is going to go
> to IO?

That's my conclusion so far, yes.

>
> Certainly there is no reason to optimize the fringe case of vfio
> sleeping if there is and incorrect concurrnent attempt to disable the
> a BAR.

Definitely agree with that position.

>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 8429d5aa31e44..e32e8e52a57ac 100644
>> +++ b/include/linux/mm.h
>> @@ -430,6 +430,15 @@ extern pgprot_t protection_map[16];
>> * continuous faults with flags (b). We should always try to detect pending
>> * signals before a retry to make sure the continuous page faults can still be
>> * interrupted if necessary.
>> + *
>> + * About @FAULT_FLAG_RETRY_NOWAIT: this is intended for callers who would like
>> + * to acquire a page, but only if the page is already in memory. If, on the
>> + * other hand, the page requires IO in order to bring it into memory, then fault
>> + * handlers will immediately return VM_FAULT_RETRY ("don't wait"), while leaving
>> + * mmap_lock held ("don't drop mmap_lock"). For example, this is useful for
>> + * virtual machines that have multiple guests running: if guest A attempts
>> + * get_user_pages() on a swapped out page, another guest can be scheduled while
>> + * waiting for IO to swap in guest A's page.
>> */
>> #define FAULT_FLAG_WRITE 0x01
>> #define FAULT_FLAG_MKWRITE 0x02
>
> It seems reasonable but people might complain about the kvm
> specifics of the explanation.
>
> It might be better to explain how the caller is supposed to know when
> it is OK to try GUP again and expect success, as it seems to me this
> is really about externalizing the sleep for page wait?
>

OK, good point. The example was helpful in the commit log, but not quite
appropriate in mm.h, yes.

thanks,
--
John Hubbard
NVIDIA