Virtualization - Nov 2021 - [PATCH] drm/virtio: delay pinning the pages till first use

On Tue, Nov 02, 2021 at 12:31:39PM +0100, Maksym Wezdecki
wrote:
> From: mwezdeck <maksym.wezdecki at collabora.co.uk>
> 
> The idea behind the commit:
>   1. not pin the pages during resource_create ioctl
>   2. pin the pages on the first use during:
> 	- transfer_*_host ioctl
>         - map ioctl
i.e. basically lazy pinning.  Approach looks sane to me.
>   3. introduce new ioctl for pinning pages on demand
What is the use case for this ioctl?
In any case this should be a separate patch.
> +	struct virtio_gpu_object_array *objs;
> +	struct virtio_gpu_object *bo;
> +	struct virtio_gpu_object_shmem *shmem;
> +
> +	objs = virtio_gpu_array_from_handles(file,
&virtio_gpu_map->handle, 1);
> +	if (objs == NULL)
> +		return -ENOENT;
> +
> +	bo = gem_to_virtio_gpu_obj(objs->objs[0]);
> +	if (bo == NULL)
> +		return -ENOENT;
> +	
> +	shmem = to_virtio_gpu_shmem(bo);
> +	if (shmem == NULL)
> +		return -ENOENT;
> +
> +	if (!shmem->pages) {
> +		virtio_gpu_object_pin(vgdev, objs, 1);
> +	}
Move this into virtio_gpu_object_pin(),
or create a helper function for it ...
> +	objs = virtio_gpu_array_from_handles(file,
&virtio_gpu_pin->handle, 1);
> +	if (objs == NULL)
> +		return -ENOENT;
> +
> +	bo = gem_to_virtio_gpu_obj(objs->objs[0]);
> +	if (bo == NULL)
> +		return -ENOENT;
> +	
> +	shmem = to_virtio_gpu_shmem(bo);
> +	if (shmem == NULL)
> +		return -ENOENT;
> +
> +	if (!shmem->pages) {
> +		return virtio_gpu_object_pin(vgdev, objs, 1);
> +	}
... to avoid this code duplication?
> +int virtio_gpu_object_pin(struct virtio_gpu_device *vgdev,
> +			  struct virtio_gpu_object_array *objs,
> +			  int num_gem_objects)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < num_gem_objects; i++) {
> +		ret = virtio_gpu_object_shmem_init(vgdev, bo, &ents, &nents);
> +		if (ret != 0) {
> +			return -EFAULT;
> +		}
> +
> +		virtio_gpu_object_attach(vgdev, bo, ents, nents);
I think it is enough to do the virtio_gpu_object_attach() call lazily.
virtio_gpu_object_shmem_init() should not actually allocate pages, that
only happens when virtio_gpu_object_attach() goes ask for a scatter
list.

take care,
  Gerd

On Tue, Nov 2, 2021 at 6:07 AM Gerd Hoffmann <kraxel at redhat.com>
wrote:
>
> On Tue, Nov 02, 2021 at 12:31:39PM +0100, Maksym Wezdecki wrote:
> > From: mwezdeck <maksym.wezdecki at collabora.co.uk>
> >
> > The idea behind the commit:
> >   1. not pin the pages during resource_create ioctl
> >   2. pin the pages on the first use during:
> >       - transfer_*_host ioctl
> >         - map ioctl
>
> i.e. basically lazy pinning.  Approach looks sane to me.
>
> >   3. introduce new ioctl for pinning pages on demand
>
> What is the use case for this ioctl?
> In any case this should be a separate patch.
Lazy pinning can be a nice optimization that userspace does not
necessarily need to know about.  This patch however skips pinning for
execbuffer ioctl and introduces a new pin ioctl instead.  That is a
red flag.