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

On Tue, Jul 20, 2021 at 2:11 AM Christian König christian.koenig@amd.com wrote:

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.

No worries at all, thank you Christian.

Regards, Hridya

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

/**