Re: [Linaro-mm-sig] [PATCH v3] dmabuf: Add the capability to expose DMA-BUF stats in sysfs

[Sumit Semwal]

On Thu, 28 Jan 2021 at 17:23, Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> wrote:
>
> Am 28.01.21 um 12:00 schrieb Sumit Semwal:
> > Hi Hridya,
> >
> > On Wed, 27 Jan 2021 at 17:36, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > 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.

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

>
> 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@xxxxxxxxxxxxxxxxxxx>
> > Best,
> > Sumit.
> > _______________________________________________
> > Linaro-mm-sig mailing list
> > Linaro-mm-sig@xxxxxxxxxxxxxxxx
> > 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