* [RFC PATCH 8/8] xfs: Lift the bs == ps restriction for HW buffered atomic writes
From: Ojaswin Mujoo @ 2025-11-12 11:06 UTC (permalink / raw)
To: Christian Brauner, djwong, ritesh.list, john.g.garry, tytso,
willy, dchinner, hch
Cc: linux-xfs, linux-kernel, linux-ext4, linux-fsdevel, linux-mm,
jack, nilay, martin.petersen, rostedt, axboe, linux-block,
linux-trace-kernel
In-Reply-To: <cover.1762945505.git.ojaswin@linux.ibm.com>
Now that we support bs < ps for HW atomic writes, lift this restirction from XFS
statx reporting
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
fs/xfs/xfs_iops.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 67d370947d95..5bd31aacf514 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -622,10 +622,9 @@ xfs_get_atomic_write_min(
return bs;
}
/*
- * Buffered IO only supports hw single block atomic writes and bs == ps
- * configurations.
+ * Buffered IO only supports hw single block atomic writes
*/
- if (xfs_inode_can_hw_atomic_write(ip) && bs == PAGE_SIZE)
+ if (xfs_inode_can_hw_atomic_write(ip))
return bs;
return 0;
@@ -661,10 +660,9 @@ xfs_get_atomic_write_max(
return XFS_FSB_TO_B(mp, mp->m_groups[XG_TYPE_AG].awu_max);
}
/*
- * Buffered IO only supports hw single block atomic writes and bs == ps
- * configurations.
+ * Buffered IO only supports hw single block atomic writes
*/
- if (xfs_inode_can_hw_atomic_write(ip) && bs == PAGE_SIZE)
+ if (xfs_inode_can_hw_atomic_write(ip))
return bs;
return 0;
--
2.51.0
^ permalink raw reply related
* [RFC PATCH 6/8] xfs: Report atomic write min and max for buf io as well
From: Ojaswin Mujoo @ 2025-11-12 11:06 UTC (permalink / raw)
To: Christian Brauner, djwong, ritesh.list, john.g.garry, tytso,
willy, dchinner, hch
Cc: linux-xfs, linux-kernel, linux-ext4, linux-fsdevel, linux-mm,
jack, nilay, martin.petersen, rostedt, axboe, linux-block,
linux-trace-kernel
In-Reply-To: <cover.1762945505.git.ojaswin@linux.ibm.com>
Now that we can reliably perform a HW based single block buffered atomic
write for page size == blocksize, start advertising it in XFS.
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
fs/xfs/xfs_iops.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index f036c46b19c5..67d370947d95 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -604,9 +604,10 @@ xfs_get_atomic_write_min(
struct xfs_inode *ip,
bool is_dio)
{
- if (is_dio) {
- struct xfs_mount *mp = ip->i_mount;
+ struct xfs_mount *mp = ip->i_mount;
+ uint32_t bs = mp->m_sb.sb_blocksize;
+ if (is_dio) {
/*
* If we can complete an atomic write via atomic out of place writes,
* then advertise a minimum size of one fsblock. Without this
@@ -618,10 +619,15 @@ xfs_get_atomic_write_min(
*/
if (xfs_inode_can_hw_atomic_write(ip) ||
xfs_inode_can_sw_atomic_write(ip))
- return mp->m_sb.sb_blocksize;
+ return bs;
}
+ /*
+ * Buffered IO only supports hw single block atomic writes and bs == ps
+ * configurations.
+ */
+ if (xfs_inode_can_hw_atomic_write(ip) && bs == PAGE_SIZE)
+ return bs;
- /* buffered IO not supported yet so return 0 right away */
return 0;
}
@@ -630,7 +636,8 @@ xfs_get_atomic_write_max(
struct xfs_inode *ip,
bool is_dio)
{
- struct xfs_mount *mp = ip->i_mount;
+ struct xfs_mount *mp = ip->i_mount;
+ uint32_t bs = mp->m_sb.sb_blocksize;
if (is_dio) {
/*
@@ -640,7 +647,7 @@ xfs_get_atomic_write_max(
*/
if (!xfs_inode_can_sw_atomic_write(ip)) {
if (xfs_inode_can_hw_atomic_write(ip))
- return mp->m_sb.sb_blocksize;
+ return bs;
return 0;
}
@@ -653,8 +660,13 @@ xfs_get_atomic_write_max(
return XFS_FSB_TO_B(mp, mp->m_groups[XG_TYPE_RTG].awu_max);
return XFS_FSB_TO_B(mp, mp->m_groups[XG_TYPE_AG].awu_max);
}
+ /*
+ * Buffered IO only supports hw single block atomic writes and bs == ps
+ * configurations.
+ */
+ if (xfs_inode_can_hw_atomic_write(ip) && bs == PAGE_SIZE)
+ return bs;
- /* buffered IO not supported yet so return 0 right away */
return 0;
}
@@ -679,7 +691,7 @@ xfs_get_atomic_write_max_opt(
return min(awu_max, xfs_inode_buftarg(ip)->bt_awu_max);
}
- /* buffered IO not supported yet so return 0 right away */
+ /* buffered IO for now only supports 1 filesyste block so max_opt is 0 */
return 0;
}
--
2.51.0
^ permalink raw reply related
* [RFC PATCH 5/8] iomap: pin pages for RWF_ATOMIC buffered write
From: Ojaswin Mujoo @ 2025-11-12 11:06 UTC (permalink / raw)
To: Christian Brauner, djwong, ritesh.list, john.g.garry, tytso,
willy, dchinner, hch
Cc: linux-xfs, linux-kernel, linux-ext4, linux-fsdevel, linux-mm,
jack, nilay, martin.petersen, rostedt, axboe, linux-block,
linux-trace-kernel
In-Reply-To: <cover.1762945505.git.ojaswin@linux.ibm.com>
Currently, if the user buffer crosses a page boundary (even if it is a
single block write), we can end up with the following scenario:
1. We prefault the 2 user pages in iomap_write_iter.
2. Due to memory pressure, 1 page is reclaimed.
3. copy_folio_from_iter_atomic() ends up doing a short copy
This is unacceptable for RWF_ATOMIC writes since at this point our folio
is already dirty and we will be unable to recover the old data to
guarantee the atomic semantics.
Get past this issue by taking inspiration from the direct IO code and
performaing the following steps for RWF_ATOMIC:
1. Pin all the user pages. This pins the physical page but the user
space mapping can still be unmapped by reclaim code, which can still
cause a short write in copy_folio_from_iter_atomic().
2. To get past the user mapping getting unmapped, don't use the user
iter anymore but rather create a bvec out of the pinned pages. This
way we area safe from unmapping since we use the kernel's mapping
directly. Having a bvec also allows us directly reuse
copy_folio_from_iter_atomic().
This ensures we should never see a short write since we prefault and pin
the pages in case of RWF_ATOMIC
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
fs/iomap/buffered-io.c | 154 +++++++++++++++++++++++++++++++++++++----
fs/read_write.c | 11 ---
2 files changed, 140 insertions(+), 25 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 947c76c2688a..e7dbe9bcb439 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1027,6 +1027,73 @@ static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied,
return __iomap_write_end(iter, pos, len, copied, folio);
}
+/*
+ * Prepare an atomic write by pinning its pages and creating a ITER_BVEC out of
+ * them. This function also advances the original iter. Incase we encounter any
+ * error later, we revert the progress.
+ */
+static int iomap_atomic_write_prep(struct iov_iter *i,
+ struct iov_iter *atomic_iter,
+ struct bio_vec **atomic_bvecs,
+ struct page ***pages)
+{
+ size_t pg_off;
+ int bytes_pinned = 0;
+ int k = 0;
+ int len, total_len = 0, off;
+ int pinned_pgs = 0;
+ struct bio_vec *tmp_bvecs;
+
+ bytes_pinned = iov_iter_extract_pages(i, pages, iov_iter_count(i),
+ UINT_MAX, 0, &pg_off);
+ /*
+ * iov_iter_extract_pages advances the iter but we didn't
+ * do any work yet, so revert.
+ */
+ iov_iter_revert(i, bytes_pinned);
+
+ pinned_pgs = DIV_ROUND_UP(pg_off + bytes_pinned, PAGE_SIZE);
+
+ tmp_bvecs = kcalloc(pinned_pgs, sizeof(struct bio_vec), GFP_KERNEL);
+
+ if (unlikely(!tmp_bvecs))
+ return -ENOMEM;
+
+ for (struct page *p; k < pinned_pgs && iov_iter_count(i); k++) {
+ p = (*pages)[k];
+ off = (unsigned long)((char *)i->ubuf + i->iov_offset) %
+ PAGE_SIZE;
+ len = min(PAGE_SIZE - off, iov_iter_count(i));
+ bvec_set_page(&tmp_bvecs[k], p, len, off);
+ iov_iter_advance(i, len);
+ total_len += len;
+ }
+
+ iov_iter_bvec(atomic_iter, ITER_SOURCE, tmp_bvecs, k, total_len);
+
+ *atomic_bvecs = tmp_bvecs;
+ return pinned_pgs;
+}
+
+static void iomap_atomic_write_cleanup(struct page ***pages, int *pinned_pgs,
+ struct bio_vec **atomic_bvecs)
+{
+ if (*pinned_pgs) {
+ unpin_user_pages(*pages, *pinned_pgs);
+ *pinned_pgs = 0;
+ }
+
+ if (*pages) {
+ kfree(*pages);
+ *pages = NULL;
+ }
+
+ if (*atomic_bvecs) {
+ kfree(*atomic_bvecs);
+ *atomic_bvecs = NULL;
+ }
+}
+
static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i,
const struct iomap_write_ops *write_ops)
{
@@ -1035,6 +1102,11 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i,
struct address_space *mapping = iter->inode->i_mapping;
size_t chunk = mapping_max_folio_size(mapping);
unsigned int bdp_flags = (iter->flags & IOMAP_NOWAIT) ? BDP_ASYNC : 0;
+ bool is_atomic = iter->flags & IOMAP_ATOMIC;
+ struct page **pages = NULL;
+ int pinned_pgs;
+ struct iov_iter atomic_iter = {0};
+ struct bio_vec *atomic_bvecs = NULL;
do {
struct folio *folio;
@@ -1057,19 +1129,52 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i,
if (bytes > iomap_length(iter))
bytes = iomap_length(iter);
- /*
- * Bring in the user page that we'll copy from _first_.
- * Otherwise there's a nasty deadlock on copying from the
- * same page as we're writing to, without it being marked
- * up-to-date.
- *
- * For async buffered writes the assumption is that the user
- * page has already been faulted in. This can be optimized by
- * faulting the user page.
- */
- if (unlikely(fault_in_iov_iter_readable(i, bytes) == bytes)) {
- status = -EFAULT;
- break;
+ if (is_atomic) {
+ /*
+ * If the user pages get reclaimed or unmapped, we could
+ * end up faulting and doing a short copy in
+ * copy_folio_from_iter_atomic(), which is undesirable
+ * for RWF_ATOMIC. Hence:
+ *
+ * 1. Pin the pages to protect against reclaim
+ *
+ * 2. Iter's user page can still get unmapped from user
+ * page table leading to short copy. Protect against
+ * this by instead using an ITER_BVEC created out of
+ * the pinned pages.
+ */
+
+ pinned_pgs = iomap_atomic_write_prep(i, &atomic_iter, &atomic_bvecs,
+ &pages);
+ if (unlikely(pinned_pgs <= 0)) {
+ status = pinned_pgs;
+ break;
+ }
+
+ if (pinned_pgs << PAGE_SHIFT < bytes) {
+ WARN_RATELIMIT(
+ true,
+ "Couldn't pin bytes for atomic write: pinned: %d, needed: %lld",
+ pinned_pgs << PAGE_SHIFT, bytes);
+ status = -EFAULT;
+ break;
+ }
+
+ } else {
+ /*
+ * Bring in the user page that we'll copy from _first_.
+ * Otherwise there's a nasty deadlock on copying from the
+ * same page as we're writing to, without it being marked
+ * up-to-date.
+ *
+ * For async buffered writes the assumption is that the user
+ * page has already been faulted in. This can be optimized by
+ * faulting the user page.
+ */
+ if (unlikely(fault_in_iov_iter_readable(i, bytes) == bytes)) {
+ status = -EFAULT;
+ break;
+ }
}
status = iomap_write_begin(iter, write_ops, &folio, &offset,
@@ -1086,9 +1191,27 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i,
if (mapping_writably_mapped(mapping))
flush_dcache_folio(folio);
- copied = copy_folio_from_iter_atomic(folio, offset, bytes, i);
+ copied = copy_folio_from_iter_atomic(
+ folio, offset, bytes, is_atomic ? &atomic_iter : i);
written = iomap_write_end(iter, bytes, copied, folio) ?
copied : 0;
+ if (is_atomic) {
+ if (written != bytes) {
+ /*
+ * short copy so revert the iter accordingly.
+ * This should never happen ideally
+ */
+ WARN_RATELIMIT(
+ 1,
+ "Short atomic write: bytes_pinned:%d bytes:%lld written:%lld\n",
+ pinned_pgs << PAGE_SHIFT, bytes,
+ written);
+ iov_iter_revert(i,
+ iov_iter_count(&atomic_iter));
+ }
+ iomap_atomic_write_cleanup(&pages, &pinned_pgs,
+ &atomic_bvecs);
+ }
/*
* Update the in-memory inode size after copying the data into
@@ -1130,6 +1253,9 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i,
}
} while (iov_iter_count(i) && iomap_length(iter));
+ if (is_atomic)
+ iomap_atomic_write_cleanup(&pages, &pinned_pgs, &atomic_bvecs);
+
return total_written ? 0 : status;
}
diff --git a/fs/read_write.c b/fs/read_write.c
index 37546aa40f0d..7e064561cc4b 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1833,17 +1833,6 @@ int generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter)
*/
if (sb->s_blocksize != PAGE_SIZE)
return -EOPNOTSUPP;
-
- /*
- * If the user buffer of atomic write crosses page boundary,
- * there's a possibility of short write, example if 1 user page
- * could not be faulted or got reclaimed before the copy
- * operation. For now don't allow such a scenario by ensuring
- * user buffer is page aligned.
- */
- if (!PAGE_ALIGNED(iov_iter_alignment(iter)))
- return -EOPNOTSUPP;
-
}
return 0;
--
2.51.0
^ permalink raw reply related
* [RFC PATCH 4/8] iomap: buffered atomic write support
From: Ojaswin Mujoo @ 2025-11-12 11:06 UTC (permalink / raw)
To: Christian Brauner, djwong, ritesh.list, john.g.garry, tytso,
willy, dchinner, hch
Cc: linux-xfs, linux-kernel, linux-ext4, linux-fsdevel, linux-mm,
jack, nilay, martin.petersen, rostedt, axboe, linux-block,
linux-trace-kernel
In-Reply-To: <cover.1762945505.git.ojaswin@linux.ibm.com>
Add special handling of PG_atomic flag to iomap buffered write path.
To flag an iomap iter for an atomic write, set IOMAP_ATOMIC. For a folio
associated with a write which has IOMAP_ATOMIC set, set PG_atomic.
Otherwise, when IOMAP_ATOMIC is unset, clear PG_atomic.
This means that for an "atomic" folio which has not been written back,
it loses it "atomicity". So if userspace issues a write with RWF_ATOMIC
set and another write with RWF_ATOMIC unset, that folio is not written back
atomically. For such a scenario to occur, it would be considered a userspace
usage error.
To ensure that a buffered atomic write is written back atomically when
the write syscall returns, RWF_SYNC or similar needs to be used (in
conjunction with RWF_ATOMIC).
Only a single BIO should ever be submitted for an atomic write. So
modify iomap_add_to_ioend() to ensure that we don't try to write back an
atomic folio as part of a larger mixed-atomicity BIO.
In iomap_alloc_ioend(), handle an atomic write by setting REQ_ATOMIC for
the allocated BIO. When a folio is written back, again clear PG_atomic,
as it is no longer required.
Currently, RWF_ATOMIC with buffered IO is limited to single block
size writes, and has 2 main restrictions:
1. Only blocksize == pagesize is supported
2. Writes where the user buffer is not aligned to PAGE_SIZE are not
supported
For more details, refer to the comment in generic_atomic_write_valid()
Co-developed-by: John Garry <john.g.garry@oracle.com>
Signed-off-by: John Garry <john.g.garry@oracle.com>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
fs/iomap/buffered-io.c | 48 ++++++++++++++++++++++++++++++++++++------
fs/iomap/ioend.c | 18 ++++++++++++----
fs/read_write.c | 34 ++++++++++++++++++++++++++++--
include/linux/iomap.h | 2 ++
4 files changed, 89 insertions(+), 13 deletions(-)
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index f099c086cbe8..947c76c2688a 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -850,11 +850,13 @@ static int iomap_write_begin(struct iomap_iter *iter,
{
const struct iomap *srcmap = iomap_iter_srcmap(iter);
loff_t pos;
- u64 len = min_t(u64, SIZE_MAX, iomap_length(iter));
+ u64 orig_len = min_t(u64, SIZE_MAX, iomap_length(iter));
+ u64 len;
struct folio *folio;
int status = 0;
+ bool is_atomic = iter->flags & IOMAP_ATOMIC;
- len = min_not_zero(len, *plen);
+ len = min_not_zero(orig_len, *plen);
*foliop = NULL;
*plen = 0;
@@ -922,6 +924,11 @@ static int iomap_write_begin(struct iomap_iter *iter,
if (unlikely(status))
goto out_unlock;
+ if (is_atomic && (len != orig_len)) {
+ status = -EINVAL;
+ goto out_unlock;
+ }
+
*foliop = folio;
*plen = len;
return 0;
@@ -931,7 +938,7 @@ static int iomap_write_begin(struct iomap_iter *iter,
return status;
}
-static bool __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
+static bool __iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
size_t copied, struct folio *folio)
{
flush_dcache_folio(folio);
@@ -951,7 +958,27 @@ static bool __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
return false;
iomap_set_range_uptodate(folio, offset_in_folio(folio, pos), len);
iomap_set_range_dirty(folio, offset_in_folio(folio, pos), copied);
- filemap_dirty_folio(inode->i_mapping, folio);
+ filemap_dirty_folio(iter->inode->i_mapping, folio);
+
+ /*
+ * Policy: non atomic write over a previously atomic range makes the
+ * range non-atomic. Handle this here.
+ */
+ if (iter->flags & IOMAP_ATOMIC) {
+ if (copied < len) {
+ /*
+ * A short atomic write is only okay as long as nothing
+ * is written at all. If we have a partial write, there
+ * is a bug in our code.
+ */
+ WARN_ON_ONCE(copied != 0);
+
+ return false;
+ }
+ folio_set_atomic(folio);
+ } else
+ folio_clear_atomic(folio);
+
return true;
}
@@ -997,7 +1024,7 @@ static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied,
return bh_written == copied;
}
- return __iomap_write_end(iter->inode, pos, len, copied, folio);
+ return __iomap_write_end(iter, pos, len, copied, folio);
}
static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i,
@@ -1124,6 +1151,8 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i,
iter.flags |= IOMAP_NOWAIT;
if (iocb->ki_flags & IOCB_DONTCACHE)
iter.flags |= IOMAP_DONTCACHE;
+ if (iocb->ki_flags & IOCB_ATOMIC)
+ iter.flags |= IOMAP_ATOMIC;
while ((ret = iomap_iter(&iter, ops)) > 0)
iter.status = iomap_write_iter(&iter, i, write_ops);
@@ -1588,6 +1617,7 @@ static int iomap_folio_mkwrite_iter(struct iomap_iter *iter,
} else {
WARN_ON_ONCE(!folio_test_uptodate(folio));
folio_mark_dirty(folio);
+ folio_clear_atomic(folio);
}
return iomap_iter_advance(iter, length);
@@ -1642,8 +1672,10 @@ void iomap_finish_folio_write(struct inode *inode, struct folio *folio,
WARN_ON_ONCE(i_blocks_per_folio(inode, folio) > 1 && !ifs);
WARN_ON_ONCE(ifs && atomic_read(&ifs->write_bytes_pending) <= 0);
- if (!ifs || atomic_sub_and_test(len, &ifs->write_bytes_pending))
+ if (!ifs || atomic_sub_and_test(len, &ifs->write_bytes_pending)) {
+ folio_clear_atomic(folio);
folio_end_writeback(folio);
+ }
}
EXPORT_SYMBOL_GPL(iomap_finish_folio_write);
@@ -1807,8 +1839,10 @@ int iomap_writeback_folio(struct iomap_writepage_ctx *wpc, struct folio *folio)
if (atomic_dec_and_test(&ifs->write_bytes_pending))
folio_end_writeback(folio);
} else {
- if (!wb_pending)
+ if (!wb_pending) {
+ folio_clear_atomic(folio);
folio_end_writeback(folio);
+ }
}
mapping_set_error(inode->i_mapping, error);
return error;
diff --git a/fs/iomap/ioend.c b/fs/iomap/ioend.c
index b49fa75eab26..c129a695ceca 100644
--- a/fs/iomap/ioend.c
+++ b/fs/iomap/ioend.c
@@ -98,13 +98,17 @@ int iomap_ioend_writeback_submit(struct iomap_writepage_ctx *wpc, int error)
EXPORT_SYMBOL_GPL(iomap_ioend_writeback_submit);
static struct iomap_ioend *iomap_alloc_ioend(struct iomap_writepage_ctx *wpc,
- loff_t pos, u16 ioend_flags)
+ loff_t pos, u16 ioend_flags,
+ bool atomic)
{
struct bio *bio;
+ blk_opf_t opf = REQ_OP_WRITE | wbc_to_write_flags(wpc->wbc);
+
+ if (atomic)
+ opf |= REQ_ATOMIC;
bio = bio_alloc_bioset(wpc->iomap.bdev, BIO_MAX_VECS,
- REQ_OP_WRITE | wbc_to_write_flags(wpc->wbc),
- GFP_NOFS, &iomap_ioend_bioset);
+ opf, GFP_NOFS, &iomap_ioend_bioset);
bio->bi_iter.bi_sector = iomap_sector(&wpc->iomap, pos);
bio->bi_write_hint = wpc->inode->i_write_hint;
wbc_init_bio(wpc->wbc, bio);
@@ -122,6 +126,9 @@ static bool iomap_can_add_to_ioend(struct iomap_writepage_ctx *wpc, loff_t pos,
if ((ioend_flags & IOMAP_IOEND_NOMERGE_FLAGS) !=
(ioend->io_flags & IOMAP_IOEND_NOMERGE_FLAGS))
return false;
+ if ((ioend_flags & IOMAP_IOEND_ATOMIC) ||
+ (ioend->io_flags & IOMAP_IOEND_ATOMIC))
+ return false;
if (pos != ioend->io_offset + ioend->io_size)
return false;
if (!(wpc->iomap.flags & IOMAP_F_ANON_WRITE) &&
@@ -156,6 +163,7 @@ ssize_t iomap_add_to_ioend(struct iomap_writepage_ctx *wpc, struct folio *folio,
unsigned int ioend_flags = 0;
unsigned int map_len = min_t(u64, dirty_len,
wpc->iomap.offset + wpc->iomap.length - pos);
+ bool is_atomic = folio_test_atomic(folio);
int error;
trace_iomap_add_to_ioend(wpc->inode, pos, dirty_len, &wpc->iomap);
@@ -180,6 +188,8 @@ ssize_t iomap_add_to_ioend(struct iomap_writepage_ctx *wpc, struct folio *folio,
ioend_flags |= IOMAP_IOEND_DONTCACHE;
if (pos == wpc->iomap.offset && (wpc->iomap.flags & IOMAP_F_BOUNDARY))
ioend_flags |= IOMAP_IOEND_BOUNDARY;
+ if (is_atomic)
+ ioend_flags |= IOMAP_IOEND_ATOMIC;
if (!ioend || !iomap_can_add_to_ioend(wpc, pos, ioend_flags)) {
new_ioend:
@@ -188,7 +198,7 @@ ssize_t iomap_add_to_ioend(struct iomap_writepage_ctx *wpc, struct folio *folio,
if (error)
return error;
}
- wpc->wb_ctx = ioend = iomap_alloc_ioend(wpc, pos, ioend_flags);
+ wpc->wb_ctx = ioend = iomap_alloc_ioend(wpc, pos, ioend_flags, is_atomic);
}
if (!bio_add_folio(&ioend->io_bio, folio, map_len, poff))
diff --git a/fs/read_write.c b/fs/read_write.c
index 833bae068770..37546aa40f0d 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1802,6 +1802,8 @@ int generic_file_rw_checks(struct file *file_in, struct file *file_out)
int generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter)
{
+ struct super_block *sb = iocb->ki_filp->f_mapping->host->i_sb;
+
size_t len = iov_iter_count(iter);
if (!iter_is_ubuf(iter))
@@ -1813,8 +1815,36 @@ int generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter)
if (!IS_ALIGNED(iocb->ki_pos, len))
return -EINVAL;
- if (!(iocb->ki_flags & IOCB_DIRECT))
- return -EOPNOTSUPP;
+ if (!(iocb->ki_flags & IOCB_DIRECT)) {
+ /* Some restrictions to buferred IO */
+
+ /*
+ * We only support block size == page size
+ * right now. This is to avoid the following:
+ * 1. 4kb block atomic write marks the complete 64kb folio as
+ * atomic.
+ * 2. Other writes, dirty the whole 64kb folio.
+ * 3. Writeback sees the whole folio dirty and atomic and tries
+ * to send a 64kb atomic write, which might exceed the
+ * allowed size and fail.
+ *
+ * Once we support sub-page atomic write tracking, we can remove
+ * this restriction.
+ */
+ if (sb->s_blocksize != PAGE_SIZE)
+ return -EOPNOTSUPP;
+
+ /*
+ * If the user buffer of atomic write crosses page boundary,
+ * there's a possibility of short write, example if 1 user page
+ * could not be faulted or got reclaimed before the copy
+ * operation. For now don't allow such a scenario by ensuring
+ * user buffer is page aligned.
+ */
+ if (!PAGE_ALIGNED(iov_iter_alignment(iter)))
+ return -EOPNOTSUPP;
+
+ }
return 0;
}
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 8b1ac08c7474..693f3e5ad03c 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -390,6 +390,8 @@ sector_t iomap_bmap(struct address_space *mapping, sector_t bno,
#define IOMAP_IOEND_DIRECT (1U << 3)
/* is DONTCACHE I/O */
#define IOMAP_IOEND_DONTCACHE (1U << 4)
+/* is atomic I/O. These are never merged */
+#define IOMAP_IOEND_ATOMIC (1U << 5)
/*
* Flags that if set on either ioend prevent the merge of two ioends.
--
2.51.0
^ permalink raw reply related
* [RFC PATCH 3/8] fs: Add initial buffered atomic write support info to statx
From: Ojaswin Mujoo @ 2025-11-12 11:06 UTC (permalink / raw)
To: Christian Brauner, djwong, ritesh.list, john.g.garry, tytso,
willy, dchinner, hch
Cc: linux-xfs, linux-kernel, linux-ext4, linux-fsdevel, linux-mm,
jack, nilay, martin.petersen, rostedt, axboe, linux-block,
linux-trace-kernel
In-Reply-To: <cover.1762945505.git.ojaswin@linux.ibm.com>
Extend statx system call to return additional info for buffered atomic
write support for a file. Currently only direct IO is supported.
New flags STATX_WRITE_ATOMIC_BUF and STATX_ATTR_WRITE_ATOMIC_BUF are for
indicating whether the file knows and supports buffered atomic writes.
Structure statx members stx_atomic_write_unit_{min, max, segments_max}
will be reused for bufferd atomic writes. Flags STATX_WRITE_ATOMIC_DIO
and STATX_WRITE_ATOMIC_BUF are mutually exclusive. With both flags set,
statx will ignore the request and neither fields in statx.result_mask
will be set.
Also, make sure ext4 and xfs report atomic write unit min and max of 0
when the new flag is passed.
Co-developed-by: John Garry <john.g.garry@oracle.com>
Signed-off-by: John Garry <john.g.garry@oracle.com>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
---
block/bdev.c | 3 +-
fs/ext4/inode.c | 7 +-
fs/stat.c | 33 +++--
fs/xfs/xfs_file.c | 9 +-
fs/xfs/xfs_iops.c | 121 ++++++++++--------
fs/xfs/xfs_iops.h | 6 +-
include/linux/fs.h | 3 +-
include/trace/misc/fs.h | 1 +
include/uapi/linux/stat.h | 2 +
tools/include/uapi/linux/stat.h | 2 +
.../trace/beauty/include/uapi/linux/stat.h | 2 +
11 files changed, 119 insertions(+), 70 deletions(-)
diff --git a/block/bdev.c b/block/bdev.c
index 3bc90d5feb4c..8f0eab0a1ecf 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -1335,8 +1335,7 @@ void bdev_statx(const struct path *path, struct kstat *stat, u32 request_mask)
generic_fill_statx_atomic_writes(stat,
queue_atomic_write_unit_min_bytes(bd_queue),
- queue_atomic_write_unit_max_bytes(bd_queue),
- 0);
+ queue_atomic_write_unit_max_bytes(bd_queue), 0, true);
}
stat->blksize = bdev_io_min(bdev);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 9555149a8ba6..0d5013993fba 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -6106,8 +6106,11 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path,
awu_max = sbi->s_awu_max;
}
- generic_fill_statx_atomic_writes(stat, awu_min, awu_max, 0);
- }
+ generic_fill_statx_atomic_writes(stat, awu_min, awu_max, 0,
+ true);
+ } else if (request_mask & STATX_WRITE_ATOMIC_BUF)
+ /* Atomic writes for buferred IO not supported yet */
+ generic_fill_statx_atomic_writes(stat, 0, 0, 0, false);
flags = ei->i_flags & EXT4_FL_USER_VISIBLE;
if (flags & EXT4_APPEND_FL)
diff --git a/fs/stat.c b/fs/stat.c
index 7eb2a247ab67..8ba3993dcd09 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -137,20 +137,27 @@ EXPORT_SYMBOL(generic_fill_statx_attr);
* @unit_min: Minimum supported atomic write length in bytes
* @unit_max: Maximum supported atomic write length in bytes
* @unit_max_opt: Optimised maximum supported atomic write length in bytes
+ * @is_dio: Is the stat request for dio
*
- * Fill in the STATX{_ATTR}_WRITE_ATOMIC_DIO flags in the kstat structure from
- * atomic write unit_min and unit_max values.
+ * Fill in the STATX{_ATTR}_WRITE_ATOMIC_{DIO,BUF} flags in the kstat structure
+ * from atomic write unit_min and unit_max values.
*/
void generic_fill_statx_atomic_writes(struct kstat *stat,
unsigned int unit_min,
unsigned int unit_max,
- unsigned int unit_max_opt)
+ unsigned int unit_max_opt,
+ bool is_dio)
{
- /* Confirm that the request type is known */
- stat->result_mask |= STATX_WRITE_ATOMIC_DIO;
+ if (is_dio) {
+ /* Confirm that the request type is known */
+ stat->result_mask |= STATX_WRITE_ATOMIC_DIO;
- /* Confirm that the file attribute type is known */
- stat->attributes_mask |= STATX_ATTR_WRITE_ATOMIC_DIO;
+ /* Confirm that the file attribute type is known */
+ stat->attributes_mask |= STATX_ATTR_WRITE_ATOMIC_DIO;
+ } else {
+ stat->result_mask |= STATX_WRITE_ATOMIC_BUF;
+ stat->attributes_mask |= STATX_ATTR_WRITE_ATOMIC_BUF;
+ }
if (unit_min) {
stat->atomic_write_unit_min = unit_min;
@@ -160,7 +167,10 @@ void generic_fill_statx_atomic_writes(struct kstat *stat,
stat->atomic_write_segments_max = 1;
/* Confirm atomic writes are actually supported */
- stat->attributes |= STATX_ATTR_WRITE_ATOMIC_DIO;
+ if (is_dio)
+ stat->attributes |= STATX_ATTR_WRITE_ATOMIC_DIO;
+ else
+ stat->attributes |= STATX_ATTR_WRITE_ATOMIC_BUF;
}
}
EXPORT_SYMBOL_GPL(generic_fill_statx_atomic_writes);
@@ -206,6 +216,13 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
stat->attributes_mask |= (STATX_ATTR_AUTOMOUNT |
STATX_ATTR_DAX);
+ if (request_mask & STATX_WRITE_ATOMIC_BUF &&
+ request_mask & STATX_WRITE_ATOMIC_DIO) {
+ /* Both are mutually exclusive, disable them */
+ request_mask &=
+ ~(STATX_WRITE_ATOMIC_BUF | STATX_WRITE_ATOMIC_DIO);
+ }
+
idmap = mnt_idmap(path->mnt);
if (inode->i_op->getattr) {
int ret;
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 5b9864c8582e..3efa575570ed 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1087,6 +1087,7 @@ xfs_file_write_iter(
struct xfs_inode *ip = XFS_I(inode);
ssize_t ret;
size_t ocount = iov_iter_count(from);
+ bool is_dio = iocb->ki_flags & IOCB_DIRECT;
XFS_STATS_INC(ip->i_mount, xs_write_calls);
@@ -1097,10 +1098,10 @@ xfs_file_write_iter(
return -EIO;
if (iocb->ki_flags & IOCB_ATOMIC) {
- if (ocount < xfs_get_atomic_write_min(ip))
+ if (ocount < xfs_get_atomic_write_min(ip, is_dio))
return -EINVAL;
- if (ocount > xfs_get_atomic_write_max(ip))
+ if (ocount > xfs_get_atomic_write_max(ip, is_dio))
return -EINVAL;
ret = generic_atomic_write_valid(iocb, from);
@@ -1111,7 +1112,7 @@ xfs_file_write_iter(
if (IS_DAX(inode))
return xfs_file_dax_write(iocb, from);
- if (iocb->ki_flags & IOCB_DIRECT) {
+ if (is_dio) {
/*
* Allow a directio write to fall back to a buffered
* write *only* in the case that we're doing a reflink
@@ -1568,7 +1569,7 @@ xfs_file_open(
if (xfs_is_shutdown(XFS_M(inode->i_sb)))
return -EIO;
file->f_mode |= FMODE_NOWAIT | FMODE_CAN_ODIRECT;
- if (xfs_get_atomic_write_min(XFS_I(inode)) > 0)
+ if (xfs_get_atomic_write_min(XFS_I(inode), file->f_flags & O_DIRECT) > 0)
file->f_mode |= FMODE_CAN_ATOMIC_WRITE;
return generic_file_open(inode, file);
}
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index f41fcdd3043b..f036c46b19c5 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -601,81 +601,99 @@ xfs_report_dioalign(
unsigned int
xfs_get_atomic_write_min(
- struct xfs_inode *ip)
+ struct xfs_inode *ip,
+ bool is_dio)
{
- struct xfs_mount *mp = ip->i_mount;
+ if (is_dio) {
+ struct xfs_mount *mp = ip->i_mount;
- /*
- * If we can complete an atomic write via atomic out of place writes,
- * then advertise a minimum size of one fsblock. Without this
- * mechanism, we can only guarantee atomic writes up to a single LBA.
- *
- * If out of place writes are not available, we can guarantee an atomic
- * write of exactly one single fsblock if the bdev will make that
- * guarantee for us.
- */
- if (xfs_inode_can_hw_atomic_write(ip) ||
- xfs_inode_can_sw_atomic_write(ip))
- return mp->m_sb.sb_blocksize;
+ /*
+ * If we can complete an atomic write via atomic out of place writes,
+ * then advertise a minimum size of one fsblock. Without this
+ * mechanism, we can only guarantee atomic writes up to a single LBA.
+ *
+ * If out of place writes are not available, we can guarantee an atomic
+ * write of exactly one single fsblock if the bdev will make that
+ * guarantee for us.
+ */
+ if (xfs_inode_can_hw_atomic_write(ip) ||
+ xfs_inode_can_sw_atomic_write(ip))
+ return mp->m_sb.sb_blocksize;
+ }
+ /* buffered IO not supported yet so return 0 right away */
return 0;
}
unsigned int
xfs_get_atomic_write_max(
- struct xfs_inode *ip)
+ struct xfs_inode *ip,
+ bool is_dio)
{
struct xfs_mount *mp = ip->i_mount;
- /*
- * If out of place writes are not available, we can guarantee an atomic
- * write of exactly one single fsblock if the bdev will make that
- * guarantee for us.
- */
- if (!xfs_inode_can_sw_atomic_write(ip)) {
- if (xfs_inode_can_hw_atomic_write(ip))
- return mp->m_sb.sb_blocksize;
- return 0;
+ if (is_dio) {
+ /*
+ * If out of place writes are not available, we can guarantee an atomic
+ * write of exactly one single fsblock if the bdev will make that
+ * guarantee for us.
+ */
+ if (!xfs_inode_can_sw_atomic_write(ip)) {
+ if (xfs_inode_can_hw_atomic_write(ip))
+ return mp->m_sb.sb_blocksize;
+ return 0;
+ }
+
+ /*
+ * If we can complete an atomic write via atomic out of place writes,
+ * then advertise a maximum size of whatever we can complete through
+ * that means. Hardware support is reported via max_opt, not here.
+ */
+ if (XFS_IS_REALTIME_INODE(ip))
+ return XFS_FSB_TO_B(mp, mp->m_groups[XG_TYPE_RTG].awu_max);
+ return XFS_FSB_TO_B(mp, mp->m_groups[XG_TYPE_AG].awu_max);
}
- /*
- * If we can complete an atomic write via atomic out of place writes,
- * then advertise a maximum size of whatever we can complete through
- * that means. Hardware support is reported via max_opt, not here.
- */
- if (XFS_IS_REALTIME_INODE(ip))
- return XFS_FSB_TO_B(mp, mp->m_groups[XG_TYPE_RTG].awu_max);
- return XFS_FSB_TO_B(mp, mp->m_groups[XG_TYPE_AG].awu_max);
+ /* buffered IO not supported yet so return 0 right away */
+ return 0;
}
unsigned int
xfs_get_atomic_write_max_opt(
- struct xfs_inode *ip)
+ struct xfs_inode *ip,
+ bool is_dio)
{
- unsigned int awu_max = xfs_get_atomic_write_max(ip);
+ if (is_dio) {
+ unsigned int awu_max = xfs_get_atomic_write_max(ip, is_dio);
- /* if the max is 1x block, then just keep behaviour that opt is 0 */
- if (awu_max <= ip->i_mount->m_sb.sb_blocksize)
- return 0;
+ /* if the max is 1x block, then just keep behaviour that opt is 0 */
+ if (awu_max <= ip->i_mount->m_sb.sb_blocksize)
+ return 0;
- /*
- * Advertise the maximum size of an atomic write that we can tell the
- * block device to perform for us. In general the bdev limit will be
- * less than our out of place write limit, but we don't want to exceed
- * the awu_max.
- */
- return min(awu_max, xfs_inode_buftarg(ip)->bt_awu_max);
+ /*
+ * Advertise the maximum size of an atomic write that we can tell the
+ * block device to perform for us. In general the bdev limit will be
+ * less than our out of place write limit, but we don't want to exceed
+ * the awu_max.
+ */
+ return min(awu_max, xfs_inode_buftarg(ip)->bt_awu_max);
+ }
+
+ /* buffered IO not supported yet so return 0 right away */
+ return 0;
}
static void
xfs_report_atomic_write(
struct xfs_inode *ip,
- struct kstat *stat)
+ struct kstat *stat,
+ bool is_dio)
{
generic_fill_statx_atomic_writes(stat,
- xfs_get_atomic_write_min(ip),
- xfs_get_atomic_write_max(ip),
- xfs_get_atomic_write_max_opt(ip));
+ xfs_get_atomic_write_min(ip, is_dio),
+ xfs_get_atomic_write_max(ip, is_dio),
+ xfs_get_atomic_write_max_opt(ip, is_dio),
+ is_dio);
}
STATIC int
@@ -741,8 +759,11 @@ xfs_vn_getattr(
case S_IFREG:
if (request_mask & (STATX_DIOALIGN | STATX_DIO_READ_ALIGN))
xfs_report_dioalign(ip, stat);
- if (request_mask & STATX_WRITE_ATOMIC_DIO)
- xfs_report_atomic_write(ip, stat);
+ if (request_mask &
+ (STATX_WRITE_ATOMIC_DIO | STATX_WRITE_ATOMIC_BUF))
+ xfs_report_atomic_write(ip, stat,
+ (request_mask &
+ STATX_WRITE_ATOMIC_DIO));
fallthrough;
default:
stat->blksize = xfs_stat_blksize(ip);
diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h
index 0896f6b8b3b8..09e79263add1 100644
--- a/fs/xfs/xfs_iops.h
+++ b/fs/xfs/xfs_iops.h
@@ -19,8 +19,8 @@ int xfs_inode_init_security(struct inode *inode, struct inode *dir,
extern void xfs_setup_inode(struct xfs_inode *ip);
extern void xfs_setup_iops(struct xfs_inode *ip);
extern void xfs_diflags_to_iflags(struct xfs_inode *ip, bool init);
-unsigned int xfs_get_atomic_write_min(struct xfs_inode *ip);
-unsigned int xfs_get_atomic_write_max(struct xfs_inode *ip);
-unsigned int xfs_get_atomic_write_max_opt(struct xfs_inode *ip);
+unsigned int xfs_get_atomic_write_min(struct xfs_inode *ip, bool is_dio);
+unsigned int xfs_get_atomic_write_max(struct xfs_inode *ip, bool is_dio);
+unsigned int xfs_get_atomic_write_max_opt(struct xfs_inode *ip, bool is_dio);
#endif /* __XFS_IOPS_H__ */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c895146c1444..2dec66913e97 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3563,7 +3563,8 @@ void generic_fill_statx_attr(struct inode *inode, struct kstat *stat);
void generic_fill_statx_atomic_writes(struct kstat *stat,
unsigned int unit_min,
unsigned int unit_max,
- unsigned int unit_max_opt);
+ unsigned int unit_max_opt,
+ bool is_dio);
extern int vfs_getattr_nosec(const struct path *, struct kstat *, u32, unsigned int);
extern int vfs_getattr(const struct path *, struct kstat *, u32, unsigned int);
void __inode_add_bytes(struct inode *inode, loff_t bytes);
diff --git a/include/trace/misc/fs.h b/include/trace/misc/fs.h
index 19ea9339b9bd..3b69910a5998 100644
--- a/include/trace/misc/fs.h
+++ b/include/trace/misc/fs.h
@@ -162,4 +162,5 @@
{ STATX_MNT_ID_UNIQUE, "MNT_ID_UNIQUE" }, \
{ STATX_SUBVOL, "SUBVOL" }, \
{ STATX_WRITE_ATOMIC_DIO, "WRITE_ATOMIC_DIO" }, \
+ { STATX_WRITE_ATOMIC_BUF, "WRITE_ATOMIC_BUF" }, \
{ STATX_DIO_READ_ALIGN, "DIO_READ_ALIGN" })
diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index 57f558be933e..2d77da04df23 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -221,6 +221,7 @@ struct statx {
/* Old name kept for backward compatibility */
#define STATX_WRITE_ATOMIC STATX_WRITE_ATOMIC_DIO
#define STATX_DIO_READ_ALIGN 0x00020000U /* Want/got dio read alignment info */
+#define STATX_WRITE_ATOMIC_BUF 0x00040000U /* Want/got buf-io atomic_write_* fields */
#define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
@@ -259,6 +260,7 @@ struct statx {
#define STATX_ATTR_WRITE_ATOMIC_DIO 0x00400000 /* File supports dio atomic write operations */
/* Old name kept for backward compatibility */
#define STATX_ATTR_WRITE_ATOMIC STATX_ATTR_WRITE_ATOMIC_DIO
+#define STATX_ATTR_WRITE_ATOMIC_BUF 0x00800000 /* File supports buf-io atomic write operations */
#endif /* _UAPI_LINUX_STAT_H */
diff --git a/tools/include/uapi/linux/stat.h b/tools/include/uapi/linux/stat.h
index 57f558be933e..a7e0036669c2 100644
--- a/tools/include/uapi/linux/stat.h
+++ b/tools/include/uapi/linux/stat.h
@@ -221,6 +221,7 @@ struct statx {
/* Old name kept for backward compatibility */
#define STATX_WRITE_ATOMIC STATX_WRITE_ATOMIC_DIO
#define STATX_DIO_READ_ALIGN 0x00020000U /* Want/got dio read alignment info */
+#define STATX_WRITE_ATOMIC_BUF 0x00040000U /* Want/got buf-io atomic_write_* fields */
#define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
@@ -259,6 +260,7 @@ struct statx {
#define STATX_ATTR_WRITE_ATOMIC_DIO 0x00400000 /* File supports dio atomic write operations */
/* Old name kept for backward compatibility */
#define STATX_ATTR_WRITE_ATOMIC STATX_ATTR_WRITE_ATOMIC_DIO
+#define STATX_ATTR_WRITE_ATOMIC_BUF 0x00800000 /* File supports buf-io atomic write operations */
#endif /* _UAPI_LINUX_STAT_H */
diff --git a/tools/perf/trace/beauty/include/uapi/linux/stat.h b/tools/perf/trace/beauty/include/uapi/linux/stat.h
index 57f558be933e..2d77da04df23 100644
--- a/tools/perf/trace/beauty/include/uapi/linux/stat.h
+++ b/tools/perf/trace/beauty/include/uapi/linux/stat.h
@@ -221,6 +221,7 @@ struct statx {
/* Old name kept for backward compatibility */
#define STATX_WRITE_ATOMIC STATX_WRITE_ATOMIC_DIO
#define STATX_DIO_READ_ALIGN 0x00020000U /* Want/got dio read alignment info */
+#define STATX_WRITE_ATOMIC_BUF 0x00040000U /* Want/got buf-io atomic_write_* fields */
#define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
@@ -259,6 +260,7 @@ struct statx {
#define STATX_ATTR_WRITE_ATOMIC_DIO 0x00400000 /* File supports dio atomic write operations */
/* Old name kept for backward compatibility */
#define STATX_ATTR_WRITE_ATOMIC STATX_ATTR_WRITE_ATOMIC_DIO
+#define STATX_ATTR_WRITE_ATOMIC_BUF 0x00800000 /* File supports buf-io atomic write operations */
#endif /* _UAPI_LINUX_STAT_H */
--
2.51.0
^ permalink raw reply related
* [RFC PATCH 1/8] fs: Rename STATX{_ATTR}_WRITE_ATOMIC -> STATX{_ATTR}_WRITE_ATOMIC_DIO
From: Ojaswin Mujoo @ 2025-11-12 11:06 UTC (permalink / raw)
To: Christian Brauner, djwong, ritesh.list, john.g.garry, tytso,
willy, dchinner, hch
Cc: linux-xfs, linux-kernel, linux-ext4, linux-fsdevel, linux-mm,
jack, nilay, martin.petersen, rostedt, axboe, linux-block,
linux-trace-kernel
In-Reply-To: <cover.1762945505.git.ojaswin@linux.ibm.com>
From: John Garry <john.g.garry@oracle.com>
This is in preparation for adding atomic write support for buffered
IO. Since the limits reported by FS for atomic write buffered IO
could be different from direct IO, rename STATX_WRITE_ATOMIC ->
STATX_WRITE_ATOMIC_DIO and STATX_ATTR_WRITE_ATOMIC ->
STATX_ATTR_WRITE_ATOMIC_DIO, to make it clear that they are only
relevant to direct IO.
Later we will add a separate flag for reporting atomic write with
buffered IO
Co-developed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
Documentation/filesystems/ext4/atomic_writes.rst | 4 ++--
block/bdev.c | 4 ++--
fs/ext4/inode.c | 2 +-
fs/stat.c | 8 ++++----
fs/xfs/xfs_iops.c | 2 +-
include/trace/misc/fs.h | 2 +-
include/uapi/linux/stat.h | 8 ++++++--
tools/include/uapi/linux/stat.h | 8 ++++++--
tools/perf/trace/beauty/include/uapi/linux/stat.h | 8 ++++++--
9 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/Documentation/filesystems/ext4/atomic_writes.rst b/Documentation/filesystems/ext4/atomic_writes.rst
index ae8995740aa8..108c9e9cb977 100644
--- a/Documentation/filesystems/ext4/atomic_writes.rst
+++ b/Documentation/filesystems/ext4/atomic_writes.rst
@@ -189,7 +189,7 @@ The write must be aligned to the filesystem's block size and not exceed the
filesystem's maximum atomic write unit size.
See ``generic_atomic_write_valid()`` for more details.
-``statx()`` system call with ``STATX_WRITE_ATOMIC`` flag can provide following
+``statx()`` system call with ``STATX_WRITE_ATOMIC_DIO`` flag can provide following
details:
* ``stx_atomic_write_unit_min``: Minimum size of an atomic write request.
@@ -198,7 +198,7 @@ details:
separate memory buffers that can be gathered into a write operation
(e.g., the iovcnt parameter for IOV_ITER). Currently, this is always set to one.
-The STATX_ATTR_WRITE_ATOMIC flag in ``statx->attributes`` is set if atomic
+The STATX_ATTR_WRITE_ATOMIC_DIO flag in ``statx->attributes`` is set if atomic
writes are supported.
.. _atomic_write_bdev_support:
diff --git a/block/bdev.c b/block/bdev.c
index 810707cca970..3bc90d5feb4c 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -1308,7 +1308,7 @@ void sync_bdevs(bool wait)
}
/*
- * Handle STATX_{DIOALIGN, WRITE_ATOMIC} for block devices.
+ * Handle STATX_{DIOALIGN, WRITE_ATOMIC_DIO} for block devices.
*/
void bdev_statx(const struct path *path, struct kstat *stat, u32 request_mask)
{
@@ -1330,7 +1330,7 @@ void bdev_statx(const struct path *path, struct kstat *stat, u32 request_mask)
stat->result_mask |= STATX_DIOALIGN;
}
- if (request_mask & STATX_WRITE_ATOMIC && bdev_can_atomic_write(bdev)) {
+ if (request_mask & STATX_WRITE_ATOMIC_DIO && bdev_can_atomic_write(bdev)) {
struct request_queue *bd_queue = bdev->bd_queue;
generic_fill_statx_atomic_writes(stat,
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index f9e4ac87211e..9555149a8ba6 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -6097,7 +6097,7 @@ int ext4_getattr(struct mnt_idmap *idmap, const struct path *path,
}
}
- if ((request_mask & STATX_WRITE_ATOMIC) && S_ISREG(inode->i_mode)) {
+ if ((request_mask & STATX_WRITE_ATOMIC_DIO) && S_ISREG(inode->i_mode)) {
struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
unsigned int awu_min = 0, awu_max = 0;
diff --git a/fs/stat.c b/fs/stat.c
index 6c79661e1b96..7eb2a247ab67 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -138,7 +138,7 @@ EXPORT_SYMBOL(generic_fill_statx_attr);
* @unit_max: Maximum supported atomic write length in bytes
* @unit_max_opt: Optimised maximum supported atomic write length in bytes
*
- * Fill in the STATX{_ATTR}_WRITE_ATOMIC flags in the kstat structure from
+ * Fill in the STATX{_ATTR}_WRITE_ATOMIC_DIO flags in the kstat structure from
* atomic write unit_min and unit_max values.
*/
void generic_fill_statx_atomic_writes(struct kstat *stat,
@@ -147,10 +147,10 @@ void generic_fill_statx_atomic_writes(struct kstat *stat,
unsigned int unit_max_opt)
{
/* Confirm that the request type is known */
- stat->result_mask |= STATX_WRITE_ATOMIC;
+ stat->result_mask |= STATX_WRITE_ATOMIC_DIO;
/* Confirm that the file attribute type is known */
- stat->attributes_mask |= STATX_ATTR_WRITE_ATOMIC;
+ stat->attributes_mask |= STATX_ATTR_WRITE_ATOMIC_DIO;
if (unit_min) {
stat->atomic_write_unit_min = unit_min;
@@ -160,7 +160,7 @@ void generic_fill_statx_atomic_writes(struct kstat *stat,
stat->atomic_write_segments_max = 1;
/* Confirm atomic writes are actually supported */
- stat->attributes |= STATX_ATTR_WRITE_ATOMIC;
+ stat->attributes |= STATX_ATTR_WRITE_ATOMIC_DIO;
}
}
EXPORT_SYMBOL_GPL(generic_fill_statx_atomic_writes);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index caff0125faea..f41fcdd3043b 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -741,7 +741,7 @@ xfs_vn_getattr(
case S_IFREG:
if (request_mask & (STATX_DIOALIGN | STATX_DIO_READ_ALIGN))
xfs_report_dioalign(ip, stat);
- if (request_mask & STATX_WRITE_ATOMIC)
+ if (request_mask & STATX_WRITE_ATOMIC_DIO)
xfs_report_atomic_write(ip, stat);
fallthrough;
default:
diff --git a/include/trace/misc/fs.h b/include/trace/misc/fs.h
index 7ead1c61f0cb..19ea9339b9bd 100644
--- a/include/trace/misc/fs.h
+++ b/include/trace/misc/fs.h
@@ -161,5 +161,5 @@
{ STATX_DIOALIGN, "DIOALIGN" }, \
{ STATX_MNT_ID_UNIQUE, "MNT_ID_UNIQUE" }, \
{ STATX_SUBVOL, "SUBVOL" }, \
- { STATX_WRITE_ATOMIC, "WRITE_ATOMIC" }, \
+ { STATX_WRITE_ATOMIC_DIO, "WRITE_ATOMIC_DIO" }, \
{ STATX_DIO_READ_ALIGN, "DIO_READ_ALIGN" })
diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index 1686861aae20..57f558be933e 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -217,7 +217,9 @@ struct statx {
#define STATX_DIOALIGN 0x00002000U /* Want/got direct I/O alignment info */
#define STATX_MNT_ID_UNIQUE 0x00004000U /* Want/got extended stx_mount_id */
#define STATX_SUBVOL 0x00008000U /* Want/got stx_subvol */
-#define STATX_WRITE_ATOMIC 0x00010000U /* Want/got atomic_write_* fields */
+#define STATX_WRITE_ATOMIC_DIO 0x00010000U /* Want/got dio atomic_write_* fields */
+/* Old name kept for backward compatibility */
+#define STATX_WRITE_ATOMIC STATX_WRITE_ATOMIC_DIO
#define STATX_DIO_READ_ALIGN 0x00020000U /* Want/got dio read alignment info */
#define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
@@ -254,7 +256,9 @@ struct statx {
#define STATX_ATTR_MOUNT_ROOT 0x00002000 /* Root of a mount */
#define STATX_ATTR_VERITY 0x00100000 /* [I] Verity protected file */
#define STATX_ATTR_DAX 0x00200000 /* File is currently in DAX state */
-#define STATX_ATTR_WRITE_ATOMIC 0x00400000 /* File supports atomic write operations */
+#define STATX_ATTR_WRITE_ATOMIC_DIO 0x00400000 /* File supports dio atomic write operations */
+/* Old name kept for backward compatibility */
+#define STATX_ATTR_WRITE_ATOMIC STATX_ATTR_WRITE_ATOMIC_DIO
#endif /* _UAPI_LINUX_STAT_H */
diff --git a/tools/include/uapi/linux/stat.h b/tools/include/uapi/linux/stat.h
index 1686861aae20..57f558be933e 100644
--- a/tools/include/uapi/linux/stat.h
+++ b/tools/include/uapi/linux/stat.h
@@ -217,7 +217,9 @@ struct statx {
#define STATX_DIOALIGN 0x00002000U /* Want/got direct I/O alignment info */
#define STATX_MNT_ID_UNIQUE 0x00004000U /* Want/got extended stx_mount_id */
#define STATX_SUBVOL 0x00008000U /* Want/got stx_subvol */
-#define STATX_WRITE_ATOMIC 0x00010000U /* Want/got atomic_write_* fields */
+#define STATX_WRITE_ATOMIC_DIO 0x00010000U /* Want/got dio atomic_write_* fields */
+/* Old name kept for backward compatibility */
+#define STATX_WRITE_ATOMIC STATX_WRITE_ATOMIC_DIO
#define STATX_DIO_READ_ALIGN 0x00020000U /* Want/got dio read alignment info */
#define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
@@ -254,7 +256,9 @@ struct statx {
#define STATX_ATTR_MOUNT_ROOT 0x00002000 /* Root of a mount */
#define STATX_ATTR_VERITY 0x00100000 /* [I] Verity protected file */
#define STATX_ATTR_DAX 0x00200000 /* File is currently in DAX state */
-#define STATX_ATTR_WRITE_ATOMIC 0x00400000 /* File supports atomic write operations */
+#define STATX_ATTR_WRITE_ATOMIC_DIO 0x00400000 /* File supports dio atomic write operations */
+/* Old name kept for backward compatibility */
+#define STATX_ATTR_WRITE_ATOMIC STATX_ATTR_WRITE_ATOMIC_DIO
#endif /* _UAPI_LINUX_STAT_H */
diff --git a/tools/perf/trace/beauty/include/uapi/linux/stat.h b/tools/perf/trace/beauty/include/uapi/linux/stat.h
index 1686861aae20..57f558be933e 100644
--- a/tools/perf/trace/beauty/include/uapi/linux/stat.h
+++ b/tools/perf/trace/beauty/include/uapi/linux/stat.h
@@ -217,7 +217,9 @@ struct statx {
#define STATX_DIOALIGN 0x00002000U /* Want/got direct I/O alignment info */
#define STATX_MNT_ID_UNIQUE 0x00004000U /* Want/got extended stx_mount_id */
#define STATX_SUBVOL 0x00008000U /* Want/got stx_subvol */
-#define STATX_WRITE_ATOMIC 0x00010000U /* Want/got atomic_write_* fields */
+#define STATX_WRITE_ATOMIC_DIO 0x00010000U /* Want/got dio atomic_write_* fields */
+/* Old name kept for backward compatibility */
+#define STATX_WRITE_ATOMIC STATX_WRITE_ATOMIC_DIO
#define STATX_DIO_READ_ALIGN 0x00020000U /* Want/got dio read alignment info */
#define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
@@ -254,7 +256,9 @@ struct statx {
#define STATX_ATTR_MOUNT_ROOT 0x00002000 /* Root of a mount */
#define STATX_ATTR_VERITY 0x00100000 /* [I] Verity protected file */
#define STATX_ATTR_DAX 0x00200000 /* File is currently in DAX state */
-#define STATX_ATTR_WRITE_ATOMIC 0x00400000 /* File supports atomic write operations */
+#define STATX_ATTR_WRITE_ATOMIC_DIO 0x00400000 /* File supports dio atomic write operations */
+/* Old name kept for backward compatibility */
+#define STATX_ATTR_WRITE_ATOMIC STATX_ATTR_WRITE_ATOMIC_DIO
#endif /* _UAPI_LINUX_STAT_H */
--
2.51.0
^ permalink raw reply related
* [RFC PATCH 2/8] mm: Add PG_atomic
From: Ojaswin Mujoo @ 2025-11-12 11:06 UTC (permalink / raw)
To: Christian Brauner, djwong, ritesh.list, john.g.garry, tytso,
willy, dchinner, hch
Cc: linux-xfs, linux-kernel, linux-ext4, linux-fsdevel, linux-mm,
jack, nilay, martin.petersen, rostedt, axboe, linux-block,
linux-trace-kernel
In-Reply-To: <cover.1762945505.git.ojaswin@linux.ibm.com>
From: John Garry <john.g.garry@oracle.com>
Add page flag PG_atomic, meaning that a folio needs to be written back
atomically. This will be used by for handling RWF_ATOMIC buffered IO
in upcoming patches.
Co-developed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
include/linux/page-flags.h | 5 +++++
include/trace/events/mmflags.h | 3 ++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 0091ad1986bf..bdce0f58a77a 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -111,6 +111,7 @@ enum pageflags {
PG_swapbacked, /* Page is backed by RAM/swap */
PG_unevictable, /* Page is "unevictable" */
PG_dropbehind, /* drop pages on IO completion */
+ PG_atomic, /* Page is marked atomic for buffered atomic writes */
#ifdef CONFIG_MMU
PG_mlocked, /* Page is vma mlocked */
#endif
@@ -644,6 +645,10 @@ FOLIO_FLAG(unevictable, FOLIO_HEAD_PAGE)
__FOLIO_CLEAR_FLAG(unevictable, FOLIO_HEAD_PAGE)
FOLIO_TEST_CLEAR_FLAG(unevictable, FOLIO_HEAD_PAGE)
+FOLIO_FLAG(atomic, FOLIO_HEAD_PAGE)
+ __FOLIO_CLEAR_FLAG(atomic, FOLIO_HEAD_PAGE)
+ FOLIO_TEST_CLEAR_FLAG(atomic, FOLIO_HEAD_PAGE)
+
#ifdef CONFIG_MMU
FOLIO_FLAG(mlocked, FOLIO_HEAD_PAGE)
__FOLIO_CLEAR_FLAG(mlocked, FOLIO_HEAD_PAGE)
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index aa441f593e9a..a8294f6146a5 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -159,7 +159,8 @@ TRACE_DEFINE_ENUM(___GFP_LAST_BIT);
DEF_PAGEFLAG_NAME(reclaim), \
DEF_PAGEFLAG_NAME(swapbacked), \
DEF_PAGEFLAG_NAME(unevictable), \
- DEF_PAGEFLAG_NAME(dropbehind) \
+ DEF_PAGEFLAG_NAME(dropbehind), \
+ DEF_PAGEFLAG_NAME(atomic) \
IF_HAVE_PG_MLOCK(mlocked) \
IF_HAVE_PG_HWPOISON(hwpoison) \
IF_HAVE_PG_IDLE(idle) \
--
2.51.0
^ permalink raw reply related
* [RFC PATCH 0/8] xfs: single block atomic writes for buffered IO
From: Ojaswin Mujoo @ 2025-11-12 11:06 UTC (permalink / raw)
To: Christian Brauner, djwong, ritesh.list, john.g.garry, tytso,
willy, dchinner, hch
Cc: linux-xfs, linux-kernel, linux-ext4, linux-fsdevel, linux-mm,
jack, nilay, martin.petersen, rostedt, axboe, linux-block,
linux-trace-kernel
This patch adds support to perform single block RWF_ATOMIC writes for
iomap xfs buffered IO. This builds upon the inital RFC shared by John
Garry last year [1]. Most of the details are present in the respective
commit messages but I'd mention some of the design points below:
1. The first 4 patches introduce the statx and iomap plubming and page
flags to add basic atomic writes support to buffered IO. However, there
are still 2 key restrictions that apply:
FIRST: If the user buffer of atomic write crosses page boundary, there's a
possibility of short write, example if 1 user page could not be faulted or got
reclaimed before the copy operation. For now don't allow such a scenario by
ensuring user buffer is page aligned. This way either the full write goes
through or nothing does. This is also discussed in Mathew Wilcox's comment here
[2]
This is lifted in patch 5. The approach we took was to:
1. pin the user pages
2. Create a BVEC out of the struct page to pass to
copy_folio_from_iter_atomic() rather than the USER backed iter. We
don't use the user iter directly because the pinned user page could
still get unmapped from the process, leading to short writes.
This approach allows us to only proceed if we are sure we will not have a short
copy.
SECOND: We only support block size == page size buf-io atomic writes.
This is to avoid the following scenario:
1. 4kb block atomic write marks the complete 64kb folio as
atomic.
2. Other writes, dirty the whole 64kb folio.
3. Writeback sees the whole folio dirty and atomic and tries
to send a 64kb atomic write, which might exceed the
allowed atomic write size and fail.
Patch 7 adds support for sub-page atomic write tracking to remove this
restriction. We do this by adding 2 more bitmaps to ifs to track atomic
write start and end.
Lastly, a non atomic write over an atomic write will remove the atomic
guarantee. Userspace is expected to make sure to sync the data to disk
after an atomic write before performing any overwrites.
This series has survived -g quick xfstests and I'll be continuing to
test it. Just wanted to put out the RFC to get some reviews on the
design and suggestions on any better approaches.
[1] https://lore.kernel.org/all/20240422143923.3927601-1-john.g.garry@oracle.com/
[2] https://lore.kernel.org/all/ZiZ8XGZz46D3PRKr@casper.infradead.org/
Thanks,
Ojaswin
John Garry (2):
fs: Rename STATX{_ATTR}_WRITE_ATOMIC -> STATX{_ATTR}_WRITE_ATOMIC_DIO
mm: Add PG_atomic
Ojaswin Mujoo (6):
fs: Add initial buffered atomic write support info to statx
iomap: buffered atomic write support
iomap: pin pages for RWF_ATOMIC buffered write
xfs: Report atomic write min and max for buf io as well
iomap: Add bs<ps buffered atomic writes support
xfs: Lift the bs == ps restriction for HW buffered atomic writes
.../filesystems/ext4/atomic_writes.rst | 4 +-
block/bdev.c | 7 +-
fs/ext4/inode.c | 9 +-
fs/iomap/buffered-io.c | 395 ++++++++++++++++--
fs/iomap/ioend.c | 21 +-
fs/iomap/trace.h | 12 +-
fs/read_write.c | 3 -
fs/stat.c | 33 +-
fs/xfs/xfs_file.c | 9 +-
fs/xfs/xfs_iops.c | 127 +++---
fs/xfs/xfs_iops.h | 6 +-
include/linux/fs.h | 3 +-
include/linux/iomap.h | 3 +
include/linux/page-flags.h | 5 +
include/trace/events/mmflags.h | 3 +-
include/trace/misc/fs.h | 3 +-
include/uapi/linux/stat.h | 10 +-
tools/include/uapi/linux/stat.h | 10 +-
.../trace/beauty/include/uapi/linux/stat.h | 10 +-
19 files changed, 551 insertions(+), 122 deletions(-)
--
2.51.0
^ permalink raw reply
* Re: [PATCH v16 4/4] perf tools: Merge deferred user callchains
From: Jens Remus @ 2025-11-12 10:05 UTC (permalink / raw)
To: Namhyung Kim, Steven Rostedt
Cc: Steven Rostedt, linux-kernel, linux-trace-kernel, bpf, x86,
Masami Hiramatsu, Mathieu Desnoyers, Josh Poimboeuf, Ingo Molnar,
Jiri Olsa, Thomas Gleixner, Andrii Nakryiko, Indu Bhagat,
Jose E. Marchesi, Beau Belgrave, Linus Torvalds, Andrew Morton,
Florian Weimer, Sam James, Kees Cook, Carlos O'Donell,
Arnaldo Carvalho de Melo, Peter Zijlstra
In-Reply-To: <20251024130203.GC3245006@noisy.programming.kicks-ass.net>
Hello Namhyung,
could you please adapt your patches from this series to Peter's latest
changes to unwind user and related perf support, especially his new
version c69993ecdd4d ("perf: Support deferred user unwind") available
at:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git perf/core
On 10/24/2025 3:02 PM, Peter Zijlstra wrote:
> On Thu, Oct 02, 2025 at 01:49:38PM -0400, Steven Rostedt wrote:
>> On Mon, 08 Sep 2025 13:53:23 -0400
>> Steven Rostedt <rostedt@kernel.org> wrote:
>>
>>> +static int evlist__deliver_deferred_samples(struct evlist *evlist,
>>> + const struct perf_tool *tool,
>>> + union perf_event *event,
>>> + struct perf_sample *sample,
>>> + struct machine *machine)
>>> +{
>>> + struct deferred_event *de, *tmp;
>>> + struct evsel *evsel;
>>> + int ret = 0;
>>> +
>>> + if (!tool->merge_deferred_callchains) {
>>> + evsel = evlist__id2evsel(evlist, sample->id);
>>> + return tool->callchain_deferred(tool, event, sample,
>>> + evsel, machine);
>>> + }
>>> +
>>> + list_for_each_entry_safe(de, tmp, &evlist->deferred_samples, list) {
>>> + struct perf_sample orig_sample;
>>
>> orig_sample is not initialized and can then contain junk.
>>
>>> +
>>> + ret = evlist__parse_sample(evlist, de->event, &orig_sample);
>>> + if (ret < 0) {
>>> + pr_err("failed to parse original sample\n");
>>> + break;
>>> + }
>>> +
>>> + if (sample->tid != orig_sample.tid)
>>> + continue;
>>> +
>>> + if (event->callchain_deferred.cookie == orig_sample.deferred_cookie)
>>> + sample__merge_deferred_callchain(&orig_sample, sample);
>>
>> The sample__merge_deferred_callchain() initializes both
>> orig_sample.deferred_callchain and the callchain. But now that it's not
>> being called, it can cause the below free to happen with junk as the
>> callchain. This needs:
>>
>> else
>> orig_sample.deferred_callchain = false;
>
> Ah, so I saw crashes from here and just deleted both free()s and got on
> with things ;-)
This needs to be properly resolved. In the meantime I am using Steven's
suggestion above to continue my work on unwind user sframe (s390).
>
>>> +
>>> + evsel = evlist__id2evsel(evlist, orig_sample.id);
>>> + ret = evlist__deliver_sample(evlist, tool, de->event,
>>> + &orig_sample, evsel,> machine); +
>>> + if (orig_sample.deferred_callchain)
>>> + free(orig_sample.callchain);
>>> +
>>> + list_del(&de->list);
>>> + free(de);
>>> +
>>> + if (ret)
>>> + break;
>>> + }
>>> + return ret;
>>> +}
>>
>> -- Steve
Thanks and regards,
Jens
--
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com
IBM
IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
^ permalink raw reply
* [PATCH v4 RESEND] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-12 9:36 UTC (permalink / raw)
To: rostedt
Cc: linux-trace-kernel, linux-kernel, Donglin Peng, Sven Schnelle,
Masami Hiramatsu
From: Donglin Peng <pengdonglin@xiaomi.com>
Currently, the funcgraph-args and funcgraph-retaddr features are
mutually exclusive. This patch resolves this limitation by modifying
funcgraph-retaddr to adopt the same implementation approach as
funcgraph-args, specifically by storing the return address in the
first entry of the args array.
As a result, both features now coexist seamlessly and function as
intended.
To verify the change, use perf to trace vfs_write with both options
enabled:
Before:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
down_read() { /* <-n_tty_write+0xa3/0x540 */
__cond_resched(); /* <-down_read+0x12/0x160 */
preempt_count_add(); /* <-down_read+0x3b/0x160 */
preempt_count_sub(); /* <-down_read+0x8b/0x160 */
}
After:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
__cond_resched(); /* <-down_read+0x12/0x160 */
preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
}
Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
v4:
- Remove redundant TRACE_GRAPH_ARGS flag check
- Eliminate unnecessary retaddr initialization
- Resend to add missing add Acked-by tags
v3:
- Replace min() with min_t() to improve type safety and maintainability
- Keep only one Signed-off-by for cleaner attribution
- Code refactoring for improved readability
v2:
- Preserve retaddr event functionality (suggested by Steven)
- Store the retaddr in args[0] (suggested by Steven)
- Refactor implementation logic and commit message clarity
---
include/linux/ftrace.h | 11 ---
kernel/trace/trace.h | 4 -
kernel/trace/trace_entries.h | 6 +-
kernel/trace/trace_functions_graph.c | 141 ++++++++++++---------------
4 files changed, 65 insertions(+), 97 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 7ded7df6e9b5..88cb54d73bdb 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
int depth;
} __packed;
-/*
- * Structure that defines an entry function trace with retaddr.
- * It's already packed but the attribute "packed" is needed
- * to remove extra padding at the end.
- */
-struct fgraph_retaddr_ent {
- unsigned long func; /* Current function */
- int depth;
- unsigned long retaddr; /* Return address */
-} __packed;
-
/*
* Structure that defines a return function trace.
* It's already packed but the attribute "packed" is needed
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 85eabb454bee..9fac291b913a 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -955,10 +955,6 @@ extern void graph_trace_close(struct trace_iterator *iter);
extern int __trace_graph_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned int trace_ctx);
-extern int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr);
extern void __trace_graph_return(struct trace_array *tr,
struct ftrace_graph_ret *trace,
unsigned int trace_ctx,
diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index de294ae2c5c5..593a74663c65 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
TRACE_GRAPH_RETADDR_ENT,
F_STRUCT(
- __field_struct( struct fgraph_retaddr_ent, graph_ent )
+ __field_struct( struct ftrace_graph_ent, graph_ent )
__field_packed( unsigned long, graph_ent, func )
__field_packed( unsigned int, graph_ent, depth )
- __field_packed( unsigned long, graph_ent, retaddr )
+ __dynamic_array(unsigned long, args )
),
F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
- (void *)__entry->retaddr)
+ (void *)__entry->args[0])
);
#else
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index a7f4b9a47a71..8c018886d4d2 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -16,6 +16,15 @@
#include "trace.h"
#include "trace_output.h"
+#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
+#define HAVE_RETADDR 1
+#define ARGS_OFFS(args_size) \
+ ((args_size) > FTRACE_REGS_MAX_ARGS * sizeof(long) ? 1 : 0)
+#else
+#define HAVE_RETADDR 0
+#define ARGS_OFFS(args_size) 0
+#endif
+
/* When set, irq functions will be ignored */
static int ftrace_graph_skip_irqs;
@@ -27,21 +36,25 @@ struct fgraph_cpu_data {
unsigned long enter_funcs[FTRACE_RETFUNC_DEPTH];
};
+/*
+ * fgraph_retaddr_ent_entry and ftrace_graph_ent_entry share layout, ent
+ * member repurposed for storage
+ */
struct fgraph_ent_args {
struct ftrace_graph_ent_entry ent;
- /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
- unsigned long args[FTRACE_REGS_MAX_ARGS];
+ /*
+ * Force the sizeof of args[] to have (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR)
+ * entries with the first entry storing the return address for
+ * TRACE_GRAPH_RETADDR_ENT.
+ */
+ unsigned long args[FTRACE_REGS_MAX_ARGS + HAVE_RETADDR];
};
struct fgraph_data {
struct fgraph_cpu_data __percpu *cpu_data;
/* Place to preserve last processed entry. */
- union {
- struct fgraph_ent_args ent;
- /* TODO allow retaddr to have args */
- struct fgraph_retaddr_ent_entry rent;
- };
+ struct fgraph_ent_args ent;
struct ftrace_graph_ret_entry ret;
int failed;
int cpu;
@@ -127,22 +140,39 @@ static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
struct ring_buffer_event *event;
struct trace_buffer *buffer = tr->array_buffer.buffer;
struct ftrace_graph_ent_entry *entry;
- int size;
+ unsigned long retaddr;
+ int size = sizeof(*entry);
+ int type = TRACE_GRAPH_ENT;
+ int nr_args = 0, args_offs = 0;
+
+ if (tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
+ retaddr = ftrace_graph_top_ret_addr(current);
+ type = TRACE_GRAPH_RETADDR_ENT;
+ nr_args += 1;
+ }
/* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
- size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
+ if (!!fregs)
+ nr_args += FTRACE_REGS_MAX_ARGS;
- event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size, trace_ctx);
+ size += nr_args * sizeof(long);
+ event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
if (!event)
return 0;
entry = ring_buffer_event_data(event);
entry->graph_ent = *trace;
+ /* Store the retaddr in args[0] */
+ if (type == TRACE_GRAPH_RETADDR_ENT) {
+ entry->args[0] = retaddr;
+ args_offs += 1;
+ }
+
#ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
- if (fregs) {
+ if (nr_args >= FTRACE_REGS_MAX_ARGS) {
for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
- entry->args[i] = ftrace_regs_get_argument(fregs, i);
+ entry->args[i + args_offs] = ftrace_regs_get_argument(fregs, i);
}
#endif
@@ -158,38 +188,6 @@ int __trace_graph_entry(struct trace_array *tr,
return __graph_entry(tr, trace, trace_ctx, NULL);
}
-#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
-int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr)
-{
- struct ring_buffer_event *event;
- struct trace_buffer *buffer = tr->array_buffer.buffer;
- struct fgraph_retaddr_ent_entry *entry;
-
- event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
- sizeof(*entry), trace_ctx);
- if (!event)
- return 0;
- entry = ring_buffer_event_data(event);
- entry->graph_ent.func = trace->func;
- entry->graph_ent.depth = trace->depth;
- entry->graph_ent.retaddr = retaddr;
- trace_buffer_unlock_commit_nostack(buffer, event);
-
- return 1;
-}
-#else
-int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr)
-{
- return 1;
-}
-#endif
-
static inline int ftrace_graph_ignore_irqs(void)
{
if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
@@ -211,7 +209,6 @@ static int graph_entry(struct ftrace_graph_ent *trace,
struct trace_array *tr = gops->private;
struct fgraph_times *ftimes;
unsigned int trace_ctx;
- int ret = 0;
if (*task_var & TRACE_GRAPH_NOTRACE)
return 0;
@@ -262,15 +259,7 @@ static int graph_entry(struct ftrace_graph_ent *trace,
return 1;
trace_ctx = tracing_gen_ctx();
- if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
- tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
- unsigned long retaddr = ftrace_graph_top_ret_addr(current);
- ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
- } else {
- ret = __graph_entry(tr, trace, trace_ctx, fregs);
- }
-
- return ret;
+ return __graph_entry(tr, trace, trace_ctx, fregs);
}
int trace_graph_entry(struct ftrace_graph_ent *trace,
@@ -634,13 +623,9 @@ get_return_for_leaf(struct trace_iterator *iter,
* Save current and next entries for later reference
* if the output fails.
*/
- if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
- data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
- } else {
- int size = min((int)sizeof(data->ent), (int)iter->ent_size);
+ int size = min_t(int, sizeof(data->ent), iter->ent_size);
- memcpy(&data->ent, curr, size);
- }
+ memcpy(&data->ent, curr, size);
/*
* If the next event is not a return type, then
* we only care about what type it is. Otherwise we can
@@ -811,21 +796,21 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
#define __TRACE_GRAPH_PRINT_RETADDR TRACE_GRAPH_PRINT_RETADDR
-static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_entry *entry,
- u32 trace_flags, bool comment)
+static void print_graph_retaddr(struct trace_seq *s, unsigned long retaddr, u32 trace_flags,
+ bool comment)
{
if (comment)
trace_seq_puts(s, " /*");
trace_seq_puts(s, " <-");
- seq_print_ip_sym(s, entry->graph_ent.retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
+ seq_print_ip_sym(s, retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
if (comment)
trace_seq_puts(s, " */");
}
#else
#define __TRACE_GRAPH_PRINT_RETADDR 0
-#define print_graph_retaddr(_seq, _entry, _tflags, _comment) do { } while (0)
+#define print_graph_retaddr(_seq, _retaddr, _tflags, _comment) do { } while (0)
#endif
#if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
@@ -869,10 +854,12 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
trace_seq_printf(s, "%ps", func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, (unsigned long)func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size),
+ (unsigned long)func);
trace_seq_putc(s, ';');
- } else
+ } else {
trace_seq_puts(s, "();");
+ }
if (print_retval || print_retaddr)
trace_seq_puts(s, " /*");
@@ -882,8 +869,7 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
}
if (print_retaddr)
- print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
- trace_flags, false);
+ print_graph_retaddr(s, entry->args[0], trace_flags, false);
if (print_retval) {
if (hex_format || (err_code == 0))
@@ -964,10 +950,12 @@ print_graph_entry_leaf(struct trace_iterator *iter,
trace_seq_printf(s, "%ps", (void *)ret_func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, ret_func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size),
+ ret_func);
trace_seq_putc(s, ';');
- } else
+ } else {
trace_seq_puts(s, "();");
+ }
}
trace_seq_putc(s, '\n');
@@ -1016,7 +1004,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
- print_function_args(s, entry->args, func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size), func);
else
trace_seq_puts(s, "()");
@@ -1024,8 +1012,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
if (flags & __TRACE_GRAPH_PRINT_RETADDR &&
entry->ent.type == TRACE_GRAPH_RETADDR_ENT)
- print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
- tr->trace_flags, true);
+ print_graph_retaddr(s, entry->args[0], tr->trace_flags, true);
trace_seq_putc(s, '\n');
if (trace_seq_has_overflowed(s))
@@ -1202,7 +1189,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
* it can be safely saved at the stack.
*/
struct ftrace_graph_ent_entry *entry;
- u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
+ u8 save_buf[sizeof(*entry) + (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR) * sizeof(long)];
/* The ent_size is expected to be as big as the entry */
if (iter->ent_size > sizeof(save_buf))
@@ -1429,16 +1416,12 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
trace_assign_type(field, entry);
return print_graph_entry(field, s, iter, flags);
}
-#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
case TRACE_GRAPH_RETADDR_ENT: {
- struct fgraph_retaddr_ent_entry saved;
struct fgraph_retaddr_ent_entry *rfield;
trace_assign_type(rfield, entry);
- saved = *rfield;
- return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
+ return print_graph_entry((typeof(field))rfield, s, iter, flags);
}
-#endif
case TRACE_GRAPH_RET: {
struct ftrace_graph_ret_entry *field;
trace_assign_type(field, entry);
--
2.34.1
^ permalink raw reply related
* [PATCH v4] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-12 9:27 UTC (permalink / raw)
To: rostedt
Cc: linux-trace-kernel, linux-kernel, Donglin Peng, Sven Schnelle,
Masami Hiramatsu
From: Donglin Peng <pengdonglin@xiaomi.com>
Currently, the funcgraph-args and funcgraph-retaddr features are
mutually exclusive. This patch resolves this limitation by modifying
funcgraph-retaddr to adopt the same implementation approach as
funcgraph-args, specifically by storing the return address in the
first entry of the args array.
As a result, both features now coexist seamlessly and function as
intended.
To verify the change, use perf to trace vfs_write with both options
enabled:
Before:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
down_read() { /* <-n_tty_write+0xa3/0x540 */
__cond_resched(); /* <-down_read+0x12/0x160 */
preempt_count_add(); /* <-down_read+0x3b/0x160 */
preempt_count_sub(); /* <-down_read+0x8b/0x160 */
}
After:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
__cond_resched(); /* <-down_read+0x12/0x160 */
preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
}
Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
---
v4:
- Remove redundant TRACE_GRAPH_ARGS flag check
- Eliminate unnecessary retaddr initialization
v3:
- Replace min() with min_t() to improve type safety and maintainability
- Keep only one Signed-off-by for cleaner attribution
- Code refactoring for improved readability
v2:
- Preserve retaddr event functionality (suggested by Steven)
- Store the retaddr in args[0] (suggested by Steven)
- Refactor implementation logic and commit message clarity
---
include/linux/ftrace.h | 11 ---
kernel/trace/trace.h | 4 -
kernel/trace/trace_entries.h | 6 +-
kernel/trace/trace_functions_graph.c | 141 ++++++++++++---------------
4 files changed, 65 insertions(+), 97 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 7ded7df6e9b5..88cb54d73bdb 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
int depth;
} __packed;
-/*
- * Structure that defines an entry function trace with retaddr.
- * It's already packed but the attribute "packed" is needed
- * to remove extra padding at the end.
- */
-struct fgraph_retaddr_ent {
- unsigned long func; /* Current function */
- int depth;
- unsigned long retaddr; /* Return address */
-} __packed;
-
/*
* Structure that defines a return function trace.
* It's already packed but the attribute "packed" is needed
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 85eabb454bee..9fac291b913a 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -955,10 +955,6 @@ extern void graph_trace_close(struct trace_iterator *iter);
extern int __trace_graph_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned int trace_ctx);
-extern int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr);
extern void __trace_graph_return(struct trace_array *tr,
struct ftrace_graph_ret *trace,
unsigned int trace_ctx,
diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index de294ae2c5c5..593a74663c65 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
TRACE_GRAPH_RETADDR_ENT,
F_STRUCT(
- __field_struct( struct fgraph_retaddr_ent, graph_ent )
+ __field_struct( struct ftrace_graph_ent, graph_ent )
__field_packed( unsigned long, graph_ent, func )
__field_packed( unsigned int, graph_ent, depth )
- __field_packed( unsigned long, graph_ent, retaddr )
+ __dynamic_array(unsigned long, args )
),
F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
- (void *)__entry->retaddr)
+ (void *)__entry->args[0])
);
#else
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index a7f4b9a47a71..8c018886d4d2 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -16,6 +16,15 @@
#include "trace.h"
#include "trace_output.h"
+#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
+#define HAVE_RETADDR 1
+#define ARGS_OFFS(args_size) \
+ ((args_size) > FTRACE_REGS_MAX_ARGS * sizeof(long) ? 1 : 0)
+#else
+#define HAVE_RETADDR 0
+#define ARGS_OFFS(args_size) 0
+#endif
+
/* When set, irq functions will be ignored */
static int ftrace_graph_skip_irqs;
@@ -27,21 +36,25 @@ struct fgraph_cpu_data {
unsigned long enter_funcs[FTRACE_RETFUNC_DEPTH];
};
+/*
+ * fgraph_retaddr_ent_entry and ftrace_graph_ent_entry share layout, ent
+ * member repurposed for storage
+ */
struct fgraph_ent_args {
struct ftrace_graph_ent_entry ent;
- /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
- unsigned long args[FTRACE_REGS_MAX_ARGS];
+ /*
+ * Force the sizeof of args[] to have (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR)
+ * entries with the first entry storing the return address for
+ * TRACE_GRAPH_RETADDR_ENT.
+ */
+ unsigned long args[FTRACE_REGS_MAX_ARGS + HAVE_RETADDR];
};
struct fgraph_data {
struct fgraph_cpu_data __percpu *cpu_data;
/* Place to preserve last processed entry. */
- union {
- struct fgraph_ent_args ent;
- /* TODO allow retaddr to have args */
- struct fgraph_retaddr_ent_entry rent;
- };
+ struct fgraph_ent_args ent;
struct ftrace_graph_ret_entry ret;
int failed;
int cpu;
@@ -127,22 +140,39 @@ static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
struct ring_buffer_event *event;
struct trace_buffer *buffer = tr->array_buffer.buffer;
struct ftrace_graph_ent_entry *entry;
- int size;
+ unsigned long retaddr;
+ int size = sizeof(*entry);
+ int type = TRACE_GRAPH_ENT;
+ int nr_args = 0, args_offs = 0;
+
+ if (tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
+ retaddr = ftrace_graph_top_ret_addr(current);
+ type = TRACE_GRAPH_RETADDR_ENT;
+ nr_args += 1;
+ }
/* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
- size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
+ if (!!fregs)
+ nr_args += FTRACE_REGS_MAX_ARGS;
- event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size, trace_ctx);
+ size += nr_args * sizeof(long);
+ event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
if (!event)
return 0;
entry = ring_buffer_event_data(event);
entry->graph_ent = *trace;
+ /* Store the retaddr in args[0] */
+ if (type == TRACE_GRAPH_RETADDR_ENT) {
+ entry->args[0] = retaddr;
+ args_offs += 1;
+ }
+
#ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
- if (fregs) {
+ if (nr_args >= FTRACE_REGS_MAX_ARGS) {
for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
- entry->args[i] = ftrace_regs_get_argument(fregs, i);
+ entry->args[i + args_offs] = ftrace_regs_get_argument(fregs, i);
}
#endif
@@ -158,38 +188,6 @@ int __trace_graph_entry(struct trace_array *tr,
return __graph_entry(tr, trace, trace_ctx, NULL);
}
-#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
-int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr)
-{
- struct ring_buffer_event *event;
- struct trace_buffer *buffer = tr->array_buffer.buffer;
- struct fgraph_retaddr_ent_entry *entry;
-
- event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
- sizeof(*entry), trace_ctx);
- if (!event)
- return 0;
- entry = ring_buffer_event_data(event);
- entry->graph_ent.func = trace->func;
- entry->graph_ent.depth = trace->depth;
- entry->graph_ent.retaddr = retaddr;
- trace_buffer_unlock_commit_nostack(buffer, event);
-
- return 1;
-}
-#else
-int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr)
-{
- return 1;
-}
-#endif
-
static inline int ftrace_graph_ignore_irqs(void)
{
if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
@@ -211,7 +209,6 @@ static int graph_entry(struct ftrace_graph_ent *trace,
struct trace_array *tr = gops->private;
struct fgraph_times *ftimes;
unsigned int trace_ctx;
- int ret = 0;
if (*task_var & TRACE_GRAPH_NOTRACE)
return 0;
@@ -262,15 +259,7 @@ static int graph_entry(struct ftrace_graph_ent *trace,
return 1;
trace_ctx = tracing_gen_ctx();
- if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
- tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
- unsigned long retaddr = ftrace_graph_top_ret_addr(current);
- ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
- } else {
- ret = __graph_entry(tr, trace, trace_ctx, fregs);
- }
-
- return ret;
+ return __graph_entry(tr, trace, trace_ctx, fregs);
}
int trace_graph_entry(struct ftrace_graph_ent *trace,
@@ -634,13 +623,9 @@ get_return_for_leaf(struct trace_iterator *iter,
* Save current and next entries for later reference
* if the output fails.
*/
- if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
- data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
- } else {
- int size = min((int)sizeof(data->ent), (int)iter->ent_size);
+ int size = min_t(int, sizeof(data->ent), iter->ent_size);
- memcpy(&data->ent, curr, size);
- }
+ memcpy(&data->ent, curr, size);
/*
* If the next event is not a return type, then
* we only care about what type it is. Otherwise we can
@@ -811,21 +796,21 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
#define __TRACE_GRAPH_PRINT_RETADDR TRACE_GRAPH_PRINT_RETADDR
-static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_entry *entry,
- u32 trace_flags, bool comment)
+static void print_graph_retaddr(struct trace_seq *s, unsigned long retaddr, u32 trace_flags,
+ bool comment)
{
if (comment)
trace_seq_puts(s, " /*");
trace_seq_puts(s, " <-");
- seq_print_ip_sym(s, entry->graph_ent.retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
+ seq_print_ip_sym(s, retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
if (comment)
trace_seq_puts(s, " */");
}
#else
#define __TRACE_GRAPH_PRINT_RETADDR 0
-#define print_graph_retaddr(_seq, _entry, _tflags, _comment) do { } while (0)
+#define print_graph_retaddr(_seq, _retaddr, _tflags, _comment) do { } while (0)
#endif
#if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
@@ -869,10 +854,12 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
trace_seq_printf(s, "%ps", func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, (unsigned long)func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size),
+ (unsigned long)func);
trace_seq_putc(s, ';');
- } else
+ } else {
trace_seq_puts(s, "();");
+ }
if (print_retval || print_retaddr)
trace_seq_puts(s, " /*");
@@ -882,8 +869,7 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
}
if (print_retaddr)
- print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
- trace_flags, false);
+ print_graph_retaddr(s, entry->args[0], trace_flags, false);
if (print_retval) {
if (hex_format || (err_code == 0))
@@ -964,10 +950,12 @@ print_graph_entry_leaf(struct trace_iterator *iter,
trace_seq_printf(s, "%ps", (void *)ret_func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, ret_func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size),
+ ret_func);
trace_seq_putc(s, ';');
- } else
+ } else {
trace_seq_puts(s, "();");
+ }
}
trace_seq_putc(s, '\n');
@@ -1016,7 +1004,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
- print_function_args(s, entry->args, func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size), func);
else
trace_seq_puts(s, "()");
@@ -1024,8 +1012,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
if (flags & __TRACE_GRAPH_PRINT_RETADDR &&
entry->ent.type == TRACE_GRAPH_RETADDR_ENT)
- print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
- tr->trace_flags, true);
+ print_graph_retaddr(s, entry->args[0], tr->trace_flags, true);
trace_seq_putc(s, '\n');
if (trace_seq_has_overflowed(s))
@@ -1202,7 +1189,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
* it can be safely saved at the stack.
*/
struct ftrace_graph_ent_entry *entry;
- u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
+ u8 save_buf[sizeof(*entry) + (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR) * sizeof(long)];
/* The ent_size is expected to be as big as the entry */
if (iter->ent_size > sizeof(save_buf))
@@ -1429,16 +1416,12 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
trace_assign_type(field, entry);
return print_graph_entry(field, s, iter, flags);
}
-#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
case TRACE_GRAPH_RETADDR_ENT: {
- struct fgraph_retaddr_ent_entry saved;
struct fgraph_retaddr_ent_entry *rfield;
trace_assign_type(rfield, entry);
- saved = *rfield;
- return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
+ return print_graph_entry((typeof(field))rfield, s, iter, flags);
}
-#endif
case TRACE_GRAPH_RET: {
struct ftrace_graph_ret_entry *field;
trace_assign_type(field, entry);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v3 RESEND] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-12 9:00 UTC (permalink / raw)
To: rostedt, Masami Hiramatsu
Cc: linux-trace-kernel, linux-kernel, Donglin Peng, Sven Schnelle
In-Reply-To: <20251112034333.2901601-1-dolinux.peng@gmail.com>
On Wed, Nov 12, 2025 at 11:46 AM Donglin Peng <dolinux.peng@gmail.com> wrote:
>
> From: Donglin Peng <pengdonglin@xiaomi.com>
>
> Currently, the funcgraph-args and funcgraph-retaddr features are mutually exclusive.
> This patch resolves this limitation by modifying funcgraph-retaddr to adopt the same
> implementation approach as funcgraph-args, specifically by storing the return address
> in the first entry of the args array.
>
> As a result, both features now coexist seamlessly and function as intended.
>
> To verify the change, use perf to trace vfs_write with both options enabled:
>
> Before:
> # perf ftrace -G vfs_write --graph-opts args,retaddr
> ......
> 0) | down_read() { /* <-n_tty_write+0xa3/0x540 */
> 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> 0) 0.079 us | preempt_count_add(); /* <-down_read+0x3b/0x160 */
> 0) 0.077 us | preempt_count_sub(); /* <-down_read+0x8b/0x160 */
> 0) 0.754 us | }
>
> After:
> # perf ftrace -G vfs_write --graph-opts args,retaddr
> ......
> 0) | down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
> 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> 0) 0.079 us | preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
> 0) 0.077 us | preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
> 0) 0.754 us | }
>
> Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
> Cc: Sven Schnelle <svens@linux.ibm.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
> ---
> v3:
> - Replace min() with min_t() to improve type safety and maintainability
> - Keep only one Signed-off-by for cleaner attribution
> - Code refactoring for improved readability
>
> v2:
> - Preserve retaddr event functionality (suggested by Steven)
> - Store the retaddr in args[0] (suggested by Steven)
> - Refactor implementation logic and commit message clarity
> ---
> include/linux/ftrace.h | 11 --
> kernel/trace/trace.h | 4 -
> kernel/trace/trace_entries.h | 6 +-
> kernel/trace/trace_functions_graph.c | 145 ++++++++++++---------------
> 4 files changed, 69 insertions(+), 97 deletions(-)
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 7ded7df6e9b5..88cb54d73bdb 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
> int depth;
> } __packed;
>
> -/*
> - * Structure that defines an entry function trace with retaddr.
> - * It's already packed but the attribute "packed" is needed
> - * to remove extra padding at the end.
> - */
> -struct fgraph_retaddr_ent {
> - unsigned long func; /* Current function */
> - int depth;
> - unsigned long retaddr; /* Return address */
> -} __packed;
> -
> /*
> * Structure that defines a return function trace.
> * It's already packed but the attribute "packed" is needed
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 85eabb454bee..9fac291b913a 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -955,10 +955,6 @@ extern void graph_trace_close(struct trace_iterator *iter);
> extern int __trace_graph_entry(struct trace_array *tr,
> struct ftrace_graph_ent *trace,
> unsigned int trace_ctx);
> -extern int __trace_graph_retaddr_entry(struct trace_array *tr,
> - struct ftrace_graph_ent *trace,
> - unsigned int trace_ctx,
> - unsigned long retaddr);
> extern void __trace_graph_return(struct trace_array *tr,
> struct ftrace_graph_ret *trace,
> unsigned int trace_ctx,
> diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> index de294ae2c5c5..593a74663c65 100644
> --- a/kernel/trace/trace_entries.h
> +++ b/kernel/trace/trace_entries.h
> @@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> TRACE_GRAPH_RETADDR_ENT,
>
> F_STRUCT(
> - __field_struct( struct fgraph_retaddr_ent, graph_ent )
> + __field_struct( struct ftrace_graph_ent, graph_ent )
> __field_packed( unsigned long, graph_ent, func )
> __field_packed( unsigned int, graph_ent, depth )
> - __field_packed( unsigned long, graph_ent, retaddr )
> + __dynamic_array(unsigned long, args )
> ),
>
> F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
> - (void *)__entry->retaddr)
> + (void *)__entry->args[0])
> );
>
> #else
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index a7f4b9a47a71..f31eeeffbb2d 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -16,6 +16,15 @@
> #include "trace.h"
> #include "trace_output.h"
>
> +#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> +#define HAVE_RETADDR 1
> +#define ARGS_OFFS(args_size) \
> + ((args_size) > FTRACE_REGS_MAX_ARGS * sizeof(long) ? 1 : 0)
> +#else
> +#define HAVE_RETADDR 0
> +#define ARGS_OFFS(args_size) 0
> +#endif
> +
> /* When set, irq functions will be ignored */
> static int ftrace_graph_skip_irqs;
>
> @@ -27,21 +36,25 @@ struct fgraph_cpu_data {
> unsigned long enter_funcs[FTRACE_RETFUNC_DEPTH];
> };
>
> +/*
> + * fgraph_retaddr_ent_entry and ftrace_graph_ent_entry share layout, ent
> + * member repurposed for storage
> + */
> struct fgraph_ent_args {
> struct ftrace_graph_ent_entry ent;
> - /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
> - unsigned long args[FTRACE_REGS_MAX_ARGS];
> + /*
> + * Force the sizeof of args[] to have (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR)
> + * entries with the first entry storing the return address for
> + * TRACE_GRAPH_RETADDR_ENT.
> + */
> + unsigned long args[FTRACE_REGS_MAX_ARGS + HAVE_RETADDR];
> };
>
> struct fgraph_data {
> struct fgraph_cpu_data __percpu *cpu_data;
>
> /* Place to preserve last processed entry. */
> - union {
> - struct fgraph_ent_args ent;
> - /* TODO allow retaddr to have args */
> - struct fgraph_retaddr_ent_entry rent;
> - };
> + struct fgraph_ent_args ent;
> struct ftrace_graph_ret_entry ret;
> int failed;
> int cpu;
> @@ -127,22 +140,43 @@ static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
> struct ring_buffer_event *event;
> struct trace_buffer *buffer = tr->array_buffer.buffer;
> struct ftrace_graph_ent_entry *entry;
> - int size;
> + unsigned long retaddr = 0;
> + int size = sizeof(*entry);
> + int type = TRACE_GRAPH_ENT;
> + bool store_args = false;
> + int nr_args = 0, args_offs = 0;
> +
> + if (tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> + retaddr = ftrace_graph_top_ret_addr(current);
> + type = TRACE_GRAPH_RETADDR_ENT;
> + nr_args += 1;
> + }
>
> /* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
> - size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
> + if (tracer_flags_is_set(TRACE_GRAPH_ARGS)) {
> + store_args = !!fregs;
> + if (store_args)
> + nr_args += FTRACE_REGS_MAX_ARGS;
> + }
>
> - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size, trace_ctx);
> + size += nr_args * sizeof(long);
> + event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
> if (!event)
> return 0;
>
> entry = ring_buffer_event_data(event);
> entry->graph_ent = *trace;
>
> + /* Store the retaddr in args[0] */
> + if (type == TRACE_GRAPH_RETADDR_ENT) {
> + entry->args[0] = retaddr;
> + args_offs += 1;
> + }
> +
> #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
> - if (fregs) {
> + if (store_args) {
> for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
> - entry->args[i] = ftrace_regs_get_argument(fregs, i);
> + entry->args[i + args_offs] = ftrace_regs_get_argument(fregs, i);
> }
> #endif
>
> @@ -158,38 +192,6 @@ int __trace_graph_entry(struct trace_array *tr,
> return __graph_entry(tr, trace, trace_ctx, NULL);
> }
>
> -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> -int __trace_graph_retaddr_entry(struct trace_array *tr,
> - struct ftrace_graph_ent *trace,
> - unsigned int trace_ctx,
> - unsigned long retaddr)
> -{
> - struct ring_buffer_event *event;
> - struct trace_buffer *buffer = tr->array_buffer.buffer;
> - struct fgraph_retaddr_ent_entry *entry;
> -
> - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
> - sizeof(*entry), trace_ctx);
> - if (!event)
> - return 0;
> - entry = ring_buffer_event_data(event);
> - entry->graph_ent.func = trace->func;
> - entry->graph_ent.depth = trace->depth;
> - entry->graph_ent.retaddr = retaddr;
> - trace_buffer_unlock_commit_nostack(buffer, event);
> -
> - return 1;
> -}
> -#else
> -int __trace_graph_retaddr_entry(struct trace_array *tr,
> - struct ftrace_graph_ent *trace,
> - unsigned int trace_ctx,
> - unsigned long retaddr)
> -{
> - return 1;
> -}
> -#endif
> -
> static inline int ftrace_graph_ignore_irqs(void)
> {
> if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
> @@ -211,7 +213,6 @@ static int graph_entry(struct ftrace_graph_ent *trace,
> struct trace_array *tr = gops->private;
> struct fgraph_times *ftimes;
> unsigned int trace_ctx;
> - int ret = 0;
>
> if (*task_var & TRACE_GRAPH_NOTRACE)
> return 0;
> @@ -262,15 +263,7 @@ static int graph_entry(struct ftrace_graph_ent *trace,
> return 1;
>
> trace_ctx = tracing_gen_ctx();
> - if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
> - tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> - unsigned long retaddr = ftrace_graph_top_ret_addr(current);
> - ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
> - } else {
> - ret = __graph_entry(tr, trace, trace_ctx, fregs);
> - }
> -
> - return ret;
> + return __graph_entry(tr, trace, trace_ctx, fregs);
> }
>
> int trace_graph_entry(struct ftrace_graph_ent *trace,
> @@ -634,13 +627,9 @@ get_return_for_leaf(struct trace_iterator *iter,
> * Save current and next entries for later reference
> * if the output fails.
> */
> - if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
> - data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
> - } else {
> - int size = min((int)sizeof(data->ent), (int)iter->ent_size);
> + int size = min_t(int, sizeof(data->ent), iter->ent_size);
>
> - memcpy(&data->ent, curr, size);
> - }
> + memcpy(&data->ent, curr, size);
> /*
> * If the next event is not a return type, then
> * we only care about what type it is. Otherwise we can
> @@ -811,21 +800,21 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
>
> #ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> #define __TRACE_GRAPH_PRINT_RETADDR TRACE_GRAPH_PRINT_RETADDR
> -static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_entry *entry,
> - u32 trace_flags, bool comment)
> +static void print_graph_retaddr(struct trace_seq *s, unsigned long retaddr, u32 trace_flags,
> + bool comment)
> {
> if (comment)
> trace_seq_puts(s, " /*");
>
> trace_seq_puts(s, " <-");
> - seq_print_ip_sym(s, entry->graph_ent.retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
> + seq_print_ip_sym(s, retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
>
> if (comment)
> trace_seq_puts(s, " */");
> }
> #else
> #define __TRACE_GRAPH_PRINT_RETADDR 0
> -#define print_graph_retaddr(_seq, _entry, _tflags, _comment) do { } while (0)
> +#define print_graph_retaddr(_seq, _retaddr, _tflags, _comment) do { } while (0)
> #endif
>
> #if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
> @@ -869,10 +858,12 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> trace_seq_printf(s, "%ps", func);
>
> if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
Hi Team,
I have a question regarding the behavior of the funcgraph-args option.
Currently, when the funcgraph-args flag is set, the function arguments
are saved.
However, when the flag is cleared, the trace output still displays the function
arguments unconditionally.
I expected that the flag would control both saving and displaying the arguments.
But it seems that the current design only controls the saving. This
behavior seems
inconsistent.
I think we should add a check for the flag in the display part,
specifically before
calling print_function_args, to ensure that the arguments are only
displayed when
the flag is set.
What are your thoughts?
Thanks,
Donglin
> - print_function_args(s, entry->args, (unsigned long)func);
> + print_function_args(s, entry->args + ARGS_OFFS(args_size),
> + (unsigned long)func);
> trace_seq_putc(s, ';');
> - } else
> + } else {
> trace_seq_puts(s, "();");
> + }
>
> if (print_retval || print_retaddr)
> trace_seq_puts(s, " /*");
> @@ -882,8 +873,7 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> }
>
> if (print_retaddr)
> - print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
> - trace_flags, false);
> + print_graph_retaddr(s, entry->args[0], trace_flags, false);
>
> if (print_retval) {
> if (hex_format || (err_code == 0))
> @@ -964,10 +954,12 @@ print_graph_entry_leaf(struct trace_iterator *iter,
> trace_seq_printf(s, "%ps", (void *)ret_func);
>
> if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
> - print_function_args(s, entry->args, ret_func);
> + print_function_args(s, entry->args + ARGS_OFFS(args_size),
> + ret_func);
> trace_seq_putc(s, ';');
> - } else
> + } else {
> trace_seq_puts(s, "();");
> + }
> }
> trace_seq_putc(s, '\n');
>
> @@ -1016,7 +1008,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
> args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
>
> if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
> - print_function_args(s, entry->args, func);
> + print_function_args(s, entry->args + ARGS_OFFS(args_size), func);
> else
> trace_seq_puts(s, "()");
>
> @@ -1024,8 +1016,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
>
> if (flags & __TRACE_GRAPH_PRINT_RETADDR &&
> entry->ent.type == TRACE_GRAPH_RETADDR_ENT)
> - print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
> - tr->trace_flags, true);
> + print_graph_retaddr(s, entry->args[0], tr->trace_flags, true);
> trace_seq_putc(s, '\n');
>
> if (trace_seq_has_overflowed(s))
> @@ -1202,7 +1193,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
> * it can be safely saved at the stack.
> */
> struct ftrace_graph_ent_entry *entry;
> - u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
> + u8 save_buf[sizeof(*entry) + (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR) * sizeof(long)];
>
> /* The ent_size is expected to be as big as the entry */
> if (iter->ent_size > sizeof(save_buf))
> @@ -1429,16 +1420,12 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
> trace_assign_type(field, entry);
> return print_graph_entry(field, s, iter, flags);
> }
> -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> case TRACE_GRAPH_RETADDR_ENT: {
> - struct fgraph_retaddr_ent_entry saved;
> struct fgraph_retaddr_ent_entry *rfield;
>
> trace_assign_type(rfield, entry);
> - saved = *rfield;
> - return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
> + return print_graph_entry((typeof(field))rfield, s, iter, flags);
> }
> -#endif
> case TRACE_GRAPH_RET: {
> struct ftrace_graph_ret_entry *field;
> trace_assign_type(field, entry);
> --
> 2.34.1
>
^ permalink raw reply
* Re: [RFC PATCH v1 01/37] KVM: guest_memfd: Introduce per-gmem attributes, use to guard user mappings
From: Binbin Wu @ 2025-11-12 8:58 UTC (permalink / raw)
To: Ackerley Tng
Cc: cgroups, kvm, linux-doc, linux-fsdevel, linux-kernel,
linux-kselftest, linux-mm, linux-trace-kernel, x86, akpm, bp,
brauner, chao.p.peng, chenhuacai, corbet, dave.hansen,
dave.hansen, david, dmatlack, erdemaktas, fan.du, fvdl, haibo1.xu,
hannes, hch, hpa, hughd, ira.weiny, isaku.yamahata, jack,
james.morse, jarkko, jgg, jgowans, jhubbard, jroedel, jthoughton,
jun.miao, kai.huang, keirf, kent.overstreet, liam.merwick,
maciej.wieczor-retman, mail, maobibo, mathieu.desnoyers, maz,
mhiramat, mhocko, mic, michael.roth, mingo, mlevitsk, mpe,
muchun.song, nikunj, nsaenz, oliver.upton, palmer, pankaj.gupta,
paul.walmsley, pbonzini, peterx, pgonda, prsampat, pvorel,
qperret, richard.weiyang, rick.p.edgecombe, rientjes, rostedt,
roypat, rppt, seanjc, shakeel.butt, shuah, steven.price,
steven.sistare, suzuki.poulose, tabba, tglx, thomas.lendacky,
vannapurve, vbabka, viro, vkuznets, wei.w.wang, will, willy,
wyihan, xiaoyao.li, yan.y.zhao, yilun.xu, yuzenghui, zhiquan1.li
In-Reply-To: <638600e19c6e23959bad60cf61582f387dff6445.1760731772.git.ackerleytng@google.com>
On 10/18/2025 4:11 AM, Ackerley Tng wrote:
[...]
>
> +static int kvm_gmem_init_inode(struct inode *inode, loff_t size, u64 flags)
> +{
> + struct gmem_inode *gi = GMEM_I(inode);
> + MA_STATE(mas, &gi->attributes, 0, (size >> PAGE_SHIFT) - 1);
> + u64 attrs;
> + int r;
> +
> + inode->i_op = &kvm_gmem_iops;
> + inode->i_mapping->a_ops = &kvm_gmem_aops;
> + inode->i_mode |= S_IFREG;
> + inode->i_size = size;
> + mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
> + mapping_set_inaccessible(inode->i_mapping);
> + /* Unmovable mappings are supposed to be marked unevictable as well. */
AS_UNMOVABLE has been removed and got merged into AS_INACCESSIBLE, not sure if
it's better to use "Inaccessible" instead of "Unmovable"
> + WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
> +
> + gi->flags = flags;
> +
> + mt_set_external_lock(&gi->attributes,
> + &inode->i_mapping->invalidate_lock);
> +
> + /*
> + * Store default attributes for the entire gmem instance. Ensuring every
> + * index is represented in the maple tree at all times simplifies the
> + * conversion and merging logic.
> + */
> + attrs = gi->flags & GUEST_MEMFD_FLAG_INIT_SHARED ? 0 : KVM_MEMORY_ATTRIBUTE_PRIVATE;
> +
> + /*
> + * Acquire the invalidation lock purely to make lockdep happy. There
> + * should be no races at this time since the inode hasn't yet been fully
> + * created.
> + */
> + filemap_invalidate_lock(inode->i_mapping);
> + r = mas_store_gfp(&mas, xa_mk_value(attrs), GFP_KERNEL);
> + filemap_invalidate_unlock(inode->i_mapping);
> +
> + return r;
> +}
> +
[...]
> @@ -925,13 +986,39 @@ static struct inode *kvm_gmem_alloc_inode(struct super_block *sb)
>
> mpol_shared_policy_init(&gi->policy, NULL);
>
> + /*
> + * Memory attributes are protected the filemap invalidation lock, but
^
protected by
> + * the lock structure isn't available at this time. Immediately mark
> + * maple tree as using external locking so that accessing the tree
> + * before its fully initialized results in NULL pointer dereferences
> + * and not more subtle bugs.
> + */
> + mt_init_flags(&gi->attributes, MT_FLAGS_LOCK_EXTERN);
> +
> gi->flags = 0;
> return &gi->vfs_inode;
> }
>
> static void kvm_gmem_destroy_inode(struct inode *inode)
> {
> - mpol_free_shared_policy(&GMEM_I(inode)->policy);
> + struct gmem_inode *gi = GMEM_I(inode);
> +
> + mpol_free_shared_policy(&gi->policy);
> +
> + /*
> + * Note! Checking for an empty tree is functionally necessary to avoid
> + * explosions if the tree hasn't been initialized, i.e. if the inode is
It makes sense to skip __mt_destroy() when mtree is empty.
But what explosions it could trigger if mtree is empty?
It seems __mt_destroy() can handle the case if the external lock is not set.
> + * being destroyed before guest_memfd can set the external lock.
> + */
> + if (!mtree_empty(&gi->attributes)) {
> + /*
> + * Acquire the invalidation lock purely to make lockdep happy,
> + * the inode is unreachable at this point.
> + */
> + filemap_invalidate_lock(inode->i_mapping);
> + __mt_destroy(&gi->attributes);
> + filemap_invalidate_unlock(inode->i_mapping);
> + }
> }
>
> static void kvm_gmem_free_inode(struct inode *inode)
> --
> 2.51.0.858.gf9c4a03a3a-goog
^ permalink raw reply
* Re: [PATCH v3 RESEND] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-12 7:22 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: rostedt, linux-trace-kernel, linux-kernel, Donglin Peng,
Sven Schnelle
In-Reply-To: <20251112161017.d9da54e462817cc9053688f6@kernel.org>
On Wed, Nov 12, 2025 at 3:10 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Wed, 12 Nov 2025 14:03:30 +0800
> Donglin Peng <dolinux.peng@gmail.com> wrote:
>
> > On Wed, Nov 12, 2025 at 1:17 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > On Wed, 12 Nov 2025 11:43:33 +0800
> > > Donglin Peng <dolinux.peng@gmail.com> wrote:
> > >
> > > > From: Donglin Peng <pengdonglin@xiaomi.com>
> > > >
> > > > Currently, the funcgraph-args and funcgraph-retaddr features are mutually exclusive.
> > > > This patch resolves this limitation by modifying funcgraph-retaddr to adopt the same
> > > > implementation approach as funcgraph-args, specifically by storing the return address
> > > > in the first entry of the args array.
>
> Ah, if you can update the patch, please keep each line shorter than
> 70-75 characters.
Thanks, I will fix it in the next version.
>
> > > >
> > > > As a result, both features now coexist seamlessly and function as intended.
> > > >
> > > > To verify the change, use perf to trace vfs_write with both options enabled:
> > > >
> > > > Before:
> > > > # perf ftrace -G vfs_write --graph-opts args,retaddr
> > > > ......
> > > > 0) | down_read() { /* <-n_tty_write+0xa3/0x540 */
> > > > 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> > > > 0) 0.079 us | preempt_count_add(); /* <-down_read+0x3b/0x160 */
> > > > 0) 0.077 us | preempt_count_sub(); /* <-down_read+0x8b/0x160 */
> > > > 0) 0.754 us | }
> > > >
> > > > After:
> > > > # perf ftrace -G vfs_write --graph-opts args,retaddr
> > > > ......
> > > > 0) | down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
> > > > 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> > > > 0) 0.079 us | preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
> > > > 0) 0.077 us | preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
> > > > 0) 0.754 us | }
> > > >
> > > > Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
> > > > Cc: Sven Schnelle <svens@linux.ibm.com>
> > > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > > Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
> > >
> > > Looks good to me except for a few nits, but it's a style issue.
> > >
> > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Thank you for ack.
> >
> > >
> > > Thank you,
> > >
> > > > ---
> > > > v3:
> > > > - Replace min() with min_t() to improve type safety and maintainability
> > > > - Keep only one Signed-off-by for cleaner attribution
> > > > - Code refactoring for improved readability
> > > >
> > > > v2:
> > > > - Preserve retaddr event functionality (suggested by Steven)
> > > > - Store the retaddr in args[0] (suggested by Steven)
> > > > - Refactor implementation logic and commit message clarity
> > > > ---
> > > > include/linux/ftrace.h | 11 --
> > > > kernel/trace/trace.h | 4 -
> > > > kernel/trace/trace_entries.h | 6 +-
> > > > kernel/trace/trace_functions_graph.c | 145 ++++++++++++---------------
> > > > 4 files changed, 69 insertions(+), 97 deletions(-)
> > > >
> > > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > > > index 7ded7df6e9b5..88cb54d73bdb 100644
> > > > --- a/include/linux/ftrace.h
> > > > +++ b/include/linux/ftrace.h
> > > > @@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
> > > > int depth;
> > > > } __packed;
> > > >
> > > > -/*
> > > > - * Structure that defines an entry function trace with retaddr.
> > > > - * It's already packed but the attribute "packed" is needed
> > > > - * to remove extra padding at the end.
> > > > - */
> > > > -struct fgraph_retaddr_ent {
> > > > - unsigned long func; /* Current function */
> > > > - int depth;
> > > > - unsigned long retaddr; /* Return address */
> > > > -} __packed;
> > > > -
> > > > /*
> > > > * Structure that defines a return function trace.
> > > > * It's already packed but the attribute "packed" is needed
> > > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > > > index 85eabb454bee..9fac291b913a 100644
> > > > --- a/kernel/trace/trace.h
> > > > +++ b/kernel/trace/trace.h
> > > > @@ -955,10 +955,6 @@ extern void graph_trace_close(struct trace_iterator *iter);
> > > > extern int __trace_graph_entry(struct trace_array *tr,
> > > > struct ftrace_graph_ent *trace,
> > > > unsigned int trace_ctx);
> > > > -extern int __trace_graph_retaddr_entry(struct trace_array *tr,
> > > > - struct ftrace_graph_ent *trace,
> > > > - unsigned int trace_ctx,
> > > > - unsigned long retaddr);
> > > > extern void __trace_graph_return(struct trace_array *tr,
> > > > struct ftrace_graph_ret *trace,
> > > > unsigned int trace_ctx,
> > > > diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> > > > index de294ae2c5c5..593a74663c65 100644
> > > > --- a/kernel/trace/trace_entries.h
> > > > +++ b/kernel/trace/trace_entries.h
> > > > @@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> > > > TRACE_GRAPH_RETADDR_ENT,
> > > >
> > > > F_STRUCT(
> > > > - __field_struct( struct fgraph_retaddr_ent, graph_ent )
> > > > + __field_struct( struct ftrace_graph_ent, graph_ent )
> > > > __field_packed( unsigned long, graph_ent, func )
> > > > __field_packed( unsigned int, graph_ent, depth )
> > > > - __field_packed( unsigned long, graph_ent, retaddr )
> > > > + __dynamic_array(unsigned long, args )
> > > > ),
> > > >
> > > > F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
> > > > - (void *)__entry->retaddr)
> > > > + (void *)__entry->args[0])
> > > > );
> > > >
> > > > #else
> > > > diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> > > > index a7f4b9a47a71..f31eeeffbb2d 100644
> > > > --- a/kernel/trace/trace_functions_graph.c
> > > > +++ b/kernel/trace/trace_functions_graph.c
> > > > @@ -16,6 +16,15 @@
> > > > #include "trace.h"
> > > > #include "trace_output.h"
> > > >
> > > > +#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > > > +#define HAVE_RETADDR 1
> > > > +#define ARGS_OFFS(args_size) \
> > > > + ((args_size) > FTRACE_REGS_MAX_ARGS * sizeof(long) ? 1 : 0)
> > > > +#else
> > > > +#define HAVE_RETADDR 0
> > > > +#define ARGS_OFFS(args_size) 0
> > > > +#endif
> > > > +
> > > > /* When set, irq functions will be ignored */
> > > > static int ftrace_graph_skip_irqs;
> > > >
> > > > @@ -27,21 +36,25 @@ struct fgraph_cpu_data {
> > > > unsigned long enter_funcs[FTRACE_RETFUNC_DEPTH];
> > > > };
> > > >
> > > > +/*
> > > > + * fgraph_retaddr_ent_entry and ftrace_graph_ent_entry share layout, ent
> > > > + * member repurposed for storage
> > > > + */
> > > > struct fgraph_ent_args {
> > > > struct ftrace_graph_ent_entry ent;
> > > > - /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
> > > > - unsigned long args[FTRACE_REGS_MAX_ARGS];
> > > > + /*
> > > > + * Force the sizeof of args[] to have (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR)
> > > > + * entries with the first entry storing the return address for
> > > > + * TRACE_GRAPH_RETADDR_ENT.
> > > > + */
> > > > + unsigned long args[FTRACE_REGS_MAX_ARGS + HAVE_RETADDR];
> > > > };
> > > >
> > > > struct fgraph_data {
> > > > struct fgraph_cpu_data __percpu *cpu_data;
> > > >
> > > > /* Place to preserve last processed entry. */
> > > > - union {
> > > > - struct fgraph_ent_args ent;
> > > > - /* TODO allow retaddr to have args */
> > > > - struct fgraph_retaddr_ent_entry rent;
> > > > - };
> > > > + struct fgraph_ent_args ent;
> > > > struct ftrace_graph_ret_entry ret;
> > > > int failed;
> > > > int cpu;
> > > > @@ -127,22 +140,43 @@ static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
> > > > struct ring_buffer_event *event;
> > > > struct trace_buffer *buffer = tr->array_buffer.buffer;
> > > > struct ftrace_graph_ent_entry *entry;
> > > > - int size;
> > > > + unsigned long retaddr = 0;
> > > > + int size = sizeof(*entry);
> > > > + int type = TRACE_GRAPH_ENT;
> > > > + bool store_args = false;
> > > > + int nr_args = 0, args_offs = 0;
> > > > +
> > > > + if (tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> > > > + retaddr = ftrace_graph_top_ret_addr(current);
> > > > + type = TRACE_GRAPH_RETADDR_ENT;
> > > > + nr_args += 1;
> > > > + }
> > > >
> > > > /* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
> > > > - size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
> > > > + if (tracer_flags_is_set(TRACE_GRAPH_ARGS)) {
> > > > + store_args = !!fregs;
> > > > + if (store_args)
> > > > + nr_args += FTRACE_REGS_MAX_ARGS;
> > > > + }
> > > >
> > > > - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size, trace_ctx);
> > > > + size += nr_args * sizeof(long);
> > > > + event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
> > > > if (!event)
> > > > return 0;
> > > >
> > > > entry = ring_buffer_event_data(event);
> > > > entry->graph_ent = *trace;
> > > >
> > > > + /* Store the retaddr in args[0] */
> > > > + if (type == TRACE_GRAPH_RETADDR_ENT) {
> > > > + entry->args[0] = retaddr;
> > > > + args_offs += 1;
> > > > + }
> > > > +
> > > > #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
> > > > - if (fregs) {
> > > > + if (store_args) {
> > > > for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
> > > > - entry->args[i] = ftrace_regs_get_argument(fregs, i);
> > > > + entry->args[i + args_offs] = ftrace_regs_get_argument(fregs, i);
> > > > }
> > > > #endif
> > > >
> > > > @@ -158,38 +192,6 @@ int __trace_graph_entry(struct trace_array *tr,
> > > > return __graph_entry(tr, trace, trace_ctx, NULL);
> > > > }
> > > >
> > > > -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > > > -int __trace_graph_retaddr_entry(struct trace_array *tr,
> > > > - struct ftrace_graph_ent *trace,
> > > > - unsigned int trace_ctx,
> > > > - unsigned long retaddr)
> > > > -{
> > > > - struct ring_buffer_event *event;
> > > > - struct trace_buffer *buffer = tr->array_buffer.buffer;
> > > > - struct fgraph_retaddr_ent_entry *entry;
> > > > -
> > > > - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
> > > > - sizeof(*entry), trace_ctx);
> > > > - if (!event)
> > > > - return 0;
> > > > - entry = ring_buffer_event_data(event);
> > > > - entry->graph_ent.func = trace->func;
> > > > - entry->graph_ent.depth = trace->depth;
> > > > - entry->graph_ent.retaddr = retaddr;
> > > > - trace_buffer_unlock_commit_nostack(buffer, event);
> > > > -
> > > > - return 1;
> > > > -}
> > > > -#else
> > > > -int __trace_graph_retaddr_entry(struct trace_array *tr,
> > > > - struct ftrace_graph_ent *trace,
> > > > - unsigned int trace_ctx,
> > > > - unsigned long retaddr)
> > > > -{
> > > > - return 1;
> > > > -}
> > > > -#endif
> > > > -
> > > > static inline int ftrace_graph_ignore_irqs(void)
> > > > {
> > > > if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
> > > > @@ -211,7 +213,6 @@ static int graph_entry(struct ftrace_graph_ent *trace,
> > > > struct trace_array *tr = gops->private;
> > > > struct fgraph_times *ftimes;
> > > > unsigned int trace_ctx;
> > > > - int ret = 0;
> > > >
> > > > if (*task_var & TRACE_GRAPH_NOTRACE)
> > > > return 0;
> > > > @@ -262,15 +263,7 @@ static int graph_entry(struct ftrace_graph_ent *trace,
> > > > return 1;
> > > >
> > > > trace_ctx = tracing_gen_ctx();
> > > > - if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
> > > > - tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> > > > - unsigned long retaddr = ftrace_graph_top_ret_addr(current);
> > > > - ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
> > > > - } else {
> > > > - ret = __graph_entry(tr, trace, trace_ctx, fregs);
> > > > - }
> > > > -
> > > > - return ret;
> > > > + return __graph_entry(tr, trace, trace_ctx, fregs);
> > > > }
> > > >
> > > > int trace_graph_entry(struct ftrace_graph_ent *trace,
> > > > @@ -634,13 +627,9 @@ get_return_for_leaf(struct trace_iterator *iter,
> > > > * Save current and next entries for later reference
> > > > * if the output fails.
> > > > */
> > > > - if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
> > > > - data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
> > > > - } else {
> > > > - int size = min((int)sizeof(data->ent), (int)iter->ent_size);
> > > > + int size = min_t(int, sizeof(data->ent), iter->ent_size);
> > > >
> > > > - memcpy(&data->ent, curr, size);
> > > > - }
> > > > + memcpy(&data->ent, curr, size);
> > > > /*
> > > > * If the next event is not a return type, then
> > > > * we only care about what type it is. Otherwise we can
> > > > @@ -811,21 +800,21 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
> > > >
> > > > #ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > > > #define __TRACE_GRAPH_PRINT_RETADDR TRACE_GRAPH_PRINT_RETADDR
> > > > -static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_entry *entry,
> > > > - u32 trace_flags, bool comment)
> > > > +static void print_graph_retaddr(struct trace_seq *s, unsigned long retaddr, u32 trace_flags,
> > > > + bool comment)
> > > > {
> > > > if (comment)
> > > > trace_seq_puts(s, " /*");
> > > >
> > > > trace_seq_puts(s, " <-");
> > > > - seq_print_ip_sym(s, entry->graph_ent.retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
> > > > + seq_print_ip_sym(s, retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
> > > >
> > > > if (comment)
> > > > trace_seq_puts(s, " */");
> > > > }
> > > > #else
> > > > #define __TRACE_GRAPH_PRINT_RETADDR 0
> > > > -#define print_graph_retaddr(_seq, _entry, _tflags, _comment) do { } while (0)
> > > > +#define print_graph_retaddr(_seq, _retaddr, _tflags, _comment) do { } while (0)
> > > > #endif
> > > >
> > > > #if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
> > > > @@ -869,10 +858,12 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> > > > trace_seq_printf(s, "%ps", func);
> > > >
> > > > if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
> > > > - print_function_args(s, entry->args, (unsigned long)func);
> > > > + print_function_args(s, entry->args + ARGS_OFFS(args_size),
> > > > + (unsigned long)func);
> > > > trace_seq_putc(s, ';');
> > > > - } else
> > > > + } else {
> > >
> > > nit: you don't need to add braces for a single line block.
> >
> > Thanks. I added the else braces for consistency with the
> > multi-statement if branch,
> > per coding-style.rst. Happy to remove if you prefer the
> > single-statement exception.
>
> Ah, OK. I also checked this passed checkpatch.pl. (some other
> macros are warned but those are false positive.)
Thanks.
>
> Thanks,
>
> >
> > https://elixir.bootlin.com/linux/v6.18-rc5/source/Documentation/process/coding-style.rst#L213
> >
> > Do not unnecessarily use braces where a single statement will do.
> >
> > .. code-block:: c
> >
> > if (condition)
> > action();
> >
> > and
> >
> > .. code-block:: c
> >
> > if (condition)
> > do_this();
> > else
> > do_that();
> >
> > This does not apply if only one branch of a conditional statement is a single
> > statement; in the latter case use braces in both branches:
> >
> > .. code-block:: c
> >
> > if (condition) {
> > do_this();
> > do_that();
> > } else {
> > otherwise();
> > }
> >
> > >
> > > > trace_seq_puts(s, "();");
> > > > + }
> > > >
> > > > if (print_retval || print_retaddr)
> > > > trace_seq_puts(s, " /*");
> > > > @@ -882,8 +873,7 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> > > > }
> > > >
> > > > if (print_retaddr)
> > > > - print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
> > > > - trace_flags, false);
> > > > + print_graph_retaddr(s, entry->args[0], trace_flags, false);
> > > >
> > > > if (print_retval) {
> > > > if (hex_format || (err_code == 0))
> > > > @@ -964,10 +954,12 @@ print_graph_entry_leaf(struct trace_iterator *iter,
> > > > trace_seq_printf(s, "%ps", (void *)ret_func);
> > > >
> > > > if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
> > > > - print_function_args(s, entry->args, ret_func);
> > > > + print_function_args(s, entry->args + ARGS_OFFS(args_size),
> > > > + ret_func);
> > > > trace_seq_putc(s, ';');
> > > > - } else
> > > > + } else {
> > >
> > > Ditto.
> > >
> > > > trace_seq_puts(s, "();");
> > > > + }
> > > > }
> > > > trace_seq_putc(s, '\n');
> > > >
> > > > @@ -1016,7 +1008,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
> > > > args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
> > > >
> > > > if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
> > > > - print_function_args(s, entry->args, func);
> > > > + print_function_args(s, entry->args + ARGS_OFFS(args_size), func);
> > > > else
> > > > trace_seq_puts(s, "()");
> > > >
> > > > @@ -1024,8 +1016,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
> > > >
> > > > if (flags & __TRACE_GRAPH_PRINT_RETADDR &&
> > > > entry->ent.type == TRACE_GRAPH_RETADDR_ENT)
> > > > - print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
> > > > - tr->trace_flags, true);
> > > > + print_graph_retaddr(s, entry->args[0], tr->trace_flags, true);
> > > > trace_seq_putc(s, '\n');
> > > >
> > > > if (trace_seq_has_overflowed(s))
> > > > @@ -1202,7 +1193,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
> > > > * it can be safely saved at the stack.
> > > > */
> > > > struct ftrace_graph_ent_entry *entry;
> > > > - u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
> > > > + u8 save_buf[sizeof(*entry) + (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR) * sizeof(long)];
> > > >
> > > > /* The ent_size is expected to be as big as the entry */
> > > > if (iter->ent_size > sizeof(save_buf))
> > > > @@ -1429,16 +1420,12 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
> > > > trace_assign_type(field, entry);
> > > > return print_graph_entry(field, s, iter, flags);
> > > > }
> > > > -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > > > case TRACE_GRAPH_RETADDR_ENT: {
> > > > - struct fgraph_retaddr_ent_entry saved;
> > > > struct fgraph_retaddr_ent_entry *rfield;
> > > >
> > > > trace_assign_type(rfield, entry);
> > > > - saved = *rfield;
> > > > - return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
> > > > + return print_graph_entry((typeof(field))rfield, s, iter, flags);
> > > > }
> > > > -#endif
> > > > case TRACE_GRAPH_RET: {
> > > > struct ftrace_graph_ret_entry *field;
> > > > trace_assign_type(field, entry);
> > > > --
> > > > 2.34.1
> > > >
> > >
> > >
> > > --
> > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 RESEND] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Masami Hiramatsu @ 2025-11-12 7:10 UTC (permalink / raw)
To: Donglin Peng
Cc: rostedt, linux-trace-kernel, linux-kernel, Donglin Peng,
Sven Schnelle
In-Reply-To: <CAErzpmsHNRzB2pBFVSAG4ii=VN98NEtehEbnYNQY-4TBWgM3ag@mail.gmail.com>
On Wed, 12 Nov 2025 14:03:30 +0800
Donglin Peng <dolinux.peng@gmail.com> wrote:
> On Wed, Nov 12, 2025 at 1:17 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Wed, 12 Nov 2025 11:43:33 +0800
> > Donglin Peng <dolinux.peng@gmail.com> wrote:
> >
> > > From: Donglin Peng <pengdonglin@xiaomi.com>
> > >
> > > Currently, the funcgraph-args and funcgraph-retaddr features are mutually exclusive.
> > > This patch resolves this limitation by modifying funcgraph-retaddr to adopt the same
> > > implementation approach as funcgraph-args, specifically by storing the return address
> > > in the first entry of the args array.
Ah, if you can update the patch, please keep each line shorter than
70-75 characters.
> > >
> > > As a result, both features now coexist seamlessly and function as intended.
> > >
> > > To verify the change, use perf to trace vfs_write with both options enabled:
> > >
> > > Before:
> > > # perf ftrace -G vfs_write --graph-opts args,retaddr
> > > ......
> > > 0) | down_read() { /* <-n_tty_write+0xa3/0x540 */
> > > 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> > > 0) 0.079 us | preempt_count_add(); /* <-down_read+0x3b/0x160 */
> > > 0) 0.077 us | preempt_count_sub(); /* <-down_read+0x8b/0x160 */
> > > 0) 0.754 us | }
> > >
> > > After:
> > > # perf ftrace -G vfs_write --graph-opts args,retaddr
> > > ......
> > > 0) | down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
> > > 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> > > 0) 0.079 us | preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
> > > 0) 0.077 us | preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
> > > 0) 0.754 us | }
> > >
> > > Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
> > > Cc: Sven Schnelle <svens@linux.ibm.com>
> > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
> >
> > Looks good to me except for a few nits, but it's a style issue.
> >
> > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Thank you for ack.
>
> >
> > Thank you,
> >
> > > ---
> > > v3:
> > > - Replace min() with min_t() to improve type safety and maintainability
> > > - Keep only one Signed-off-by for cleaner attribution
> > > - Code refactoring for improved readability
> > >
> > > v2:
> > > - Preserve retaddr event functionality (suggested by Steven)
> > > - Store the retaddr in args[0] (suggested by Steven)
> > > - Refactor implementation logic and commit message clarity
> > > ---
> > > include/linux/ftrace.h | 11 --
> > > kernel/trace/trace.h | 4 -
> > > kernel/trace/trace_entries.h | 6 +-
> > > kernel/trace/trace_functions_graph.c | 145 ++++++++++++---------------
> > > 4 files changed, 69 insertions(+), 97 deletions(-)
> > >
> > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > > index 7ded7df6e9b5..88cb54d73bdb 100644
> > > --- a/include/linux/ftrace.h
> > > +++ b/include/linux/ftrace.h
> > > @@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
> > > int depth;
> > > } __packed;
> > >
> > > -/*
> > > - * Structure that defines an entry function trace with retaddr.
> > > - * It's already packed but the attribute "packed" is needed
> > > - * to remove extra padding at the end.
> > > - */
> > > -struct fgraph_retaddr_ent {
> > > - unsigned long func; /* Current function */
> > > - int depth;
> > > - unsigned long retaddr; /* Return address */
> > > -} __packed;
> > > -
> > > /*
> > > * Structure that defines a return function trace.
> > > * It's already packed but the attribute "packed" is needed
> > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > > index 85eabb454bee..9fac291b913a 100644
> > > --- a/kernel/trace/trace.h
> > > +++ b/kernel/trace/trace.h
> > > @@ -955,10 +955,6 @@ extern void graph_trace_close(struct trace_iterator *iter);
> > > extern int __trace_graph_entry(struct trace_array *tr,
> > > struct ftrace_graph_ent *trace,
> > > unsigned int trace_ctx);
> > > -extern int __trace_graph_retaddr_entry(struct trace_array *tr,
> > > - struct ftrace_graph_ent *trace,
> > > - unsigned int trace_ctx,
> > > - unsigned long retaddr);
> > > extern void __trace_graph_return(struct trace_array *tr,
> > > struct ftrace_graph_ret *trace,
> > > unsigned int trace_ctx,
> > > diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> > > index de294ae2c5c5..593a74663c65 100644
> > > --- a/kernel/trace/trace_entries.h
> > > +++ b/kernel/trace/trace_entries.h
> > > @@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> > > TRACE_GRAPH_RETADDR_ENT,
> > >
> > > F_STRUCT(
> > > - __field_struct( struct fgraph_retaddr_ent, graph_ent )
> > > + __field_struct( struct ftrace_graph_ent, graph_ent )
> > > __field_packed( unsigned long, graph_ent, func )
> > > __field_packed( unsigned int, graph_ent, depth )
> > > - __field_packed( unsigned long, graph_ent, retaddr )
> > > + __dynamic_array(unsigned long, args )
> > > ),
> > >
> > > F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
> > > - (void *)__entry->retaddr)
> > > + (void *)__entry->args[0])
> > > );
> > >
> > > #else
> > > diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> > > index a7f4b9a47a71..f31eeeffbb2d 100644
> > > --- a/kernel/trace/trace_functions_graph.c
> > > +++ b/kernel/trace/trace_functions_graph.c
> > > @@ -16,6 +16,15 @@
> > > #include "trace.h"
> > > #include "trace_output.h"
> > >
> > > +#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > > +#define HAVE_RETADDR 1
> > > +#define ARGS_OFFS(args_size) \
> > > + ((args_size) > FTRACE_REGS_MAX_ARGS * sizeof(long) ? 1 : 0)
> > > +#else
> > > +#define HAVE_RETADDR 0
> > > +#define ARGS_OFFS(args_size) 0
> > > +#endif
> > > +
> > > /* When set, irq functions will be ignored */
> > > static int ftrace_graph_skip_irqs;
> > >
> > > @@ -27,21 +36,25 @@ struct fgraph_cpu_data {
> > > unsigned long enter_funcs[FTRACE_RETFUNC_DEPTH];
> > > };
> > >
> > > +/*
> > > + * fgraph_retaddr_ent_entry and ftrace_graph_ent_entry share layout, ent
> > > + * member repurposed for storage
> > > + */
> > > struct fgraph_ent_args {
> > > struct ftrace_graph_ent_entry ent;
> > > - /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
> > > - unsigned long args[FTRACE_REGS_MAX_ARGS];
> > > + /*
> > > + * Force the sizeof of args[] to have (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR)
> > > + * entries with the first entry storing the return address for
> > > + * TRACE_GRAPH_RETADDR_ENT.
> > > + */
> > > + unsigned long args[FTRACE_REGS_MAX_ARGS + HAVE_RETADDR];
> > > };
> > >
> > > struct fgraph_data {
> > > struct fgraph_cpu_data __percpu *cpu_data;
> > >
> > > /* Place to preserve last processed entry. */
> > > - union {
> > > - struct fgraph_ent_args ent;
> > > - /* TODO allow retaddr to have args */
> > > - struct fgraph_retaddr_ent_entry rent;
> > > - };
> > > + struct fgraph_ent_args ent;
> > > struct ftrace_graph_ret_entry ret;
> > > int failed;
> > > int cpu;
> > > @@ -127,22 +140,43 @@ static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
> > > struct ring_buffer_event *event;
> > > struct trace_buffer *buffer = tr->array_buffer.buffer;
> > > struct ftrace_graph_ent_entry *entry;
> > > - int size;
> > > + unsigned long retaddr = 0;
> > > + int size = sizeof(*entry);
> > > + int type = TRACE_GRAPH_ENT;
> > > + bool store_args = false;
> > > + int nr_args = 0, args_offs = 0;
> > > +
> > > + if (tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> > > + retaddr = ftrace_graph_top_ret_addr(current);
> > > + type = TRACE_GRAPH_RETADDR_ENT;
> > > + nr_args += 1;
> > > + }
> > >
> > > /* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
> > > - size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
> > > + if (tracer_flags_is_set(TRACE_GRAPH_ARGS)) {
> > > + store_args = !!fregs;
> > > + if (store_args)
> > > + nr_args += FTRACE_REGS_MAX_ARGS;
> > > + }
> > >
> > > - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size, trace_ctx);
> > > + size += nr_args * sizeof(long);
> > > + event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
> > > if (!event)
> > > return 0;
> > >
> > > entry = ring_buffer_event_data(event);
> > > entry->graph_ent = *trace;
> > >
> > > + /* Store the retaddr in args[0] */
> > > + if (type == TRACE_GRAPH_RETADDR_ENT) {
> > > + entry->args[0] = retaddr;
> > > + args_offs += 1;
> > > + }
> > > +
> > > #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
> > > - if (fregs) {
> > > + if (store_args) {
> > > for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
> > > - entry->args[i] = ftrace_regs_get_argument(fregs, i);
> > > + entry->args[i + args_offs] = ftrace_regs_get_argument(fregs, i);
> > > }
> > > #endif
> > >
> > > @@ -158,38 +192,6 @@ int __trace_graph_entry(struct trace_array *tr,
> > > return __graph_entry(tr, trace, trace_ctx, NULL);
> > > }
> > >
> > > -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > > -int __trace_graph_retaddr_entry(struct trace_array *tr,
> > > - struct ftrace_graph_ent *trace,
> > > - unsigned int trace_ctx,
> > > - unsigned long retaddr)
> > > -{
> > > - struct ring_buffer_event *event;
> > > - struct trace_buffer *buffer = tr->array_buffer.buffer;
> > > - struct fgraph_retaddr_ent_entry *entry;
> > > -
> > > - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
> > > - sizeof(*entry), trace_ctx);
> > > - if (!event)
> > > - return 0;
> > > - entry = ring_buffer_event_data(event);
> > > - entry->graph_ent.func = trace->func;
> > > - entry->graph_ent.depth = trace->depth;
> > > - entry->graph_ent.retaddr = retaddr;
> > > - trace_buffer_unlock_commit_nostack(buffer, event);
> > > -
> > > - return 1;
> > > -}
> > > -#else
> > > -int __trace_graph_retaddr_entry(struct trace_array *tr,
> > > - struct ftrace_graph_ent *trace,
> > > - unsigned int trace_ctx,
> > > - unsigned long retaddr)
> > > -{
> > > - return 1;
> > > -}
> > > -#endif
> > > -
> > > static inline int ftrace_graph_ignore_irqs(void)
> > > {
> > > if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
> > > @@ -211,7 +213,6 @@ static int graph_entry(struct ftrace_graph_ent *trace,
> > > struct trace_array *tr = gops->private;
> > > struct fgraph_times *ftimes;
> > > unsigned int trace_ctx;
> > > - int ret = 0;
> > >
> > > if (*task_var & TRACE_GRAPH_NOTRACE)
> > > return 0;
> > > @@ -262,15 +263,7 @@ static int graph_entry(struct ftrace_graph_ent *trace,
> > > return 1;
> > >
> > > trace_ctx = tracing_gen_ctx();
> > > - if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
> > > - tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> > > - unsigned long retaddr = ftrace_graph_top_ret_addr(current);
> > > - ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
> > > - } else {
> > > - ret = __graph_entry(tr, trace, trace_ctx, fregs);
> > > - }
> > > -
> > > - return ret;
> > > + return __graph_entry(tr, trace, trace_ctx, fregs);
> > > }
> > >
> > > int trace_graph_entry(struct ftrace_graph_ent *trace,
> > > @@ -634,13 +627,9 @@ get_return_for_leaf(struct trace_iterator *iter,
> > > * Save current and next entries for later reference
> > > * if the output fails.
> > > */
> > > - if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
> > > - data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
> > > - } else {
> > > - int size = min((int)sizeof(data->ent), (int)iter->ent_size);
> > > + int size = min_t(int, sizeof(data->ent), iter->ent_size);
> > >
> > > - memcpy(&data->ent, curr, size);
> > > - }
> > > + memcpy(&data->ent, curr, size);
> > > /*
> > > * If the next event is not a return type, then
> > > * we only care about what type it is. Otherwise we can
> > > @@ -811,21 +800,21 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
> > >
> > > #ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > > #define __TRACE_GRAPH_PRINT_RETADDR TRACE_GRAPH_PRINT_RETADDR
> > > -static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_entry *entry,
> > > - u32 trace_flags, bool comment)
> > > +static void print_graph_retaddr(struct trace_seq *s, unsigned long retaddr, u32 trace_flags,
> > > + bool comment)
> > > {
> > > if (comment)
> > > trace_seq_puts(s, " /*");
> > >
> > > trace_seq_puts(s, " <-");
> > > - seq_print_ip_sym(s, entry->graph_ent.retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
> > > + seq_print_ip_sym(s, retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
> > >
> > > if (comment)
> > > trace_seq_puts(s, " */");
> > > }
> > > #else
> > > #define __TRACE_GRAPH_PRINT_RETADDR 0
> > > -#define print_graph_retaddr(_seq, _entry, _tflags, _comment) do { } while (0)
> > > +#define print_graph_retaddr(_seq, _retaddr, _tflags, _comment) do { } while (0)
> > > #endif
> > >
> > > #if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
> > > @@ -869,10 +858,12 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> > > trace_seq_printf(s, "%ps", func);
> > >
> > > if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
> > > - print_function_args(s, entry->args, (unsigned long)func);
> > > + print_function_args(s, entry->args + ARGS_OFFS(args_size),
> > > + (unsigned long)func);
> > > trace_seq_putc(s, ';');
> > > - } else
> > > + } else {
> >
> > nit: you don't need to add braces for a single line block.
>
> Thanks. I added the else braces for consistency with the
> multi-statement if branch,
> per coding-style.rst. Happy to remove if you prefer the
> single-statement exception.
Ah, OK. I also checked this passed checkpatch.pl. (some other
macros are warned but those are false positive.)
Thanks,
>
> https://elixir.bootlin.com/linux/v6.18-rc5/source/Documentation/process/coding-style.rst#L213
>
> Do not unnecessarily use braces where a single statement will do.
>
> .. code-block:: c
>
> if (condition)
> action();
>
> and
>
> .. code-block:: c
>
> if (condition)
> do_this();
> else
> do_that();
>
> This does not apply if only one branch of a conditional statement is a single
> statement; in the latter case use braces in both branches:
>
> .. code-block:: c
>
> if (condition) {
> do_this();
> do_that();
> } else {
> otherwise();
> }
>
> >
> > > trace_seq_puts(s, "();");
> > > + }
> > >
> > > if (print_retval || print_retaddr)
> > > trace_seq_puts(s, " /*");
> > > @@ -882,8 +873,7 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> > > }
> > >
> > > if (print_retaddr)
> > > - print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
> > > - trace_flags, false);
> > > + print_graph_retaddr(s, entry->args[0], trace_flags, false);
> > >
> > > if (print_retval) {
> > > if (hex_format || (err_code == 0))
> > > @@ -964,10 +954,12 @@ print_graph_entry_leaf(struct trace_iterator *iter,
> > > trace_seq_printf(s, "%ps", (void *)ret_func);
> > >
> > > if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
> > > - print_function_args(s, entry->args, ret_func);
> > > + print_function_args(s, entry->args + ARGS_OFFS(args_size),
> > > + ret_func);
> > > trace_seq_putc(s, ';');
> > > - } else
> > > + } else {
> >
> > Ditto.
> >
> > > trace_seq_puts(s, "();");
> > > + }
> > > }
> > > trace_seq_putc(s, '\n');
> > >
> > > @@ -1016,7 +1008,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
> > > args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
> > >
> > > if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
> > > - print_function_args(s, entry->args, func);
> > > + print_function_args(s, entry->args + ARGS_OFFS(args_size), func);
> > > else
> > > trace_seq_puts(s, "()");
> > >
> > > @@ -1024,8 +1016,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
> > >
> > > if (flags & __TRACE_GRAPH_PRINT_RETADDR &&
> > > entry->ent.type == TRACE_GRAPH_RETADDR_ENT)
> > > - print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
> > > - tr->trace_flags, true);
> > > + print_graph_retaddr(s, entry->args[0], tr->trace_flags, true);
> > > trace_seq_putc(s, '\n');
> > >
> > > if (trace_seq_has_overflowed(s))
> > > @@ -1202,7 +1193,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
> > > * it can be safely saved at the stack.
> > > */
> > > struct ftrace_graph_ent_entry *entry;
> > > - u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
> > > + u8 save_buf[sizeof(*entry) + (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR) * sizeof(long)];
> > >
> > > /* The ent_size is expected to be as big as the entry */
> > > if (iter->ent_size > sizeof(save_buf))
> > > @@ -1429,16 +1420,12 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
> > > trace_assign_type(field, entry);
> > > return print_graph_entry(field, s, iter, flags);
> > > }
> > > -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > > case TRACE_GRAPH_RETADDR_ENT: {
> > > - struct fgraph_retaddr_ent_entry saved;
> > > struct fgraph_retaddr_ent_entry *rfield;
> > >
> > > trace_assign_type(rfield, entry);
> > > - saved = *rfield;
> > > - return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
> > > + return print_graph_entry((typeof(field))rfield, s, iter, flags);
> > > }
> > > -#endif
> > > case TRACE_GRAPH_RET: {
> > > struct ftrace_graph_ret_entry *field;
> > > trace_assign_type(field, entry);
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 RESEND] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-12 6:36 UTC (permalink / raw)
To: rostedt
Cc: linux-trace-kernel, linux-kernel, Donglin Peng, Sven Schnelle,
Masami Hiramatsu
In-Reply-To: <20251112034333.2901601-1-dolinux.peng@gmail.com>
On Wed, Nov 12, 2025 at 11:46 AM Donglin Peng <dolinux.peng@gmail.com> wrote:
>
> From: Donglin Peng <pengdonglin@xiaomi.com>
>
> Currently, the funcgraph-args and funcgraph-retaddr features are mutually exclusive.
> This patch resolves this limitation by modifying funcgraph-retaddr to adopt the same
> implementation approach as funcgraph-args, specifically by storing the return address
> in the first entry of the args array.
>
> As a result, both features now coexist seamlessly and function as intended.
>
> To verify the change, use perf to trace vfs_write with both options enabled:
>
> Before:
> # perf ftrace -G vfs_write --graph-opts args,retaddr
> ......
> 0) | down_read() { /* <-n_tty_write+0xa3/0x540 */
> 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> 0) 0.079 us | preempt_count_add(); /* <-down_read+0x3b/0x160 */
> 0) 0.077 us | preempt_count_sub(); /* <-down_read+0x8b/0x160 */
> 0) 0.754 us | }
>
> After:
> # perf ftrace -G vfs_write --graph-opts args,retaddr
> ......
> 0) | down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
> 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> 0) 0.079 us | preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
> 0) 0.077 us | preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
> 0) 0.754 us | }
>
> Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
> Cc: Sven Schnelle <svens@linux.ibm.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
> ---
> v3:
> - Replace min() with min_t() to improve type safety and maintainability
> - Keep only one Signed-off-by for cleaner attribution
> - Code refactoring for improved readability
>
> v2:
> - Preserve retaddr event functionality (suggested by Steven)
> - Store the retaddr in args[0] (suggested by Steven)
> - Refactor implementation logic and commit message clarity
> ---
> include/linux/ftrace.h | 11 --
> kernel/trace/trace.h | 4 -
> kernel/trace/trace_entries.h | 6 +-
> kernel/trace/trace_functions_graph.c | 145 ++++++++++++---------------
> 4 files changed, 69 insertions(+), 97 deletions(-)
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 7ded7df6e9b5..88cb54d73bdb 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
> int depth;
> } __packed;
>
> -/*
> - * Structure that defines an entry function trace with retaddr.
> - * It's already packed but the attribute "packed" is needed
> - * to remove extra padding at the end.
> - */
> -struct fgraph_retaddr_ent {
> - unsigned long func; /* Current function */
> - int depth;
> - unsigned long retaddr; /* Return address */
> -} __packed;
> -
> /*
> * Structure that defines a return function trace.
> * It's already packed but the attribute "packed" is needed
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 85eabb454bee..9fac291b913a 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -955,10 +955,6 @@ extern void graph_trace_close(struct trace_iterator *iter);
> extern int __trace_graph_entry(struct trace_array *tr,
> struct ftrace_graph_ent *trace,
> unsigned int trace_ctx);
> -extern int __trace_graph_retaddr_entry(struct trace_array *tr,
> - struct ftrace_graph_ent *trace,
> - unsigned int trace_ctx,
> - unsigned long retaddr);
> extern void __trace_graph_return(struct trace_array *tr,
> struct ftrace_graph_ret *trace,
> unsigned int trace_ctx,
> diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> index de294ae2c5c5..593a74663c65 100644
> --- a/kernel/trace/trace_entries.h
> +++ b/kernel/trace/trace_entries.h
> @@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> TRACE_GRAPH_RETADDR_ENT,
>
> F_STRUCT(
> - __field_struct( struct fgraph_retaddr_ent, graph_ent )
> + __field_struct( struct ftrace_graph_ent, graph_ent )
> __field_packed( unsigned long, graph_ent, func )
> __field_packed( unsigned int, graph_ent, depth )
> - __field_packed( unsigned long, graph_ent, retaddr )
> + __dynamic_array(unsigned long, args )
> ),
>
> F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
> - (void *)__entry->retaddr)
> + (void *)__entry->args[0])
> );
>
> #else
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index a7f4b9a47a71..f31eeeffbb2d 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -16,6 +16,15 @@
> #include "trace.h"
> #include "trace_output.h"
>
> +#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> +#define HAVE_RETADDR 1
> +#define ARGS_OFFS(args_size) \
> + ((args_size) > FTRACE_REGS_MAX_ARGS * sizeof(long) ? 1 : 0)
> +#else
> +#define HAVE_RETADDR 0
> +#define ARGS_OFFS(args_size) 0
> +#endif
> +
> /* When set, irq functions will be ignored */
> static int ftrace_graph_skip_irqs;
>
> @@ -27,21 +36,25 @@ struct fgraph_cpu_data {
> unsigned long enter_funcs[FTRACE_RETFUNC_DEPTH];
> };
>
> +/*
> + * fgraph_retaddr_ent_entry and ftrace_graph_ent_entry share layout, ent
> + * member repurposed for storage
> + */
> struct fgraph_ent_args {
> struct ftrace_graph_ent_entry ent;
> - /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
> - unsigned long args[FTRACE_REGS_MAX_ARGS];
> + /*
> + * Force the sizeof of args[] to have (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR)
> + * entries with the first entry storing the return address for
> + * TRACE_GRAPH_RETADDR_ENT.
> + */
> + unsigned long args[FTRACE_REGS_MAX_ARGS + HAVE_RETADDR];
> };
>
> struct fgraph_data {
> struct fgraph_cpu_data __percpu *cpu_data;
>
> /* Place to preserve last processed entry. */
> - union {
> - struct fgraph_ent_args ent;
> - /* TODO allow retaddr to have args */
> - struct fgraph_retaddr_ent_entry rent;
> - };
> + struct fgraph_ent_args ent;
> struct ftrace_graph_ret_entry ret;
> int failed;
> int cpu;
> @@ -127,22 +140,43 @@ static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
> struct ring_buffer_event *event;
> struct trace_buffer *buffer = tr->array_buffer.buffer;
> struct ftrace_graph_ent_entry *entry;
> - int size;
> + unsigned long retaddr = 0;
Remove the unnecessary initialization of retaddr. It will be assigned a proper
value before use. Will fix it in the next version.
> + int size = sizeof(*entry);
> + int type = TRACE_GRAPH_ENT;
> + bool store_args = false;
> + int nr_args = 0, args_offs = 0;
> +
> + if (tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> + retaddr = ftrace_graph_top_ret_addr(current);
> + type = TRACE_GRAPH_RETADDR_ENT;
> + nr_args += 1;
> + }
>
> /* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
> - size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
> + if (tracer_flags_is_set(TRACE_GRAPH_ARGS)) {
> + store_args = !!fregs;
> + if (store_args)
> + nr_args += FTRACE_REGS_MAX_ARGS;
> + }
>
> - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size, trace_ctx);
> + size += nr_args * sizeof(long);
> + event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
> if (!event)
> return 0;
>
> entry = ring_buffer_event_data(event);
> entry->graph_ent = *trace;
>
> + /* Store the retaddr in args[0] */
> + if (type == TRACE_GRAPH_RETADDR_ENT) {
> + entry->args[0] = retaddr;
> + args_offs += 1;
> + }
> +
> #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
> - if (fregs) {
> + if (store_args) {
> for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
> - entry->args[i] = ftrace_regs_get_argument(fregs, i);
> + entry->args[i + args_offs] = ftrace_regs_get_argument(fregs, i);
> }
> #endif
>
> @@ -158,38 +192,6 @@ int __trace_graph_entry(struct trace_array *tr,
> return __graph_entry(tr, trace, trace_ctx, NULL);
> }
>
> -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> -int __trace_graph_retaddr_entry(struct trace_array *tr,
> - struct ftrace_graph_ent *trace,
> - unsigned int trace_ctx,
> - unsigned long retaddr)
> -{
> - struct ring_buffer_event *event;
> - struct trace_buffer *buffer = tr->array_buffer.buffer;
> - struct fgraph_retaddr_ent_entry *entry;
> -
> - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
> - sizeof(*entry), trace_ctx);
> - if (!event)
> - return 0;
> - entry = ring_buffer_event_data(event);
> - entry->graph_ent.func = trace->func;
> - entry->graph_ent.depth = trace->depth;
> - entry->graph_ent.retaddr = retaddr;
> - trace_buffer_unlock_commit_nostack(buffer, event);
> -
> - return 1;
> -}
> -#else
> -int __trace_graph_retaddr_entry(struct trace_array *tr,
> - struct ftrace_graph_ent *trace,
> - unsigned int trace_ctx,
> - unsigned long retaddr)
> -{
> - return 1;
> -}
> -#endif
> -
> static inline int ftrace_graph_ignore_irqs(void)
> {
> if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
> @@ -211,7 +213,6 @@ static int graph_entry(struct ftrace_graph_ent *trace,
> struct trace_array *tr = gops->private;
> struct fgraph_times *ftimes;
> unsigned int trace_ctx;
> - int ret = 0;
>
> if (*task_var & TRACE_GRAPH_NOTRACE)
> return 0;
> @@ -262,15 +263,7 @@ static int graph_entry(struct ftrace_graph_ent *trace,
> return 1;
>
> trace_ctx = tracing_gen_ctx();
> - if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
> - tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> - unsigned long retaddr = ftrace_graph_top_ret_addr(current);
> - ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
> - } else {
> - ret = __graph_entry(tr, trace, trace_ctx, fregs);
> - }
> -
> - return ret;
> + return __graph_entry(tr, trace, trace_ctx, fregs);
> }
>
> int trace_graph_entry(struct ftrace_graph_ent *trace,
> @@ -634,13 +627,9 @@ get_return_for_leaf(struct trace_iterator *iter,
> * Save current and next entries for later reference
> * if the output fails.
> */
> - if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
> - data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
> - } else {
> - int size = min((int)sizeof(data->ent), (int)iter->ent_size);
> + int size = min_t(int, sizeof(data->ent), iter->ent_size);
>
> - memcpy(&data->ent, curr, size);
> - }
> + memcpy(&data->ent, curr, size);
> /*
> * If the next event is not a return type, then
> * we only care about what type it is. Otherwise we can
> @@ -811,21 +800,21 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
>
> #ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> #define __TRACE_GRAPH_PRINT_RETADDR TRACE_GRAPH_PRINT_RETADDR
> -static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_entry *entry,
> - u32 trace_flags, bool comment)
> +static void print_graph_retaddr(struct trace_seq *s, unsigned long retaddr, u32 trace_flags,
> + bool comment)
> {
> if (comment)
> trace_seq_puts(s, " /*");
>
> trace_seq_puts(s, " <-");
> - seq_print_ip_sym(s, entry->graph_ent.retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
> + seq_print_ip_sym(s, retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
>
> if (comment)
> trace_seq_puts(s, " */");
> }
> #else
> #define __TRACE_GRAPH_PRINT_RETADDR 0
> -#define print_graph_retaddr(_seq, _entry, _tflags, _comment) do { } while (0)
> +#define print_graph_retaddr(_seq, _retaddr, _tflags, _comment) do { } while (0)
> #endif
>
> #if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
> @@ -869,10 +858,12 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> trace_seq_printf(s, "%ps", func);
>
> if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
> - print_function_args(s, entry->args, (unsigned long)func);
> + print_function_args(s, entry->args + ARGS_OFFS(args_size),
> + (unsigned long)func);
> trace_seq_putc(s, ';');
> - } else
> + } else {
> trace_seq_puts(s, "();");
> + }
>
> if (print_retval || print_retaddr)
> trace_seq_puts(s, " /*");
> @@ -882,8 +873,7 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> }
>
> if (print_retaddr)
> - print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
> - trace_flags, false);
> + print_graph_retaddr(s, entry->args[0], trace_flags, false);
>
> if (print_retval) {
> if (hex_format || (err_code == 0))
> @@ -964,10 +954,12 @@ print_graph_entry_leaf(struct trace_iterator *iter,
> trace_seq_printf(s, "%ps", (void *)ret_func);
>
> if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
> - print_function_args(s, entry->args, ret_func);
> + print_function_args(s, entry->args + ARGS_OFFS(args_size),
> + ret_func);
> trace_seq_putc(s, ';');
> - } else
> + } else {
> trace_seq_puts(s, "();");
> + }
> }
> trace_seq_putc(s, '\n');
>
> @@ -1016,7 +1008,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
> args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
>
> if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
> - print_function_args(s, entry->args, func);
> + print_function_args(s, entry->args + ARGS_OFFS(args_size), func);
> else
> trace_seq_puts(s, "()");
>
> @@ -1024,8 +1016,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
>
> if (flags & __TRACE_GRAPH_PRINT_RETADDR &&
> entry->ent.type == TRACE_GRAPH_RETADDR_ENT)
> - print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
> - tr->trace_flags, true);
> + print_graph_retaddr(s, entry->args[0], tr->trace_flags, true);
> trace_seq_putc(s, '\n');
>
> if (trace_seq_has_overflowed(s))
> @@ -1202,7 +1193,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
> * it can be safely saved at the stack.
> */
> struct ftrace_graph_ent_entry *entry;
> - u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
> + u8 save_buf[sizeof(*entry) + (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR) * sizeof(long)];
>
> /* The ent_size is expected to be as big as the entry */
> if (iter->ent_size > sizeof(save_buf))
> @@ -1429,16 +1420,12 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
> trace_assign_type(field, entry);
> return print_graph_entry(field, s, iter, flags);
> }
> -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> case TRACE_GRAPH_RETADDR_ENT: {
> - struct fgraph_retaddr_ent_entry saved;
> struct fgraph_retaddr_ent_entry *rfield;
>
> trace_assign_type(rfield, entry);
> - saved = *rfield;
> - return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
> + return print_graph_entry((typeof(field))rfield, s, iter, flags);
> }
> -#endif
> case TRACE_GRAPH_RET: {
> struct ftrace_graph_ret_entry *field;
> trace_assign_type(field, entry);
> --
> 2.34.1
>
^ permalink raw reply
* Re: [PATCH v3 RESEND] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-12 6:03 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: rostedt, linux-trace-kernel, linux-kernel, Donglin Peng,
Sven Schnelle
In-Reply-To: <20251112141705.a7f2f79f6d7b058201307e89@kernel.org>
On Wed, Nov 12, 2025 at 1:17 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Wed, 12 Nov 2025 11:43:33 +0800
> Donglin Peng <dolinux.peng@gmail.com> wrote:
>
> > From: Donglin Peng <pengdonglin@xiaomi.com>
> >
> > Currently, the funcgraph-args and funcgraph-retaddr features are mutually exclusive.
> > This patch resolves this limitation by modifying funcgraph-retaddr to adopt the same
> > implementation approach as funcgraph-args, specifically by storing the return address
> > in the first entry of the args array.
> >
> > As a result, both features now coexist seamlessly and function as intended.
> >
> > To verify the change, use perf to trace vfs_write with both options enabled:
> >
> > Before:
> > # perf ftrace -G vfs_write --graph-opts args,retaddr
> > ......
> > 0) | down_read() { /* <-n_tty_write+0xa3/0x540 */
> > 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> > 0) 0.079 us | preempt_count_add(); /* <-down_read+0x3b/0x160 */
> > 0) 0.077 us | preempt_count_sub(); /* <-down_read+0x8b/0x160 */
> > 0) 0.754 us | }
> >
> > After:
> > # perf ftrace -G vfs_write --graph-opts args,retaddr
> > ......
> > 0) | down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
> > 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> > 0) 0.079 us | preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
> > 0) 0.077 us | preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
> > 0) 0.754 us | }
> >
> > Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
> > Cc: Sven Schnelle <svens@linux.ibm.com>
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
>
> Looks good to me except for a few nits, but it's a style issue.
>
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Thank you for ack.
>
> Thank you,
>
> > ---
> > v3:
> > - Replace min() with min_t() to improve type safety and maintainability
> > - Keep only one Signed-off-by for cleaner attribution
> > - Code refactoring for improved readability
> >
> > v2:
> > - Preserve retaddr event functionality (suggested by Steven)
> > - Store the retaddr in args[0] (suggested by Steven)
> > - Refactor implementation logic and commit message clarity
> > ---
> > include/linux/ftrace.h | 11 --
> > kernel/trace/trace.h | 4 -
> > kernel/trace/trace_entries.h | 6 +-
> > kernel/trace/trace_functions_graph.c | 145 ++++++++++++---------------
> > 4 files changed, 69 insertions(+), 97 deletions(-)
> >
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 7ded7df6e9b5..88cb54d73bdb 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
> > int depth;
> > } __packed;
> >
> > -/*
> > - * Structure that defines an entry function trace with retaddr.
> > - * It's already packed but the attribute "packed" is needed
> > - * to remove extra padding at the end.
> > - */
> > -struct fgraph_retaddr_ent {
> > - unsigned long func; /* Current function */
> > - int depth;
> > - unsigned long retaddr; /* Return address */
> > -} __packed;
> > -
> > /*
> > * Structure that defines a return function trace.
> > * It's already packed but the attribute "packed" is needed
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 85eabb454bee..9fac291b913a 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -955,10 +955,6 @@ extern void graph_trace_close(struct trace_iterator *iter);
> > extern int __trace_graph_entry(struct trace_array *tr,
> > struct ftrace_graph_ent *trace,
> > unsigned int trace_ctx);
> > -extern int __trace_graph_retaddr_entry(struct trace_array *tr,
> > - struct ftrace_graph_ent *trace,
> > - unsigned int trace_ctx,
> > - unsigned long retaddr);
> > extern void __trace_graph_return(struct trace_array *tr,
> > struct ftrace_graph_ret *trace,
> > unsigned int trace_ctx,
> > diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> > index de294ae2c5c5..593a74663c65 100644
> > --- a/kernel/trace/trace_entries.h
> > +++ b/kernel/trace/trace_entries.h
> > @@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> > TRACE_GRAPH_RETADDR_ENT,
> >
> > F_STRUCT(
> > - __field_struct( struct fgraph_retaddr_ent, graph_ent )
> > + __field_struct( struct ftrace_graph_ent, graph_ent )
> > __field_packed( unsigned long, graph_ent, func )
> > __field_packed( unsigned int, graph_ent, depth )
> > - __field_packed( unsigned long, graph_ent, retaddr )
> > + __dynamic_array(unsigned long, args )
> > ),
> >
> > F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
> > - (void *)__entry->retaddr)
> > + (void *)__entry->args[0])
> > );
> >
> > #else
> > diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> > index a7f4b9a47a71..f31eeeffbb2d 100644
> > --- a/kernel/trace/trace_functions_graph.c
> > +++ b/kernel/trace/trace_functions_graph.c
> > @@ -16,6 +16,15 @@
> > #include "trace.h"
> > #include "trace_output.h"
> >
> > +#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > +#define HAVE_RETADDR 1
> > +#define ARGS_OFFS(args_size) \
> > + ((args_size) > FTRACE_REGS_MAX_ARGS * sizeof(long) ? 1 : 0)
> > +#else
> > +#define HAVE_RETADDR 0
> > +#define ARGS_OFFS(args_size) 0
> > +#endif
> > +
> > /* When set, irq functions will be ignored */
> > static int ftrace_graph_skip_irqs;
> >
> > @@ -27,21 +36,25 @@ struct fgraph_cpu_data {
> > unsigned long enter_funcs[FTRACE_RETFUNC_DEPTH];
> > };
> >
> > +/*
> > + * fgraph_retaddr_ent_entry and ftrace_graph_ent_entry share layout, ent
> > + * member repurposed for storage
> > + */
> > struct fgraph_ent_args {
> > struct ftrace_graph_ent_entry ent;
> > - /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
> > - unsigned long args[FTRACE_REGS_MAX_ARGS];
> > + /*
> > + * Force the sizeof of args[] to have (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR)
> > + * entries with the first entry storing the return address for
> > + * TRACE_GRAPH_RETADDR_ENT.
> > + */
> > + unsigned long args[FTRACE_REGS_MAX_ARGS + HAVE_RETADDR];
> > };
> >
> > struct fgraph_data {
> > struct fgraph_cpu_data __percpu *cpu_data;
> >
> > /* Place to preserve last processed entry. */
> > - union {
> > - struct fgraph_ent_args ent;
> > - /* TODO allow retaddr to have args */
> > - struct fgraph_retaddr_ent_entry rent;
> > - };
> > + struct fgraph_ent_args ent;
> > struct ftrace_graph_ret_entry ret;
> > int failed;
> > int cpu;
> > @@ -127,22 +140,43 @@ static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
> > struct ring_buffer_event *event;
> > struct trace_buffer *buffer = tr->array_buffer.buffer;
> > struct ftrace_graph_ent_entry *entry;
> > - int size;
> > + unsigned long retaddr = 0;
> > + int size = sizeof(*entry);
> > + int type = TRACE_GRAPH_ENT;
> > + bool store_args = false;
> > + int nr_args = 0, args_offs = 0;
> > +
> > + if (tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> > + retaddr = ftrace_graph_top_ret_addr(current);
> > + type = TRACE_GRAPH_RETADDR_ENT;
> > + nr_args += 1;
> > + }
> >
> > /* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
> > - size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
> > + if (tracer_flags_is_set(TRACE_GRAPH_ARGS)) {
> > + store_args = !!fregs;
> > + if (store_args)
> > + nr_args += FTRACE_REGS_MAX_ARGS;
> > + }
> >
> > - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size, trace_ctx);
> > + size += nr_args * sizeof(long);
> > + event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
> > if (!event)
> > return 0;
> >
> > entry = ring_buffer_event_data(event);
> > entry->graph_ent = *trace;
> >
> > + /* Store the retaddr in args[0] */
> > + if (type == TRACE_GRAPH_RETADDR_ENT) {
> > + entry->args[0] = retaddr;
> > + args_offs += 1;
> > + }
> > +
> > #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
> > - if (fregs) {
> > + if (store_args) {
> > for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
> > - entry->args[i] = ftrace_regs_get_argument(fregs, i);
> > + entry->args[i + args_offs] = ftrace_regs_get_argument(fregs, i);
> > }
> > #endif
> >
> > @@ -158,38 +192,6 @@ int __trace_graph_entry(struct trace_array *tr,
> > return __graph_entry(tr, trace, trace_ctx, NULL);
> > }
> >
> > -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > -int __trace_graph_retaddr_entry(struct trace_array *tr,
> > - struct ftrace_graph_ent *trace,
> > - unsigned int trace_ctx,
> > - unsigned long retaddr)
> > -{
> > - struct ring_buffer_event *event;
> > - struct trace_buffer *buffer = tr->array_buffer.buffer;
> > - struct fgraph_retaddr_ent_entry *entry;
> > -
> > - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
> > - sizeof(*entry), trace_ctx);
> > - if (!event)
> > - return 0;
> > - entry = ring_buffer_event_data(event);
> > - entry->graph_ent.func = trace->func;
> > - entry->graph_ent.depth = trace->depth;
> > - entry->graph_ent.retaddr = retaddr;
> > - trace_buffer_unlock_commit_nostack(buffer, event);
> > -
> > - return 1;
> > -}
> > -#else
> > -int __trace_graph_retaddr_entry(struct trace_array *tr,
> > - struct ftrace_graph_ent *trace,
> > - unsigned int trace_ctx,
> > - unsigned long retaddr)
> > -{
> > - return 1;
> > -}
> > -#endif
> > -
> > static inline int ftrace_graph_ignore_irqs(void)
> > {
> > if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
> > @@ -211,7 +213,6 @@ static int graph_entry(struct ftrace_graph_ent *trace,
> > struct trace_array *tr = gops->private;
> > struct fgraph_times *ftimes;
> > unsigned int trace_ctx;
> > - int ret = 0;
> >
> > if (*task_var & TRACE_GRAPH_NOTRACE)
> > return 0;
> > @@ -262,15 +263,7 @@ static int graph_entry(struct ftrace_graph_ent *trace,
> > return 1;
> >
> > trace_ctx = tracing_gen_ctx();
> > - if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
> > - tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> > - unsigned long retaddr = ftrace_graph_top_ret_addr(current);
> > - ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
> > - } else {
> > - ret = __graph_entry(tr, trace, trace_ctx, fregs);
> > - }
> > -
> > - return ret;
> > + return __graph_entry(tr, trace, trace_ctx, fregs);
> > }
> >
> > int trace_graph_entry(struct ftrace_graph_ent *trace,
> > @@ -634,13 +627,9 @@ get_return_for_leaf(struct trace_iterator *iter,
> > * Save current and next entries for later reference
> > * if the output fails.
> > */
> > - if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
> > - data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
> > - } else {
> > - int size = min((int)sizeof(data->ent), (int)iter->ent_size);
> > + int size = min_t(int, sizeof(data->ent), iter->ent_size);
> >
> > - memcpy(&data->ent, curr, size);
> > - }
> > + memcpy(&data->ent, curr, size);
> > /*
> > * If the next event is not a return type, then
> > * we only care about what type it is. Otherwise we can
> > @@ -811,21 +800,21 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
> >
> > #ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > #define __TRACE_GRAPH_PRINT_RETADDR TRACE_GRAPH_PRINT_RETADDR
> > -static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_entry *entry,
> > - u32 trace_flags, bool comment)
> > +static void print_graph_retaddr(struct trace_seq *s, unsigned long retaddr, u32 trace_flags,
> > + bool comment)
> > {
> > if (comment)
> > trace_seq_puts(s, " /*");
> >
> > trace_seq_puts(s, " <-");
> > - seq_print_ip_sym(s, entry->graph_ent.retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
> > + seq_print_ip_sym(s, retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
> >
> > if (comment)
> > trace_seq_puts(s, " */");
> > }
> > #else
> > #define __TRACE_GRAPH_PRINT_RETADDR 0
> > -#define print_graph_retaddr(_seq, _entry, _tflags, _comment) do { } while (0)
> > +#define print_graph_retaddr(_seq, _retaddr, _tflags, _comment) do { } while (0)
> > #endif
> >
> > #if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
> > @@ -869,10 +858,12 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> > trace_seq_printf(s, "%ps", func);
> >
> > if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
> > - print_function_args(s, entry->args, (unsigned long)func);
> > + print_function_args(s, entry->args + ARGS_OFFS(args_size),
> > + (unsigned long)func);
> > trace_seq_putc(s, ';');
> > - } else
> > + } else {
>
> nit: you don't need to add braces for a single line block.
Thanks. I added the else braces for consistency with the
multi-statement if branch,
per coding-style.rst. Happy to remove if you prefer the
single-statement exception.
https://elixir.bootlin.com/linux/v6.18-rc5/source/Documentation/process/coding-style.rst#L213
Do not unnecessarily use braces where a single statement will do.
.. code-block:: c
if (condition)
action();
and
.. code-block:: c
if (condition)
do_this();
else
do_that();
This does not apply if only one branch of a conditional statement is a single
statement; in the latter case use braces in both branches:
.. code-block:: c
if (condition) {
do_this();
do_that();
} else {
otherwise();
}
>
> > trace_seq_puts(s, "();");
> > + }
> >
> > if (print_retval || print_retaddr)
> > trace_seq_puts(s, " /*");
> > @@ -882,8 +873,7 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> > }
> >
> > if (print_retaddr)
> > - print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
> > - trace_flags, false);
> > + print_graph_retaddr(s, entry->args[0], trace_flags, false);
> >
> > if (print_retval) {
> > if (hex_format || (err_code == 0))
> > @@ -964,10 +954,12 @@ print_graph_entry_leaf(struct trace_iterator *iter,
> > trace_seq_printf(s, "%ps", (void *)ret_func);
> >
> > if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
> > - print_function_args(s, entry->args, ret_func);
> > + print_function_args(s, entry->args + ARGS_OFFS(args_size),
> > + ret_func);
> > trace_seq_putc(s, ';');
> > - } else
> > + } else {
>
> Ditto.
>
> > trace_seq_puts(s, "();");
> > + }
> > }
> > trace_seq_putc(s, '\n');
> >
> > @@ -1016,7 +1008,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
> > args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
> >
> > if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
> > - print_function_args(s, entry->args, func);
> > + print_function_args(s, entry->args + ARGS_OFFS(args_size), func);
> > else
> > trace_seq_puts(s, "()");
> >
> > @@ -1024,8 +1016,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
> >
> > if (flags & __TRACE_GRAPH_PRINT_RETADDR &&
> > entry->ent.type == TRACE_GRAPH_RETADDR_ENT)
> > - print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
> > - tr->trace_flags, true);
> > + print_graph_retaddr(s, entry->args[0], tr->trace_flags, true);
> > trace_seq_putc(s, '\n');
> >
> > if (trace_seq_has_overflowed(s))
> > @@ -1202,7 +1193,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
> > * it can be safely saved at the stack.
> > */
> > struct ftrace_graph_ent_entry *entry;
> > - u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
> > + u8 save_buf[sizeof(*entry) + (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR) * sizeof(long)];
> >
> > /* The ent_size is expected to be as big as the entry */
> > if (iter->ent_size > sizeof(save_buf))
> > @@ -1429,16 +1420,12 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
> > trace_assign_type(field, entry);
> > return print_graph_entry(field, s, iter, flags);
> > }
> > -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > case TRACE_GRAPH_RETADDR_ENT: {
> > - struct fgraph_retaddr_ent_entry saved;
> > struct fgraph_retaddr_ent_entry *rfield;
> >
> > trace_assign_type(rfield, entry);
> > - saved = *rfield;
> > - return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
> > + return print_graph_entry((typeof(field))rfield, s, iter, flags);
> > }
> > -#endif
> > case TRACE_GRAPH_RET: {
> > struct ftrace_graph_ret_entry *field;
> > trace_assign_type(field, entry);
> > --
> > 2.34.1
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH] trace/pid_list: optimize pid_list->lock contention
From: Yongliang Gao @ 2025-11-12 5:27 UTC (permalink / raw)
To: Steven Rostedt
Cc: Sebastian Andrzej Siewior, mhiramat, mathieu.desnoyers,
linux-kernel, linux-trace-kernel, Yongliang Gao, Huang Cun,
frankjpliu
In-Reply-To: <20251111102739.2a0a64cf@gandalf.local.home>
Hi Steven,
Thank you for your detailed response and the proposed RCU-like approach.
I've looked into using a regular seqlock instead of the current
implementation, but as you pointed out, the write side is indeed a
critical path. More importantly, I found that even with seqlock, the
write_seqlock() function internally uses spin_lock() which on
PREEMPT_RT gets converted to an mutex. This would cause the same
issues we're trying to avoid - potential sleep in atomic contexts.
bool trace_pid_list_is_set(struct trace_pid_list *pid_list, unsigned int pid)
{
union upper_chunk *upper_chunk;
union lower_chunk *lower_chunk;
unsigned int seq;
unsigned long flags;
unsigned int upper1;
unsigned int upper2;
unsigned int lower;
bool ret = false;
if (!pid_list)
return false;
if (pid_split(pid, &upper1, &upper2, &lower) < 0)
return false;
do {
local_irq_save(flags);
seq = read_seqbegin(&pid_list->lock);
ret = false;
upper_chunk = pid_list->upper[upper1];
if (upper_chunk) {
lower_chunk = upper_chunk->data[upper2];
if (lower_chunk)
ret = test_bit(lower, lower_chunk->data);
}
local_irq_restore(flags);
} while (read_seqretry(&pid_list->lock, seq));
return ret;
}
BUG: sleeping function called from invalid context at
kernel/locking/spinlock_rt.c:48
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 192, name: bash
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
CPU: 3 UID: 0 PID: 192 Comm: bash Not tainted 6.18.0-rc5+ #84 PREEMPT_{RT,LAZY}
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
Call Trace:
<TASK>
dump_stack_lvl+0x4f/0x70
__might_resched+0x113/0x160
rt_spin_lock+0x41/0x130
trace_pid_list_set+0x52/0x150
ftrace_pid_follow_sched_process_fork+0x19/0x30
kernel_clone+0x1b8/0x3e0
__do_sys_clone+0x65/0x90
do_syscall_64+0x48/0xa60
entry_SYSCALL_64_after_hwframe+0x76/0x7e
Your proposed solution using atomic counters and memory barriers
should provide the lock-free read path we need while maintaining code
correctness. However, to achieve this correctly requires careful
consideration of all memory ordering scenarios, and I'm concerned
about introducing subtle bugs given the complexity.
Would you be willing to submit a patch implementing your approach?
On Tue, Nov 11, 2025 at 11:27 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 11 Nov 2025 09:13:14 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>
> > Nope, no read-write lock that can be used in atomic sections. Well,
> > there is RCU.
>
> Well, it can't simply be replaced by RCU as the write side is also a
> critical path. It happens when new tasks are spawned.
>
> Now we could possibly do some RCU like magic, and remove the lock in the
> read, but it would need some care with the writes.
>
> Something like this (untested):
>
> bool trace_pid_list_is_set(struct trace_pid_list *pid_list, unsigned int pid)
> {
> union upper_chunk *upper_chunk;
> union lower_chunk *lower_chunk;
> unsigned long flags;
> unsigned int upper1;
> unsigned int upper2;
> unsigned int lower;
> bool ret = false;
>
> if (!pid_list)
> return false;
>
> if (pid_split(pid, &upper1, &upper2, &lower) < 0)
> return false;
>
> upper_chunk = READ_ONCE(pid_list->upper[upper1]);
> if (upper_chunk) {
> lower_chunk = READ_ONCE(upper_chunk->data[upper2]);
> if (lower_chunk)
> ret = test_bit(lower, lower_chunk->data);
> }
>
> return ret;
> }
>
> Now when all the bits of a chunk is cleared, it goes to a free-list. And
> when a new chunk is needed, it acquires it from that free-list. We need to
> make sure that the chunk acquired in the read hasn't gone through the
> free-list.
>
> Now we could have an atomic counter in the pid_list and make this more of a
> seqcount? That is, have the counter updated when a chunk goes to the free
> list and also when it is taken from the free list. We could then make this:
>
> again:
> counter = atomic_read(&pid_list->counter);
> smp_rmb();
> upper_chunk = READ_ONCE(pid_list->upper[upper1]);
> if (upper_chunk) {
> lower_chunk = READ_ONCE(upper_chunk->data[upper2]);
> if (lower_chunk) {
> ret = test_bit(lower, lower_chunk->data);
> smp_rmb();
> if (unlikely(counter != atomic_read(&pid_list->counter))) {
> ret = false;
> goto again;
> }
> }
> }
>
>
> And in the set we need:
>
> upper_chunk = pid_list->upper[upper1];
> if (!upper_chunk) {
> upper_chunk = get_upper_chunk(pid_list);
> if (!upper_chunk) {
> ret = -ENOMEM;
> goto out;
> }
> atomic_inc(&pid_list->counter);
> smp_wmb();
> WRITE_ONCE(pid_list->upper[upper1], upper_chunk);
> }
> lower_chunk = upper_chunk->data[upper2];
> if (!lower_chunk) {
> lower_chunk = get_lower_chunk(pid_list);
> if (!lower_chunk) {
> ret = -ENOMEM;
> goto out;
> }
> atomic_inc(&pid_list->counter);
> smp_wmb();
> WRITE_ONCE(upper_chunk->data[upper2], lower_chunk);
> }
>
> and in the clear:
>
> if (find_first_bit(lower_chunk->data, LOWER_MAX) >= LOWER_MAX) {
> put_lower_chunk(pid_list, lower_chunk);
> WRITE_ONCE(upper_chunk->data[upper2], NULL);
> smp_wmb();
> atomic_inc(&pid_list->counter);
> if (upper_empty(upper_chunk)) {
> put_upper_chunk(pid_list, upper_chunk);
> WRITE_ONCE(pid_list->upper[upper1], NULL);
> smp_wmb();
> atomic_inc(&pid_list->counter);
> }
> }
>
> That is, the counter gets updated after setting the chunk to NULL and
> before assigning it a new value. And reading it, the counter is read before
> looking at any of the chunks, and tested after getting the result. If the
> value is the same, then the chunks are for the correct PID and haven't
> swapped in a free/alloc swap where it's looking at a chunk for a different
> PID.
>
> This would allow for the read to not take any locks.
>
> -- Steve
^ permalink raw reply
* Re: [PATCH v3 RESEND] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Masami Hiramatsu @ 2025-11-12 5:17 UTC (permalink / raw)
To: Donglin Peng
Cc: rostedt, linux-trace-kernel, linux-kernel, Donglin Peng,
Sven Schnelle, Masami Hiramatsu
In-Reply-To: <20251112034333.2901601-1-dolinux.peng@gmail.com>
On Wed, 12 Nov 2025 11:43:33 +0800
Donglin Peng <dolinux.peng@gmail.com> wrote:
> From: Donglin Peng <pengdonglin@xiaomi.com>
>
> Currently, the funcgraph-args and funcgraph-retaddr features are mutually exclusive.
> This patch resolves this limitation by modifying funcgraph-retaddr to adopt the same
> implementation approach as funcgraph-args, specifically by storing the return address
> in the first entry of the args array.
>
> As a result, both features now coexist seamlessly and function as intended.
>
> To verify the change, use perf to trace vfs_write with both options enabled:
>
> Before:
> # perf ftrace -G vfs_write --graph-opts args,retaddr
> ......
> 0) | down_read() { /* <-n_tty_write+0xa3/0x540 */
> 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> 0) 0.079 us | preempt_count_add(); /* <-down_read+0x3b/0x160 */
> 0) 0.077 us | preempt_count_sub(); /* <-down_read+0x8b/0x160 */
> 0) 0.754 us | }
>
> After:
> # perf ftrace -G vfs_write --graph-opts args,retaddr
> ......
> 0) | down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
> 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> 0) 0.079 us | preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
> 0) 0.077 us | preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
> 0) 0.754 us | }
>
> Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
> Cc: Sven Schnelle <svens@linux.ibm.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
Looks good to me except for a few nits, but it's a style issue.
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Thank you,
> ---
> v3:
> - Replace min() with min_t() to improve type safety and maintainability
> - Keep only one Signed-off-by for cleaner attribution
> - Code refactoring for improved readability
>
> v2:
> - Preserve retaddr event functionality (suggested by Steven)
> - Store the retaddr in args[0] (suggested by Steven)
> - Refactor implementation logic and commit message clarity
> ---
> include/linux/ftrace.h | 11 --
> kernel/trace/trace.h | 4 -
> kernel/trace/trace_entries.h | 6 +-
> kernel/trace/trace_functions_graph.c | 145 ++++++++++++---------------
> 4 files changed, 69 insertions(+), 97 deletions(-)
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 7ded7df6e9b5..88cb54d73bdb 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
> int depth;
> } __packed;
>
> -/*
> - * Structure that defines an entry function trace with retaddr.
> - * It's already packed but the attribute "packed" is needed
> - * to remove extra padding at the end.
> - */
> -struct fgraph_retaddr_ent {
> - unsigned long func; /* Current function */
> - int depth;
> - unsigned long retaddr; /* Return address */
> -} __packed;
> -
> /*
> * Structure that defines a return function trace.
> * It's already packed but the attribute "packed" is needed
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 85eabb454bee..9fac291b913a 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -955,10 +955,6 @@ extern void graph_trace_close(struct trace_iterator *iter);
> extern int __trace_graph_entry(struct trace_array *tr,
> struct ftrace_graph_ent *trace,
> unsigned int trace_ctx);
> -extern int __trace_graph_retaddr_entry(struct trace_array *tr,
> - struct ftrace_graph_ent *trace,
> - unsigned int trace_ctx,
> - unsigned long retaddr);
> extern void __trace_graph_return(struct trace_array *tr,
> struct ftrace_graph_ret *trace,
> unsigned int trace_ctx,
> diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> index de294ae2c5c5..593a74663c65 100644
> --- a/kernel/trace/trace_entries.h
> +++ b/kernel/trace/trace_entries.h
> @@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> TRACE_GRAPH_RETADDR_ENT,
>
> F_STRUCT(
> - __field_struct( struct fgraph_retaddr_ent, graph_ent )
> + __field_struct( struct ftrace_graph_ent, graph_ent )
> __field_packed( unsigned long, graph_ent, func )
> __field_packed( unsigned int, graph_ent, depth )
> - __field_packed( unsigned long, graph_ent, retaddr )
> + __dynamic_array(unsigned long, args )
> ),
>
> F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
> - (void *)__entry->retaddr)
> + (void *)__entry->args[0])
> );
>
> #else
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index a7f4b9a47a71..f31eeeffbb2d 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -16,6 +16,15 @@
> #include "trace.h"
> #include "trace_output.h"
>
> +#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> +#define HAVE_RETADDR 1
> +#define ARGS_OFFS(args_size) \
> + ((args_size) > FTRACE_REGS_MAX_ARGS * sizeof(long) ? 1 : 0)
> +#else
> +#define HAVE_RETADDR 0
> +#define ARGS_OFFS(args_size) 0
> +#endif
> +
> /* When set, irq functions will be ignored */
> static int ftrace_graph_skip_irqs;
>
> @@ -27,21 +36,25 @@ struct fgraph_cpu_data {
> unsigned long enter_funcs[FTRACE_RETFUNC_DEPTH];
> };
>
> +/*
> + * fgraph_retaddr_ent_entry and ftrace_graph_ent_entry share layout, ent
> + * member repurposed for storage
> + */
> struct fgraph_ent_args {
> struct ftrace_graph_ent_entry ent;
> - /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
> - unsigned long args[FTRACE_REGS_MAX_ARGS];
> + /*
> + * Force the sizeof of args[] to have (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR)
> + * entries with the first entry storing the return address for
> + * TRACE_GRAPH_RETADDR_ENT.
> + */
> + unsigned long args[FTRACE_REGS_MAX_ARGS + HAVE_RETADDR];
> };
>
> struct fgraph_data {
> struct fgraph_cpu_data __percpu *cpu_data;
>
> /* Place to preserve last processed entry. */
> - union {
> - struct fgraph_ent_args ent;
> - /* TODO allow retaddr to have args */
> - struct fgraph_retaddr_ent_entry rent;
> - };
> + struct fgraph_ent_args ent;
> struct ftrace_graph_ret_entry ret;
> int failed;
> int cpu;
> @@ -127,22 +140,43 @@ static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
> struct ring_buffer_event *event;
> struct trace_buffer *buffer = tr->array_buffer.buffer;
> struct ftrace_graph_ent_entry *entry;
> - int size;
> + unsigned long retaddr = 0;
> + int size = sizeof(*entry);
> + int type = TRACE_GRAPH_ENT;
> + bool store_args = false;
> + int nr_args = 0, args_offs = 0;
> +
> + if (tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> + retaddr = ftrace_graph_top_ret_addr(current);
> + type = TRACE_GRAPH_RETADDR_ENT;
> + nr_args += 1;
> + }
>
> /* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
> - size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
> + if (tracer_flags_is_set(TRACE_GRAPH_ARGS)) {
> + store_args = !!fregs;
> + if (store_args)
> + nr_args += FTRACE_REGS_MAX_ARGS;
> + }
>
> - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size, trace_ctx);
> + size += nr_args * sizeof(long);
> + event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
> if (!event)
> return 0;
>
> entry = ring_buffer_event_data(event);
> entry->graph_ent = *trace;
>
> + /* Store the retaddr in args[0] */
> + if (type == TRACE_GRAPH_RETADDR_ENT) {
> + entry->args[0] = retaddr;
> + args_offs += 1;
> + }
> +
> #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
> - if (fregs) {
> + if (store_args) {
> for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
> - entry->args[i] = ftrace_regs_get_argument(fregs, i);
> + entry->args[i + args_offs] = ftrace_regs_get_argument(fregs, i);
> }
> #endif
>
> @@ -158,38 +192,6 @@ int __trace_graph_entry(struct trace_array *tr,
> return __graph_entry(tr, trace, trace_ctx, NULL);
> }
>
> -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> -int __trace_graph_retaddr_entry(struct trace_array *tr,
> - struct ftrace_graph_ent *trace,
> - unsigned int trace_ctx,
> - unsigned long retaddr)
> -{
> - struct ring_buffer_event *event;
> - struct trace_buffer *buffer = tr->array_buffer.buffer;
> - struct fgraph_retaddr_ent_entry *entry;
> -
> - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
> - sizeof(*entry), trace_ctx);
> - if (!event)
> - return 0;
> - entry = ring_buffer_event_data(event);
> - entry->graph_ent.func = trace->func;
> - entry->graph_ent.depth = trace->depth;
> - entry->graph_ent.retaddr = retaddr;
> - trace_buffer_unlock_commit_nostack(buffer, event);
> -
> - return 1;
> -}
> -#else
> -int __trace_graph_retaddr_entry(struct trace_array *tr,
> - struct ftrace_graph_ent *trace,
> - unsigned int trace_ctx,
> - unsigned long retaddr)
> -{
> - return 1;
> -}
> -#endif
> -
> static inline int ftrace_graph_ignore_irqs(void)
> {
> if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
> @@ -211,7 +213,6 @@ static int graph_entry(struct ftrace_graph_ent *trace,
> struct trace_array *tr = gops->private;
> struct fgraph_times *ftimes;
> unsigned int trace_ctx;
> - int ret = 0;
>
> if (*task_var & TRACE_GRAPH_NOTRACE)
> return 0;
> @@ -262,15 +263,7 @@ static int graph_entry(struct ftrace_graph_ent *trace,
> return 1;
>
> trace_ctx = tracing_gen_ctx();
> - if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
> - tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> - unsigned long retaddr = ftrace_graph_top_ret_addr(current);
> - ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
> - } else {
> - ret = __graph_entry(tr, trace, trace_ctx, fregs);
> - }
> -
> - return ret;
> + return __graph_entry(tr, trace, trace_ctx, fregs);
> }
>
> int trace_graph_entry(struct ftrace_graph_ent *trace,
> @@ -634,13 +627,9 @@ get_return_for_leaf(struct trace_iterator *iter,
> * Save current and next entries for later reference
> * if the output fails.
> */
> - if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
> - data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
> - } else {
> - int size = min((int)sizeof(data->ent), (int)iter->ent_size);
> + int size = min_t(int, sizeof(data->ent), iter->ent_size);
>
> - memcpy(&data->ent, curr, size);
> - }
> + memcpy(&data->ent, curr, size);
> /*
> * If the next event is not a return type, then
> * we only care about what type it is. Otherwise we can
> @@ -811,21 +800,21 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
>
> #ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> #define __TRACE_GRAPH_PRINT_RETADDR TRACE_GRAPH_PRINT_RETADDR
> -static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_entry *entry,
> - u32 trace_flags, bool comment)
> +static void print_graph_retaddr(struct trace_seq *s, unsigned long retaddr, u32 trace_flags,
> + bool comment)
> {
> if (comment)
> trace_seq_puts(s, " /*");
>
> trace_seq_puts(s, " <-");
> - seq_print_ip_sym(s, entry->graph_ent.retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
> + seq_print_ip_sym(s, retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
>
> if (comment)
> trace_seq_puts(s, " */");
> }
> #else
> #define __TRACE_GRAPH_PRINT_RETADDR 0
> -#define print_graph_retaddr(_seq, _entry, _tflags, _comment) do { } while (0)
> +#define print_graph_retaddr(_seq, _retaddr, _tflags, _comment) do { } while (0)
> #endif
>
> #if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
> @@ -869,10 +858,12 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> trace_seq_printf(s, "%ps", func);
>
> if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
> - print_function_args(s, entry->args, (unsigned long)func);
> + print_function_args(s, entry->args + ARGS_OFFS(args_size),
> + (unsigned long)func);
> trace_seq_putc(s, ';');
> - } else
> + } else {
nit: you don't need to add braces for a single line block.
> trace_seq_puts(s, "();");
> + }
>
> if (print_retval || print_retaddr)
> trace_seq_puts(s, " /*");
> @@ -882,8 +873,7 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> }
>
> if (print_retaddr)
> - print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
> - trace_flags, false);
> + print_graph_retaddr(s, entry->args[0], trace_flags, false);
>
> if (print_retval) {
> if (hex_format || (err_code == 0))
> @@ -964,10 +954,12 @@ print_graph_entry_leaf(struct trace_iterator *iter,
> trace_seq_printf(s, "%ps", (void *)ret_func);
>
> if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
> - print_function_args(s, entry->args, ret_func);
> + print_function_args(s, entry->args + ARGS_OFFS(args_size),
> + ret_func);
> trace_seq_putc(s, ';');
> - } else
> + } else {
Ditto.
> trace_seq_puts(s, "();");
> + }
> }
> trace_seq_putc(s, '\n');
>
> @@ -1016,7 +1008,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
> args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
>
> if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
> - print_function_args(s, entry->args, func);
> + print_function_args(s, entry->args + ARGS_OFFS(args_size), func);
> else
> trace_seq_puts(s, "()");
>
> @@ -1024,8 +1016,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
>
> if (flags & __TRACE_GRAPH_PRINT_RETADDR &&
> entry->ent.type == TRACE_GRAPH_RETADDR_ENT)
> - print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
> - tr->trace_flags, true);
> + print_graph_retaddr(s, entry->args[0], tr->trace_flags, true);
> trace_seq_putc(s, '\n');
>
> if (trace_seq_has_overflowed(s))
> @@ -1202,7 +1193,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
> * it can be safely saved at the stack.
> */
> struct ftrace_graph_ent_entry *entry;
> - u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
> + u8 save_buf[sizeof(*entry) + (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR) * sizeof(long)];
>
> /* The ent_size is expected to be as big as the entry */
> if (iter->ent_size > sizeof(save_buf))
> @@ -1429,16 +1420,12 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
> trace_assign_type(field, entry);
> return print_graph_entry(field, s, iter, flags);
> }
> -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> case TRACE_GRAPH_RETADDR_ENT: {
> - struct fgraph_retaddr_ent_entry saved;
> struct fgraph_retaddr_ent_entry *rfield;
>
> trace_assign_type(rfield, entry);
> - saved = *rfield;
> - return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
> + return print_graph_entry((typeof(field))rfield, s, iter, flags);
> }
> -#endif
> case TRACE_GRAPH_RET: {
> struct ftrace_graph_ret_entry *field;
> trace_assign_type(field, entry);
> --
> 2.34.1
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* [PATCH v3 RESEND] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-12 3:43 UTC (permalink / raw)
To: rostedt
Cc: linux-trace-kernel, linux-kernel, Donglin Peng, Sven Schnelle,
Masami Hiramatsu
From: Donglin Peng <pengdonglin@xiaomi.com>
Currently, the funcgraph-args and funcgraph-retaddr features are mutually exclusive.
This patch resolves this limitation by modifying funcgraph-retaddr to adopt the same
implementation approach as funcgraph-args, specifically by storing the return address
in the first entry of the args array.
As a result, both features now coexist seamlessly and function as intended.
To verify the change, use perf to trace vfs_write with both options enabled:
Before:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
0) | down_read() { /* <-n_tty_write+0xa3/0x540 */
0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
0) 0.079 us | preempt_count_add(); /* <-down_read+0x3b/0x160 */
0) 0.077 us | preempt_count_sub(); /* <-down_read+0x8b/0x160 */
0) 0.754 us | }
After:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
0) | down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
0) 0.079 us | preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
0) 0.077 us | preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
0) 0.754 us | }
Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
---
v3:
- Replace min() with min_t() to improve type safety and maintainability
- Keep only one Signed-off-by for cleaner attribution
- Code refactoring for improved readability
v2:
- Preserve retaddr event functionality (suggested by Steven)
- Store the retaddr in args[0] (suggested by Steven)
- Refactor implementation logic and commit message clarity
---
include/linux/ftrace.h | 11 --
kernel/trace/trace.h | 4 -
kernel/trace/trace_entries.h | 6 +-
kernel/trace/trace_functions_graph.c | 145 ++++++++++++---------------
4 files changed, 69 insertions(+), 97 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 7ded7df6e9b5..88cb54d73bdb 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
int depth;
} __packed;
-/*
- * Structure that defines an entry function trace with retaddr.
- * It's already packed but the attribute "packed" is needed
- * to remove extra padding at the end.
- */
-struct fgraph_retaddr_ent {
- unsigned long func; /* Current function */
- int depth;
- unsigned long retaddr; /* Return address */
-} __packed;
-
/*
* Structure that defines a return function trace.
* It's already packed but the attribute "packed" is needed
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 85eabb454bee..9fac291b913a 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -955,10 +955,6 @@ extern void graph_trace_close(struct trace_iterator *iter);
extern int __trace_graph_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned int trace_ctx);
-extern int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr);
extern void __trace_graph_return(struct trace_array *tr,
struct ftrace_graph_ret *trace,
unsigned int trace_ctx,
diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index de294ae2c5c5..593a74663c65 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
TRACE_GRAPH_RETADDR_ENT,
F_STRUCT(
- __field_struct( struct fgraph_retaddr_ent, graph_ent )
+ __field_struct( struct ftrace_graph_ent, graph_ent )
__field_packed( unsigned long, graph_ent, func )
__field_packed( unsigned int, graph_ent, depth )
- __field_packed( unsigned long, graph_ent, retaddr )
+ __dynamic_array(unsigned long, args )
),
F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
- (void *)__entry->retaddr)
+ (void *)__entry->args[0])
);
#else
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index a7f4b9a47a71..f31eeeffbb2d 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -16,6 +16,15 @@
#include "trace.h"
#include "trace_output.h"
+#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
+#define HAVE_RETADDR 1
+#define ARGS_OFFS(args_size) \
+ ((args_size) > FTRACE_REGS_MAX_ARGS * sizeof(long) ? 1 : 0)
+#else
+#define HAVE_RETADDR 0
+#define ARGS_OFFS(args_size) 0
+#endif
+
/* When set, irq functions will be ignored */
static int ftrace_graph_skip_irqs;
@@ -27,21 +36,25 @@ struct fgraph_cpu_data {
unsigned long enter_funcs[FTRACE_RETFUNC_DEPTH];
};
+/*
+ * fgraph_retaddr_ent_entry and ftrace_graph_ent_entry share layout, ent
+ * member repurposed for storage
+ */
struct fgraph_ent_args {
struct ftrace_graph_ent_entry ent;
- /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
- unsigned long args[FTRACE_REGS_MAX_ARGS];
+ /*
+ * Force the sizeof of args[] to have (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR)
+ * entries with the first entry storing the return address for
+ * TRACE_GRAPH_RETADDR_ENT.
+ */
+ unsigned long args[FTRACE_REGS_MAX_ARGS + HAVE_RETADDR];
};
struct fgraph_data {
struct fgraph_cpu_data __percpu *cpu_data;
/* Place to preserve last processed entry. */
- union {
- struct fgraph_ent_args ent;
- /* TODO allow retaddr to have args */
- struct fgraph_retaddr_ent_entry rent;
- };
+ struct fgraph_ent_args ent;
struct ftrace_graph_ret_entry ret;
int failed;
int cpu;
@@ -127,22 +140,43 @@ static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
struct ring_buffer_event *event;
struct trace_buffer *buffer = tr->array_buffer.buffer;
struct ftrace_graph_ent_entry *entry;
- int size;
+ unsigned long retaddr = 0;
+ int size = sizeof(*entry);
+ int type = TRACE_GRAPH_ENT;
+ bool store_args = false;
+ int nr_args = 0, args_offs = 0;
+
+ if (tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
+ retaddr = ftrace_graph_top_ret_addr(current);
+ type = TRACE_GRAPH_RETADDR_ENT;
+ nr_args += 1;
+ }
/* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
- size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
+ if (tracer_flags_is_set(TRACE_GRAPH_ARGS)) {
+ store_args = !!fregs;
+ if (store_args)
+ nr_args += FTRACE_REGS_MAX_ARGS;
+ }
- event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size, trace_ctx);
+ size += nr_args * sizeof(long);
+ event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
if (!event)
return 0;
entry = ring_buffer_event_data(event);
entry->graph_ent = *trace;
+ /* Store the retaddr in args[0] */
+ if (type == TRACE_GRAPH_RETADDR_ENT) {
+ entry->args[0] = retaddr;
+ args_offs += 1;
+ }
+
#ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
- if (fregs) {
+ if (store_args) {
for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
- entry->args[i] = ftrace_regs_get_argument(fregs, i);
+ entry->args[i + args_offs] = ftrace_regs_get_argument(fregs, i);
}
#endif
@@ -158,38 +192,6 @@ int __trace_graph_entry(struct trace_array *tr,
return __graph_entry(tr, trace, trace_ctx, NULL);
}
-#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
-int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr)
-{
- struct ring_buffer_event *event;
- struct trace_buffer *buffer = tr->array_buffer.buffer;
- struct fgraph_retaddr_ent_entry *entry;
-
- event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
- sizeof(*entry), trace_ctx);
- if (!event)
- return 0;
- entry = ring_buffer_event_data(event);
- entry->graph_ent.func = trace->func;
- entry->graph_ent.depth = trace->depth;
- entry->graph_ent.retaddr = retaddr;
- trace_buffer_unlock_commit_nostack(buffer, event);
-
- return 1;
-}
-#else
-int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr)
-{
- return 1;
-}
-#endif
-
static inline int ftrace_graph_ignore_irqs(void)
{
if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
@@ -211,7 +213,6 @@ static int graph_entry(struct ftrace_graph_ent *trace,
struct trace_array *tr = gops->private;
struct fgraph_times *ftimes;
unsigned int trace_ctx;
- int ret = 0;
if (*task_var & TRACE_GRAPH_NOTRACE)
return 0;
@@ -262,15 +263,7 @@ static int graph_entry(struct ftrace_graph_ent *trace,
return 1;
trace_ctx = tracing_gen_ctx();
- if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
- tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
- unsigned long retaddr = ftrace_graph_top_ret_addr(current);
- ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
- } else {
- ret = __graph_entry(tr, trace, trace_ctx, fregs);
- }
-
- return ret;
+ return __graph_entry(tr, trace, trace_ctx, fregs);
}
int trace_graph_entry(struct ftrace_graph_ent *trace,
@@ -634,13 +627,9 @@ get_return_for_leaf(struct trace_iterator *iter,
* Save current and next entries for later reference
* if the output fails.
*/
- if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
- data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
- } else {
- int size = min((int)sizeof(data->ent), (int)iter->ent_size);
+ int size = min_t(int, sizeof(data->ent), iter->ent_size);
- memcpy(&data->ent, curr, size);
- }
+ memcpy(&data->ent, curr, size);
/*
* If the next event is not a return type, then
* we only care about what type it is. Otherwise we can
@@ -811,21 +800,21 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
#define __TRACE_GRAPH_PRINT_RETADDR TRACE_GRAPH_PRINT_RETADDR
-static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_entry *entry,
- u32 trace_flags, bool comment)
+static void print_graph_retaddr(struct trace_seq *s, unsigned long retaddr, u32 trace_flags,
+ bool comment)
{
if (comment)
trace_seq_puts(s, " /*");
trace_seq_puts(s, " <-");
- seq_print_ip_sym(s, entry->graph_ent.retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
+ seq_print_ip_sym(s, retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
if (comment)
trace_seq_puts(s, " */");
}
#else
#define __TRACE_GRAPH_PRINT_RETADDR 0
-#define print_graph_retaddr(_seq, _entry, _tflags, _comment) do { } while (0)
+#define print_graph_retaddr(_seq, _retaddr, _tflags, _comment) do { } while (0)
#endif
#if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
@@ -869,10 +858,12 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
trace_seq_printf(s, "%ps", func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, (unsigned long)func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size),
+ (unsigned long)func);
trace_seq_putc(s, ';');
- } else
+ } else {
trace_seq_puts(s, "();");
+ }
if (print_retval || print_retaddr)
trace_seq_puts(s, " /*");
@@ -882,8 +873,7 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
}
if (print_retaddr)
- print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
- trace_flags, false);
+ print_graph_retaddr(s, entry->args[0], trace_flags, false);
if (print_retval) {
if (hex_format || (err_code == 0))
@@ -964,10 +954,12 @@ print_graph_entry_leaf(struct trace_iterator *iter,
trace_seq_printf(s, "%ps", (void *)ret_func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, ret_func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size),
+ ret_func);
trace_seq_putc(s, ';');
- } else
+ } else {
trace_seq_puts(s, "();");
+ }
}
trace_seq_putc(s, '\n');
@@ -1016,7 +1008,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
- print_function_args(s, entry->args, func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size), func);
else
trace_seq_puts(s, "()");
@@ -1024,8 +1016,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
if (flags & __TRACE_GRAPH_PRINT_RETADDR &&
entry->ent.type == TRACE_GRAPH_RETADDR_ENT)
- print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
- tr->trace_flags, true);
+ print_graph_retaddr(s, entry->args[0], tr->trace_flags, true);
trace_seq_putc(s, '\n');
if (trace_seq_has_overflowed(s))
@@ -1202,7 +1193,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
* it can be safely saved at the stack.
*/
struct ftrace_graph_ent_entry *entry;
- u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
+ u8 save_buf[sizeof(*entry) + (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR) * sizeof(long)];
/* The ent_size is expected to be as big as the entry */
if (iter->ent_size > sizeof(save_buf))
@@ -1429,16 +1420,12 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
trace_assign_type(field, entry);
return print_graph_entry(field, s, iter, flags);
}
-#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
case TRACE_GRAPH_RETADDR_ENT: {
- struct fgraph_retaddr_ent_entry saved;
struct fgraph_retaddr_ent_entry *rfield;
trace_assign_type(rfield, entry);
- saved = *rfield;
- return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
+ return print_graph_entry((typeof(field))rfield, s, iter, flags);
}
-#endif
case TRACE_GRAPH_RET: {
struct ftrace_graph_ret_entry *field;
trace_assign_type(field, entry);
--
2.34.1
^ permalink raw reply related
* [PATCH v3] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-12 3:15 UTC (permalink / raw)
To: rostedt
Cc: linux-trace-kernel, linux-kernel, Donglin Peng, Sven Schnelle,
Masami Hiramatsu
From: Donglin Peng <pengdonglin@xiaomi.com>
From: Donglin Peng <pengdonglin@xiaomi.com>
Currently, the funcgraph-args and funcgraph-retaddr features are mutually exclusive.
This patch resolves this limitation by modifying funcgraph-retaddr to adopt the same
implementation approach as funcgraph-args, specifically by storing the return address
in the first entry of the args array.
As a result, both features now coexist seamlessly and function as intended.
To verify the change, use perf to trace vfs_write with both options enabled:
Before:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
0) | down_read() { /* <-n_tty_write+0xa3/0x540 */
0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
0) 0.079 us | preempt_count_add(); /* <-down_read+0x3b/0x160 */
0) 0.077 us | preempt_count_sub(); /* <-down_read+0x8b/0x160 */
0) 0.754 us | }
After:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
0) | down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
0) 0.079 us | preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
0) 0.077 us | preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
0) 0.754 us | }
Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
---
v3:
- Replace min() with min_t() to improve type safety and maintainability
- Keep only one Signed-off-by for cleaner attribution
- Code refactoring for improved readability
v2:
- Preserve retaddr event functionality (suggested by Steven)
- Store the retaddr in args[0] (suggested by Steven)
- Refactor implementation logic and commit message clarity
---
include/linux/ftrace.h | 11 --
kernel/trace/trace.h | 4 -
kernel/trace/trace_entries.h | 6 +-
kernel/trace/trace_functions_graph.c | 145 ++++++++++++---------------
4 files changed, 69 insertions(+), 97 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 7ded7df6e9b5..88cb54d73bdb 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
int depth;
} __packed;
-/*
- * Structure that defines an entry function trace with retaddr.
- * It's already packed but the attribute "packed" is needed
- * to remove extra padding at the end.
- */
-struct fgraph_retaddr_ent {
- unsigned long func; /* Current function */
- int depth;
- unsigned long retaddr; /* Return address */
-} __packed;
-
/*
* Structure that defines a return function trace.
* It's already packed but the attribute "packed" is needed
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 85eabb454bee..9fac291b913a 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -955,10 +955,6 @@ extern void graph_trace_close(struct trace_iterator *iter);
extern int __trace_graph_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned int trace_ctx);
-extern int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr);
extern void __trace_graph_return(struct trace_array *tr,
struct ftrace_graph_ret *trace,
unsigned int trace_ctx,
diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index de294ae2c5c5..593a74663c65 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
TRACE_GRAPH_RETADDR_ENT,
F_STRUCT(
- __field_struct( struct fgraph_retaddr_ent, graph_ent )
+ __field_struct( struct ftrace_graph_ent, graph_ent )
__field_packed( unsigned long, graph_ent, func )
__field_packed( unsigned int, graph_ent, depth )
- __field_packed( unsigned long, graph_ent, retaddr )
+ __dynamic_array(unsigned long, args )
),
F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
- (void *)__entry->retaddr)
+ (void *)__entry->args[0])
);
#else
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index a7f4b9a47a71..f31eeeffbb2d 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -16,6 +16,15 @@
#include "trace.h"
#include "trace_output.h"
+#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
+#define HAVE_RETADDR 1
+#define ARGS_OFFS(args_size) \
+ ((args_size) > FTRACE_REGS_MAX_ARGS * sizeof(long) ? 1 : 0)
+#else
+#define HAVE_RETADDR 0
+#define ARGS_OFFS(args_size) 0
+#endif
+
/* When set, irq functions will be ignored */
static int ftrace_graph_skip_irqs;
@@ -27,21 +36,25 @@ struct fgraph_cpu_data {
unsigned long enter_funcs[FTRACE_RETFUNC_DEPTH];
};
+/*
+ * fgraph_retaddr_ent_entry and ftrace_graph_ent_entry share layout, ent
+ * member repurposed for storage
+ */
struct fgraph_ent_args {
struct ftrace_graph_ent_entry ent;
- /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
- unsigned long args[FTRACE_REGS_MAX_ARGS];
+ /*
+ * Force the sizeof of args[] to have (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR)
+ * entries with the first entry storing the return address for
+ * TRACE_GRAPH_RETADDR_ENT.
+ */
+ unsigned long args[FTRACE_REGS_MAX_ARGS + HAVE_RETADDR];
};
struct fgraph_data {
struct fgraph_cpu_data __percpu *cpu_data;
/* Place to preserve last processed entry. */
- union {
- struct fgraph_ent_args ent;
- /* TODO allow retaddr to have args */
- struct fgraph_retaddr_ent_entry rent;
- };
+ struct fgraph_ent_args ent;
struct ftrace_graph_ret_entry ret;
int failed;
int cpu;
@@ -127,22 +140,43 @@ static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
struct ring_buffer_event *event;
struct trace_buffer *buffer = tr->array_buffer.buffer;
struct ftrace_graph_ent_entry *entry;
- int size;
+ unsigned long retaddr = 0;
+ int size = sizeof(*entry);
+ int type = TRACE_GRAPH_ENT;
+ bool store_args = false;
+ int nr_args = 0, args_offs = 0;
+
+ if (tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
+ retaddr = ftrace_graph_top_ret_addr(current);
+ type = TRACE_GRAPH_RETADDR_ENT;
+ nr_args += 1;
+ }
/* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
- size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
+ if (tracer_flags_is_set(TRACE_GRAPH_ARGS)) {
+ store_args = !!fregs;
+ if (store_args)
+ nr_args += FTRACE_REGS_MAX_ARGS;
+ }
- event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size, trace_ctx);
+ size += nr_args * sizeof(long);
+ event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
if (!event)
return 0;
entry = ring_buffer_event_data(event);
entry->graph_ent = *trace;
+ /* Store the retaddr in args[0] */
+ if (type == TRACE_GRAPH_RETADDR_ENT) {
+ entry->args[0] = retaddr;
+ args_offs += 1;
+ }
+
#ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
- if (fregs) {
+ if (store_args) {
for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
- entry->args[i] = ftrace_regs_get_argument(fregs, i);
+ entry->args[i + args_offs] = ftrace_regs_get_argument(fregs, i);
}
#endif
@@ -158,38 +192,6 @@ int __trace_graph_entry(struct trace_array *tr,
return __graph_entry(tr, trace, trace_ctx, NULL);
}
-#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
-int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr)
-{
- struct ring_buffer_event *event;
- struct trace_buffer *buffer = tr->array_buffer.buffer;
- struct fgraph_retaddr_ent_entry *entry;
-
- event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
- sizeof(*entry), trace_ctx);
- if (!event)
- return 0;
- entry = ring_buffer_event_data(event);
- entry->graph_ent.func = trace->func;
- entry->graph_ent.depth = trace->depth;
- entry->graph_ent.retaddr = retaddr;
- trace_buffer_unlock_commit_nostack(buffer, event);
-
- return 1;
-}
-#else
-int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr)
-{
- return 1;
-}
-#endif
-
static inline int ftrace_graph_ignore_irqs(void)
{
if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
@@ -211,7 +213,6 @@ static int graph_entry(struct ftrace_graph_ent *trace,
struct trace_array *tr = gops->private;
struct fgraph_times *ftimes;
unsigned int trace_ctx;
- int ret = 0;
if (*task_var & TRACE_GRAPH_NOTRACE)
return 0;
@@ -262,15 +263,7 @@ static int graph_entry(struct ftrace_graph_ent *trace,
return 1;
trace_ctx = tracing_gen_ctx();
- if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
- tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
- unsigned long retaddr = ftrace_graph_top_ret_addr(current);
- ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
- } else {
- ret = __graph_entry(tr, trace, trace_ctx, fregs);
- }
-
- return ret;
+ return __graph_entry(tr, trace, trace_ctx, fregs);
}
int trace_graph_entry(struct ftrace_graph_ent *trace,
@@ -634,13 +627,9 @@ get_return_for_leaf(struct trace_iterator *iter,
* Save current and next entries for later reference
* if the output fails.
*/
- if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
- data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
- } else {
- int size = min((int)sizeof(data->ent), (int)iter->ent_size);
+ int size = min_t(int, sizeof(data->ent), iter->ent_size);
- memcpy(&data->ent, curr, size);
- }
+ memcpy(&data->ent, curr, size);
/*
* If the next event is not a return type, then
* we only care about what type it is. Otherwise we can
@@ -811,21 +800,21 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
#define __TRACE_GRAPH_PRINT_RETADDR TRACE_GRAPH_PRINT_RETADDR
-static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_entry *entry,
- u32 trace_flags, bool comment)
+static void print_graph_retaddr(struct trace_seq *s, unsigned long retaddr, u32 trace_flags,
+ bool comment)
{
if (comment)
trace_seq_puts(s, " /*");
trace_seq_puts(s, " <-");
- seq_print_ip_sym(s, entry->graph_ent.retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
+ seq_print_ip_sym(s, retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
if (comment)
trace_seq_puts(s, " */");
}
#else
#define __TRACE_GRAPH_PRINT_RETADDR 0
-#define print_graph_retaddr(_seq, _entry, _tflags, _comment) do { } while (0)
+#define print_graph_retaddr(_seq, _retaddr, _tflags, _comment) do { } while (0)
#endif
#if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
@@ -869,10 +858,12 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
trace_seq_printf(s, "%ps", func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, (unsigned long)func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size),
+ (unsigned long)func);
trace_seq_putc(s, ';');
- } else
+ } else {
trace_seq_puts(s, "();");
+ }
if (print_retval || print_retaddr)
trace_seq_puts(s, " /*");
@@ -882,8 +873,7 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
}
if (print_retaddr)
- print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
- trace_flags, false);
+ print_graph_retaddr(s, entry->args[0], trace_flags, false);
if (print_retval) {
if (hex_format || (err_code == 0))
@@ -964,10 +954,12 @@ print_graph_entry_leaf(struct trace_iterator *iter,
trace_seq_printf(s, "%ps", (void *)ret_func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, ret_func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size),
+ ret_func);
trace_seq_putc(s, ';');
- } else
+ } else {
trace_seq_puts(s, "();");
+ }
}
trace_seq_putc(s, '\n');
@@ -1016,7 +1008,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
- print_function_args(s, entry->args, func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size), func);
else
trace_seq_puts(s, "()");
@@ -1024,8 +1016,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
if (flags & __TRACE_GRAPH_PRINT_RETADDR &&
entry->ent.type == TRACE_GRAPH_RETADDR_ENT)
- print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
- tr->trace_flags, true);
+ print_graph_retaddr(s, entry->args[0], tr->trace_flags, true);
trace_seq_putc(s, '\n');
if (trace_seq_has_overflowed(s))
@@ -1202,7 +1193,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
* it can be safely saved at the stack.
*/
struct ftrace_graph_ent_entry *entry;
- u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
+ u8 save_buf[sizeof(*entry) + (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR) * sizeof(long)];
/* The ent_size is expected to be as big as the entry */
if (iter->ent_size > sizeof(save_buf))
@@ -1429,16 +1420,12 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
trace_assign_type(field, entry);
return print_graph_entry(field, s, iter, flags);
}
-#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
case TRACE_GRAPH_RETADDR_ENT: {
- struct fgraph_retaddr_ent_entry saved;
struct fgraph_retaddr_ent_entry *rfield;
trace_assign_type(rfield, entry);
- saved = *rfield;
- return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
+ return print_graph_entry((typeof(field))rfield, s, iter, flags);
}
-#endif
case TRACE_GRAPH_RET: {
struct ftrace_graph_ret_entry *field;
trace_assign_type(field, entry);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v8 00/27] mm/ksw: Introduce KStackWatch debugging tool
From: Jinchao Wang @ 2025-11-12 2:14 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Andrew Morton, Masami Hiramatsu (Google), Peter Zijlstra,
Randy Dunlap, Marco Elver, Mike Rapoport, Alexander Potapenko,
Adrian Hunter, Alexander Shishkin, Alice Ryhl, Andrey Konovalov,
Andrey Ryabinin, Andrii Nakryiko, Ard Biesheuvel,
Arnaldo Carvalho de Melo, Ben Segall, Bill Wendling,
Borislav Petkov, Catalin Marinas, Dave Hansen, David Hildenbrand,
David Kaplan, David S. Miller, Dietmar Eggemann, Dmitry Vyukov,
H. Peter Anvin, Ian Rogers, Ingo Molnar, James Clark, Jinjie Ruan,
Jiri Olsa, Jonathan Corbet, Juri Lelli, Justin Stitt, kasan-dev,
Kees Cook, Liam R. Howlett, Liang Kan, Linus Walleij,
linux-arm-kernel, linux-doc, linux-kernel, linux-mm,
linux-perf-users, linux-trace-kernel, llvm, Lorenzo Stoakes,
Mark Rutland, Masahiro Yamada, Mathieu Desnoyers, Mel Gorman,
Michal Hocko, Miguel Ojeda, Nam Cao, Namhyung Kim,
Nathan Chancellor, Naveen N Rao, Nick Desaulniers, Rong Xu,
Sami Tolvanen, Steven Rostedt, Suren Baghdasaryan,
Thomas Gleixner, Thomas Weißschuh, Valentin Schneider,
Vincent Guittot, Vincenzo Frascino, Vlastimil Babka, Will Deacon,
workflows, x86
In-Reply-To: <aRIh4pBs7KCDhQOp@casper.infradead.org>
On Mon, Nov 10, 2025 at 05:33:22PM +0000, Matthew Wilcox wrote:
> On Tue, Nov 11, 2025 at 12:35:55AM +0800, Jinchao Wang wrote:
> > Earlier this year, I debugged a stack corruption panic that revealed the
> > limitations of existing debugging tools. The bug persisted for 739 days
> > before being fixed (CVE-2025-22036), and my reproduction scenario
> > differed from the CVE report—highlighting how unpredictably these bugs
> > manifest.
>
> Well, this demonstrates the dangers of keeping this problem siloed
> within your own exfat group. The fix made in 1bb7ff4204b6 is wrong!
> It was fixed properly in 7375f22495e7 which lists its Fixes: as
> Linux-2.6.12-rc2, but that's simply the beginning of git history.
> It's actually been there since v2.4.6.4 where it's documented as simply:
>
> - some subtle fs/buffer.c race conditions (Andrew Morton, me)
>
> As far as I can tell the changes made in 1bb7ff4204b6 should be
> reverted.
Thank you for the correction and the detailed history. I wasn't aware this
dated back to v2.4.6.4. I'm not part of the exfat group; I simply
encountered a bug that 1bb7ff4204b6 happened to resolve in my scenario.
The timeline actually illustrates the exact problem KStackWatch addresses:
a bug introduced in 2001, partially addressed in 2025, then properly fixed
months later. The 24-year gap suggests these silent stack corruptions are
extremely difficult to locate.
>
> > Initially, I enabled KASAN, but the bug did not reproduce. Reviewing the
> > code in __blk_flush_plug(), I found it difficult to trace all logic
> > paths due to indirect function calls through function pointers.
>
> So why is the solution here not simply to fix KASAN instead of this
> giant patch series?
KASAN caught 7375f22495e7 because put_bh() accessed bh->b_count after
wait_on_buffer() of another thread returned—the stack was invalid.
In 1bb7ff4204b6 and my case, corruption occurred before the victim
function of another thread returned. The stack remained valid to KASAN,
so no warning triggered. This is timing-dependent, not a KASAN deficiency.
Making KASAN treat parts of active stack frame as invalid would be
complex and add significant overhead, likely worsening the reproduction
prevention issue. KASAN's overhead already prevented reproduction in my
environment.
KStackWatch takes a different approach: it watches stack frame regardless
of whether KASAN considers them valid or invalid, with much less overhead
thereby preserving reproduction scenarios.
The value proposition:
Finding where corruption occurs is the bottleneck. Once located,
subsystem experts can analyze the root cause. Without that location, even
experts are stuck.
If KStackWatch had existed earlier, this 24-year-old bug might have been
found sooner when someone hit a similar corruption. The same applies to
other stack corruption bugs.
I'd appreciate your thoughts on whether this addresses your concerns.
Best regards,
Jinchao
^ permalink raw reply
* Re: [PATCH v2] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-12 1:47 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-trace-kernel, linux-kernel, Sven Schnelle, Masami Hiramatsu,
Donglin Peng
In-Reply-To: <20251111183703.0045ca23@gandalf.local.home>
On Wed, Nov 12, 2025 at 7:36 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 11 Nov 2025 21:54:32 +0800
> Donglin Peng <dolinux.peng@gmail.com> wrote:
>
> > + /* Store the retaddr in args[0] */
> > + if (type == TRACE_GRAPH_RETADDR_ENT)
> > + entry->args[i++] = retaddr;
> > +
> > #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
> > - if (fregs) {
> > - for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
> > - entry->args[i] = ftrace_regs_get_argument(fregs, i);
> > + if (store_args) {
> > + while (i < nr_args) {
>
> Nit, this could stay as a for loop. You just don't initialize the 'i':
Thanks, I will fix it in v3.
>
> for (; i < nr_args; i++) {
>
> > + entry->args[i] = ftrace_regs_get_argument(fregs,
> > + i - (type == TRACE_GRAPH_RETADDR_ENT ? 1 : 0));
> > + i++;
>
> Then you don't need the "i++" here.
>
> -- Steve
>
> > + }
> > }
^ permalink raw reply
* Re: [PATCH v2] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-12 1:46 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-trace-kernel, linux-kernel, Sven Schnelle, Masami Hiramatsu,
Donglin Peng
In-Reply-To: <20251111182820.4b44cc8e@gandalf.local.home>
On Wed, Nov 12, 2025 at 7:28 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 11 Nov 2025 21:54:32 +0800
> Donglin Peng <dolinux.peng@gmail.com> wrote:
>
> > Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
> > Signed-off-by: Donglin Peng <dolinux.peng@gmail.com>
>
> BTW, are you two people?
>
> You only need one signed-off-by. Which one do you want to keep?
Thanks for catching this. I'll fix it in v3 by keeping only one
Signed-off-by - I'll use my Xiaomi email since this is work-related.
Apologies for the oversight.
>
> -- Steve
^ permalink raw reply
* Re: [PATCH] mm/migrate_device: Add tracepoints for debugging
From: Balbir Singh @ 2025-11-11 23:55 UTC (permalink / raw)
To: Andrew Morton, Steven Rostedt
Cc: linux-mm, linux-trace-kernel, linux-kernel, Masami Hiramatsu,
Mathieu Desnoyers, David Hildenbrand, Zi Yan, Joshua Hahn,
Rakie Kim, Byungchul Park, Gregory Price, Ying Huang,
Alistair Popple
In-Reply-To: <20251111155042.b07ecb045c978fe4c457b1d0@linux-foundation.org>
On 11/12/25 10:50, Andrew Morton wrote:
> On Mon, 10 Nov 2025 16:19:54 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> On Thu, 16 Oct 2025 16:46:19 +1100
>> Balbir Singh <balbirs@nvidia.com> wrote:
>>
>>> Add tracepoints for debugging device migration flow in migrate_device.c.
>>> This is helpful in debugging how long migration took (time can be
>>> tracked backwards from migrate_device_finalize to migrate_vma_setup).
>>>
>>> A combination of these events along with existing thp:*, exceptions:*
>>> and migrate:* is very useful for debugging issues related to
>>> migration.
>>>
>>
>> ...
>>
>>> hmm-tests-855 [002] 50.042800: set_migration_pmd: addr=7f2908a00000, pmd=dfffffffd39ffe00
>>> hmm-tests-855 [002] 50.062345: remove_migration_pmd: addr=7f2908a00000, pmd=efffffe00403fe00
>>
>> Each TRACE_EVENT() is equivalent to:
>>
>> DECLARE_EVENT_CLASS(event, ...)
>> DEFINE_EVENT(event, event, ...)
>>
>> Where a class is around 4-5K in size, and the DEFINE_EVENT is between
>> 500 and 1k in size.
>>
>> By using a single DECLARE_EVENT_CLASS() for multiple events, you can save
>> several thousands of bytes of memory.
>
> Thanks for the detailed review. I'll drop this version of the patch.
Thanks Steve and Andrew!
I've been looking at EVENT_CLASSES and looking for the savings you mentioned. I've not yet
finished my patch, I will resend it as soon as I have a new version
Balbir
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox