| Message ID | 20190227035448.117169-1-fengc@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Improve the dma-buf tracking | expand |
Hello Chenbo,Thank you for your RFC series. On Wed, 27 Feb 2019 at 09:24, Chenbo Feng <fengc@google.com> wrote: > > Currently, all dma-bufs share the same anonymous inode. While we can count > how many dma-buf fds or mappings a process has, we can't get the size of > the backing buffers or tell if two entries point to the same dma-buf. And > in debugfs, we can get a per-buffer breakdown of size and reference count, > but can't tell which processes are actually holding the references to each > buffer. > > To resolve the issue above and provide better method for userspace to track > the dma-buf usage across different processes, the following changes are > proposed in dma-buf kernel side. First of all, replace the singleton inode > inside the dma-buf subsystem with a mini-filesystem, and assign each > dma-buf a unique inode out of this filesystem. With this change, calling > stat(2) on each entry gives the caller a unique ID (st_ino), the buffer's > size (st_size), and even the number of pages assigned to each dma-buffer. > Secoundly, add the inode information to /sys/kernel/debug/dma_buf/bufinfo > so in the case where a buffer is mmap()ed into a process’s address space > but all remaining fds have been closed, we can still get the dma-buf > information and try to accociate it with the process by searching the > proc/pid/maps and looking for the corresponding inode number exposed in > dma-buf debug fs. Thirdly, created an ioctl to assign names to dma-bufs > which lets userspace assign short names (e.g., "CAMERA") to buffers. This > information can be extremely helpful for tracking and accounting shared > buffers based on their usage and original purpose. Last but not least, add > dma-buf information to /proc/pid/fdinfo by adding a show_fdinfo() handler > to dma_buf_file_operations. The handler will print the file_count and name > of each buffer. In general, I think I like the idea as it contributes to a much more relevant usage analysis of dma-buf backed buffers. I will get to doing a more detailed review soon, but immediately, we might want to think a bit about the get/set_name IOCTLS - do we need to think of disallowing multiple renaming of buffers once they start getting used? It could otherwise make the whole metrics a lot confused? > > Greg Hackmann (3): > dma-buf: give each buffer a full-fledged inode > dma-buf: add DMA_BUF_{GET,SET}_NAME ioctls > dma-buf: add show_fdinfo handler > > drivers/dma-buf/dma-buf.c | 121 ++++++++++++++++++++++++++++++++--- > include/linux/dma-buf.h | 5 +- > include/uapi/linux/dma-buf.h | 4 ++ > include/uapi/linux/magic.h | 1 + > 4 files changed, 122 insertions(+), 9 deletions(-) > > -- > 2.21.0.rc2.261.ga7da99ff1b-goog > Best, Sumit.
On Thu, Mar 14, 2019 at 10:49 AM Sumit Semwal <sumit.semwal@linaro.org> wrote: > > Hello Chenbo,Thank you for your RFC series. > > On Wed, 27 Feb 2019 at 09:24, Chenbo Feng <fengc@google.com> wrote: > > > > Currently, all dma-bufs share the same anonymous inode. While we can count > > how many dma-buf fds or mappings a process has, we can't get the size of > > the backing buffers or tell if two entries point to the same dma-buf. And > > in debugfs, we can get a per-buffer breakdown of size and reference count, > > but can't tell which processes are actually holding the references to each > > buffer. > > > > To resolve the issue above and provide better method for userspace to track > > the dma-buf usage across different processes, the following changes are > > proposed in dma-buf kernel side. First of all, replace the singleton inode > > inside the dma-buf subsystem with a mini-filesystem, and assign each > > dma-buf a unique inode out of this filesystem. With this change, calling > > stat(2) on each entry gives the caller a unique ID (st_ino), the buffer's > > size (st_size), and even the number of pages assigned to each dma-buffer. > > Secoundly, add the inode information to /sys/kernel/debug/dma_buf/bufinfo > > so in the case where a buffer is mmap()ed into a process’s address space > > but all remaining fds have been closed, we can still get the dma-buf > > information and try to accociate it with the process by searching the > > proc/pid/maps and looking for the corresponding inode number exposed in > > dma-buf debug fs. Thirdly, created an ioctl to assign names to dma-bufs > > which lets userspace assign short names (e.g., "CAMERA") to buffers. This > > information can be extremely helpful for tracking and accounting shared > > buffers based on their usage and original purpose. Last but not least, add > > dma-buf information to /proc/pid/fdinfo by adding a show_fdinfo() handler > > to dma_buf_file_operations. The handler will print the file_count and name > > of each buffer. > > In general, I think I like the idea as it contributes to a much more > relevant usage analysis of dma-buf backed buffers. > I will get to doing a more detailed review soon, but immediately, we > might want to think a bit about the get/set_name IOCTLS - do we need > to think of disallowing multiple renaming of buffers once they start > getting used? It could otherwise make the whole metrics a lot > confused? Yes, I agree we need to control the dma-buf naming to make sure it doesn't confuse the user. Ideally the process asked for dma-buf need to make sure they only set the name once when they are using the buffer for the same purpose. But I guess we can add check in kernel side to make sure the name only get initialized once and can never change after it. Or do we have better way to disallow multiple renaming of buffers once they start getting used? Can we find a way to set the name information in dma_buf_export so no one can change it after that? > > > > > Greg Hackmann (3): > > dma-buf: give each buffer a full-fledged inode > > dma-buf: add DMA_BUF_{GET,SET}_NAME ioctls > > dma-buf: add show_fdinfo handler > > > > drivers/dma-buf/dma-buf.c | 121 ++++++++++++++++++++++++++++++++--- > > include/linux/dma-buf.h | 5 +- > > include/uapi/linux/dma-buf.h | 4 ++ > > include/uapi/linux/magic.h | 1 + > > 4 files changed, 122 insertions(+), 9 deletions(-) > > > > -- > > 2.21.0.rc2.261.ga7da99ff1b-goog > > > > Best, > Sumit.
On Thu, Mar 14, 2019 at 6:49 PM Sumit Semwal <sumit.semwal@linaro.org> wrote: > > Hello Chenbo,Thank you for your RFC series. > > On Wed, 27 Feb 2019 at 09:24, Chenbo Feng <fengc@google.com> wrote: > > > > Currently, all dma-bufs share the same anonymous inode. While we can count > > how many dma-buf fds or mappings a process has, we can't get the size of > > the backing buffers or tell if two entries point to the same dma-buf. And > > in debugfs, we can get a per-buffer breakdown of size and reference count, > > but can't tell which processes are actually holding the references to each > > buffer. > > > > To resolve the issue above and provide better method for userspace to track > > the dma-buf usage across different processes, the following changes are > > proposed in dma-buf kernel side. First of all, replace the singleton inode > > inside the dma-buf subsystem with a mini-filesystem, and assign each > > dma-buf a unique inode out of this filesystem. With this change, calling > > stat(2) on each entry gives the caller a unique ID (st_ino), the buffer's > > size (st_size), and even the number of pages assigned to each dma-buffer. > > Secoundly, add the inode information to /sys/kernel/debug/dma_buf/bufinfo > > so in the case where a buffer is mmap()ed into a process’s address space > > but all remaining fds have been closed, we can still get the dma-buf > > information and try to accociate it with the process by searching the > > proc/pid/maps and looking for the corresponding inode number exposed in > > dma-buf debug fs. Thirdly, created an ioctl to assign names to dma-bufs > > which lets userspace assign short names (e.g., "CAMERA") to buffers. This > > information can be extremely helpful for tracking and accounting shared > > buffers based on their usage and original purpose. Last but not least, add > > dma-buf information to /proc/pid/fdinfo by adding a show_fdinfo() handler > > to dma_buf_file_operations. The handler will print the file_count and name > > of each buffer. > > In general, I think I like the idea as it contributes to a much more > relevant usage analysis of dma-buf backed buffers. > I will get to doing a more detailed review soon, but immediately, we > might want to think a bit about the get/set_name IOCTLS - do we need > to think of disallowing multiple renaming of buffers once they start > getting used? It could otherwise make the whole metrics a lot > confused? > > > > > Greg Hackmann (3): > > dma-buf: give each buffer a full-fledged inode > > dma-buf: add DMA_BUF_{GET,SET}_NAME ioctls > > dma-buf: add show_fdinfo handler I'm not seeing the patches, so just quick comment here: Some drivers (at least vc4) already have the concept of buffer names. Would be neat to integrate this between dma-buf and drm_gem in prime. Aside from that sounds like a good idea overall, but I can't see any details. -Daniel > > > > drivers/dma-buf/dma-buf.c | 121 ++++++++++++++++++++++++++++++++--- > > include/linux/dma-buf.h | 5 +- > > include/uapi/linux/dma-buf.h | 4 ++ > > include/uapi/linux/magic.h | 1 + > > 4 files changed, 122 insertions(+), 9 deletions(-) > > > > -- > > 2.21.0.rc2.261.ga7da99ff1b-goog > > > > Best, > Sumit. > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel, On Fri, 15 Mar 2019 at 16:36, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Mar 14, 2019 at 6:49 PM Sumit Semwal <sumit.semwal@linaro.org> wrote: > > > > Hello Chenbo,Thank you for your RFC series. > > > > On Wed, 27 Feb 2019 at 09:24, Chenbo Feng <fengc@google.com> wrote: > > > > > > Currently, all dma-bufs share the same anonymous inode. While we can count > > > how many dma-buf fds or mappings a process has, we can't get the size of > > > the backing buffers or tell if two entries point to the same dma-buf. And > > > in debugfs, we can get a per-buffer breakdown of size and reference count, > > > but can't tell which processes are actually holding the references to each > > > buffer. > > > > > > To resolve the issue above and provide better method for userspace to track > > > the dma-buf usage across different processes, the following changes are > > > proposed in dma-buf kernel side. First of all, replace the singleton inode > > > inside the dma-buf subsystem with a mini-filesystem, and assign each > > > dma-buf a unique inode out of this filesystem. With this change, calling > > > stat(2) on each entry gives the caller a unique ID (st_ino), the buffer's > > > size (st_size), and even the number of pages assigned to each dma-buffer. > > > Secoundly, add the inode information to /sys/kernel/debug/dma_buf/bufinfo > > > so in the case where a buffer is mmap()ed into a process’s address space > > > but all remaining fds have been closed, we can still get the dma-buf > > > information and try to accociate it with the process by searching the > > > proc/pid/maps and looking for the corresponding inode number exposed in > > > dma-buf debug fs. Thirdly, created an ioctl to assign names to dma-bufs > > > which lets userspace assign short names (e.g., "CAMERA") to buffers. This > > > information can be extremely helpful for tracking and accounting shared > > > buffers based on their usage and original purpose. Last but not least, add > > > dma-buf information to /proc/pid/fdinfo by adding a show_fdinfo() handler > > > to dma_buf_file_operations. The handler will print the file_count and name > > > of each buffer. > > > > In general, I think I like the idea as it contributes to a much more > > relevant usage analysis of dma-buf backed buffers. > > I will get to doing a more detailed review soon, but immediately, we > > might want to think a bit about the get/set_name IOCTLS - do we need > > to think of disallowing multiple renaming of buffers once they start > > getting used? It could otherwise make the whole metrics a lot > > confused? > > > > > > > > Greg Hackmann (3): > > > dma-buf: give each buffer a full-fledged inode > > > dma-buf: add DMA_BUF_{GET,SET}_NAME ioctls > > > dma-buf: add show_fdinfo handler > > I'm not seeing the patches, so just quick comment here: Some drivers > (at least vc4) already have the concept of buffer names. Would be neat > to integrate this between dma-buf and drm_gem in prime. > FYI, here's the patch series - https://patchwork.freedesktop.org/series/57282/ Best, Sumit. > Aside from that sounds like a good idea overall, but I can't see any details. > -Daniel > > > > > > > drivers/dma-buf/dma-buf.c | 121 ++++++++++++++++++++++++++++++++--- > > > include/linux/dma-buf.h | 5 +- > > > include/uapi/linux/dma-buf.h | 4 ++ > > > include/uapi/linux/magic.h | 1 + > > > 4 files changed, 122 insertions(+), 9 deletions(-) > > > > > > -- > > > 2.21.0.rc2.261.ga7da99ff1b-goog > > > > > > > Best, > > Sumit. > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Currently, all dma-bufs share the same anonymous inode. While we can count how many dma-buf fds or mappings a process has, we can't get the size of the backing buffers or tell if two entries point to the same dma-buf. And in debugfs, we can get a per-buffer breakdown of size and reference count, but can't tell which processes are actually holding the references to each buffer. To resolve the issue above and provide better method for userspace to track the dma-buf usage across different processes, the following changes are proposed in dma-buf kernel side. First of all, replace the singleton inode inside the dma-buf subsystem with a mini-filesystem, and assign each dma-buf a unique inode out of this filesystem. With this change, calling stat(2) on each entry gives the caller a unique ID (st_ino), the buffer's size (st_size), and even the number of pages assigned to each dma-buffer. Secoundly, add the inode information to /sys/kernel/debug/dma_buf/bufinfo so in the case where a buffer is mmap()ed into a process’s address space but all remaining fds have been closed, we can still get the dma-buf information and try to accociate it with the process by searching the proc/pid/maps and looking for the corresponding inode number exposed in dma-buf debug fs. Thirdly, created an ioctl to assign names to dma-bufs which lets userspace assign short names (e.g., "CAMERA") to buffers. This information can be extremely helpful for tracking and accounting shared buffers based on their usage and original purpose. Last but not least, add dma-buf information to /proc/pid/fdinfo by adding a show_fdinfo() handler to dma_buf_file_operations. The handler will print the file_count and name of each buffer. Greg Hackmann (3): dma-buf: give each buffer a full-fledged inode dma-buf: add DMA_BUF_{GET,SET}_NAME ioctls dma-buf: add show_fdinfo handler drivers/dma-buf/dma-buf.c | 121 ++++++++++++++++++++++++++++++++--- include/linux/dma-buf.h | 5 +- include/uapi/linux/dma-buf.h | 4 ++ include/uapi/linux/magic.h | 1 + 4 files changed, 122 insertions(+), 9 deletions(-)