linux-kernel - Re: [PATCH v4] drm/virtio: Track total GPU memory for virtio driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKB3++bLzn5YVaR3iUTHtNYwBeG6Z27NjtWZ1q-xmjOkApAQwg@mail.gmail.com>
Date:   Thu, 21 Jan 2021 23:58:22 -0800
From:   Yiwei Zhang‎ <zzyiwei@...roid.com>
To:     David Airlie <airlied@...ux.ie>, Gerd Hoffmann <kraxel@...hat.com>,
        Daniel Vetter <daniel@...ll.ch>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        "open list:VIRTIO CORE, NET..." 
        <virtualization@...ts.linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        kernel-team <kernel-team@...roid.com>
Subject: Re: [PATCH v4] drm/virtio: Track total GPU memory for virtio driver

On Thu, Jan 21, 2021 at 9:40 PM Yiwei Zhang <zzyiwei@...roid.com> wrote:
>
> On the success of virtio_gpu_object_create, add size of newly allocated
> bo to the tracked total_mem. In drm_gem_object_funcs.free, after the gem
> bo loses its last refcount, subtract the bo size from the tracked
> total_mem if the original underlying memory allocation is successful.
>
> It's more accurate to do this in device driver layer to best match when
> the underlying resource gets allocated and destroyed during tracing.
>
> Signed-off-by: Yiwei Zhang <zzyiwei@...roid.com>
> ---
>  drivers/gpu/drm/virtio/Kconfig          |  1 +
>  drivers/gpu/drm/virtio/virtgpu_drv.h    |  2 ++
>  drivers/gpu/drm/virtio/virtgpu_object.c | 11 +++++++++++
>  3 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
> index b925b8b1da16..e103b7e883b1 100644
> --- a/drivers/gpu/drm/virtio/Kconfig
> +++ b/drivers/gpu/drm/virtio/Kconfig
> @@ -5,6 +5,7 @@ config DRM_VIRTIO_GPU
>         select DRM_KMS_HELPER
>         select DRM_GEM_SHMEM_HELPER
>         select VIRTIO_DMA_SHARED_BUFFER
> +       select TRACE_GPU_MEM
>         help
>            This is the virtual GPU driver for virtio.  It can be used with
>            QEMU based VMMs (like KVM or Xen).
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 6a232553c99b..c5622f9b591f 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -249,6 +249,8 @@ struct virtio_gpu_device {
>         spinlock_t resource_export_lock;
>         /* protects map state and host_visible_mm */
>         spinlock_t host_visible_lock;
> +
> +       atomic64_t total_mem;
>  };
>
>  struct virtio_gpu_fpriv {
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
> index d69a5b6da553..e2251fc41509 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -25,12 +25,21 @@
>
>  #include <linux/dma-mapping.h>
>  #include <linux/moduleparam.h>
> +#include <trace/events/gpu_mem.h>
>
>  #include "virtgpu_drv.h"
>
>  static int virtio_gpu_virglrenderer_workaround = 1;
>  module_param_named(virglhack, virtio_gpu_virglrenderer_workaround, int, 0400);
>
> +static inline void virtio_gpu_trace_total_mem(struct virtio_gpu_device *vgdev,
> +                                             s64 delta)
> +{
> +       u64 total_mem = atomic64_add_return(delta, &vgdev->total_mem);
> +
> +       trace_gpu_mem_total(vgdev->ddev->primary->index, 0, total_mem);
> +}
> +
>  int virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev, uint32_t *resid)
>  {
>         if (virtio_gpu_virglrenderer_workaround) {
> @@ -104,6 +113,7 @@ static void virtio_gpu_free_object(struct drm_gem_object *obj)
>         struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private;
>
>         if (bo->created) {
> +               virtio_gpu_trace_total_mem(vgdev, -(obj->size));
>                 virtio_gpu_cmd_unref_resource(vgdev, bo);
>                 virtio_gpu_notify(vgdev);
>                 /* completion handler calls virtio_gpu_cleanup_object() */
> @@ -265,6 +275,7 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
>                 virtio_gpu_object_attach(vgdev, bo, ents, nents);
>         }
>
> +       virtio_gpu_trace_total_mem(vgdev, shmem_obj->base.size);
>         *bo_ptr = bo;
>         return 0;
>
> --
> 2.30.0.280.ga3ce27912f-goog
>

Re Gerd and Daniel:

I'm not sure why we want to couple this patch too much with the
dma-bufs tracking. The tracepoint added here itself is pretty useful
for tracking gem bo total usage in virtio gpu upon tracing. The
original purpose for integrating this tracepoint in all Android gpu
kernel drivers is to just track total gpu memory usage and serve the
accurate data to game developers in a much easier way. It's something
they can rely on for robust testing and regression monitoring.

The only overlap with the dma-buf side is when we export a bo via
prime to a dma-buf. But still, the total here is already useful for
this particular device. Using which approach to account for the
overlap wouldn't block this small integration from my understanding.

Besides, there's no plan for adding per-process gem total tracking in
virtio-gpu at this moment. This patch should be light enough to carry
without worrying about tech debt I believe.

Many thanks!
Yiwei