Add a warning that this UAPI wasn't such a good idea and shouldn't be
used by anybody.
That should give us a better chance to remove it at some point and
prevents others from running into the same issues.
Signed-off-by: Christian König <christian.koenig(a)amd.com>
---
drivers/dma-buf/Kconfig | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 541efe01abc7..e4dc53a36428 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -75,7 +75,7 @@ menuconfig DMABUF_HEAPS
between drivers.
menuconfig DMABUF_SYSFS_STATS
- bool "DMA-BUF sysfs statistics"
+ bool "DMA-BUF sysfs statistics (DEPRECATED)"
depends on DMA_SHARED_BUFFER
help
Choose this option to enable DMA-BUF sysfs statistics
@@ -85,6 +85,10 @@ menuconfig DMABUF_SYSFS_STATS
statistics for the DMA-BUF with the unique inode number
<inode_number>.
+ This option is deprecated and should sooner or later be removed.
+ Android is the only user of this and it turned out that this resulted
+ in quite some performance problems.
+
source "drivers/dma-buf/heaps/Kconfig"
endmenu
--
2.25.1
Recently, we noticed an issue where a process went into direct reclaim
while holding the kernfs rw semaphore for sysfs in write (exclusive)
mode. This caused processes who were doing DMA-BUF exports and releases
to go into uninterruptible sleep since they needed to acquire the same
semaphore for the DMA-BUF sysfs entry creation/deletion. In order to avoid
blocking DMA-BUF export for an indeterminate amount of time while
another process is holding the sysfs rw semaphore in exclusive mode,
this patch moves the per-buffer sysfs file creation to the default work
queue. Note that this can lead to a short-term inaccuracy in the dmabuf
sysfs statistics, but this is a tradeoff to prevent the hot path from
being blocked. A work_struct is added to dma_buf to achieve this, but as
it is unioned with the kobject in the sysfs_entry, dma_buf does not
increase in size.
Fixes: bdb8d06dfefd ("dmabuf: Add the capability to expose DMA-BUF stats in sysfs")
Originally-by: Hridya Valsaraju <hridya(a)google.com>
Signed-off-by: T.J. Mercier <tjmercier(a)google.com>
---
See the originally submitted patch by Hridya Valsaraju here:
https://lkml.org/lkml/2022/1/4/1066
v2 changes:
- Defer only sysfs creation instead of creation and teardown per
Christian König
- Use a work queue instead of a kthread for deferred work per
Christian König
---
drivers/dma-buf/dma-buf-sysfs-stats.c | 56 ++++++++++++++++++++-------
include/linux/dma-buf.h | 14 ++++++-
2 files changed, 54 insertions(+), 16 deletions(-)
diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c
index 2bba0babcb62..67b0a298291c 100644
--- a/drivers/dma-buf/dma-buf-sysfs-stats.c
+++ b/drivers/dma-buf/dma-buf-sysfs-stats.c
@@ -11,6 +11,7 @@
#include <linux/printk.h>
#include <linux/slab.h>
#include <linux/sysfs.h>
+#include <linux/workqueue.h>
#include "dma-buf-sysfs-stats.h"
@@ -168,10 +169,46 @@ void dma_buf_uninit_sysfs_statistics(void)
kset_unregister(dma_buf_stats_kset);
}
+static void sysfs_add_workfn(struct work_struct *work)
+{
+ struct dma_buf_sysfs_entry *sysfs_entry =
+ container_of(work, struct dma_buf_sysfs_entry, sysfs_add_work);
+ struct dma_buf *dmabuf = sysfs_entry->dmabuf;
+
+ /*
+ * A dmabuf is ref-counted via its file member. If this handler holds the only
+ * reference to the dmabuf, there is no need for sysfs kobject creation. This is an
+ * optimization and a race; when the reference count drops to 1 immediately after
+ * this check it is not harmful as the sysfs entry will still get cleaned up in
+ * dma_buf_stats_teardown, which won't get called until the final dmabuf reference
+ * is released, and that can't happen until the end of this function.
+ */
+ if (file_count(dmabuf->file) > 1) {
+ /*
+ * kobject_init_and_add expects kobject to be zero-filled, but we have populated it
+ * (the sysfs_add_work union member) to trigger this work function.
+ */
+ memset(&dmabuf->sysfs_entry->kobj, 0, sizeof(dmabuf->sysfs_entry->kobj));
+ dmabuf->sysfs_entry->kobj.kset = dma_buf_per_buffer_stats_kset;
+ if (kobject_init_and_add(&dmabuf->sysfs_entry->kobj, &dma_buf_ktype, NULL,
+ "%lu", file_inode(dmabuf->file)->i_ino)) {
+ kobject_put(&dmabuf->sysfs_entry->kobj);
+ dmabuf->sysfs_entry = NULL;
+ }
+ } else {
+ /*
+ * Free the sysfs_entry and reset the pointer so dma_buf_stats_teardown doesn't
+ * attempt to operate on it.
+ */
+ kfree(dmabuf->sysfs_entry);
+ dmabuf->sysfs_entry = NULL;
+ }
+ dma_buf_put(dmabuf);
+}
+
int dma_buf_stats_setup(struct dma_buf *dmabuf)
{
struct dma_buf_sysfs_entry *sysfs_entry;
- int ret;
if (!dmabuf || !dmabuf->file)
return -EINVAL;
@@ -181,25 +218,16 @@ int dma_buf_stats_setup(struct dma_buf *dmabuf)
return -EINVAL;
}
- sysfs_entry = kzalloc(sizeof(struct dma_buf_sysfs_entry), GFP_KERNEL);
+ sysfs_entry = kmalloc(sizeof(struct dma_buf_sysfs_entry), GFP_KERNEL);
if (!sysfs_entry)
return -ENOMEM;
- sysfs_entry->kobj.kset = dma_buf_per_buffer_stats_kset;
sysfs_entry->dmabuf = dmabuf;
-
dmabuf->sysfs_entry = sysfs_entry;
- /* create the directory for buffer stats */
- ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL,
- "%lu", file_inode(dmabuf->file)->i_ino);
- if (ret)
- goto err_sysfs_dmabuf;
+ INIT_WORK(&dmabuf->sysfs_entry->sysfs_add_work, sysfs_add_workfn);
+ get_dma_buf(dmabuf); /* This reference will be dropped in sysfs_add_workfn. */
+ schedule_work(&dmabuf->sysfs_entry->sysfs_add_work);
return 0;
-
-err_sysfs_dmabuf:
- kobject_put(&sysfs_entry->kobj);
- dmabuf->sysfs_entry = NULL;
- return ret;
}
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 2097760e8e95..0200caa3c515 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -22,6 +22,7 @@
#include <linux/fs.h>
#include <linux/dma-fence.h>
#include <linux/wait.h>
+#include <linux/workqueue.h>
struct device;
struct dma_buf;
@@ -365,7 +366,7 @@ struct dma_buf {
*/
const char *name;
- /** @name_lock: Spinlock to protect name acces for read access. */
+ /** @name_lock: Spinlock to protect name access for read access. */
spinlock_t name_lock;
/**
@@ -441,6 +442,7 @@ struct dma_buf {
__poll_t active;
} cb_in, cb_out;
+
#ifdef CONFIG_DMABUF_SYSFS_STATS
/**
* @sysfs_entry:
@@ -449,7 +451,15 @@ struct dma_buf {
* `DMA-BUF statistics`_ for the uapi this enables.
*/
struct dma_buf_sysfs_entry {
- struct kobject kobj;
+ union {
+ struct kobject kobj;
+
+ /** @sysfs_add_work:
+ *
+ * For deferred sysfs kobject creation using a workqueue.
+ */
+ struct work_struct sysfs_add_work;
+ };
struct dma_buf *dmabuf;
} *sysfs_entry;
#endif
--
2.36.0.550.gb090851708-goog
The print function dev_err() is redundant because platform_get_irq()
already prints an error.
This was found by coccicheck:
./drivers/usb/gadget/udc/aspeed_udc.c:1546:2-9: line 1546 is redundant because platform_get_irq() already prints an error.
Signed-off-by: Jiapeng Chong <jiapeng.chong(a)linux.alibaba.com>
---
drivers/usb/gadget/udc/aspeed_udc.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
index 1fc15228ff15..2c3dc80d6b8c 100644
--- a/drivers/usb/gadget/udc/aspeed_udc.c
+++ b/drivers/usb/gadget/udc/aspeed_udc.c
@@ -1543,7 +1543,6 @@ static int ast_udc_probe(struct platform_device *pdev)
/* Find interrupt and install handler */
udc->irq = platform_get_irq(pdev, 0);
if (udc->irq < 0) {
- dev_err(&pdev->dev, "Failed to get interrupt\n");
rc = udc->irq;
goto err;
}
--
2.20.1.7.g153144c
On Wed, Jun 15, 2022 at 06:48:33PM +0800, heliang wrote:
> In tegra_uart_init(), of_find_matching_node() will return a node
> pointer with refcount incremented. We should use of_node_put()
> when it is not used anymore.
>
> Signed-off-by: heliang <windhl(a)126.com>
We need a real name please, one you sign documents with.
thanks,
greg k-h
RFC
I don't have a good name for this yet and I did not spend
any time on documentataion (for that reason)
We create fences (out fences) as part of operations execution, which
are short-lived objects, we want to release all memory after operation
execution is completed or when operation gets cancelled/deleted via
ioctl().
This creates a bit of a problem. DMA fences are refcounted objects and
exporter never knows when importer imports a fence or puts its refcount,
so exporter never knows when fence will be destoyed, which should not
be a problem for refcounted objects, but here comes the twist...
operation A - creates and exports out fence X
... user-space imports fence X
operation A - finishes execution, signals fence X
kfree operation A, put dma_fence
DMA fences are designed to borrow spinlock that DMA fences use to
protect struct dma_fence members:
struct dma_fence {
spinlock_t *lock;
const struct dma_fence_ops *ops;
.....
};
void dma_fence_init(struct dma_fence *fence,
const struct dma_fence_ops *ops,
spinlock_t *lock,
u64 context,
u64 seqno);
So the `lock` should have at least same lifespan as the DMA fence
that borrows it, which is impossible to guarantee in our case. When
we kfree operation A struct we also kfree ->lock that operation
lends to DMA fence, which outlives operation A (depending on what
fence importers do and when they drop imported fence refcount).
This patch adds a new memnber to struct dma_fence: __lock_inplace.
Which is a lock that DMA fence will use to protect its own data when
it cannot reliably borrow a lock from the outside object.
I also had a patch that puts inplace and borrowed locks to an unnamed
uninon and adds one more dma_fence_flag_bits to distinguish between
fences with borrowed and inplace locks
struct dma_fence {
uninon {
spinlock_t *lock;
spinlock_t __lock_inplace;
};
...
};
And then instead of locking/unlocking ->lock directly we would use
dma_fence_lock_irqsave()/dma_fence_unlock_irqrestore() macros which
would check fence flags and either use borrowed lock or inplace lock.
But after seeing how owten drivers directly access fence ->lock I
decided to scratch that approach and just add extra spinlock member.
Not-Yet-Signed-off-by: Sergey Senozhatsky <senozhatsky(a)chromium.org>
---
drivers/dma-buf/dma-fence.c | 10 ++++++++++
include/linux/dma-fence.h | 6 ++++++
2 files changed, 16 insertions(+)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 066400ed8841..7ae40b8adb73 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -958,3 +958,13 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
trace_dma_fence_init(fence);
}
EXPORT_SYMBOL(dma_fence_init);
+
+void dma_fence_inplace_lock_init(struct dma_fence *fence,
+ const struct dma_fence_ops *ops,
+ u64 context, u64 seqno)
+{
+ spin_lock_init(&fence->__lock_inplace);
+
+ dma_fence_init(fence, ops, &fence->__lock_inplace, context, seqno);
+}
+EXPORT_SYMBOL(dma_fence_inplace_lock_init);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 1ea691753bd3..6b15a0d2eccf 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -64,6 +64,8 @@ struct dma_fence_cb;
*/
struct dma_fence {
spinlock_t *lock;
+ spinlock_t __lock_inplace;
+
const struct dma_fence_ops *ops;
/*
* We clear the callback list on kref_put so that by the time we
@@ -262,6 +264,10 @@ struct dma_fence_ops {
void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
spinlock_t *lock, u64 context, u64 seqno);
+void dma_fence_inplace_lock_init(struct dma_fence *fence,
+ const struct dma_fence_ops *ops,
+ u64 context, u64 seqno);
+
void dma_fence_release(struct kref *kref);
void dma_fence_free(struct dma_fence *fence);
void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq);
--
2.36.1.124.g0e6072fb45-goog