Message-ID: <4638d2fd-d8cd-d38b-b935-8ac8c0a935c8@amd.com>
Date:   Mon, 12 Jul 2021 08:43:45 +0200
From:   Christian König <christian.koenig@....com>
To:     Hridya Valsaraju <hridya@...gle.com>, daniel@...ll.ch,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org
Cc:     kernel-team@...roid.com, john.stultz@...aro.org, surenb@...gle.com
Subject: Re: [PATCH] dma-buf: Delete the DMA-BUF attachment sysfs statistics

Am 10.07.21 um 07:10 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.
> 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@...gle.com>
> ---
>
> Hello all,
>
> One of our partners recently reported a perf regression in a driver
> which was being caused due to the overhead of setup/teardown of the
> sysfs attachment statistics in the dma_buf_attach()/dma_buf_detach()
> invocations. Since the driver's latency requirements were of the order
> of microseconds(~100us), the overhead was significant.

I find it rather questionable to rely on the performance of attach cause 
that can easily take more than 10ms depending on memory migration needs 
of the exporter.

You should probably consider changing this use case to use cached 
attachments instead.

> Since this indicates that the solution might not work well for
> all DMA-BUF importers, I think the right thing to do might be to delete
> the same before it reaches upstream and becomes ABI :( I apologize for
> the inconvenience.

The information that this is not UABI yet should be part of the commit 
message.

Apart from that feel free to add Reviewed-by: Christian König 
<christian.koenig@....com>

>
> This patch is based on the drm-misc-next branch. Please feel free to
> let me know if you would prefer that I send a full revert and new patch that
> adds the rest of the statistics.

Yeah, that's perfectly ok. Please provide a v2 with updated commit 
message and I can push that ASAP.

Regards,
Christian.

>
> Regards,
> 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@...gle.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@...gle.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@...gle.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@...gle.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@...gle.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
>   };
>   
>   /**

Powered by blists - more mailing lists