From: Steven Rostedt <rostedt@goodmis.org>
To: Xiang Gao <gxxa03070307@gmail.com>
Cc: sumit.semwal@linaro.org, christian.koenig@amd.com,
mhiramat@kernel.org, linux-media@vger.kernel.org,
dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
linux-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com,
dhowells@redhat.com, kuba@kernel.org, brauner@kernel.org,
akpm@linux-foundation.org, linux-trace-kernel@vger.kernel.org,
gaoxiang17 <gaoxiang17@xiaomi.com>
Subject: Re: [PATCH v2] dma-buf: add some tracepoints to debug.
Date: Tue, 25 Nov 2025 20:39:24 -0500 [thread overview]
Message-ID: <20251125203924.3bd59b2c@gandalf.local.home> (raw)
In-Reply-To: <20251125162949.220488-1-gxxa03070307@gmail.com>
On Wed, 26 Nov 2025 00:29:49 +0800
Xiang Gao <gxxa03070307@gmail.com> wrote:
> +++ b/include/trace/events/dma_buf.h
> @@ -0,0 +1,281 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM dma_buf
> +
> +#if !defined(_TRACE_DMA_BUF_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_DMA_BUF_H
> +
> +#include <linux/dma-buf.h>
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(dma_buf_export,
> +
> + TP_PROTO(struct dma_buf *dmabuf),
> +
> + TP_ARGS(dmabuf),
> +
> + TP_STRUCT__entry(
> + __string(exp_name, dmabuf->exp_name)
> + __string(name, dmabuf->name)
> + __field(size_t, size)
> + __field(ino_t, ino)
> + __field(long, f_refcnt)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(exp_name);
> + spin_lock(&dmabuf->name_lock);
> + __assign_str(name);
> + spin_unlock(&dmabuf->name_lock);
The above isn't doing what you think it's doing. The name is assigned
before this by the above __string(name, dmabuf->name).
You really shouldn't be taking any locks in a tracepoint. A tracepoint is a
callback, that isn't called most of the time. You could be hiding very
hard to find deadlocks by taking a lock in a tracepoint callback.
You need to take the lock around the tracepoint call itself where it is
called in the code. Not in the TRACE_EVENT.
You may need to have something like:
@@ -220,6 +223,8 @@ static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma)
dmabuf->size >> PAGE_SHIFT)
return -EINVAL;
+ if (trace_dma_buf_mmap_internal_enabled()) {
+ guard(spinlock)(&dmabuf->namelock);
+ trace_dma_buf_mmap_internal(dmabuf);
+ }
+
return dmabuf->ops->mmap(dmabuf, vma);
}
The "trace_dma_buf_mmap_internal_enabled()" is a static branch, where it is
either a nop or a jump to the tracing code. It's not a normal conditional
branch. It acts the same as tracepoints themselves do.
> + __entry->size = dmabuf->size;
> + __entry->ino = dmabuf->file->f_inode->i_ino;
> + __entry->f_refcnt = file_count(dmabuf->file);
> + ),
> +
> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu f_refcnt=%ld",
> + __get_str(exp_name),
> + __get_str(name),
> + __entry->size,
> + __entry->ino,
> + __entry->f_refcnt)
> +);
Below seems to be a lot of very similar TRACE_EVENT()s. A TRACE_EVENT() is
literally defined as:
#define TRACE_EVENT(name, proto, args, tstruct, assign, print) \
DECLARE_EVENT_CLASS(name, \
PARAMS(proto), \
PARAMS(args), \
PARAMS(tstruct), \
PARAMS(assign), \
PARAMS(print)); \
DEFINE_EVENT(name, name, PARAMS(proto), PARAMS(args));
That is, it is both a DECLARE_EVENT_CLASS() and a DEFINE_EVENT(). You can
make one DECLARE_EVENT_CLASS() and use many DEFINE_EVENT()s with it.
Each DECLARE_EVENT_CLASS() takes up around 4 to 5 kilobytes of memory. Each
DEFINE_EVENT() takes around 300 bytes to 1K of memory. The more
DEFINE_EVENT()s you use with a single DECLARE_EVENT_CLASS(), the more
memory you save. Please try to do that.
-- Steve
> +
> +TRACE_EVENT(dma_buf_fd,
> +
> + TP_PROTO(struct dma_buf *dmabuf, int fd),
> +
> + TP_ARGS(dmabuf, fd),
> +
> + TP_STRUCT__entry(
> + __string(exp_name, dmabuf->exp_name)
> + __string(name, dmabuf->name)
> + __field(size_t, size)
> + __field(ino_t, ino)
> + __field(int, fd)
> + __field(long, f_refcnt)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(exp_name);
> + spin_lock(&dmabuf->name_lock);
> + __assign_str(name);
> + spin_unlock(&dmabuf->name_lock);
> + __entry->size = dmabuf->size;
> + __entry->ino = dmabuf->file->f_inode->i_ino;
> + __entry->fd = fd;
> + __entry->f_refcnt = file_count(dmabuf->file);
> + ),
> +
> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu fd=%d f_refcnt=%ld",
> + __get_str(exp_name),
> + __get_str(name),
> + __entry->size,
> + __entry->ino,
> + __entry->fd,
> + __entry->f_refcnt)
> +);
> +
> +TRACE_EVENT(dma_buf_mmap_internal,
> +
> + TP_PROTO(struct dma_buf *dmabuf),
> +
> + TP_ARGS(dmabuf),
> +
> + TP_STRUCT__entry(
> + __string(exp_name, dmabuf->exp_name)
> + __string(name, dmabuf->name)
> + __field(size_t, size)
> + __field(ino_t, ino)
> + __field(long, f_refcnt)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(exp_name);
> + spin_lock(&dmabuf->name_lock);
> + __assign_str(name);
> + spin_unlock(&dmabuf->name_lock);
> + __entry->size = dmabuf->size;
> + __entry->ino = dmabuf->file->f_inode->i_ino;
> + __entry->f_refcnt = file_count(dmabuf->file);
> + ),
> +
> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu f_refcnt=%ld",
> + __get_str(exp_name),
> + __get_str(name),
> + __entry->size,
> + __entry->ino,
> + __entry->f_refcnt)
> +);
> +
> +TRACE_EVENT(dma_buf_mmap,
> +
> + TP_PROTO(struct dma_buf *dmabuf),
> +
> + TP_ARGS(dmabuf),
> +
> + TP_STRUCT__entry(
> + __string(exp_name, dmabuf->exp_name)
> + __string(name, dmabuf->name)
> + __field(size_t, size)
> + __field(ino_t, ino)
> + __field(long, f_refcnt)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(exp_name);
> + spin_lock(&dmabuf->name_lock);
> + __assign_str(name);
> + spin_unlock(&dmabuf->name_lock);
> + __entry->size = dmabuf->size;
> + __entry->ino = dmabuf->file->f_inode->i_ino;
> + __entry->f_refcnt = file_count(dmabuf->file);
> + ),
> +
> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu f_refcnt=%ld",
> + __get_str(exp_name),
> + __get_str(name),
> + __entry->size,
> + __entry->ino,
> + __entry->f_refcnt)
> +);
> +
> +TRACE_EVENT(dma_buf_attach,
> +
> + TP_PROTO(struct dma_buf *dmabuf, struct device *dev),
> +
> + TP_ARGS(dmabuf, dev),
> +
> + TP_STRUCT__entry(
> + __string(dname, dev_name(dev))
> + __string(exp_name, dmabuf->exp_name)
> + __string(name, dmabuf->name)
> + __field(size_t, size)
> + __field(ino_t, ino)
> + __field(long, f_refcnt)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(dname);
> + __assign_str(exp_name);
> + spin_lock(&dmabuf->name_lock);
> + __assign_str(name);
> + spin_unlock(&dmabuf->name_lock);
> + __entry->size = dmabuf->size;
> + __entry->ino = dmabuf->file->f_inode->i_ino;
> + __entry->f_refcnt = file_count(dmabuf->file);
> + ),
> +
> + TP_printk("dev_name=%s exp_name=%s name=%s size=%zu ino=%lu f_refcnt=%ld",
> + __get_str(dname),
> + __get_str(exp_name),
> + __get_str(name),
> + __entry->size,
> + __entry->ino,
> + __entry->f_refcnt)
> +);
> +
> +TRACE_EVENT(dma_buf_detach,
> +
> + TP_PROTO(struct dma_buf *dmabuf),
> +
> + TP_ARGS(dmabuf),
> +
> + TP_STRUCT__entry(
> + __string(exp_name, dmabuf->exp_name)
> + __string(name, dmabuf->name)
> + __field(size_t, size)
> + __field(ino_t, ino)
> + __field(long, f_refcnt)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(exp_name);
> + spin_lock(&dmabuf->name_lock);
> + __assign_str(name);
> + spin_unlock(&dmabuf->name_lock);
> + __entry->size = dmabuf->size;
> + __entry->ino = dmabuf->file->f_inode->i_ino;
> + __entry->f_refcnt = file_count(dmabuf->file);
> + ),
> +
> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu f_refcnt=%ld",
> + __get_str(exp_name),
> + __get_str(name),
> + __entry->size,
> + __entry->ino,
> + __entry->f_refcnt)
> +);
> +
> +TRACE_EVENT(dma_buf_get,
> +
> + TP_PROTO(int fd, struct file *file),
> +
> + TP_ARGS(fd, file),
> +
> + TP_STRUCT__entry(
> + __string(exp_name, ((struct dma_buf *)file->private_data)->exp_name)
> + __string(name, ((struct dma_buf *)file->private_data)->name)
> + __field(size_t, size)
> + __field(ino_t, ino)
> + __field(int, fd)
> + __field(long, f_refcnt)
> + ),
> +
> + TP_fast_assign(
> + struct dma_buf *dmabuf = (struct dma_buf *)file->private_data;
> +
> + __assign_str(exp_name);
> + spin_lock(&dmabuf->name_lock);
> + __assign_str(name);
> + spin_unlock(&dmabuf->name_lock);
> + __entry->size = dmabuf->size;
> + __entry->ino = dmabuf->file->f_inode->i_ino;
> + __entry->fd = fd;
> + __entry->f_refcnt = file_count(file);
> + ),
> +
> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu fd=%d f_refcnt=%ld",
> + __get_str(exp_name),
> + __get_str(name),
> + __entry->size,
> + __entry->ino,
> + __entry->fd,
> + __entry->f_refcnt)
> +);
> +
> +TRACE_EVENT(dma_buf_put,
> +
> + TP_PROTO(struct dma_buf *dmabuf),
> +
> + TP_ARGS(dmabuf),
> +
> + TP_STRUCT__entry(
> + __string(exp_name, dmabuf->exp_name)
> + __string(name, dmabuf->name)
> + __field(size_t, size)
> + __field(ino_t, ino)
> + __field(long, f_refcnt)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(exp_name);
> + spin_lock(&dmabuf->name_lock);
> + __assign_str(name);
> + spin_unlock(&dmabuf->name_lock);
> + __entry->size = dmabuf->size;
> + __entry->ino = dmabuf->file->f_inode->i_ino;
> + __entry->f_refcnt = file_count(dmabuf->file);
> + ),
> +
> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu f_refcnt=%ld",
> + __get_str(exp_name),
> + __get_str(name),
> + __entry->size,
> + __entry->ino,
> + __entry->f_refcnt)
> +);
> +
> +#endif /* _TRACE_DMA_BUF_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
prev parent reply other threads:[~2025-11-26 1:38 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-25 16:29 [PATCH v2] dma-buf: add some tracepoints to debug Xiang Gao
2025-11-26 1:39 ` Steven Rostedt [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251125203924.3bd59b2c@gandalf.local.home \
--to=rostedt@goodmis.org \
--cc=akpm@linux-foundation.org \
--cc=brauner@kernel.org \
--cc=christian.koenig@amd.com \
--cc=dhowells@redhat.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=gaoxiang17@xiaomi.com \
--cc=gxxa03070307@gmail.com \
--cc=kuba@kernel.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mhiramat@kernel.org \
--cc=sumit.semwal@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox