Message-ID: <CAKMK7uEwm5tLT3fo_+QtzUthht3JLkhCpZ+6yJ2XSB6U4Qp5wg@mail.gmail.com>
Date:   Mon, 1 Feb 2021 19:37:18 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Sumit Semwal <sumit.semwal@...aro.org>
Cc:     Christian Koenig <christian.koenig@....com>,
        Android Kernel Team <kernel-team@...roid.com>,
        kernel test robot <lkp@...el.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        DRI mailing list <dri-devel@...ts.freedesktop.org>,
        Linaro MM SIG <linaro-mm-sig@...ts.linaro.org>,
        hyesoo.yu@...sung.com, Hridya Valsaraju <hridya@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        "open list:DMA BUFFER SHARING FRAMEWORK" 
        <linux-media@...r.kernel.org>
Subject: Re: [Linaro-mm-sig] [PATCH v3] dmabuf: Add the capability to expose
 DMA-BUF stats in sysfs

On Thu, Jan 28, 2021 at 1:03 PM Sumit Semwal <sumit.semwal@...aro.org> wrote:
>
> On Thu, 28 Jan 2021 at 17:23, Christian König
> <ckoenig.leichtzumerken@...il.com> wrote:
> >
> > Am 28.01.21 um 12:00 schrieb Sumit Semwal:
> > > Hi Hridya,
> > >
> > > On Wed, 27 Jan 2021 at 17:36, Greg KH <gregkh@...uxfoundation.org> wrote:
> > >> On Tue, Jan 26, 2021 at 12:42:36PM -0800, Hridya Valsaraju wrote:
> > >>> This patch allows statistics to be enabled for each DMA-BUF in
> > >>> sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS.
> > >>>
> > >>> The following stats will be exposed by the interface:
> > >>>
> > >>> /sys/kernel/dmabuf/buffers/<inode_number>/exporter_name
> > >>> /sys/kernel/dmabuf/buffers/<inode_number>/size
> > >>> /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attach_uid>/device
> > >>> /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attach_uid>/map_counter
> > >>>
> > >>> The inode_number is unique for each DMA-BUF and was added earlier [1]
> > >>> in order to allow userspace to track DMA-BUF usage across different
> > >>> processes.
> > >>>
> > >>> Currently, this information is exposed in
> > >>> /sys/kernel/debug/dma_buf/bufinfo.
> > >>> However, since debugfs is considered unsafe to be mounted in production,
> > >>> it is being duplicated in sysfs.
> > >>>
> > >>> This information will be used to derive DMA-BUF
> > >>> per-exporter stats and per-device usage stats for Android Bug reports.
> > >>> The corresponding userspace changes can be found at [2].
> > >>> Telemetry tools will also capture this information(along with other
> > >>> memory metrics) periodically as well as on important events like a
> > >>> foreground app kill (which might have been triggered by Low Memory
> > >>> Killer). It will also contribute to provide a snapshot of the system
> > >>> memory usage on other events such as OOM kills and Application Not
> > >>> Responding events.
> > >>>
> > >>> A shell script that can be run on a classic Linux environment to read
> > >>> out the DMA-BUF statistics can be found at [3](suggested by John
> > >>> Stultz).
> > >>>
> > >>> The patch contains the following improvements over the previous version:
> > >>> 1) Each attachment is represented by its own directory to allow creating
> > >>> a symlink to the importing device and to also provide room for future
> > >>> expansion.
> > >>> 2) The number of distinct mappings of each attachment is exposed in a
> > >>> separate file.
> > >>> 3) The per-buffer statistics are now in /sys/kernel/dmabuf/buffers
> > >>> inorder to make the interface expandable in future.
> > >>>
> > >>> All of the improvements above are based on suggestions/feedback from
> > >>> Daniel Vetter and Christian König.
> > >>>
> > >>> [1]: https://lore.kernel.org/patchwork/patch/1088791/
> > >>> [2]: https://android-review.googlesource.com/q/topic:%22dmabuf-sysfs%22+(status:open%20OR%20status:merged)
> > >>> [3]: https://android-review.googlesource.com/c/platform/system/memory/libmeminfo/+/1549734
> > >>>
> > >>> Signed-off-by: Hridya Valsaraju <hridya@...gle.com>
> > >>> Reported-by: kernel test robot <lkp@...el.com>
> > > Thanks for the patch!
> > >
> > > Christian: If you're satisfied with the explanation around not
> > > directly embedding kobjects into the dma_buf and dma_buf_attachment
> > > structs, then with Greg's r-b from sysfs PoV, I think we can merge it.
> > > Please let me know if you feel otherwise!
> >
> >  From the technical side it looks clean to me, feel free to add my
> > acked-by while pushing.
> >
> > But I would at least try to convince Daniel on the design. At least some
> > of his concerns seems to be valid and keep in mind that we need to
> > support this interface forever.
>
> Naturally.
>
> Since he didn't comment over Hridya's last clarification about the
> tracepoints to track total GPU memory allocations being orthogonal to
> this series, I assumed he agreed with it.

