Re: [Linaro-mm-sig] [PATCH v2] dma-buf: Delete the DMA-BUF attachment sysfs statistics

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 }; /**