Re: [PATCH v1 5/5] drm/virtio: Add memory shrinker

2022-03-09 Thread Dmitry Osipenko 


On 3/9/22 04:12, Rob Clark wrote:
>> +static unsigned long
>> +virtio_gpu_gem_shrinker_count_objects(struct shrinker *shrinker,
>> + struct shrink_control *sc)
>> +{
>> +   struct drm_gem_shmem_object *shmem;
>> +   struct virtio_gpu_device *vgdev;
>> +   unsigned long count = 0;
>> +   bool empty = true;
>> +
>> +   vgdev = container_of(shrinker, struct virtio_gpu_device,
>> +vgshrinker.shrinker);
>> +
>> +   if (!mutex_trylock(>mm_lock))
>> +   return 0;
> One bit of advice from previously dealing with shrinker and heavy
> memory pressure situations (turns out 4GB chromebooks can be pretty
> much under *constant* memory pressure):
> 
> You *really* want to make shrinker->count_objects lockless.. and
> minimize the lock contention on shrinker->scan_objects (ie.  The
> problem is you can end up with shrinking going on on all CPU cores in
> parallel, you want to not funnel that thru one lock as much as
> possible.
> 
> See in particular:
> 
> 25ed38b3ed26 ("drm/msm: Drop mm_lock in scan loop")
> cc8a4d5a1bd8 ("drm/msm: Avoid mutex in shrinker_count()")

Thank you, I'll take that into account for v2.

Re: [PATCH v1 5/5] drm/virtio: Add memory shrinker

2022-03-08 Thread Rob Clark 
On Tue, Mar 8, 2022 at 5:17 AM Dmitry Osipenko
 wrote:
>
> Add memory shrinker and new madvise IOCTL to the VirtIO-GPU driver.
> Userspace (BO cache manager of Mesa driver) will mark BOs as "don't need"
> using the new IOCTL to let shrinker purge the marked BOs on OOM, thus
> shrinker will lower memory pressure and prevent OOM kills.
>
> Signed-off-by: Daniel Almeida 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/gpu/drm/virtio/Makefile   |   3 +-
>  drivers/gpu/drm/virtio/virtgpu_drv.h  |  26 +++-
>  drivers/gpu/drm/virtio/virtgpu_gem.c  |  84 
>  drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c | 124 ++
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c|  37 ++
>  drivers/gpu/drm/virtio/virtgpu_kms.c  |  10 ++
>  drivers/gpu/drm/virtio/virtgpu_object.c   |   7 +
>  drivers/gpu/drm/virtio/virtgpu_plane.c|  17 ++-
>  drivers/gpu/drm/virtio/virtgpu_vq.c   |  15 +++
>  include/uapi/drm/virtgpu_drm.h|  14 ++
>  10 files changed, 333 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c
>

[snip]

> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c 
> b/drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c
> new file mode 100644
> index ..39eb9a3e7e4a
> --- /dev/null
> +++ b/drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Collabora Ltd.
> + */
> +
> +#include 
> +#include 
> +
> +#include "virtgpu_drv.h"
> +
> +static unsigned long
> +virtio_gpu_gem_shrinker_count_objects(struct shrinker *shrinker,
> + struct shrink_control *sc)
> +{
> +   struct drm_gem_shmem_object *shmem;
> +   struct virtio_gpu_device *vgdev;
> +   unsigned long count = 0;
> +   bool empty = true;
> +
> +   vgdev = container_of(shrinker, struct virtio_gpu_device,
> +vgshrinker.shrinker);
> +
> +   if (!mutex_trylock(>mm_lock))
> +   return 0;

One bit of advice from previously dealing with shrinker and heavy
memory pressure situations (turns out 4GB chromebooks can be pretty
much under *constant* memory pressure):

You *really* want to make shrinker->count_objects lockless.. and
minimize the lock contention on shrinker->scan_objects (ie.  The
problem is you can end up with shrinking going on on all CPU cores in
parallel, you want to not funnel that thru one lock as much as
possible.

See in particular:

25ed38b3ed26 ("drm/msm: Drop mm_lock in scan loop")
cc8a4d5a1bd8 ("drm/msm: Avoid mutex in shrinker_count()")

BR,
-R

> +   list_for_each_entry(shmem, >vgshrinker.list, madv_list) {
> +   empty = false;
> +
> +   if (!mutex_trylock(>pages_lock))
> +   continue;
> +
> +   if (drm_gem_shmem_is_purgeable(shmem))
> +   count += shmem->base.size >> PAGE_SHIFT;
> +
> +   mutex_unlock(>pages_lock);
> +   }
> +
> +   mutex_unlock(>mm_lock);
> +
> +   return empty ? SHRINK_EMPTY : count;
> +}
> +
> +static bool virtio_gpu_gem_shrinker_purge(struct virtio_gpu_device *vgdev,
> + struct drm_gem_object *obj)
> +{
> +   struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
> +   struct drm_gem_shmem_object *shmem = >base;
> +   int err;
> +
> +   if (!dma_resv_test_signaled(obj->resv, true) ||
> +   !drm_gem_shmem_is_purgeable(shmem) ||
> +   refcount_read(>pin_count))
> +   return false;
> +
> +   /*
> +* Release host's memory before guest's memory is gone to ensure that
> +* host won't touch released memory of the guest.
> +*/
> +   err = virtio_gpu_gem_host_mem_release(bo);
> +   if (err)
> +   return false;
> +
> +   list_del_init(>madv_list);
> +   drm_gem_shmem_purge_locked(shmem);
> +
> +   return true;
> +}
> +
> +static unsigned long
> +virtio_gpu_gem_shrinker_scan_objects(struct shrinker *shrinker,
> +struct shrink_control *sc)
> +{
> +   struct drm_gem_shmem_object *shmem, *tmp;
> +   struct virtio_gpu_device *vgdev;
> +   unsigned long freed = 0;
> +
> +   vgdev = container_of(shrinker, struct virtio_gpu_device,
> +vgshrinker.shrinker);
> +
> +   if (!mutex_trylock(>mm_lock))
> +   return SHRINK_STOP;
> +
> +   list_for_each_entry_safe(shmem, tmp, >vgshrinker.list, 
> madv_list) {
> +   if (freed >= sc->nr_to_scan)
> +   break;
> +
> +   if (!dma_resv_trylock(shmem->base.resv))
> +   continue;
> +
> +   if (!mutex_trylock(>pages_lock))
> +   goto resv_unlock;
> +
> +   if (virtio_gpu_gem_shrinker_purge(vgdev, >base))
> +

Re: [PATCH v1 5/5] drm/virtio: Add memory shrinker

2022-03-08 Thread Dmitry Osipenko 


On 3/8/22 16:17, Dmitry Osipenko wrote:
> @@ -246,20 +246,28 @@ static int virtio_gpu_plane_prepare_fb(struct drm_plane 
> *plane,
>   struct virtio_gpu_device *vgdev = dev->dev_private;
>   struct virtio_gpu_framebuffer *vgfb;
>   struct virtio_gpu_object *bo;
> + int err;
>  
>   if (!new_state->fb)
>   return 0;
>  
>   vgfb = to_virtio_gpu_framebuffer(new_state->fb);
>   bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
> - if (!bo || (plane->type == DRM_PLANE_TYPE_PRIMARY && !bo->guest_blob))
> +
> + err = virtio_gpu_gem_pin(bo);
> + if (err)
> + return err;

I just noticed that this produces a refcount debug warning because I
missed to initialize the refcount when BO is created. That warning splat
was hidden by a huge lockdep splat produced by
drm_aperture_remove_conflicting_pci_framebuffers(), which probably
should be fixed. I'll correct it in v2.

[PATCH v1 5/5] drm/virtio: Add memory shrinker

2022-03-08 Thread Dmitry Osipenko 
Add memory shrinker and new madvise IOCTL to the VirtIO-GPU driver.
Userspace (BO cache manager of Mesa driver) will mark BOs as "don't need"
using the new IOCTL to let shrinker purge the marked BOs on OOM, thus
shrinker will lower memory pressure and prevent OOM kills.

Signed-off-by: Daniel Almeida 
Signed-off-by: Dmitry Osipenko 
---
 drivers/gpu/drm/virtio/Makefile   |   3 +-
 drivers/gpu/drm/virtio/virtgpu_drv.h  |  26 +++-
 drivers/gpu/drm/virtio/virtgpu_gem.c  |  84 
 drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c | 124 ++
 drivers/gpu/drm/virtio/virtgpu_ioctl.c|  37 ++
 drivers/gpu/drm/virtio/virtgpu_kms.c  |  10 ++
 drivers/gpu/drm/virtio/virtgpu_object.c   |   7 +
 drivers/gpu/drm/virtio/virtgpu_plane.c|  17 ++-
 drivers/gpu/drm/virtio/virtgpu_vq.c   |  15 +++
 include/uapi/drm/virtgpu_drm.h|  14 ++
 10 files changed, 333 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c

diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index b99fa4a73b68..e1715ae02a2d 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -6,6 +6,7 @@
 virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
virtgpu_display.o virtgpu_vq.o \
virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
-   virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
+   virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o \
+   virtgpu_gem_shrinker.o
 
 obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h 
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index b2d93cb12ebf..f907fcd81a24 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -94,6 +94,8 @@ struct virtio_gpu_object {
 
int uuid_state;
uuid_t uuid;
+
+   refcount_t pin_count;
 };
 #define gem_to_virtio_gpu_obj(gobj) \
container_of((gobj), struct virtio_gpu_object, base.base)
@@ -211,6 +213,11 @@ struct virtio_gpu_drv_cap_cache {
atomic_t is_valid;
 };
 
+struct virtio_gpu_shrinker {
+   struct shrinker shrinker;
+   struct list_head list;
+};
+
 struct virtio_gpu_device {
struct drm_device *ddev;
 
@@ -261,6 +268,11 @@ struct virtio_gpu_device {
spinlock_t resource_export_lock;
/* protects map state and host_visible_mm */
spinlock_t host_visible_lock;
+
+   struct virtio_gpu_shrinker vgshrinker;
+
+   /* protects all memory management operations */
+   struct mutex mm_lock;
 };
 
 struct virtio_gpu_fpriv {
@@ -274,7 +286,7 @@ struct virtio_gpu_fpriv {
 };
 
 /* virtgpu_ioctl.c */
-#define DRM_VIRTIO_NUM_IOCTLS 12
+#define DRM_VIRTIO_NUM_IOCTLS 13
 extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
 void virtio_gpu_create_context(struct drm_device *dev, struct drm_file *file);
 
@@ -310,6 +322,12 @@ void virtio_gpu_array_put_free(struct 
virtio_gpu_object_array *objs);
 void virtio_gpu_array_put_free_delayed(struct virtio_gpu_device *vgdev,
   struct virtio_gpu_object_array *objs);
 void virtio_gpu_array_put_free_work(struct work_struct *work);
+int virtio_gpu_array_validate(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_object_array *objs);
+bool virtio_gpu_gem_madvise(struct virtio_gpu_object *obj, int madv);
+int virtio_gpu_gem_host_mem_release(struct virtio_gpu_object *bo);
+int virtio_gpu_gem_pin(struct virtio_gpu_object *bo);
+void virtio_gpu_gem_unpin(struct virtio_gpu_object *bo);
 
 /* virtgpu_vq.c */
 int virtio_gpu_alloc_vbufs(struct virtio_gpu_device *vgdev);
@@ -321,6 +339,8 @@ void virtio_gpu_cmd_create_resource(struct 
virtio_gpu_device *vgdev,
struct virtio_gpu_fence *fence);
 void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev,
   struct virtio_gpu_object *bo);
+int virtio_gpu_cmd_release_resource(struct virtio_gpu_device *vgdev,
+   struct virtio_gpu_object *bo);
 void virtio_gpu_cmd_transfer_to_host_2d(struct virtio_gpu_device *vgdev,
uint64_t offset,
uint32_t width, uint32_t height,
@@ -483,4 +503,8 @@ void virtio_gpu_vram_unmap_dma_buf(struct device *dev,
   struct sg_table *sgt,
   enum dma_data_direction dir);
 
+/* virtgpu_gem_shrinker.c */
+int virtio_gpu_gem_shrinker_init(struct virtio_gpu_device *vgdev);
+void virtio_gpu_gem_shrinker_fini(struct virtio_gpu_device *vgdev);
+
 #endif
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c 
b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 48d3c9955f0d..dbe5b8fa8285 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++