Re: [PATCH] cgroup: Trace event cgroup id fields should be u64

[William Kucharski <william.kucharski@oracle.com>]

No problem, it makes my life easier for V2. :-)

Thanks!!

> On Dec 1, 2021, at 8:45 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Wed,  1 Dec 2021 07:40:43 -0700
> William Kucharski <william.kucharski@oracle.com> wrote:
> 
>> Various trace event fields that store cgroup IDs were declared
>> as ints, but cgroup_id(() returns a u64 and the conversion was
>> not intended.
>> 
>> Also remove extraneous spaces in fields that are no longer proper C style.
> 
> Um, actually, please do not do this!
> 
>> 
>> Fixes: 743210386c03 ("cgroup: use cgrp->kn->id as the cgroup ID")
>> Signed-off-by: William Kucharski <william.kucharski@oracle.com>
>> ---
>> include/trace/events/cgroup.h | 42 +++++++++++++++++------------------
>> 1 file changed, 21 insertions(+), 21 deletions(-)
>> 
>> diff --git a/include/trace/events/cgroup.h b/include/trace/events/cgroup.h
>> index 7f42a3de59e6..a85b01aa1fff 100644
>> --- a/include/trace/events/cgroup.h
>> +++ b/include/trace/events/cgroup.h
>> @@ -15,9 +15,9 @@ DECLARE_EVENT_CLASS(cgroup_root,
>> 	TP_ARGS(root),
>> 
>> 	TP_STRUCT__entry(
>> -		__field(	int,		root			)
>> -		__field(	u16,		ss_mask			)
>> -		__string(	name,		root->name		)
>> +		__field(int, root)
>> +		__field(u16, ss_mask)
>> +		__string(name, root->name)
> 
> The spaces are there for a reason. These are special macros and not C code.
> It's defining a structure, and the spacing is to imitate just that!
> 
> struct __entry {
> 	int		root;
> 	u16		ss_mask;
> 	name		root->name;
> };
> 
> 
> See the difference. You just made it less readable than the original.
> 
> So leave it alone.
> 
> NACK on the style changes.
> 
> -- Steve
> 
> 
>> 	),
>> 
>> 	TP_fast_assign(
>> @@ -58,10 +58,10 @@ DECLARE_EVENT_CLASS(cgroup,
>> 	TP_ARGS(cgrp, path),
>> 
>> 	TP_STRUCT__entry(
>> -		__field(	int,		root			)
>> -		__field(	int,		id			)
>> -		__field(	int,		level			)
>> -		__string(	path,		path			)
>> +		__field(int, root)
>> +		__field(u64, id)
>> +		__field(int, level)
>> +		__string(path, path)
>> 	),
>> 
>> 	TP_fast_assign(
>> @@ -71,7 +71,7 @@ DECLARE_EVENT_CLASS(cgroup,
>> 		__assign_str(path, path);
>> 	),
>> 
>> -	TP_printk("root=%d id=%d level=%d path=%s",
>> +	TP_printk("root=%d id=%llu level=%d path=%s",
>> 		  __entry->root, __entry->id, __entry->level, __get_str(path))
>> );
>> 
>> @@ -125,12 +125,12 @@ DECLARE_EVENT_CLASS(cgroup_migrate,
>> 	TP_ARGS(dst_cgrp, path, task, threadgroup),
>> 
>> 	TP_STRUCT__entry(
>> -		__field(	int,		dst_root		)
>> -		__field(	int,		dst_id			)
>> -		__field(	int,		dst_level		)
>> -		__field(	int,		pid			)
>> -		__string(	dst_path,	path			)
>> -		__string(	comm,		task->comm		)
>> +		__field(int, dst_root)
>> +		__field(u64, dst_id)
>> +		__field(int, dst_level)
>> +		__field(int, pid)
>> +		__string(dst_path, path)
>> +		__string(comm, task->comm)
>> 	),
>> 
>> 	TP_fast_assign(
>> @@ -142,7 +142,7 @@ DECLARE_EVENT_CLASS(cgroup_migrate,
>> 		__assign_str(comm, task->comm);
>> 	),
>> 
>> -	TP_printk("dst_root=%d dst_id=%d dst_level=%d dst_path=%s pid=%d comm=%s",
>> +	TP_printk("dst_root=%d dst_id=%llu dst_level=%d dst_path=%s pid=%d comm=%s",
>> 		  __entry->dst_root, __entry->dst_id, __entry->dst_level,
>> 		  __get_str(dst_path), __entry->pid, __get_str(comm))
>> );
>> @@ -170,11 +170,11 @@ DECLARE_EVENT_CLASS(cgroup_event,
>> 	TP_ARGS(cgrp, path, val),
>> 
>> 	TP_STRUCT__entry(
>> -		__field(	int,		root			)
>> -		__field(	int,		id			)
>> -		__field(	int,		level			)
>> -		__string(	path,		path			)
>> -		__field(	int,		val			)
>> +		__field(int, root)
>> +		__field(u64, id)
>> +		__field(int, level)
>> +		__string(path, path)
>> +		__field(int, val)
>> 	),
>> 
>> 	TP_fast_assign(
>> @@ -185,7 +185,7 @@ DECLARE_EVENT_CLASS(cgroup_event,
>> 		__entry->val = val;
>> 	),
>> 
>> -	TP_printk("root=%d id=%d level=%d path=%s val=%d",
>> +	TP_printk("root=%d id=%llu level=%d path=%s val=%d",
>> 		  __entry->root, __entry->id, __entry->level, __get_str(path),
>> 		  __entry->val)
>> );
>