Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 8BIT
To: Eric Anholt <eric@anholt.net>, dri-devel@lists.freedesktop.org
From: Chris Wilson <chris@chris-wilson.co.uk>
In-Reply-To: <20170622205054.31472-1-eric@anholt.net>
Cc: linux-kernel@vger.kernel.org
References: <20170622205054.31472-1-eric@anholt.net>
Message-ID: <150100255722.3991.2029035898362260709@mail.alporthouse.com>
User-Agent: alot/0.3.6
Subject: Re: [PATCH] vc4: Add an ioctl for labeling GEM BOs for summary stats
Date: Tue, 25 Jul 2017 18:09:17 +0100
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4673
Lines: 128

Quoting Eric Anholt (2017-06-22 21:50:54)
> This has proven immensely useful for debugging memory leaks and
> overallocation (which is a rather serious concern on the platform,
> given that we typically run at about 256MB of CMA out of up to 1GB
> total memory, with framebuffers that are about 8MB ecah).
> 
> The state of the art without this is to dump debug logs from every GL
> application, guess as to kernel allocations based on bo_stats, and try
> to merge that all together into a global picture of memory allocation
> state.  With this, you can add a couple of calls to the debug build of
> the 3D driver and get a pretty detailed view of GPU memory usage from
> /debug/dri/0/bo_stats (or when we debug print to dmesg on allocation
> failure).
> 
> The Mesa side currently labels at the gallium resource level (so you
> see that a 1920x20 pixmap has been created, presumably for the window
> system panel), but we could extend that to be even more useful with
> glObjectLabel() names being sent all the way down to the kernel.
> 
> (partial) example of sorted debugfs output with Mesa labeling all
> resources:
> 
>                kernel BO cache:  16392kb BOs (3)
>        tiling shadow 1920x1080:   8160kb BOs (1)
>        resource 1920x1080@32/0:   8160kb BOs (1)
> scanout resource 1920x1080@32/0:   8100kb BOs (1)
>                         kernel:   8100kb BOs (1)
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---

>  static void vc4_bo_stats_dump(struct vc4_dev *vc4)
>  {

Now unused?

> -       DRM_INFO("num bos allocated: %d\n",
> -                vc4->bo_stats.num_allocated);
> -       DRM_INFO("size bos allocated: %dkb\n",
> -                vc4->bo_stats.size_allocated / 1024);
> -       DRM_INFO("num bos used: %d\n",
> -                vc4->bo_stats.num_allocated - vc4->bo_stats.num_cached);
> -       DRM_INFO("size bos used: %dkb\n",
> -                (vc4->bo_stats.size_allocated -
> -                 vc4->bo_stats.size_cached) / 1024);
> -       DRM_INFO("num bos cached: %d\n",
> -                vc4->bo_stats.num_cached);
> -       DRM_INFO("size bos cached: %dkb\n",
> -                vc4->bo_stats.size_cached / 1024);
> +       int i;
> +
> +       for (i = 0; i < vc4->num_labels; i++) {
> +               if (!vc4->bo_labels[i].num_allocated)
> +                       continue;
> +
> +               DRM_INFO("%30s: %6dkb BOs (%d)\n",
> +                        vc4->bo_labels[i].name,
> +                        vc4->bo_labels[i].size_allocated / 1024,
> +                        vc4->bo_labels[i].num_allocated);
> +       }
>  }
>  
>  #ifdef CONFIG_DEBUG_FS

> +/* Must be called with bo_lock held. */
> +static void vc4_bo_set_label(struct drm_gem_object *gem_obj, int label)
> +{
> +       struct vc4_bo *bo = to_vc4_bo(gem_obj);
> +       struct vc4_dev *vc4 = to_vc4_dev(gem_obj->dev);

lockdep_assert_held(&vc4->bo_lock);

> +
> +       vc4->bo_labels[label].num_allocated++;
> +       vc4->bo_labels[label].size_allocated += gem_obj->size;

This gets called with label=-1 on destroy.

> +       vc4->bo_labels[bo->label].num_allocated--;
> +       vc4->bo_labels[bo->label].size_allocated -= gem_obj->size;

Ok, preassigned to TYPE_KERNEL on creation.

> +       if (vc4->bo_labels[bo->label].num_allocated == 0 &&
> +           is_user_label(bo->label)) {
> +               /* Free user BO label slots on last unreference. */
> +               kfree(vc4->bo_labels[bo->label].name);
> +               vc4->bo_labels[bo->label].name = NULL;
> +       }

This seems dubious. As a user I would expect the label I created to last
until I closed the fd. Otherwise if I ever close all bo of one type,
wait a few seconds for the cache to be reaped, then reallocate a new bo,
someone else may have assigned my label a new name.

If that's guarded against, a comment would help.

> +
> +       bo->label = label;
> +}

> +int vc4_label_bo_ioctl(struct drm_device *dev, void *data,
> +                      struct drm_file *file_priv)
> +{
> +       struct vc4_dev *vc4 = to_vc4_dev(dev);
> +       struct drm_vc4_label_bo *args = data;
> +       char *name;
> +       struct drm_gem_object *gem_obj;
> +       int ret = 0, label;
> +
> +       name = kmalloc(args->len + 1, GFP_KERNEL);
> +       if (!name)
> +               return -ENOMEM;
> +
> +       if (copy_from_user(name, (void __user *)(uintptr_t)args->name,
> +                          args->len)) {
> +               kfree(name);
> +               return -EFAULT;
> +       }
> +       name[args->len] = 0;

if (!args->len)
	return -EINVAL;

name = strndup_user(u64_to_user_ptr(args->name), args->len + 1);
if (IS_ERR(name))
	return PTR_ERR(name);
-Chris