Re: [RFC PATCH 3/3] drm/virtio: implement blob userptr resource object

["Huang, Honglei1" <Honglei1.Huang@amd.com>]

On 2024/12/27 10:02, Huang, Honglei1 wrote:
> On 2024/12/22 9:59, Demi Marie Obenour wrote:
>> On 12/20/24 10:35 AM, Simona Vetter wrote:
>>> On Fri, Dec 20, 2024 at 06:04:09PM +0800, Honglei Huang wrote:
>>>> From: Honglei Huang<Honglei1.Huang@amd.com>
>>>>
>>>> A virtio-gpu userptr is based on HMM notifier.
>>>> Used for let host access guest userspace memory and
>>>> notice the change of userspace memory.
>>>> This series patches are in very beginning state,
>>>> User space are pinned currently to ensure the host
>>>> device memory operations are correct.
>>>> The free and unmap operations for userspace can be
>>>> handled by MMU notifier this is a simple and basice
>>>> SVM feature for this series patches.
>>>> The physical PFNS update operations is splited into
>>>> two OPs in here. The evicted memories won't be used
>>>> anymore but remap into host again to achieve same
>>>> effect with hmm_rang_fault.
>>> So in my opinion there are two ways to implement userptr that make sense:
>>>
>>> - pinned userptr with pin_user_pages(FOLL_LONGTERM). there is not mmu
>>>    notifier
>>>
>>> - unpinnned userptr where you entirely rely on userptr and do not hold any
>>>    page references or page pins at all, for full SVM integration. This
>>>    should use hmm_range_fault ideally, since that's the version that
>>>    doesn't ever grab any page reference pins.
>>>
>>> All the in-between variants are imo really bad hacks, whether they hold a
>>> page reference or a temporary page pin (which seems to be what you're
>>> doing here). In much older kernels there was some justification for them,
>>> because strange stuff happened over fork(), but with FOLL_LONGTERM this is
>>> now all sorted out. So there's really only fully pinned, or true svm left
>>> as clean design choices imo.
>>>
>>> With that background, why does pin_user_pages(FOLL_LONGTERM) not work for
>>> you?
>> +1 on using FOLL_LONGTERM.  Fully dynamic memory management has a huge cost
>> in complexity that pinning everything avoids.  Furthermore, this avoids the
>> host having to take action in response to guest memory reclaim requests.
>> This avoids additional complexity (and thus attack surface) on the host side.
>> Furthermore, since this is for ROCm and not for graphics, I am less concerned
>> about supporting systems that require swappable GPU VRAM.
> Hi Sima and Demi, I totally agree the flag FOLL_LONGTERM is needed, I 
> will add it in next version. And for the first pin variants 
> implementation, the MMU notifier is also needed I think.
> Cause the userptr feature in UMD generally used like this: the 
> registering of userptr
> always is explicitly invoked by user code like 
> "registerMemoryToGPU(userptrAddr, ...)",
> but for the userptr release/free, there is no explicit API for it, at 
> least in hsakmt/KFD stack.
> User just need call system call "free(userptrAddr)", thenkernel driver 
> will release the userptr
> by MMU notifier callback.Virtio-GPU has no other way to know if user has 
> been free the userptr
> except for MMU notifior.And in UMD theres is no way to get the free() 
> operation is invoked by user.
> the only way is use MMU notifierin virtio-GPU driver and free the 
> corresponding data in host
> by some virtio CMDs as far as I can see.
> And for the second way that is use hmm_range_fault, there is a 
> predictable issues as far as I can see, at least in hsakmt/KFD stack. 
> That is the memory may migrate when GPU/device is working. In bare 
> metal, when memory is migrating KFD driver will pause the compute work 
> of the device in mmap_wirte_lock then use hmm_range_fault to remap the 
> migrated/evicted memories to GPU then restore the compute work of device 
> to ensure the correction of the data. But in virtio-GPU driver the 
> migration happen in guest kernel, the evict mmu notifier callback 
> happens in guest, a virtio CMD can be used for notify host but as lack 
> of mmap_write_lock protection in host kernel, host will hold invalid 
> data for a short period of time, this may lead to some issues. And it is 
> hard to fix as far as I can see.
> Finally I will extract some APIs into helper according to your request, 
> and I will refactor the whole userptr
> implementation, use some callbacks in page getting path, let the pin 
> method and hmm_range_fault can be choiced
> in this series patches.
> 
> Regards,
> Honglei

Hi Sima,

I modified the code, remove all the MMU nitifior and use 
pin_user_pages_fast only. Under this implementation userptr fully
managed by UMD. We did a performance test, it decreased by 30% in
OpenCL stack in Geekbench6 benmark.
We use AMD V2000 for test:
use MMU notifior + pin_user_pages:
near 13000 score: https://browser.geekbench.com/v6/compute/3257793

use pin_user_pages only:
near 10000 socre: https://browser.geekbench.com/v6/compute/3496228

The code is under clean up, I will send out later.

And I found a another thing, it seems like in intel i915 userptr
implementation, the pin_user_pages is also used in MMU notifior.
Code path is: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/i915/gem/i915_gem_userptr.c?h=v6.13#:~:text=ret%20%3D%20pin_user_pages_fast(obj%2D%3Euserptr.ptr%20%2B%20pinned%20*%20PAGE_SIZE%2C

Patch set: 
https://lore.kernel.org/all/159353552439.22701.14005121342739071590@emeril.freedesktop.org/T/
https://patchwork.kernel.org/project/intel-gfx/patch/20210323155059.628690-17-maarten.lankhorst@linux.intel.com/#24064663

And I didn't find the hmm_range_fault code path, maybe I missed it?

Regards,
Honglei