The tracepoint being orthogonal didn't really look convincing to me,
since I do expect we'll need that at a much more generic level, at
allocators. Whether that's dma-buf heaps or in drm or wherever. And we
probably also need that to somehow align with cgroups accounting.

But I guess for this it should be easy to extend however we see fit,
so retrofitting allocator sources and anything else we want/need for
the overall gpu memory account shouldn't be a problem. Also, it's
first, so the proof for showing it all works together is more on the
tracepoints :-)

> Daniel, do you still have objections around adding this patch in?

Needs docs (especially the uapi I think would be useful to document),
igt tests, that kind of stuff still I think? It's meant to be generic
uapi across drivers, generally we're a pile stricter for that (and yes
dma-buf heaps I think didn't do all that, so maybe there's an argument
for doing this a bit more sloppy or at least "the testsuite is
somewhere else").

But I think it would be good to have this all done.
-Daniel

>
> >
> > Regards,
> > Christian.
>
> Best,
> Sumit.
> >
> > >
> > >>> ---
> > >>> Changes in v3:
> > >>> Fix a warning reported by the kernel test robot.
> > >>>
> > >>> Changes in v2:
> > >>> -Move statistics to /sys/kernel/dmabuf/buffers in oder to allow addition
> > >>> of other DMA-BUF-related sysfs stats in future. Based on feedback from
> > >>> Daniel Vetter.
> > >>> -Each attachment has its own directory to represent attaching devices as
> > >>> symlinks and to introduce map_count as a separate file. Based on
> > >>> feedback from Daniel Vetter and Christian König. Thank you both!
> > >>> -Commit messages updated to point to userspace code in AOSP that will
> > >>> read the DMA-BUF sysfs stats.
> > >>>
> > >>>
> > >>>   .../ABI/testing/sysfs-kernel-dmabuf-buffers   |  52 ++++
> > >>>   drivers/dma-buf/Kconfig                       |  11 +
> > >>>   drivers/dma-buf/Makefile                      |   1 +
> > >>>   drivers/dma-buf/dma-buf-sysfs-stats.c         | 285 ++++++++++++++++++
> > >>>   drivers/dma-buf/dma-buf-sysfs-stats.h         |  62 ++++
> > >>>   drivers/dma-buf/dma-buf.c                     |  37 +++
> > >>>   include/linux/dma-buf.h                       |  20 ++
> > >>>   7 files changed, 468 insertions(+)
> > >>>   create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers
> > >>>   create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c
> > >>>   create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h
> > >> I don't know the dma-buf code at all, but from a sysfs/kobject point of
> > >> view, this patch looks good to me:
> > >>
> > >> Reviewed-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > > Best,
> > > Sumit.
> > > _______________________________________________
> > > Linaro-mm-sig mailing list
> > > Linaro-mm-sig@...ts.linaro.org
> > > https://lists.linaro.org/mailman/listinfo/linaro-mm-sig
> >
>
>
> --
> Thanks and regards,
>
> Sumit Semwal
> Linaro Consumer Group - Tech Lead
> Linaro.org │ Open source software for ARM SoCs
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists