Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH] dmabuf: Add the capability to expose DMA-BUF stats in sysfs | From | Christian König <> | Date | Thu, 10 Dec 2020 10:58:50 +0100 |
| |
In general a good idea, but I have a few concern/comments here.
Am 10.12.20 um 05:43 schrieb Hridya Valsaraju: > 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/<inode_number>/exporter_name > /sys/kernel/dmabuf/<inode_number>/size > /sys/kernel/dmabuf/<inode_number>/dev_map_info > > 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.
Mhm, this makes it part of the UAPI. What is the justification for this?
In other words do we really need those debug information in a production environment?
> > This information is intended to help with root-causing > low-memory kills and the debugging/analysis of other memory-related issues. > > It will also be used to derive DMA-BUF > per-exporter stats and per-device usage stats for Android Bug reports. > > [1]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1088791%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C30a0e015502b4d20e18208d89cc63f1a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637431722574983797%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=RdGMvj5VsFUwJcVOuSPaLuAr4eI3CR1YOaznupmpTqg%3D&reserved=0 > > Signed-off-by: Hridya Valsaraju <hridya@google.com> > --- > Documentation/ABI/testing/sysfs-kernel-dmabuf | 32 ++++ > drivers/dma-buf/Kconfig | 11 ++ > drivers/dma-buf/Makefile | 1 + > drivers/dma-buf/dma-buf-sysfs-stats.c | 162 ++++++++++++++++++ > drivers/dma-buf/dma-buf-sysfs-stats.h | 37 ++++ > drivers/dma-buf/dma-buf.c | 29 ++++ > include/linux/dma-buf.h | 13 ++ > 7 files changed, 285 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf > create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c > create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h > > diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf b/Documentation/ABI/testing/sysfs-kernel-dmabuf > new file mode 100644 > index 000000000000..02d407d57aaa > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf > @@ -0,0 +1,32 @@ > +What: /sys/kernel/dmabuf > +Date: November 2020 > +KernelVersion: v5.11 > +Contact: Hridya Valsaraju <hridya@google.com> > +Description: The /sys/kernel/dmabuf directory contains a > + snapshot of the internal state of every DMA-BUF. > + /sys/kernel/dmabuf/<inode_number> will contain the > + statistics for the DMA-BUF with the unique inode number > + <inode_number> > +Users: kernel memory tuning/debugging tools > + > +What: /sys/kernel/dmabuf/<inode_number>/exporter_name > +Date: November 2020 > +KernelVersion: v5.11 > +Contact: Hridya Valsaraju <hridya@google.com> > +Description: This file is read-only and contains the name of the exporter of > + the DMA-BUF. > + > +What: /sys/kernel/dmabuf/<inode_number>/size > +Dat: November 2020 > +KernelVersion: v5.11 > +Contact: Hridya Valsaraju <hridya@google.com> > +Description: This file is read-only and specifies the size of the DMA-BUF in > + bytes. > + > +What: /sys/kernel/dmabuf/<inode_number>/dev_map_info > +Dat: November 2020 > +KernelVersion: v5.11 > +Contact: Hridya Valsaraju <hridya@google.com> > +Description: This file is read-only and lists the name of devices currently > + mapping the DMA-BUF in a space-separated format. > + > diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig > index 4f8224a6ac95..2fed26f14548 100644 > --- a/drivers/dma-buf/Kconfig > +++ b/drivers/dma-buf/Kconfig > @@ -64,6 +64,17 @@ menuconfig DMABUF_HEAPS > allows userspace to allocate dma-bufs that can be shared > between drivers. > > +menuconfig DMABUF_SYSFS_STATS > + bool "DMA-BUF sysfs statistics" > + select DMA_SHARED_BUFFER > + help > + Choose this option to enable DMA-BUF sysfs statistics > + in location /sys/kernel/dmabuf. > + > + /sys/kernel/dmabuf/<inode_number> will contain > + statistics for the DMA-BUF with the unique inode number > + <inode_number>. > + > source "drivers/dma-buf/heaps/Kconfig" > > endmenu > diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile > index 995e05f609ff..40d81f23cacf 100644 > --- a/drivers/dma-buf/Makefile > +++ b/drivers/dma-buf/Makefile > @@ -6,6 +6,7 @@ obj-$(CONFIG_DMABUF_HEAPS) += heaps/ > obj-$(CONFIG_SYNC_FILE) += sync_file.o > obj-$(CONFIG_SW_SYNC) += sw_sync.o sync_debug.o > obj-$(CONFIG_UDMABUF) += udmabuf.o > +obj-$(CONFIG_DMABUF_SYSFS_STATS) += dma-buf-sysfs-stats.o > > dmabuf_selftests-y := \ > selftest.o \ > diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c > new file mode 100644 > index 000000000000..bcbef81e0a5f > --- /dev/null > +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c > @@ -0,0 +1,162 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > + > +#include <linux/dma-buf.h> > +#include <linux/dma-resv.h> > +#include <linux/kobject.h> > +#include <linux/printk.h> > +#include <linux/slab.h> > +#include <linux/sysfs.h> > + > +#define to_dma_buf_entry_from_kobj(x) container_of(x, struct dma_buf_sysfs_entry, kobj) > + > +struct dma_buf_stats_attribute { > + struct attribute attr; > + ssize_t (*show)(struct dma_buf *dmabuf, > + struct dma_buf_stats_attribute *attr, char *buf); > +}; > +#define to_dma_buf_stats_attr(x) container_of(x, struct dma_buf_stats_attribute, attr) > + > +static ssize_t dma_buf_stats_attribute_show(struct kobject *kobj, > + struct attribute *attr, > + char *buf) > +{ > + struct dma_buf_stats_attribute *attribute; > + struct dma_buf_sysfs_entry *sysfs_entry; > + struct dma_buf *dmabuf; > + > + attribute = to_dma_buf_stats_attr(attr); > + sysfs_entry = to_dma_buf_entry_from_kobj(kobj); > + dmabuf = sysfs_entry->dmabuf; > + > + if (!dmabuf || !attribute->show) > + return -EIO; > + > + return attribute->show(dmabuf, attribute, buf); > +} > + > +static const struct sysfs_ops dma_buf_stats_sysfs_ops = { > + .show = dma_buf_stats_attribute_show, > +}; > + > +static ssize_t exporter_name_show(struct dma_buf *dmabuf, > + struct dma_buf_stats_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "%s\n", dmabuf->exp_name); > +} > + > +static ssize_t size_show(struct dma_buf *dmabuf, > + struct dma_buf_stats_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "%zu\n", dmabuf->size); > +} > + > +static ssize_t dev_map_info_show(struct dma_buf *dmabuf, > + struct dma_buf_stats_attribute *attr, > + char *buf) > +{ > + ssize_t ret; > + struct dma_buf_attachment *attachment; > + > + ret = dma_resv_lock_interruptible(dmabuf->resv, NULL); > + if (ret) > + return ret; > + > + list_for_each_entry(attachment, &dmabuf->attachments, node) { > + if (attachment->map_counter) { > + ret += sysfs_emit_at(buf, ret, "%s ", > + dev_name(attachment->dev)); > + }
Why do you emit this only when there is a mapping? It is perfectly valid and current practice that importers map things only on first use.
The number of mapping is in general rather interesting, but I would just print that together with the importer name.
And BTW I would rename the dev_map_info to something like "attachments".
Regards, Christian.
> + } > + dma_resv_unlock(dmabuf->resv); > + > + ret += sysfs_emit_at(buf, ret, "\n"); > + return ret; > +} > + > +static struct dma_buf_stats_attribute exporter_name_attribute = > + __ATTR_RO(exporter_name); > +static struct dma_buf_stats_attribute size_attribute = __ATTR_RO(size); > +static struct dma_buf_stats_attribute dev_map_info_attribute = > + __ATTR_RO(dev_map_info); > + > +static struct attribute *dma_buf_stats_default_attrs[] = { > + &exporter_name_attribute.attr, > + &size_attribute.attr, > + &dev_map_info_attribute.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(dma_buf_stats_default); > + > +static void dma_buf_sysfs_release(struct kobject *kobj) > +{ > + struct dma_buf_sysfs_entry *sysfs_entry; > + > + sysfs_entry = to_dma_buf_entry_from_kobj(kobj); > + kfree(sysfs_entry); > +} > + > +static struct kobj_type dma_buf_ktype = { > + .sysfs_ops = &dma_buf_stats_sysfs_ops, > + .release = dma_buf_sysfs_release, > + .default_groups = dma_buf_stats_default_groups, > +}; > + > +void dma_buf_sysfs_free(struct dma_buf *dmabuf) > +{ > + struct dma_buf_sysfs_entry *sysfs_entry; > + > + sysfs_entry = dmabuf->sysfs_entry; > + if (!sysfs_entry) > + return; > + > + kobject_del(&sysfs_entry->kobj); > + kobject_put(&sysfs_entry->kobj); > +} > + > +static struct kset *dma_buf_stats_kset; > +int dma_buf_init_sysfs_statistics(void) > +{ > + dma_buf_stats_kset = kset_create_and_add("dmabuf", NULL, kernel_kobj); > + if (!dma_buf_stats_kset) > + return -ENOMEM; > + > + return 0; > +} > + > +void dma_buf_uninit_sysfs_statistics(void) > +{ > + kset_unregister(dma_buf_stats_kset); > +} > + > +int dma_buf_init_stats_kobj(struct dma_buf *dmabuf) > +{ > + struct dma_buf_sysfs_entry *sysfs_entry; > + int ret; > + > + if (!dmabuf || !dmabuf->file) > + return -EINVAL; > + > + if (!dmabuf->exp_name) { > + pr_err("exporter name must not be empty if stats needed\n"); > + return -EINVAL; > + } > + > + sysfs_entry = kzalloc(sizeof(struct dma_buf_sysfs_entry), GFP_KERNEL); > + if (!sysfs_entry) > + return -ENOMEM; > + > + sysfs_entry->kobj.kset = dma_buf_stats_kset; > + sysfs_entry->dmabuf = dmabuf; > + > + dmabuf->sysfs_entry = sysfs_entry; > + > + ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL, > + "%lu", file_inode(dmabuf->file)->i_ino); > + if (ret) > + kobject_put(&sysfs_entry->kobj); > + > + return ret; > +} > diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma-buf-sysfs-stats.h > new file mode 100644 > index 000000000000..42fae7d1b11f > --- /dev/null > +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h > @@ -0,0 +1,37 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef _DMA_BUF_SYSFS_STATS_H > +#define _DMA_BUF_SYSFS_STATS_H > + > +#ifdef CONFIG_DMABUF_SYSFS_STATS > + > +int dma_buf_init_sysfs_statistics(void); > +void dma_buf_uninit_sysfs_statistics(void); > + > +int dma_buf_init_stats_kobj(struct dma_buf *dmabuf); > +static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, > + int delta) > +{ > + attach->map_counter += delta; > +} > +void dma_buf_sysfs_free(struct dma_buf *dmabuf); > + > +#else > + > +static inline int dma_buf_init_sysfs_statistics(void) > +{ > + return 0; > +} > + > +static inline void dma_buf_uninit_sysfs_statistics(void) {} > + > +static inline int dma_buf_init_stats_kobj(struct dma_buf *dmabuf) > +{ > + return 0; > +} > +static inline void dma_buf_sysfs_free(struct dma_buf *dmabuf) {} > +static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, > + int delta) {} > + > +#endif > +#endif // _DMA_BUF_SYSFS_STATS_H > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index e63684d4cd90..e93df0069bf8 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -29,6 +29,8 @@ > #include <uapi/linux/dma-buf.h> > #include <uapi/linux/magic.h> > > +#include "dma-buf-sysfs-stats.h" > + > static inline int is_dma_buf_file(struct file *); > > struct dma_buf_list { > @@ -83,6 +85,7 @@ static void dma_buf_release(struct dentry *dentry) > if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) > dma_resv_fini(dmabuf->resv); > > + dma_buf_sysfs_free(dmabuf); > module_put(dmabuf->owner); > kfree(dmabuf->name); > kfree(dmabuf); > @@ -566,6 +569,10 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) > file->f_mode |= FMODE_LSEEK; > dmabuf->file = file; > > + ret = dma_buf_init_stats_kobj(dmabuf); > + if (ret) > + goto err_sysfs; > + > mutex_init(&dmabuf->lock); > INIT_LIST_HEAD(&dmabuf->attachments); > > @@ -575,6 +582,14 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) > > return dmabuf; > > +err_sysfs: > + /* > + * Set file->f_path.dentry->d_fsdata to NULL so that when > + * dma_buf_release() gets invoked by dentry_ops, it exits > + * early before calling the release() dma_buf op. > + */ > + file->f_path.dentry->d_fsdata = NULL; > + fput(file); > err_dmabuf: > kfree(dmabuf); > err_module: > @@ -732,6 +747,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > dma_resv_unlock(attach->dmabuf->resv); > attach->sgt = sgt; > attach->dir = DMA_BIDIRECTIONAL; > + dma_buf_update_attachment_map_count(attach, 1 /* delta */); > } > > return attach; > @@ -786,6 +802,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) > dma_resv_lock(attach->dmabuf->resv, NULL); > > dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir); > + dma_buf_update_attachment_map_count(attach, -1 /* delta */); > > if (dma_buf_is_dynamic(attach->dmabuf)) { > dma_buf_unpin(attach); > @@ -925,6 +942,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, > } > #endif /* CONFIG_DMA_API_DEBUG */ > > + if (!IS_ERR(sg_table)) > + dma_buf_update_attachment_map_count(attach, 1 /* delta */); > + > return sg_table; > } > EXPORT_SYMBOL_GPL(dma_buf_map_attachment); > @@ -962,6 +982,8 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, > if (dma_buf_is_dynamic(attach->dmabuf) && > !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) > dma_buf_unpin(attach); > + > + dma_buf_update_attachment_map_count(attach, -1 /* delta */); > } > EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); > > @@ -1399,6 +1421,12 @@ static inline void dma_buf_uninit_debugfs(void) > > static int __init dma_buf_init(void) > { > + int ret; > + > + ret = dma_buf_init_sysfs_statistics(); > + if (ret) > + return ret; > + > dma_buf_mnt = kern_mount(&dma_buf_fs_type); > if (IS_ERR(dma_buf_mnt)) > return PTR_ERR(dma_buf_mnt); > @@ -1414,5 +1442,6 @@ static void __exit dma_buf_deinit(void) > { > dma_buf_uninit_debugfs(); > kern_unmount(dma_buf_mnt); > + dma_buf_uninit_sysfs_statistics(); > } > __exitcall(dma_buf_deinit); > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index cf72699cb2bc..f5cab13afdfc 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -294,6 +294,7 @@ struct dma_buf_ops { > * @poll: for userspace poll support > * @cb_excl: for userspace poll support > * @cb_shared: for userspace poll support > + * @sysfs_entry: for exposing information about this buffer in sysfs > * > * This represents a shared buffer, created by calling dma_buf_export(). The > * userspace representation is a normal file descriptor, which can be created by > @@ -329,6 +330,13 @@ struct dma_buf { > > __poll_t active; > } cb_excl, cb_shared; > +#ifdef CONFIG_DMABUF_SYSFS_STATS > + /* for sysfs stats */ > + struct dma_buf_sysfs_entry { > + struct kobject kobj; > + struct dma_buf *dmabuf; > + } *sysfs_entry; > +#endif > }; > > /** > @@ -378,6 +386,8 @@ struct dma_buf_attach_ops { > * @importer_ops: importer operations for this attachment, if provided > * dma_buf_map/unmap_attachment() must be called with the dma_resv lock held. > * @importer_priv: importer specific attachment data. > + * @map_counter: Number of times the buffer has been mapped through this > + * dma_buf_map_attachment. > * > * This structure holds the attachment information between the dma_buf buffer > * and its user device(s). The list contains one attachment struct per device > @@ -398,6 +408,9 @@ struct dma_buf_attachment { > const struct dma_buf_attach_ops *importer_ops; > void *importer_priv; > void *priv; > +#ifdef CONFIG_DMABUF_SYSFS_STATS > + unsigned int map_counter; > +#endif > }; > > /**
| ![\](/images/icornerr.gif) |