On Wed, Feb 3, 2021 at 2:25 AM Daniel Vetter <daniel@xxxxxxxx> wrote: > > On Mon, Feb 01, 2021 at 01:02:30PM -0800, Hridya Valsaraju wrote: > > On Mon, Feb 1, 2021 at 10:37 AM Daniel Vetter <daniel@xxxxxxxx> wrote: > > > > > > 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"). > > > > Thank you for taking another look Daniel! > > > > I will try adding an IGT test for the sysfs files. Other than the > > documentation in > > Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers(included in the > > patch), is there another place you would like to see the documentation > > copied to? > > So just read the other thread, and sounds like Christian König brought up > a solid concern with dma-buf fds generally not staying around for much. Thank you for the reply Daniel! Could you please elaborate on the connection with the other thread you mentioned? I am a little confused since this patch does not deal with tracking DMA-BUF fds. Regards, Hridya > > So I'm leaning more towards "this sounds like it's going to be useful for > Android only, nothing else" concern. > -Daniel > > > > > Regards, > > Hridya > > > > > > > > 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 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
