Messages in this thread | | | Subject | Re: [PATCH v2] dma-buf: Delete the DMA-BUF attachment sysfs statistics | From | Christian König <> | Date | Tue, 20 Jul 2021 11:11:43 +0200 |
| |
Am 13.07.21 um 06:07 schrieb Hridya Valsaraju: > The DMA-BUF attachment statistics form a subset of the DMA-BUF > sysfs statistics that recently merged to the drm-misc tree. They are not > UABI yet since they have not merged to the upstream Linux kernel. > > Since there has been a reported a performance regression due to the > overhead of sysfs directory creation/teardown during > dma_buf_attach()/dma_buf_detach(), this patch deletes the DMA-BUF > attachment statistics from sysfs. > > Fixes: bdb8d06dfefd (dmabuf: Add the capability to expose DMA-BUF stats > in sysfs) > Signed-off-by: Hridya Valsaraju <hridya@google.com> > Reviewed-by: Christian König <christian.koenig@amd.com> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > > Changes in v2: > Updated commit message to clarify that the sysfs files being removed > have not yet merged to upstream Linux and are hence not ABI. > > Hi Christian, > > I have updated the commit message as per your suggestion. Please do take > another look when you get a chance.
I've just pushed that one to drm-misc-next. Sorry for the delay.
Regards, Christian.
> > Thanks, > Hridya > > .../ABI/testing/sysfs-kernel-dmabuf-buffers | 28 ---- > drivers/dma-buf/dma-buf-sysfs-stats.c | 140 +----------------- > drivers/dma-buf/dma-buf-sysfs-stats.h | 27 ---- > drivers/dma-buf/dma-buf.c | 16 -- > include/linux/dma-buf.h | 17 --- > 5 files changed, 4 insertions(+), 224 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > index a243984ed420..5d3bc997dc64 100644 > --- a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers > @@ -22,31 +22,3 @@ KernelVersion: v5.13 > 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/buffers/<inode_number>/attachments > -Date: May 2021 > -KernelVersion: v5.13 > -Contact: Hridya Valsaraju <hridya@google.com> > -Description: This directory will contain subdirectories representing every > - attachment of the DMA-BUF. > - > -What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid> > -Date: May 2021 > -KernelVersion: v5.13 > -Contact: Hridya Valsaraju <hridya@google.com> > -Description: This directory will contain information on the attached device > - and the number of current distinct device mappings. > - > -What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid>/device > -Date: May 2021 > -KernelVersion: v5.13 > -Contact: Hridya Valsaraju <hridya@google.com> > -Description: This file is read-only and is a symlink to the attached device's > - sysfs entry. > - > -What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid>/map_counter > -Date: May 2021 > -KernelVersion: v5.13 > -Contact: Hridya Valsaraju <hridya@google.com> > -Description: This file is read-only and contains a map_counter indicating the > - number of distinct device mappings of the attachment. > diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c > index a2638e84199c..053baadcada9 100644 > --- a/drivers/dma-buf/dma-buf-sysfs-stats.c > +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c > @@ -40,14 +40,11 @@ > * > * * ``/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 information in the interface can also be used to derive per-exporter and > - * per-device usage statistics. The data from the interface can be gathered > - * on error conditions or other important events to provide a snapshot of > - * DMA-BUF usage. It can also be collected periodically by telemetry to monitor > - * various metrics. > + * The information in the interface can also be used to derive per-exporter > + * statistics. The data from the interface can be gathered on error conditions > + * or other important events to provide a snapshot of DMA-BUF usage. > + * It can also be collected periodically by telemetry to monitor various metrics. > * > * Detailed documentation about the interface is present in > * Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers. > @@ -121,120 +118,6 @@ static struct kobj_type dma_buf_ktype = { > .default_groups = dma_buf_stats_default_groups, > }; > > -#define to_dma_buf_attach_entry_from_kobj(x) container_of(x, struct dma_buf_attach_sysfs_entry, kobj) > - > -struct dma_buf_attach_stats_attribute { > - struct attribute attr; > - ssize_t (*show)(struct dma_buf_attach_sysfs_entry *sysfs_entry, > - struct dma_buf_attach_stats_attribute *attr, char *buf); > -}; > -#define to_dma_buf_attach_stats_attr(x) container_of(x, struct dma_buf_attach_stats_attribute, attr) > - > -static ssize_t dma_buf_attach_stats_attribute_show(struct kobject *kobj, > - struct attribute *attr, > - char *buf) > -{ > - struct dma_buf_attach_stats_attribute *attribute; > - struct dma_buf_attach_sysfs_entry *sysfs_entry; > - > - attribute = to_dma_buf_attach_stats_attr(attr); > - sysfs_entry = to_dma_buf_attach_entry_from_kobj(kobj); > - > - if (!attribute->show) > - return -EIO; > - > - return attribute->show(sysfs_entry, attribute, buf); > -} > - > -static const struct sysfs_ops dma_buf_attach_stats_sysfs_ops = { > - .show = dma_buf_attach_stats_attribute_show, > -}; > - > -static ssize_t map_counter_show(struct dma_buf_attach_sysfs_entry *sysfs_entry, > - struct dma_buf_attach_stats_attribute *attr, > - char *buf) > -{ > - return sysfs_emit(buf, "%u\n", sysfs_entry->map_counter); > -} > - > -static struct dma_buf_attach_stats_attribute map_counter_attribute = > - __ATTR_RO(map_counter); > - > -static struct attribute *dma_buf_attach_stats_default_attrs[] = { > - &map_counter_attribute.attr, > - NULL, > -}; > -ATTRIBUTE_GROUPS(dma_buf_attach_stats_default); > - > -static void dma_buf_attach_sysfs_release(struct kobject *kobj) > -{ > - struct dma_buf_attach_sysfs_entry *sysfs_entry; > - > - sysfs_entry = to_dma_buf_attach_entry_from_kobj(kobj); > - kfree(sysfs_entry); > -} > - > -static struct kobj_type dma_buf_attach_ktype = { > - .sysfs_ops = &dma_buf_attach_stats_sysfs_ops, > - .release = dma_buf_attach_sysfs_release, > - .default_groups = dma_buf_attach_stats_default_groups, > -}; > - > -void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach) > -{ > - struct dma_buf_attach_sysfs_entry *sysfs_entry; > - > - sysfs_entry = attach->sysfs_entry; > - if (!sysfs_entry) > - return; > - > - sysfs_delete_link(&sysfs_entry->kobj, &attach->dev->kobj, "device"); > - > - kobject_del(&sysfs_entry->kobj); > - kobject_put(&sysfs_entry->kobj); > -} > - > -int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, > - unsigned int uid) > -{ > - struct dma_buf_attach_sysfs_entry *sysfs_entry; > - int ret; > - struct dma_buf *dmabuf; > - > - if (!attach) > - return -EINVAL; > - > - dmabuf = attach->dmabuf; > - > - sysfs_entry = kzalloc(sizeof(struct dma_buf_attach_sysfs_entry), > - GFP_KERNEL); > - if (!sysfs_entry) > - return -ENOMEM; > - > - sysfs_entry->kobj.kset = dmabuf->sysfs_entry->attach_stats_kset; > - > - attach->sysfs_entry = sysfs_entry; > - > - ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_attach_ktype, > - NULL, "%u", uid); > - if (ret) > - goto kobj_err; > - > - ret = sysfs_create_link(&sysfs_entry->kobj, &attach->dev->kobj, > - "device"); > - if (ret) > - goto link_err; > - > - return 0; > - > -link_err: > - kobject_del(&sysfs_entry->kobj); > -kobj_err: > - kobject_put(&sysfs_entry->kobj); > - attach->sysfs_entry = NULL; > - > - return ret; > -} > void dma_buf_stats_teardown(struct dma_buf *dmabuf) > { > struct dma_buf_sysfs_entry *sysfs_entry; > @@ -243,7 +126,6 @@ void dma_buf_stats_teardown(struct dma_buf *dmabuf) > if (!sysfs_entry) > return; > > - kset_unregister(sysfs_entry->attach_stats_kset); > kobject_del(&sysfs_entry->kobj); > kobject_put(&sysfs_entry->kobj); > } > @@ -290,7 +172,6 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) > { > struct dma_buf_sysfs_entry *sysfs_entry; > int ret; > - struct kset *attach_stats_kset; > > if (!dmabuf || !dmabuf->file) > return -EINVAL; > @@ -315,21 +196,8 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf) > if (ret) > goto err_sysfs_dmabuf; > > - /* create the directory for attachment stats */ > - attach_stats_kset = kset_create_and_add("attachments", > - &dmabuf_sysfs_no_uevent_ops, > - &sysfs_entry->kobj); > - if (!attach_stats_kset) { > - ret = -ENOMEM; > - goto err_sysfs_attach; > - } > - > - sysfs_entry->attach_stats_kset = attach_stats_kset; > - > return 0; > > -err_sysfs_attach: > - kobject_del(&sysfs_entry->kobj); > err_sysfs_dmabuf: > kobject_put(&sysfs_entry->kobj); > dmabuf->sysfs_entry = NULL; > diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma-buf-sysfs-stats.h > index 5f4703249117..a49c6e2650cc 100644 > --- a/drivers/dma-buf/dma-buf-sysfs-stats.h > +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h > @@ -14,23 +14,8 @@ int dma_buf_init_sysfs_statistics(void); > void dma_buf_uninit_sysfs_statistics(void); > > int dma_buf_stats_setup(struct dma_buf *dmabuf); > -int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, > - unsigned int uid); > -static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, > - int delta) > -{ > - struct dma_buf_attach_sysfs_entry *entry = attach->sysfs_entry; > > - entry->map_counter += delta; > -} > void dma_buf_stats_teardown(struct dma_buf *dmabuf); > -void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach); > -static inline unsigned int dma_buf_update_attach_uid(struct dma_buf *dmabuf) > -{ > - struct dma_buf_sysfs_entry *entry = dmabuf->sysfs_entry; > - > - return entry->attachment_uid++; > -} > #else > > static inline int dma_buf_init_sysfs_statistics(void) > @@ -44,19 +29,7 @@ static inline int dma_buf_stats_setup(struct dma_buf *dmabuf) > { > return 0; > } > -static inline int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, > - unsigned int uid) > -{ > - return 0; > -} > > static inline void dma_buf_stats_teardown(struct dma_buf *dmabuf) {} > -static inline void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach) {} > -static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, > - int delta) {} > -static inline unsigned int dma_buf_update_attach_uid(struct dma_buf *dmabuf) > -{ > - return 0; > -} > #endif > #endif // _DMA_BUF_SYSFS_STATS_H > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 510b42771974..b1a6db71c656 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -738,7 +738,6 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > { > struct dma_buf_attachment *attach; > int ret; > - unsigned int attach_uid; > > if (WARN_ON(!dmabuf || !dev)) > return ERR_PTR(-EINVAL); > @@ -764,13 +763,8 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > } > dma_resv_lock(dmabuf->resv, NULL); > list_add(&attach->node, &dmabuf->attachments); > - attach_uid = dma_buf_update_attach_uid(dmabuf); > dma_resv_unlock(dmabuf->resv); > > - ret = dma_buf_attach_stats_setup(attach, attach_uid); > - if (ret) > - goto err_sysfs; > - > /* When either the importer or the exporter can't handle dynamic > * mappings we cache the mapping here to avoid issues with the > * reservation object lock. > @@ -797,7 +791,6 @@ 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; > @@ -814,7 +807,6 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > if (dma_buf_is_dynamic(attach->dmabuf)) > dma_resv_unlock(attach->dmabuf->resv); > > -err_sysfs: > dma_buf_detach(dmabuf, attach); > return ERR_PTR(ret); > } > @@ -864,7 +856,6 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) > dma_resv_lock(attach->dmabuf->resv, NULL); > > __unmap_dma_buf(attach, attach->sgt, attach->dir); > - dma_buf_update_attachment_map_count(attach, -1 /* delta */); > > if (dma_buf_is_dynamic(attach->dmabuf)) { > dmabuf->ops->unpin(attach); > @@ -878,7 +869,6 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) > if (dmabuf->ops->detach) > dmabuf->ops->detach(dmabuf, attach); > > - dma_buf_attach_stats_teardown(attach); > kfree(attach); > } > EXPORT_SYMBOL_GPL(dma_buf_detach); > @@ -1020,10 +1010,6 @@ 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); > @@ -1061,8 +1047,6 @@ 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); > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index 2b814fde0d11..678b2006be78 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -444,15 +444,6 @@ struct dma_buf { > struct dma_buf_sysfs_entry { > struct kobject kobj; > struct dma_buf *dmabuf; > - > - /** > - * @sysfs_entry.attachment_uid: > - * > - * This is protected by the dma_resv_lock() on @resv and is > - * incremented on each attach. > - */ > - unsigned int attachment_uid; > - struct kset *attach_stats_kset; > } *sysfs_entry; > #endif > }; > @@ -504,7 +495,6 @@ 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. > - * @sysfs_entry: For exposing information about this attachment in sysfs. > * > * This structure holds the attachment information between the dma_buf buffer > * and its user device(s). The list contains one attachment struct per device > @@ -525,13 +515,6 @@ struct dma_buf_attachment { > const struct dma_buf_attach_ops *importer_ops; > void *importer_priv; > void *priv; > -#ifdef CONFIG_DMABUF_SYSFS_STATS > - /* for sysfs stats */ > - struct dma_buf_attach_sysfs_entry { > - struct kobject kobj; > - unsigned int map_counter; > - } *sysfs_entry; > -#endif > }; > > /**
| |