linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Introduce tracepoint for hugetlbfs
@ 2024-06-12  1:11 Hongbo Li
  2024-06-12  1:11 ` [PATCH 1/2] hugetlbfs: support tracepoint Hongbo Li
  2024-06-12  1:11 ` [PATCH 2/2] hugetlbfs: use tracepoints in hugetlbfs functions Hongbo Li
  0 siblings, 2 replies; 8+ messages in thread
From: Hongbo Li @ 2024-06-12  1:11 UTC (permalink / raw)
  To: muchun.song, rostedt, mhiramat
  Cc: mathieu.desnoyers, linux-mm, linux-trace-kernel, linux-fsdevel,
	lihongbo22

Here we add some basic tracepoints for debugging hugetlbfs: {alloc, free,
evict}_inode, setattr and fallocate.

Hongbo Li (2):
  hugetlbfs: support tracepoint
  hugetlbfs: use tracepoints in hugetlbfs functions.

 MAINTAINERS                      |   1 +
 fs/hugetlbfs/inode.c             |  21 +++-
 include/trace/events/hugetlbfs.h | 164 +++++++++++++++++++++++++++++++
 3 files changed, 184 insertions(+), 2 deletions(-)
 create mode 100644 include/trace/events/hugetlbfs.h

-- 
2.34.1



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] hugetlbfs: support tracepoint
  2024-06-12  1:11 [PATCH 0/2] Introduce tracepoint for hugetlbfs Hongbo Li
@ 2024-06-12  1:11 ` Hongbo Li
  2024-06-12  1:11 ` [PATCH 2/2] hugetlbfs: use tracepoints in hugetlbfs functions Hongbo Li
  1 sibling, 0 replies; 8+ messages in thread
From: Hongbo Li @ 2024-06-12  1:11 UTC (permalink / raw)
  To: muchun.song, rostedt, mhiramat
  Cc: mathieu.desnoyers, linux-mm, linux-trace-kernel, linux-fsdevel,
	lihongbo22

Add basic tracepoints for {alloc, evict, free}_inode, setattr and
fallocate. These can help users to debug hugetlbfs more conveniently.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 MAINTAINERS                      |   1 +
 include/trace/events/hugetlbfs.h | 164 +++++++++++++++++++++++++++++++
 2 files changed, 165 insertions(+)
 create mode 100644 include/trace/events/hugetlbfs.h

diff --git a/MAINTAINERS b/MAINTAINERS
index aacccb376c28..df6fe4aa0f50 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10187,6 +10187,7 @@ F:	Documentation/mm/hugetlbfs_reserv.rst
 F:	Documentation/mm/vmemmap_dedup.rst
 F:	fs/hugetlbfs/
 F:	include/linux/hugetlb.h
+F:	include/trace/events/hugetlbfs.h
 F:	mm/hugetlb.c
 F:	mm/hugetlb_vmemmap.c
 F:	mm/hugetlb_vmemmap.h
diff --git a/include/trace/events/hugetlbfs.h b/include/trace/events/hugetlbfs.h
new file mode 100644
index 000000000000..a4d785c87155
--- /dev/null
+++ b/include/trace/events/hugetlbfs.h
@@ -0,0 +1,164 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM hugetlbfs
+
+#if !defined(_TRACE_HUGETLBFS_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_HUGETLBFS_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(hugetlbfs_alloc_inode,
+
+	TP_PROTO(struct inode *inode, struct inode *dir, int mode),
+
+	TP_ARGS(inode, dir, mode),
+
+	TP_STRUCT__entry(
+		__field(dev_t,		dev)
+		__field(ino_t,		ino)
+		__field(ino_t,		dir)
+		__field(__u16,		mode)
+	),
+
+	TP_fast_assign(
+		__entry->dev		= inode->i_sb->s_dev;
+		__entry->ino		= inode->i_ino;
+		__entry->dir		= dir->i_ino;
+		__entry->mode		= mode;
+	),
+
+	TP_printk("dev = (%d,%d), ino = %lu, dir = %lu, mode = 0%o",
+		MAJOR(__entry->dev), MINOR(__entry->dev),
+		(unsigned long) __entry->ino,
+		(unsigned long) __entry->dir, __entry->mode)
+);
+
+DECLARE_EVENT_CLASS(hugetlbfs__inode,
+
+	TP_PROTO(struct inode *inode),
+
+	TP_ARGS(inode),
+
+	TP_STRUCT__entry(
+		__field(dev_t,		dev)
+		__field(ino_t,		ino)
+		__field(__u16,		mode)
+		__field(loff_t,		size)
+		__field(unsigned int,	nlink)
+		__field(unsigned int,	seals)
+		__field(blkcnt_t,	blocks)
+	),
+
+	TP_fast_assign(
+		__entry->dev		= inode->i_sb->s_dev;
+		__entry->ino		= inode->i_ino;
+		__entry->mode		= inode->i_mode;
+		__entry->size		= inode->i_size;
+		__entry->nlink		= inode->i_nlink;
+		__entry->seals		= HUGETLBFS_I(inode)->seals;
+		__entry->blocks		= inode->i_blocks;
+	),
+
+	TP_printk("dev = (%d,%d), ino = %lu, i_mode = 0%o, i_size = %lld, i_nlink = %u, seals = %u, i_blocks = %llu",
+		MAJOR(__entry->dev), MINOR(__entry->dev), (unsigned long) __entry->ino,
+		__entry->mode, __entry->size, __entry->nlink, __entry->seals,
+		(unsigned long long)__entry->blocks)
+);
+
+DEFINE_EVENT(hugetlbfs__inode, hugetlbfs_evict_inode,
+
+	TP_PROTO(struct inode *inode),
+
+	TP_ARGS(inode)
+);
+
+DEFINE_EVENT(hugetlbfs__inode, hugetlbfs_free_inode,
+
+	TP_PROTO(struct inode *inode),
+
+	TP_ARGS(inode)
+);
+
+TRACE_EVENT(hugetlbfs_setattr,
+
+	TP_PROTO(struct inode *inode,
+		unsigned int d_len, const unsigned char *d_name,
+		unsigned int ia_valid, unsigned int ia_mode,
+		unsigned int ia_uid, unsigned int ia_gid,
+		loff_t old_size, loff_t ia_size),
+
+	TP_ARGS(inode, d_len, d_name,
+		ia_valid, ia_mode, ia_uid, ia_gid, old_size, ia_size),
+
+	TP_STRUCT__entry(
+		__field(dev_t,		dev)
+		__field(ino_t,		ino)
+		__field(unsigned int,	d_len)
+		__string(d_name,	d_name)
+		__field(unsigned int,	ia_valid)
+		__field(unsigned int,	ia_mode)
+		__field(unsigned int,	ia_uid)
+		__field(unsigned int,	ia_gid)
+		__field(loff_t,		old_size)
+		__field(loff_t,		ia_size)
+	),
+
+	TP_fast_assign(
+		__entry->dev		= inode->i_sb->s_dev;
+		__entry->ino		= inode->i_ino;
+		__entry->d_len		= d_len;
+		__assign_str(d_name);
+		__entry->ia_valid	= ia_valid;
+		__entry->ia_mode	= ia_mode;
+		__entry->ia_uid		= ia_uid;
+		__entry->ia_gid		= ia_gid;
+		__entry->old_size	= old_size;
+		__entry->ia_size	= ia_size;
+	),
+
+	TP_printk("dev = (%d,%d), ino = %lu, name = %.*s, ia_valid = %u, ia_mode = 0%o, ia_uid = %u, ia_gid = %u, old_size = %lld, ia_size = %lld",
+		MAJOR(__entry->dev), MINOR(__entry->dev), (unsigned long)__entry->ino,
+		__entry->d_len, __get_str(d_name), __entry->ia_valid, __entry->ia_mode,
+		__entry->ia_uid, __entry->ia_gid, __entry->old_size, __entry->ia_size)
+);
+
+TRACE_EVENT(hugetlbfs_fallocate,
+
+	TP_PROTO(struct inode *inode, int mode,
+		loff_t offset, loff_t len, int ret),
+
+	TP_ARGS(inode, mode, offset, len, ret),
+
+	TP_STRUCT__entry(
+		__field(dev_t,		dev)
+		__field(ino_t,		ino)
+		__field(int,		mode)
+		__field(loff_t,		offset)
+		__field(loff_t,		len)
+		__field(loff_t,		size)
+		__field(int,		ret)
+	),
+
+	TP_fast_assign(
+		__entry->dev		= inode->i_sb->s_dev;
+		__entry->ino		= inode->i_ino;
+		__entry->mode		= mode;
+		__entry->offset		= offset;
+		__entry->len		= len;
+		__entry->size		= inode->i_size;
+		__entry->ret		= ret;
+	),
+
+	TP_printk("dev = (%d,%d), ino = %lu, mode = %x, offset = %lld, len = %lld,  i_size = %lld, ret = %d",
+		MAJOR(__entry->dev), MINOR(__entry->dev),
+		(unsigned long)__entry->ino, __entry->mode,
+		(unsigned long long)__entry->offset,
+		(unsigned long long)__entry->len,
+		(unsigned long long)__entry->size,
+		__entry->ret)
+);
+
+#endif /* _TRACE_HUGETLBFS_H */
+
+ /* This part must be outside protection */
+#include <trace/define_trace.h>
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] hugetlbfs: use tracepoints in hugetlbfs functions.
  2024-06-12  1:11 [PATCH 0/2] Introduce tracepoint for hugetlbfs Hongbo Li
  2024-06-12  1:11 ` [PATCH 1/2] hugetlbfs: support tracepoint Hongbo Li
@ 2024-06-12  1:11 ` Hongbo Li
  2024-06-20 11:50   ` Hongbo Li
  2024-07-01 23:49   ` Steven Rostedt
  1 sibling, 2 replies; 8+ messages in thread
From: Hongbo Li @ 2024-06-12  1:11 UTC (permalink / raw)
  To: muchun.song, rostedt, mhiramat
  Cc: mathieu.desnoyers, linux-mm, linux-trace-kernel, linux-fsdevel,
	lihongbo22

Here we use the hugetlbfs tracepoint to track the call stack. And
the output in trace is as follows:

```
touch-5307    [004] .....  1402.167607: hugetlbfs_alloc_inode: dev = (0,50), ino = 21380, dir = 16921, mode = 0100644
touch-5307    [004] .....  1402.167638: hugetlbfs_setattr: dev = (0,50), ino = 21380, name = testfile1, ia_valid = 131184, ia_mode = 0132434, ia_uid = 2863018275, ia_gid = 4294967295, old_size = 0, ia_size = 4064
truncate-5328    [003] .....  1436.031054: hugetlbfs_setattr: dev = (0,50), ino = 21380, name = testfile1, ia_valid = 8296, ia_mode = 0177777, ia_uid = 2862574544, ia_gid = 4294967295, old_size = 0, ia_size = 2097152
rm-5338    [004] .....  1484.426247: hugetlbfs_evict_inode: dev = (0,50), ino = 21380, i_mode = 0100644, i_size = 2097152, i_nlink = 0, seals = 1, i_blocks = 0
<idle>-0       [004] ..s1.  1484.446668: hugetlbfs_free_inode: dev = (0,50), ino = 21380, i_mode = 0100644, i_size = 2097152, i_nlink = 0, seals = 1, i_blocks = 0
```

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 fs/hugetlbfs/inode.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 412f295acebe..f3399c6a02ca 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -39,6 +39,9 @@
 #include <linux/uaccess.h>
 #include <linux/sched/mm.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/hugetlbfs.h>
+
 static const struct address_space_operations hugetlbfs_aops;
 static const struct file_operations hugetlbfs_file_operations;
 static const struct inode_operations hugetlbfs_dir_inode_operations;
@@ -686,6 +689,7 @@ static void hugetlbfs_evict_inode(struct inode *inode)
 {
 	struct resv_map *resv_map;
 
+	trace_hugetlbfs_evict_inode(inode);
 	remove_inode_hugepages(inode, 0, LLONG_MAX);
 
 	/*
@@ -813,8 +817,10 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
 	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
 		return -EOPNOTSUPP;
 
-	if (mode & FALLOC_FL_PUNCH_HOLE)
-		return hugetlbfs_punch_hole(inode, offset, len);
+	if (mode & FALLOC_FL_PUNCH_HOLE) {
+		error = hugetlbfs_punch_hole(inode, offset, len);
+		goto out_nolock;
+	}
 
 	/*
 	 * Default preallocate case.
@@ -918,6 +924,9 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
 	inode_set_ctime_current(inode);
 out:
 	inode_unlock(inode);
+
+out_nolock:
+	trace_hugetlbfs_fallocate(inode, mode, offset, len, error);
 	return error;
 }
 
@@ -934,6 +943,12 @@ static int hugetlbfs_setattr(struct mnt_idmap *idmap,
 	if (error)
 		return error;
 
+	trace_hugetlbfs_setattr(inode, dentry->d_name.len, dentry->d_name.name,
+			attr->ia_valid, attr->ia_mode,
+			from_kuid(&init_user_ns, attr->ia_uid),
+			from_kgid(&init_user_ns, attr->ia_gid),
+			inode->i_size, attr->ia_size);
+
 	if (ia_valid & ATTR_SIZE) {
 		loff_t oldsize = inode->i_size;
 		loff_t newsize = attr->ia_size;
@@ -1032,6 +1047,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
 			break;
 		}
 		lockdep_annotate_inode_mutex_key(inode);
+		trace_hugetlbfs_alloc_inode(inode, dir, mode);
 	} else {
 		if (resv_map)
 			kref_put(&resv_map->refs, resv_map_release);
@@ -1274,6 +1290,7 @@ static struct inode *hugetlbfs_alloc_inode(struct super_block *sb)
 
 static void hugetlbfs_free_inode(struct inode *inode)
 {
+	trace_hugetlbfs_free_inode(inode);
 	kmem_cache_free(hugetlbfs_inode_cachep, HUGETLBFS_I(inode));
 }
 
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] hugetlbfs: use tracepoints in hugetlbfs functions.
  2024-06-12  1:11 ` [PATCH 2/2] hugetlbfs: use tracepoints in hugetlbfs functions Hongbo Li
@ 2024-06-20 11:50   ` Hongbo Li
  2024-07-01 23:49   ` Steven Rostedt
  1 sibling, 0 replies; 8+ messages in thread
From: Hongbo Li @ 2024-06-20 11:50 UTC (permalink / raw)
  To: muchun.song, rostedt, mhiramat
  Cc: mathieu.desnoyers, linux-mm, linux-trace-kernel, linux-fsdevel

Just a friendly ping to the patch :)

https://lore.kernel.org/all/20240612011156.2891254-1-lihongbo22@huawei.com/

Thanks,
Hongbo

On 2024/6/12 9:11, Hongbo Li wrote:
> Here we use the hugetlbfs tracepoint to track the call stack. And
> the output in trace is as follows:
> 
> ```
> touch-5307    [004] .....  1402.167607: hugetlbfs_alloc_inode: dev = (0,50), ino = 21380, dir = 16921, mode = 0100644
> touch-5307    [004] .....  1402.167638: hugetlbfs_setattr: dev = (0,50), ino = 21380, name = testfile1, ia_valid = 131184, ia_mode = 0132434, ia_uid = 2863018275, ia_gid = 4294967295, old_size = 0, ia_size = 4064
> truncate-5328    [003] .....  1436.031054: hugetlbfs_setattr: dev = (0,50), ino = 21380, name = testfile1, ia_valid = 8296, ia_mode = 0177777, ia_uid = 2862574544, ia_gid = 4294967295, old_size = 0, ia_size = 2097152
> rm-5338    [004] .....  1484.426247: hugetlbfs_evict_inode: dev = (0,50), ino = 21380, i_mode = 0100644, i_size = 2097152, i_nlink = 0, seals = 1, i_blocks = 0
> <idle>-0       [004] ..s1.  1484.446668: hugetlbfs_free_inode: dev = (0,50), ino = 21380, i_mode = 0100644, i_size = 2097152, i_nlink = 0, seals = 1, i_blocks = 0
> ```
> 
> Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
> ---
>   fs/hugetlbfs/inode.c | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 412f295acebe..f3399c6a02ca 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -39,6 +39,9 @@
>   #include <linux/uaccess.h>
>   #include <linux/sched/mm.h>
>   
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/hugetlbfs.h>
> +
>   static const struct address_space_operations hugetlbfs_aops;
>   static const struct file_operations hugetlbfs_file_operations;
>   static const struct inode_operations hugetlbfs_dir_inode_operations;
> @@ -686,6 +689,7 @@ static void hugetlbfs_evict_inode(struct inode *inode)
>   {
>   	struct resv_map *resv_map;
>   
> +	trace_hugetlbfs_evict_inode(inode);
>   	remove_inode_hugepages(inode, 0, LLONG_MAX);
>   
>   	/*
> @@ -813,8 +817,10 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>   	if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
>   		return -EOPNOTSUPP;
>   
> -	if (mode & FALLOC_FL_PUNCH_HOLE)
> -		return hugetlbfs_punch_hole(inode, offset, len);
> +	if (mode & FALLOC_FL_PUNCH_HOLE) {
> +		error = hugetlbfs_punch_hole(inode, offset, len);
> +		goto out_nolock;
> +	}
>   
>   	/*
>   	 * Default preallocate case.
> @@ -918,6 +924,9 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>   	inode_set_ctime_current(inode);
>   out:
>   	inode_unlock(inode);
> +
> +out_nolock:
> +	trace_hugetlbfs_fallocate(inode, mode, offset, len, error);
>   	return error;
>   }
>   
> @@ -934,6 +943,12 @@ static int hugetlbfs_setattr(struct mnt_idmap *idmap,
>   	if (error)
>   		return error;
>   
> +	trace_hugetlbfs_setattr(inode, dentry->d_name.len, dentry->d_name.name,
> +			attr->ia_valid, attr->ia_mode,
> +			from_kuid(&init_user_ns, attr->ia_uid),
> +			from_kgid(&init_user_ns, attr->ia_gid),
> +			inode->i_size, attr->ia_size);
> +
>   	if (ia_valid & ATTR_SIZE) {
>   		loff_t oldsize = inode->i_size;
>   		loff_t newsize = attr->ia_size;
> @@ -1032,6 +1047,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
>   			break;
>   		}
>   		lockdep_annotate_inode_mutex_key(inode);
> +		trace_hugetlbfs_alloc_inode(inode, dir, mode);
>   	} else {
>   		if (resv_map)
>   			kref_put(&resv_map->refs, resv_map_release);
> @@ -1274,6 +1290,7 @@ static struct inode *hugetlbfs_alloc_inode(struct super_block *sb)
>   
>   static void hugetlbfs_free_inode(struct inode *inode)
>   {
> +	trace_hugetlbfs_free_inode(inode);
>   	kmem_cache_free(hugetlbfs_inode_cachep, HUGETLBFS_I(inode));
>   }
>   


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] hugetlbfs: use tracepoints in hugetlbfs functions.
  2024-06-12  1:11 ` [PATCH 2/2] hugetlbfs: use tracepoints in hugetlbfs functions Hongbo Li
  2024-06-20 11:50   ` Hongbo Li
@ 2024-07-01 23:49   ` Steven Rostedt
  2024-07-02 11:55     ` Hongbo Li
  1 sibling, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2024-07-01 23:49 UTC (permalink / raw)
  To: Hongbo Li
  Cc: muchun.song, mhiramat, mathieu.desnoyers, linux-mm,
	linux-trace-kernel, linux-fsdevel

On Wed, 12 Jun 2024 09:11:56 +0800
Hongbo Li <lihongbo22@huawei.com> wrote:

> @@ -934,6 +943,12 @@ static int hugetlbfs_setattr(struct mnt_idmap *idmap,
>  	if (error)
>  		return error;
>  
> +	trace_hugetlbfs_setattr(inode, dentry->d_name.len, dentry->d_name.name,
> +			attr->ia_valid, attr->ia_mode,
> +			from_kuid(&init_user_ns, attr->ia_uid),
> +			from_kgid(&init_user_ns, attr->ia_gid),
> +			inode->i_size, attr->ia_size);
> +

That's a lot of parameters to pass to a tracepoint. Why not just pass the
dentry and attr and do the above in the TP_fast_assign() logic? That would
put less pressure on the icache for the code part.

-- Steve


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] hugetlbfs: use tracepoints in hugetlbfs functions.
  2024-07-01 23:49   ` Steven Rostedt
@ 2024-07-02 11:55     ` Hongbo Li
  2024-07-02 13:30       ` Mathieu Desnoyers
  0 siblings, 1 reply; 8+ messages in thread
From: Hongbo Li @ 2024-07-02 11:55 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: muchun.song, mhiramat, mathieu.desnoyers, linux-mm,
	linux-trace-kernel, linux-fsdevel



On 2024/7/2 7:49, Steven Rostedt wrote:
> On Wed, 12 Jun 2024 09:11:56 +0800
> Hongbo Li <lihongbo22@huawei.com> wrote:
> 
>> @@ -934,6 +943,12 @@ static int hugetlbfs_setattr(struct mnt_idmap *idmap,
>>   	if (error)
>>   		return error;
>>   
>> +	trace_hugetlbfs_setattr(inode, dentry->d_name.len, dentry->d_name.name,
>> +			attr->ia_valid, attr->ia_mode,
>> +			from_kuid(&init_user_ns, attr->ia_uid),
>> +			from_kgid(&init_user_ns, attr->ia_gid),
>> +			inode->i_size, attr->ia_size);
>> +
> 
> That's a lot of parameters to pass to a tracepoint. Why not just pass the
> dentry and attr and do the above in the TP_fast_assign() logic? That would
> put less pressure on the icache for the code part.

Thanks for reviewing!

Some logic such as kuid_t --> uid_t might be reasonable obtained in 
filesystem layer. Passing the dentry and attr will let trace know the 
meaning of structure, perhaps tracepoint should not be aware of the
members of these structures as much as possible.

Thanks,
Hongbo

> 
> -- Steve
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] hugetlbfs: use tracepoints in hugetlbfs functions.
  2024-07-02 11:55     ` Hongbo Li
@ 2024-07-02 13:30       ` Mathieu Desnoyers
  2024-07-02 14:21         ` Hongbo Li
  0 siblings, 1 reply; 8+ messages in thread
From: Mathieu Desnoyers @ 2024-07-02 13:30 UTC (permalink / raw)
  To: Hongbo Li, Steven Rostedt
  Cc: muchun.song, mhiramat, linux-mm, linux-trace-kernel,
	linux-fsdevel

On 2024-07-02 07:55, Hongbo Li wrote:
> 
> 
> On 2024/7/2 7:49, Steven Rostedt wrote:
>> On Wed, 12 Jun 2024 09:11:56 +0800
>> Hongbo Li <lihongbo22@huawei.com> wrote:
>>
>>> @@ -934,6 +943,12 @@ static int hugetlbfs_setattr(struct mnt_idmap 
>>> *idmap,
>>>       if (error)
>>>           return error;
>>> +    trace_hugetlbfs_setattr(inode, dentry->d_name.len, 
>>> dentry->d_name.name,
>>> +            attr->ia_valid, attr->ia_mode,
>>> +            from_kuid(&init_user_ns, attr->ia_uid),
>>> +            from_kgid(&init_user_ns, attr->ia_gid),
>>> +            inode->i_size, attr->ia_size);
>>> +
>>
>> That's a lot of parameters to pass to a tracepoint. Why not just pass the
>> dentry and attr and do the above in the TP_fast_assign() logic? That 
>> would
>> put less pressure on the icache for the code part.
> 
> Thanks for reviewing!
> 
> Some logic such as kuid_t --> uid_t might be reasonable obtained in 
> filesystem layer. Passing the dentry and attr will let trace know the 
> meaning of structure, perhaps tracepoint should not be aware of the
> members of these structures as much as possible.

As maintainer of the LTTng out-of-tree kernel tracer, I appreciate the
effort to decouple instrumentation from the subsystem instrumentation,
but as long as the structure sits in public headers and the global
variables used within the TP_fast_assign() logic (e.g. init_user_ns)
are export-gpl, this is enough to make it easy for tracer integration
and it keeps the tracepoint caller code footprint to a minimum.

The TRACE_EVENT definitions are specific to the subsystem anyway,
so I don't think it matters that the TRACE_EVENT() need to access
the dentry and attr structures.

So I agree with Steven's suggestion. However, just as a precision,
I suspect it will have mainly an impact on code size, but not
necessarily on icache footprint, because it will shrink the code
size within the tracepoint unlikely branch (cold instructions).

Thanks,

Mathieu

> 
> Thanks,
> Hongbo
> 
>>
>> -- Steve
>>

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] hugetlbfs: use tracepoints in hugetlbfs functions.
  2024-07-02 13:30       ` Mathieu Desnoyers
@ 2024-07-02 14:21         ` Hongbo Li
  0 siblings, 0 replies; 8+ messages in thread
From: Hongbo Li @ 2024-07-02 14:21 UTC (permalink / raw)
  To: Mathieu Desnoyers, Steven Rostedt
  Cc: muchun.song, mhiramat, linux-mm, linux-trace-kernel,
	linux-fsdevel



On 2024/7/2 21:30, Mathieu Desnoyers wrote:
> On 2024-07-02 07:55, Hongbo Li wrote:
>>
>>
>> On 2024/7/2 7:49, Steven Rostedt wrote:
>>> On Wed, 12 Jun 2024 09:11:56 +0800
>>> Hongbo Li <lihongbo22@huawei.com> wrote:
>>>
>>>> @@ -934,6 +943,12 @@ static int hugetlbfs_setattr(struct mnt_idmap 
>>>> *idmap,
>>>>       if (error)
>>>>           return error;
>>>> +    trace_hugetlbfs_setattr(inode, dentry->d_name.len, 
>>>> dentry->d_name.name,
>>>> +            attr->ia_valid, attr->ia_mode,
>>>> +            from_kuid(&init_user_ns, attr->ia_uid),
>>>> +            from_kgid(&init_user_ns, attr->ia_gid),
>>>> +            inode->i_size, attr->ia_size);
>>>> +
>>>
>>> That's a lot of parameters to pass to a tracepoint. Why not just pass 
>>> the
>>> dentry and attr and do the above in the TP_fast_assign() logic? That 
>>> would
>>> put less pressure on the icache for the code part.
>>
>> Thanks for reviewing!
>>
>> Some logic such as kuid_t --> uid_t might be reasonable obtained in 
>> filesystem layer. Passing the dentry and attr will let trace know the 
>> meaning of structure, perhaps tracepoint should not be aware of the
>> members of these structures as much as possible.
> 
> As maintainer of the LTTng out-of-tree kernel tracer, I appreciate the
> effort to decouple instrumentation from the subsystem instrumentation,
> but as long as the structure sits in public headers and the global
> variables used within the TP_fast_assign() logic (e.g. init_user_ns)
> are export-gpl, this is enough to make it easy for tracer integration
Thank you for your friendly elaboration and suggestion!
I will update this part based on your suggestion in next version.

Thanks,
Hongbo
> and it keeps the tracepoint caller code footprint to a minimum.
> 
> The TRACE_EVENT definitions are specific to the subsystem anyway,
> so I don't think it matters that the TRACE_EVENT() need to access
> the dentry and attr structures.
> 
> So I agree with Steven's suggestion. However, just as a precision,
> I suspect it will have mainly an impact on code size, but not
> necessarily on icache footprint, because it will shrink the code
> size within the tracepoint unlikely branch (cold instructions).
> 
> Thanks,
> 
> Mathieu
> 
>>
>> Thanks,
>> Hongbo
>>
>>>
>>> -- Steve
>>>
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-07-02 14:21 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-12  1:11 [PATCH 0/2] Introduce tracepoint for hugetlbfs Hongbo Li
2024-06-12  1:11 ` [PATCH 1/2] hugetlbfs: support tracepoint Hongbo Li
2024-06-12  1:11 ` [PATCH 2/2] hugetlbfs: use tracepoints in hugetlbfs functions Hongbo Li
2024-06-20 11:50   ` Hongbo Li
2024-07-01 23:49   ` Steven Rostedt
2024-07-02 11:55     ` Hongbo Li
2024-07-02 13:30       ` Mathieu Desnoyers
2024-07-02 14:21         ` Hongbo Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).