From: Eric Anholt <eric@anholt.net>
To: Chris Wilson <chris@chris-wilson.co.uk>,
        dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] vc4: Add an ioctl for labeling GEM BOs for summary stats
In-Reply-To: <150100255722.3991.2029035898362260709@mail.alporthouse.com>
References: <20170622205054.31472-1-eric@anholt.net> <150100255722.3991.2029035898362260709@mail.alporthouse.com>
User-Agent: Notmuch/0.22.2+1~gb0bcfaa (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu)
Date: Tue, 25 Jul 2017 11:27:25 -0700
Message-ID: <87inigqsmq.fsf@eliezer.anholt.net>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
        micalg=pgp-sha512; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6566
Lines: 176

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

Chris Wilson <chris@chris-wilson.co.uk> writes:

> 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).
>>=20
>> 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).
>>=20
>> 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.
>>=20
>> (partial) example of sorted debugfs output with Mesa labeling all
>> resources:
>>=20
>>                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)
>>=20
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>
>>  static void vc4_bo_stats_dump(struct vc4_dev *vc4)
>>  {
>
> Now unused?

Still used from the splat when CMA allocation fails.  It's less
catastrophic than it used to be, but still bad, so we're dumping to
dmesg for now.

>> -       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 =3D 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);
>> +       }
>>  }
>>=20=20
>>  #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 =3D to_vc4_bo(gem_obj);
>> +       struct vc4_dev *vc4 =3D to_vc4_dev(gem_obj->dev);
>
> lockdep_assert_held(&vc4->bo_lock);

Nice.  I've converted the other instances of this comment, too.

>> +
>> +       vc4->bo_labels[label].num_allocated++;
>> +       vc4->bo_labels[label].size_allocated +=3D gem_obj->size;
>
> This gets called with label=3D-1 on destroy.

Oh, good catch, thanks.  This code got reworked a couple of times and I
lost that check.

>> +       vc4->bo_labels[bo->label].num_allocated--;
>> +       vc4->bo_labels[bo->label].size_allocated -=3D gem_obj->size;
>
> Ok, preassigned to TYPE_KERNEL on creation.
>
>> +       if (vc4->bo_labels[bo->label].num_allocated =3D=3D 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 =3D 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.

Userspace can't see the label index, though, and can only set string
names on BOs.  New text:

		/* Free user BO label slots on last unreference.
		 * Slots are just where we track the stats for a given
		 * name, and once a name is unused we can reuse that
		 * slot.
		 */

>> +
>> +       bo->label =3D label;
>> +}
>
>> +int vc4_label_bo_ioctl(struct drm_device *dev, void *data,
>> +                      struct drm_file *file_priv)
>> +{
>> +       struct vc4_dev *vc4 =3D to_vc4_dev(dev);
>> +       struct drm_vc4_label_bo *args =3D data;
>> +       char *name;
>> +       struct drm_gem_object *gem_obj;
>> +       int ret =3D 0, label;
>> +
>> +       name =3D 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] =3D 0;
>
> if (!args->len)
> 	return -EINVAL;
>
> name =3D strndup_user(u64_to_user_ptr(args->name), args->len + 1);
> if (IS_ERR(name))
> 	return PTR_ERR(name);
> -Chris

Oh, nice.  I've converted the code I was copying from to use
u64_to_user_ptr(), too.

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAll3jY0ACgkQtdYpNtH8
nuhz9BAAtTXhAjaTPGSZwkhY3rN5dYKGp9kBNLHjIv4s0J0wSleclsFKYz5Z/mKD
iHcYpfialwpQwX4nDhgxwQgzfZ0Rmng/Q+sReEVDyj2Gu7zCwr/4i7ao+Ttae4Pp
b74d+3cCRA23HOgrLNd0ufcBDHHYbB1TVy0QPBD3gQFzEIikl0icpaHX/LcLBtkf
/mGX2Zl+CDHU7JkqG5Lt2doMMy4iXAl6Wns6QLr1Zsfsumy9jqHHOn/vQm91FmBZ
i0e9TTZnsAHUHhVX8ZlkEPf29z3TYhLskprxrdjJy4uVnqsPHJf7Nxdwn+o07Vto
t3LoVBaM2GTcKf7YNjFoRZ/CPB5rq+t5/kW6q/hapbhhQot/OID+KrkLJKn/RosL
EEPGTop/9jb3Q4l0mc1FG58rycNGFTvL4dJdeJmdPJxGRCVzO5ztDhuUC8M4pRTQ
DxNgs7aRGlSaGNpQ9uk7FHZ9BWtxW8zQa7B+5RmEKrmwfb6aobBUVaKglr3shRXv
eYppuaP2riPIrsgOd78uS5P5hsIqVYYFILFG+MZDUH9wx7+02Z+rtX9HBUbhgIZY
C8I6VYEh2+R//V40yuIgmZWUqUbNfpoWtOCAZbt2oFvPzfRPJ0dgfk++o0iG8Utw
i4fa+R9D6oouP6f7eLrAqn/p1Gk3wZs6NxAucf2CVgdv9f2tk1w=
=K1D2
-----END PGP SIGNATURE-----
--=-=-=--