On Thu, Jan 28, 2021 at 1:03 PM Sumit Semwal <sumit.semwal@xxxxxxxxxx> wrote: > > 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. 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@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 > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
